prio-queue: use bottom-up sift for extract-min#2132
Open
spkrka wants to merge 2 commits into
Open
Conversation
spkrka
force-pushed
the
cascade-sift-down
branch
9 times, most recently
from
0a3a2b0 to
9ca2fab
Compare
Author
|
/submit |
|
Submitted as pull.2132.git.1780250236304.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:
I'll add René the recipients, as _replace() was added by him as
optimization, so "this new one is functionally equivalent to the
original" somewhat misses the point, even though we may all agree
that the change is a very good one overall in the end when we look
at the entire picture.
> From: Kristofer Karlsson <krka@spotify.com>
>
> Replace the standard sift-down in prio_queue_get() with a
> cascade-down approach.
>
> The standard approach places the last array element at the root,
> then sifts it down. At each level this requires two comparisons
> (left vs right child, then element vs winner) and, when the
> element is larger, a swap (three 16-byte copies).
>
> The cascade approach instead promotes the smaller child into the
> vacant root slot at each level — one comparison and one copy.
> The vacancy sinks to a leaf, where the last array element is
> placed and sifted up if needed — typically zero levels since the
> last array element tends to be large.
>
> In the common case, work per extract drops from 2d comparisons
> + 3d copies to d comparisons + d copies: roughly half the
> comparisons and a third of the data movement. The sift-up phase
> can add work when the last element is smaller than ancestors of
> the leaf vacancy, but this is rare in practice.
>
> Simplify prio_queue_replace() to a plain get+put sequence. This
> is semantically equivalent: the old implementation wrote to slot 0
> and sifted down, which has the same observable effect as removing
> the root and inserting a new element. No caller observes queue
> state between the two operations. The previous implementation
> shared sift_down_root() with get, but the cascade approach no
> longer accommodates that cleanly since sift_down_root() now
> expects the element to reinsert at queue->array[queue->nr], left
> there by prio_queue_get() after decrementing nr. This is fine in
> practice: replace is only called from pop_most_recent_commit()
> (fetch-pack, object-name, walker) and show-branch — none of
> which appear in any hot path.
>
> A synthetic benchmark (10 rounds of 10M put+get cycles, ascending
> integer keys, CPU-pinned, median of 3 runs, same compiler and
> Makefile flags) shows consistent improvement across all queue
> sizes, with no regressions:
>
> queue width baseline cascade speedup
> ------------------------------------------------
> 10 4.32s 3.97s 1.09x
> 100 7.95s 6.49s 1.23x
> 1,000 11.30s 9.66s 1.17x
> 10,000 16.34s 14.15s 1.16x
> 100,000 21.43s 18.66s 1.15x
>
> With descending keys (worst case — the last element always sinks
> to a leaf in both approaches) the cascade still wins slightly
> (1-4%) by replacing swaps with copies, and never regresses.
>
> In end-to-end git commands the improvement is modest because
> sift_down_root is only ~8% of total runtime. Profiling
> rev-list --count on a 2.5M-commit monorepo shows sift_down_root
> dropping from 8.2% to 0.4% of total runtime. The improvement
> scales with DAG width: wider DAGs produce larger priority queues,
> amplifying the per-level savings. In small or narrow repos the
> queues stay shallow and the effect is negligible.
>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> prio-queue: use cascade-down sift for faster extract-min
>
> Hi, I am not sure this is just noise or not but I thought it at least
> was interesting.
>
> I looked into the internals of prio_queue and found it was technically
> doing too much work and could be simplified/optimized. I found I could
> optimize it by ~20% for the common case (adding commits that would
> typically end up far back in the queue) but only ~1% for the reverse
> case (adding things to the front of the prio queue). The average speedup
> is somewhere in between I suppose. That said, this is not really the
> bottleneck so the overall boost seems to be around ~3-4% improvement for
> repos with wide DAGs.
>
> I would normally classify this as not urgent or important, but I think
> the advantage is that the change is very small and simple and it already
> has good unit tests (t/unit-tests/u-prio-queue.c).
>
> With that said, here are the details:
>
> The prio_queue_get impl is based on removing the root entry, then moving
> the very last element into the root slot, then sifting it down into the
> right place. This uses both comparisons between sibling elements in the
> heap as well as comparisons between the element to add and one of the
> siblings. Then it uses swap operations to move things correctly.
>
> This patch instead promotes the smaller child upward at each level,
> leaving a vacancy that sinks to a leaf, then places the removed element
> there with a short sift-up to keep the heap balanced.
>
> We can analytically compare this - for a sift-distance of d we can
> reason about the number of operations to execute.
>
> Before: 2d comparisons + 3d copies
> After: d comparisons + d copies
>
>
> After changing sift_down in this way, the replace operation can't simply
> depend on it anymore, so I reimplemented it as a sequence of get + put.
> This is technically correct but maybe not as efficient. However, I am
> not sure that it matters, since I couldn't see any usage of the replace
> operation in any hot path.
>
> Performance: Profiling git rev-list --count on a 2.5M-commit monorepo
> shows sift_down_root dropping from 8.2% to 0.4% of total runtime,
> effectively eliminated as significant overhead.
>
> Synthetic benchmark 10 rounds of 10M put+get cycles, CPU-pinned, median
> of 3 runs, same compiler and Makefile flags.
>
> Ascending keys (git's typical pattern -- parents have lower priority
> than children):
>
> queue width baseline patched speedup
> 10 4.32s 3.97s 1.09x
> 100 7.95s 6.49s 1.23x
> 1,000 11.30s 9.66s 1.17x
> 10,000 16.34s 14.15s 1.16x
> 100,000 21.43s 18.66s 1.15x
>
>
> Descending keys (worst case — last element always sinks to leaf in both
> approaches):
>
> queue width baseline patched speedup
> 10 4.84s 4.78s 1.01x
> 100 9.43s 9.20s 1.03x
> 1,000 15.28s 14.71s 1.04x
> 10,000 23.61s 23.49s 1.01x
> 100,000 29.16s 28.22s 1.03x
>
>
> No regressions in any scenario.
>
> End-to-end benchmarks
>
> All benchmarks use a benchmark setup of 1 warmup run followed by 10
> timed runs. Each configuration is built from the same source tree and
> tested on the same repo in alternating order.
>
> linux kernel (1.4M commits) — range v5.0..v6.0 (311K commits):
>
> Command baseline patched speedup
> rev-list --count v5.0..v6.0 455ms 440ms 1.04x
>
>
> I also ran it on git.git but did not see any performance diff at all,
> due to the size and narrow DAG.
>
> The improvement scales with DAG width: wider DAGs produce larger
> priority queues, amplifying the per-level savings. In small or narrow
> repositories the priority queues stay shallow and the sift-down cost is
> already negligible, so the change is not noticeable.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2132%2Fspkrka%2Fcascade-sift-down-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2132/spkrka/cascade-sift-down-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/2132
>
> prio-queue.c | 22 ++++++++++++----------
> 1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/prio-queue.c b/prio-queue.c
> index 9748528ce6..18005c43c4 100644
> --- a/prio-queue.c
> +++ b/prio-queue.c
> @@ -62,17 +62,21 @@ static void sift_down_root(struct prio_queue *queue)
> {
> size_t ix, child;
>
> - /* Push down the one at the root */
> - for (ix = 0; ix * 2 + 1 < queue->nr; ix = child) {
> - child = ix * 2 + 1; /* left */
> + for (ix = 0; (child = ix * 2 + 1) < queue->nr; ix = child) {
> if (child + 1 < queue->nr &&
> compare(queue, child, child + 1) >= 0)
> child++; /* use right child */
> + queue->array[ix] = queue->array[child];
> + }
>
> - if (compare(queue, ix, child) <= 0)
> + /* Place queue->array[queue->nr] (left by caller) and sift up. */
> + queue->array[ix] = queue->array[queue->nr];
> + while (ix) {
> + size_t parent = (ix - 1) / 2;
> + if (compare(queue, parent, ix) <= 0)
> break;
> -
> - swap(queue, child, ix);
> + swap(queue, parent, ix);
> + ix = parent;
> }
> }
>
> @@ -89,7 +93,6 @@ void *prio_queue_get(struct prio_queue *queue)
> if (!--queue->nr)
> return result;
>
> - queue->array[0] = queue->array[queue->nr];
> sift_down_root(queue);
> return result;
> }
> @@ -111,8 +114,7 @@ void prio_queue_replace(struct prio_queue *queue, void *thing)
> queue->array[queue->nr - 1].ctr = queue->insertion_ctr++;
> queue->array[queue->nr - 1].data = thing;
> } else {
> - queue->array[0].ctr = queue->insertion_ctr++;
> - queue->array[0].data = thing;
> - sift_down_root(queue);
> + prio_queue_get(queue);
> + prio_queue_put(queue, thing);
> }
> }
>
> base-commit: c69baaf57ba26cf117c2b6793802877f19738b0d |
|
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/prio-queue.c b/prio-queue.c
> index 9748528ce6..18005c43c4 100644
> --- a/prio-queue.c
> +++ b/prio-queue.c
> @@ -62,17 +62,21 @@ static void sift_down_root(struct prio_queue *queue)
> {
> size_t ix, child;
>
> - /* Push down the one at the root */
> - for (ix = 0; ix * 2 + 1 < queue->nr; ix = child) {
> - child = ix * 2 + 1; /* left */
> + for (ix = 0; (child = ix * 2 + 1) < queue->nr; ix = child) {
> if (child + 1 < queue->nr &&
> compare(queue, child, child + 1) >= 0)
> child++; /* use right child */
> + queue->array[ix] = queue->array[child];
> + }
>
> - if (compare(queue, ix, child) <= 0)
> + /* Place queue->array[queue->nr] (left by caller) and sift up. */
> + queue->array[ix] = queue->array[queue->nr];
Here we always sift/bubble up the last element.
I am wondering if it makes sense to teach sift_down_root to take an
extra argument, "struct prio_queue_entry entry" (passed by value)
and sift/bubble it up, not always queue->array[queue->nr], and ...
> + while (ix) {
> + size_t parent = (ix - 1) / 2;
> + if (compare(queue, parent, ix) <= 0)
> break;
> -
> - swap(queue, child, ix);
> + swap(queue, parent, ix);
> + ix = parent;
> }
> }
>
> @@ -89,7 +93,6 @@ void *prio_queue_get(struct prio_queue *queue)
> if (!--queue->nr)
> return result;
>
> - queue->array[0] = queue->array[queue->nr];
> sift_down_root(queue);
> return result;
> }
> @@ -111,8 +114,7 @@ void prio_queue_replace(struct prio_queue *queue, void *thing)
> queue->array[queue->nr - 1].ctr = queue->insertion_ctr++;
> queue->array[queue->nr - 1].data = thing;
> } else {
> - queue->array[0].ctr = queue->insertion_ctr++;
> - queue->array[0].data = thing;
> - sift_down_root(queue);
> + prio_queue_get(queue);
> + prio_queue_put(queue, thing);
... update this part in the else clause to do something like
struct prio_queue_entry entry;
entry.ctr = queue->insertion_ctr++;
entry.data = thing;
sift_down_root(queue, entry);
to retain the optimization? It would perform a single cascade-down
sift, followed by a single sift-up, so it would save a comparison, a
copy, and a swap in the worset case compared to the get+put sequence?
Of course, the original sift_down_root() caller (i.e. prio_queue_get())
needs to pass queue->array[queue->nr] as the second parameter to match.
> }
> }
>
> base-commit: c69baaf57ba26cf117c2b6793802877f19738b0d |
|
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): Thanks for the quick and very valid feedback! I already started
investigating - I think I was too quick (and wrong) when I reasoned
about the replace operation.I will rework it a bit and come back with
a patch version 2 soon that ensures that neither get and replace have
regressed in any way.
- Kristofer
On Mon, 1 Jun 2026 at 08:16, Junio C Hamano <gitster@pobox.com> wrote:
>
> "Kristofer Karlsson via GitGitGadget" <gitgitgadget@gmail.com>
> writes:
>
> > diff --git a/prio-queue.c b/prio-queue.c
> > index 9748528ce6..18005c43c4 100644
> > --- a/prio-queue.c
> > +++ b/prio-queue.c
> > @@ -62,17 +62,21 @@ static void sift_down_root(struct prio_queue *queue)
> > {
> > size_t ix, child;
> >
> > - /* Push down the one at the root */
> > - for (ix = 0; ix * 2 + 1 < queue->nr; ix = child) {
> > - child = ix * 2 + 1; /* left */
> > + for (ix = 0; (child = ix * 2 + 1) < queue->nr; ix = child) {
> > if (child + 1 < queue->nr &&
> > compare(queue, child, child + 1) >= 0)
> > child++; /* use right child */
> > + queue->array[ix] = queue->array[child];
> > + }
> >
> > - if (compare(queue, ix, child) <= 0)
> > + /* Place queue->array[queue->nr] (left by caller) and sift up. */
> > + queue->array[ix] = queue->array[queue->nr];
>
> Here we always sift/bubble up the last element.
>
> I am wondering if it makes sense to teach sift_down_root to take an
> extra argument, "struct prio_queue_entry entry" (passed by value)
> and sift/bubble it up, not always queue->array[queue->nr], and ...
>
> > + while (ix) {
> > + size_t parent = (ix - 1) / 2;
> > + if (compare(queue, parent, ix) <= 0)
> > break;
> > -
> > - swap(queue, child, ix);
> > + swap(queue, parent, ix);
> > + ix = parent;
> > }
> > }
> >
> > @@ -89,7 +93,6 @@ void *prio_queue_get(struct prio_queue *queue)
> > if (!--queue->nr)
> > return result;
> >
> > - queue->array[0] = queue->array[queue->nr];
> > sift_down_root(queue);
> > return result;
> > }
> > @@ -111,8 +114,7 @@ void prio_queue_replace(struct prio_queue *queue, void *thing)
> > queue->array[queue->nr - 1].ctr = queue->insertion_ctr++;
> > queue->array[queue->nr - 1].data = thing;
> > } else {
> > - queue->array[0].ctr = queue->insertion_ctr++;
> > - queue->array[0].data = thing;
> > - sift_down_root(queue);
> > + prio_queue_get(queue);
> > + prio_queue_put(queue, thing);
>
> ... update this part in the else clause to do something like
>
> struct prio_queue_entry entry;
> entry.ctr = queue->insertion_ctr++;
> entry.data = thing;
> sift_down_root(queue, entry);
>
> to retain the optimization? It would perform a single cascade-down
> sift, followed by a single sift-up, so it would save a comparison, a
> copy, and a swap in the worset case compared to the get+put sequence?
>
> Of course, the original sift_down_root() caller (i.e. prio_queue_get())
> needs to pass queue->array[queue->nr] as the second parameter to match.
>
> > }
> > }
> >
> > base-commit: c69baaf57ba26cf117c2b6793802877f19738b0d |
|
User |
Author
|
/submit |
|
Submitted as pull.2132.v2.git.1780301856444.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
This patch series was integrated into seen via git@3bbc3b3. |
|
René Scharfe wrote on the Git mailing list (how to reply to this email): On 6/1/26 10:17 AM, Kristofer Karlsson via GitGitGadget wrote:
>
> Changes since v1:
>
> * Kept sift_down_root() and prio_queue_replace() completely unchanged,
> preserving René's optimization that avoids the get+put overhead for
> replace. The cascade approach now only applies to prio_queue_get().
The prospect of no longer needing prio_queue_replace() had me excited in
round 1. The benchmarks from commits that added its callers [1][2][3]
did show performance regressions with your patch 1 plus changes to
revert prio_queue_peek()+prio_queue_replace() to prio_queue_get()+
prio_queue_put(), but for two of them low enough to be in the noise.
'git describe $(git rev-list v2.41.0..v2.47.0)' took a 50%+ hit, though.
[1] a79e3519d6 (commit: use prio_queue_replace() in pop_most_recent_commit(), 2025-07-18)
[2] 08bb69d70f (describe: use prio_queue_replace(), 2025-08-03)
[3] abf05d856f (show-branch: use prio_queue, 2025-12-26)
> * Extracted the new logic into a separate sift_up_rebalance() function
> rather than inlining it in prio_queue_get().
>
> * Updated benchmark numbers for ascending, descending and random
> insertion ordering. No regressions in any scenario.
I don't see any regression for the benchmarks mentioned above with
patch 2 alone, unsurprisingly. The describe command still takes that
50%+ performance hit after reverting [2] on top.
Would you be interested in benchmarking the following patch for making
prio_queue_replace() unnecessary by doing its optimization
automatically? I get a 1% performance hit for the describe command
that I can't explain. And it leaves the heap unbalanced after a
prio_queue_get(), which complicates things, so I found it lacking.
But I wonder how it stacks up against your cascade approach for your
use case and if there's anything to salvage.
René
---
prio-queue.c | 60 +++++++++++++++++++++++++++++++++++++++++-------------------
prio-queue.h | 1 +
2 files changed, 42 insertions(+), 19 deletions(-)
diff --git a/prio-queue.c b/prio-queue.c
index 9748528ce6..ba6b460a46 100644
--- a/prio-queue.c
+++ b/prio-queue.c
@@ -34,12 +34,46 @@ void clear_prio_queue(struct prio_queue *queue)
queue->nr = 0;
queue->alloc = 0;
queue->insertion_ctr = 0;
+ queue->sift_down_root_pending = false;
+}
+
+static void sift_down_root(struct prio_queue *queue)
+{
+ size_t ix, child;
+
+ /* Push down the one at the root */
+ for (ix = 0; ix * 2 + 1 < queue->nr; ix = child) {
+ child = ix * 2 + 1; /* left */
+ if (child + 1 < queue->nr &&
+ compare(queue, child, child + 1) >= 0)
+ child++; /* use right child */
+
+ if (compare(queue, ix, child) <= 0)
+ break;
+
+ swap(queue, child, ix);
+ }
+ queue->sift_down_root_pending = false;
}
void prio_queue_put(struct prio_queue *queue, void *thing)
{
size_t ix, parent;
+ if (queue->sift_down_root_pending) {
+ /*
+ * Restore the original heap size. The last item is
+ * still in the right place.
+ */
+ queue->nr++;
+
+ /* Now fill the hole at the root with the new item. */
+ queue->array[0].ctr = queue->insertion_ctr++;
+ queue->array[0].data = thing;
+ sift_down_root(queue);
+ return;
+ }
+
/* Append at the end */
ALLOC_GROW(queue->array, queue->nr + 1, queue->alloc);
queue->array[queue->nr].ctr = queue->insertion_ctr++;
@@ -58,24 +92,6 @@ void prio_queue_put(struct prio_queue *queue, void *thing)
}
}
-static void sift_down_root(struct prio_queue *queue)
-{
- size_t ix, child;
-
- /* Push down the one at the root */
- for (ix = 0; ix * 2 + 1 < queue->nr; ix = child) {
- child = ix * 2 + 1; /* left */
- if (child + 1 < queue->nr &&
- compare(queue, child, child + 1) >= 0)
- child++; /* use right child */
-
- if (compare(queue, ix, child) <= 0)
- break;
-
- swap(queue, child, ix);
- }
-}
-
void *prio_queue_get(struct prio_queue *queue)
{
void *result;
@@ -85,12 +101,14 @@ void *prio_queue_get(struct prio_queue *queue)
if (!queue->compare)
return queue->array[--queue->nr].data; /* LIFO */
+ if (queue->sift_down_root_pending)
+ sift_down_root(queue);
result = queue->array[0].data;
if (!--queue->nr)
return result;
queue->array[0] = queue->array[queue->nr];
- sift_down_root(queue);
+ queue->sift_down_root_pending = true;
return result;
}
@@ -100,6 +118,8 @@ void *prio_queue_peek(struct prio_queue *queue)
return NULL;
if (!queue->compare)
return queue->array[queue->nr - 1].data;
+ if (queue->sift_down_root_pending)
+ sift_down_root(queue);
return queue->array[0].data;
}
@@ -111,6 +131,8 @@ void prio_queue_replace(struct prio_queue *queue, void *thing)
queue->array[queue->nr - 1].ctr = queue->insertion_ctr++;
queue->array[queue->nr - 1].data = thing;
} else {
+ if (queue->sift_down_root_pending)
+ sift_down_root(queue);
queue->array[0].ctr = queue->insertion_ctr++;
queue->array[0].data = thing;
sift_down_root(queue);
diff --git a/prio-queue.h b/prio-queue.h
index da7fad2f1f..5977fba438 100644
--- a/prio-queue.h
+++ b/prio-queue.h
@@ -32,6 +32,7 @@ struct prio_queue {
void *cb_data;
size_t alloc, nr;
struct prio_queue_entry *array;
+ bool sift_down_root_pending;
};
/*
|
|
User |
|
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): On Tue, 2 Jun 2026 at 18:37, René Scharfe <l.s.r@web.de> wrote:
>
> Would you be interested in benchmarking the following patch for making
> prio_queue_replace() unnecessary by doing its optimization
> automatically? I get a 1% performance hit for the describe command
> that I can't explain. And it leaves the heap unbalanced after a
> prio_queue_get(), which complicates things, so I found it lacking.
> But I wonder how it stacks up against your cascade approach for your
> use case and if there's anything to salvage.
>
> René
Thank you for the detailed feedback and the patch! It was very
helpful to have a concrete alternative to compare against.
I spent some time benchmarking the different approaches on a
large monorepo with a wide DAG.
All measurements include the nonstale O(1) tracking from my other
series as a common base, since that dominates the merge-base path.
The approaches I compared:
1. cascade-only: the sift_up_rebalance from this patch (v2)
2. rene-lazy: your deferred sift_down_root patch
3. cascade+lazy: cascade for unfused gets, lazy fusion for
get+put pairs
Results (10 runs, 1 warmup, CPU pinned to performance):
merge-base --all master master~1000 (~4s workload):
cascade-only 4.18s (median)
rene-lazy 4.25s
cascade+lazy 4.24s
rev-list --count master~1000..master (~3.8s workload):
cascade-only 3.86s
rene-lazy 3.75s
cascade+lazy 3.74s
The lazy approaches show a small win on rev-list (~3%) where get+put
pairs are common in limit_list. On merge-base --all, everything is
within noise, the prio_queue is a small fraction of total runtime
there. Combining cascade with lazy fusion didn't produce additional
gains beyond what each gives individually.
Looking at your patch, I think the deferred sift-down logic is
essentially the same optimization as the lazy_queue wrapper you
wrote for describe.c - both defer the work from get and fuse it
with a following put. So I'd be hesitant to add a second form of
that deferral directly into prio_queue when lazy_queue already
"owns" that responsibility as a wrapper.
That said, I think it would make sense to fold lazy_queue entirely
into prio_queue. It's an optimization that never hurts as far as I can
tell, and it would simplify several callers. pop_most_recent_commit
and show-branch both independently re-implement the same
peek+replace pattern that lazy_queue formalizes. Making it automatic
in prio_queue would clean up all of them.
I have a local branch exploring that direction. Maybe it makes more
sense to do the lazy_queue fold first, and then see if the cascade
change is still worth adding on top?
Either way, I think the two directions are complementary - cascade
reduces comparisons per sift, while lazy fusion can eliminate full
rebalance cycles.
I'm on a company offsite now so I may be slow to answer, but I will
definitely resume this when I get back home.
- Kristofer |
|
User |
|
This branch is now known as |
|
This patch series was integrated into seen via git@d8ee62b. |
|
This patch series was integrated into seen via git@772137d. |
|
There was a status update in the "Cooking" section about the branch prio_queue_get() has been optimized by using a cascade-down approach (promoting the smaller child at each level and sifting up the last element from the leaf vacancy), which halves the number of comparisons per extract-min operation in the common case. Expecting a reroll. cf. <CAL71e4Ob-B5MJ5DPY+_tzpj6nyrbQ5WutxED2T93SWJV6kJGPA@mail.gmail.com> source: <pull.2132.v2.git.1780301856444.gitgitgadget@gmail.com> |
|
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): I did some more benchmarking to understand how these approaches
interact, with four variants based on origin/next on my large monorepo:
1. base: next as-is
2. cascade: base + sift_up_rebalance from this patch (v2)
3. lazy-fold: base + lazy get fusion folded into prio_queue
4. cascade+lazy: both combined
Note that alt 3 is not yet shared with the mailing list so it's hard for you
to reason about it, though it's quite straightforward. I will submit a new
patch for that one soon, not necessarily with the primary goal to merge it,
but rather show how it is implemented.
merge-base --all master master~1000:
base 4.27s
cascade 4.07s (1.05x)
lazy-fold 4.12s (1.03x)
cascade+lazy 4.01s (1.06x)
rev-list --count master~1000..master:
base 3.60s
cascade 3.35s (1.08x)
lazy-fold 3.37s (1.07x)
cascade+lazy 3.30s (1.09x)
So both optimizations are valuable both on their own, and when combined,
which I think helps to reason about it. This cascading sift seems to have a
larger effect, but folding lazy_queue into prio_queue also speeds up other
use cases and simplifies the code a bit.
Based on this, my (very subjective) approach would be:
1. Land this cascade patch first since it's a pure algorithmic improvement,
2. Follow up with a separate patch that folds lazy_queue into
prio_queue. Will post it separately soon, as I mentioned.
- Kristofer |
|
There was a status update in the "Cooking" section about the branch 'prio_queue_get()' has been optimized by using a cascade-down approach (promoting the smaller child at each level and sifting up the last element from the leaf vacancy), which halves the number of comparisons per extract-min operation in the common case. On hold, waiting for kk/prio-queue-get-put-fusion to land first. cf. <CAL71e4MYNiScZjTwkApjDAjRh2LM0_SP59h5HCTywV-Pua03tw@mail.gmail.com> source: <pull.2132.v2.git.1780301856444.gitgitgadget@gmail.com> |
|
René Scharfe wrote on the Git mailing list (how to reply to this email): On 7/6/26 11:52 PM, Kristofer Karlsson wrote:
> On Sun, 7 Jun 2026 at 09:30, René Scharfe <l.s.r@web.de> wrote:
>>
>> So I guess we keep the full sift-down for prio_queue_replace(), knowing
>> that sometimes we have a lot of items that end up at or close to the
>> root of the heap.
>
> The lazy-fold series (kk/prio-queue-get-put-fusion) is in next now.
> I rebased this cascade patch on top of it to check if it's still
> useful.
>
> With lazy-fold in place the regression scenario you identified
> is resolved. The only remaining change is in flush_get(),
> where unfused gets now cascade instead of sifting down:
>
> - queue->array[0] = queue->array[--queue->nr_];
> - sift_down_root(queue);
> + --queue->nr_;
> + sift_up_rebalance(queue);
>
> plus the ~20-line sift_up_rebalance() implementation.
>
> I benchmarked this on the linux kernel repo and on a large
> merge-heavy repo.
>
> The results are consistent: a real but small 1-2% end-to-end
> improvement across commands. A prio-queue microbenchmark
> would likely show a larger difference, but the queue
> is only a fraction of the total work in any real git operation.
>
> The lazy-fold optimization cannibalized some of the value here,
> so cascade only helps the remaining unfused gets. As you observed,
> cascade is better there, but there are fewer of them now that there
> is more fusing happening.
>
> I am on the fence about whether 1-2% end-to-end justifies adding
> another sift function. If you (René and Junio) think the benefit
> is too small for the code cost, I am happy to drop this patch.
> Otherwise I can submit a small reroll on top of
> kk/prio-queue-get-put-fusion (or rather next, in practice).
tl;dr: Yes, please, but I'm biased.
The text size of prio-queue.o on Apple silicon increases from 1351 to
1563 bytes for me, 212 bytes or 16% more. OK.
It makes intuitive sense to find the new position of the last item by
searching from the bottom up instead of from the top down. Timings
confirm it. Are there pathologic cases that perform worse, though? I
don't see how to construct one. It would require an unbalanced heap,
where the bottom items from one branch would rise high in other
branches. Is this even possible?
For a full drain (only _get(), no _put()) of up to 12 items the answer
is no, at least. Cascade never needs more comparisons for any
permutation; test code below. Here are the aggregate numbers:
next cascade
n min max mean min max mean
2 0 0 0.0 0 0 0.0
3 1 1 1.0 1 1 1.0
4 3 3 3.0 3 3 3.0
5 5 6 5.8 5 6 5.6
6 7 10 8.7 7 9 8.0
7 10 14 12.0 9 12 10.9
8 14 18 16.3 12 16 13.9
9 18 23 20.9 15 20 17.4
10 22 29 25.5 18 24 20.7
11 26 35 30.5 21 28 24.4
12 30 41 35.5 24 33 27.9
sift_up_rebalance() is a combination of sift_down_root() with an empty
root and the bubble-up operation from prio_queue_put(). The latter can
easily be factored out into a sift-up function, reducing code
duplication.
Extending sift_down_root() to deal with an empty root would be easy as
well, but also a bit tricky to avoid pointless checks for each caller.
Not sure it's worth it. Like this perhaps?
static inline size_t sift_down_root(struct prio_queue *queue, bool empty)
{
size_t ix, child;
/* Push down the one at the root */
for (ix = 0; ix * 2 + 1 < queue->nr_; ix = child) {
child = ix * 2 + 1; /* left */
if (child + 1 < queue->nr_ &&
compare(queue, child, child + 1) >= 0)
child++; /* use right child */
if (empty)
queue->array[ix] = queue->array[child];
else if (compare(queue, ix, child) <= 0)
break;
else
swap(queue, child, ix);
}
return ix;
}
Anyway, my point is that it's not "adding another sift function", but
remixing existing ones, which I only count as half. :)
I'd very much like to see this go in because it seems to be strictly
faster, makes intuitive sense and adds only little code. I didn't
find this method used anywhere else, which is a warning sign, but I
can't find any catch.
René
$ for n in $(seq 2 2)
do
t/helper/test-tool prio-queue permute get $n |
awk -v n=$n -v max=0 '
{sum+=$2}
max < $2 {max=$2}
!min || min > $2 {min=$2}
END {printf "%2d %3d %3d %5.1f \n", n, min, max, sum/NR}
'
done
---
Makefile | 1 +
t/helper/test-prio-queue.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++
t/helper/test-tool.c | 1 +
t/helper/test-tool.h | 1 +
4 files changed, 94 insertions(+)
diff --git a/Makefile b/Makefile
index 1f3f099f5c5..ba7d293cf5f 100644
--- a/Makefile
+++ b/Makefile
@@ -843,6 +843,7 @@ TEST_BUILTINS_OBJS += test-partial-clone.o
TEST_BUILTINS_OBJS += test-path-utils.o
TEST_BUILTINS_OBJS += test-path-walk.o
TEST_BUILTINS_OBJS += test-pcre2-config.o
+TEST_BUILTINS_OBJS += test-prio-queue.o
TEST_BUILTINS_OBJS += test-pkt-line.o
TEST_BUILTINS_OBJS += test-proc-receive.o
TEST_BUILTINS_OBJS += test-progress.o
diff --git a/t/helper/test-prio-queue.c b/t/helper/test-prio-queue.c
new file mode 100644
index 00000000000..c175021b12b
--- /dev/null
+++ b/t/helper/test-prio-queue.c
@@ -0,0 +1,91 @@
+#include "test-tool.h"
+#include "prio-queue.h"
+
+/* Generate all permutations using Heap's algorithm. */
+static int permute_ints(size_t n, void (*fn)(int *, size_t))
+{
+ int *arr;
+ size_t *c;
+
+ ALLOC_ARRAY(arr, n);
+ for (size_t i = 0; i < n; i++)
+ arr[i] = i + 1;
+ CALLOC_ARRAY(c, n);
+
+ fn(arr, n);
+ for (size_t i = 1; i < n; i++) {
+ if (c[i] < i) {
+ SWAP(arr[i & 1 ? c[i] : 0], arr[i]);
+ fn(arr, n);
+ c[i]++;
+ i = 0;
+ } else {
+ c[i] = 0;
+ }
+ }
+
+ free(arr);
+ free(c);
+
+ return 0;
+}
+
+static uintmax_t nr_of_compares;
+
+static int compare_ints(const void *a_, const void *b_, void *cb_data UNUSED)
+{
+ const int *a = a_;
+ const int *b = b_;
+ nr_of_compares++;
+ return *a - *b;
+}
+
+static void report(const char *name, const int *arr, size_t n)
+{
+ printf("%s %"PRIuMAX" for", name, nr_of_compares);
+ for (size_t i = 0; i < n; i++)
+ printf(" %d", arr[i]);
+ putchar('\n');
+}
+
+static void get_permutation(int *arr, size_t n)
+{
+ static struct prio_queue queue = { compare_ints };
+
+ for (size_t i = 0; i < n; i++)
+ prio_queue_put(&queue, &arr[i]);
+
+ nr_of_compares = 0;
+ for (size_t i = 0; i < n; i++)
+ prio_queue_get(&queue);
+
+ report("get", arr, n);
+}
+
+static void put_permutation(int *arr, size_t n)
+{
+ struct prio_queue queue = { compare_ints };
+
+ nr_of_compares = 0;
+ for (size_t i = 0; i < n; i++)
+ prio_queue_put(&queue, &arr[i]);
+
+ report("put", arr, n);
+
+ clear_prio_queue(&queue);
+}
+
+int cmd__prio_queue(int argc, const char **argv)
+{
+ if (argc == 4 && !strcmp(argv[1], "permute")) {
+ size_t n = strtoul(argv[3], NULL, 10);
+ if (!strcmp(argv[2], "get"))
+ return permute_ints(n, get_permutation);
+ if (!strcmp(argv[2], "put"))
+ return permute_ints(n, put_permutation);
+ }
+
+ fprintf(stderr, "usage: test-tool prio-queue permute get <n>\n");
+ fprintf(stderr, " or: test-tool prio-queue permute put <n>\n");
+ return 129;
+}
diff --git a/t/helper/test-tool.c b/t/helper/test-tool.c
index b71a22b43bb..69352f541f4 100644
--- a/t/helper/test-tool.c
+++ b/t/helper/test-tool.c
@@ -57,6 +57,7 @@ static struct test_cmd cmds[] = {
{ "path-walk", cmd__path_walk },
{ "pcre2-config", cmd__pcre2_config },
{ "pkt-line", cmd__pkt_line },
+ { "prio-queue", cmd__prio_queue },
{ "proc-receive", cmd__proc_receive },
{ "progress", cmd__progress },
{ "reach", cmd__reach },
diff --git a/t/helper/test-tool.h b/t/helper/test-tool.h
index f2885b33d58..ab0d3e01d1e 100644
--- a/t/helper/test-tool.h
+++ b/t/helper/test-tool.h
@@ -50,6 +50,7 @@ int cmd__path_utils(int argc, const char **argv);
int cmd__path_walk(int argc, const char **argv);
int cmd__pcre2_config(int argc, const char **argv);
int cmd__pkt_line(int argc, const char **argv);
+int cmd__prio_queue(int argc, const char **argv);
int cmd__proc_receive(int argc, const char **argv);
int cmd__progress(int argc, const char **argv);
int cmd__reach(int argc, const char **argv);
|
|
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): On Wed, 8 Jul 2026 at 12:44, René Scharfe <l.s.r@web.de> wrote:
>
> tl;dr: Yes, please, but I'm biased.
>
> The text size of prio-queue.o on Apple silicon increases from 1351 to
> 1563 bytes for me, 212 bytes or 16% more. OK.
>
> It makes intuitive sense to find the new position of the last item by
> searching from the bottom up instead of from the top down. Timings
> confirm it. Are there pathologic cases that perform worse, though? I
> don't see how to construct one. It would require an unbalanced heap,
> where the bottom items from one branch would rise high in other
> branches. Is this even possible?
Agreed, I also struggle to come up with such a case. Perhaps
theoretically possible to construct, but would not invalidate
the general heuristic?
> For a full drain (only _get(), no _put()) of up to 12 items the answer
> is no, at least. Cascade never needs more comparisons for any
> permutation; test code below. Here are the aggregate numbers:
>
> next cascade
> n min max mean min max mean
> 2 0 0 0.0 0 0 0.0
> 3 1 1 1.0 1 1 1.0
> 4 3 3 3.0 3 3 3.0
> 5 5 6 5.8 5 6 5.6
> 6 7 10 8.7 7 9 8.0
> 7 10 14 12.0 9 12 10.9
> 8 14 18 16.3 12 16 13.9
> 9 18 23 20.9 15 20 17.4
> 10 22 29 25.5 18 24 20.7
> 11 26 35 30.5 21 28 24.4
> 12 30 41 35.5 24 33 27.9
I am sincerely grateful that you took the time to analyze it
to this level of detail. Very nice analysis and data!
> sift_up_rebalance() is a combination of sift_down_root() with an empty
> root and the bubble-up operation from prio_queue_put(). The latter can
> easily be factored out into a sift-up function, reducing code
> duplication.
Good point! I am not sure how messy this gets in practice, but I
will see if I can implement this split for the next patch.
> Extending sift_down_root() to deal with an empty root would be easy as
> well, but also a bit tricky to avoid pointless checks for each caller.
> Not sure it's worth it. Like this perhaps?
>
> static inline size_t sift_down_root(struct prio_queue *queue, bool empty)
> {
> size_t ix, child;
>
> /* Push down the one at the root */
> for (ix = 0; ix * 2 + 1 < queue->nr_; ix = child) {
> child = ix * 2 + 1; /* left */
> if (child + 1 < queue->nr_ &&
> compare(queue, child, child + 1) >= 0)
> child++; /* use right child */
>
> if (empty)
> queue->array[ix] = queue->array[child];
> else if (compare(queue, ix, child) <= 0)
> break;
> else
> swap(queue, child, ix);
> }
> return ix;
> }
Yes, something like that would work, but I agree -- ideally we
could have something that's even nicer and avoids the boolean flag
for split behavior.
> Anyway, my point is that it's not "adding another sift function", but
> remixing existing ones, which I only count as half. :)
>
> I'd very much like to see this go in because it seems to be strictly
> faster, makes intuitive sense and adds only little code. I didn't
> find this method used anywhere else, which is a warning sign, but I
> can't find any catch.
Thanks, I think that is enough motivation for me to at least attempt
another version and then it will be easier to reason about dropping
or keeping.
I am not sure why it's a warning sign to have no other usages,
especially when it's a file local static function. I guess it could
be inlined instead (though I would not prefer that).
Thanks for the very thorough and insightful review,
Kristofer |
|
René Scharfe wrote on the Git mailing list (how to reply to this email): On 7/8/26 12:59 PM, Kristofer Karlsson wrote:
> On Wed, 8 Jul 2026 at 12:44, René Scharfe <l.s.r@web.de> wrote:
>>
>> I didn't
>> find this method used anywhere else, which is a warning sign, but I
>> can't find any catch.
>
> I am not sure why it's a warning sign to have no other usages,
> especially when it's a file local static function.
I meant that I didn't find this optimization in other priority queue
implementations or papers, but admittedly I didn't do an exhaustive
search. Given it's benefits I would have expected to find prior art
on it pretty easily, though.
René
|
|
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): On Wed, 8 Jul 2026 at 13:55, René Scharfe <l.s.r@web.de> wrote:
>
> I meant that I didn't find this optimization in other priority queue
> implementations or papers, but admittedly I didn't do an exhaustive
> search. Given it's benefits I would have expected to find prior art
> on it pretty easily, though.
Aha! Got it, I misunderstood you first.
It's actually described here[1]:
> Bottom-up heapsort conceptually replaces the root with a value of −∞
> and sifts it down using only one comparison per level
> (since no child can possibly be less than −∞)
> until the leaves are reached,
> then replaces the −∞ with the correct value and sifts it up
> (again, using one comparison per level) until the correct position
> is found.
It's for heapsort, not an interactive heap, but the algorithm
still matches.
Also, your idea to split out sift up/down into helper functions did
work, and was quite clean - will share patch shortly once I have
cleaned up the commits and benchmark data.
Thanks again,
Kristofer
[1] https://en.wikipedia.org/wiki/Heapsort#Bottom-up_heapsort |
|
There are merge commits in this Pull Request: Please rebase the branch and force-push. |
spkrka
changed the title
Author
|
/base-commit 9f75e7a |
1 similar comment
Author
|
/base-commit 9f75e7a |
Author
|
/preview |
1 similar comment
Author
|
/preview |
|
Preview email sent as pull.2132.v3.git.1783519221.gitgitgadget@gmail.com |
Author
|
/preview |
|
Preview email sent as pull.2132.v3.git.1783520853.gitgitgadget@gmail.com |
Factor out the bubble-up loop from prio_queue_put() into a standalone sift_up() function. This is a pure refactor with no behavior change, preparing for reuse in a subsequent commit. Suggested-by: Rene Scharfe <l.s.r@web.de> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
When flush_get() removes the root without an immediate replacement, use a cascade-then-sift-up strategy instead of sift-down. Standard sift-down places the last element at the root and sifts it down. This needs two comparisons per level (pick the smaller child, then compare against the element), even though the displaced element almost always ends up near the bottom where it came from. cascade_down() instead moves the vacancy down by promoting the smaller child at each level (one comparison per level), leaving the vacancy at a leaf. The last element is then placed at the vacancy and sift_up() floats it to its correct position, which is typically very little work since it already belongs near the bottom. This is the well-known "bottom-up" variant of sift-down [1]. [1] https://en.wikipedia.org/wiki/Heapsort#Bottom-up_heapsort Helped-by: Rene Scharfe <l.s.r@web.de> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
Author
|
/preview |
|
Preview email sent as pull.2132.v3.git.1783529884.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2132.v3.git.1783532989.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
There was a status update in the "Cooking" section about the branch 'prio_queue_get()' has been optimized by using a cascade-down approach (promoting the smaller child at each level and sifting up the last element from the leaf vacancy), which halves the number of comparisons per extract-min operation in the common case. Needs review. source: <pull.2132.v3.git.1783532989.gitgitgadget@gmail.com> |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -55,19 +66,35 @@ static void sift_down_root(struct prio_queue *queue) | |||
| } | |||
There was a problem hiding this comment.
René Scharfe wrote on the Git mailing list (how to reply to this email):
On 7/8/26 7:49 PM, Kristofer Karlsson via GitGitGadget wrote:
> From: Kristofer Karlsson <krka@spotify.com>
>
> When flush_get() removes the root without an immediate replacement,
> use a cascade-then-sift-up strategy instead of sift-down.
>
> Standard sift-down places the last element at the root and sifts it
> down. This needs two comparisons per level (pick the smaller child,
> then compare against the element), even though the displaced element
> almost always ends up near the bottom where it came from.
>
> cascade_down() instead moves the vacancy down by promoting the
> smaller child at each level (one comparison per level), leaving the
> vacancy at a leaf. The last element is then placed at the vacancy
> and sift_up() floats it to its correct position, which is typically
> very little work since it already belongs near the bottom.
>
> This is the well-known "bottom-up" variant of sift-down [1].
>
> [1] https://en.wikipedia.org/wiki/Heapsort#Bottom-up_heapsort
On an Apple M1 I get a 1% slowdown for bulk describe on Git's repo:
Benchmark 1: ./git_next describe $(git rev-list v2.41.0..v2.47.0)
Time (mean ± σ): 939.5 ms ± 3.6 ms [User: 576.8 ms, System: 65.0 ms]
Range (min … max): 935.0 ms … 946.2 ms 10 runs
Benchmark 2: ./git describe $(git rev-list v2.41.0..v2.47.0)
Time (mean ± σ): 945.5 ms ± 3.3 ms [User: 581.6 ms, System: 67.5 ms]
Range (min … max): 940.1 ms … 950.5 ms 10 runs
Summary
./git_next describe $(git rev-list v2.41.0..v2.47.0) ran
1.01 ± 0.01 times faster than ./git describe $(git rev-list v2.41.0..v2.47.0)
... and on Linux's repo:
Benchmark 1: ./git_next -C ../linux describe $(git -C ../linux rev-list v4.0..v4.1)
Time (mean ± σ): 4.880 s ± 0.014 s [User: 3.914 s, System: 0.252 s]
Range (min … max): 4.864 s … 4.905 s 10 runs
Benchmark 2: ./git -C ../linux describe $(git -C ../linux rev-list v4.0..v4.1)
Time (mean ± σ): 4.917 s ± 0.011 s [User: 3.948 s, System: 0.254 s]
Range (min … max): 4.902 s … 4.938 s 10 runs
Summary
./git_next -C ../linux describe $(git -C ../linux rev-list v4.0..v4.1) ran
1.01 ± 0.00 times faster than ./git -C ../linux describe $(git -C ../linux rev-list v4.0..v4.1)
I see a 1% slowdown on an Apple M5 as well in both cases. I can't
reproduce it on a Ryzen laptop, but that's too noisy to measure 1%
changes anyway.
Checked the total number of prio_queue comparisons with the crude patch
below, and as expected they go down, from 70386235 to 60682175 for Git
and from 473983445 to 439809087 for Linux. So there's less work to do,
still user time goes up -- no idea why.
Also this -- what's up with the system time here:
Benchmark 1: ./git_next rev-list --all --count
Time (mean ± σ): 115.2 ms ± 0.8 ms [User: 95.6 ms, System: 17.7 ms]
Range (min … max): 113.0 ms … 117.1 ms 24 runs
Benchmark 2: ./git rev-list --all --count
Time (mean ± σ): 116.5 ms ± 0.8 ms [User: 95.4 ms, System: 19.0 ms]
Range (min … max): 115.1 ms … 118.6 ms 24 runs
Summary
./git_next rev-list --all --count ran
1.01 ± 0.01 times faster than ./git rev-list --all --count
But:
Benchmark 1: ./git_next -C ../linux rev-list --all --count
Time (mean ± σ): 937.6 ms ± 2.2 ms [User: 887.2 ms, System: 45.5 ms]
Range (min … max): 933.2 ms … 939.9 ms 10 runs
Benchmark 2: ./git -C ../linux rev-list --all --count
Time (mean ± σ): 937.3 ms ± 1.7 ms [User: 887.8 ms, System: 45.0 ms]
Range (min … max): 934.6 ms … 940.3 ms 10 runs
Summary
./git -C ../linux rev-list --all --count ran
1.00 ± 0.00 times faster than ./git_next -C ../linux rev-list --all --count
:-?
> Helped-by: Rene Scharfe <l.s.r@web.de>
> Signed-off-by: Kristofer Karlsson <krka@spotify.com>
> ---
> prio-queue.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/prio-queue.c b/prio-queue.c
> index 926fc04e85..230d6f5e33 100644
> --- a/prio-queue.c
> +++ b/prio-queue.c
> @@ -66,13 +66,31 @@ static void sift_down_root(struct prio_queue *queue)
> }
> }
>
> +/* Cascade vacancy toward a leaf, promoting the smaller child at each level */
> +static size_t cascade_down(struct prio_queue *queue)
> +{
> + size_t ix, child;
> +
> + for (ix = 0; (child = ix * 2 + 1) < queue->nr_; ix = child) {
> + if (child + 1 < queue->nr_ &&
> + compare(queue, child, child + 1) >= 0)
> + child++;
> + queue->array[ix] = queue->array[child];
> + }
> + return ix;
> +}
> +
> static inline void flush_get(struct prio_queue *queue)
> {
> + size_t ix;
> +
> if (!queue->get_pending)
> return;
> queue->get_pending = 0;
> - queue->array[0] = queue->array[--queue->nr_];
> - sift_down_root(queue);
> + --queue->nr_;
> + ix = cascade_down(queue);
> + queue->array[ix] = queue->array[queue->nr_];
> + sift_up(queue, ix);
> }
>
> void prio_queue_put(struct prio_queue *queue, void *thing)
The patch looks fine, though. It introduces struct assignments, but
they should be OK. Tried replacing them with swap() instead (which
does a useless extra write), but that didn't change the performance
(still 1% slowdown). Odd.
René
diff --git a/builtin/describe.c b/builtin/describe.c
index c0abc931a59..4a6ad976d30 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -791,5 +791,6 @@ int cmd_describe(int argc,
while (argc-- > 0)
describe(*argv++, argc == 0);
}
+ print_compares();
return 0;
}
diff --git a/prio-queue.c b/prio-queue.c
index 199775d5afd..b0189bf80e6 100644
--- a/prio-queue.c
+++ b/prio-queue.c
@@ -1,6 +1,13 @@
#include "git-compat-util.h"
#include "prio-queue.h"
+static uintmax_t compares;
+
+void print_compares(void)
+{
+ fprintf(stderr, "compares: %lu\n", compares);
+}
+
static inline int compare(struct prio_queue *queue, size_t i, size_t j)
{
int cmp = queue->compare(queue->array[i].data, queue->array[j].data,
@@ -8,6 +15,7 @@ static inline int compare(struct prio_queue *queue, size_t i, size_t j)
if (!cmp)
cmp = (queue->array[i].ctr > queue->array[j].ctr) -
(queue->array[i].ctr < queue->array[j].ctr);
+ compares++;
return cmp;
}
diff --git a/prio-queue.h b/prio-queue.h
index 570b48e6485..e4cc0c4fb83 100644
--- a/prio-queue.h
+++ b/prio-queue.h
@@ -68,4 +68,6 @@ void clear_prio_queue(struct prio_queue *);
/* Reverse the LIFO elements */
void prio_queue_reverse(struct prio_queue *);
+void print_compares(void);
+
#endif /* PRIO_QUEUE_H */
There was a problem hiding this comment.
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email):
On Fri, 10 Jul 2026 at 18:37, René Scharfe <l.s.r@web.de> wrote:
>
> I see a 1% slowdown on an Apple M5 as well in both cases. I can't
> reproduce it on a Ryzen laptop, but that's too noisy to measure 1%
> changes anyway.
>
> Checked the total number of prio_queue comparisons with the crude patch
> below, and as expected they go down, from 70386235 to 60682175 for Git
> and from 473983445 to 439809087 for Linux. So there's less work to do,
> still user time goes up -- no idea why.
[snip]
> The patch looks fine, though. It introduces struct assignments, but
> they should be OK. Tried replacing them with swap() instead (which
> does a useless extra write), but that didn't change the performance
> (still 1% slowdown). Odd.
First of all, thanks again for the very comprehensive
investigation on your own hardware!
I don't have any Apple machine to test on but I reran your
exact operation on my machine
(Lenovo Thinkpad Intel(R) Core(TM) Ultra 7 155U)
and just saw noise.
As you say, this feels _logically_ better since it's fewer
compares but perhaps this boils down to the cost difference
between executing CPU operations versus memory
access and the CPU cache?
My random guess:
cascade_down() has fewer operations and compares
but needs to visit all levels of the heap,
while sift_down_root perhaps stops slightly earlier,
so the memory region right before the end
gets fewer visits and reduces pressure on
the cache.
I have no idea if my guess is correct,
it's maybe more subtle than that, but
I think ultimately this points to the fact
that while the change is a theoretical
improvement, the real world hardware
tradeoffs make it a non-obvious change.
I think this means we should simply drop the change
and move on -- it produced bigger gains
before making the lazy prio_queue the default,
but now it seems like it is pure noise, unless
the comparator function grows more expensive
in the future.
Thanks,
Kristofer
|
User |
|
René Scharfe wrote on the Git mailing list (how to reply to this email): On 7/8/26 7:49 PM, Kristofer Karlsson via GitGitGadget wrote:
> Note: sift_up() currently uses swap, matching the existing code style. It
> could be further optimized to use copy (hold the element in a temp, shift
> parents down, write once), but that would require changing compare() to
> accept element values instead of array indices. Left for a potential
> follow-up.
Same for sift_down_root(), I guess? It could almost halve the number of
writes, right? I wonder how much of that benefit will be eaten by
caching.
René
|
|
User |
|
Kristofer Karlsson wrote on the Git mailing list (how to reply to this email): On Fri, 10 Jul 2026 at 18:37, René Scharfe <l.s.r@web.de> wrote:
>
> On 7/8/26 7:49 PM, Kristofer Karlsson via GitGitGadget wrote:
> > Note: sift_up() currently uses swap, matching the existing code style. It
> > could be further optimized to use copy (hold the element in a temp, shift
> > parents down, write once), but that would require changing compare() to
> > accept element values instead of array indices. Left for a potential
> > follow-up.
>
> Same for sift_down_root(), I guess? It could almost halve the number of
> writes, right? I wonder how much of that benefit will be eaten by
> caching.
Hm yes indeed, I stopped looking past sift_up() when I realized I should
not expand the scope of the change. But I think the CPU cache
effectively makes the swap almost as cheap in practice.
- Kristofer |
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 tweaks the prio_queue implementation to
use a bottom-up approach sifting for get [1].
In practice, the performance boost is small, but
measurable for reasonably large prio_queue:s
(thousands of elements, not millions) but it
should never increase the work.
Minor note on v3:
After the most recent discussion I am not 100% sure
how to reason about the value of this change -
both the value gain and code cost seem small,
but since there was some interest and research
done by René I wanted to complete this v3
anyway so it can be properly discussed
(though still maybe ultimately closed).
Here's how it works:
Instead of placing the last element at the root and sifting it
down with two comparisons per level, cascade the vacancy
down by promoting the smaller child (one comparison
per level), then place the last element at the vacancy
and sift it up. Since the displaced element
is likely to belong near the bottom of the heap, sift_up()
typically does very little work.
sift_down_root() is kept as-is for the fused replace path
in prio_queue_put(), where the new element is arbitrary and
may belong near the root -- Rene's testing showed that
cascade regresses on git-describe for this reason.
Benchmarks (rev-list --all --count) on public repos confirm
no regression on git.git and linux.git. On a large example
repo with thousands of active branches the cascade yields a
measurable (~2%) end-to-end improvement; the gain is modest
because the lazy-fold optimization (now in next) already
fuses most get+put pairs, leaving only the remaining unfused
gets to benefit from cascade.
René's exhaustive analysis [2] of all permutations up to n=12
confirms that cascade never requires more comparisons than
standard sift-down for a full drain.
Note: sift_up() currently uses swap, matching the existing
code style. It could be further optimized to use copy
(hold the element in a temp, shift parents down, write once),
but that would require changing compare() to accept element
values instead of array indices. Left for a potential
follow-up.
Changes since v2:
Rebased on kk/prio-queue-get-put-fusion (now in next).
Split into two commits - refactoring and then
introducing cascade_down.
Changes since v1:
Kept sift_down_root() and prio_queue_replace() completely unchanged,
preserving René's optimization that avoids the get+put overhead for
replace. The cascade approach now only applies to prio_queue_get().
Extracted the new logic into a separate sift_up_rebalance() function
rather than inlining it in prio_queue_get().
Updated benchmark numbers for ascending, descending and random
insertion ordering. No regressions in any scenario.
[1] https://en.wikipedia.org/wiki/Heapsort#Bottom-up_heapsort
[2] https://lore.kernel.org/git/pull.2132.git.1780250236304.gitgitgadget@gmail.com/T/#m114df6e1c2845acbbc64d875ed7dc1d7d9193ed5
cc: René Scharfe l.s.r@web.de
cc: Kristofer Karlsson krka@spotify.com
cc: René Scharfe l.s.r@web.de
cc: Kristofer Karlsson krka@spotify.com