fetch-pack: derive tunable worker defaults for URI packs

Documentation/config/fetch.adoc

@@ -96,13 +96,21 @@ config setting.
 
 `fetch.packfileUriJobs`::
 	Specifies the maximum number of packfile URI downloads and indexers
-	to run at once. The default is 1, which preserves advertised URI
-	order.
+	to run at once. If unset, defaults to the number of online CPUs,
+	capped at 8. Values of 1 or less preserve advertised URI order.
+	An explicit value greater than 8 is permitted. The number of URI
+	packs in the response also limits the number of jobs.
 +
 Values greater than 1 are used only when the server advertises and the
 client requests the `no-ref-delta` promise. Each URI pack is then checked
 with `index-pack --no-ref-delta` before it is accepted.
 Responses with one URI retain the serial path.
++
+When `pack.threads` is unset or zero, each parallel URI indexer uses the
+number of online CPUs divided by the actual number of jobs, rounded down,
+with a minimum of 1 thread and a maximum of 4. A positive `pack.threads`
+value overrides this per-indexer default, including values greater than 4.
+Increasing both settings can increase CPU and memory use substantially.
 
 `fetch.writeCommitGraph`::
 	Set to true to write a commit-graph after every `git fetch` command

Documentation/config/pack.adoc

@@ -81,6 +81,11 @@ pack.threads::
 	is however multiplied by the number of threads.
 	Specifying 0 will cause Git to auto-detect the number of CPUs
 	and set the number of threads accordingly.
++
+This also sets the number of threads used to resolve deltas in
+linkgit:git-index-pack[1]. Parallel packfile URI indexers share the online
+CPUs when this is unset or zero; see `fetch.packfileUriJobs` for their
+automatic limits. A positive value applies to each indexer independently.
 
 pack.indexVersion::
 	Specify the default pack index version.  Valid values are 1 for

fetch-pack.c

@@ -37,6 +37,7 @@
 #include "mergesort.h"
 #include "prio-queue.h"
 #include "promisor-remote.h"
+#include "thread-utils.h"
 
 static int transfer_unpack_limit = -1;
 static int fetch_unpack_limit = -1;
@@ -49,7 +50,7 @@ static int fetch_fsck_objects = -1;
 static int transfer_fsck_objects = -1;
 static int agent_supported;
 static int server_supports_filtering;
-static int fetch_packfile_uri_jobs = 1;
+static int fetch_packfile_uri_jobs;
 static struct shallow_lock shallow_lock;
 static const char *alternate_shallow_file;
 static struct strbuf fsck_msg_types = STRBUF_INIT;
