perf(btree): decide page-reuse eligibility at free time, not per allocation

[mkhairi]

Opening as a draft: this touches in-session page reuse, which is load-bearing for crash safety, so I would rather have the equivalence argument below confirmed before this is treated as mergeable.

What

BTree::allocate_page re-derived the in-session page-reuse rule on every call, scanning the whole freed list and, for each entry, all of free_page_consumed. Both are Vec<u64>, so the membership test is linear:

let pos = self.freed.iter().rposition(|&id| {
    id >= self.reuse_threshold || consumed.as_ref().is_some_and(|c| c.contains(&id))
});

reuse_threshold is the session-start next_page_id (txn/write/txn.rs:115), so every page freed during a transaction is below it and the cheap first disjunct is false for essentially every entry. Each allocation therefore costs O(|freed| × |consumed|), with both vectors growing monotonically through the flush.

This change decides eligibility once, in free_page, and keeps the eligible pages in a second freed_reusable list that allocate_page pops in O(1).

Why it is equivalent, not merely faster

Both inputs to the decision are monotonic within a session: reuse_threshold is fixed once the transaction opens, and free_page_consumed only grows. A page can therefore only ever move from ineligible to eligible, and it can only be drawn from the shared cache before it is freed — so evaluating the rule at free time gives the same answer as evaluating it at allocation time.

Two details that are easy to miss:

• set_reuse_threshold re-partitions, for callers that set the threshold after a free has already happened. Normally that is a walk over two empty vectors.
• drain_freed drains both lists. An eligible page that no allocation claimed is still a page this session freed, and the deferred-free queue has to hear about it or it leaks.

How this showed up

A long-running embedder was found pinning one core at 99.4% for 84 minutes inside a single batch_write, with 5,048 s of user CPU against 2.9 s of system time and 104 read syscalls for the whole process lifetime — no I/O, no progress, and still answering health checks, because this is unbounded work rather than a deadlock. perf record on that thread put 99.51% of samples in allocate_page, and the stack was:

start_auto_flush -> flush -> batch_write -> BTree::put -> allocate_page

Testing

Three tests in btree::tree::core::tests, all on MemVfs:

• reuse_eligibility_matches_the_threshold_rule — below-threshold held back, at/above recycled, and a cache-drawn page recyclable below the threshold.
• threshold_set_after_a_free_repartitions — the post-hoc set_reuse_threshold path.
• allocation_cost_does_not_grow_with_the_freed_list — a hang detector rather than a benchmark: 20k allocations against 20k held-back frees, 10 s bound.

The first two pass unmodified against main, which is the evidence that this preserves existing semantics. The third does not finish in 180 s on main (already compiled, so that is pure runtime) and completes in 0.00 s here.

Gates run locally: cargo nextest run --all-features 763 passed / 0 failed, the same with PAGEDB_INVARIANT_CHECKS=1, cargo clippy --all-targets --all-features -- -D warnings, cargo deny check, RUSTDOCFLAGS="-D warnings" cargo doc --no-deps --all-features, and cargo fmt --all --check.

Benchmarks

cargo bench --bench write_path, 3 runs per side on one machine. Neutral within measurement noise — run-to-run spread on this box reaches ±50% on several cases, which swamps every delta. The two tightest measurements (scaling_file_committed@100000, ±2% on both sides) agree to 0.4%.

That is the expected result: at benchmark scale freed and free_page_consumed stay small, so the quadratic term never dominates. The gain is entirely at the tail, where a flush touches tens of thousands of pages — the regime allocation_cost_does_not_grow_with_the_freed_list covers, and where main stops completing at all. Anyone with a quieter machine will get a cleaner comparison than I could; I would rather state the measurement limit than quote a number I cannot reproduce.

Tradeoff

One extra Vec<u64> per BTree, and one contains check moved from allocation time to free time. The free-time check is against the same consumed vector, so a pathological case would need a session that frees far more pages than it allocates — the inverse of the flush profile this fixes.

On upgrade

No format, API, or durability-ordering change. Both lists are in-memory transaction state; nothing here is persisted, and an existing store is unaffected.

[farhan-syah]

The equivalence argument holds. Checked what it rests on: reuse_threshold is set once per tree, always on a fresh BTree with empty lists; free_page_consumed is cleared before the trees open and taken after both drain_freed() calls, and allocate_page is its only pusher — so ineligible → eligible really is the only transition. Selection order is preserved (rposition → pop on a partition that keeps relative order), and nothing outside core.rs/drain_freed reads freed.

Three changes to land before this leaves draft.

1. Make the membership test O(1)

is_reusable_in_session still scans free_page_consumed linearly, and below-threshold frees dominate the flush profile this fixes — a CoW free of a pre-existing page is below the threshold by construction. So O(frees × |consumed|) survives, and |consumed| is bounded by the free-list window (WINDOW_PAGES × chain_capacity(page_size)), not by something small: a large enough flush is still a long stretch of pure CPU with no progress. Same failure shape, one order of magnitude out.

Change the shared sink to Arc<Mutex<FxHashSet<u64>>>. The commit path already collects it into a HashSet<u64> and only calls contains, begin_write only clears it, and nothing depends on its order or on duplicates — so this is near drop-in, makes the free-time check O(1), and removes the collect at commit.

Then seed consumed at window scale in allocation_cost_does_not_grow_with_the_freed_list. At its current size that test cannot see this term, so it would not catch a regression into it.

2. Make the wiring order unrepresentable instead of assumed

Eligibility is now decided at free time, so it is only correct if all three inputs are in place before the first free. Today they arrive through set_reuse_threshold, set_free_page_cache and set_free_page_consumed, callable in any order at any point in a session — and only the first of the three re-partitions. Wire set_free_page_consumed after a free and a cache-drawn page silently stays held back for the rest of the txn; that is a live gap in the new invariant, not a hypothetical, and an assert would only report it in debug builds while leaving the shape that allows it.

Replace all three setters with one, taking the session's allocation state as a unit:

pub struct PageSource {
    pub reuse_threshold: u64,
    pub cache: Arc<parking_lot::Mutex<Vec<u64>>>,
    pub consumed: Arc<parking_lot::Mutex<FxHashSet<u64>>>,
}

Pass Option<PageSource> at BTree::open — None for compaction's repack trees, which then have bump-only allocation by construction rather than by leaving a threshold at its default. Callers to update: the data and catalog trees in begin_write, and hist_tree in the catalog path (threshold 0, sharing the same cache and sink).

This buys three things beyond ordering safety: "cache wired but sink not" stops being representable, the re-partition loop in set_reuse_threshold is deleted rather than documented, and threshold_set_after_a_free_repartitions goes with it — it tests a path that should not exist. Net less code than this PR currently adds.

3. Drop the special case in is_reusable_in_session

fn is_reusable_in_session(&self, page_id: u64) -> bool {
    page_id >= self.reuse_threshold
        || self.consumed_this_session(page_id)
}

The reuse_threshold == 0 arm is unreachable by construction — page_id >= 0 always holds, so the zero-threshold case is already the first arm. Keep the reasoning in the doc comment; the branch itself only invites a reader to work out whether the two arms can disagree.

CHANGELOG

Drop the [Unreleased] block rather than moving it. 0.1.0 isn't released, so that section is still in progress and [Unreleased] above it implies it shipped. Folding it in is also wrong by that section's own rule — this scan was introduced and fixed inside the pre-0.1.0 window, so it's part of what 0.1.0 is, not a change to it.

Otherwise

Tests co-located and MemVfs-only, comments explain why, and "the first two pass unmodified against main" is the right evidence to offer. One note: the timing assertion is the only wall-clock one in src/ — keep the hang-detector framing in the docstring so it isn't later read as a benchmark and tightened.

[mkhairi]

Thanks — the free-time scan was a real hole, and you're right that it was the dominant case rather than an edge one. All of it addressed in fdfe9b4.

The surviving O(frees × |consumed|). Fixed as you suggested: free_page_consumed is now an FxHashSet<u64>, so is_reusable_in_session is O(1). It was near drop-in — commit.rs was collecting it into a HashSet and only calling contains, so that line became a plain take; begin_write only clears it; the single pusher became insert. Six sites total.

The test could not see it, and now can. consumed is seeded at WINDOW_PAGES × chain_capacity(PAGE) before the frees, so every free pays the eligibility test against a full-size sink. Verified the strengthened version is still red against main: grafted onto main (adapted to its Vec sink) it does not finish in 150 s, already compiled. At the old 500-entry seed it would have passed there, which is exactly your point.

debug_assert in all three setters — and it removed code. Adding the assert to set_reuse_threshold made its own re-partition branch unreachable, so the branch and the test covering it are deleted rather than kept as dead code. The doc comment now states the ordering requirement positively: the threshold must be set before the first free, because eligibility is decided at free time. That also disposes of your ordering note — there is no longer a re-partition whose ordering could be unspecified. Suite is 762 rather than 763 for that reason, not because anything was skipped.

Dead disjunct removed, with a one-line comment saying a zero threshold needs no special case since every page id clears it.

CHANGELOG — [Unreleased] block dropped entirely, per your reasoning that a scan introduced and fixed inside the pre-0.1.0 window is part of what 0.1.0 is. Nothing folded into the 0.1.0 section either.

Hang-detector framing kept, and the docstring now says outright that it is the only wall-clock assertion in src/ and names both scans it guards.

Gates after the change: cargo nextest run --all-features 762 passed / 0 failed, same with PAGEDB_INVARIANT_CHECKS=1, clippy --all-targets --all-features -D warnings clean, cargo deny check ok, RUSTDOCFLAGS="-D warnings" cargo doc clean, cargo fmt --all --check clean.

