Re: [PATCH v6 5/6] diff-lib: parallelize run_diff_files for submodules
- From
Calvin Wan <calvinwan@google.com>
- Date
- Jan 26, 2023, 18:52 UTC
- Message-ID
- <CAFySSZBiW7=ZTmXRaLzCoKUi0Jd=fzvW5PJ6=Ka0jKHoP2ddSw@mail.gmail.com>
- In-Reply-To
- <kl6lilgtveoe.fsf@chooglen-macbookpro.roam.corp.google.com>
On Thu, Jan 26, 2023 at 1:16 AM Glen Choo <chooglen@google.com> wrote:
Show 13 quoted lines
>
>
> Calvin Wan <calvinwan@google.com> writes:
>
> > @@ -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.Ah looks like a vestigial sanity check. You're correct that we would always be deferring anyways.
Show 26 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.Unfortunately this doesn't work because tmp is deallocated off the stack after changing scope.
Show 24 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.
ack.
Show 33 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".
ack.