Uh oh!
There was an error while loading. Please reload this page.
commit-reach: early termination for paint_down_to_common() - #2150
Open
newren wants to merge 1 commit into
Open
Conversation
When computing merge bases, paint_down_to_common() can waste enormous time processing STALE commits after the merge bases have already been found. This happens when a merge commit introduces a side branch rooted far back in history, creating a large "generation gap" that forces the algorithm to traverse hundreds of thousands of intermediate commits to exhaustively propagate STALE flags. Add an optimization that checks whether all merge bases have been found and terminates early. The key insight: a new merge base requires both PARENT1 and PARENT2 flags to meet at a commit for the first time. Once either side has no exclusive (non-STALE) commits left in the queue, no new merge bases can appear because that flag can only propagate through STALE commits which already carry both flags. Track exclusive territory with O(1) counters (p1_exclusive_count, p2_exclusive_count, pending_merges_count) that are maintained incrementally: - Incremented when commits are enqueued with exclusive flags - Decremented when commits are dequeued with exclusive flags - Adjusted at paint time when a commit transitions out of exclusive status (e.g., a PARENT2-exclusive commit gains PARENT1) The termination check is placed after parent processing (not before) to ensure the last exclusive commit on a side has its parents fully painted before we decide to stop; a parent might become a merge-base candidate, in which case pending_merges_count is incremented and the check correctly does not fire. Safety: this optimization is gated on corrected_commit_dates_enabled(), which guarantees child_gen > parent_gen and thus monotonic dequeue order (parents never processed before children) for commits covered by the commit graph. The `(p->object.flags & flags) == flags` check on the parent-visit path also prevents enqueueing a second copy of a commit in the same flag state, even through a diamond merge. Together these two properties make the counters exact: each enqueue in an exclusive non-STALE state contributes +1, balanced by either the transition that takes the commit out of that state or by its eventual dequeue. The counters are therefore always >= 0; runtime BUG() checks verify this invariant. We additionally disable the optimization whenever both sides have ever touched a commit with GENERATION_NUMBER_INFINITY (i.e., not in the commit graph), since those sort by raw timestamp which may not be monotonic between parent and child. Detection happens at push time -- both at the initial pushes of `one` and `twos[]` and whenever a parent is enqueued during the walk -- because a cross-side flag transition on an already-dequeued INFINITY parent can otherwise desync the exclusive counts before the gate would fire if checked only at dequeue. When only one side has INFINITY commits, the other side's territory stays entirely in graph space (commit-graph writes are ancestry-closed) and the optimization remains active. In that disabled case the counters are no longer maintained and the BUG() checks are correspondingly suppressed. The optimization can therefore help only when the corrected-date commit graph covers at least one side of the queried tips (e.g., after `git maintenance run` or `git commit-graph write --reachable`). In repo_in_merge_bases_many(), short-circuit when the queried commit is itself one of the references: a commit is trivially reachable from itself, and detecting this avoids invoking paint_down_to_common() with `one == twos[i]`, which would paint PARENT1|PARENT2 on the same commit during the initial pushes and trip the counter invariants described above. Add an `in_merge_bases_many:self` case to t6600-test-reach.sh to guard against this regression. Also guard against duplicate entries within twos[] at the initial-push site: skip pushing twos[i] if PARENT2 is already set on it. Without this guard, a duplicate inflates p2_exclusive_count by one extra, leaving a "phantom" count that can delay the early-termination check (though it never affects correctness, since the other side eventually exhausts its exclusive territory). Add a `get_merge_bases_many:duplicate-twos` case to t6600-test-reach.sh that exercises this with the corrected-date commit graph in place. Also account for an additional flag transition in the counter logic: when a descendant merge-base propagates STALE up to a commit that is still a pending (PARENT1|PARENT2) candidate, the candidate transitions to (PARENT1|PARENT2|STALE) before being dequeued. At its later dequeue the dequeue-time decrement does not fire (the STALE bit causes the (PARENT1|PARENT2)&&!RESULT check to fail), so without compensation pending_merges_count would be permanently inflated by one and the early-termination check would silently never fire for the rest of the walk. Decrement pending_merges_count at the transition site to keep the count exact (guarded by !RESULT to avoid decrementing twice when STALE arrives at an already-recorded merge-base). Add a `get_merge_bases_many:pending-stale` case to t6600-test-reach.sh that constructs a side topology where one merge-base candidate is a descendant of another, so that STALE propagation from the recorded candidate reaches a still-pending one. The 10x10 grid in t6600 does not exercise this transition because no merge-base candidate there is a descendant of another. Add a `get_merge_bases_many:infinity-both-sides` case to t6600-test-reach.sh covering the INFINITY-gate. It builds a tiny side topology with non-monotonic commit dates whose tips are outside the half commit-graph, so both sides enter the walk with INFINITY generation. Without the push-time gate, the walk would underflow p1_exclusive_count when the merge-base candidate propagates STALE up to an already-dequeued INFINITY parent. Broad correctness of the optimization is already covered by the existing t6600-test-reach.sh suite: run_all_modes runs every reach test under four commit-graph configurations -- no graph, full graph, partial graph, and graph without corrected dates -- exercising both the use_early_termination=0 path and the optimization-enabled path, including partial-graph mode where queried tips may be outside the commit graph. The new `infinity-both-sides` case specifically hits the initial push-time INFINITY gate in that partial-graph mode. The few new cases above only target the corner cases this patch specifically introduces. Add p6012-merge-base-deep-side-branch.sh to demonstrate the speedup with a synthetic repository. The test creates a 500K-commit main branch with a side branch forked from commit 10K that has old timestamps. It then merges the side branch at the tip. A PR branch forks from main just before the merge. Computing merge-base(main, pr) must discover that the merge-base is at the tip of main (just below the merge), but without early termination, the algorithm chases STALE flags all the way back through the 490K commits between the side branch fork point and the merge-base. The test also verifies the merge-base result for correctness. Performance on a large production repository: Before: merge-base HEAD PR_head = 0.68s (334K wasted iterations) After: merge-base HEAD PR_head = 0.01s (terminates at ~50 iterations) No regressions on other merge-base queries (all faster or same). Assisted-by: Claude Opus 4.6/4.7 & GPT 5.5 Signed-off-by: Elijah Newren <newren@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a patch I discovered last month (and which Patrick Lühne independently discovered earlier this week), and is now live at GitHub. It feels slightly incomplete in that it aborts when both sides have an infinite generation number -- I think it could be extended to that case, but due to other fires, I didn't have the time to complete it.
...the optimization was also independently discovered by Kristoffer Karlsson; see https://lore.kernel.org/git/CAL71e4PD+zT2jLjjvC7EuYX5z6v_VafnWOUDHeEDBq2LGOK7Pw@mail.gmail.com/T/#mcf16970e250e7caf74f090cc48545b3b947949fd.