Skip to content

ZSET B+ Tree PR 3: Replace skiplist with FB+ Tree implementation - #4206

Open
rainsupreme wants to merge 33 commits into
valkey-io:zset-btreefrom
rainsupreme:oi/pr3-fbtree-v2
Open

ZSET B+ Tree PR 3: Replace skiplist with FB+ Tree implementation#4206
rainsupreme wants to merge 33 commits into
valkey-io:zset-btreefrom
rainsupreme:oi/pr3-fbtree-v2

Conversation

@rainsupreme

@rainsupreme rainsupreme commented Jul 18, 2026

Copy link
Copy Markdown
Contributor

This is the third PR for implementing B+ tree for ZSET (#3166). After this PR I will merge the feature branch into unstable and submit future enhancements to the unstable branch.

Overview

This PR replaces the skiplist with a B+ tree as the OrderedIndex backend. The skiplist is fully removed. The encoding name changes from "skiplist" to "btree" in OBJECT ENCODING responses.

The implementation is called "fbtree" — short for FB+ tree or Feature B+ Tree, a B+ tree variant that stores per-subtree feature metadata (subtree size, a few distinguishing bytes) in inner nodes to minimize memory fetches. See issue #3166 for details (design, performance, memory overhead)

What's included

New files:

  • fbtree.c — the B+ tree implementation (insert, delete, split, seek, iterate, defrag)
  • fbtree.h / fbtree_internal.h — public and internal headers

Modified files:

  • ordered_index.c — rewritten as the fbtree adapter
  • ordered_index.h — iterator size increased to fit fbtreeIterator
  • server.c / server.hzsetExtractElement with packed-item detection; zsetMarkLookupKey/zsetUnmarkLookupKey for heterogeneous hashtable lookup; OBJ_ENCODING_BTREE
  • t_zset.c — lookup callsites wrapped with mark/unmark for hashtable operations
  • Makefile — skiplist.o removed, fbtree.o added
  • defrag.c — renamed defragZsetSkiplist → defragZset

Deleted files:

  • skiplist.c, skiplist.h

Reviewing guide

  • fbtree is written as a general-purpose ordered collection of strings. It only depends on sds for string operations.
  • ordered_index.c is the adapter which uses fbtree to implement the ZSET OrderedIndex interface (score packing, lex range sentinels, seek helpers).
  • hashtable does not currently support heterogeneous lookup, which is where the stored entry and the lookup key are different types. To avoid allocating a temporary packed entry for every hashtable lookup operation I've cooked up a scheme to mark the lookup string sds so hashtableType methods can differentiate between packed entry and simple lookup string. (Not pretty. I plan to separately propose hashtable changes to make this unnecessary.)

Suggested reading order

  1. fbtree.h (3 min) — public API surface
  2. fbtree_internal.h (3 min) — node layout, constants, internal structs
  3. fbtree.c (30 min) — core tree operations: leaf ops, inner node routing, splits, iteration/seek, defrag scan
  4. ordered_index.c (15 min) — adapter layer. Key areas: score-element packing, lex-range sentinel handling, seekForBound helper for positioned range operations, defrag bridge
  5. server.c changes (5 min) — zsetExtractElement packed-item path, mark/unmark helpers, zsetHashtableType
  6. Tests — encoding assertions updated from "skiplist" to "btree"; unit tests exercise orderedIndex functions directly (which now route to fbtree)

Design decisions

  • Node sizing — leaf nodes are 512 B and inner nodes 2048 B, tuned to exactly fill jemalloc size classes at 61-way fanout
  • No merge on delete — simplified initial implementation. Leaves can become sparse; background compaction is planned for a later PR
  • Score+element packed into sds — each leaf item is a single sds containing [8B score][element bytes], enabling standard sds comparison for tree ordering
  • Lookup-key marking — hashtable callbacks must distinguish packed fbtree items (skip 8B score prefix) from plain lookup keys. Solution: transiently mark lookup keys via sds aux bit (sdshdr8+) or unused type value (sdshdr5), check in zsetExtractElement, unmark after lookup
  • Sentinels handled in adapter — fbtree.c is a generic B+ tree with no ZSET knowledge; lex-range sentinel handling lives in the adapter
  • O(log N) defrag scan — uses rank-based tree navigation to resume from cursor position, processes 4 full leaves per call (~244 items)
  • Cursor-between-items iterator — fbtreeSeekToRank(N) positions cursor so next() returns rank N and prev() returns rank N-1, matching the Java ListIterator semantics established in PR 2

Testing

  • 275 unit tests pass — 193 new in test_fbtree.cpp (tree operations, SIMD feature search, property/fuzz tests), plus the 82 existing ordered-index tests from PR 2, unchanged and now exercising fbtree
  • 337 integration tests pass

Benchmark Results

ARM Graviton 3 (c7g.metal, 64 cores), io-threads=9, pipeline=10, 3M-member zset, 5 repetitions per test. 95% confidence intervals are ≤2% for all tests except ZRANDMEMBER (±5%, low absolute throughput).

Command Workload Skiplist fbtree Change
ZADD Score update (all hits) 205K 421K +105%
ZREM 50/50 random add/remove 386K 680K +76%
ZPOPMIN Pop min + append new member 1.08M 1.08M 0%
ZRANGE 100-element rank scan 174K 183K +6%
ZRANGEBYSCORE 100-element score scan 95K 96K +1%
ZRANDMEMBER 100 random samples 5.0K 5.1K +2%
ZRANK Point rank lookup 692K 825K +19%
ZCOUNT Score-range count 720K 917K +27%
ZSCORE Point score lookup (hashtable) 1.44M 1.46M +2%

Mutations that reposition elements in the ordered index (ZADD, ZREM) show the largest gains from improved cache locality. ZRANK and ZCOUNT benefit from O(log N) rank arithmetic. Scan operations are unchanged, as expected. ZSCORE and ZRANDMEMBER are hashtable-based and perform identically since the ordered index is not involved. Range operations are unchanged because they are bottlenecked by the large reply size.

Memory Overhead

5M-member zset, 20-byte members (+ 8-byte score), measured via jemalloc heap profiling. Overhead is per-member bytes excluding user data, measured across two insertion patterns that produce different ordered-index occupancy:

Insertion pattern Skiplist fbtree Savings
Sequential (ascending scores, dense packing) 50.3 B 28.5 B −21.8 B (−43%)
Random (scattered scores, fresh fill) 50.3 B 32.0 B −18.2 B (−36%)

fbtree overhead scales with leaf occupancy: dense sequential fills pack tightest (−43%), while scattered random fills leave leaves partially full (−36%). The skiplist stays flat at ~50.3 B across both patterns, since per-node cost is independent of insertion order and every node is freed on delete.

Scope boundaries

This PR does NOT:

  • Implement merge/compaction (planned for a later PR)
  • Add the heterogeneous hashtable lookup that would make the sds marking scheme unnecessary (separate PR)

Remove skiplist entirely and implement OrderedIndex directly with fbtree.
This absorbs what was previously planned as PR 3 + PR 4 + PR 5.

Key changes:
- ordered_index.c is now the fbtree adapter (no vtable, no dispatch)
- ordered_index.h unchanged (public API preserved from PR 2)
- OBJ_ENCODING_SKIPLIST -> OBJ_ENCODING_BTREE
- Lookup-key marking (zsetMarkLookupKey/zsetUnmarkLookupKey) for
  hashtable callback disambiguation between packed items and plain sds
- Defrag: void*(*defragfn)(void*) with sdsAllocPtr offset dance in fbtree.c
- Iterator: cursor-between-items model (fbtreeSeekToRank gives correct
  semantics for next()/prev() without adaptation)
- Unit tests for the tree (FbtreeTest) and its SIMD feature search
  (FeatureSearchTest)

Bug fix: zsetExtractElement must handle SDS_TYPE_5 marked keys specially
since sdslen() returns 0 when type bits are overwritten with the marker.
Read length directly from flags byte for marked SDS_TYPE_5.

337/337 zset integration tests pass.
All aofrw, keyspace, sort, memefficiency, scan tests pass.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Replace the linear leaf-walk (O(range size)) with two rank-returning
seeks: count = end_rank - start_rank. fbtreeSeekToScore returns the rank
of the first item >= the seek key in O(log N) using child_sizes descent.

Boundary handling:
- min inclusive -> seek(min); min exclusive -> seek(min+1)
- max exclusive -> seek(max); max inclusive -> seek(max+1)
where +1 is the next representable score in sortable space.

No iterator step-back needed (that's only for cursor positioning, not
rank counting). +inf upper bound is safe: seeking past tree max clamps
to length, giving the correct end_rank.

337/337 integration, all unit tests pass.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
fbtreeSeekToValue now returns the rank of the first item with packed
value >= the seek key (mirroring fbtreeSeekToScore), accumulated from
child_sizes during the existing tree descent. Existing void-discarding
callers are unaffected.

orderedIndexCountLexRange now computes count = end_rank - start_rank
instead of walking leaves. Since lex elements are variable-length (no
'next representable' value), inclusive/exclusive boundaries are resolved
by probing whether the bound itself is present via fbtreeGetAtRank +
sdscmp -- correct because packed (score,element) values are unique.

337/337 integration, all unit tests pass. ZLEXCOUNT boundary smoke test
covers inclusive/exclusive ends, sentinels, single-element and empty
ranges.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
The three lex-range functions (DeleteRangeByLex, CountLexRange,
SeekToLexRange) grab the shared score prefix from the lowest element.
fbtreePeekMin reads leftmost_leaf->values[0] directly (O(1)) instead of
fbtreeGetAtRank(fbt, 0)'s full root-to-leaf descent (O(log N)). Same
result, clearer intent.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
fbtreeNext/fbtreePrev previously returned bool and wrote the element
through a `const_sds *pos` out-parameter. Return the element directly
(const_sds, NULL on end-of-iteration) instead.

Rationale:
- Matches the convention already used by orderedIndexNext/Prev and
  fbtreeGetAtRank, so the internal iterator reads the same as every
  consumer (`while ((x = fbtreeNext(&it)) != NULL)`).
- Collapses the orderedIndexNext/Prev wrappers from a bool->pointer
  bridge to a single cast.
- Removes a redundant return channel: the bool and the out-parameter
  both encoded end-of-iteration. NULL is an unambiguous sentinel here
  because stored elements are always valid sds and can never be NULL.

Behavioral note: the old bool form left *pos untouched on a false
return. MultilevelMixedIteration relied on that to read the last
element after draining a loop; with the new form that value is NULL,
so the test now captures the last non-null element explicitly. All
other call sites are mechanical conversions to the assignment-in-
condition idiom and ASSERT_NE/ASSERT_EQ against nullptr.

Tested: 161 FbtreeTest, 82 ordered-index unit tests, 337 zset
integration tests; build and clang-format clean.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
… leaf searches

Extract the shared root->split boundary-path descent into buildBoundaryPaths(),
used by both range-delete and the new range-count path, so the two are no longer
duplicated. Add fbtreeCountRangeByScore/fbtreeCountRangeByValue which count via a
single shared descent plus rank arithmetic (end_rank - start_rank) instead of two
independent seeks, and wire ZCOUNT (orderedIndexCountScoreRange) and ZLEXCOUNT
(orderedIndexCountLexRange) to them.

For the common split case (the two range boundaries land in different leaves),
resolve the two leaf indices with resolveBothIdxPrefetch(): a single-threaded,
software-pipelined pair of binary searches that interleaves the two independent
leaves and prefetches both probe payloads per step, so their cache misses overlap
(memory-level parallelism) instead of serializing. Each leaf value is a pointer to
a separate sds allocation, so every probe is a miss; this targets the ZCOUNT
regression on wide-ish ranges whose boundaries span separate leaves.

Delete callers now resolve start_idx/end_idx after the descent (behavior
unchanged; buildBoundaryPaths is descent-only).

Tests: 243 fbtree + ordered-index unit tests pass; zset ZCOUNT/ZLEXCOUNT
integration tests pass; build clean under -Werror -Wpedantic -Wextra with
ORDERED_INDEX_FBTREE=yes.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
…verse results

fbtreeNext/fbtreePrev only set ITER_AT_POSITION in their first-call
(current_leaf == NULL) branch. When the iterator was positioned by a seek that
landed on the leftmost element, fbtreeSeekToValue/fbtreeSeekToScore leave it in
ITER_BEFORE_START; fbtreeNext then returns the element but never clears that
state, so a following fbtreePrev early-returns NULL and the element is dropped.
Symmetric issue for ITER_PAST_END with fbtreePrev-then-fbtreeNext.

This surfaced as ZREVRANGEBYLEX/ZREVRANGEBYSCORE returning [] instead of the
smallest matching element whenever the range resolves to just that element
(e.g. `ZREVRANGEBYLEX z [a [a` with a as the rank-0 member), and as an
intermittent failure of the ZRANGEBYLEX fuzzy integration test.

Fix: set ITER_AT_POSITION whenever Next/Prev returns a valid element. Being at
the iterator level, this covers every seek-then-reverse caller.

Adds targeted unit tests for the boundary-transition family: seek-to-rank-0 then
Next-then-Prev (score and value seeks, incl. the sole-element case), seek below
all, and the PAST_END Prev-then-Next symmetric case. Reverting the fix fails
three of them.

Tests: 249 unit tests pass; a seeded ZRANGEBYLEX/ZLEXCOUNT fuzzer runs ~1.5M
queries clean across 5 seeds (incl. one that reproduced the failure pre-fix);
zset integration green over repeated runs.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Add fbtreeHeight() which descends the leftmost spine in O(height), and
use it to replace the orderedIndexGetHeight stub that returned 0. Add a
unit test covering empty, single-leaf, leaf-overflow, and three-level
trees.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
The forward+exclusive and reverse+inclusive cases re-sought the tree
(a second O(log n) descent) to position the cursor after peeking the
boundary element. A single O(1) fbtreePrev() step lands in the same
place. Also document the offset-encoding convention shared by
orderedIndexSeekToScoreRange and orderedIndexSeekToLexRange.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Merge test_fbtree_feature_search.cpp into test_fbtree.cpp, matching the
one-test-file-per-module convention in src/unit. The FeatureSearchTest
suite name is unchanged, so filtering and output grouping still work.
The local FEATURE_SIZE/FEATURE_ROW_SIZE/FEATURE_BIAS definitions are
dropped in favor of fbtree_internal.h (already included), and a stale
comment referencing fbtree_ordered_index.c is corrected to fbtree.c.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
@coderabbitai

coderabbitai Bot commented Jul 18, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 8ebf601b-8351-4e88-8486-dc45550b38a2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@rainsupreme rainsupreme moved this from Todo to Needs Review in Valkey 9.2 Jul 18, 2026
@codecov

codecov Bot commented Jul 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.62306% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.23%. Comparing base (ea14eeb) to head (ccfd875).
⚠️ Report is 1 commits behind head on zset-btree.

Files with missing lines Patch % Lines
src/module.c 0.00% 7 Missing ⚠️
src/ordered_index.c 99.56% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff               @@
##           zset-btree    #4206      +/-   ##
==============================================
+ Coverage       76.92%   78.23%   +1.31%     
==============================================
  Files             164      166       +2     
  Lines           80707    87024    +6317     
==============================================
+ Hits            62082    68085    +6003     
- Misses          18625    18939     +314     
Files with missing lines Coverage Δ
src/aof.c 80.40% <100.00%> (+0.09%) ⬆️
src/db.c 94.69% <100.00%> (-0.27%) ⬇️
src/debug.c 55.24% <100.00%> (+0.07%) ⬆️
src/defrag.c 80.13% <100.00%> (-1.00%) ⬇️
src/fbtree.c 95.47% <ø> (ø)
src/geo.c 94.35% <100.00%> (+0.03%) ⬆️
src/lazyfree.c 88.38% <100.00%> (ø)
src/object.c 90.28% <100.00%> (+1.70%) ⬆️
src/rdb.c 76.96% <100.00%> (-0.02%) ⬇️
src/server.c 89.54% <100.00%> (+0.02%) ⬆️
... and 8 more

... and 16 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@valkey-review-bot valkey-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found three regressions in the new BTREE backend: signed-zero scores stop behaving like numeric zero in score-range operations, the child-save dismiss path no longer releases packed member payloads, and identical packed duplicates can become unreachable after a leaf split.

Comment thread src/ordered_index.c
Comment thread src/fbtree.c
Comment thread src/fbtree.c
@rainsupreme
rainsupreme marked this pull request as ready for review July 18, 2026 09:03
…ting

Three fixes for the regressions found in review:

1. Canonicalize negative zero in scoreToSortable. IEEE -0.0 and +0.0
   have different bit patterns, so the sortable-key transform mapped
   them to distinct tree keys while the previous skiplist compared
   scores numerically (-0 == 0). Members stored with score -0 became
   invisible to ZRANGEBYSCORE/ZCOUNT/ZREMRANGEBYSCORE on 0. Collapse
   -0.0 to +0.0 before packing.

2. Dismiss packed member payloads in fbtreeDismissMemory. The packed
   items are allocated separately from the 512-byte leaf nodes, so
   madvising only the leaves released nothing; dismissZsetObject gates
   dismissal on page-sized average member size, which is exactly when
   the member payloads dominate. Walk each leaf's items and dismiss the
   sds allocations as well.

3. Route duplicate lookups through equal-anchor siblings. Leaf items
   are matched by pointer identity, but the descent routed only to the
   leftmost child with an equal anchor. After a leaf split, duplicate
   (score, element) pairs span sibling leaves, leaving later instances
   unreachable for delete and rank lookup. Continue into the next
   sibling while the child's high key equals the item value.

Each fix comes with a unit test that fails without it (signed-zero
range ops, dismiss walk integrity, several-leaves-of-duplicates rank
and pointer-delete), plus a zset integration test for signed zero.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
The ENGINE_SERVER_OBJ list is alphabetically ordered. skiplist.o was
originally placed next to t_zset.o, ordered_index.o inherited that slot
through the rename, and fbtree.o followed the same pattern. Move both
to their alphabetical positions.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Comment thread src/ordered_index.c
Comment thread src/fbtree.c Outdated
Comment thread src/fbtree.c Outdated
Comment thread src/ordered_index.c Outdated
The maxstring sentinel packed to [score][0xFF] as an upper bound, but a
single byte suffix cannot dominate its own continuations under sds
comparison: a strict prefix sorts before longer strings, so members with
a leading 0xFF byte sorted past the bound and escaped
ZREMRANGEBYLEX key - + and were undercounted by ZLEXCOUNT. Bound the
range with the next score bucket's 8-byte prefix, exclusive, instead.
Valid scores never map to an all-ones sortable value (that bit pattern
is a NaN), so the increment cannot wrap.

Adds a unit test with 0xFF-leading members (fails without the fix) and
a zset integration test.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
The score-only descent extracted FEATURE_SIZE feature bytes starting at
prefix_len without checking against the 8-byte key length. With a
shared anchor prefix of 5 to 7 bytes (clustered scores differing only
in their low mantissa bytes), the extraction read up to 3 bytes past
the caller's 8-byte score buffer. Pad positions past the key with zero,
the minimal continuation, matching findChildIndex's handling of keys
shorter than the feature window.

Adds a test inserting adjacent representable doubles so inner-node
anchors share most of their score prefix; AddressSanitizer reports a
stack-buffer-overflow at the extraction site without the fix.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Inner nodes whose shared prefix exceeds EMBED_PREFIX_LEN store it in a
separately allocated buffer (innerNodeSetPrefix). freeNodeRecursive
releases that buffer, but the five paths that free an emptied child
directly — the two pointer-delete empty-child sites and the three
range-delete boundary sites — called zfree on the node alone, orphaning
the buffer. Release the spilled prefix before freeing inner nodes at
all five sites. Dying boundary leaves are unlinked by the range
delete's leaf-stitching pass, so the boundary sites only need the
inner-node prefix release.

Adds two tests that collapse a three-level tree of long-prefix members
through both paths; LeakSanitizer reports the orphaned buffers without
the fix.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
The estimator returned a hardcoded 64 bytes per item, ignoring the
sample_size that MEMORY USAGE forwards and losing the sampling accuracy
the skiplist estimator provided. Sample item allocation sizes from the
leftmost leaves and extrapolate, and add structural overhead from the
leaf count (exact when the sample covers the whole chain, otherwise
extrapolated from the sampled fill) with inner levels shrinking
geometrically by the fanout.

Adds a test verifying estimates scale with actual member sizes.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Move payload sampling from the ordered index into objectComputeSize,
matching the pattern the hash and set encodings use: containers report
their structural memory (hashtableMemUsage, and now
orderedIndexEstimateStructureMemory), while the object layer samples
element payloads. The hashtable entries are the packed items shared
with the ordered index, so sampling them covers the member payloads,
and hashtable order does not correlate with member size the way the
index's score order can.

The ordered index API drops the sample_size parameter: item payloads
are owned jointly with the caller, so the index only accounts for its
own nodes, with leaf count extrapolated from the fill of the first
leaves.

Adds a MEMORY USAGE integration test and reworks the estimator unit
test for the structural contract.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Raw ASSERT_EQ on doubles instantiates gtest's operator== comparison,
which trips -Werror -Wfloat-equal on the macOS build. The fixture's
assertScore helper (ASSERT_DOUBLE_EQ) is the established comparison for
scores.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Comment thread src/fbtree.c Outdated
Comment thread src/ordered_index.c Outdated
Comment thread src/fbtree.c Outdated
The second check's condition is the first check's condition with an
additional conjunct, so any state satisfying it already returned on the
preceding line.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
deleteRangeSameLeaf released emptied children with a bare zfree, which
orphaned the heap prefix buffer of inner nodes whose shared prefix
exceeds EMBED_PREFIX_LEN. Consolidate the release pattern into
freeEmptyNode (unlinks leaves from the leaf chain) and
freeEmptyNodeAlreadyUnlinked (for range-delete boundary subtrees, whose
dying leaves are already unlinked by the leaf-chain splice) and use them
at all six release sites.

Add a same-leaf range-delete collapse test that empties a spilled-prefix
tree through single-member lex range deletes; LeakSanitizer fails it
without the fix (310 bytes leaked in 1 allocation).

Signed-off-by: Rain Valentine <rsg000@gmail.com>
orderedIndexVerifyIntegrity reported the same generic message for every
failure. Thread an error buffer through fbtreeDebugValidate so the first
failed check is described with its location and values (leaf overflow,
anchor/prefix/feature mismatches, stored-vs-actual subtree sizes and
child item counts, tree size and leaf cache mismatches), restoring the
per-failure detail the DEBUG consumers had with the skiplist validator.

Add a test that corrupts an inner node's stored subtree size and checks
the reported message names the failed check.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Casting node * directly to innerNode * trips clang's -Wcast-align (the
common initial-sequence node header has weaker alignment than the full
innerNode struct). Casting through void * expresses the intentional
downcast; the pointer is always a real innerNode allocation since the
test asserts the root is not a leaf.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Comment thread src/fbtree.c
Comment thread src/unit/test_fbtree.cpp
Inner nodes store each child's high key as an anchor by pointer. When
fbtreeDefragScan relocated a leaf's last item it rewrote leaf->values[]
but left those anchors pointing at the freed block, so a later descent
that compared or read an anchor hit freed memory.

Record the descent path and, on relocating a leaf's last item, republish
the new pointer to the direct parent's anchor and up the enclosing
subtree while the leaf remains its rightmost leaf. Descent now processes
one whole leaf per call so each step carries a fresh path; the caller
already loops the scan under its time budget.

The new gtest relocates every allocation through a force-copy defragfn
and validates the tree plus full in-order content after a sweep;
AddressSanitizer reports the use-after-free without the fix.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
fbtreeDefragScan relocated a leaf's items but never the leaf node
itself, so active-defrag could drain the pages around a leaf yet never
move the leaf off a fragmented page. Relocate the leaf before its items
and repoint every external reference from the recorded descent path: the
parent's child pointer (or the tree root for a single-leaf tree), the
leaf-chain neighbors, and the leftmost/rightmost caches.

The new gtest force-relocates every leaf across a sweep and checks the
tree validates, iterates identically in both directions, and answers
rank seeks correctly.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Active-defrag relocated leaves and items but never the tree's inner
nodes, so a fragmented page holding an inner node could not be
reclaimed. Add fbtreeDefragNodes, a depth-first pass that relocates every
inner node struct and repoints the parent child pointer (or the root),
and run it from orderedIndexDefragInternals before the rank scan. Inner
nodes are few and fixed-size, so relocating them all in one pass is
cheap; leaves and items stay in the incremental scan.

