diff mbox series

[v4,5/5] diff-lib: parallelize run_diff_files for submodules

Message ID 20221108184200.2813458-6-calvinwan@google.com (mailing list archive)
State New, archived
Headers show
Series submodule: parallelize diff | expand

Commit Message

Calvin Wan Nov. 8, 2022, 6:42 p.m. UTC
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. Subprocess output is duplicated and passed to
status_pipe_output which parses it. 

Add config option submodule.diffJobs to set the maximum number
of parallel jobs. The option defaults to 1 if unset. If set to 0, the
number of jobs is set to online_cpus().

Since run_diff_files is called from many different commands, I chose
to grab the config option in the function rather than adding variables
to every git command and then figuring out how to pass them all in.

Signed-off-by: Calvin Wan <calvinwan@google.com>
---
 Documentation/config/submodule.txt |  12 +++
 diff-lib.c                         |  80 +++++++++++++--
 submodule.c                        | 154 +++++++++++++++++++++++++++++
 submodule.h                        |   9 ++
 t/t4027-diff-submodule.sh          |  19 ++++
 t/t7506-status-submodule.sh        |  19 ++++
 6 files changed, 287 insertions(+), 6 deletions(-)

Comments

Jonathan Tan Nov. 28, 2022, 9:01 p.m. UTC | #1
Calvin Wan <calvinwan@google.com> writes:
>  submodule.c                        | 154 +++++++++++++++++++++++++++++

I think the way to implement this is to have a parallel implementation,
and then have the serial implementation call the parallel implementation's
functions, or have a common set of functions that both the parallel
implementation and the serial implementation call. Here, it seems that
the parallel implementation exists completely separate from the serial
implementation, with no code shared. That makes it both more difficult to
review, and also makes it difficult to make changes to how we diff submodules
in the future (since we would have to make changes in two parts of the code).

I think that the layout of the code will be substantially different if we do
that, so I'll hold off on a more thorough review for now.
Elijah Newren Nov. 29, 2022, 5:13 a.m. UTC | #2
On Tue, Nov 8, 2022 at 11:32 AM Calvin Wan <calvinwan@google.com> 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. Subprocess output is duplicated and passed to
> status_pipe_output which parses it.
>
> Add config option submodule.diffJobs to set the maximum number
> of parallel jobs. The option defaults to 1 if unset. If set to 0, the
> number of jobs is set to online_cpus().
>
> Since run_diff_files is called from many different commands, I chose
> to grab the config option in the function rather than adding variables
> to every git command and then figuring out how to pass them all in.
>
> Signed-off-by: Calvin Wan <calvinwan@google.com>
> ---
>  Documentation/config/submodule.txt |  12 +++
>  diff-lib.c                         |  80 +++++++++++++--
>  submodule.c                        | 154 +++++++++++++++++++++++++++++
>  submodule.h                        |   9 ++
>  t/t4027-diff-submodule.sh          |  19 ++++
>  t/t7506-status-submodule.sh        |  19 ++++
>  6 files changed, 287 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
> index 6490527b45..1144a5ad74 100644
> --- a/Documentation/config/submodule.txt
> +++ b/Documentation/config/submodule.txt
> @@ -93,6 +93,18 @@ submodule.fetchJobs::
>         in parallel. A value of 0 will give some reasonable default.
>         If unset, it defaults to 1.
>
> +submodule.diffJobs::
> +       Specifies how many submodules are diffed at the same time. A
> +       positive integer allows up to that number of submodules diffed
> +       in parallel. A value of 0 will give the number of logical cores.

Why hardcode that 0 gives the number of logical cores?  Why not just
state that a value of 0 "gives a guess at optimal parallelism",
allowing us to adjust it in the future if we can do some smart
heuristics?  It'd be nice to not have us tied down and prevented from
taking a smarter approach.

> +       If unset, it defaults to 1. The diff operation is used by many
> +       other git commands such as add, merge, diff, status, stash and
> +       more. Note that the expensive part of the diff operation is
> +       reading the index from cache or memory. Therefore multiple jobs
> +       may be detrimental to performance if your hardware does not
> +       support parallel reads or if the number of jobs greatly exceeds
> +       the amount of supported reads.

So, in the future, someone who wants to speed things up is going to
need to configure submodule.diffJobs, submodule.fetchJobs,
submodule.checkoutJobs, submodule.grepJobs, submodule.mergeJobs, etc.?
 I worry that we're headed towards a bit of a suboptimal user
