Re: [PATCH v6 5/6] diff-lib: parallelize run_diff_files for submodules
- From
Glen Choo <chooglen@google.com>
- Date
- Jan 26, 2023, 09:16 UTC
- Message-ID
- <kl6lilgtveoe.fsf@chooglen-macbookpro.roam.corp.google.com>
- In-Reply-To
- <20230117193041.708692-6-calvinwan@google.com>
Calvin Wan <calvinwan@google.com> writes:
Show 6 quoted lines
> @@ -226,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;What is the reasoning behind this condition? I would expect revs->repo to always be set, and we would always end up deferring.
Show 15 quoted lines
> 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,
> + .path = ce->name,
> + };
> + struct string_list_item *item;
> +
> + item = string_list_append(&submodules, ce->name);
> + item->util = xmalloc(sizeof(tmp));
> + memcpy(item->util, &tmp, sizeof(tmp));(Not a C expert) Since we don't return the string list, I wonder if we can avoid the memcpy() by using &tmp like so:
struct string_list_item *item; item = string_list_append(&submodules, ce->name); item->util = &tmp;
And then when we call string_list_clear(), we wouldn't need to free the util since we exit the stack frame.
Show 19 quoted lines
> +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 > + ) > +' > +
Could we get tests to check that the output of git diff isn't changed by setting parallelism? This might not be feasible for submodule.diffJobs > 1 due to raciness, but it would be good to see for submodule.diffJobs = 1 at least.
Show 30 quoted lines
> 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 > + ) > +' > +
Ditto for "status".