One thing I want to flag rather than leave implied: the end-to-end evidence I originally had — a real embedder flush completing on a large store — is gone. The 15 GB store that produced the profile hit an unrelated single-page AEAD failure on its next open, and the embedder's recovery path replaced it with an empty one before I could re-measure. So what backs this PR now is the profile, the two semantic tests passing unmodified against main, and the hang detector. I have no large-store before/after timing, and the bench numbers are noise-bound on my machine, as noted in the description.

[mkhairi]

Followed up with a controlled A/B on real data, since the benchmark section only established "neutral at bench scale" and that is a weak claim for a page-allocator change.

Method. Two identical bulk imports of 7,000 records through an embedder onto separate scratch stores, release build, same machine, same source database, no embedding provider configured (its network latency would dominate). The two builds differ only in allocate_page: one is 8217994, the other 8217994 + this branch. I verified the control binary compiled the pristine checkout by checking its dep-info — it names the pristine source tree for pagedb and never the patched one. Latencies are the server's own per-request latency_ms for POST /objects, bucketed in windows of 500.

records	main p95	main max	+branch p95	+branch max	max ratio
500	332 ms	1,409 ms	173 ms	360 ms	3.9x
1,000	954 ms	2,944 ms	313 ms	879 ms	3.3x
1,500	3,042 ms	9,299 ms	395 ms	727 ms	12.8x
~1,900	8,896 ms	17,253 ms	523 ms	903 ms	19x
7,000	not reached	not reached	1,284 ms	3,691 ms	—

On main the max climbs 1.4 → 2.9 → 9.3 → 17.3 s with accelerating increments (+1.5, +6.4, +8.0 s), and throughput halves every three minutes — 1,000 records in the first interval, then 400, then 200. It reached 1,909 records in 13 minutes, at which point I stopped it; extrapolating, the run would have taken hours.

With the branch the max stays in a band — 0.36, 0.88, 0.73, 0.90 s through the early windows and 3.7 s at 7,000 — and the whole import finished in about 40 minutes with zero errors, zero aborted flushes and no AEAD failures. The store reopened clean afterwards: healthy status, no AEAD or checksum lines, and the data readable.

The diagnostic detail is that p50 is nearly identical in both arms (121 → 172 ms on main, 101 → 154 ms with the branch). The median write never touches the scan; the one write per flush cycle that absorbs the flush pays all of it. That is the same signature as the profile in the description, now visible in request latencies rather than in a stack.

Two things this does not show, which I would rather state than have you infer:

• A residual slope survives. With the branch, p95 still rises 173 → 1,284 ms and max 0.36 → 3.69 s across the run. Something still does work proportional to store size on every flush — plausibly whole-segment rewriting rather than allocation. It is now hundreds of milliseconds where it used to be minutes, but it is not flat, and it is a separate problem from this one.
• One run per arm, not a repeated trial. The effect size is far outside the run-to-run variance I measured on the microbenchmarks (±50%), so I am confident in the direction and the order of magnitude, not in the precise ratios.

Also worth noting for anyone reading the commits: your own recent work on this path (allocation churn, Bytes, FxHashMap) is in both arms here. The control's healthy p50 is that work; the collapsing tail is what this branch addresses. They are complementary rather than overlapping.

Taking this out of draft on the strength of the above — the equivalence argument is confirmed, the free-time scan you caught is fixed in fdfe9b4, and this closes the evidence gap.

[farhan-syah]

Pushed 12574b7 — the debug_assert you implemented was in the first revision of my review; I replaced it with a structural fix in a later edit, which GitHub doesn't notify on. Rather than send you round again, I did it.

Setters gone. A session's allocation state is one value, supplied at construction:

BTree::open(pager, realm, root, next, page_size)                  // bump-only
BTree::open_session(pager, realm, root, next, page_size, source)  // threshold + cache + sink

So a late threshold and "cache wired but sink not" are both unconstructible. The asserts go with the setters — deleting the re-partition behind a debug-only check had left release builds with neither.

The sink can no longer shrink mid-session. Free-time eligibility is only sound while free_page_consumed doesn't lose entries under already-banked pages; that was emergent, not held. ConsumedPages now owns the set and hands trees a ConsumedHandle that records and tests but cannot empty. take() is the only way to empty, and both boundaries call it.

Your FxHashSet is preserved, just behind ConsumedPages. Two tests added: no-source trees recycle everything, and a held-back page isn't promoted by a later record — the one intended divergence from the old scan. Net −25 lines.

Gates: 764 passed / 0 failed (764 vs 762 is the two new tests), same with PAGEDB_INVARIANT_CHECKS=1, clippy -D warnings, cargo deny, cargo doc -D warnings, cargo fmt --check — all clean.