revision: use priority queue in limit_list()#2114
Open
spkrka wants to merge 1 commit into
Open
Conversation
limit_list() maintains a date-sorted work queue of commits using a
linked list with commit_list_insert_by_date() for insertion. Each
insertion walks the list to find the right position — O(n) per insert.
In repositories with merge-heavy histories, the symmetric difference
can contain thousands of commits, making this O(n) insertion the
dominant cost.
Replace the sorted linked list with a prio_queue (binary heap). This
gives O(log n) insertion and O(log n) extraction instead of O(n)
insertion and O(1) extraction, which is a net win when the queue is
large.
The still_interesting() and everybody_uninteresting() helpers are
updated to scan the prio_queue's contiguous array instead of walking a
linked list. process_parents() already accepts both a commit_list and
a prio_queue parameter, so the change in limit_list() simply switches
which one is passed.
Benchmark: git rev-list --left-right --count HEAD~N...HEAD
Repository: 2.3M commits, merge-heavy DAG (monorepo)
Best of 5 runs, times in seconds:
commits in
symmetric diff baseline patched speedup
-------------- -------- ------- -------
10 0.01 0.01 1.0x
50 0.01 0.01 1.0x
3751 21.23 8.49 2.5x
4524 21.70 8.29 2.6x
10130 20.10 6.65 3.0x
No change for small traversals; 2.5-3.0x faster when the queue grows
to thousands of commits.
Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Author
|
/submit |
|
Submitted as pull.2114.git.1778777491939.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> Benchmark: git rev-list --left-right --count HEAD~N...HEAD
> Repository: 2.3M commits, merge-heavy DAG (monorepo)
> Best of 5 runs, times in seconds:
>
> commits in
> symmetric diff baseline patched speedup
> -------------- -------- ------- -------
> 10 0.01 0.01 1.0x
> 50 0.01 0.01 1.0x
> 3751 21.23 8.49 2.5x
> 4524 21.70 8.29 2.6x
> 10130 20.10 6.65 3.0x
>
> No change for small traversals; 2.5-3.0x faster when the queue grows
> to thousands of commits.
Impressive.
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> revision: use priority queue in limit_list()
> ...
> This affects any command that triggers limit_list() — i.e., when
> revs->limited is set — including --left-right, --cherry-mark,
> --cherry-pick, --ancestry-path, bisect, and rebase's fork-point
> computation. The practical trigger is git status --ahead-behind on a
> branch that has diverged from upstream in a merge-heavy repository.
I found this description a bit curious. Notably missing from the
above list of revs->limited users is a bog standard A..B and it is
unclear the omission is because that case is not improved and if so
why.
I think a major reason of the omission of A..B from the above is,
despite my recollection that such a range (i.e., any presense of
UNINTERESTING commit) computation _always_ worked on a limited list,
these days we conditionally do not when we have commit graph and we
are showing in --topo-order (which is implicitly enabled when many
options other than --topo-order is in effect) since 1b4d8827
(revision: use generation for A..B --topo-order queries, 2019-05-21).
It might be interesting to extend your benchmark over the same
history with the same command line, perhaps with and without an
explicit "--topo-order" added, in a repository _without_
commit-graph enabled.
Thanks. |
|
Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 5/14/2026 12:51 PM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> limit_list() maintains a date-sorted work queue of commits using a
> linked list with commit_list_insert_by_date() for insertion. Each
> insertion walks the list to find the right position — O(n) per insert.
> In repositories with merge-heavy histories, the symmetric difference
> can contain thousands of commits, making this O(n) insertion the
> dominant cost.
Linear operations are bad, especially when multiplied by linear-ish
loops, causing quadratic behavior.
> Replace the sorted linked list with a prio_queue (binary heap). This
> gives O(log n) insertion and O(log n) extraction instead of O(n)
> insertion and O(1) extraction, which is a net win when the queue is
> large.
Yes, much better.
> The still_interesting() and everybody_uninteresting() helpers are
> updated to scan the prio_queue's contiguous array instead of walking a
> linked list. process_parents() already accepts both a commit_list and
> a prio_queue parameter, so the change in limit_list() simply switches
> which one is passed.
>
> Benchmark: git rev-list --left-right --count HEAD~N...HEAD
> Repository: 2.3M commits, merge-heavy DAG (monorepo)
> Best of 5 runs, times in seconds:
>
> commits in
> symmetric diff baseline patched speedup
> -------------- -------- ------- -------
> 10 0.01 0.01 1.0x
> 50 0.01 0.01 1.0x
> 3751 21.23 8.49 2.5x
> 4524 21.70 8.29 2.6x
> 10130 20.10 6.65 3.0x
>
> No change for small traversals; 2.5-3.0x faster when the queue grows
> to thousands of commits.
This is good. Is there any chance that you could demonstrate this with
any commits in the Git repo? It does have some interesting behavior,
especially around point releases that are independent from the 'master'
branch and thus could have lopsided symmetric differences using well-
established tag names.
> Switching to a prio_queue (binary heap) reduces insertion cost to O(log
> w), bringing total cost to O(N·log w). The practical result on the same
> repository:
>
> commits in
> symmetric diff before after speedup
> -------------- -------- ------- -------
> 3751 21.2s 8.5s 2.5x
> 4524 21.7s 8.3s 2.6x
> 10130 20.1s 6.6s 3.0x
Very nice! I notice that this data is in your cover letter, but
not the commit message. Is that intentional?
> This affects any command that triggers limit_list() — i.e., when
> revs->limited is set — including --left-right, --cherry-mark,
> --cherry-pick, --ancestry-path, bisect, and rebase's fork-point
> computation. The practical trigger is git status --ahead-behind on a
> branch that has diverged from upstream in a merge-heavy repository.
This also impacts 'git log --graph' when there is no serialized
commit-graph file. We are still using limit_list() in that case.
> The change is minimal (+21/−17 lines, single file) because
> process_parents() already accepts both a commit_list and a prio_queue
> parameter — limit_list() just switches which one it passes.
The key logic is turning the initial list into the starting
points for the priority queue and everything else is about
moving types around, it seems.
> @@ -1451,6 +1447,7 @@ static int limit_list(struct rev_info *revs)
> struct commit_list *newlist = NULL;
> struct commit_list **p = &newlist;
> struct commit *interesting_cache = NULL;
> + struct prio_queue queue = { .compare = compare_commits_by_commit_date };
Here, we are _not_ using generation numbers, which is correct
for this case because we are matching the date-based sorting
of the previous list.
> while (original_list) {
> struct commit *commit = pop_commit(&original_list);
> + prio_queue_put(&queue, commit);
> + }
> +
> + while (queue.nr) {
> + struct commit *commit = prio_queue_get(&queue);
> struct object *obj = &commit->object;
This is a fun reuse of lines to take the old "drain the
list as it is being mutated" loop and turn it into "fill
the priority queue" and "drain the priority queue as it
is being mutated"
This code change looks good. No new tests are needed, since
this is a performance-only change. Do any of the tests in
t/perf/ demonstrate this improvement?
Thanks,
-Stolee
|
|
User |
|
Jeff King wrote on the Git mailing list (how to reply to this email): On Thu, May 14, 2026 at 04:51:31PM +0000, Kristofer Karlsson via GitGitGadget wrote:
> @@ -1451,6 +1447,7 @@ static int limit_list(struct rev_info *revs)
> struct commit_list *newlist = NULL;
> struct commit_list **p = &newlist;
> struct commit *interesting_cache = NULL;
> + struct prio_queue queue = { .compare = compare_commits_by_commit_date };
>
> if (revs->ancestry_path_implicit_bottoms) {
> collect_bottom_commits(original_list,
> @@ -1461,6 +1458,11 @@ static int limit_list(struct rev_info *revs)
>
> while (original_list) {
> struct commit *commit = pop_commit(&original_list);
> + prio_queue_put(&queue, commit);
> + }
> +
> + while (queue.nr) {
> + struct commit *commit = prio_queue_get(&queue);
Here we push the whole starting list into the prio-queue, which will let
us pull the commits out in date order. But is the incoming list always
in date order?
If revs->unsorted_input, then we don't sort the initial list. So we'd
now see the commits in a different order, and put them onto newlist in
that different order.
I _think_ it may not matter because we don't call limit_list() when
revs->no_walk is set, and we only have revs->unsorted_input when no_walk
is also set. If that wasn't true, it would get weird when limit_list()
calls process_parents(), which uses commit_list_insert_by_date().
I was on the lookout for this issue particularly because I have another
patch which converts revs.commits to a prio_queue totally. And I
remember running into issues (and the solution is that sometimes the
prio_queue has a NULL comparator and acts like a LIFO queue). But if my
analysis is right above, we can ignore that for now. And if we
eventually move to revs.commits as a prio_queue, then it will just slot
in nicely here (we can drop the queue generation step and just use it
directly).
The rest of the patch looks as I'd expect from what my other patch does.
-Peff |
|
User |
|
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): Thanks for the reviews!
**Junio C Hamano**:
Good question about A..B. Since 1b4d8827 (revision: use
generation for A..B --topo-order queries, 2019-05-21), a plain `A..B`
with commit-graph avoids limit_list() entirely via init_topo_walk().
The commands I listed are those that still force `revs->limited = 1`
even with a commit-graph.
As you suggested, I ran benchmarks without commit-graph. On the same
2.3M-commit repo with `core.commitGraph=false`:
git rev-list --left-right --count HEAD~100...HEAD (3,751 sym-diff)
baseline (no commit-graph): 67.0s
patched (no commit-graph): 43.1s (1.6x speedup)
baseline (with commit-graph): 21.2s
patched (with commit-graph): 8.5s (2.5x speedup)
The gain is smaller without commit-graph because more time goes to
parsing commits from pack, but it's still a meaningful improvement.
**Derrick Stolee**:
Unfortunately git.git's mostly-linear history doesn't
trigger the quadratic behavior (the queue stays narrow). Even with
5,584 commits in the symmetric diff, `--left-right --count` finishes
in ~0.4s on git.git for both baseline and patched. A 50-pair
interleaved run shows no statistically significant difference:
git rev-list --left-right --count v2.47.1...v2.54.0 (git.git, 5,584 commits)
50 interleaved paired runs:
baseline: mean 393ms, stdev 13ms, median 392ms
patched: mean 396ms, stdev 14ms, median 393ms
paired t-test: +2.9ms, t=1.16, p>0.05 (not significant)
There may be a tiny constant-factor overhead (~1%) from the heap's
bookkeeping on narrow queues (sift-up/sift-down vs simple pointer
splice), but it's well within noise and dwarfed by the 2.5-3x win
on wide queues.
The improvement is specific to merge-heavy DAGs where the active
frontier (queue width) grows large.
I also measured `--ancestry-path`, which hits the same limit_list()
bottleneck. 74% of CPU was in commit_list_insert_by_date():
git log --oneline --ancestry-path HEAD~100..HEAD (monorepo, 100 results)
baseline: 16.5s
patched: 3.8s (4.3x speedup)
You're right that `git log --graph` without commit-graph also goes
through limit_list(). I can add that to the description.
Regarding the O(N·w) analysis in the cover letter vs commit message:
I'll move the key points into the commit message in v2.
The existing t/perf tests don't cover this path. p0001 doesn't
use --left-right and p6010 is merge-base specific. I could add a
perf test, though it would need a merge-heavy test repo to show the
difference. Would a synthetic one (like p6010 does) be useful?
**Jeff King**
Confirmed: unsorted_input is only set alongside no_walk, and
limit_list() is called after the no_walk early return.
So the incoming list is always date-sorted when limit_list() runs.
That said, even if unsorted input did reach this code, the prio_queue
maintains its sorted invariant on every prio_queue_put(), so the
output order would still be correct; the heap sorts by commit date
regardless of insertion order.
Your patch to convert revs.commits to a prio_queue sounds like a
natural next step; this change would indeed slot right in (the
initial drain-and-fill loop would just disappear).
On Fri, 15 May 2026 at 06:16, Jeff King <peff@peff.net> wrote:
>
> On Thu, May 14, 2026 at 04:51:31PM +0000, Kristofer Karlsson via GitGitGadget wrote:
>
> > @@ -1451,6 +1447,7 @@ static int limit_list(struct rev_info *revs)
> > struct commit_list *newlist = NULL;
> > struct commit_list **p = &newlist;
> > struct commit *interesting_cache = NULL;
> > + struct prio_queue queue = { .compare = compare_commits_by_commit_date };
> >
> > if (revs->ancestry_path_implicit_bottoms) {
> > collect_bottom_commits(original_list,
> > @@ -1461,6 +1458,11 @@ static int limit_list(struct rev_info *revs)
> >
> > while (original_list) {
> > struct commit *commit = pop_commit(&original_list);
> > + prio_queue_put(&queue, commit);
> > + }
> > +
> > + while (queue.nr) {
> > + struct commit *commit = prio_queue_get(&queue);
>
> Here we push the whole starting list into the prio-queue, which will let
> us pull the commits out in date order. But is the incoming list always
> in date order?
>
> If revs->unsorted_input, then we don't sort the initial list. So we'd
> now see the commits in a different order, and put them onto newlist in
> that different order.
>
> I _think_ it may not matter because we don't call limit_list() when
> revs->no_walk is set, and we only have revs->unsorted_input when no_walk
> is also set. If that wasn't true, it would get weird when limit_list()
> calls process_parents(), which uses commit_list_insert_by_date().
>
>
> I was on the lookout for this issue particularly because I have another
> patch which converts revs.commits to a prio_queue totally. And I
> remember running into issues (and the solution is that sometimes the
> prio_queue has a NULL comparator and acts like a LIFO queue). But if my
> analysis is right above, we can ignore that for now. And if we
> eventually move to revs.commits as a prio_queue, then it will just slot
> in nicely here (we can drop the queue generation step and just use it
> directly).
>
> The rest of the patch looks as I'd expect from what my other patch does.
>
> -Peff |
|
User |
|
Derrick Stolee wrote on the Git mailing list (how to reply to this email): On 5/15/2026 3:47 AM, Kristofer Karlsson wrote:
> Unfortunately git.git's mostly-linear history doesn't
> trigger the quadratic behavior (the queue stays narrow). Even with
> 5,584 commits in the symmetric diff, `--left-right --count` finishes
> in ~0.4s on git.git for both baseline and patched. A 50-pair
> interleaved run shows no statistically significant difference:
>
> git rev-list --left-right --count v2.47.1...v2.54.0 (git.git, 5,584 commits)
> 50 interleaved paired runs:
>
> baseline: mean 393ms, stdev 13ms, median 392ms
> patched: mean 396ms, stdev 14ms, median 393ms
> paired t-test: +2.9ms, t=1.16, p>0.05 (not significant)
Thanks for sharing these details! Consider my curiosity sated.
> The existing t/perf tests don't cover this path. p0001 doesn't
> use --left-right and p6010 is merge-base specific. I could add a
> perf test, though it would need a merge-heavy test repo to show the
> difference. Would a synthetic one (like p6010 does) be useful?
I'm usually interested in encoding ways to repeatedly exercise
these performance gains and preventing regression in the future.
However, you've demonstrated that not all repositories have a
data shape that reveals the performance problem.
If you happen to find a publicly-available repository that shows
this improvement, then documenting the performance benefits for
that repo would be sufficient. I'm familiar with performance
work that doesn't reveal its most important gains until working
with private repositories at the proper scale, so don't sweat
not having a public example.
I don't think it's worth constructing a synthetic repo to
demonstrate this issue. I was hoping that it would be low-
hanging fruit to cover this in the perf test suite, but that
does not seem to be the case.
Thanks,
-Stolee
|
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.
This patch speeds up
limit_list()by 2.5–3x on large, merge-heavyrepositories by replacing a sorted linked list with a priority queue.
The sorted linked list used as a work queue in
limit_list()has O(n)insertion cost per commit, where n is the current queue length (the
"width" of the active walk frontier). In merge-heavy DAGs this
frontier grows wide — profiling on a 2.3M-commit monorepo showed 59%
of total CPU time in
commit_list_insert_by_date(). Total cost isO(N·w) where N is commits walked and w is peak queue width; in
merge-heavy histories w scales with N, approaching O(N²).
Switching to a
prio_queue(binary heap) reduces insertion cost toO(log w), bringing total cost to O(N·log w). The practical result on
the same repository:
This affects any command that triggers
limit_list()— i.e., whenrevs->limitedis set — including--left-right,--cherry-mark,--cherry-pick,--ancestry-path, bisect, and rebase's fork-pointcomputation. The practical trigger is
git status --ahead-behindona branch that has diverged from upstream in a merge-heavy repository.
The change is minimal (+21/−17 lines, single file) because
process_parents()already accepts both acommit_listand aprio_queueparameter —limit_list()just switches which one itpasses.
cc: Derrick Stolee stolee@gmail.com
cc: Jeff King peff@peff.net
cc: Kristofer Karlsson krka@spotify.com