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

From: Calvin Wan <calvinwan@google.com>
Date: Sep 26, 2022, 19:12 UTC
Message-ID: <CAFySSZBCaAUGZuEdxSJEW_7tEcOVJfnkA_nY6BTkTQ1s3xGd5g@mail.gmail.com>
In-Reply-To: <xmqqy1u9sr3a.fsf@gitster.g>

> You make it sound as if this is only for "git status", but shouldn't
> it benefit the usual "git diff" the same way when you have
> submodules that can be dirty?

It should also! I'll reword the commit message.

Show 23 quoted lines

> > Add config option status.parallelSubmodules to set the maximum number
> > of parallel jobs.
>
> Configuration is fine, but I am having a hard time justifying the
> addition of an extra parameter to run_diff_files().  It might be
> more palatable to give a new bit (default off) in the rev structure
> that tells it that it is OK to go into the "defer_submodule_status"
> mode, if we absolutely want to change the behaviour of
> run_diff_files() only for a single caller or something, but I doubt
> it should even be needed.
>
> I cannot think of a sane user interface that says "when
> run_diff_files() is invoked as an implementation detail of 'status',
> use this value, and when it is running for another command 'foo',
> use this other value".  How would a user decide to pick a different
> value for different commands?
>
> Letting a single configuration variable to decide the degree of
> parallelism inside run_diff_files() would be sufficient, and we
> shouldn't have to touch all the existing calling sites of
> run_diff_files() all over the place.  If you absolutely want to do
> this, I'd rather see the new member for the configuration variable
> not in wt_status but in rev_info.

I agree the configuration variable should be in rev_info and not be specific to status since other callers can take advantage of it.

Show 10 quoted lines

>
> > +status.parallelSubmodules::
> > +     When linkgit:git-status[1] is run in a superproject with
> > +     submodules, a status subprocess is spawned for every submodule.
> > +     This option specifies the number of submodule status subprocesses
> > +     to run in parallel. If unset, it defaults to 1.
>
> As I said, I am not sure <cmd>.parallelSubmodules per command makes
> much sense.  "If unset, it defaults to" sounds a bit redundant (if
> it is explicitly set, the default value should not matter).

ack.

Show 11 quoted lines

> > @@ -138,6 +153,9 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
> >               struct cache_entry *ce = istate->cache[i];
> >               int changed;
> >               unsigned dirty_submodule = 0;
> > +             int defer_submodule_status = revs && revs->repo &&
> > +                                                     !strcmp(revs->repo->gitdir, ".git");
>
> What is this overly deeply indented comparison with ".git" doing?
> What are we checking and why?  Is that something we need to do for
> each and every active_cache[] entry, or isn't it constant over the
> loop?

It is checking to see whether we are in the superproject or not, since it doesn't make sense to parallelize status in a submodule subprocess. It doesn't need to be in the loop though -- will move out.

Show 27 quoted lines

> > +             int ignore_untracked_in_submodules;
> >
> >               if (diff_can_quit_early(&revs->diffopt))
> >                       break;
> > @@ -269,11 +287,36 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
> >                       }
> >
> >                       changed = match_stat_with_submodule(&revs->diffopt, ce, &st,
> > +                                                     ce_option, &dirty_submodule,
> > +                                                     &defer_submodule_status,
> > +                                                     &ignore_untracked_in_submodules);
> >                       newmode = ce_mode_from_stat(ce, st.st_mode);
> > +                     if (defer_submodule_status) {
> > +                             struct string_list_item *item =
> > +                                                             string_list_append(&submodules, ce->name);
> > +                             struct submodule_status_util *util = xmalloc(sizeof(*util));
> > +                             util->changed = changed;
> > +                             util->dirty_submodule = 0;
> > +                             util->ignore_untracked = ignore_untracked_in_submodules;
> > +                             util->newmode = newmode;
> > +                             util->ce = ce;
> > +                             item->util = util;
> > +                             continue;
>
> This makes me wonder if defer_submodule_status should be a string
> list that receives the punted submodules---iow, don't these details
> belong to match_stat_with_submodule() rather than its caller here?

That makes sense. I can definitely clean up this section and match_stat_with_submodules() more.

Show 5 quoted lines

>
> Even better may be to start a new child task for the submodule here,
> letting it work while the parent process moves on to the next entry
> in the active_cache[].  I do not know if you thought about doing it
> that way.

I think the implementation complexity of doing it this way would be very high -- basically reimplementing run_processes_parallel(), but within this loop. And the benefit of doing so I think is not worth it -- at that point, I might as well parallelize the entire function.

> > +             for (int i = 0; i < submodules.nr; i++) {
>
> We still do not allow "for (type var = init; ...)" if I am not
> mistaken.  Check the coding guidelines.

ack.