Re: [PATCH 4/4] diff-lib: parallelize run_diff_files for submodules

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Calvin

On 23/09/2022 00:29, Calvin Wan wrote:
> During the iteration of the index entries in run_diff_files, whenever
> a submodule is found and needs its status checked, a subprocess is
> spawned for it. Instead of spawning the subprocess immediately and
> waiting for its completion to continue, hold onto all submodules and
> relevant information in a list. Then use that list to create tasks for
> run_processes_parallel. Finished subprocesses pipe their output to
> status_finish which parses it and sets the relevant variables.
> 
> Add config option status.parallelSubmodules to set the maximum number
> of parallel jobs.

I suspect in the future we may want to parallelize other commands for 
submodules in which case a more general name such as submodules.threads 
might be a better choice. The speed up in the cover letter is 
impressive, could this be safely enabled by default?

> index fcf9c85947..c5147a7952 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1468,6 +1468,12 @@ static int git_status_config(const char *k, const char *v, void *cb)
>   		s->detect_rename = git_config_rename(k, v);
>   		return 0;
>   	}
> +	if (!strcmp(k, "status.parallelsubmodules")) {
> +		s->parallel_jobs_submodules = git_config_int(k, v);
> +		if (s->parallel_jobs_submodules < 0)
> +			die(_("status.parallelsubmodules cannot be negative"));

What does a value of zero mean?

> diff --git a/diff-lib.c b/diff-lib.c
> index 2e148b79e6..ec745755fc 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c

> -int run_diff_files(struct rev_info *revs, unsigned int option)
> +int run_diff_files(struct rev_info *revs, unsigned int option, int parallel_jobs)

Another possibility would be to add a member to struct diff_opts, rather 
than changing the function signature here, I'm wondering what the trade 
offs of the two approaches are. Also seeing all the callers from other 
commands being changed made me wonder if they would benefit from 
parallelizing submodules as well. There aren't any tests - could we use 
GIT_TRACE2 to check that we are running the submodule diffs in parallel?

Best Wishes

Phillip