prio-queue: use cascade-down sift for faster extract-min

From: Kristofer Karlsson <[email protected]>

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.

---
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(-)

index 9748528ce6..18005c43c4 100644
@@ -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

gitgitgadget

"Kristofer Karlsson via GitGitGadget" <[email protected]> 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.

"Kristofer Karlsson via GitGitGadget" <[email protected]>

		struct prio_queue_entry entry;
		entry.ctr = queue->insertion_ctr++;
		entry.data = thing;
		sift_down_root(queue, entry);

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

From: Kristofer Karlsson <[email protected]>

Add sift_up_rebalance(), an alternative to sift_down_root() that halves the number of comparisons per extract-min.

The standard extract places the last array element at the root and sifts it down. At each level this requires two comparisons (left vs right child, then element vs winner) and a swap.

sift_up_rebalance() instead promotes the smaller child into the root slot at each level — one comparison and one copy — until the vacancy reaches a leaf. The last array element is placed at the vacancy and sifted up to restore heap order. In practice the sift-up rarely moves more than a level or two because the last array element tends to be large.

Work per extract drops from 2d comparisons + d swaps to d comparisons + d copies + a short sift-up.

prio_queue_get() now calls sift_up_rebalance() instead of placing the last element at root and calling sift_down_root().

sift_down_root() and prio_queue_replace() are left unchanged.

Synthetic benchmark (10 rounds of 10M put+get cycles, CPU-pinned, same compiler and Makefile flags):

