WIP: open and consume the node front gap — not correct yet by psiha · Pull Request #160 · psiha/vm

psiha · 2026-09-09T09:18:46Z

Draft — do not merge. Two tests fail (bp_tree.nonunique, bp_tree.playground). Opened so the diagnosis is not lost; #159 (the plumbing, gap pinned at zero) is the last correct state.

What this adds

open_slot_from_front() — make room at a logical position by moving the entries below it down into the gap instead of the ones above it up. The free slot lands at the same logical position either way; this direction moves pos entries rather than num_vals - pos, and it is the only direction available once a leaf's entries reach the end of their array.
insert() picks the cheaper side, and must pick the front in that case.
relieve_into_sibling: giving to the left becomes start = to_move — no move at all; giving to the right consumes the sibling's gap.
A recycled free-list node no longer inherits a gap.

Three faults found so far, in order — the first two are fixed and correct on their own terms

A hang, not a fault. Eight sites took a bare pointer to the key array rather than to the node's first entry, so intra-node search scanned from the array base. Fixed.
relieve_into_sibling's give-to-right shifted the sibling by the full to_move from a base that already had a gap. The room check bounds num_vals + to_move, not start + num_vals + to_move. The existing gap supplies part of the opening, so only the deficit should be shifted and the gap then closed. Fixed.
Still open. A temporary probe in verify() now reports the invariant directly and pins it:
```
[GAP] start=30 num_vals=123 max=123
```
A node reached full while still carrying a gap of 30. insert() cannot produce that — its guard keeps start + num_vals constant, so num_vals reaches max only as start reaches 0 — which leaves the paths that set num_vals directly: the bulk fill and merge lanes, which compute free capacity as max_values - num_vals and so overrun a node whose entries do not start at 0.

The fix that follows from that: full() means num_vals == max_values, but a node with a gap only has max_values - start slots. Recentre eagerly — a gap must not survive into any path that fills to capacity — guarding the sites that compute available space that way (bulk_append_tail, the merge in-place fill). That keeps every existing invariant, including split_to_insert's num_vals == max_values, as a precondition at a handful of entry points rather than a change to what full() means.

The probe should come out before this leaves draft.

4400 lines in one file, though the class hierarchy inside it was already layered exactly along the lines the file now follows: b+tree/base.hpp the node pool and everything independent of the key type - headers, slots, the free list, depth and size b+tree/keyed_base.hpp knows the key type but not its ordering: node layouts, entry moves, split/merge/underflow, the bulk paths b+tree/impl.hpp the comparator-dependent half: search, and the operations that must search before they can place b+tree.hpp the public interface, unchanged in name and content Pure move: every line is where it was, only redistributed. The umbrella header keeps its path, so nothing downstream has to change.

"vm_vector.hpp" resolved when the include block sat in containers/; from containers/b+tree/ it does not. MSVC and clang-cl found it anyway - they search the directories of every file on the include stack, so containers/ was still in scope through the umbrella header - while GCC and Clang search only the including file's own directory plus -I paths. Hence a split that built and passed on Windows and failed on every POSIX arm. Uses the same <psi/vm/...> form as the rest of the block, which does not care where the file sits.

The pull_request trigger filtered on a master base, so in a stack of pull requests only the bottom one - the one actually targeting master - was ever built. Every branch above it could be reviewed and merged without a single job having run against it. Dropping the filter keeps the push trigger on master as it was.

…s a valid insert hint lower_bound expressed 'one past the end of a leaf' as the first value of the FOLLOWING leaf. Past the LAST leaf there is no following leaf, so it built an iterator from a null node slot - while end() is the last leaf at its num_vals offset. On a non-empty tree, for a key greater than every key present: tree.lower_bound( k ) == tree.end() was false, and tree.insert( tree.lower_bound( k ), k ) access-violated. So the ordinary sorted-container idiom failed for every append. end() is accepted as the append hint: it already works structurally - it is the last leaf at its num_vals offset, which insert() appends to - only the debug hint assertions dereferenced the hint unconditionally. The returned iterator now goes through make_iter( insert_pos_t ), which steps back from the next-insert position and so stays correct across a split that moved the value into the new node.

