Repository navigation
Reduce default registered read payload to 32 KiB - #302
Merged
Merged
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 51 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
⛔ Files ignored due to path filters (3)
📒 Files selected for processing (15)
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
Ordinary compact records and grouped reads do not need the existing 1 MiB default registered read payload. Reduce it to 32 KiB, retaining the existing oversized-read path. This is a draft because the measured SET difference is unresolved; it is not a demonstrated regression-free optimization.
Includes merged Bycorf PR eloqdata/bycorf#9, pinned to its main-branch commit
85d8c9c9e5372da79fbc41fbe9da71e34bb6ba24. The merged tree matches the previously validated dependency tree. Smaller slots allow more ordinary reads to reach SPDK before the registered pool applies backpressure, exposing a pre-existing qpair descriptor-exhaustion error. The runtime correctness fix is independently reviewable there.Behavior before and after
--storage-read-buffer-kb=1024restores the old slot size.Three alternating native-SPDK sessions per variant, 640 memtier connections, 1 KiB values, 1B existing keys, defrag enabled:
The primary comparison changes both runtime and slot size. Supplemental fixed-runtime/1 MiB measurements are near the baseline, but use a different sampling sequence and cannot conclusively attribute the SET difference.
Implementation
Change two defaults and correct a stale direct-read comment. Pin Bycorf to the deferred-submission fix with compact 48-byte request metadata. Initial submission reuses its already-resolved namespace/qpair; only deferred retries resolve the saved fixed-file index. Completion ownership comes from the existing worker TLS, with no per-request backend pointer or additional polling context. Add the bilingual benchmark report, original outputs, checksums and diagnostic evidence under
perf_reports/lavik-read-slots-32k-2026-10-09/.Design decisions and alternatives
32 KiB leaves room around common compact/grouped records without asserting a universal 16 KiB bound. Preserve explicit configuration and existing overflow handling.
Diagnostic scheduler tracing explains why fixed global concurrency can still exhaust one qpair: other workers continue returning requests while one owner is preempted, concentrating pending work there. Five workers' first captured pressure episodes show eight long polling gaps with 95.7–99.4% off-CPU time. Both diagnostic configurations have consistent completion accounting and local block-owner routing. These traces explain a saturation mechanism; they neither reconstruct every original error nor establish the SET slowdown's cause. Instrumented QPS is excluded from the performance comparison.
Documentation and comments
Update nearby capacity comments and the performance-report index. This default-size tuning changes no Lavik architecture claim, disk format or ownership model. Bycorf's queue lifetime/flow documentation is updated in the dependency PR. Temporary tracing code is not included in either PR.
Test plan
The Bycorf dependency also includes
bycorf_spdk_backpressure_stub_check, registered in its bypass CTest jobs. It exercises the actual backend with link-time device stubs and requires no SPDK hardware or hugepages. Local Clang 18 Debug and GCC 13 Release runs pass; disabling ENOMEM deferral makes the regression fail as expected. This adds CI correctness coverage without additional performance runs.Passed on the test host (build directory:
/mnt/dev/peer-bench/read-slots-32k-2026-10-09/build):36 unit checks and three grouped E2E cases passed. The archived protocol runner verifies strings through 2 MiB, large collection elements, long keys and restart recovery with both io_uring and SPDK. Native-SPDK test keys in DB 15 were deleted afterward; DB 0 remains at 1B keys. All 12 primary and six supplemental throughput points have no client errors; GETs have no misses.
The latest compact-metadata build and runtime hardware regression check pass four deterministic descriptor-exhaustion bursts; old runtime fails as expected. The earlier traced 32 KiB run (before metadata compaction) recovers all 138 initial transient submission failures. Both diagnostic sessions drain all accepted operations and exit cleanly. NVMe drivers, hugepages and unsafe no-IOMMU are restored. Raw commands, build flags, outputs and hashes for the original comparison are in the report archives. The layout-only runtime follow-up has no completed throughput comparison; its Release/SPDK build and hardware correctness check passed, including an accepted read completed by shutdown under the owning worker context.
Risk assessment
The smaller payload changes slot count and the point at which reads wait. The SET median is 2.04% lower in the primary comparison; do not merge this draft as a validated no-regression change. Default 64 MiB-budget throughput also remains unmeasured.
Rollback plan
Set
--storage-read-buffer-kb=1024or revert the default-size change. The independent Bycorf correctness fix can remain. No data reload or disk-format rollback is required.Reviewer guide
Review defaults and the dependency pin first, then the report's methodology and limitations. Review the runtime implementation in Bycorf PR #9. The evidence archives contain temporary diagnostic patches for reproducibility, not production code.
Follow-up work
Resolve the SET difference with a matched single-variable experiment before taking this PR out of draft.