Ascending keys (git's typical pattern — parents have lower priority than children):

queue width baseline patched speedup

           10     4.39s    3.91s    1.12x
          100     9.10s    6.61s    1.38x
        1,000    11.84s    9.25s    1.28x
       10,000    17.50s   13.92s    1.26x
      100,000    23.97s   20.19s    1.19x

Descending keys (worst case — last element always sinks to leaf):

queue width baseline patched speedup

           10     4.94s    4.95s    1.00x
          100     9.75s    9.42s    1.03x
        1,000    15.01s   15.29s    0.98x
       10,000    24.79s   23.88s    1.04x
      100,000    29.69s   28.24s    1.05x

Random keys:

queue width baseline patched speedup

           10     5.05s    4.99s    1.01x
          100     9.90s    9.50s    1.04x
        1,000    15.35s   14.77s    1.04x
       10,000    25.35s   24.21s    1.05x
      100,000    65.71s   63.38s    1.04x

No regressions in any scenario.

End-to-end benchmark on the linux kernel repo (1.4M commits, range v5.0..v6.0, 311K commits, 20 interleaved runs, 1 warmup):

  Command                      baseline  patched  speedup
  rev-list --count v5.0..v6.0    484ms     474ms    1.02x

The improvement scales with DAG width: wider DAGs produce larger priority queues, amplifying the per-level savings. In small or narrow repositories the queues stay shallow and the sift-down cost is already negligible.

---
prio-queue: use cascade-down sift for faster extract-min

This is a small optimization to prio_queue_get() that reduces the number of comparisons per extract-min from 2d to d (where d is the sift distance).

The standard extract places the last array element at the root and sifts it down, comparing against both children at each level. The new sift_up_rebalance() instead promotes the smaller child at each level (one comparison and one copy) leaving a vacancy that sinks to a leaf. The last element is placed there and sifted up, which in practice rarely moves more than a level or two.

The improvement shows clearly in synthetic benchmarks (up to 1.38x for ascending keys at queue width 100) but is modest end-to-end since sift_down_root is only a fraction of total runtime. On the linux kernel repo, rev-list --count v5.0..v6.0 improves by ~2%. The effect scales with DAG width.

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.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2132%2Fspkrka%2Fcascade-sift-down-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2132/spkrka/cascade-sift-down-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2132

Range-diff vs v1:

1: 9ca2fab4dc ! 1: 6051d44e59 prio-queue: use cascade-down sift for faster extract-min
@@ Metadata
Author: Kristofer Karlsson <[email protected]>

## Commit message ##
- prio-queue: use cascade-down sift for faster extract-min
-
- 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.
+ prio-queue: use cascade-down for faster extract-min
+
+ Add sift_up_rebalance(), an alternative to sift_down_root() that
+ halves the number of comparisons per extract-min.
+
+ The standard extract places the last array element at the root and
+ sifts it down. At each level this requires two comparisons (left
+ vs right child, then element vs winner) and a swap.
+
+ sift_up_rebalance() instead promotes the smaller child into the
+ root slot at each level — one comparison and one copy — until the
+ vacancy reaches a leaf. The last array element is placed at the
+ vacancy and sifted up to restore heap order. In practice the
+ sift-up rarely moves more than a level or two because the last
+ array element tends to be large.
+
+ Work per extract drops from 2d comparisons + d swaps to
+ d comparisons + d copies + a short sift-up.
+
+ prio_queue_get() now calls sift_up_rebalance() instead of placing
+ the last element at root and calling sift_down_root().
+
+ sift_down_root() and prio_queue_replace() are left unchanged.
+
+ Synthetic benchmark (10 rounds of 10M put+get cycles, CPU-pinned,
+ same compiler and Makefile flags):
+
+ Ascending keys (git's typical pattern — parents have lower
+ priority than children):
+
+ queue width baseline patched speedup
+ 10 4.39s 3.91s 1.12x
+ 100 9.10s 6.61s 1.38x
+ 1,000 11.84s 9.25s 1.28x
+ 10,000 17.50s 13.92s 1.26x
+ 100,000 23.97s 20.19s 1.19x
+
+ Descending keys (worst case — last element always sinks to leaf):
+
+ queue width baseline patched speedup
+ 10 4.94s 4.95s 1.00x
+ 100 9.75s 9.42s 1.03x
+ 1,000 15.01s 15.29s 0.98x
+ 10,000 24.79s 23.88s 1.04x
+ 100,000 29.69s 28.24s 1.05x
+
+ Random keys:
+
+ queue width baseline patched speedup
+ 10 5.05s 4.99s 1.01x
+ 100 9.90s 9.50s 1.04x
+ 1,000 15.35s 14.77s 1.04x
+ 10,000 25.35s 24.21s 1.05x
+ 100,000 65.71s 63.38s 1.04x
+
+ No regressions in any scenario.
+
+ End-to-end benchmark on the linux kernel repo (1.4M commits,
+ range v5.0..v6.0, 311K commits, 20 interleaved runs, 1 warmup):
+
+ Command baseline patched speedup
+ rev-list --count v5.0..v6.0 484ms 474ms 1.02x
+
+ The improvement scales with DAG width: wider DAGs produce larger
+ priority queues, amplifying the per-level savings. In small or
+ narrow repositories the queues stay shallow and the sift-down
+ cost is already negligible.

## prio-queue.c ##
@@ prio-queue.c: 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 */
++static void sift_up_rebalance(struct prio_queue *queue) ++{
++ size_t ix, child;
++
++ /* Cascade: promote smaller child at each level. */
+ 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 */
++ if (child + 1 < queue->nr &&
++ compare(queue, child, child + 1) >= 0)
++ child++;
+ queue->array[ix] = queue->array[child];
+ }
-
-- if (compare(queue, ix, child) <= 0)
-+ /* Place queue->array[queue->nr] (left by caller) and sift up. */
++
++ /* Place the last element at the vacancy 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);
++ break;
+ swap(queue, parent, ix);
+ ix = parent;
- }
- }
-
++ }
++}
++
+ void *prio_queue_get(struct prio_queue *queue)
+ {
+ void *result;
@@ prio-queue.c: void *prio_queue_get(struct prio_queue *queue)
if (!--queue->nr)
return result;

- queue->array[0] = queue->array[queue->nr];
- sift_down_root(queue);
+- sift_down_root(queue);
++ sift_up_rebalance(queue);
return result;
}
-@@ prio-queue.c: 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);
- }
- }
+

 prio-queue.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

index 9748528ce6..66d445b078 100644
@@ -76,6 +76,29 @@ static void sift_down_root(struct prio_queue *queue)
 	}
 }
 
+static void sift_up_rebalance(struct prio_queue *queue)
+{
+	size_t ix, child;
+
+	/* Cascade: promote smaller child at each level. */
+	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];
+	}
+
+	/* Place the last element at the vacancy 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, parent, ix);
+		ix = parent;
+	}
+}
+
 void *prio_queue_get(struct prio_queue *queue)
 {
 	void *result;

@@ -89,8 +112,7 @@ void *prio_queue_get(struct prio_queue *queue)
 	if (!--queue->nr)
 		return result;
 
-	queue->array[0] = queue->array[queue->nr];
-	sift_down_root(queue);
+	sift_up_rebalance(queue);
 	return result;
 }
 

base-commit: 1666c1265231b0bc5f613fbbf3f0a9896cdef76e

gitgitgadget

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)

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(-)

index 9748528ce6..ba6b460a46 100644
@@ -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);

index da7fad2f1f..5977fba438 100644
@@ -32,6 +32,7 @@ struct prio_queue {
 	void *cb_data;
 	size_t alloc, nr;
 	struct prio_queue_entry *array;
+	bool sift_down_root_pending;
 };
 
 /*

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

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

Right. I was wondering, though: Why is sift-down so much faster than cascade in the describe benchmark from 30598ccc4d (describe: use oidset in finish_depth_computation(), 2025-09-02)?

I think I mostly understand it now: cascade is better in prio_queue_get() because the sift-down item is from the bottom and will likely end up back at the bottom, just of a different branch of the heap. Thus a sift-down costs 3 compares times the number of levels, while a cascade costs just 2 compares times the number of levels and there is likely little to no need to sift it back up.

For prio_queue_replace() we sift down a random item, though; we don't know where it will end up. If it belongs at the very top then sift-down just needs 3 compares, while cascade needs 2 compares times the number of levels to bring the hole down and the same to bring the item up.

Below is a diff on top of your second cascade patch to use sift-down only for the root and cascade otherwise. It comes remarkably close to the performance of a full sift-down. I don't know how to find the optimal number of levels to try sift-down before switching to cascade for a given random item, though.

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.

Benchmark 1: ./git_main describe $(git rev-list v2.41.0..v2.47.0)
Time (mean ± σ): 602.4 ms ± 1.2 ms [User: 539.2 ms, System: 47.7 ms]
Range (min … max): 600.5 ms … 604.7 ms 10 runs

Benchmark 2: ./git_cascade1 describe $(git rev-list v2.41.0..v2.47.0)
Time (mean ± σ): 993.9 ms ± 1.7 ms [User: 930.2 ms, System: 48.2 ms]
Range (min … max): 991.1 ms … 996.6 ms 10 runs

Benchmark 3: ./git_cascade2 describe $(git rev-list v2.41.0..v2.47.0)
Time (mean ± σ): 602.4 ms ± 1.7 ms [User: 539.1 ms, System: 47.6 ms]
Range (min … max): 599.9 ms … 606.2 ms 10 runs

Benchmark 4: ./git describe $(git rev-list v2.41.0..v2.47.0)
Time (mean ± σ): 625.4 ms ± 1.7 ms [User: 561.8 ms, System: 48.0 ms]
Range (min … max): 623.4 ms … 627.9 ms 10 runs

Summary
./git_main describe $(git rev-list v2.41.0..v2.47.0) ran

    1.00 ± 0.00 times faster than ./git_cascade2 describe $(git rev-list v2.41.0..v2.47.0)
    1.04 ± 0.00 times faster than ./git describe $(git rev-list v2.41.0..v2.47.0)
    1.65 ± 0.00 times faster than ./git_cascade1 describe $(git rev-list v2.41.0..v2.47.0)

git_main and git_cascade2 (your v2): sift-down git_cascade1 (your v1): cascade
git (your v2 and the patch below): sift-down for root then cascade

René

index 66d445b078..4d7debc2ba 100644
@@ -58,30 +58,12 @@ void prio_queue_put(struct prio_queue *queue, void *thing)
 	}
 }
 
-static void sift_down_root(struct prio_queue *queue)
+static void sift_up_rebalance(struct prio_queue *queue, size_t ix)
 {
-	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);
-	}
-}
-
-static void sift_up_rebalance(struct prio_queue *queue)
-{
-	size_t ix, child;
+	size_t child;
 
 	/* Cascade: promote smaller child at each level. */