Two places took a bound on faith. The minimum fill is not an arbitrary half: 'min_values = ceil( max / 2 )' is exactly what makes '2 * min <= max + 1' true, which is what makes 'either a sibling can lend a value, or the two merge into one node' true - the property the whole underflow half rests on (handle_underflow, merge_right_into_left, append_and_free, and the bulk-fill partitions written as 'min_values * 2'). Raising the minimum past it does not merely make those suboptimal, it makes the 2-into-1 merge overflow the node silently, before it asserts. Now a static_assert, so a higher fill target has to bring its own merge shape. The intra-node lower_bound/upper_bound took the LEAF's capacity for both the bound asserted on num_vals and the compile-time choice between a linear scan and a binary search - while also being called on inner nodes. For a set the leaf is the larger of the two, so the assumption held and the dispatch was merely conservative. Neither survives a leaf that carries anything besides the key: the leaf then holds fewer entries than the inner node, the assumption becomes false for a full inner node - an assumption, not an assertion, so a release build miscompiles rather than trips - and the dispatch would drag the larger node onto the linear path, past the byte limit it was measured against. Capacity is now a parameter, taken from whichever node is being searched.

Every site that relocates values inside or between nodes named the key array directly - move_keys, rshift_keys/lshift_keys, and nine raw std::shift_left/ right calls on node.keys. That is the same thing as assuming an entry IS a key, which holds only for a set. The three helpers now carry whatever arrays make up an entry, keyed off the node type: a map's leaf has a parallel array of mapped values that has to move by the same indices, in the same direction, at the same time. Child slots are deliberately not part of an entry - relocating those has to re-index and dirty every child touched, which is what move_chldrn is for. No behavioural change: with no node carrying values yet, every added branch is constant-false and compiles away.

A split-on-full b+tree does not sit at its minimum fill, but it does not sit near full either. Measured by the accompanying characterisation test, 8M uint32 keys: random one-by-one insertion settles at 69.5% leaf occupancy with 512-byte nodes and 72.8% with 4096-byte ones - Yao's ln 2 - and SEQUENTIAL insertion, ascending or descending, settles at exactly 50%, which is where the waste actually is. The bulk paths are already at 100% and stay there. So before splitting, an overflowing leaf now hands values to a same-parent sibling that still has room. This is the exact dual of handle_underflow's borrow branches and uses the same idioms: sibling existence is resolved from parent_child_idx, never the level links (those cross parents), and the separator follows the values. It hands over half the room found, not all of it - a sibling emptied of slack just moves the next split one node over, and a sibling left full would have to be split by the very insertion being relieved. Leaves only, deliberately: a node's children carry a back-index into their parent, so relocating an inner node's children re-indexes and dirties every one of them, costing more than the split it would save. random, 512-byte nodes 69.5% -> 87.1% 5.94 -> 4.74 leaf bytes/key sequential 50.0% -> 99.2% 8.26 -> 4.16 random, 4096-byte nodes 72.8% -> 89.9% 5.52 -> 4.46 bulk / appended merge unchanged at 100% / 99.9% Confirmed on the resident node pool, not just leaf bytes (hence nodes_used() and nodes_reserved()): 6.09 -> 4.87 pool bytes/key random, 8.53 -> 4.30 sequential. The minimum fill is untouched and no merge code changes - this raises occupancy without raising the floor, which is what would oblige a 3-into-2 merge. Inserts are neutral-to-faster throughout. The one measured cost is find() in the 512-byte configuration, 1.08x, where intra-node search is a linear scan and a fuller leaf is a longer one; at 4096 bytes, where that search is binary, the same measurement is 0.95x. That is a property of occupancy, not of this policy - a bulk-built tree pays it too. PSI_VM_BT_REDISTRIBUTE_ON_OVERFLOW=0 restores plain split-on-full.

Seventy-odd sites reached into node.keys[ i ] directly, which states in each of them that entry i of a node lives at offset i of its key array. That is true today and it is the only reason the tree cannot yet leave a gap at the front of a node. key_at( node, i ) is that statement, made once. It is still exactly node.keys[ i ] - no behavioural change, and it compiles to the same thing - but there is now a single place where "where does a node's first live entry sit" is answered, which is what a devector node needs and what the parallel value array of a map needs. The raw-array search helpers keep taking a bare Key const keys[]: their caller has already resolved the base, and they must stay callable on a span that is not a node at all (the bulk merge path passes source keys straight in).

What fill a b+tree actually reaches, what the classical variants do about it, what shipping systems actually do (read at source, not from secondary literature - no true B* among them), why this container is not becoming one, and which of LeanStore's node-level techniques transfer to a container with no buffer manager and no synchronisation.