experience here.  It'd be nice to have a more central configuration of
"yes, I want parallelism; please don't make me benchmark things in
order to take advantage of it", if that's possible.  It may just be
that the "optimal" parallelism varies significantly between commands,
and also varies a lot based on hardware, repository sizes, background
load on the system, etc. such that we can't provide a reasonable
suggestion for those that want a value greater than 1.  Or maybe in
the future we allow folks somehow to request our best guess at a good
parallelization level and then let users override with these
individual flags.  I'm just a little worried we might be making users
do work that we should somehow figure out.
Glen Choo Nov. 29, 2022, 10:29 p.m. UTC | #3
Jonathan Tan <jonathantanmy@google.com> writes:

> Calvin Wan <calvinwan@google.com> writes:
>>  submodule.c                        | 154 +++++++++++++++++++++++++++++
>
> I think the way to implement this is to have a parallel implementation,
> and then have the serial implementation call the parallel implementation's
> functions, or have a common set of functions that both the parallel
> implementation and the serial implementation call. Here, it seems that
> the parallel implementation exists completely separate from the serial
> implementation, with no code shared. That makes it both more difficult to
> review, and also makes it difficult to make changes to how we diff submodules
> in the future (since we would have to make changes in two parts of the code).

It seems that most of the code is copied from is_submodule_modified(),
so a possible way to do this would be:

- Split is_submodule_modified() into 2 functions, one that sets up the
  "git status --porcelain=2" process (named something like
  setup_status_porcelain()) and one that parses its output (this is
  parse_status_porcelain() in Patch 2). The serial implementation
  (is_submodule_modified()) uses both of these and has some extra logic
  to run the child process.

- Refactor get_next_submodule_status() (from this patch) to use
  setup_status_porcelain().
Calvin Wan Nov. 30, 2022, 6:04 p.m. UTC | #4
> > diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
> > index 6490527b45..1144a5ad74 100644
> > --- a/Documentation/config/submodule.txt
> > +++ b/Documentation/config/submodule.txt
> > @@ -93,6 +93,18 @@ submodule.fetchJobs::
> >         in parallel. A value of 0 will give some reasonable default.
> >         If unset, it defaults to 1.
> >
> > +submodule.diffJobs::
> > +       Specifies how many submodules are diffed at the same time. A
> > +       positive integer allows up to that number of submodules diffed
> > +       in parallel. A value of 0 will give the number of logical cores.
>
> Why hardcode that 0 gives the number of logical cores?  Why not just
> state that a value of 0 "gives a guess at optimal parallelism",
> allowing us to adjust it in the future if we can do some smart
> heuristics?  It'd be nice to not have us tied down and prevented from
> taking a smarter approach.

I was unaware that the original intention of "reasonable default" was for
flexibility (I have a WIP series standardizing these parallelism config
options that also used "number of logical cores" but I think that should
probably change now). There are other parallel config options that
hardcode 0 as well, so my initial thought was that we should be using
the more precise wording -- the argument for flexibility now seems
more preferable, however.

>
> > +       If unset, it defaults to 1. The diff operation is used by many
> > +       other git commands such as add, merge, diff, status, stash and
> > +       more. Note that the expensive part of the diff operation is
> > +       reading the index from cache or memory. Therefore multiple jobs
> > +       may be detrimental to performance if your hardware does not
> > +       support parallel reads or if the number of jobs greatly exceeds
> > +       the amount of supported reads.
>
> So, in the future, someone who wants to speed things up is going to
> need to configure submodule.diffJobs, submodule.fetchJobs,
> submodule.checkoutJobs, submodule.grepJobs, submodule.mergeJobs, etc.?
>  I worry that we're headed towards a bit of a suboptimal user
> experience here.  It'd be nice to have a more central configuration of
> "yes, I want parallelism; please don't make me benchmark things in
> order to take advantage of it", if that's possible.  It may just be
> that the "optimal" parallelism varies significantly between commands,
> and also varies a lot based on hardware, repository sizes, background
> load on the system, etc. such that we can't provide a reasonable
> suggestion for those that want a value greater than 1.  Or maybe in
> the future we allow folks somehow to request our best guess at a good
> parallelization level and then let users override with these
> individual flags.  I'm just a little worried we might be making users
> do work that we should somehow figure out.

