ZSET B+ Tree PR 3: Replace skiplist with FB+ Tree implementation - #4206
ZSET B+ Tree PR 3: Replace skiplist with FB+ Tree implementation#4206rainsupreme wants to merge 33 commits into
Conversation
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>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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. Comment |
Codecov Report❌ Patch coverage is
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
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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.
…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>
f998461 to
99047d8
Compare
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>
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>
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>
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 headersModified files:
ordered_index.c— rewritten as the fbtree adapterordered_index.h— iterator size increased to fit fbtreeIteratorserver.c/server.h—zsetExtractElementwith packed-item detection;zsetMarkLookupKey/zsetUnmarkLookupKeyfor heterogeneous hashtable lookup;OBJ_ENCODING_BTREEt_zset.c— lookup callsites wrapped with mark/unmark for hashtable operationsMakefile— skiplist.o removed, fbtree.o addeddefrag.c— renamed defragZsetSkiplist → defragZsetDeleted files:
skiplist.c,skiplist.hReviewing guide
Suggested reading order
fbtree.h(3 min) — public API surfacefbtree_internal.h(3 min) — node layout, constants, internal structsfbtree.c(30 min) — core tree operations: leaf ops, inner node routing, splits, iteration/seek, defrag scanordered_index.c(15 min) — adapter layer. Key areas: score-element packing, lex-range sentinel handling,seekForBoundhelper for positioned range operations, defrag bridgeserver.cchanges (5 min) —zsetExtractElementpacked-item path, mark/unmark helpers,zsetHashtableTypeDesign decisions
[8B score][element bytes], enabling standard sds comparison for tree orderingzsetExtractElement, unmark after lookupTesting
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).
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:
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: