Merge branch 'ps/fetch-packfile-uris-parallel' into jch

The `git fetch` and `git clone` commands have been optimized to
download packfile URIs in parallel when the new
`fetch.packfileURIThreads` configuration is set, significantly
speeding up fetches from servers that advertise multiple packfiles.

* ps/fetch-packfile-uris-parallel:
  fetch-pack: allow parallelizing packfile URI fetches
  fetch-pack: prepare for threaded fetching of packfile URIs

Documentation/config/fetch.adoc

@@ -94,6 +94,15 @@ A value of 0 will give some reasonable default. If unset, it defaults to 1.
 For submodules, this setting can be overridden using the `submodule.fetchJobs`
 config setting.
 
+`fetch.packfileURIThreads`::
+	Specifies the number of threads used to download packfiles
+	advertised by the server via the `packfile-uris` capability in
+	parallel. Each packfile is downloaded via a separate
+	linkgit:git-http-fetch[1] process.
++
+A value of 0 will use a reasonable default based on the number of available
+CPUs. If unset, it defaults to 1, downloading packfiles sequentially.
+
 `fetch.writeCommitGraph`::
 	Set to true to write a commit-graph after every `git fetch` command
 	that downloads a pack-file from a remote. Using the `--split` option,

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;
@@ -53,6 +54,7 @@ static struct shallow_lock shallow_lock;
 static const char *alternate_shallow_file;
 static struct strbuf fsck_msg_types = STRBUF_INIT;
 static struct string_list uri_protocols = STRING_LIST_INIT_DUP;
+static unsigned int packfile_uri_threads = 1;
 
 /* Remember to update object flag allocation in object.h */
 #define COMPLETE	(1U << 0)
@@ -1668,6 +1670,167 @@ static void do_check_stateless_delimiter(int stateless_rpc,
 				  _("git fetch-pack: expected response end packet"));
 }
 
