Optimization batch 14: trivial directory resolution

[newren]

This series depends textually on ort-perf-batch-12, but is
semantically independent. (It is both semantically and textually
independent of ort-perf-batch-13.)

Most of my previous series dramatically accelerated cases with lots of
renames, while providing comparatively minor benefits for cases with
few or no renames. This series is the opposite; it provides huge
benefits when there are few or no renames, and comparatively smaller
(though still quite decent) benefits for cases with many uncached
renames.

Changes since v3:

• Add Stolee's Acked-by.

Changes since v2, addressing feedback from Stolee:

• Created a separate struct for three related variables to hint they are related
• Simplified a lengthy comment that was duplicated by the commit message
• Various other minor cleanups

Changes since v1:

• Minor tweak to the final patch to correct implicit assumption that rename detection running implies all renames were found (rename limits could have been exceeded and prevented finding renames)

=== Basic Optimization idea ===

unpack_trees has had a concept of trivial merges for individual files
(see Documentation/technical/trivial-merge.txt). The same idea can be
applied in merge-ort. It'd be really nice to extend that idea to
trees as well, as it could provide a huge performance boost; sadly
however, applying it in general would wreck both regular rename
detection (the unmatched side can have new files that serve as
potential destinations in rename detection) and directory rename
detection (the unmatched side could have a new directory that was
moved into it).

If we somehow knew rename detection wasn't needed, we could do trivial
directory resolution. In the past, this wasn't possible. However...

With recent optimizations we have created a possibility to do trivial
directory resolutions in some cases. These came from the addition of
the "skipping irrelevant renames" optimizations (from ort-perf-batch-9
and ort-perf-batch-10), and in particular noting that we added an
ability to entirely skip rename detection in commit f89b4f2
("merge-ort: skip rename detection entirely if possible", 2021-03-11)
when there are no relevant sources. We can detect if there are no
relevant sources without recursing into the directories in question.

As a cherry on top, the caching of renames (from ort-perf-batch-11)
allows us to cover additional cases.

This series is all about adding all the special checks needed to
safely perform trival directory resolutions.

=== Results ===

For the testcases mentioned in commit 557ac03 ("merge-ort: begin
performance work; instrument with trace2_region_* calls", 2020-10-28),
the changes in just this series improves the performance as follows:

                     Before Series           After Series
no-renames:        5.235 s ±  0.042 s   204.2  ms ±  3.0  ms
mega-renames:      9.419 s ±  0.107 s     1.076 s ±  0.015 s
just-one-mega:   480.1  ms ±  3.9  ms   364.1  ms ±  7.0  ms

As a reminder, before any merge-ort/diffcore-rename performance work,
the performance results we started with (for merge-recursive as of
git-2.30.0) were:

no-renames-am:      6.940 s ±  0.485 s
no-renames:        18.912 s ±  0.174 s
mega-renames:    5964.031 s ± 10.459 s
just-one-mega:    149.583 s ±  0.751 s

cc: Derrick Stolee stolee@gmail.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Elijah Newren newren@gmail.com
cc: Bagas Sanjaya bagasdotme@gmail.com

[newren]

/submit

[gitgitgadget]

Submitted as pull.988.git.1625111177.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-988/newren/ort-perf-batch-14-v1

To fetch this version to local tag pr-988/newren/ort-perf-batch-14-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-988/newren/ort-perf-batch-14-v1

[gitgitgadget]

On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):

On Thu, Jul 01 2021, Elijah Newren via GitGitGadget wrote:

> This series depends textually on ort-perf-batch-12, but is semantically
> independent. (It is both semantically and textually independent of
> ort-perf-batch-13.)

For others following along, that ort-perf-batch-12 is at
https://lore.kernel.org/git/pull.962.v4.git.1623168703.gitgitgadget@gmail.com/#t
& currently marked as 'will merge to next' in what's cooking.

> Most of my previous series dramatically accelerated cases with lots of
> renames, while providing comparatively minor benefits for cases with few or
> no renames. This series is the opposite; it provides huge benefits when
> there are few or no renames, and comparatively smaller (though still quite
> decent) benefits for cases with many uncached renames.

Sounds good, one thing I haven't seen at a glance is how these
performance numbers compare to the merge-recursive backend. Are we in a
state of reaching parity with it, or pulling ahead?

