perf(persistence): coalesce each AOF group-commit batch into one write#242
Conversation
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? |
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis PR coalesces AOF group-commit writes into a single batch buffer and single write per batch, changes monoio writer receive handling for everysec/no policies to poll without parking, updates related tests and changelog text, and splits accept backoff resource-exhaustion checks by platform. ChangesAOF Writer Batching and Polling
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Producer
participant WriterLoop
participant commit_group_commit_batch as commit_group_commit_batch
participant Sink
Producer->>WriterLoop: send message
WriterLoop->>WriterLoop: recv_next selects poll_recv or recv_timeout
WriterLoop->>commit_group_commit_batch: drained batch
commit_group_commit_batch->>commit_group_commit_batch: coalesce records
commit_group_commit_batch->>Sink: write_all(one buffer)
Sink-->>commit_group_commit_batch: result
commit_group_commit_batch->>Sink: sync() if required
commit_group_commit_batch->>WriterLoop: ack_batch
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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/persistence/aof/writer_task.rs (2)
1524-1543: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider reserving
batch_bufcapacity before filling.
batch_buf.clear()is followed by per-messageextend_from_slicecalls without a pre-computedreserve, unlike the TopLevel coalescing ingroup_commit.rswhich sizes its buffer upfront viaVec::with_capacity(total). Oncebatch_bufis reset above the 1MB high-water mark (line 1557), the next large batch can incur several reallocations while growing from empty. Precomputing the total framed size and callingbatch_buf.reserve(total)before the loop would avoid this.🤖 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/persistence/aof/writer_task.rs` around lines 1524 - 1543, The batch framing in writer_task’s per-message loop builds `batch_buf` from empty after `clear()` without reserving space first, which can trigger repeated reallocations on large batches. In the `writer_task` logic where `batch_buf` is filled from `batch.data`, precompute the total framed size for the messages that will be written and call `batch_buf.reserve(total)` before the loop, following the same upfront sizing approach used by `group_commit.rs`’s coalescing path. Ensure the reservation accounts for each retained `AofMessage::Append`/`AppendSync` payload plus the fixed header size, while still skipping zero-length payloads.
1-1729: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffFile size approaching/exceeding the 1500-line guideline.
This PR adds new helpers (
poll_recv,recv_next,batch_bufcoalescing, plus unit tests) to an already very largewriter_task.rs. Per the repo's file-size guideline, consider splitting the monoio/tokio writer-loop implementations (or the new poll helpers) into a submodule.As per coding guidelines,
src/**/*.rs: "No single.rsfile should exceed 1500 lines; split larger modules into submodules, and split single Redis command groups into read/write files once they exceed 1000 lines."#!/bin/bash # Description: Confirm the current line count of writer_task.rs fd writer_task.rs --exec wc -l {}🤖 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/persistence/aof/writer_task.rs` around lines 1 - 1729, The file has grown beyond the repository’s size guideline and should be split into submodules. Move the new helper logic and/or the large writer-loop implementations from aof_writer_task and per_shard_aof_writer_task into separate module files, keeping only the public task entrypoints and shared types in writer_task.rs. Preserve the existing behavior by factoring out poll_recv, recv_next, IdleWait, and the batch coalescing path (including batch_buf handling) into a dedicated submodule, and keep the unit tests alongside the extracted helpers.Source: Coding guidelines
🤖 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/persistence/aof/group_commit.rs`:
- Around line 240-256: The multi-message branch in commit_group_commit_batch is
allocating a fresh Vec for every batch on the AOF write hot path. Update this
path to reuse a caller-owned scratch buffer, similar to the batch_buf pattern
already used in writer_task.rs for the PerShard writer, and thread that buffer
through the TopLevel writer loop so the buffer is cleared and reused instead of
recreated for each commit_group_commit_batch call.
---
Nitpick comments:
In `@src/persistence/aof/writer_task.rs`:
- Around line 1524-1543: The batch framing in writer_task’s per-message loop
builds `batch_buf` from empty after `clear()` without reserving space first,
which can trigger repeated reallocations on large batches. In the `writer_task`
logic where `batch_buf` is filled from `batch.data`, precompute the total framed
size for the messages that will be written and call `batch_buf.reserve(total)`
before the loop, following the same upfront sizing approach used by
`group_commit.rs`’s coalescing path. Ensure the reservation accounts for each
retained `AofMessage::Append`/`AppendSync` payload plus the fixed header size,
while still skipping zero-length payloads.
- Around line 1-1729: The file has grown beyond the repository’s size guideline
and should be split into submodules. Move the new helper logic and/or the large
writer-loop implementations from aof_writer_task and per_shard_aof_writer_task
into separate module files, keeping only the public task entrypoints and shared
types in writer_task.rs. Preserve the existing behavior by factoring out
poll_recv, recv_next, IdleWait, and the batch coalescing path (including
batch_buf handling) into a dedicated submodule, and keep the unit tests
alongside the extracted helpers.
🪄 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: 06de267b-ce54-484b-97a6-0f356f9e4625
📒 Files selected for processing (4)
CHANGELOG.mdsrc/persistence/aof/group_commit.rssrc/persistence/aof/writer_task.rstests/wal_group_commit.rs
| match batch.data.len() { | ||
| 0 => {} | ||
| 1 => { | ||
| if sink.write_all(msg_body(&batch.data[0])).is_err() { | ||
| return ack_batch(batch, BatchAck::WriteFailed); | ||
| } | ||
| } | ||
| _ => { | ||
| let total: usize = batch.data.iter().map(|m| msg_body(m).len()).sum(); | ||
| let mut buf = Vec::with_capacity(total); | ||
| for msg in &batch.data { | ||
| buf.extend_from_slice(msg_body(msg)); | ||
| } | ||
| if sink.write_all(&buf).is_err() { | ||
| return ack_batch(batch, BatchAck::WriteFailed); | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Per-batch heap allocation on the AOF write hot path.
The multi-message branch allocates a fresh Vec::with_capacity(total) on every commit_group_commit_batch call, with no reuse across batches. This is the same I/O-driver hot path that the PerShard writer in writer_task.rs was specifically reworked to use a reusable, capped batch_buf for (see writer_task.rs lines 1440-1442, 1516-1559) — this coalescing path lacks the equivalent optimization, allocating and dropping a new buffer per batch instead of reusing one across the writer's lifetime.
Consider threading a caller-owned scratch buffer into commit_group_commit_batch (mirroring the batch_buf pattern already established for PerShard), so the TopLevel writer loop in writer_task.rs also amortizes this allocation the same way.
As per coding guidelines, src/**/*.rs: "Avoid hot-path allocations in command dispatch, protocol parsing, shard event loops, and I/O drivers: no Box::new(), Vec::new(), String::new(), Arc::new(), clone(), format!(), or to_string() in those paths; prefer preallocated buffers, SmallVec, itoa, write!, or borrowing."
♻️ Sketch of a reusable-buffer signature change
pub fn commit_group_commit_batch<S: GroupCommitSink + ?Sized>(
sink: &mut S,
batch: &mut GroupCommitBatch,
do_fsync: bool,
+ scratch: &mut Vec<u8>,
) -> CommitOutcome {
match batch.data.len() {
0 => {}
1 => {
if sink.write_all(msg_body(&batch.data[0])).is_err() {
return ack_batch(batch, BatchAck::WriteFailed);
}
}
_ => {
- let total: usize = batch.data.iter().map(|m| msg_body(m).len()).sum();
- let mut buf = Vec::with_capacity(total);
+ scratch.clear();
for msg in &batch.data {
- buf.extend_from_slice(msg_body(msg));
+ scratch.extend_from_slice(msg_body(msg));
}
- if sink.write_all(&buf).is_err() {
+ if sink.write_all(scratch).is_err() {
return ack_batch(batch, BatchAck::WriteFailed);
}
+ if scratch.capacity() > 1 << 20 {
+ *scratch = Vec::new();
+ }
}
}
...🤖 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/persistence/aof/group_commit.rs` around lines 240 - 256, The
multi-message branch in commit_group_commit_batch is allocating a fresh Vec for
every batch on the AOF write hot path. Update this path to reuse a caller-owned
scratch buffer, similar to the batch_buf pattern already used in writer_task.rs
for the PerShard writer, and thread that buffer through the TopLevel writer loop
so the buffer is cleared and reused instead of recreated for each
commit_group_commit_batch call.
Source: Coding guidelines
The monoio AOF writer was write-syscall-bound under appendfsync always with pipelining. strace -c on aof-writer-0 during an 8s always-P16 SET window (GCE c3-standard-8, 2 shards): write: 127,812 calls, 1.25s (~1 syscall per record, 9us each) fdatasync: 2,144 calls, 0.20s The writer spent 6x more time issuing write(2) than fsyncing: both monoio paths wrote each record individually on the raw unbuffered File -- the TopLevel path one write per message via commit_group_commit_batch, the PerShard framed path a header+body write PAIR per record. Redis batches ~120 records into one write via aof_buf (its main thread did 32,791 writes total for MORE throughput), which is where the remaining 0.85x always-P16 gap lived: each ~60-record batch paid ~540us of serialized write syscalls before its fsync could start. Fix, both monoio paths: - commit_group_commit_batch (TopLevel): multi-message batches coalesce into one contiguous buffer and ONE sink.write_all; single-message batches keep the zero-copy direct write. - PerShard framed loop: records are framed into a reusable batch_buf ([u64 lsn][u32 len] header + body, channel order) and written with ONE write_all; buffer high-water capped at 1MB. - tokio writer loops already amortize via BufWriter -- untouched. Failure semantics unchanged: a failed batch write acks every AppendSync waiter WriteFailed (none Synced) and engages the torn-stream latch; the single per-batch fsync still runs strictly after all bytes are written, so fsync-before-ack (H1) is preserved. The everysec path shares the same write code and gets the same syscall reduction. wal_group_commit sink tests updated to pin the coalesced contract: one write per batch, bytes concatenated in channel order, fsync after it, whole-batch write failure (no torn half-batch acks). Validation: - wal_group_commit 9/9 green (contract tests updated). - crash_matrix_per_shard_aof --ignored 3/3 green (SIGKILL durability contract intact, including appendfsync always). - fmt, clippy x2, cargo test --lib x2 (monoio 3868 / tokio 3143), unwrap ratchet green. - GCE always-P16 A/B vs Redis to be posted on the PR before merge. author: Tin Dang
is_resource_exhaustion (accept-loop backoff, PR #230) matched libc::EMFILE/ENFILE/ENOBUFS/ENOMEM without a cfg guard; libc is not linked on Windows targets, so every main-push Windows check has failed E0433 since the merge (PR CI never caught it -- the Windows job is skipped on PRs). Split the function: unix keeps the errno match, Windows matches WSAEMFILE (10024) / WSAENOBUFS (10055) and ErrorKind::OutOfMemory. author: Tin Dang
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/server/accept_backoff.rs (1)
102-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider named constants for the winsock error codes.
10024/10055as bare literals require readers to cross-reference the doc comment to know they'reWSAEMFILE/WSAENOBUFS. Named constants would self-document at the call site.♻️ Optional refactor
+const WSAEMFILE: i32 = 10024; +const WSAENOBUFS: i32 = 10055; + #[cfg(not(unix))] fn is_resource_exhaustion(err: &std::io::Error) -> bool { - matches!(err.raw_os_error(), Some(10024) | Some(10055)) + matches!(err.raw_os_error(), Some(WSAEMFILE) | Some(WSAENOBUFS)) || matches!(err.kind(), std::io::ErrorKind::OutOfMemory) }🤖 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/server/accept_backoff.rs` around lines 102 - 105, The resource exhaustion check in is_resource_exhaustion uses bare Winsock error literals, so replace 10024 and 10055 with named constants that clearly identify WSAEMFILE and WSAENOBUFS. Update the match in is_resource_exhaustion to use those constants so the intent is self-documenting at the call site and easier to maintain.
🤖 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/server/accept_backoff.rs`:
- Around line 90-106: The test around is_resource_exhaustion only covers the
unix errno branch, so it will not match the Windows/non-unix behavior in the
existing conditional implementations. Update the
resource_exhaustion_classification test in accept_backoff to be target-gated
with #[cfg(unix)] for the libc::EMFILE/ENFILE path and add a separate non-unix
case that exercises the Windows-specific raw_os_error values or
ErrorKind::OutOfMemory branch.
---
Nitpick comments:
In `@src/server/accept_backoff.rs`:
- Around line 102-105: The resource exhaustion check in is_resource_exhaustion
uses bare Winsock error literals, so replace 10024 and 10055 with named
constants that clearly identify WSAEMFILE and WSAENOBUFS. Update the match in
is_resource_exhaustion to use those constants so the intent is self-documenting
at the call site and easier to maintain.
🪄 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: 2bd13c0e-c8e0-4db2-ba7a-aab3b51075f5
📒 Files selected for processing (2)
CHANGELOG.mdsrc/server/accept_backoff.rs
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
647890a to
7c1b911
Compare
Follow-up to the accept_backoff Windows fix in PR #242: the module's resource_exhaustion_classification unit test also referenced libc::EMFILE/ENFILE/ECONNABORTED, so the main-push Windows check kept failing E0433 on the test build. cfg-split the test the same way as the function under test: unix keeps the libc errnos, Windows uses WSAEMFILE (10024) / WSAENOBUFS (10055) / WSAECONNABORTED (10053). The Windows job runs only on main pushes (skipped on PRs), which is why neither the original break (PR #230) nor the partial fix could be validated pre-merge. author: Tin Dang Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
-#242) (#244) Add BENCHMARK.md section 7.3 documenting the 2026-07-08 durability write-path campaign and update the tuning docs with the measured ratios. BENCHMARK.md: - section 7.3: full vs-Redis matrix (GCE c3-standard-8, Redis 7.0.15, --shards 2, 3 alternated reps, provenance-probed) -- appendfsync always P16 SET 0.12x -> 0.91x, everysec P16 SET 1.32x WIN, everysec P1 SET 0.80x -> 0.99x parity, pub/sub fan-out delivery 438 msg/s -> 5.09M msg/s (0 drops, 1.04x Redis). - per-PR root-cause table with the strace diagnostics that drove each fix (149,718 -> 96 futex calls for the park-free poll; 127,812 write(2) per 8s -> one write per batch for the coalesced write). - durability-invariant note (fsync-before-ack preserved; crash-matrix 3/3) and same-instance A/B reproduction steps (section 15). - executive summary + header updated. docs/production-guide.md, docs/benchmarks.md, docs/configuration.md: - appendfsync tuning guidance updated with the measured ratios and a plain-language explanation of group commit, coalesced batch writes, and the park-free writer poll -- all automatic, durability unchanged. Docs-only; no code paths touched. author: Tin Dang Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
…237) Wave-2 graph engine (W2-1..W2-14): frozen-tier copy-up writes (SET/DELETE/ MERGE on frozen rows), IndexScan range predicates, row-BFS multi-segment fast path, write-side plan cache, Cypher aggregations (count/sum/avg/min/ max/collect), OPTIONAL MATCH null-padding + WITH mid-pipeline rebinding, configurable traversal timeout, and a kill-9 crash suite for Cypher SET/SET-label WAL durability. Stable node/edge ids across WAL replay. P0 v3 graph-durability fix: disk-offload graphs replay through a dedicated WAL path (GCP-soak-found, re-verified 45,464/45,464 recovery). Cypher-2x wave (profiling showed 82.5% of shard CPU in the mutable-tier index_scan_keys linear scan): P1 mutable-tier property index (point queries 25.5k -> 29.0k qps, p50 -27%, shard CPU -70%); P2 write-gen- invalidated result cache with TinyLFU doorkeeper admission (cache-hostile cycling -21% -> -2.4%); P3 CONTAINS/STARTS WITH/ENDS WITH text predicates via FTS SegmentTextIndex reuse. Vector time-to-index-green: parallel HNSW build (shared-graph concurrent inserts, per-node parking_lot locks, connectivity-repair pass) + insert- path compaction trigger + a Linux affinity-mask fix (core-pinned shard threads made available_parallelism() return 1, silently serializing the "parallel" build and sizing the compactor pool to 1 worker). FT.COMPACT 30.2s -> 2.71s; GCE load-to-HNSW-serving now beats Qdrant 1.6x (x86) / 2.3x (ARM). Multi-shard verified. Benchmarks (BENCHMARK.md): GCE dedicated-core validation vs FalkorDB (Cypher 1-hop 2.3-2.7x, dedicated-core p99 beats FalkorDB) and vs Qdrant (ingest 10x, matched-recall search 2.7-3.4x, time-to-green now a win on both arches). Full CI-parity gate green after merging main (fmt, clippy default+tokio, tests default+tokio); merge resolved conflicts from the WAL v2 removal (#236) and AOF/pub-sub perf waves (#238-#242). author: Tin Dang
2 participants
Summary
The monoio AOF writer was write-syscall-bound, not fsync-bound, under
appendfsync always+ pipelining.strace -conaof-writer-0during an 8s always-P16 SET window:Both monoio paths wrote each record individually on the raw unbuffered
File(the PerShard framed path used a header+body write pair per record). Redis batches ~120 records per write viaaof_buf(32,791 writes total for more throughput). Each ~60-record batch paid ~540µs of serialized write syscalls before its fsync could start.Change
commit_group_commit_batch(TopLevel): multi-message batches coalesce into one contiguous buffer + ONEsink.write_all; single-message batches keep the zero-copy direct write.batch_buf(channel order preserved) + ONEwrite_all; buffer high-water capped at 1MB.BufWriter— untouched.WriteFailed(whole-batch, no torn half-batch acks) and engages the torn-stream latch; the single per-batch fsync still runs strictly after all bytes — fsync-before-ack (H1) preserved.GCE A/B (c3-standard-8, Redis 7.0.15, 3 alternated reps,
-c 8 -r 100000 -d 64, Moon--shards 2)Test plan
wal_group_commitcontract tests updated to the coalesced contract (one write per batch, channel-order bytes, fsync after it, whole-batch write failure) — 9/9 greencrash_matrix_per_shard_aof --ignored3/3 green (SIGKILL durability incl.always)cargo test --lib×2 (monoio 3868 / tokio 3143); unwrap ratchetSummary by CodeRabbit
Performance
EverySec/Nofsync modes via bounded polling to keep producer delivery mostly in user space.Behavior
Alwaysfsync mode preserves existing blocking/park behavior.Bug Fixes
Tests