Skip to content

commit-reach: replace queue_has_nonstale with a counter#2124

Open
spkrka wants to merge 3 commits into
gitgitgadget:masterfrom
spkrka:queue-has-nonstale-v3
Open

commit-reach: replace queue_has_nonstale with a counter#2124
spkrka wants to merge 3 commits into
gitgitgadget:masterfrom
spkrka:queue-has-nonstale-v3

Conversation

@spkrka
Copy link
Copy Markdown

@spkrka spkrka commented May 24, 2026

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 --all and
git for-each-ref --format='%(ahead-behind:...)':

git.git (merge-base)
                                          Baseline  Dedup  Dedup+Ctr
seen..next, 33 merge bases:               157ms    165ms    143ms
seen..master, 1 base:                      47ms     40ms     44ms
master..next, 1 base:                      62ms     60ms     63ms

(seen=fe056fe1, next=c82f1880, master=6a4418c3)

Large monorepo, 2.4M commits (merge-base)
                                          Baseline        Dedup+Ctr
component import, wide frontier (1):      8083ms           3778ms
component import, wide frontier (2):      5664ms           4207ms
component import, wide frontier (3):      4558ms           1796ms

Large monorepo, 2.4M commits (ahead-behind)
                                          Baseline        Dedup+Ctr
component import, wide frontier (1):      8216ms           4145ms
component import, wide frontier (2):      6107ms           4528ms
component import, wide frontier (3):      4725ms           1999ms

Linear history (merge-base), no regression:
master vs HEAD~10000:                     4410ms           4180ms
master vs HEAD~50000:                     4412ms           4494ms

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.

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 spkrka force-pushed the queue-has-nonstale-v3 branch from 30c6cb6 to ac9f211 Compare May 24, 2026 13:39
@gitgitgadget
Copy link
Copy Markdown

gitgitgadget Bot commented May 24, 2026

There is an issue in commit f780d59:
commit-reach: replace O(n) queue scan with nonstale counter in paint_down_to_common

  • First line of commit message is too long (> 76 columns)

spkrka added 2 commits May 24, 2026 17:10
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 spkrka force-pushed the queue-has-nonstale-v3 branch from ac9f211 to 711a0e2 Compare May 24, 2026 15:10
@spkrka spkrka changed the title commit-reach: O(1) nonstale termination for paint_down_to_common commit-reach: replace queue_has_nonstale with a counter May 24, 2026
@spkrka spkrka marked this pull request as ready for review May 24, 2026 17:41
@spkrka
Copy link
Copy Markdown
Author

spkrka commented May 24, 2026

/submit

@gitgitgadget
Copy link
Copy Markdown

gitgitgadget Bot commented May 24, 2026

Submitted as pull.2124.git.1779644541.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-2124/spkrka/queue-has-nonstale-v3-v1

To fetch this version to local tag pr-2124/spkrka/queue-has-nonstale-v3-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-2124/spkrka/queue-has-nonstale-v3-v1

Comment thread commit-reach.c
@@ -17,8 +17,9 @@
#define PARENT2 (1u<<17)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Junio C Hamano wrote on the Git mailing list (how to reply to this email):

"Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> diff --git a/commit-reach.c b/commit-reach.c
> index d3a9b3ed6f..c16d4b061c 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -17,8 +17,9 @@
>  #define PARENT2		(1u<<17)
>  #define STALE		(1u<<18)
>  #define RESULT		(1u<<19)
> +#define ENQUEUED	(1u<<20)
>  
> -static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT);
> +static const unsigned all_flags = (PARENT1 | PARENT2 | STALE | RESULT | ENQUEUED);
> ...
> diff --git a/object.h b/object.h
> index d814647ebe..05cbf728e9 100644
> --- a/object.h
> +++ b/object.h
> @@ -74,7 +74,7 @@ void object_array_init(struct object_array *array);
>   * bundle.c:                                        16
>   * http-push.c:                          11-----14
>   * commit-graph.c:                                15
> - * commit-reach.c:                                  16-----19
> + * commit-reach.c:                                  16-------20
>   * builtin/last-modified.c:                         1617
>   * sha1-name.c:                                              20
>   * list-objects-filter.c:                                      21

Not directly the fault of this series, but we'd need to audit and
update this table of bit assignment to match more recent reality.

For example, there no longer exists sha1-name.c but the table claims
that bit 20 is in use for its own purpose, and it being stale makes
it harder to audit and ensure that this new use would not crash with
these existing uses (note. there are other uses of bit 20 in other
subsystems).

FWIW, object-name.c, which was formerly known as sha1-name.c, uses
the bit 20 as ONELINE_SEEN bit, which is used to turn textual object
names like :/string (i.e., commit with that string in its message)
into raw object name, and bit 20 is cleared from all the objects
involved in the search before the helper function returns.
Presumably, once commit-reach.c starts queueing commits and reuses
this bit for its own purpose, we will never try to parse a textual
commit object name to clobber what we thought is ENQUEUED bit,
breaking the code introduced here, so we are probably safe against
its use.

I didn't check all other uses of bit 20, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant