perf: deep-review hot-path optimizations (storage, WAL, SPSC, protocol, vector, io_uring)#168
Merged
Merged
Conversation
…policy parse Database::get previously probed the DashTable three times on a live hit (expiry check, is_some guard, return get) and twice on a miss. A single probe now classifies the key as Live/Expired/Absent; NLL forces one re-probe on the live path (the cold-tier fallback mutates self), giving 2 probes on hit and 1 on miss. EvictionPolicy::from_str allocated a lowercase String per call — invoked once per write command under memory pressure. Replaced with eq_ignore_ascii_case table matching; EvictionPolicy now derives Copy. Deep-review findings ST-F1 (P0) and ST-F4. Behavior unchanged; covered by existing storage::db and storage::eviction unit tests. author: Tin Dang
… checkpoint state WAL v2 do_write issued three write(2) syscalls per 1ms flush (7B header, payload, 4B CRC). The full block frame is now assembled in a reusable staging buffer and written with one syscall; on-disk layout unchanged: [block_len:4][cmd_count:2][db_idx:1][payload][crc32:4]. write_wal_v3_record called payload.to_vec() on every non-FPI record to type-unify with the LZ4 branch — an unconditional heap alloc per append in disk-offload/CDC mode. Cow<[u8]> keeps the common path borrowed. CheckpointState::advance_tick cloned the state enum every 1ms tick; all fields are scalar so it now derives Copy. Deep-review findings PS-F2 (P0), PS-F1, PS-F6. Format compatibility covered by existing wal/wal_v3/checkpoint round-trip tests (70+48+11). author: Tin Dang
drain_spsc_shared allocated two fresh Vec<ShardMessage> on every call — it runs on the 1ms tick and every I/O select arm, so ~2K+ allocations/s per shard even when idle. The batch buffers are now thread-local scratch (one shard per OS thread) taken with mem::take for re-entrancy safety and returned drained, retaining capacity across cycles. Deep-review finding HP-F2 (P0). Behavior unchanged; covered by existing shard unit tests. author: Tin Dang
Frame::Double heap-allocated via format!/to_string in both the RESP2
downgrade and native RESP3 paths — on the response hot path for graph
commands and RESP3 clients. A 400-byte stack formatter (F64Display)
now renders through the same std Display impl, guaranteeing
byte-identical wire output (Display never switches to scientific
notation, unlike ryu); inf/nan use static slices.
New tests pin F64Display against format!("{}") across extremes
(f64::MAX, 5e-324 subnormal — the ~326-char worst case that guards the
buffer capacity) and assert RESP2/RESP3 wire bytes.
Deep-review finding HP-F8 (P1).
author: Tin Dang
…iltered Three per-query allocations introduced with SQ8 (PR #166) removed: - hnsw_search_filtered allocated a Vec<f32> query copy per SQ8 query, breaking the VEC-HNSW-03 zero-allocation guarantee. Now an inline SmallVec<[f32; 512]> (stack for all realistic dims). - Both SQ8 brute-force paths (filtered + MVCC) did query_f32.to_vec() per search. New Sq8Query helper borrows the raw query outright for L2 (encode side stores raw values) and inline-normalizes for the unit-sphere metrics. - search_filtered allocated q_rotated + lut_buf per IVF segment inside the fan-out loop; hoisted above the loop, mirroring the fix already applied to search_mvcc. Deep-review findings VE-F1 (P0), VE-F2 (P0), VE-F10. Recall/correctness covered by the existing 531 vector unit tests incl. SQ8 exact-match, cosine ranking, IP angle-ranking and persistence round-trip suites. author: Tin Dang
drain_completions allocated two Vecs per CQE batch (decoded-CQE staging + event list), called on every eventfd wake and the 1ms tick fallback. The CQE staging now lives in a persistent driver field (mem::take dance avoids the ring/buf_ring borrow conflict), and the event list is loaned to the event loop via take_event_scratch/return_event_scratch so the drain loop is allocation-free at steady state. The allocating drain_completions() remains as a thin wrapper. Deep-review finding HP-F3 (P0). Linux-only code; verified with a cold cargo check on the moon-dev VM. author: Tin Dang
Linux clippy (default features) flags clippy::large_enum_variant on Sq8Query. The 2KB inline variant is the intended stack buffer that keeps the SQ8 per-query path heap-free; add a per-item allow with rationale. author: Tin Dang
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR removes per-call heap allocations across hot paths by reusing scratch buffers, introducing stack-backed/borrowed buffers and formatters, deriving Copy on small enums, and consolidating writes into single buffered syscalls. ChangesAllocation-Free Optimization Pass
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/protocol/serialize.rs (1)
702-714: 💤 Low valueConsider adding direct wire tests for NaN serialization.
The finite and infinity paths are well-tested, but there's no direct assertion that
Frame::Double(f64::NAN)produces the expected wire bytes ($3\r\nnan\r\nfor RESP2,,nan\r\nfor RESP3). Round-trip tests don't work for NaN due toNaN != NaN, so a direct wire-format assertion would close this gap.🧪 Optional test to cover NaN wire format
#[test] fn test_serialize_double_nan_wire_format() { // RESP2: NaN → BulkString "nan" let buf = serialize_frame(&Frame::Double(f64::NAN)); assert_eq!(&buf[..], b"$3\r\nnan\r\n"); // RESP3: NaN → native double "nan" let buf3 = serialize_resp3_frame(&Frame::Double(f64::NAN)); assert_eq!(&buf3[..], b",nan\r\n"); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/protocol/serialize.rs` around lines 702 - 714, Add a direct wire-format unit test for NaN by creating a test (e.g., test_serialize_double_nan_wire_format) that calls serialize_frame(&Frame::Double(f64::NAN)) and asserts the returned bytes equal b"$3\r\nnan\r\n", and also calls serialize_resp3_frame(&Frame::Double(f64::NAN)) and asserts the returned bytes equal b",nan\r\n"; reference the existing helpers serialize_frame, serialize_resp3_frame and the Frame::Double variant so the test mirrors the other double serialization tests.src/io/uring_driver.rs (1)
302-303: ⚡ Quick winSize the scratch buffers from the configured ring depth.
ring_sizeis user-configurable, but both scratch vecs always start at 256. Any shard running with a larger ring will reallocate on its first busy batch, which defeats the steady-state allocation win this change is aiming for.cqe_scratchshould track CQ depth (ring_size * 2), andevent_scratchlikely wants one more step of headroom because some CQEs emit twoIoEvents.Suggested sizing change
- Ok(Self { + let cqe_scratch_cap = (config.ring_size as usize).saturating_mul(2); + let event_scratch_cap = cqe_scratch_cap.saturating_mul(2); + Ok(Self { ring, fd_table, buf_ring, send_buf_pool, connections: HashMap::new(), @@ pending_sqes: 0, tick: 0, cqe_eventfd: efd, - cqe_scratch: Vec::with_capacity(256), - event_scratch: Vec::with_capacity(256), + cqe_scratch: Vec::with_capacity(cqe_scratch_cap), + event_scratch: Vec::with_capacity(event_scratch_cap), })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/io/uring_driver.rs` around lines 302 - 303, The scratch buffers are hard-coded to 256 which will reallocate when ring_size is larger; change the initial capacities to be derived from the configured ring_size: set cqe_scratch capacity to ring_size * 2 (to match CQ depth) and size event_scratch with extra headroom (e.g., ring_size * 3 or ring_size * 2 + ring_size/2) so it can accommodate CQEs that emit up to two IoEvent entries; update the Vec::with_capacity calls that initialize cqe_scratch and event_scratch to use these ring_size-based capacities.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/io/uring_driver.rs`:
- Around line 618-620: The drain_completions() helper currently allocates a
fresh Vec via Vec::new() which breaks scratch-buffer reuse; change it to reuse
the existing event_scratch buffer (or at minimum seed capacity from it) and then
call drain_completions_into(&mut events). Specifically, in the drain_completions
method replace the new Vec creation with either taking/clearing
self.event_scratch (e.g., swap or drain into a local Vec) or creating events
with Vec::with_capacity(self.event_scratch.capacity()) so the call to
drain_completions_into(&mut events) reuses retained capacity and avoids
allocating on each call; reference drain_completions, drain_completions_into,
and the event_scratch field to locate the code.
---
Nitpick comments:
In `@src/io/uring_driver.rs`:
- Around line 302-303: The scratch buffers are hard-coded to 256 which will
reallocate when ring_size is larger; change the initial capacities to be derived
from the configured ring_size: set cqe_scratch capacity to ring_size * 2 (to
match CQ depth) and size event_scratch with extra headroom (e.g., ring_size * 3
or ring_size * 2 + ring_size/2) so it can accommodate CQEs that emit up to two
IoEvent entries; update the Vec::with_capacity calls that initialize cqe_scratch
and event_scratch to use these ring_size-based capacities.
In `@src/protocol/serialize.rs`:
- Around line 702-714: Add a direct wire-format unit test for NaN by creating a
test (e.g., test_serialize_double_nan_wire_format) that calls
serialize_frame(&Frame::Double(f64::NAN)) and asserts the returned bytes equal
b"$3\r\nnan\r\n", and also calls serialize_resp3_frame(&Frame::Double(f64::NAN))
and asserts the returned bytes equal b",nan\r\n"; reference the existing helpers
serialize_frame, serialize_resp3_frame and the Frame::Double variant so the test
mirrors the other double serialization tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee4dd505-b4ef-41a4-8f9d-465adc6cdf00
📒 Files selected for processing (12)
src/io/uring_driver.rssrc/persistence/checkpoint.rssrc/persistence/wal.rssrc/persistence/wal_v3/record.rssrc/protocol/serialize.rssrc/shard/event_loop.rssrc/shard/spsc_handler.rssrc/storage/db.rssrc/storage/eviction.rssrc/vector/hnsw/search.rssrc/vector/segment/holder.rssrc/vector/segment/mutable.rs
Comment on lines
+618
to
+620
| pub fn drain_completions(&mut self) -> Vec<IoEvent> { | ||
| let mut events = Vec::new(); | ||
| self.drain_completions_into(&mut events); |
There was a problem hiding this comment.
drain_completions() reintroduces a fresh Vec allocation.
This helper now allocates a new zero-capacity buffer on every call, which bypasses the scratch-buffer reuse added elsewhere and violates the repo rule for src/io/**. Reuse event_scratch here too, or at least seed from its retained capacity instead of Vec::new(). As per coding guidelines, src/io/**/*.rs must not use Vec::new() in I/O-path code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/io/uring_driver.rs` around lines 618 - 620, The drain_completions()
helper currently allocates a fresh Vec via Vec::new() which breaks
scratch-buffer reuse; change it to reuse the existing event_scratch buffer (or
at minimum seed capacity from it) and then call drain_completions_into(&mut
events). Specifically, in the drain_completions method replace the new Vec
creation with either taking/clearing self.event_scratch (e.g., swap or drain
into a local Vec) or creating events with
Vec::with_capacity(self.event_scratch.capacity()) so the call to
drain_completions_into(&mut events) reuses retained capacity and avoids
allocating on each call; reference drain_completions, drain_completions_into,
and the event_scratch field to locate the code.
Source: Coding guidelines
Lint gate requires a CHANGELOG entry; documents the 11 implemented findings and single/multi-shard benchmark outcomes. author: Tin Dang
5 tasks
This was referenced Jul 4, 2026
Merged
Merged
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
2 participants
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.
Summary
Implements the 11 verified findings from a 5-area deep performance review (hot path, data store, persistence, vector, graph). 60 findings were collected by parallel specialist reviews; the top 15 were adversarially verified against the code (11 confirmed, 3 downgraded, 1 refuted) and the confirmed quick wins implemented. Full report:
tmp/perf-review/REPORT.md(local, untracked).Benchmark (moon-dev VM, aarch64 Linux, 1 shard, monoio, 200k req, r=100k)
Changes
Database::get3→2 DashTable probes on hit, 2→1 on miss (single-probe classify); allocation-freeEvictionPolicy::from_strto_vec()→Cow::Borrowed;CheckpointStateclone-per-tick →Copydrain_spsc_sharedper-call Vec pair → thread-local scratch (re-entrancy-safemem::take)Frame::Doubleformat!→ stackF64Display(byte-identical Display output, RESP2+RESP3); new edge-case tests pinf64::MAX/5e-324worst-case lengthsSmallVec, L2 borrows raw query); IVFq_rotated/lut_bufhoisted out of the segment loop insearch_filtered(mirrorssearch_mvcc)drain_completionsper-batch CQE + event Vecs → persistent driver scratch loaned to the event loopValidation
cargo fmt --check,cargo clippy -D warnings(default + tokio,jemalloc feature sets) green on Linux VMcargo checkgreen on Linux (covers#[cfg(target_os = "linux")]io_uring changes)Deferred (follow-up candidates from the review)
Single-pass RESP parse (HP-F1), Cypher columnar rows (GR-F1, est. 40-70% multi-hop win), ParallelBfs flat neighbor buffer (GR-F2), NEON SQ8 ADC kernel (VE-F6, est. 3-4x kernel),
key_hash_to_keyclone-per-FT.SEARCH (VE-F7), monoio snapshot finalize blocking the event loop (PS-F10).Summary by CodeRabbit