I had the same worry as well -- see the discussion I had here:
https://lore.kernel.org/git/CAFySSZAbsPuyPVX0+DQzArny2CEWs+GpQqJ3AOxUB_ffo8B3SQ@mail.gmail.com/
I would like to also eventually solve this problem, but this patch
won't be the one to do so.
Calvin Wan Nov. 30, 2022, 6:11 p.m. UTC | #5
On Tue, Nov 29, 2022 at 2:29 PM Glen Choo <chooglen@google.com> wrote:
>
> Jonathan Tan <jonathantanmy@google.com> writes:
>
> > Calvin Wan <calvinwan@google.com> writes:
> >>  submodule.c                        | 154 +++++++++++++++++++++++++++++
> >
> > I think the way to implement this is to have a parallel implementation,
> > and then have the serial implementation call the parallel implementation's
> > functions, or have a common set of functions that both the parallel
> > implementation and the serial implementation call. Here, it seems that
> > the parallel implementation exists completely separate from the serial
> > implementation, with no code shared. That makes it both more difficult to
> > review, and also makes it difficult to make changes to how we diff submodules
> > in the future (since we would have to make changes in two parts of the code).
>
> It seems that most of the code is copied from is_submodule_modified(),
> so a possible way to do this would be:
>
> - Split is_submodule_modified() into 2 functions, one that sets up the
>   "git status --porcelain=2" process (named something like
>   setup_status_porcelain()) and one that parses its output (this is
>   parse_status_porcelain() in Patch 2). The serial implementation
>   (is_submodule_modified()) uses both of these and has some extra logic
>   to run the child process.
>
> - Refactor get_next_submodule_status() (from this patch) to use
>   setup_status_porcelain().

That sounds like a reasonable plan to me. Thanks!
diff mbox series

Patch

diff --git a/Documentation/config/submodule.txt b/Documentation/config/submodule.txt
index 6490527b45..1144a5ad74 100644
--- a/Documentation/config/submodule.txt
+++ b/Documentation/config/submodule.txt
@@ -93,6 +93,18 @@  submodule.fetchJobs::
 	in parallel. A value of 0 will give some reasonable default.
 	If unset, it defaults to 1.
 
+submodule.diffJobs::
+	Specifies how many submodules are diffed at the same time. A
+	positive integer allows up to that number of submodules diffed
+	in parallel. A value of 0 will give the number of logical cores.
+	If unset, it defaults to 1. The diff operation is used by many
+	other git commands such as add, merge, diff, status, stash and
+	more. Note that the expensive part of the diff operation is
+	reading the index from cache or memory. Therefore multiple jobs
+	may be detrimental to performance if your hardware does not
+	support parallel reads or if the number of jobs greatly exceeds
+	the amount of supported reads.
+
 submodule.alternateLocation::
 	Specifies how the submodules obtain alternates when submodules are
 	cloned. Possible values are `no`, `superproject`.
diff --git a/diff-lib.c b/diff-lib.c
index f5257c0c71..30a3d9a2b5 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -14,6 +14,7 @@ 
 #include "dir.h"
 #include "fsmonitor.h"
 #include "commit-reach.h"
+#include "config.h"
 
 /*
  * diff-files
@@ -65,15 +66,20 @@  static int check_removed(const struct index_state *istate, const struct cache_en
  * Return 1 when changes are detected, 0 otherwise. If the DIRTY_SUBMODULES
  * option is set, the caller does not only want to know if a submodule is
  * modified at all but wants to know all the conditions that are met (new
- * commits, untracked content and/or modified content).
+ * commits, untracked content and/or modified content). If
+ * defer_submodule_status bit is set, dirty_submodule will be left to the
+ * caller to set. defer_submodule_status can also be set to 0 in this
+ * function if there is no need to check if the submodule is modified.
  */
 static int match_stat_with_submodule(struct diff_options *diffopt,
 				     const struct cache_entry *ce,
 				     struct stat *st, unsigned ce_option,
-				     unsigned *dirty_submodule)
+				     unsigned *dirty_submodule, int *defer_submodule_status,
+				     unsigned *ignore_untracked)
 {
 	int changed = ie_match_stat(diffopt->repo->index, ce, st, ce_option);
 	struct diff_flags orig_flags;
+	int defer = 0;
 
 	if (!S_ISGITLINK(ce->ce_mode))
 		goto ret;
@@ -86,12 +92,20 @@  static int match_stat_with_submodule(struct diff_options *diffopt,
 		goto cleanup;
 	}
 	if (!diffopt->flags.ignore_dirty_submodules &&
-	    (!changed || diffopt->flags.dirty_submodules))
-		*dirty_submodule = is_submodule_modified(ce->name,
+	    (!changed || diffopt->flags.dirty_submodules)) {
+		if (defer_submodule_status && *defer_submodule_status) {
+			defer = 1;
+			*ignore_untracked = diffopt->flags.ignore_untracked_in_submodules;
+		} else {
+			*dirty_submodule = is_submodule_modified(ce->name,
 					 diffopt->flags.ignore_untracked_in_submodules);
+		}
+	}
 cleanup:
 	diffopt->flags = orig_flags;
 ret:
+	if (defer_submodule_status)
+		*defer_submodule_status = defer;
 	return changed;
 }
 