@@ -1778,10 +1779,23 @@ static void fetch_packfile_uris_parallel(
 	struct packfile_uri_task *tasks;
 	struct pollfd *pollfds;
 	int precreate_keeps = index_pack_args_have_keep(index_pack_args);
+	int threads = 0;
 	size_t next = 0, running = 0;
 
 	if (task_nr > (size_t)fetch_packfile_uri_jobs)
 		task_nr = fetch_packfile_uri_jobs;
+
+	/* Share CPUs across active packs unless the caller chose a thread count. */
+	repo_config_get_int(the_repository, "pack.threads", &threads);
+	if (!threads) {
+		threads = online_cpus() / (int)task_nr;
+		if (threads < 1)
+			threads = 1;
+		else if (threads > 4)
+			threads = 4;
+	}
+	strvec_pushf(index_pack_args, "--threads=%d", threads);
+
 	CALLOC_ARRAY(tasks, task_nr);
 	CALLOC_ARRAY(pollfds, task_nr);
 
@@ -2036,12 +2050,6 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		fetch_packfile_uri_jobs > 1 &&
 		packfile_uris.nr > 1;
 	if (parallel_uri_indexing) {
-		/*
-		 * Bound total indexer threads by the number of URI jobs. The
-		 * URI packs are independent, so each indexer can stay single
-		 * threaded while several indexers run at once.
-		 */
-		strvec_push(&index_pack_args, "--threads=1");
 		fetch_packfile_uris_parallel(&packfile_uris, &index_pack_args,
 					    pack_lockfiles,
 					    &fsck_options.gitmodules_found);
@@ -2166,6 +2174,9 @@ static int fetch_pack_config_cb(const char *var, const char *value,
 
 static void fetch_pack_config(void)
 {
+	fetch_packfile_uri_jobs = online_cpus();
+	if (fetch_packfile_uri_jobs > 8)
+		fetch_packfile_uri_jobs = 8;
 	repo_config_get_int(the_repository, "fetch.unpacklimit", &fetch_unpack_limit);
 	repo_config_get_int(the_repository, "transfer.unpacklimit", &transfer_unpack_limit);
 	repo_config_get_bool(the_repository, "repack.usedeltabaseoffset", &prefer_ofs_delta);

t/t5702-protocol-v2.sh

@@ -1321,6 +1321,7 @@ test_expect_success 'no-ref-delta URI packs are indexed concurrently' '
 	GIT_TEST_SIDEBAND_ALL=1 \
 	git -c protocol.version=2 -c fetch.uriprotocols=http \
 		-c fetch.packfileUriJobs=2 \
+		-c pack.threads=1 \
 		clone "$HTTPD_URL/smart/http_parent" http_child-no-ref &&
 
 	test_grep "> no-ref-delta" no-ref-packet.trace &&
@@ -1332,6 +1333,176 @@ test_expect_success 'no-ref-delta URI packs are indexed concurrently' '
 	test_line_count = 3 no-ref-uri-indexers
 '
 
+test_expect_success 'setup packfile URI worker tuning' '
+	P="$HTTPD_DOCUMENT_ROOT_PATH/http_tuning" &&
+	git init "$P" &&
+	git -C "$P" config uploadpack.allowsidebandall true &&
+	git -C "$P" config uploadpack.allowNoRefDelta true &&
+	for i in $(test_seq 10)
+	do
+		echo "$i" >"$P/blob-$i" &&
+		git -C "$P" add "blob-$i" || return 1
+	done &&
+	git -C "$P" commit -m objects &&
+	for i in $(test_seq 10)
+	do
+		configure_exclusion "$P" "blob-$i" >/dev/null || return 1
+	done &&
+	uri_cpus=$(test-tool online-cpus)
+'
+
+clone_uri_tuning () {
+	local repository="$1" destination="$2" &&
+	shift 2 &&
+	GIT_TRACE2_EVENT="$TRASH_DIRECTORY/$destination.trace" \
+	GIT_TRACE_PACKET="$TRASH_DIRECTORY/$destination.packet" \
+	GIT_TEST_SIDEBAND_ALL=1 \
+	git -c protocol.version=2 -c fetch.uriprotocols=http "$@" \
+		clone "$HTTPD_URL/smart/$repository" "$destination" &&
+	git -C "$destination" fsck --no-reflogs
+}
+
+# Count outstanding URI children in their parent's Trace2 events. Indexers
+# have their own child IDs, so pair each ID with its parent's session ID.
+test_uri_fetch_jobs () {
+	awk -v expected="$2" '
+	function child_key(    fields, sid, id) {
+		split($0, fields, "\"sid\":\"");
+		split(fields[2], sid, "\"");
+		split($0, fields, "\"child_id\":");
+		id = fields[2] + 0;
+		return sid[1] ":" id;
+	}
+	/"event":"child_start".*"http-fetch","--packfile=/ {
+		active[child_key()] = 1;
+		running++;
+		if (running > maximum)
+			maximum = running;
+	}
+	/"event":"child_exit"/ {
+		key = child_key();
+		if (active[key]) {
+			delete active[key];
+			running--;
+		}
+	}
+	END {
+		if (running != 0 || maximum != expected) {
+			printf "URI workers: maximum %d, expected %d, outstanding %d\n",
+				maximum, expected, running;
+			exit 1;
+		}
+	}
+	' "$1"
+}
+
+test_expect_success 'packfile URI defaults bound workers and indexer threads' '
+	jobs=$uri_cpus &&
+	if test "$jobs" -gt 8
+	then
+		jobs=8
+	fi &&
+	clone_uri_tuning http_tuning uri-default &&
+	test_uri_fetch_jobs uri-default.trace "$jobs" &&
+	if test "$jobs" -gt 1
+	then
+		threads=$((uri_cpus / jobs)) &&
+		if test "$threads" -gt 4
+		then
+			threads=4
+		fi &&
+		test_grep "> no-ref-delta" uri-default.packet &&
+		grep "\"event\":\"child_start\".*\"index-pack\".*--no-ref-delta.*--threads=$threads\"" \
+			uri-default.trace >uri-default.indexers &&
+		test_line_count = 10 uri-default.indexers
+	else
+		test_grep ! "> no-ref-delta" uri-default.packet
+	fi
+'
+
+for threads in 1 2 4 8
+do
+	test_expect_success "parallel URI indexers honor pack.threads=$threads" '
+		clone_uri_tuning http_tuning uri-threads-$threads \
+			-c fetch.packfileUriJobs=2 -c pack.threads=$threads &&
+		test_uri_fetch_jobs uri-threads-$threads.trace 2 &&
+		grep "\"event\":\"child_start\".*\"index-pack\".*--no-ref-delta.*--threads=$threads\"" \
+			uri-threads-$threads.trace >uri-indexers &&
+		test_line_count = 10 uri-indexers
+	'
+done
+
+test_expect_success 'automatic URI threads use the actual worker count' '
+	# More requested jobs than URI packs must not dilute the CPU budget.
+	threads=$((uri_cpus / 3)) &&
+	if test "$threads" -lt 1
+	then
+		threads=1
+	elif test "$threads" -gt 4
+	then
+		threads=4
+	fi &&
+	clone_uri_tuning http_parent uri-auto-threads \
+		-c fetch.packfileUriJobs=100 -c pack.threads=0 &&
+	test_uri_fetch_jobs uri-auto-threads.trace 3 &&
+	grep "\"event\":\"child_start\".*\"index-pack\".*--no-ref-delta.*--threads=$threads\"" \
+		uri-auto-threads.trace >uri-indexers &&
+	test_line_count = 3 uri-indexers
+'
+
+test_expect_success 'explicit URI jobs can exceed the automatic limit' '
+	clone_uri_tuning http_tuning uri-many-jobs \
+		-c fetch.packfileUriJobs=100 -c pack.threads=1 &&
+	test_uri_fetch_jobs uri-many-jobs.trace 10
+'
+
+test_expect_success 'fetch honors packfile URI worker tuning' '
+	git init uri-fetch &&
+	GIT_TRACE2_EVENT="$TRASH_DIRECTORY/uri-fetch.trace" \
+	GIT_TEST_SIDEBAND_ALL=1 \
+	git -C uri-fetch -c protocol.version=2 -c fetch.uriprotocols=http \
+		-c fetch.packfileUriJobs=2 -c pack.threads=2 \
+		fetch "$HTTPD_URL/smart/http_tuning" &&
+	test_uri_fetch_jobs uri-fetch.trace 2 &&
+	grep "\"event\":\"child_start\".*\"index-pack\".*--no-ref-delta.*--threads=2\"" \
+		uri-fetch.trace >uri-indexers &&
+	test_line_count = 10 uri-indexers &&
+	git -C uri-fetch fsck --no-reflogs
+'
+
+for jobs in 0 1 -1
+do
+	test_expect_success "fetch.packfileUriJobs=$jobs retains serial indexing" '
+		clone_uri_tuning http_tuning uri-serial-$jobs \
+			-c fetch.packfileUriJobs=$jobs -c pack.threads=2 &&
+		test_uri_fetch_jobs uri-serial-$jobs.trace 1 &&
+		test_grep ! "> no-ref-delta" uri-serial-$jobs.packet &&
+		test_grep ! -e "--threads=" uri-serial-$jobs.trace
+	'
+done
+
+test_expect_success 'parallel URI defaults require the server promise' '
+	test_config -C "$P" uploadpack.allowNoRefDelta false &&
+	clone_uri_tuning http_tuning uri-no-promise &&
+	test_uri_fetch_jobs uri-no-promise.trace 1 &&
+	test_grep ! "> no-ref-delta" uri-no-promise.packet &&
+	test_grep ! -e "--threads=" uri-no-promise.trace
+'
+
+test_expect_success 'single URI retains ordinary indexer thread selection' '
+	P="$HTTPD_DOCUMENT_ROOT_PATH/http_single_uri" &&
+	git init "$P" &&
+	git -C "$P" config uploadpack.allowsidebandall true &&
+	git -C "$P" config uploadpack.allowNoRefDelta true &&
+	test_commit -C "$P" one &&
+	configure_exclusion "$P" one.t >/dev/null &&
+	clone_uri_tuning http_single_uri uri-single \
+		-c fetch.packfileUriJobs=8 -c pack.threads=2 &&
+	test_uri_fetch_jobs uri-single.trace 1 &&
+	test_grep "> no-ref-delta" uri-single.packet &&
+	test_grep ! -e "--threads=" uri-single.trace
+'
+
 test_expect_success 'packfile URIs with fetch instead of clone' '
 	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
 	rm -rf "$P" http_child log &&