threads / patch / 51633

v2, 5 partsfetch: Extend --jobs to multiple remotes

Subject: [PATCH v2 0/5] fetch: Extend --jobs to multiple remotes

## tl;dr

14 messages between Aug 12, 2019 and Aug 14, 2019. Diffs are folded; open one to read it.

replies: 13people: 5as markdown or json

Palmer Dabbelt· Aug 12, 2019, 21:34 UTC · lore

It seems like the feedback for v1 was fairly positive, so I managed to find some time to go ahead finish it. The result is a much cleaner patch set. I think this could be merged in its current state, but there are a few outstanding concerns I have:

* fetch.jobs isn't documented because I couldn't find any
  documentation for submodule.fetchjobs so I didn't know where to start
  writing.
* I took the complicated approach and added --submodule-fetch-jobs and
  --fetch-jobs before converting --jobs over to set both.  At the time I
  thought it wouldn't add too much extra complexity.  I wasn't looking
  closely enough and ended up with a custom parsing function which is a
  bit ugly.

I'm happy to fix either of these, but I wanted to send out the v2 before going any farther because I wasn't sure if --jobs would be converted over right away or if there was going to be a deprecation period. I've written the patch set such that the final patch can easily be dropped to avoid changing existing behavior.

I'm also happy to re-spin this to just make --jobs control remotes and drop the --fetch-jobs and --submodule-fetch-jobs arguments, which would make the patch set somewhat simpler but preclude a deprecation period.

Changes since v1 <20190717015903.4384-1-palmer@sifive.com>:
* fetch_multiple() has been rewritten to use
  run_processes_parallel_tr2() rather than manage children on its own.
* The --fetch-jobs argument has been added.
* The --jobs arugemnt has been used, instead of the --parallel argument.
Palmer Dabbelt· Aug 12, 2019, 21:34 UTC · re: Palmer Dabbelt · lore

[PATCH v2 1/5] fetch: Rename max_children to max_children_for_submodules

This does not change any functionality, but instead just prepares for the upcoming "--fetch-jobs=<n>" support. Essentially the "max_children" variable is ambiguously named, which would complicate the diff in squashed into the "--fetch-jobs=<n>" patch.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 builtin/fetch.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
Show changes to builtin/fetch.c +6 −5
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 717dd14e8961..8aa6a0caf1ab 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -54,7 +54,7 @@ static int all, append, dry_run, force, keep, multiple, update_head_ok, verbosit
 static int progress = -1;
 static int enable_auto_gc = 1;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
-static int max_children = 1;
+static int max_children_for_submodules = 1;
 static enum transport_family family;
 static const char *depth;
 static const char *deepen_since;
@@ -96,7 +96,7 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 	}
 
 	if (!strcmp(k, "submodule.fetchjobs")) {
-		max_children = parse_submodule_fetchjobs(k, v);
+		max_children_for_submodules = parse_submodule_fetchjobs(k, v);
 		return 0;
 	} else if (!strcmp(k, "fetch.recursesubmodules")) {
 		recurse_submodules = parse_fetch_recurse_submodules_arg(k, v);
@@ -134,7 +134,7 @@ static struct option builtin_fetch_options[] = {
 		    N_("fetch all tags and associated objects"), TAGS_SET),
 	OPT_SET_INT('n', NULL, &tags,
 		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
-	OPT_INTEGER('j', "jobs", &max_children,
+	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
 		    N_("number of submodules fetched in parallel")),
 	OPT_BOOL('p', "prune", &prune,
 		 N_("prune remote-tracking branches no longer on remote")),
@@ -1633,7 +1633,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	for (i = 1; i < argc; i++)
 		strbuf_addf(&default_rla, " %s", argv[i]);
 
-	fetch_config_from_gitmodules(&max_children, &recurse_submodules);
+	fetch_config_from_gitmodules(&max_children_for_submodules,
+				     &recurse_submodules);
 	git_config(git_fetch_config, NULL);
 
 	argc = parse_options(argc, argv, prefix,
@@ -1716,7 +1717,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 						    recurse_submodules,
 						    recurse_submodules_default,
 						    verbosity < 0,
-						    max_children);
+						    max_children_for_submodules);
 		argv_array_clear(&options);
 	}
 
-- 
2.21.0
Palmer Dabbelt· Aug 12, 2019, 21:34 UTC · re: Palmer Dabbelt · lore

[PATCH v2 2/5] fetch: Add the "--fetch-jobs" option

This argument allows users to select the number of jobs that will run in parallel when fetching, with each job fetching from a different remote. This is particularly useful when fetching from many remotes that change slowly on a high latency link, as the fetch time is dominated by handshake latency -- probably not the most common use case, but one that ends up frusturating me with some frequency.

This patch implements --fetch-jobs via run_processes_parallel_tr2(), which allows the user to restrict the amount of parallelism. While this adds a lot of boilerplate to the implementation, the pattern is fairly straight-forward and matches the existing submodule parallel fetch code. The submodule version of this is a lot more complicated and does things like retrying and scheduling I/O, but I don't get bit by those issues so it didn't seem worth the effort.

The speedup is quite noticable for me, even when I'm on a fast link -- my Linux fetches go from 6 seconds to 2 seconds, for example.

I've given this a quick test locally, but haven't done anything exhaustive like attempting to fetch against broken remotes (where the intent is that the other remotes are fetched and the result is an error).

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 Documentation/fetch-options.txt |   5 ++
 builtin/fetch.c                 | 141 +++++++++++++++++++++++++++-----
 2 files changed, 126 insertions(+), 20 deletions(-)
Show changes to 2 files +126 −20

Documentation/fetch-options.txt, builtin/fetch.c

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 3c9b4f9e0951..dbd2add686dd 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -165,6 +165,11 @@ ifndef::git-pull[]
 	submodules will be faster. By default submodules will be fetched
 	one at a time.
 
+--fetch-jobs=<n>::
+	Number of parallel children to be used for fetching.  Each will fetch
+	from different remotes, such that fetching from many remotes will be
+	faster.  By default remotes will be fetched from one at a time.
+
 --no-recurse-submodules::
 	Disable recursive fetching of submodules (this has the same effect as
 	using the `--recurse-submodules=no` option).
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 8aa6a0caf1ab..fa12ad44e7d9 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -55,6 +55,7 @@ static int progress = -1;
 static int enable_auto_gc = 1;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow, deepen;
 static int max_children_for_submodules = 1;
+static int max_children_for_fetch = 1;
 static enum transport_family family;
 static const char *depth;
 static const char *deepen_since;
@@ -136,6 +137,8 @@ static struct option builtin_fetch_options[] = {
 		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
 	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
 		    N_("number of submodules fetched in parallel")),
+	OPT_INTEGER(0, "fetch-jobs", &max_children_for_fetch,
+		    N_("number of remotes fetched in parallel")),
 	OPT_BOOL('p', "prune", &prune,
 		 N_("prune remote-tracking branches no longer on remote")),
 	OPT_BOOL('P', "prune-tags", &prune_tags,
@@ -1463,10 +1466,100 @@ static void add_options_to_argv(struct argv_array *argv)
 
 }
 
-static int fetch_multiple(struct string_list *list)
+/*
+ * Support for fetching multiple remotes in parallel.  This is spread over
+ * multiple functions and structures, all of which are helpers for
+ * fench_multiple().  The general idea is that there is a single struct
+ * fetch_remote, which contains the entire state for a fetch_multiple()
+ * instance.
+ */
+struct fetch_remote_task {
+	int in_use;
+	struct argv_array argv;
+};
+
+struct fetch_remote {
+	struct string_list *all_remotes;
+	int next_remote_index;
+	struct fetch_remote_task *all_tasks;
+	int task_count;
+	int result;
+};
+
+static int next_remote_to_fetch(struct child_process *cp,
+				struct strbuf *out,
+				void *state_uncast,
+				void **task_state_out)
+{
+	int i;
+	struct fetch_remote *state = state_uncast;
+	struct fetch_remote_task *task_state = NULL;
+	const char *remote_name;
+
+	if (state->next_remote_index >= state->all_remotes->nr)
+		return 0;
+
+	remote_name = state->all_remotes->items[state->next_remote_index].string;
+	state->next_remote_index++;
+
+	/*
+	 * Finds somewhere to store the state for a task.  This is guarnteed to
+	 * succeed because there are always enough tasks allocated to cover the
+	 * number that have been requested to run in parallel.  Rather than
+	 * bothering with some sort of free list, this just brute force
+	 * searches for a free task.  The assumption is that there aren't that
+	 * many tasks to look through.
+	 */
+	for (i = 0; i < state->task_count; ++i) {
+		if (!state->all_tasks[i].in_use) {
+			task_state = state->all_tasks + i;
+			break;
+		}
+	}
+	assert(task_state != NULL);
+	task_state->in_use = 1;
+
+	*task_state_out = task_state;
+	argv_array_push(&task_state->argv, remote_name);
+	cp->argv = task_state->argv.argv;
+	cp->git_cmd = 1;
+
+	printf(_("Fetching %s\n"), remote_name);
+
+	return 1;
+}
+
+static int remote_fetch_failed_to_start(struct strbuf *out,
+					void *state_uncast,
+					void *task_state_uncast)
+{
+	struct fetch_remote *state = state_uncast;
+	struct fetch_remote_task *task_state = task_state_uncast;
+	assert(task_state != NULL);
+	argv_array_pop(&task_state->argv);
+	task_state->in_use = 0;
+	state->result |= -1;
+	return 0;
+}
+
+static int remote_fetch_finished(int result,
+				 struct strbuf *out,
+				 void *state_uncast,
+				 void *task_state_uncast)
+{
+	struct fetch_remote *state = state_uncast;
+	struct fetch_remote_task *task_state = task_state_uncast;
+	assert(task_state != NULL);
+	argv_array_pop(&task_state->argv);
+	task_state->in_use = 0;
+	state->result |= result;
+	return 0;
+}
+
+static int fetch_multiple(struct string_list *all_remotes, int max_children)
 {
 	int i, result = 0;
-	struct argv_array argv = ARGV_ARRAY_INIT;
+	struct fetch_remote state;
 
 	if (!append && !dry_run) {
 		int errcode = truncate_fetch_head();
@@ -1474,23 +1567,31 @@ static int fetch_multiple(struct string_list *list)
 			return errcode;
 	}
 
-	argv_array_pushl(&argv, "fetch", "--append", "--no-auto-gc", NULL);
-	add_options_to_argv(&argv);
-
-	for (i = 0; i < list->nr; i++) {
-		const char *name = list->items[i].string;
-		argv_array_push(&argv, name);
-		if (verbosity >= 0)
-			printf(_("Fetching %s\n"), name);
-		if (run_command_v_opt(argv.argv, RUN_GIT_CMD)) {
-			error(_("Could not fetch %s"), name);
-			result = 1;
-		}
-		argv_array_pop(&argv);
-	}
-
-	argv_array_clear(&argv);
-	return result;
+	state.all_remotes = all_remotes;
+	state.next_remote_index = 0;
+	state.all_tasks = xcalloc(sizeof(*state.all_tasks), max_children);
+	state.task_count = max_children;
+	state.result = 0;
+
+	for (i = 0; i < max_children; ++i) {
+		struct argv_array *argv = &state.all_tasks[i].argv;
+		state.all_tasks[i].in_use = 0;
+		argv_array_init(argv);
+		argv_array_pushl(argv, "fetch", "--append", "--no-auto-gc", NULL);
+		add_options_to_argv(argv);
+	};
+
+	result = run_processes_parallel_tr2(max_children,
+					    &next_remote_to_fetch,
+					    &remote_fetch_failed_to_start,
+					    &remote_fetch_finished,
+					    &state,
+					    "fetch", "parallel");
+
+	for (i = 0; i < max_children; ++i)
+		argv_array_clear(&state.all_tasks[i].argv);
+	free(state.all_tasks);
+	return result | state.result;
 }
 
 /*
@@ -1704,7 +1805,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			die(_("--filter can only be used with the remote "
 			      "configured in extensions.partialclone"));
 		/* TODO should this also die if we have a previous partial-clone? */
-		result = fetch_multiple(&list);
+		result = fetch_multiple(&list, max_children_for_fetch);
 	}
 
 	if (!result && (recurse_submodules != RECURSE_SUBMODULES_OFF)) {
-- 
2.21.0
Eric Wong· Aug 13, 2019, 14:44 UTC · re: Palmer Dabbelt · lore

Re: [PATCH v2 2/5] fetch: Add the "--fetch-jobs" option

Palmer Dabbelt <palmer@sifive.com> wrote:
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 8aa6a0caf1ab..fa12ad44e7d9 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
<snip>
Show 30 quoted lines
> +static int next_remote_to_fetch(struct child_process *cp,
> +				struct strbuf *out,
> +				void *state_uncast,
> +				void **task_state_out)
> +{
> +	int i;
> +	struct fetch_remote *state = state_uncast;
> +	struct fetch_remote_task *task_state = NULL;
> +	const char *remote_name;
> +
> +	if (state->next_remote_index >= state->all_remotes->nr)
> +		return 0;
> +
> +	remote_name = state->all_remotes->items[state->next_remote_index].string;
> +	state->next_remote_index++;
> +
> +	/*
> +	 * Finds somewhere to store the state for a task.  This is guarnteed to
> +	 * succeed because there are always enough tasks allocated to cover the
> +	 * number that have been requested to run in parallel.  Rather than
> +	 * bothering with some sort of free list, this just brute force
> +	 * searches for a free task.  The assumption is that there aren't that
> +	 * many tasks to look through.
> +	 */
> +	for (i = 0; i < state->task_count; ++i) {
> +		if (!state->all_tasks[i].in_use) {
> +			task_state = state->all_tasks + i;
> +			break;
> +		}
> +	}

Fwiw, I added list.h, the linked-list derived from the Linux kernel to simplify usage of free lists, queues, etc...

I think it could improve readability, too; but I'm not really a C programmer and prefer high-level scripting languages.

Palmer Dabbelt· Aug 12, 2019, 21:34 UTC · re: Palmer Dabbelt · lore

[PATCH v2 3/5] fetch: Add the fetch.jobs config key

This allows users to default to parallel fetches, so they don't have to pass --fetch-jobs=N on the command line every time. The implementation matches submodule.fetchjobs by die()ing on invalid job counts.

I couldn't find any documentation for submodule.fetchjobs, so I didn't write any for this one. I've tested this in my config and in conjunction with the command-line argument.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 builtin/fetch.c | 7 +++++++
 1 file changed, 7 insertions(+)
Show changes to builtin/fetch.c +7 −0
diff --git a/builtin/fetch.c b/builtin/fetch.c
index fa12ad44e7d9..4c5f2ea3a931 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -104,6 +104,13 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 
+	if (!strcmp(k, "fetch.jobs")) {
+		max_children_for_fetch = git_config_int(k, v);
+		if (max_children_for_fetch < 0)
+			die(_("negative values not allowed for fetch.jobs"));
+		return 0;
+	}
+
 	return git_default_config(k, v, cb);
 }
 
-- 
2.21.0
Palmer Dabbelt· Aug 12, 2019, 21:34 UTC · re: Palmer Dabbelt · lore

[PATCH v2 4/5] fetch: Add the --submodule-fetch-jobs option

This is exactly the same as --jobs, but the more explicit name will allow us to later change the behavior of --jobs to control both the number of jobs for remotes as well as submodules.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 Documentation/fetch-options.txt | 1 +
 builtin/fetch.c                 | 2 ++
 2 files changed, 3 insertions(+)
Show changes to 2 files +3 −0

Documentation/fetch-options.txt, builtin/fetch.c

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index dbd2add686dd..5836024f1934 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -160,6 +160,7 @@ ifndef::git-pull[]
 
 -j::
 --jobs=<n>::
+--submodule-fetch-jobs=<n>::
 	Number of parallel children to be used for fetching submodules.
 	Each will fetch from different submodules, such that fetching many
 	submodules will be faster. By default submodules will be fetched
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 4c5f2ea3a931..67d001f3f78b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -146,6 +146,8 @@ static struct option builtin_fetch_options[] = {
 		    N_("number of submodules fetched in parallel")),
 	OPT_INTEGER(0, "fetch-jobs", &max_children_for_fetch,
 		    N_("number of remotes fetched in parallel")),
+	OPT_INTEGER(0, "submodule-fetch-jobs", &max_children_for_submodules,
+		    N_("number of submodules fetched in parallel")),
 	OPT_BOOL('p', "prune", &prune,
 		 N_("prune remote-tracking branches no longer on remote")),
 	OPT_BOOL('P', "prune-tags", &prune_tags,
-- 
2.21.0
Palmer Dabbelt· Aug 12, 2019, 21:34 UTC · re: Palmer Dabbelt · lore

[PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

The existing --jobs argument was defined to control the number of jobs used for submodule fetching, but it makes more sense to have this argument control the number of jobs to be used when fetching from multiple remotes as well.

This patch simply changes the --jobs argument parsing code to set both max_children_for_{submodules,fetch}, as well as noting this new behavior in the documentation.

Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
---
 Documentation/fetch-options.txt |  4 ++++
 builtin/fetch.c                 | 21 ++++++++++++++++++---
 2 files changed, 22 insertions(+), 3 deletions(-)
Show changes to 2 files +22 −3

Documentation/fetch-options.txt, builtin/fetch.c

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 5836024f1934..0915fd4ed6d5 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -160,6 +160,10 @@ ifndef::git-pull[]
 
 -j::
 --jobs=<n>::
+	Number of parallel children to be used for all forms of fetching.
+	This is the same as passing `--submodule-fetch-jobs=<n>` and
+	`--fetch-jobs=<n>`.
+
 --submodule-fetch-jobs=<n>::
 	Number of parallel children to be used for fetching submodules.
 	Each will fetch from different submodules, such that fetching many
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 67d001f3f78b..41498e9efb3b 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
 	return git_default_config(k, v, cb);
 }
 
+static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)
+{
+	int jobs;
+
+	jobs = atoi(arg);
+	if (jobs < 1)
+		die(_("There must be a positive number of jobs"));
+
+	max_children_for_submodules = jobs;
+	max_children_for_fetch = jobs;
+
+	return 0;
+}
+
 static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
 {
 	BUG_ON_OPT_NEG(unset);
@@ -142,12 +156,13 @@ static struct option builtin_fetch_options[] = {
 		    N_("fetch all tags and associated objects"), TAGS_SET),
 	OPT_SET_INT('n', NULL, &tags,
 		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
-	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
+	{ OPTION_CALLBACK, 'j', "jobs", NULL, N_("jobs"),
+		    N_("number of parallel tasks to run while fetching"),
+		    PARSE_OPT_OPTARG, &parse_jobs_arg },
+	OPT_INTEGER(0, "submodule-fetch-jobs", &max_children_for_submodules,
 		    N_("number of submodules fetched in parallel")),
 	OPT_INTEGER(0, "fetch-jobs", &max_children_for_fetch,
 		    N_("number of remotes fetched in parallel")),
-	OPT_INTEGER(0, "submodule-fetch-jobs", &max_children_for_submodules,
-		    N_("number of submodules fetched in parallel")),
 	OPT_BOOL('p', "prune", &prune,
 		 N_("prune remote-tracking branches no longer on remote")),
 	OPT_BOOL('P', "prune-tags", &prune_tags,
-- 
2.21.0
Johannes Schindelin· Aug 13, 2019, 20:16 UTC · re: Palmer Dabbelt · lore

Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

Hi,
On Mon, 12 Aug 2019, Palmer Dabbelt wrote:
Show 11 quoted lines
> The existing --jobs argument was defined to control the number of jobs
> used for submodule fetching, but it makes more sense to have this
> argument control the number of jobs to be used when fetching from
> multiple remotes as well.
>
> This patch simply changes the --jobs argument parsing code to set both
> max_children_for_{submodules,fetch}, as well as noting this new behavior
> in the documentation.
>
> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
> ---

I very much miss in this description a reflection of my analysis in https://public-inbox.org/git/nycvar.QRO.7.76.6.1907191507420.47@tvgsbejvaqbjf.bet/

Given that analysis, combined with the fact that the `--jobs` option tries to control both the `--multiple` and `--recursive-submodules` code paths in the end, anyway, I do doubt that it makes sense to even introduce the `--fetch-jobs` and the `--submodule-fetch-jobs` options; They are probably only confusing and do not add much benefit to the end user.

Ciao, Johannes

Show 65 quoted lines
>  Documentation/fetch-options.txt |  4 ++++
>  builtin/fetch.c                 | 21 ++++++++++++++++++---
>  2 files changed, 22 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
> index 5836024f1934..0915fd4ed6d5 100644
> --- a/Documentation/fetch-options.txt
> +++ b/Documentation/fetch-options.txt
> @@ -160,6 +160,10 @@ ifndef::git-pull[]
>
>  -j::
>  --jobs=<n>::
> +	Number of parallel children to be used for all forms of fetching.
> +	This is the same as passing `--submodule-fetch-jobs=<n>` and
> +	`--fetch-jobs=<n>`.
> +
>  --submodule-fetch-jobs=<n>::
>  	Number of parallel children to be used for fetching submodules.
>  	Each will fetch from different submodules, such that fetching many
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 67d001f3f78b..41498e9efb3b 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
>  	return git_default_config(k, v, cb);
>  }
>
> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)
> +{
> +	int jobs;
> +
> +	jobs = atoi(arg);
> +	if (jobs < 1)
> +		die(_("There must be a positive number of jobs"));
> +
> +	max_children_for_submodules = jobs;
> +	max_children_for_fetch = jobs;
> +
> +	return 0;
> +}
> +
>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
>  {
>  	BUG_ON_OPT_NEG(unset);
> @@ -142,12 +156,13 @@ static struct option builtin_fetch_options[] = {
>  		    N_("fetch all tags and associated objects"), TAGS_SET),
>  	OPT_SET_INT('n', NULL, &tags,
>  		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
> -	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
> +	{ OPTION_CALLBACK, 'j', "jobs", NULL, N_("jobs"),
> +		    N_("number of parallel tasks to run while fetching"),
> +		    PARSE_OPT_OPTARG, &parse_jobs_arg },
> +	OPT_INTEGER(0, "submodule-fetch-jobs", &max_children_for_submodules,
>  		    N_("number of submodules fetched in parallel")),
>  	OPT_INTEGER(0, "fetch-jobs", &max_children_for_fetch,
>  		    N_("number of remotes fetched in parallel")),
> -	OPT_INTEGER(0, "submodule-fetch-jobs", &max_children_for_submodules,
> -		    N_("number of submodules fetched in parallel")),
>  	OPT_BOOL('p', "prune", &prune,
>  		 N_("prune remote-tracking branches no longer on remote")),
>  	OPT_BOOL('P', "prune-tags", &prune_tags,
> --
> 2.21.0
>
>
Palmer Dabbelt· Aug 13, 2019, 22:06 UTC · re: Johannes Schindelin · lore

Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

On Tue, 13 Aug 2019 13:16:11 PDT (-0700), Johannes.Schindelin@gmx.de wrote:
Show 26 quoted lines
> Hi,
>
>
> On Mon, 12 Aug 2019, Palmer Dabbelt wrote:
>
>> The existing --jobs argument was defined to control the number of jobs
>> used for submodule fetching, but it makes more sense to have this
>> argument control the number of jobs to be used when fetching from
>> multiple remotes as well.
>>
>> This patch simply changes the --jobs argument parsing code to set both
>> max_children_for_{submodules,fetch}, as well as noting this new behavior
>> in the documentation.
>>
>> Signed-off-by: Palmer Dabbelt <palmer@sifive.com>
>> ---
>
> I very much miss in this description a reflection of my analysis in
> https://public-inbox.org/git/nycvar.QRO.7.76.6.1907191507420.47@tvgsbejvaqbjf.bet/
>
> Given that analysis, combined with the fact that the `--jobs` option
> tries to control both the `--multiple` and `--recursive-submodules` code
> paths in the end, anyway, I do doubt that it makes sense to even
> introduce the `--fetch-jobs` and the `--submodule-fetch-jobs` options;
> They are probably only confusing and do not add much benefit to the end
> user.

The cover letter at least attempts to describe this. I figured I'd have to pick one option for a v2, so I went with the more complicated one under the assumption it would be easy to re-spin a v3 that drops the extra arguments. I'm happy to do so.

Show 69 quoted lines
>
> Ciao,
> Johannes
>
>>  Documentation/fetch-options.txt |  4 ++++
>>  builtin/fetch.c                 | 21 ++++++++++++++++++---
>>  2 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
>> index 5836024f1934..0915fd4ed6d5 100644
>> --- a/Documentation/fetch-options.txt
>> +++ b/Documentation/fetch-options.txt
>> @@ -160,6 +160,10 @@ ifndef::git-pull[]
>>
>>  -j::
>>  --jobs=<n>::
>> +	Number of parallel children to be used for all forms of fetching.
>> +	This is the same as passing `--submodule-fetch-jobs=<n>` and
>> +	`--fetch-jobs=<n>`.
>> +
>>  --submodule-fetch-jobs=<n>::
>>  	Number of parallel children to be used for fetching submodules.
>>  	Each will fetch from different submodules, such that fetching many
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 67d001f3f78b..41498e9efb3b 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
>>  	return git_default_config(k, v, cb);
>>  }
>>
>> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)
>> +{
>> +	int jobs;
>> +
>> +	jobs = atoi(arg);
>> +	if (jobs < 1)
>> +		die(_("There must be a positive number of jobs"));
>> +
>> +	max_children_for_submodules = jobs;
>> +	max_children_for_fetch = jobs;
>> +
>> +	return 0;
>> +}
>> +
>>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
>>  {
>>  	BUG_ON_OPT_NEG(unset);
>> @@ -142,12 +156,13 @@ static struct option builtin_fetch_options[] = {
>>  		    N_("fetch all tags and associated objects"), TAGS_SET),
>>  	OPT_SET_INT('n', NULL, &tags,
>>  		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
>> -	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
>> +	{ OPTION_CALLBACK, 'j', "jobs", NULL, N_("jobs"),
>> +		    N_("number of parallel tasks to run while fetching"),
>> +		    PARSE_OPT_OPTARG, &parse_jobs_arg },
>> +	OPT_INTEGER(0, "submodule-fetch-jobs", &max_children_for_submodules,
>>  		    N_("number of submodules fetched in parallel")),
>>  	OPT_INTEGER(0, "fetch-jobs", &max_children_for_fetch,
>>  		    N_("number of remotes fetched in parallel")),
>> -	OPT_INTEGER(0, "submodule-fetch-jobs", &max_children_for_submodules,
>> -		    N_("number of submodules fetched in parallel")),
>>  	OPT_BOOL('p', "prune", &prune,
>>  		 N_("prune remote-tracking branches no longer on remote")),
>>  	OPT_BOOL('P', "prune-tags", &prune_tags,
>> --
>> 2.21.0
>>
>>
Junio C Hamano· Aug 13, 2019, 22:00 UTC · re: Palmer Dabbelt · lore

Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

Palmer Dabbelt <palmer@sifive.com> writes:
Show 8 quoted lines
> The existing --jobs argument was defined to control the number of jobs
> used for submodule fetching, but it makes more sense to have this
> argument control the number of jobs to be used when fetching from
> multiple remotes as well.
>
> This patch simply changes the --jobs argument parsing code to set both
> max_children_for_{submodules,fetch}, as well as noting this new behavior
> in the documentation.

That's a sensible, if overly careful, transition plan. This patch cannot be queued together with the other four, though, for the plan to be practical. It probably needs to come a few releases after the other four hits a release.

A less involved and much more careless transition plan may be to just declare that "--jobs that only controls submodule fetches is a bug and it must also affect how fetches from multiple remote repositories are done" and come directly to this step, without necessarily introducing options that control them independently.

I have a suspicion that we can afford to go the careless route for this particular one, if we wanted to, as not may people would care about controlling these independently.

Palmer Dabbelt· Aug 13, 2019, 22:06 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

On Tue, 13 Aug 2019 15:00:13 PDT (-0700), gitster@pobox.com wrote:
Show 25 quoted lines
> Palmer Dabbelt <palmer@sifive.com> writes:
>
>> The existing --jobs argument was defined to control the number of jobs
>> used for submodule fetching, but it makes more sense to have this
>> argument control the number of jobs to be used when fetching from
>> multiple remotes as well.
>>
>> This patch simply changes the --jobs argument parsing code to set both
>> max_children_for_{submodules,fetch}, as well as noting this new behavior
>> in the documentation.
>
> That's a sensible, if overly careful, transition plan.  This patch
> cannot be queued together with the other four, though, for the plan
> to be practical.  It probably needs to come a few releases after the
> other four hits a release.
>
> A less involved and much more careless transition plan may be to
> just declare that "--jobs that only controls submodule fetches is a
> bug and it must also affect how fetches from multiple remote
> repositories are done" and come directly to this step, without
> necessarily introducing options that control them independently.
>
> I have a suspicion that we can afford to go the careless route for
> this particular one, if we wanted to, as not may people would care
> about controlling these independently.

This was brought up as part of the v1, and the cover letter lays out a plan to do so. I'm happy to re-spin the patch set to just have --jobs control everything.

SZEDER Gábor· Aug 14, 2019, 08:32 UTC · re: Palmer Dabbelt · lore

Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

On Mon, Aug 12, 2019 at 02:34:48PM -0700, Palmer Dabbelt wrote:
Show 33 quoted lines
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 67d001f3f78b..41498e9efb3b 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
>  	return git_default_config(k, v, cb);
>  }
>  
> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)
> +{
> +	int jobs;
> +
> +	jobs = atoi(arg);
> +	if (jobs < 1)
> +		die(_("There must be a positive number of jobs"));
> +
> +	max_children_for_submodules = jobs;
> +	max_children_for_fetch = jobs;
> +
> +	return 0;
> +}
> +
>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
>  {
>  	BUG_ON_OPT_NEG(unset);
> @@ -142,12 +156,13 @@ static struct option builtin_fetch_options[] = {
>  		    N_("fetch all tags and associated objects"), TAGS_SET),
>  	OPT_SET_INT('n', NULL, &tags,
>  		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
> -	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
> +	{ OPTION_CALLBACK, 'j', "jobs", NULL, N_("jobs"),
> +		    N_("number of parallel tasks to run while fetching"),
> +		    PARSE_OPT_OPTARG, &parse_jobs_arg },

These changes result segmentation faults in the tests '--quiet propagates to parallel submodules' and 'fetching submodules respects parallel settings' in 't5526-fetch-submodules.sh'.

If the number of jobs is specified as '-j 2' or '--jobs 7', i.e. as an unstuck argument of the option, as opposed to '-j2' or '--jobs=7', then 'arg' in the parse_jobs_arg() callback is NULL, which then causes the segfault somewhere inside that atoi() call.

Junio C Hamano· Aug 14, 2019, 15:54 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

SZEDER Gábor <szeder.dev@gmail.com> writes:
Show 43 quoted lines
> On Mon, Aug 12, 2019 at 02:34:48PM -0700, Palmer Dabbelt wrote:
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 67d001f3f78b..41498e9efb3b 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
>>  	return git_default_config(k, v, cb);
>>  }
>>  
>> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)
>> +{
>> +	int jobs;
>> +
>> +	jobs = atoi(arg);
>> +	if (jobs < 1)
>> +		die(_("There must be a positive number of jobs"));
>> +
>> +	max_children_for_submodules = jobs;
>> +	max_children_for_fetch = jobs;
>> +
>> +	return 0;
>> +}
>> +
>>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
>>  {
>>  	BUG_ON_OPT_NEG(unset);
>> @@ -142,12 +156,13 @@ static struct option builtin_fetch_options[] = {
>>  		    N_("fetch all tags and associated objects"), TAGS_SET),
>>  	OPT_SET_INT('n', NULL, &tags,
>>  		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
>> -	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
>> +	{ OPTION_CALLBACK, 'j', "jobs", NULL, N_("jobs"),
>> +		    N_("number of parallel tasks to run while fetching"),
>> +		    PARSE_OPT_OPTARG, &parse_jobs_arg },
>
> These changes result segmentation faults in the tests '--quiet
> propagates to parallel submodules' and 'fetching submodules respects
> parallel settings' in 't5526-fetch-submodules.sh'.
>
> If the number of jobs is specified as '-j 2' or '--jobs 7', i.e. as an
> unstuck argument of the option, as opposed to '-j2' or '--jobs=7',
> then 'arg' in the parse_jobs_arg() callback is NULL, which then causes
> the segfault somewhere inside that atoi() call.
True.  

An easier and more readable way would be to set another "nr-jobs" variable using plain vanilla OPT_INTEGER() and override the other two with it when they are not set after parse_options() returns, I guess.

Palmer Dabbelt· Aug 14, 2019, 18:33 UTC · re: SZEDER Gábor · lore

Re: [PATCH v2 5/5] fetch: Make --jobs control submodules and remotes

On Wed, 14 Aug 2019 01:32:45 PDT (-0700), szeder.dev@gmail.com wrote:
Show 43 quoted lines
> On Mon, Aug 12, 2019 at 02:34:48PM -0700, Palmer Dabbelt wrote:
>> diff --git a/builtin/fetch.c b/builtin/fetch.c
>> index 67d001f3f78b..41498e9efb3b 100644
>> --- a/builtin/fetch.c
>> +++ b/builtin/fetch.c
>> @@ -114,6 +114,20 @@ static int git_fetch_config(const char *k, const char *v, void *cb)
>>  	return git_default_config(k, v, cb);
>>  }
>>
>> +static int parse_jobs_arg(const struct option *opt, const char *arg, int unset)
>> +{
>> +	int jobs;
>> +
>> +	jobs = atoi(arg);
>> +	if (jobs < 1)
>> +		die(_("There must be a positive number of jobs"));
>> +
>> +	max_children_for_submodules = jobs;
>> +	max_children_for_fetch = jobs;
>> +
>> +	return 0;
>> +}
>> +
>>  static int parse_refmap_arg(const struct option *opt, const char *arg, int unset)
>>  {
>>  	BUG_ON_OPT_NEG(unset);
>> @@ -142,12 +156,13 @@ static struct option builtin_fetch_options[] = {
>>  		    N_("fetch all tags and associated objects"), TAGS_SET),
>>  	OPT_SET_INT('n', NULL, &tags,
>>  		    N_("do not fetch all tags (--no-tags)"), TAGS_UNSET),
>> -	OPT_INTEGER('j', "jobs", &max_children_for_submodules,
>> +	{ OPTION_CALLBACK, 'j', "jobs", NULL, N_("jobs"),
>> +		    N_("number of parallel tasks to run while fetching"),
>> +		    PARSE_OPT_OPTARG, &parse_jobs_arg },
>
> These changes result segmentation faults in the tests '--quiet
> propagates to parallel submodules' and 'fetching submodules respects
> parallel settings' in 't5526-fetch-submodules.sh'.
>
> If the number of jobs is specified as '-j 2' or '--jobs 7', i.e. as an
> unstuck argument of the option, as opposed to '-j2' or '--jobs=7',
> then 'arg' in the parse_jobs_arg() callback is NULL, which then causes
> the segfault somewhere inside that atoi() call.
Thanks, I'll fix that in the v3.

← back to recent threads