Node entries were pinned to offset 0 of their array, so taking entries from the FRONT of a node - which is what relieve_into_sibling and both of handle_underflow's borrow branches do - had to move everything that remained. That is the wrong way round: the move is largest exactly when the number of entries handed over is smallest. At 4096-byte nodes it is a 4 KiB memmove to relocate a handful of keys. node_header now carries where a node's live entries begin, and every accessor honours it - key_at, the keys/children spans, the shift primitives, and move_entries. This commit only establishes that; nothing opens a gap yet, so the change is inert and provably so: the gap is zero everywhere and the tests are unchanged. The layout is a policy, because it is a real trade rather than a free win: plain (default) 'start' is its own member. sizeof( node_header ) grows by the ALIGNMENT of node_slot, not by the width of the field - the header is already a multiple of it - so 16 becomes 20 and a 4096-byte node holds 1019 4-byte keys instead of 1020. Every field stays a plain load. packed num_vals, start, parent_child_idx and dirty share one 32-bit word: the header does not grow and no capacity is lost. num_vals, read on every node visit, becomes a bit extract, and the gap is capped at 255 entries. The packed budget is computed from node_size alone, since the header cannot see Key. parent_child_idx is the interesting one: children always cost a slot each, so its bound does NOT depend on the key width, which is what makes the budget fit at all - 12 + 8 + 10 + 1 = 31 bits at 4096-byte nodes, 9 + 8 + 7 + 1 = 25 at 512. A static_assert states it rather than assuming it. Selected by PSI_VM_BT_PACKED_NODE_HEADER; both arms are gated. Two incidental repairs the packed layout forced, both improvements in their own right: a reference bound to num_vals (which cannot bind to a bitfield) is now the lvalue it aliased, and four sites that let num_vals' type leak into deduction now say which type they mean.

NOT for the stack. bt/7-devector-nodes (the plumbing, gap pinned at zero) is the last gated state; this is the follow-on that actually opens a gap, and it access-violates in bp_tree.nonunique. What is here: - open_slot_from_front(): make room at a logical position by moving the entries BELOW it down into the gap instead of the ones above it up - insert() picks the cheaper side, and MUST pick the front once a leaf's entries reach the end of their array - relieve_into_sibling: giving to the left is now just 'start = to_move' (no move at all); giving to the right consumes the sibling's gap if it has one - a recycled free-list node no longer inherits a gap - eight sites that took a bare pointer to the key array rather than to the node's first entry, which is what made the first run hang rather than fault: the intra-node search was scanning from the array base Leading hypotheses for the remaining fault, in order: 1. split_to_insert passes leaf_node::max_values as a LOGICAL end offset (move_entries( node, mid - 1, max, ... )). With a gap that addresses keys[ start + max ], past the array. The invariant that makes this safe is "a full node has start == 0"; it is argued but nowhere asserted, and handle_underflow's borrow-from-left does ++num_vals then rshift_entries without checking that start + num_vals is still within the array. 2. the same unchecked logical-vs-array end in the bulk/merge fill paths. Next step is to assert start + num_vals <= max_values on entry to verify(), which turns whichever of these it is into a caught assertion in a Debug run rather than a fault in Release.

…nonunique) Both are correct in their own right and should be kept, but neither resolves the access violation, so both hypotheses in the previous commit are RULED OUT: - handle_underflow's borrow-from-left did 'num_vals++; rshift_entries(node)' unguarded. Taking one entry in at the FRONT is exactly what the gap is for, so it is now '--node.start' when a gap exists. (This is also the borrow the in-tree TODO was originally written for.) - split_to_insert passed leaf_node::max_values as a LOGICAL end offset to move_entries. It is num_vals - the node is full - so it now says that. Remaining hypothesis, and the one that fits the evidence best now that those two are excluded: some path fills a leaf to max_values without subtracting 'start', so a node that was relieved (and therefore has start > 0 and start + num_vals == max_values) is then appended to past the end of its array. The bulk/merge fill paths are the candidates - bulk_append_tail appends until num_vals == max_values. That points at the design rather than at a site: 'full( node )' means num_vals == max_values (true capacity), but a node with a gap only has max_values - start slots left. Two ways out: a. recentre eagerly - any path that wants capacity calls a recentre() first, so a gap never survives past the insertion it was opened for. Keeps every existing invariant, including split_to_insert's num_vals == max_values. b. make full() gap-aware - cheaper, but then a "full" node can hold fewer than max_values entries and split_to_insert's assumption has to go. (a) is the one to try: it is a precondition added at a handful of entry points rather than a change to what full() means. First: add 'start + num_vals <= max_values' to verify(), build Debug, and let the assertion name the site instead of guessing at it again.