> [...]
> For the testcases mentioned in commit 557ac0350d ("merge-ort: begin
> performance work; instrument with trace2_region_* calls", 2020-10-28), the
> changes in just this series improves the performance as follows:
>
>                      Before Series           After Series
> no-renames:        5.235 s ±  0.042 s   204.2  ms ±  3.0  ms
> mega-renames:      9.419 s ±  0.107 s     1.076 s ±  0.015 s
> just-one-mega:   480.1  ms ±  3.9  ms   364.1  ms ±  7.0  ms
>
>
> As a reminder, before any merge-ort/diffcore-rename performance work, the
> performance results we started with were:
>
> no-renames-am:      6.940 s ±  0.485 s
> no-renames:        18.912 s ±  0.174 s
> mega-renames:    5964.031 s ± 10.459 s
> just-one-mega:    149.583 s ±  0.751 s

I haven't given any of this a detailed look, just a note/question that
(depending on the answer to the "v.s. merge-recursive above") we may
want to consider bumping the default for the diff.renamelimit at some
point along with any major optimizations.

<random musings follow, the tl;dr is above this line :)>

As an aside that we have diff.renamelimit is one of the most "dangerous"
landmines/fork-in-eye/shotgun-to-foot edge cases we have in using diff
as plumbing IMO.

E.g. I somewhat recently had to deal with some 3rd party Go-language
lint plugin that can be configured to enforce lints "as of a commit".
I.e. it does a diff from that commit, sees in any introduced "issues"
are "new", and complains accordingly. The idea is that it allows you to
enforce lints on "only new code", say ignoring the return value of
os.Write(), without insisting that all existing code must be
whitelisted/fixed first.

The problem being two-fold, one that the thing will get slower over time
as we grow history (can't be avoided), but the more subtle one that at
some point we'll bump into the diff.renamelimit, and whatever unlucky
sob does so will find that the lint is now complaining about ALL THE
THINGS, since "old" code is now ending up as "new" to a naïve diff
parser relying on not bumping into the diff.renamelimit.

Arguably bumping the diff.renamelimit would make that sort of problem
worse for plumbing consumers, since they'd have more rope with which to
hang themselves, maybe it's better to step on that landmine early.

Sorry about the digression somewhat pointless but perhaps amusing
digression in the last 4 paragraphs :)

P.S.: I ended up dealing with the Go plugin by not using the "diff"
      feature, but just a one-off giant whitelist of all existing
      instances of stuff it would complain about.

[gitgitgadget]

User Ævar Arnfjörð Bjarmason <avarab@gmail.com> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

On Thu, Jul 1, 2021 at 6:31 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> On Thu, Jul 01 2021, Elijah Newren via GitGitGadget wrote:
>
> > This series depends textually on ort-perf-batch-12, but is semantically
> > independent. (It is both semantically and textually independent of
> > ort-perf-batch-13.)
>
> For others following along, that ort-perf-batch-12 is at
> https://lore.kernel.org/git/pull.962.v4.git.1623168703.gitgitgadget@gmail.com/#t
> & currently marked as 'will merge to next' in what's cooking.
>
> > Most of my previous series dramatically accelerated cases with lots of
> > renames, while providing comparatively minor benefits for cases with few or
> > no renames. This series is the opposite; it provides huge benefits when
> > there are few or no renames, and comparatively smaller (though still quite
> > decent) benefits for cases with many uncached renames.
>
> Sounds good, one thing I haven't seen at a glance is how these
> performance numbers compare to the merge-recursive backend. Are we in a
> state of reaching parity with it, or pulling ahead?

Oh, wow, I really need to improve my wording if that's a question.
When I referred to "the performance results we started with" at the
end, that's referring to the numbers I got with merge-recursive before
I started this journey.  So these were the merge-recursive numbers
(with git-2.29.0 or git-2.30.0):

no-renames-am:      6.940 s ±  0.485 s
no-renames:        18.912 s ±  0.174 s
mega-renames:    5964.031 s ± 10.459 s
just-one-mega:    149.583 s ±  0.751 s

