Repository navigation
perf: edit grouped ZSet pages without per-member allocations - #359
Merged
thweetkomputer merged 3 commits intoOct 19, 2026
Merged
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 10
✨ Finishing Touches
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
A single-member grouped ZSet update currently materializes a string for every member in the affected score pages. This adds allocation and copying even when the edit preserves page IDs and links.
Behavior before and after
Single-member mutations can now keep a fully validated, owned encoded page and construct its after-image without per-member string allocations. Empty or overflowing pages still use the existing mutable retirement, redistribution and split planner. Durable formats, Redis semantics, TTL, dual-index publication and recovery boundaries are unchanged.
Implementation
LocalSharedPtrfor the shared byte block: snapshots stay on their key owner worker through decoding, staging, suspension and destruction, so no atomic reference count is needed. Debug builds check this thread affinity.86d686724ddd1c5617fb1b62f8131e05571e8423to587a9e776f9f64729456d6772596d53597f4a983, incorporating the receive-ring change in Use a resizable receive ring with one inline descriptor bycorf#17.Design decisions and alternatives
Keep complete input validation and copy each loaded page into owned storage so no physical read lease survives planning. Materialize member strings only when the existing topology-changing planner needs them. Batch and pop paths retain owned-member snapshots.
Documentation and comments
Updated the grouped-collection architecture's mutation ownership description. API and implementation comments cover ownership, validation, fallback and failure behavior.
Test plan
pre-commit run clang-format --from-ref upstream/main --to-ref HEADandgit diff --check: passed.99cc65efwith Bycorf587a9e7: GCC 13 Debug and Release/SPDK builds passed, as did all 192 selected tests (the same 179 Lavik cases, 11LocalSharedPtrownership/affinity cases, andbycorf_received_buffer_queue_check/bycorf_tcp_buffered_read_check). Validation completed on 2026-10-19 at 13:53 UTC.An 80-connection, 8,192-key crossover ran once per point, using the same data across versions. These measurements compare main
6a3d186cwith candidateb0e79a05, both using the earlier Bycorf86d68672, before the final dependency and worker-local ownership updates:The ZINCRBY mean gain is not a demonstrated sustained improvement. Candidate throughput fell from 37,775 to 35,456 to 27,269 ops/s across the three 20-second windows; the last window was only 1.6% above main. ZREM/ZADD improved 2.8% in this single fixed-order run. The 8 KiB control, which does not use the new grouped path, was effectively unchanged. The final dependency and worker-local ownership updates have not been benchmarked; no stable performance claim is made for them.
Risk assessment
The main risk is ownership or framing mistakes when an encoded page crosses planning and publication. Full validation, owned storage, explicit fallback and recovery tests cover these boundaries. The dependency update also changes receive-buffer queue ownership and is validated separately.
Rollback plan
Revert this PR; no durable-format migration or data conversion is required.
Reviewer guide
Start with the encoded owner/editor in
collection.handcollection.cpp, then the point-edit/fallback branch inzset.cppand the publication/demotion adapters. Review the Bycorf gitlink separately from the benchmarked change.Follow-up work
Investigate the late ZINCRBY throughput decline before claiming sustained gains. Existing measurements do not isolate device latency, completion scheduling or page-shape changes.