@@ -103,6 +117,7 @@  int run_diff_files(struct rev_info *revs, unsigned int option)
 			      ? CE_MATCH_RACY_IS_DIRTY : 0);
 	uint64_t start = getnanotime();
 	struct index_state *istate = revs->diffopt.repo->index;
+	struct string_list submodules = STRING_LIST_INIT_NODUP;
 
 	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
 
@@ -227,6 +242,8 @@  int run_diff_files(struct rev_info *revs, unsigned int option)
 			newmode = ce->ce_mode;
 		} else {
 			struct stat st;
+			unsigned ignore_untracked = 0;
+			int defer_submodule_status = !!revs->repo;
 
 			changed = check_removed(istate, ce, &st);
 			if (changed) {
@@ -248,8 +265,25 @@  int run_diff_files(struct rev_info *revs, unsigned int option)
 			}
 
 			changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
-							    ce_option, &dirty_submodule);
+							    ce_option, &dirty_submodule,
+							    &defer_submodule_status,
+							    &ignore_untracked);
 			newmode = ce_mode_from_stat(ce, st.st_mode);
+			if (defer_submodule_status) {
+				struct submodule_status_util tmp = {
+					.changed = changed,
+					.dirty_submodule = 0,
+					.ignore_untracked = ignore_untracked,
+					.newmode = newmode,
+					.ce = ce,
+				};
+				struct string_list_item *item;
+
+				item = string_list_append(&submodules, ce->name);
+				item->util = xmalloc(sizeof(tmp));
+				memcpy(item->util, &tmp, sizeof(tmp));
+				continue;
+			}
 		}
 
 		if (!changed && !dirty_submodule) {
@@ -268,6 +302,40 @@  int run_diff_files(struct rev_info *revs, unsigned int option)
 			    ce->name, 0, dirty_submodule);
 
 	}
+	if (submodules.nr > 0) {
+		int parallel_jobs;
+		if (git_config_get_int("submodule.diffjobs", &parallel_jobs))
+			parallel_jobs = 1;
+		else if (!parallel_jobs)
+			parallel_jobs = online_cpus();
+		else if (parallel_jobs < 0)
+			die(_("submodule.diffjobs cannot be negative"));
+
+		if (get_submodules_status(revs->repo, &submodules, parallel_jobs))
+			die(_("submodule status failed"));
+		for (size_t i = 0; i < submodules.nr; i++) {
+			struct submodule_status_util *util = submodules.items[i].util;
+			struct cache_entry *ce = util->ce;
+			unsigned int oldmode;
+			const struct object_id *old_oid, *new_oid;
+
+			if (!util->changed && !util->dirty_submodule) {
+				ce_mark_uptodate(ce);
+				mark_fsmonitor_valid(istate, ce);
+				if (!revs->diffopt.flags.find_copies_harder)
+					continue;
+			}
+			oldmode = ce->ce_mode;
+			old_oid = &ce->oid;
+			new_oid = util->changed ? null_oid() : &ce->oid;
+			diff_change(&revs->diffopt, oldmode, util->newmode,
+				    old_oid, new_oid,
+				    !is_null_oid(old_oid),
+				    !is_null_oid(new_oid),
+				    ce->name, 0, util->dirty_submodule);
+		}
+	}
+	string_list_clear(&submodules, 1);
 	diffcore_std(&revs->diffopt);
 	diff_flush(&revs->diffopt);
 	trace_performance_since(start, "diff-files");