As per commit 557ac0350d ("merge-ort: begin performance work;
instrument with trace2_region_* calls", 2021-01-23), merge-ort was
faster than merge-recursive at the beginning.  But I wasn't content
with that....

Also in the cover letter, I showed the merge-ort numbers before and
after this series:

no-renames:        5.235 s ±  0.042 s   204.2  ms ±  3.0  ms
mega-renames:      9.419 s ±  0.107 s     1.076 s ±  0.015 s
just-one-mega:   480.1  ms ±  3.9  ms   364.1  ms ±  7.0  ms

So yeah, we've pulled a little ahead.  (Like my understatement there?)

Granted, merge-recursive doesn't take quite as long as it used to; it
also benefited from some of my optimizations[1].  Nowhere near as much
as merge-ort benefited, but it still would be about 20x faster on the
cases with "mega" in their name.  So, although today's merge-ort is
~5542.7x faster than git-2.29.0's merge-recursive for a massive set of
renames in a really long rebase sequence, it is probably only a mere
277x faster than today's merge-recursive on the same case.


If you'd like to catch up on the various optimizations, you can find
most of them with this:

git log --reverse --grep=mega-renames --author=newren


[1] In particular, merge-recursive would benefit from these ones:
12bd17521c ("Merge branch 'en/diffcore-rename'", 2021-03-01),
d3a035b055 ("Merge branch 'en/merge-ort-perf'", 2021-02-11), and
a5ac31b5b1 ("Merge branch 'en/diffcore-rename'", 2021-01-25)

> > [...]
> > For the testcases mentioned in commit 557ac0350d ("merge-ort: begin
> > performance work; instrument with trace2_region_* calls", 2020-10-28), the
> > changes in just this series improves the performance as follows:
> >
> >                      Before Series           After Series
> > no-renames:        5.235 s ±  0.042 s   204.2  ms ±  3.0  ms
> > mega-renames:      9.419 s ±  0.107 s     1.076 s ±  0.015 s
> > just-one-mega:   480.1  ms ±  3.9  ms   364.1  ms ±  7.0  ms
> >
> >
> > As a reminder, before any merge-ort/diffcore-rename performance work, the
> > performance results we started with were:
> >
> > no-renames-am:      6.940 s ±  0.485 s
> > no-renames:        18.912 s ±  0.174 s
> > mega-renames:    5964.031 s ± 10.459 s
> > just-one-mega:    149.583 s ±  0.751 s
>
> I haven't given any of this a detailed look, just a note/question that
> (depending on the answer to the "v.s. merge-recursive above") we may
> want to consider bumping the default for the diff.renamelimit at some
> point along with any major optimizations.

I do think we should bump the diff.renamelimit, but not for these
reasons.  We should bump them for the same reasons we bumped them last
time at commit 92c57e5c1d ("bump rename limit defaults (again)",
2011-02-19).

In particular, none of my optimizations made the quadratic portion of
rename detection any faster.  It just added multiple ways for things
to either skip rename detection or be handled by a linear algorithm
instead and not be included in the quadratic loop.  Since
diff.renamelimit only applies to the stuff that goes into the
quadratic portion, most folks will notice that newer versions of git
detect renames with the same limits that older git would have given up
with.

It perhaps also is worth bearing that some of my rename detection
optimizations were specific to three-way merges (and thus help
merge/cherry-pick/rebase/etc. but not diff/log), some of my
optimizations were specific to reapplying long sequences of linear
commits (and thus help cherry-pick or rebase but not diff, log, or
even merge), while others help out all uses of the diff machinery
(diff/log/merge/cherry-pick/rebase/revert/etc.).

> <random musings follow, the tl;dr is above this line :)>
>
> As an aside that we have diff.renamelimit is one of the most "dangerous"
> landmines/fork-in-eye/shotgun-to-foot edge cases we have in using diff
> as plumbing IMO.
>
> E.g. I somewhat recently had to deal with some 3rd party Go-language
> lint plugin that can be configured to enforce lints "as of a commit".
> I.e. it does a diff from that commit, sees in any introduced "issues"
> are "new", and complains accordingly. The idea is that it allows you to
> enforce lints on "only new code", say ignoring the return value of
> os.Write(), without insisting that all existing code must be
> whitelisted/fixed first.
>
> The problem being two-fold, one that the thing will get slower over time
> as we grow history (can't be avoided), but the more subtle one that at
> some point we'll bump into the diff.renamelimit, and whatever unlucky
> sob does so will find that the lint is now complaining about ALL THE
> THINGS, since "old" code is now ending up as "new" to a naïve diff
> parser relying on not bumping into the diff.renamelimit.
>
> Arguably bumping the diff.renamelimit would make that sort of problem
> worse for plumbing consumers, since they'd have more rope with which to
> hang themselves, maybe it's better to step on that landmine early.
>
> Sorry about the digression somewhat pointless but perhaps amusing
> digression in the last 4 paragraphs :)

Certainly a digression, but yes it's amusing.  :-)

Let me add a digression of my own: I got started on this because
people complained they couldn't cherry-pick patches to maintenance
branches, git just messed it all up when it gave up on renames.  I
told them to set diff.renameLimit higher, and they told me that didn't
work.

I didn't believe them.

So, I started on a bit of a journey, somewhere around commit
9f7e4bfa3b ("diff: remove silent clamp of renameLimit", 2017-11-13).
Then after that commit, git could detect renames but took forever
doing so (we needed a renameLimit of 48941 for the very first testcase
I was pointed at).  I dug into that, trying to figure out why
cherry-picking a simple patch that modified 7 files (and not with
particularly big edits) would take something approaching 10 minutes to
complete.  Then I went and found optimizations and rewrote the entire
merge machinery.  Now that same cherry-pick can complete without
bumping diff.renameLimit (1000 is more than enough), and completes in
less than a second (well, with all my optimizations, including the
series not yet submitted).

So, as I said, I didn't believe them when they reported git couldn't
detect renames for their case.  Look at all the work I've done in
order to continue to not believe them.  ;-)

[gitgitgadget]

User Elijah Newren <newren@gmail.com> has been added to the cc: list.

[gitgitgadget]

On the Git mailing list, Elijah Newren wrote (reply to this):

A small correction/clarification...

On Thu, Jul 1, 2021 at 8:04 AM Elijah Newren <newren@gmail.com> wrote:
>
> On Thu, Jul 1, 2021 at 6:31 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >
> > On Thu, Jul 01 2021, Elijah Newren via GitGitGadget wrote:
> >
> > > This series depends textually on ort-perf-batch-12, but is semantically
> > > independent. (It is both semantically and textually independent of
> > > ort-perf-batch-13.)
> >
> > For others following along, that ort-perf-batch-12 is at
> > https://lore.kernel.org/git/pull.962.v4.git.1623168703.gitgitgadget@gmail.com/#t
> > & currently marked as 'will merge to next' in what's cooking.

Yeah, I should have referred to it as en/ort-perf-batch-12, the branch
name Junio used.

> Granted, merge-recursive doesn't take quite as long as it used to; it
> also benefited from some of my optimizations[1].  Nowhere near as much
> as merge-ort benefited, but it still would be about 20x faster on the
> cases with "mega" in their name.  So, although today's merge-ort is
> ~5542.7x faster than git-2.29.0's merge-recursive for a massive set of
> renames in a really long rebase sequence, it is probably only a mere
> 277x faster than today's merge-recursive on the same case.

