Re: [BUG?] Major performance issue with some commands on our repo's master branch

From: Tao Klerks <tao@klerks.biz>
Date: Jun 6, 2022, 05:18 UTC
Message-ID: <CAPMMpog-7eDOrgSU9GjV4G9rk5RkL-PJhaUAO3_0p2YxfRf=LA@mail.gmail.com>
In-Reply-To: <87y1yb2xc8.fsf@gnu.org>

On Sun, Jun 5, 2022 at 12:55 PM Tassilo Horn <tsdh@gnu.org> wrote:

Show 18 quoted lines

>
> Tao Klerks <tao@klerks.biz> writes:
>
> > I haven't attempted to debug this, and personally have little
> > incentive to do, as switching to "git log" and accepting the process
> > overheads solved *my* problem.
>
> And I'm happy to report it solves *my* problem as well.  There's a PR
> for the Magit git porcelain replacing "git show" with an equivalent "git
> log" incarnation which makes the 30seconds "refresh status buffer"
> operation instant.
>
>   https://github.com/magit/magit/issues/4702
>   https://github.com/magit/magit/compare/km/show-to-log
>
> Still maybe someone might want to have a look at the "git show" issue to
> double-check if the performance burden in this specific case (no diff
> should be generated) is warranted.

I spent a little time with this yesterday, and can confirm:
* My issue seems to be the same as yours, "export GIT_TRACE2_PERF=1"
shows all the time being spent in rename detection
* "git show" is a slightly different entry point into the "git log"
code (log.c, cmd_show())
* options to the "git log" functionality are largely collected in a
"rev_info" object (defined in revision.h)
* one option is the "-c / --diff-merges=combined" option of "git log"
(setting rev_info.diff, rev_info.combine_merges and
rev_info.dense_combined_merges)
* another option is "-s / --no-patch" option of "git log" (setting
rev_info.diffopt.output_format |= DIFF_FORMAT_NO_OUTPUT)
* the "--patch" and "--no-patch" options seem to be (and are
documented as) opposites, but they are not implemented as such; one
calls for the work to be done, and the other only hides the output.
* the "diffs" options are set automatically/implicitly for "git show",
before argument parsing
* we can simulate the "git show" performance issue in "--git log -1"
by setting "--diff-merges=combined" *and* "--no-patch" explicitly
* this performance issue does *not* present in a "git log" call with
only the regular "--patch" argument, however; this basic "show
patches" instruction defaults to "--diff-merges=off", which means the
rename detection work does not need to happen. There might still be
slight diff-related overheads, but they are undetectable in my
testing.

Therefore, I have confirmed it is possible to get "git show" to behave the way we would expect for "--no-patch", by *also* specifying "--diff-merges=off".

There are at least two possible approaches / directions to improving these issues generically in the "git show" implementation, I think:

1. Add some "git show"-specific code, saying something along the lines
of "if --no-patch is specified, then also imply "--diff-merges=off".
This feels like the safer option / less likely to have side-effects.

2. Add some post-processing to the generic "git log & git show"
options parsing, to generically propagate "--no-patch" into other
properties like those set by "--diff-merges=combined"

I don't feel confident enough with the code here to try for the second approach, but the first looks like something I should be able to propose a patch for - and in the meantime I know how to get the "single-process, many arbitrary commits" performance benefit of "git show" again. Thanks for sparking the exploration down this little rabbit-hole!