@@ -315,7 +383,7 @@  static int get_stat_data(const struct index_state *istate,
 			return -1;
 		}
 		changed = match_stat_with_submodule(diffopt, ce, &st,
-						    0, dirty_submodule);
+						    0, dirty_submodule, NULL, NULL);
 		if (changed) {
 			mode = ce_mode_from_stat(ce, st.st_mode);
 			oid = null_oid();
diff --git a/submodule.c b/submodule.c
index fd3385620c..763a05d523 100644
--- a/submodule.c
+++ b/submodule.c
@@ -1363,6 +1363,18 @@  int submodule_touches_in_range(struct repository *r,
 	return ret;
 }
 
+struct submodule_parallel_status {
+	size_t index_count;
+	int result;
+
+	struct string_list *submodule_names;
+	struct repository *r;
+
+	/* Pending statuses by OIDs */
+	struct status_task **oid_status_tasks;
+	int oid_status_tasks_nr, oid_status_tasks_alloc;
+};
+
 struct submodule_parallel_fetch {
 	/*
 	 * The index of the last index entry processed by
@@ -1445,6 +1457,12 @@  struct fetch_task {
 	struct oid_array *commits; /* Ensure these commits are fetched */
 };
 
+struct status_task {
+	const char *path;
+	unsigned dirty_submodule;
+	int ignore_untracked;
+};
+
 /**
  * When a submodule is not defined in .gitmodules, we cannot access it
  * via the regular submodule-config. Create a fake submodule, which we can
@@ -1956,6 +1974,142 @@  unsigned is_submodule_modified(const char *path, int ignore_untracked)
 	return dirty_submodule;
 }
 
+static struct status_task *
+get_status_task_from_index(struct submodule_parallel_status *sps,
+			   struct strbuf *err)
+{
+	for (; sps->index_count < sps->submodule_names->nr; sps->index_count++) {
+		struct submodule_status_util *util = sps->submodule_names->items[sps->index_count].util;
+		const struct cache_entry *ce = util->ce;
+		struct status_task *task;
+		struct status_task tmp = {
+			.path = ce->name,
+			.dirty_submodule = util->dirty_submodule,
+			.ignore_untracked = util->ignore_untracked,
+		};
+		struct strbuf buf = STRBUF_INIT;
+		const char *git_dir;
+
+		strbuf_addf(&buf, "%s/.git", ce->name);
+		git_dir = read_gitfile(buf.buf);
+		if (!git_dir)
+			git_dir = buf.buf;
+		if (!is_git_directory(git_dir)) {
+			if (is_directory(git_dir))
+				die(_("'%s' not recognized as a git repository"), git_dir);
+			strbuf_release(&buf);
+			/* The submodule is not checked out, so it is not modified */
+			util->dirty_submodule = 0;
+			continue;
+		}
+		strbuf_release(&buf);
+
+		task = xmalloc(sizeof(*task));
+		memcpy(task, &tmp, sizeof(*task));
+		sps->index_count++;
+		return task;
+	}
+	return NULL;
+}
+
+
+static int get_next_submodule_status(struct child_process *cp,
+				     struct strbuf *err, void *data,
+				     void **task_cb)
+{
+	struct submodule_parallel_status *sps = data;
+	struct status_task *task = get_status_task_from_index(sps, err);
+
+	if (!task)
+		return 0;
+
+	child_process_init(cp);
+	prepare_submodule_repo_env_in_gitdir(&cp->env);
+
+	strvec_init(&cp->args);
+	strvec_pushl(&cp->args, "status", "--porcelain=2", NULL);
+	if (task->ignore_untracked)
+		strvec_push(&cp->args, "-uno");
+
+	prepare_submodule_repo_env(&cp->env);
+	cp->git_cmd = 1;
+	cp->dir = task->path;
+	*task_cb = task;
+	return 1;
+}
+
+static int status_start_failure(struct strbuf *err,
+			        void *cb, void *task_cb)
+{
+	struct submodule_parallel_status *sps = cb;
+
+	sps->result = 1;
+	return 0;
+}
+
+static void status_duplicate_output(struct strbuf *process_out,
+				    struct strbuf *out,
+				    void *cb, void *task_cb)
+{
+	struct status_task *task = task_cb;
+	struct string_list list = STRING_LIST_INIT_DUP;
+	struct string_list_item *item;
+
+	string_list_split(&list, process_out->buf, '\n', -1);
+
+	for_each_string_list_item(item, &list) {
+		if (parse_status_porcelain(item->string,
+				   strlen(item->string),
+				   &task->dirty_submodule,
+				   task->ignore_untracked))
+			break;
+	}
+	string_list_clear(&list, 0);
+	strbuf_reset(out);
+}
+
+static int status_finish(int retvalue, struct strbuf *err,
+			 void *cb, void *task_cb)
+{
+	struct submodule_parallel_status *sps = cb;
+	struct status_task *task = task_cb;
+	struct string_list_item *it =
+		string_list_lookup(sps->submodule_names, task->path);
+	struct submodule_status_util *util = it->util;
+
+	util->dirty_submodule = task->dirty_submodule;
+	free(task);
+
+	return 0;
+}
+
+int get_submodules_status(struct repository *r,
+			  struct string_list *submodules,
+			  int max_parallel_jobs)
+{
+	struct submodule_parallel_status sps = {
+                .r = r,
+                .submodule_names = submodules,
+        };
+	const struct run_process_parallel_opts opts = {
+		.tr2_category = "submodule",
+		.tr2_label = "parallel/status",
+
+		.processes = max_parallel_jobs,
+
+		.get_next_task = get_next_submodule_status,
+		.start_failure = status_start_failure,
+		.duplicate_output = status_duplicate_output,
+		.task_finished = status_finish,
+		.data = &sps,
+	};
+
+	string_list_sort(sps.submodule_names);
+	run_processes_parallel(&opts);
+
+	return sps.result;
+}
+
 int submodule_uses_gitfile(const char *path)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