The 20x number here I was just spit-balling, whereas the other numbers
were measured.  I think 20x may have been a bit too high.  Regardless,
though, to me the important takeaway is not the performance difference
between merge-ort and merge-recursive now (though that's also huge in
merge-ort's favor), but between merge-ort now and merge-recursive when
I started.  If someone really wants me to get the difference between
the two now, I can dig in, but it's annoyingly painful waiting for
merge-recursive to finish tests several times.

[gitgitgadget]

This branch is now known as en/ort-perf-batch-14.

[gitgitgadget]

This patch series was integrated into seen via git@12b1a87.

[gitgitgadget]

This patch series was integrated into seen via git@6813f9b.

[gitgitgadget]

This patch series was integrated into seen via git@d1585ab.

[gitgitgadget]

There was a status update in the "New Topics" section about the branch en/ort-perf-batch-14 on the Git mailing list:

Further optimization on "merge -sort" backend.

[gitgitgadget]

This patch series was integrated into seen via git@d99c6a2.

[gitgitgadget]

This patch series was integrated into seen via git@e89073e.

[gitgitgadget]

This patch series was integrated into seen via git@514cb87.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch en/ort-perf-batch-14 on the Git mailing list:

Further optimization on "merge -sort" backend.

Reviews?

[gitgitgadget]

This patch series was integrated into seen via git@0df9466.

[gitgitgadget]

This patch series was integrated into seen via git@042568b.

[gitgitgadget]

There is an issue in commit 3ca6bea:
Commit checks stopped - the message is too short

[newren]

/submit

[gitgitgadget]

Submitted as pull.988.v2.git.1626204784.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-988/newren/ort-perf-batch-14-v2

To fetch this version to local tag pr-988/newren/ort-perf-batch-14-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-988/newren/ort-perf-batch-14-v2

[gitgitgadget]

User Bagas Sanjaya <bagasdotme@gmail.com> has been added to the cc: list.

[gitgitgadget]

This patch series was integrated into seen via git@3d672c9.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch en/ort-perf-batch-14 on the Git mailing list:

Further optimization on "merge -sort" backend.

Reviews?

[gitgitgadget]

This patch series was integrated into seen via git@cd6e010.

[gitgitgadget]

This patch series was integrated into seen via git@b569790.

[gitgitgadget]

This patch series was integrated into seen via git@25d7d73.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch en/ort-perf-batch-14 on the Git mailing list:

Further optimization on "merge -sort" backend.

Will merge to 'next'.

[gitgitgadget]

Found multiple candidates in gitster/git:
refs/remotes/gitster/en/ort-perf-batch-14
refs/remotes/gitster/temporary/ort-perf-batch-14;

Using the first one.

[gitgitgadget]

This patch series was integrated into seen via git@c348b09.

[gitgitgadget]

This patch series was integrated into next via git@89cfdc4.

[gitgitgadget]

Found multiple candidates in gitster/git:
refs/remotes/gitster/en/ort-perf-batch-14
refs/remotes/gitster/temporary/ort-perf-batch-14;

Using the first one.

[gitgitgadget]

Found multiple candidates in gitster/git:
refs/remotes/gitster/en/ort-perf-batch-14
refs/remotes/gitster/temporary/ort-perf-batch-14;

Using the first one.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch en/ort-perf-batch-14 on the Git mailing list:

Further optimization on "merge -sort" backend.

Will merge to 'master'.

[gitgitgadget]

Found multiple candidates in gitster/git:
refs/remotes/gitster/en/ort-perf-batch-14
refs/remotes/gitster/temporary/ort-perf-batch-14;

Using the first one.

[gitgitgadget]

This patch series was integrated into seen via git@55f6318.

[gitgitgadget]

Found multiple candidates in gitster/git:
refs/remotes/gitster/en/ort-perf-batch-14
refs/remotes/gitster/temporary/ort-perf-batch-14;

Using the first one.

[gitgitgadget]

Found multiple candidates in gitster/git:
refs/remotes/gitster/en/ort-perf-batch-14
refs/remotes/gitster/temporary/ort-perf-batch-14;

Using the first one.

[gitgitgadget]

Found multiple candidates in gitster/git:
refs/remotes/gitster/en/ort-perf-batch-14
refs/remotes/gitster/temporary/ort-perf-batch-14;

Using the first one.

[gitgitgadget]

This patch series was integrated into seen via git@c283a77.

[gitgitgadget]

Found multiple candidates in gitster/git:
refs/remotes/gitster/en/ort-perf-batch-14
refs/remotes/gitster/temporary/ort-perf-batch-14;

Using the first one.

[gitgitgadget]

This patch series was integrated into seen via git@1cbe4a7.

[gitgitgadget]

Found multiple candidates in gitster/git:
refs/remotes/gitster/en/ort-perf-batch-14
refs/remotes/gitster/temporary/ort-perf-batch-14;

Using the first one.

[gitgitgadget]

Found multiple candidates in gitster/git:
refs/remotes/gitster/en/ort-perf-batch-14
refs/remotes/gitster/temporary/ort-perf-batch-14;

Using the first one.

[gitgitgadget]

This patch series was integrated into seen via git@02eb3e8.

[gitgitgadget]

Found multiple candidates in gitster/git:
refs/remotes/gitster/en/ort-perf-batch-14
refs/remotes/gitster/temporary/ort-perf-batch-14;

Using the first one.

[gitgitgadget]

There was a status update in the "Cooking" section about the branch en/ort-perf-batch-14 on the Git mailing list:

Further optimization on "merge -sort" backend.

Will merge to 'master'.

[gitgitgadget]

Found multiple candidates in gitster/git:
refs/remotes/gitster/en/ort-perf-batch-14
refs/remotes/gitster/temporary/ort-perf-batch-14;

Using the first one.

[gitgitgadget]

This patch series was integrated into seen via git@1a6fb01.

[gitgitgadget]

This patch series was integrated into master via git@1a6fb01.

[gitgitgadget]

Closed via 1a6fb01.