ZSET B+ Tree PR 3: Replace skiplist with FB+ Tree implementation#4206
Open
rainsupreme wants to merge 12 commits into
Open
ZSET B+ Tree PR 3: Replace skiplist with FB+ Tree implementation#4206rainsupreme wants to merge 12 commits into
rainsupreme wants to merge 12 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.04% +1.11%
==============================================
Files 164 166 +2
Lines 80707 86521 +5814
==============================================
+ Hits 62082 67523 +5441
- Misses 18625 18998 +373
🚀 New features to boost your workflow:
|
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
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.
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>
rainsupreme
force-pushed
the
oi/pr3-fbtree-v2
branch
from
July 20, 2026 04:51
f998461 to
99047d8
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
1 participant
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.
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: