Re: [PATCHv3 07/13] fetch_populated_submodules: use new parallel job processing

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2015, 16:28 UTC
Message-ID: <xmqqvbb24dvj.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1442875159-13027-8-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 22 quoted lines

> In a later patch we enable parallel processing of submodules, this
> only adds the possibility for it. So this change should not change
> any user facing behavior.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  builtin/fetch.c |   3 +-
>  submodule.c     | 119 +++++++++++++++++++++++++++++++++++++++-----------------
>  submodule.h     |   2 +-
>  3 files changed, 87 insertions(+), 37 deletions(-)
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index ee1f1a9..6620ed0 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1217,7 +1217,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  		result = fetch_populated_submodules(&options,
>  						    submodule_prefix,
>  						    recurse_submodules,
> -						    verbosity < 0);
> +						    verbosity < 0,
> +						    0);

This one...

Show 7 quoted lines

>  int fetch_populated_submodules(const struct argv_array *options,
>  			       const char *prefix, int command_line_option,
> -			       int quiet)
> +			       int quiet, int max_parallel_jobs)
>  {
> -	int i, result = 0;
> -	struct child_process cp = CHILD_PROCESS_INIT;

... together with this one, could have been made easier to follow if you didn't add a new parameter to the function. Instead, you could define a local variable max_parallel_jobs initialized to 1 in this function without changing the function signature (and the caller) in this step, and then did the above two changes in the same commit as the one that actually enables the feature.

That would be more in line with the stated "only adds the possiblity for it" goal.

As posted, I suspect that by passing 0 to max_parallel_jobs, you are telling run_processes_parallel_init() to check online_cpus() and run that many processes in parallel, no?

Show 11 quoted lines

> +int get_next_submodule(void *data, struct child_process *cp,
> +		       struct strbuf *err)
> +{
> +	int ret = 0;
> +	struct submodule_parallel_fetch *spf = data;
> ...
> +			child_process_init(cp);
> +			cp->dir = strbuf_detach(&submodule_path, NULL);
> +			cp->git_cmd = 1;
> +			cp->no_stdout = 1;
> +			cp->no_stdin = 1;

In run-commands.c::start_command(), no_stdout triggers dup_devnull(1) and causes dup2(2, 1) that is triggered by stdout_to_stderrd to be bypassed. This looks wrong to me.

> +			cp->stdout_to_stderr = 1;
> +			cp->err = -1;

OK, the original left the standard error stream connected to the invoker's standard error, but now we are capturing it via a pipe.