Re: [PATCH v6 5/6] diff-lib: parallelize run_diff_files for submodules

From: Calvin Wan <calvinwan@google.com>
To: Glen Choo <chooglen@google.com>
Cc: git@vger.kernel.org, emilyshaffer@google.com, avarab@gmail.com,
	phillip.wood123@gmail.com, newren@gmail.com,
	jonathantanmy@google.com
Subject: Re: [PATCH v6 5/6] diff-lib: parallelize run_diff_files for submodules
Date: Thu, 26 Jan 2023 10:52:17 -0800	[thread overview]
Message-ID: <CAFySSZBiW7=ZTmXRaLzCoKUi0Jd=fzvW5PJ6=Ka0jKHoP2ddSw@mail.gmail.com> (raw)
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:
>
>
> 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.

>
> >                       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.

> > +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.

>
> >  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.