A temporary probe in verify() (Release-visible fprintf, not an assert) now reports the invariant directly, and it moved the diagnosis twice: [GAP] start=14 num_vals=116 max=123 sum constant at 130 = max + 7 A constant sum meant inserts were doing the right thing and the bad state was created earlier - by relieve_into_sibling's give-to-right, which shifted the sibling by the FULL to_move from a base that already had a gap. The room check bounds num_vals + to_move, not start + num_vals + to_move. Fixed: the existing gap supplies part of the opening, so only the deficit is shifted and the gap is then closed. That is correct on its own terms and should be kept. With it fixed the probe reports a different, sharper state: [GAP] start=30 num_vals=123 max=123 - a node reached FULL while still carrying a gap of 30. insert() cannot produce that (its guard keeps start + num_vals constant, so num_vals reaches max only as start reaches 0), which leaves the paths that set num_vals directly: the bulk fill and merge lanes, which compute free capacity as max_values - num_vals and so overrun a node whose entries do not start at 0. That confirms the design point recorded earlier: 'full' means num_vals == max_values, but a node with a gap only has max_values - start slots. The fix is to recentre eagerly - a gap must not survive into any path that fills to capacity - and the sites to guard are the ones computing available space that way (bulk_append_tail and the merge in-place fill). Two tests fail: bp_tree.nonunique and bp_tree.playground.

key_at, shift_entries_left and node_capacity are members of bptree_base, which is a dependent base of the class templates calling them, and they were being named unqualified. Unqualified lookup does not consider dependent bases, so this is ill-formed - clang-cl accepts it under MS compatibility, and clang with libc++ rejects every one of them: error: explicit qualification required to use member 'key_at' from dependent base class error: use of undeclared identifier 'node_capacity' which is 60 errors across b+tree.hpp and b+tree/impl.hpp, i.e. the branch does not compile on Linux at all and nothing on Windows says so. Qualified the way the rest of the tree already does it: bptree_base:: for node_capacity (the form #155 introduced it with), base:: and impl_base:: for the two functions, per the alias in scope. No behaviour change - every name resolves to what it already resolved to on the toolchain that accepted it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

All three are one mistake in three places: a path sizes its write from num_vals and then addresses through key_at, which is keys[ start + i ] - so a node carrying a front gap is written past the end of its array by exactly 'start'. Together they close the devector fault: bp_tree. nonunique and bp_tree.playground both go from SIGSEGV to passing. append_and_free moves the source's entries to key_at( target, num_vals ) under a bound that only covers num_vals. Witnessed with a trap on the invariant: start=29, num_vals=123, max_values=123 - 29 entries past the end. relieve_into_sibling computes both siblings' room as max - num_vals, while its give-to-LEFT branch receives at the sibling's TAIL, where a front gap supplies nothing. With max=123, num_vals=100 and start=20 it hands over 11 entries into 3 free slots. The give-to-RIGHT branch is already correct for a different reason: it receives at the FRONT, where the gap IS the supply, and spends it before shifting the deficit - so the two sides genuinely count room differently, and the comment now says so. bp_tree_impl::merge has it twice over: available_space is again max - num_vals, and the move_backward that opens the merge point targets tgt_keys[ num_vals + copy_size ], where tgt_keys is &keys[ start ]. Witnessed: start=1, num_vals=123, max_values=123. The new recentre() closes the gap. It is safe for inner nodes and cheap, because it preserves every logical index - entry i sits at start + i before and at i after - so a child's parent_child_idx still names the same child. Each fix is necessary and none is sufficient: nonunique needs the first two, playground needs the third as well. Established in isolated clean builds, which is the only way any of it was decidable - testing one candidate while another site was still corrupting made it look like a no-op, twice. Found by giving verify() a defaulted std::source_location so the first trap names its own caller. Worth keeping in mind for the next one of these: the gap-touching paths call verify_min_max(), which checks num_vals against min/max and cannot see start + num_vals at all, so a violation created in one of them surfaces far away at whatever reader trips over it next. Full suite: 2049 passed, 1 pre-existing skip, 0 failed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

psiha added 4 commits September 9, 2026 02:11

psiha force-pushed the bt/7-devector-nodes branch from 944e568 to a29ae00 Compare September 9, 2026 10:34

psiha force-pushed the wip/devector-live branch from 154d7f3 to 3bc31d4 Compare September 9, 2026 10:34

psiha added 9 commits September 9, 2026 12:38

psiha force-pushed the bt/7-devector-nodes branch from a29ae00 to bc3ec57 Compare September 9, 2026 10:39

psiha force-pushed the wip/devector-live branch from 3bc31d4 to 0753fdf Compare September 9, 2026 10:39

psiha force-pushed the bt/7-devector-nodes branch 6 times, most recently from 1780d04 to 2785abf Compare September 9, 2026 12:01

This was referenced Sep 11, 2026

b+tree: qualify the dependent-base members, so this builds on Linux too #167

Merged

b+tree: three front-gap overruns — a node's entries do not start at zero #169

Merged

Navigation Menu

WIP: open and consume the node front gap — not correct yet - #160

WIP: open and consume the node front gap — not correct yet#160
psiha wants to merge 15 commits into
bt/7-devector-nodesfrom
wip/devector-live

psiha commented Sep 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

psiha commented Sep 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant