perf(storage): reduce temporary work in grouped Hash and Set writes

[thweetkomputer]

Summary

Reduce temporary allocations and repeated metadata work in grouped Hash/Set point writes, based on perf samples of merged main a6e93d3d with 50,000 × 1 MiB Sets (128 B members).

• Replace an unchanged routing interval without erasing/rebalancing it first. Splits still retire parents before inserting children, with coverage/overlap validation intact.
• Route borrowed edit views in one contiguous list, preserving original operand order within each leaf, and inline the single-edit case.
• Inline single-page preflight/publication arrays; retain spill capacity and lifetime for multi-page batches.
• Use exact metadata lookup without constructing an iterator ancestor stack. Skip extent-index lookups for inline records.
• Replace an existing inline physical-index slot by copying its admitted compact arrays, preserving other coordinates and retirement bits; avoid expanding/re-encoding the entire page through a temporary vector. Pages with extents and structural changes retain the general merge. This also covers the shared GC relocation path.
• Copy into the pre-sized encoded payload directly; skip empty Set values. Yield between leaf edits rather than before each single-field write.

No value/page cache, durable-format change, admission bypass, or commit-notification change. Existing immutable snapshot ownership and corruption checks remain in place. No architecture claim changes.

Perf evidence and limits

All eight Hash/Set conditions now compare recovered main a6e93d3d with the current PR head 80792c41, using paired datasets: 50,000 × 1 MiB keys and 500 × 100 MiB keys, each with 128 B and 1 KiB entries. Clean eight-second write points at 80–5,120 connections give:

• Hash / 1 MiB / 128 B: 1.007–1.158× main.
• Hash / 1 MiB / 1 KiB: 1.025–1.194× main.
• Hash / 100 MiB / 128 B: 1.106–1.122× main.
• Hash / 100 MiB / 1 KiB: 1.065–1.158× main.
• Set / 1 MiB / 128 B: 1.084–1.179× main.
• Set / 1 MiB / 1 KiB: 1.119–1.191× main.
• Set / 100 MiB / 128 B: 1.160–1.359× main.
• Set / 100 MiB / 1 KiB: 1.077–1.249× main.

Separate 30-second write diagnostics (20 seconds sampled) support gains at higher connections. For example, Hash / 1 KiB at 1,280/5,120 connections is main 118,624/102,819 vs PR 136,227/117,466 QPS. Tail latency is variable; no uniform latency improvement is claimed. Kvrocks still has a substantial peak-throughput advantage on several write workloads. Peer durability/cache settings differ, as documented; no peer benchmarks were rerun.

Using all reported symbols at 1,280 connections, allocation/free self CPU shares decrease from 12.19% to 10.72% for Set / 128 B and 16.80% to 14.53% for Hash / 1 KiB. Active-group lookup drops from 2.01% to 0.19% and 2.43% to 0.93%. These are named-function CPU sample shares, not allocation counts or all inlined work. Physical-record lookup remains around 4–5%.

Read checks retain every lower point:

• Hash / 100 MiB / 1 KiB HGET is about 23% / 20% below main at 1,280 / 2,560 connections in the short grid; these points are retained. On one fixed dataset, with no intervening writes or perf, three 30-second HGET repeats at 1,280 connections give main 417,998 / 417,250 / 441,911 versus PR 412,563 / 405,788 / 411,345 QPS. PR mean is still 3.7% lower, with zero errors and unchanged cardinalities. The approximately 20% gap did not recur, but these few sequential repetitions cannot rule out a smaller regression. No point-read improvement is claimed. Raw repeats and provenance.

• Set / 128 B, three unprofiled 30-second SMEMBERS repetitions at 80 connections: main 1,966 / 1,967 / 1,945, PR 1,909 / 1,922 / 1,941 QPS (mean −1.8%).

• Hash / 128 B HGETALL: main 1,830 / 1,815 / 1,797, PR 1,861 / 1,857 / 1,842, followed by main again on the same dataset 1,842 / 1,838 / 1,842 QPS. PR / return-main mean is 1.007×. The earlier short-run drop was not established as a stable regression.

• Separate HGETALL profiles show the same memory-copy and vector-append hotspots in both versions; these existing costs are not removed by this write-focused change. Single-run connection grids and three-repeat sequential checks do not provide confidence intervals.

All 200 clean connection-grid points have zero errors and every key is validated before/after. Eight conditions and all 16 chart files are published. Per-point ratios and database peaks are inspectable alongside the source/binary manifest.

The 100 MiB Set / 128 B, 16-connection SMEMBERS short point was lower, so longer checks were added. Main returned OOM grouped operation scratch admission in its third repetition and PR returned the same error in its second. Neither process crashed; both closed normally afterward. The worker-local scratch gate and approximately 1.2 GiB reservation per full read are unchanged by this PR. Both failed sustained-read runs are disclosed beside the eight-second plot and excluded from aggregate throughput comparisons. Zero errors in the short grids does not establish sustained-read stability.

Embedded main/PR graphs, CPU evidence and raw results. Exact source/binary provenance and reproducible profiling/full-read scripts are included. Main grids and separate diagnostics remain distinct. The short grids run reads then writes for main followed by PR, so intervening writes can change physical layout; the extra read-only checks explicitly avoid those writes.

Validation

Compilation completed before any tests or benchmarks ran.

• RelWithDebInfo/SPDK build and Debug/io_uring fault-injection build.
• 77 grouped Hash/edit/object-index unit tests.
• 31 Hash/Set write end-to-end tests, including changed successor writes, pending-decision notification, commit failure, EXEC, crash recovery, OOM, duplicates and batch ordering.
• 12 shared grouped demotion/List/Sorted Set checks, including point updates, extent-backed entries, failure and recovery.
• Full pre-commit passed for all changed files.

Added directory coverage/snapshot regression coverage for mixed replacement/split batches, missing retirements, duplicate mutations and invalid aggregate counts.

The physical-index tests also verify untouched coordinates and retired prefix ancestors across the copied page and preserve old snapshots.

Summary by CodeRabbit

• Performance Improvements

  • Grouped hash lookups and updates now use more direct, efficient processing, including for single-record page updates.
  • Inline records avoid unnecessary extent metadata retrieval during reads.

• Bug Fixes

  • Group replacements and splits preserve routing coverage and existing snapshots, while invalid or duplicate updates are rejected.
  • Relocation and incremental updates preserve unaffected records and their metadata.

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Warning

Review limit reached

Next included review available in 9 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 887c732c-8d54-416b-9822-f09eedf10f8c

📥 Commits

Reviewing files that changed from the base of the PR and between 80792c4 and 31f0988.

📒 Files selected for processing (8)

• include/lavik/storage/detail/grouped_hash.h
• src/storage/engine/grouped_hash.cpp
• src/storage/engine/grouped_mutation.cpp
• src/storage/engine/grouped_object_index.cpp
• src/storage/engine/grouped_read.cpp
• src/storage/engine/hash_tree.cpp
• tests/grouped_hash_test.cpp
• tests/grouped_object_index_test.cpp

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: ba68e95a-acb4-43bc-9951-26a8100333a8

📥 Commits

Reviewing files that changed from the base of the PR and between a6e93d3 and 80792c4.

📒 Files selected for processing (8)

• include/lavik/storage/detail/grouped_hash.h
• src/storage/engine/grouped_hash.cpp
• src/storage/engine/grouped_mutation.cpp
• src/storage/engine/grouped_object_index.cpp
• src/storage/engine/grouped_read.cpp
• src/storage/engine/hash_tree.cpp
• tests/grouped_hash_test.cpp
• tests/grouped_object_index_test.cpp

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

---

Walkthrough

The changes add direct group lookup, revise directory replacement and grouped edit processing, and update mutation and physical-index paths. Tests cover directory batches, record relocation, and incremental index updates.

Changes

Grouped Hash Update and Read Paths

Layer / File(s)	Summary
Directory lookup and replacement include/lavik/storage/detail/grouped_hash.h, src/storage/engine/grouped_hash.cpp, tests/grouped_hash_test.cpp	HashGroupMap adds direct lookup. Directory operations use this lookup, sort updates, and check replacement and overlap conditions. Encoding copies nonempty byte views into the pre-sized buffer. Tests cover replacement, splitting, and invalid batches.
Mutation publication and physical index updates src/storage/engine/grouped_mutation.cpp, src/storage/engine/grouped_object_index.cpp, tests/grouped_object_index_test.cpp	Mutation collections use inline-capacity vectors and direct group lookup. An eligible single physical-index replacement updates compact page arrays and retains node metadata. Tests check relocation and incremental updates.
Grouped edit routing and payload access src/storage/engine/hash_tree.cpp, src/storage/engine/grouped_read.cpp	Grouped leaf edits use a sorted flat list and a reusable page-edit vector. Scratch admission visits each edited group once. Payload loading requests extent metadata only for external records.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Refactor

Merge Risk: ⚪ Minimal · up to 80792

The grouped Hash/Set optimizations have no established merge-blocking issue. Merge after normal build and test checks; runtime validation results supplied here are author-reported rather than independently reproduced.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 80792

The checked changes preserve identity validation, isolation and failure-handling controls. No introduced security weakness was established, but the available evidence does not support a complete assurance assessment.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The reviewed attacker-controlled fields and values reach the current key's directory-selected Hash/Set leaves. The changed routing and leaf validation do not establish expanded reachability into unrelated objects. Authentication, tenant isolation and deployment exposure outside this path were not established by the supplied coverage.

Trust Boundaries and Controls

• observed — Publication authority remains tied to the current population and logical snapshot. Foreground preparation checks database and replication epochs, index generations and logical-root identity; GC re-resolves the source after IO and compares the expected immutable object before installing its replacement under owner serialization.

Resilience and Maintainability Implications

• observed — Existing failure containment remains in the inspected paths: rejected GC copies are marked dead; failed grouped batches retire staged records or retain them behind an absent batch decision; failed publication handoff marks the decision failed and stops the writer rather than exposing an indefinitely pending graph. The inline-container substitutions retain coroutine-frame lifetime and do not change these policies.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 5.88% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 8 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the main change: reducing temporary work in grouped Hash and Set writes.
Description check	✅ Passed	The description provides detailed context, implementation changes, performance evidence, validation results, risks, and limitations. It does not use every template heading and does not explicitly prov…
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches 💡 1

🛠️ Fix failing CI checks 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Create a new PR

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit checks each hash with care,
Then sorts the edits in tidy rows.
It copies bytes that are there,
And leaves empty fields in repose.
Page by page, the records align,
The rabbit hops away at nine.}

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