+struct fetch_packfile_uri_result {
+	struct oidset gitmodules_found;
+	char packhash[GIT_MAX_HEXSZ + 1];
+	bool created_keep;
+};
+
+static void fetch_packfile_uri(const char *uri_with_hash,
+			       const struct strvec *index_pack_args,
+			       struct fetch_packfile_uri_result *result)
+{
+	struct child_process cmd = CHILD_PROCESS_INIT;
+	const char *uri = uri_with_hash +
+		the_hash_algo->hexsz + 1;
+
+	strvec_push(&cmd.args, "http-fetch");
+	strvec_pushf(&cmd.args, "--packfile=%.*s",
+		     (int) the_hash_algo->hexsz, uri_with_hash);
+	for (size_t j = 0; j < index_pack_args->nr; j++)
+		strvec_pushf(&cmd.args, "--index-pack-arg=%s",
+			     index_pack_args->v[j]);
+	strvec_push(&cmd.args, uri);
+	cmd.git_cmd = 1;
+	cmd.no_stdin = 1;
+	cmd.out = -1;
+
+	/*
+	 * Multiple threads may spawn and reap children concurrently in here.
+	 * This is safe because the child-cleanup bookkeeping in run-command.c,
+	 * which is not thread-safe, is only ever used when `clean_on_exit` is
+	 * set.
+	 */
+	if (start_command(&cmd))
+		die("fetch-pack: unable to spawn http-fetch");
+
+	if (read_in_full(cmd.out, result->packhash, 5) != 5 ||
+	    (memcmp(result->packhash, "keep\t", 5) &&
+	     memcmp(result->packhash, "pack\t", 5)))
+		die("fetch-pack: expected pack or keep then TAB at start of http-fetch output");
+	result->created_keep = !memcmp(result->packhash, "keep\t", 5);
+
+	if (read_in_full(cmd.out, result->packhash,
+			 the_hash_algo->hexsz + 1) != the_hash_algo->hexsz + 1 ||
+	    result->packhash[the_hash_algo->hexsz] != '\n')
+		die("fetch-pack: expected hash then LF in http-fetch output");
+	result->packhash[the_hash_algo->hexsz] = '\0';
+
+	parse_gitmodules_oids(cmd.out, &result->gitmodules_found);
+
+	close(cmd.out);
+
+	if (finish_command(&cmd))
+		die("fetch-pack: unable to finish http-fetch");
+
+	if (memcmp(uri_with_hash, result->packhash, the_hash_algo->hexsz))
+		die("fetch-pack: pack downloaded from %s does not match expected hash %.*s",
+		    uri, (int) the_hash_algo->hexsz,
+		    uri_with_hash);
+}
+
+struct fetch_packfile_uris_state {
+	const struct string_list *packfile_uris;
+	const struct strvec *index_pack_args;
+	struct fetch_packfile_uri_result *results;
+	size_t next;
+	pthread_mutex_t lock;
+};
+
+static void *fetch_packfile_uris_thread(void *data)
+{
+	struct fetch_packfile_uris_state *state = data;
+
+	trace2_thread_start("fetch_packfile_uri");
+
+	for (;;) {
+		size_t i;
+
+		pthread_mutex_lock(&state->lock);
+		i = state->next++;
+		pthread_mutex_unlock(&state->lock);
+		if (i >= state->packfile_uris->nr)
+			break;
+
+		fetch_packfile_uri(state->packfile_uris->items[i].string,
+				   state->index_pack_args,
+				   &state->results[i]);
+	}
+
+	trace2_thread_exit();
+
+	return NULL;
+}
+
+static void fetch_packfile_uris(const struct string_list *packfile_uris,
+				const struct strvec *index_pack_args,
+				struct oidset *gitmodules_found,
+				struct string_list *pack_lockfiles)
+{
+	unsigned int nr_threads = packfile_uri_threads;
+	struct fetch_packfile_uri_result *results;
+
+	if (!nr_threads)
+		nr_threads = online_cpus();
+	if (nr_threads > packfile_uris->nr)
+		nr_threads = packfile_uris->nr;
+
+	/* Initialize the data. */
+	CALLOC_ARRAY(results, packfile_uris->nr);
+	for (size_t i = 0; i < packfile_uris->nr; i++)
+		oidset_init(&results[i].gitmodules_found, 0);
+
+	/* Perform the fetches. */
+	if (nr_threads > 1) {
+		struct fetch_packfile_uris_state state = {
+			.packfile_uris = packfile_uris,
+			.index_pack_args = index_pack_args,
+			.results = results,
+		};
+		pthread_t *threads;
+
+		pthread_mutex_init(&state.lock, NULL);
+		ALLOC_ARRAY(threads, nr_threads);
+
+		for (size_t i = 0; i < nr_threads; i++)
+			if (pthread_create(&threads[i], NULL,
+					   fetch_packfile_uris_thread, &state))
+				die(_("failed to create thread"));
+		for (size_t i = 0; i < nr_threads; i++)
+			if (pthread_join(threads[i], NULL))
+				die(_("failed to join thread"));
+
+		pthread_mutex_destroy(&state.lock);
+		free(threads);
+	} else {
+		for (size_t i = 0; i < packfile_uris->nr; i++)
+			fetch_packfile_uri(packfile_uris->items[i].string,
+					   index_pack_args, &results[i]);
+	}
+
+	/* Aggregate results. */
+	for (size_t i = 0; i < packfile_uris->nr; i++) {
+		struct fetch_packfile_uri_result *result = &results[i];
+		const struct object_id *oid;
+		struct oidset_iter iter;
+
+		if (result->created_keep) {
+			char *lockfile = xstrfmt("%s/pack/pack-%s.keep",
+						 repo_get_object_directory(the_repository),
+						 result->packhash);
+			string_list_append_nodup(pack_lockfiles, lockfile);
+		}
+
+		oidset_iter_init(&result->gitmodules_found, &iter);
+		while ((oid = oidset_iter_next(&iter)))
+			oidset_insert(gitmodules_found, oid);
+
+		oidset_clear(&result->gitmodules_found);
+	}
+
+	free(results);
+}
+
 static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 				    int fd[2],
 				    const struct ref *orig_ref,
@@ -1692,7 +1855,6 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 	struct object_id common_oid;
 	int received_ready = 0;
 	struct string_list packfile_uris = STRING_LIST_INIT_DUP;
-	int i;
 	struct strvec index_pack_args = STRVEC_INIT;
 	const char *promisor_remote_config;
 
@@ -1853,59 +2015,8 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
 		}
 	}
 
-	for (i = 0; i < packfile_uris.nr; i++) {
+	fetch_packfile_uris(&packfile_uris, &index_pack_args,
-		bool created_keep;
+			    &fsck_options.gitmodules_found, pack_lockfiles);
-		int j;
-		struct child_process cmd = CHILD_PROCESS_INIT;
-		char packhash[GIT_MAX_HEXSZ + 1];
-		const char *uri = packfile_uris.items[i].string +
-			the_hash_algo->hexsz + 1;
-
-		strvec_push(&cmd.args, "http-fetch");
-		strvec_pushf(&cmd.args, "--packfile=%.*s",
-			     (int) the_hash_algo->hexsz,
-			     packfile_uris.items[i].string);
-		for (j = 0; j < index_pack_args.nr; j++)
-			strvec_pushf(&cmd.args, "--index-pack-arg=%s",
-				     index_pack_args.v[j]);
-		strvec_push(&cmd.args, uri);
-		cmd.git_cmd = 1;
-		cmd.no_stdin = 1;
-		cmd.out = -1;
-		if (start_command(&cmd))
-			die("fetch-pack: unable to spawn http-fetch");
-
-		if (read_in_full(cmd.out, packhash, 5) != 5 ||
-		    (memcmp(packhash, "keep\t", 5) &&
-		     memcmp(packhash, "pack\t", 5)))
-			die("fetch-pack: expected pack or keep then TAB at start of http-fetch output");
-		created_keep = !memcmp(packhash, "keep\t", 5);
-
-		if (read_in_full(cmd.out, packhash,
-				 the_hash_algo->hexsz + 1) != the_hash_algo->hexsz + 1 ||
-		    packhash[the_hash_algo->hexsz] != '\n')
-			die("fetch-pack: expected hash then LF in http-fetch output");
-		packhash[the_hash_algo->hexsz] = '\0';
-
-		parse_gitmodules_oids(cmd.out, &fsck_options.gitmodules_found);
-
-		close(cmd.out);
-
-		if (finish_command(&cmd))
-			die("fetch-pack: unable to finish http-fetch");
-
-		if (memcmp(packfile_uris.items[i].string, packhash,
-			   the_hash_algo->hexsz))
-			die("fetch-pack: pack downloaded from %s does not match expected hash %.*s",
-			    uri, (int) the_hash_algo->hexsz,
-			    packfile_uris.items[i].string);
-
-		if (created_keep)
-			string_list_append_nodup(pack_lockfiles,
-						 xstrfmt("%s/pack/pack-%s.keep",
-							 repo_get_object_directory(the_repository),
-							 packhash));
-	}
 	string_list_clear(&packfile_uris, 0);
 	strvec_clear(&index_pack_args);
 
@@ -1978,6 +2089,15 @@ static void fetch_pack_config(void)
 		}
 	}
 
+	if (!repo_config_get_uint(the_repository, "fetch.packfileurithreads",
+				  &packfile_uri_threads)) {
+		if (!HAVE_THREADS && packfile_uri_threads != 1) {
+			warning(_("no threads support, ignoring %s"),
+				"fetch.packfileURIThreads");
+			packfile_uri_threads = 1;
+		}
+	}
+
 	repo_config(the_repository, fetch_pack_config_cb, NULL);
 }
 

t/t5702-protocol-v2.sh

@@ -1270,6 +1270,50 @@ test_expect_success 'part of packfile response provided as URI' '
 	test_line_count = 6 filelist
 '
 
+test_expect_success 'packfile URIs are downloaded in parallel' '
+	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
+	rm -rf "$P" http_child log trace2.txt &&
+
+	git init "$P" &&
+	git -C "$P" config "uploadpack.allowsidebandall" "true" &&
+
+	for i in one two three
+	do
+		echo blob-$i >"$P"/blob-$i &&
+		git -C "$P" add blob-$i &&
+		configure_exclusion "$P" blob-$i >h-$i || return 1
+	done &&
+	git -C "$P" commit -m message &&
+
+	GIT_TRACE2_EVENT="$(pwd)/trace2.txt" GIT_TEST_SIDEBAND_ALL=1 git \
+		-c protocol.version=2 \
+		-c fetch.uriprotocols=http,https \
+		-c fetch.packfileurithreads=2 \
+		clone --quiet "$HTTPD_URL/smart/http_parent" http_child 2>err &&
+
+	# Ensure that all objects were found.
+	for i in one two three
+	do
+		git -C http_child cat-file -e "$(cat h-$i)" || return 1
+	done &&
+
+	# Ensure that there are exactly 4 packfiles with associated .idx.
+	ls http_child/.git/objects/pack/*.pack \
+	    http_child/.git/objects/pack/*.idx >filelist &&
+	test_line_count = 8 filelist &&
+
+	if test_have_prereq PTHREADS
+	then
+		# Ensure that exactly two worker threads were spawned.
+		git grep --no-index --only-matching "\"thread\":\"th[0-9]*:fetch_packfile_uri\"" trace2.txt >threads &&
+		sort -u <threads >threads.unique &&
+		test_line_count = 2 threads.unique &&
+		test_grep ! "warning: no threads support, ignoring fetch.packfileURIThreads" err
+	else
+		test_grep "warning: no threads support, ignoring fetch.packfileURIThreads" err
+	fi
+'
+
 test_expect_success 'packfile URIs with fetch instead of clone' '
 	P="$HTTPD_DOCUMENT_ROOT_PATH/http_parent" &&
 	rm -rf "$P" http_child log &&