The new gtest force-relocates every inner node across single-leaf (no
inner nodes), two-level, and three-level trees and checks the result
validates and iterates unchanged.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
fbtreeItemDefragFn and fbtreeLeafDefragFn were declared but never used,
and their comment described a leaf callback parameter the scan does not
take. Remove both. Rename defragZsetNodeCallback to
defragZsetItemCallback: it runs when a packed member item is relocated
and updates the companion hashtable pointer, not a node. Refresh the
scan/internals doc comments to describe the per-leaf scan and the
inner-node relocation split.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Cover the paths the single force-relocate-everything test did not: a
sweep visits each item exactly once and terminates; partial relocation
(only every Nth allocation moves); a no-op defragfn leaves the tree
unchanged; a single-leaf tree relocates its root leaf (the depth==0
path); the node pass followed by a full item sweep validates and answers
forward, backward, and rank queries; and dismiss preserves content.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
The active-defrag tests are written for readability by C developers (per
CONTRIBUTING): verify order and content by iterating the tree with
fbtreeNext/fbtreePrev and comparing each element to a formatted key
buffer via memcmp, using fixed C arrays and index loops. Drops the
std::vector/std::string expectations, std::reverse, and
range-for-over-initializer-list the first drafts used.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
qsort declares its array argument non-null, so calling it with a NULL
pointer and count 0 (as the full-delete consistency tests do once the
index is empty) is undefined behavior that UBSan flags. Return early
when the count is zero.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
NodeAllocationSizes asserts zmalloc_usable_size returns the exact
jemalloc size class for the node structs. Other allocators round
differently (e.g. glibc returns 2056/520), so skip it unless jemalloc is
in use, matching the existing jemalloc-only test guard in test_entry.cpp.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
orderedIndexItemCreate, orderedIndexItemSetScore, and orderedIndexInsertItem back the store-aggregation path (ZUNIONSTORE/ZDIFFSTORE/ZINTERSTORE) but had no direct unit coverage; ItemSetScore's no-reposition branch is reachable only through this path. Cover create-then-set-then-insert ordering, interleaving with existing items, and a multi-level tree build.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
orderedIndexDefragInternals and orderedIndexScanDefrag are the wrappers defrag.c calls, but only the fbtree-layer primitives had gtests. Cover the two-phase sweep with a force-relocate defragfn (struct, nodes, leaves, and items all move), the item-relocation callback firing once per moved item, a no-op defragfn, and the empty-index case.

Signed-off-by: Rain Valentine <rsg000@gmail.com>
The OrderedIndex is fbtree-backed; the overview comment still described it as a skiplist with the B+ tree "planned", and the height getter referred to "skiplist levels".

Signed-off-by: Rain Valentine <rsg000@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

2 participants