-	for (ix = 0; (child = ix * 2 + 1) < queue->nr; ix = child) {
+	for (; (child = ix * 2 + 1) < queue->nr; ix = child) {
 		if (child + 1 < queue->nr &&
 		    compare(queue, child, child + 1) >= 0)
 			child++;

@@ -112,7 +94,7 @@ void *prio_queue_get(struct prio_queue *queue)
 	if (!--queue->nr)
 		return result;
 
-	sift_up_rebalance(queue);
+	sift_up_rebalance(queue, 0);
 	return result;
 }
 

@@ -132,9 +114,20 @@ void prio_queue_replace(struct prio_queue *queue, void *thing)
 	} else if (!queue->compare) {
 		queue->array[queue->nr - 1].ctr = queue->insertion_ctr++;
 		queue->array[queue->nr - 1].data = thing;
+	} else if (queue->nr < 3) {
+		prio_queue_get(queue);
+		prio_queue_put(queue, thing);
 	} else {
-		queue->array[0].ctr = queue->insertion_ctr++;
+		size_t child = compare(queue, 1, 2) <= 0 ? 1 : 2;
+		queue->array[0].ctr = queue->insertion_ctr;
 		queue->array[0].data = thing;
-		sift_down_root(queue);
+		if (compare(queue, 0, child) <= 0) {
+			queue->insertion_ctr++;
+		} else {
+			queue->array[0] = queue->array[child];
+			queue->nr--;
+			sift_up_rebalance(queue, child);
+			prio_queue_put(queue, thing);
+		}
 	}
 }

Yes, I think that reasoning is correct. It depends on where the item will land.

For get() the last array element came from the bottom of the heap and will almost certainly end up back near the bottom, so cascade's blind descent is a good default.

For replace() the new element is arbitrary -- in describe's pattern it often belongs near the root, so sift-down's early exit after 2-3 compares dominates.

Your benchmark confirms this: v2 (cascade only in get) matches the baseline exactly for describe, while the hybrid is 4% slower.

Agreed. And with the lazy-fold series (v3 just sent), replace() is removed as a public API entirely. But the same principle applies to the fused replace path inside prio_queue_put(): when get_pending is set and a put arrives, we write the new element at the root and sift it down -- that path should keep sift-down for the same reason your analysis shows.

If (that's a big if) both my patches eventually land, the split would be:
- unfused get-flush (in get() and peek()): use cascade
- fused replace (in put()): keep sift-down

Which is exactly the split your analysis predicts is optimal.

Now I am thinking it would be easier to reason about this if the other patch lands first, since the cascade change becomes simpler to evaluate when replace is already gone and only the unfused paths remain.

- Kristofer

Sorry, I should have noticed this message and responded earlier. Let's make sure that the "other patch" is ready then and merge it down.

Since June 8th, nothing seemed to have happened to the thread for the "other patch".

https://lore.kernel.org/git/[email protected]/

Is everybody happy with these two patches?

Thanks.

An excellent observation, showing clear and analytic mind. This is one of the reasons why I love reading review messages from you (and also explanation in the proposed commit log messages in your patches).

Thanks.

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).

Thanks,
Kristofer

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(+)

index 1f3f099f5c5..ba7d293cf5f 100644
@@ -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

index b71a22b43bb..69352f541f4 100644
@@ -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 },

index f2885b33d58..ab0d3e01d1e 100644
@@ -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);

Agreed, I also struggle to come up with such a case. Perhaps theoretically possible to construct, but would not invalidate the general heuristic?

I am sincerely grateful that you took the time to analyze it to this level of detail. Very nice analysis and data!

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.

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.

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

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é

Aha! Got it, I misunderstood you first.
It's actually described here[1]:

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

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/[email protected]/T/#m114df6e1c2845acbbc64d875ed7dc1d7d9193ed5

Kristofer Karlsson (2):
prio-queue: extract sift_up() from prio_queue_put()
prio-queue: use cascade for unfused gets

 prio-queue.c | 43 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 11 deletions(-)

base-commit: 00534a21ce949ef80a5b8b9d7fc20b7d381038e9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2132%2Fspkrka%2Fcascade-sift-down-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2132/spkrka/cascade-sift-down-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2132

Range-diff vs v2:

 -:  ---------- > 1:  ec6a448563 prio-queue: extract sift_up() from prio_queue_put()
 1:  6051d44e59 ! 2:  89a22c6a75 prio-queue: use cascade-down for faster extract-min

@@ Metadata
Author: Kristofer Karlsson <[email protected]>

## Commit message ##
- prio-queue: use cascade-down for faster extract-min
+ prio-queue: use cascade for unfused gets

- Add sift_up_rebalance(), an alternative to sift_down_root() that
- halves the number of comparisons per extract-min.
+ When flush_get() removes the root without an immediate replacement,
+ use a cascade-then-sift-up strategy instead of sift-down.

- The standard extract places the last array element at the root and
- sifts it down. At each level this requires two comparisons (left
- vs right child, then element vs winner) and a swap.
+ 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.

- sift_up_rebalance() instead promotes the smaller child into the
- root slot at each level — one comparison and one copy — until the
- vacancy reaches a leaf. The last array element is placed at the
- vacancy and sifted up to restore heap order. In practice the
- sift-up rarely moves more than a level or two because the last
- array element tends to be large.
+ 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.

- Work per extract drops from 2d comparisons + d swaps to
- d comparisons + d copies + a short sift-up.
+ This is the well-known "bottom-up" variant of sift-down [1].

- prio_queue_get() now calls sift_up_rebalance() instead of placing
- the last element at root and calling sift_down_root().
-
- sift_down_root() and prio_queue_replace() are left unchanged.
-
- Synthetic benchmark (10 rounds of 10M put+get cycles, CPU-pinned,
- same compiler and Makefile flags):
-
- Ascending keys (git's typical pattern — parents have lower
- priority than children):
-
- queue width baseline patched speedup
- 10 4.39s 3.91s 1.12x
- 100 9.10s 6.61s 1.38x
- 1,000 11.84s 9.25s 1.28x
- 10,000 17.50s 13.92s 1.26x
- 100,000 23.97s 20.19s 1.19x
-
- Descending keys (worst case — last element always sinks to leaf):
-
- queue width baseline patched speedup
- 10 4.94s 4.95s 1.00x
- 100 9.75s 9.42s 1.03x
- 1,000 15.01s 15.29s 0.98x
- 10,000 24.79s 23.88s 1.04x
- 100,000 29.69s 28.24s 1.05x
-
- Random keys:
-
- queue width baseline patched speedup
- 10 5.05s 4.99s 1.01x
- 100 9.90s 9.50s 1.04x
- 1,000 15.35s 14.77s 1.04x
- 10,000 25.35s 24.21s 1.05x
- 100,000 65.71s 63.38s 1.04x
-
- No regressions in any scenario.
-
- End-to-end benchmark on the linux kernel repo (1.4M commits,
- range v5.0..v6.0, 311K commits, 20 interleaved runs, 1 warmup):
-
- Command baseline patched speedup
- rev-list --count v5.0..v6.0 484ms 474ms 1.02x
-
- The improvement scales with DAG width: wider DAGs produce larger
- priority queues, amplifying the per-level savings. In small or
- narrow repositories the queues stay shallow and the sift-down
- cost is already negligible.
+ [1] https://en.wikipedia.org/wiki/Heapsort#Bottom-up_heapsort

+ Helped-by: Rene Scharfe <[email protected]>

## prio-queue.c ##
@@ prio-queue.c: static void sift_down_root(struct prio_queue *queue)
}
}

-+static void sift_up_rebalance(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;
+
-+ /* Cascade: promote smaller child at each level. */
-+ for (ix = 0; (child = ix * 2 + 1) < queue->nr; ix = child) {
-+ if (child + 1 < queue->nr &&
++ 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];
+ }
-+
-+ /* Place the last element at the vacancy 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, parent, ix);
-+ ix = parent;
-+ }
++ return ix;
+}
+
- void *prio_queue_get(struct prio_queue *queue)
+ static inline void flush_get(struct prio_queue *queue)
{
- void *result;
-@@ prio-queue.c: void *prio_queue_get(struct prio_queue *queue)
- if (!--queue->nr)
- return result;
-
-- queue->array[0] = queue->array[queue->nr];
++ size_t ix;
++
+ if (!queue->get_pending)
+ return;
+ queue->get_pending = 0;
+- queue->array[0] = queue->array[--queue->nr_];
- sift_down_root(queue);
-+ sift_up_rebalance(queue);
- return result;
++ --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)

gitgitgadget

From: Kristofer Karlsson <[email protected]>

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.

---
 prio-queue.c | 21 ++++++++++++---------
 1 file changed, 12 insertions(+), 9 deletions(-)

index 199775d5af..926fc04e85 100644
@@ -37,6 +37,17 @@ void clear_prio_queue(struct prio_queue *queue)
 	queue->get_pending = 0;
 }
 
+static void sift_up(struct prio_queue *queue, size_t ix)
+{
+	while (ix) {
+		size_t parent = (ix - 1) / 2;
+		if (compare(queue, parent, ix) <= 0)
+			break;
+		swap(queue, parent, ix);
+		ix = parent;
+	}
+}
+
 static void sift_down_root(struct prio_queue *queue)
 {
 	size_t ix, child;

@@ -66,8 +77,6 @@ static inline void flush_get(struct prio_queue *queue)
 
 void prio_queue_put(struct prio_queue *queue, void *thing)
 {
-	size_t ix, parent;
-
 	if (queue->get_pending) {
 		queue->get_pending = 0;
 		queue->array[0].ctr = queue->insertion_ctr++;

@@ -85,13 +94,7 @@ void prio_queue_put(struct prio_queue *queue, void *thing)
 		return; /* LIFO */
 
 	/* Bubble up the new one */
-	for (ix = queue->nr_ - 1; ix; ix = parent) {
-		parent = (ix - 1) / 2;
-		if (compare(queue, parent, ix) <= 0)
-			break;
-
-		swap(queue, parent, ix);
-	}
+	sift_up(queue, queue->nr_ - 1);
 }
 
 void *prio_queue_get(struct prio_queue *queue)

gitgitgadget

From: Kristofer Karlsson <[email protected]>

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

---
 prio-queue.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

index 926fc04e85..230d6f5e33 100644
@@ -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)

gitgitgadget

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

:-?

index c0abc931a59..4a6ad976d30 100644
@@ -791,5 +791,6 @@ int cmd_describe(int argc,
 		while (argc-- > 0)
 			describe(*argv++, argc == 0);
 	}
+	print_compares();
 	return 0;
 }

index 199775d5afd..b0189bf80e6 100644
@@ -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;
 }
 

index 570b48e6485..e4cc0c4fb83 100644
@@ -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 */

[snip]

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

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é

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