diff --git a/submodule.h b/submodule.h
index 6a9fec6de1..cbb7231a5d 100644
--- a/submodule.h
+++ b/submodule.h
@@ -41,6 +41,12 @@  struct submodule_update_strategy {
 	.type = SM_UPDATE_UNSPECIFIED, \
 }
 
+struct submodule_status_util {
+	int changed, ignore_untracked;
+	unsigned dirty_submodule, newmode;
+	struct cache_entry *ce;
+};
+
 int is_gitmodules_unmerged(struct index_state *istate);
 int is_writing_gitmodules_ok(void);
 int is_staging_gitmodules_ok(struct index_state *istate);
@@ -94,6 +100,9 @@  int fetch_submodules(struct repository *r,
 		     int command_line_option,
 		     int default_option,
 		     int quiet, int max_parallel_jobs);
+int get_submodules_status(struct repository *r,
+			  struct string_list *submodules,
+			  int max_parallel_jobs);
 unsigned is_submodule_modified(const char *path, int ignore_untracked);
 int submodule_uses_gitfile(const char *path);
 
diff --git a/t/t4027-diff-submodule.sh b/t/t4027-diff-submodule.sh
index 40164ae07d..e08ee315a7 100755
--- a/t/t4027-diff-submodule.sh
+++ b/t/t4027-diff-submodule.sh
@@ -34,6 +34,25 @@  test_expect_success setup '
 	subtip=$3 subprev=$2
 '
 
+test_expect_success 'diff in superproject with submodules respects parallel settings' '
+	test_when_finished "rm -f trace.out" &&
+	(
+		GIT_TRACE=$(pwd)/trace.out git diff &&
+		grep "1 tasks" trace.out &&
+		>trace.out &&
+
+		git config submodule.diffJobs 8 &&
+		GIT_TRACE=$(pwd)/trace.out git diff &&
+		grep "8 tasks" trace.out &&
+		>trace.out &&
+
+		GIT_TRACE=$(pwd)/trace.out git -c submodule.diffJobs=0 diff &&
+		grep "preparing to run up to [0-9]* tasks" trace.out &&
+		! grep "up to 0 tasks" trace.out &&
+		>trace.out
+	)
+'
+
 test_expect_success 'git diff --raw HEAD' '
 	hexsz=$(test_oid hexsz) &&
 	git diff --raw --abbrev=$hexsz HEAD >actual &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index d050091345..52a82b703f 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -412,4 +412,23 @@  test_expect_success 'status with added file in nested submodule (short)' '
 	EOF
 '
 
+test_expect_success 'status in superproject with submodules respects parallel settings' '
+	test_when_finished "rm -f trace.out" &&
+	(
+		GIT_TRACE=$(pwd)/trace.out git status &&
+		grep "1 tasks" trace.out &&
+		>trace.out &&
+
+		git config submodule.diffJobs 8 &&
+		GIT_TRACE=$(pwd)/trace.out git status &&
+		grep "8 tasks" trace.out &&
+		>trace.out &&
+
+		GIT_TRACE=$(pwd)/trace.out git -c submodule.diffJobs=0 status &&
+		grep "preparing to run up to [0-9]* tasks" trace.out &&
+		! grep "up to 0 tasks" trace.out &&
+		>trace.out
+	)
+'
+
 test_done