b+tree: do not hand a fresh tree's whole node pool to its first commit - #166
Merged
Merged
Conversation
map_memory() threads the reserved pool onto the free list through free(), and free() writes each node's header and marks it dirty. At that point the bit says something untrue: the tree has only just been created, no COW clone of it exists, and the storage already holds what those nodes say. commit_to() copies every dirty node, so the first commit of the tree's first clone copied the entire reserved pool. It then looked fine forever after, because commit_to() clears the bit in its target and later clones inherit the cleaned state - which is exactly why a single-commit test cannot see this. Measured with a counter in commit_to() over map_memory(20000) plus 100 inserted values: the first commit copied 170 nodes, 169 of which held no values at all, to move one node of real change. With the bits cleared it copies 2. Cleared in map_memory() only. reserve()/reserve_additional() can run on a clone that genuinely owes its new pool nodes to a target, so the same reasoning does not hold there. nodes_dirty() joins nodes_used()/nodes_reserved() so the property is assertable rather than only observable from a probe, and the test drives two commit cycles so a regression cannot hide behind the self-correction described above. Note what the test does NOT assert: dirty <= used is not an invariant, because taking a node off the free list also rewrites its successor's back-link (new_node -> unlink_right), so a single allocation legitimately dirties two nodes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
psiha
force-pushed
the
fix/map-memory-clean-free-pool
branch
from
September 11, 2026 14:17
aa358c4 to
9450aa0
Compare
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
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.
Stacked on
bt/10-comparator-cost(#165) — it only needs to be after it in thestack, there is no logical dependency. Happy to rebase onto
bt/9-bench-rigorormasterif that suits the ordering better.The defect
map_memory( N )threads the reserved pool onto the free list viaassign_nodes_to_free_pool, which callsfree()on every node — andfree()writesthe node header and marks it dirty.
At that moment the bit asserts something untrue. The tree has just been created, no
COW clone of it exists, and the storage already holds exactly what those nodes say.
commit_to()copies every dirty node, so the first commit of the tree's firstclone copied the entire reserved pool. And then it looks fine forever:
commit_to()clears the bit in its target, and later clones inherit the cleaned state. That
self-correction is precisely why a single-commit test cannot see this.
Measured
A counter in
commit_to()tallyingnodes / dirty / of which empty (num_vals == 0),over
map_memory( 20000 )+ 100 inserted values + two clone→mutate→commit cycles:So the first commit was copying 169 empty free-pool nodes in order to move one node
of real change, and the fix makes the first commit cost what every later one costs.
The larger the reservation, the larger the one-off — a consumer that sizes the pool
from a known row count pays for the whole thing.
The fix
Clear the bits right after
assign_nodes_to_free_pool( 0 ), inmap_memoryonly.Deliberately not in
reserve()/reserve_additional(): those can run on a clonethat genuinely owes its newly added pool nodes to a target, so the same reasoning
does not hold there.
Testing
nodes_dirty()joinsnodes_used()/nodes_reserved()so the property isassertable from a test rather than only observable from a probe's stderr — and it is
a reasonable thing for a consumer to ask, being what a
commit_to()of a clone willcost.
bptree_cow.a_fresh_tree_owes_a_commit_nothingasserts it, and drives two commitcycles so a regression cannot hide behind the self-correction above.
One thing the test deliberately does not assert:
dirty <= usedis not aninvariant. Taking a node off the free list also rewrites its successor's back-link
(
new_node→unlink_right), so a single allocation legitimately dirties two nodes.I tried that assertion first and it failed at 2 vs 1; the comment now records why, so
the next person does not re-derive it.
Gate
Full suite, Release, Windows clang-cl: 2081 passed / 1 skipped / 0 failed (the
skip is the pre-existing
bp_tree.replace_keys_inplace_stale_separator_underflow_repro).2077 → 2081 is the four new assertions.
🤖 Generated with Claude Code