perf(server): appendfsync-always local writes join per-batch group commit#239
Conversation
…mmit Under `appendfsync always`, all three connection handlers awaited one fsync ack PER COMMAND for plain local writes (try_send_append_durable). A 16-deep pipeline therefore paid 16 serialized fsync round-trips per connection, while Redis fsyncs once per event-loop iteration — measured as an 8x SET deficit at P16 (Moon 5.2k vs Redis 44k ops/s, GCE c3-standard-8; tmp/MOON-VS-REDIS-DURABILITY.md). The writer-side group commit (one fsync per drained batch) already existed, and cross-shard writes + coordinator local legs already used the fire-and-forget + barrier contract from PR #213. This change extends that contract to plain local writes: - handler_monoio / handler_sharded: local writes, MOVE, and COPY enqueue via send_append_group(); successful responses join local_leg_write_idxs and the existing end-of-batch resolve_local_leg_barrier() (one fsync_barrier per pipelined batch) converts them to AOF_FSYNC_ERR on failure — never a silent +OK. - handler_single: flush_with_aof_ack collects barrier indexes and issues ONE fsync_barrier(0) for the whole flush; the inline pre-SUBSCRIBE path and single-shard GRAPH.* WAL-record loop use the same pattern. - The AOF writer processes its channel in order, so an acked zero-length AppendSync barrier proves every prior Append durable: the H1 fsync-before-ack guarantee is unchanged. - everysec/no policies unaffected (send_append_group returns Ok(false), no barrier joined). Validation: - crash_matrix_per_shard_aof --ignored: 3/3 green (SIGKILL under appendfsync always still recovers 100% of acked writes). - flush_with_aof_ack H1 ordering test updated to the batch protocol (Append then AppendSync barrier; ack still gates the response). - fmt, clippy x2 (default + tokio,jemalloc), cargo test --lib x2 (monoio 3864 / tokio 3143), cargo test --no-run x2: all green. 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? |
📝 WalkthroughWalkthroughThis PR changes AOF durability behavior under ChangesGroup-commit AOF durability refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (2)
src/server/conn/handler_single.rs (1)
75-75: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid allocating
barrier_idxson the AOF flush path.
flush_with_aof_ackruns on the write response path, andVec::new()adds a per-flush hot-path allocation. Use a stack-backedSmallVecor reusable scratch storage.♻️ Proposed fix
- let mut barrier_idxs: Vec<usize> = Vec::new(); + let mut barrier_idxs: smallvec::SmallVec<[usize; 64]> = smallvec::SmallVec::new();As per coding guidelines,
src/**/*.rshot paths should avoidVec::new()and prefer preallocated buffers orSmallVec.🤖 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/conn/handler_single.rs` at line 75, The hot path in flush_with_aof_ack is allocating barrier_idxs with Vec::new(), which should be avoided on the AOF flush path. Replace the Vec-based scratch storage with a stack-backed SmallVec or another reusable preallocated buffer, and keep the change localized around the barrier_idxs usage in handler_single::flush_with_aof_ack so the write response path does not incur per-flush heap allocations.Source: Coding guidelines
src/server/conn/handler_sharded/mod.rs (1)
1224-1226: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove
aof_bytesinstead of cloning it in MOVE/COPY.These branches
continueafter handling AOF, soaof_bytescan be consumed and passed directly tosend_append_group.♻️ Proposed fix
- if let Some(ref bytes) = aof_bytes { + if let Some(bytes) = aof_bytes { if let Some(ref pool) = ctx.aof_pool { let lsn = aof::AofWriterPool::issue_append_lsn(&ctx.repl_state, ctx.shard_id, bytes.len()); match pool - .send_append_group(ctx.shard_id, lsn, bytes.clone()) + .send_append_group(ctx.shard_id, lsn, bytes) .awaitApply the same pattern in the
COPYbranch.As per coding guidelines,
src/**/*.rshot paths should avoidclone()and prefer borrowing or moving.Also applies to: 1282-1284
🤖 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/conn/handler_sharded/mod.rs` around lines 1224 - 1226, The MOVE/COPY AOF path in `handler_sharded::mod` is cloning `aof_bytes` before `send_append_group`, even though those branches terminate with `continue` and can move the buffer instead. Update the MOVE branch to pass `aof_bytes` directly into `send_append_group`, and apply the same change in the COPY branch so the buffer is consumed rather than cloned. Use the existing `send_append_group` call sites in this match block as the target for the fix.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.
Nitpick comments:
In `@src/server/conn/handler_sharded/mod.rs`:
- Around line 1224-1226: The MOVE/COPY AOF path in `handler_sharded::mod` is
cloning `aof_bytes` before `send_append_group`, even though those branches
terminate with `continue` and can move the buffer instead. Update the MOVE
branch to pass `aof_bytes` directly into `send_append_group`, and apply the same
change in the COPY branch so the buffer is consumed rather than cloned. Use the
existing `send_append_group` call sites in this match block as the target for
the fix.
In `@src/server/conn/handler_single.rs`:
- Line 75: The hot path in flush_with_aof_ack is allocating barrier_idxs with
Vec::new(), which should be avoided on the AOF flush path. Replace the Vec-based
scratch storage with a stack-backed SmallVec or another reusable preallocated
buffer, and keep the change localized around the barrier_idxs usage in
handler_single::flush_with_aof_ack so the write response path does not incur
per-flush heap allocations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 803afc9a-6bd3-450a-b1e5-bbde78d2768a
📒 Files selected for processing (4)
CHANGELOG.mdsrc/server/conn/handler_monoio/mod.rssrc/server/conn/handler_sharded/mod.rssrc/server/conn/handler_single.rs
GCE A/B results (c3-standard-8, pd-ssd, us-central1-a, Redis 7.0.15, 3 alternated reps, fresh server+dir per scenario)
The 8× always-P16 deficit (0.12×) is closed to 0.85× of Redis. p1 is fsync-device-bound for all engines (~3.4k = disk fsync rate, parity). Remaining 15% gap at p16 is the next hardening target. Durability re-verified post-change: |
2 participants
Summary
Closes the biggest Moon-vs-Redis durability gap: under
appendfsync alwayswith pipelining, Moon was 8× slower than Redis (P16 SET: 5.2k vs 44k ops/s, GCE c3-standard-8 — full matrix intmp/MOON-VS-REDIS-DURABILITY.md).Root cause: all three connection handlers awaited one fsync ack per command (
try_send_append_durable). A 16-deep pipeline paid 16 serialized fsync round-trips per connection; Redis fsyncs once per event-loop iteration. The writer-side group commit (one fsync per drained batch,src/persistence/aof/group_commit.rs) already existed but could only batch across connections — the per-command await defeated it within a connection.Change
Extend the fire-and-forget + barrier contract (already used by cross-shard writes and coordinator local legs since PR #213) to plain local writes:
send_append_group(); successful responses joinlocal_leg_write_idxs, and the existing end-of-batchresolve_local_leg_barrier()issues ONEfsync_barrierper pipelined batch, converting joined responses toAOF_FSYNC_ERRon failure — never a silent+OK.flush_with_aof_ackcollects barrier indexes and issues onefsync_barrier(0)per flush; inline pre-SUBSCRIBE and single-shard GRAPH.* WAL-record paths use the same pattern.AppendSyncbarrier proves every priorAppenddurable — the fsync-before-ack (H1) guarantee is unchanged.everysec/nopolicies unaffected (send_append_groupreturnsOk(false), no barrier joined).Test plan
crash_matrix_per_shard_aof --ignored: 3/3 green — SIGKILL underappendfsync alwaysstill recovers 100% of acked writes (the durability contract this change must not break).flush_with_aof_ackH1 ordering test updated to the batch protocol (oneAppend+ oneAppendSyncbarrier; ack still gates the first response).cargo fmt --check; clippy ×2 (default + tokio,jemalloc);cargo test --lib×2 (monoio 3864 / tokio 3143);cargo test --no-run×2.Summary by CodeRabbit