commit-reach: replace queue_has_nonstale with a counter#2124
Draft
spkrka wants to merge 3 commits into
Draft
Conversation
paint_down_to_common() can enqueue the same commit multiple times when it is reached through different parents with different flag combinations. Add an ENQUEUED flag to track whether a commit is currently in the priority queue, and skip it if already present. This change is performance-neutral on its own: the O(n) queue_has_nonstale() scan still dominates the per-iteration cost. However, the deduplication guarantee (each commit appears in the queue at most once) is a prerequisite for the next commit, which replaces that scan with an O(1) nonstale counter. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
spkrka
force-pushed
the
queue-has-nonstale-v3
branch
from
30c6cb6 to
ac9f211
Compare
|
There is an issue in commit f780d59:
|
paint_down_to_common() terminates when every commit remaining in its priority queue is STALE. This was checked by queue_has_nonstale(), which performed an O(n) linear scan of the entire queue on every iteration, resulting in O(n*m) total overhead where n is the queue size and m is the number of commits processed. Replace this with an O(1) nonstale_count that tracks the number of non-stale commits currently in the queue. The counter is incremented by maybe_enqueue() and decremented on dequeue and by mark_stale() when a commit transitions to STALE while still in the queue. Since each commit appears at most once (guaranteed by the ENQUEUED flag from the previous commit), the counter is exact. ahead_behind() also uses queue_has_nonstale() and will be converted in the next commit. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Apply the same nonstale_count optimization from the previous commit to ahead_behind(). This replaces the remaining caller of the O(n) queue_has_nonstale() scan with an O(1) counter check, allowing queue_has_nonstale() to be removed. ahead_behind() already deduplicates queue entries using the PARENT2 flag (via insert_no_dup), so the counter is maintained through insert_no_dup() and mark_stale() using PARENT2 as the queued_flag. Signed-off-by: Kristofer Karlsson <krka@spotify.com>
spkrka
force-pushed
the
queue-has-nonstale-v3
branch
from
ac9f211 to
711a0e2
Compare
spkrka
changed the title
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 free
to 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.
paint_down_to_common() and ahead_behind() terminate when every commit
in their priority queue is STALE. The current check, queue_has_nonstale(),
does an O(n) linear scan of the queue on every iteration, costing O(n*m)
total where n is the queue size and m is the number of commits processed.
This series replaces that scan with an O(1) counter.
Performance measurements with
git merge-base --allandgit for-each-ref --format='%(ahead-behind:...)':The improvement depends on how wide the frontier gets during the
walk. Component imports in the monorepo create wide frontiers where
the queue grows large, making the O(n) scan expensive -- up to 2.5x
speedup for merge-base and 2.4x for ahead-behind. Linear history and
simple merges show no regression.
With a very narrow frontier the counter approach adds a small constant
overhead per iteration (maintaining the counter and the ENQUEUED flag)
compared to the old scan which would return almost immediately. Both
are O(1) and cheap in that scenario, so it should not matter in
practice -- the benchmark numbers above confirm this.