perf(server): spanning multi-key reads join the slotted batch (#513 A2a) - #768
Merged
Conversation
A2a) Since #512 closed the #507 write-loss inversion, a multi-key command in the middle of a pipeline forced the batch tail to defer across a dispatch boundary. #721 (A1) took the case where a single shard owns every key. This takes the genuinely SPANNING case, for the two per-key decomposable READS: MGET and EXISTS. The command is split per owner shard and each part is routed exactly like an ordinary single-shard command at the command's own position in the batch -- appended to remote_groups[owner], or run inline against the local slice -- and the parts are folded back into one client reply at the drain. That removes the coordinator's second, immediate SPSC push, which was the #507 inversion, rather than merely declining to wait for it. Ordering is per-key: a key has exactly one owner, every operation on it in the batch lands in that owner's vector in loop order, and one thread executes that vector in order. must_wait_for_pending_remote and the routing side now consult ONE function, multikey_placement, so the guard cannot say "safe" about a command that routing still runs inline. Everything uncertain resolves to Coordinator and keeps waiting: workspace connections, an untrustworthy key mask, anything inside a cross-shard TXN, MSETNX/BITOP/COPY, and MSET/DEL/UNLINK -- those last are decomposable, but splitting a WRITE adds per-part AOF, replication, group-commit and tracking-invalidation work, and are held back to A2b so a durability bug and a throughput change cannot bisect to one commit. The fan-out sits strictly BELOW the #500 txn_multikey_write refusal in both handlers. One-shard behaviour is bit-for-bit unchanged: at --shards 1 every key mask has one bit, so multikey_placement answers Slotted and the branch is unreachable, which respects the handler_sharded/handler_monoio num_shards<=1 asymmetry without a shard-count check of its own. handler_single.rs and src/shard/ are untouched. A part that fails for any reason -- backpressure give-up, reply timeout, the shard's own error, or a slot nothing ever wrote -- fails the whole reply rather than assembling a partial answer. INFO stats gains total_pipeline_multikey_fanout (moon_pipeline_multikey_fanout_total), one increment per fanned-out command. It is load-bearing, not curiosity: the deferral counter alone cannot separate a real fix from a dangerous one, because a change that merely stopped waiting while the command still ran inline would drive deferrals to zero too, and that is #507 reopened. Mutation-tested -- that mutation passes the deferral assertion and is caught only by this counter. Tests: pco16 asserts 0 deferrals AND 32 fan-outs across all six shard pairs at --shards 4, plus an order-sensitive interleaved-key value leg and an EXISTS leg. pco12/pco13/pco15 controls moved off spanning MGET -- the shape this change deliberately stops deferring -- onto spanning MSET or keyless DBSIZE; pco12's subject assertion flips from >0 to ==0 as its own comment instructed. 10 unit tests cover the split/fold machinery directly. Refs: #513, #507, #512, #721, #500, #592 author: Tin Dang
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
|
Warning Review limit reachedNext included review available in 18 minutes. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (12)
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 |
Collaborator
Author
The missing Linux spanning numberThis PR names its own gap:
Ran it. Numbers below. Rig and method
Results
Medians of n=36 (bulk) and n=21 (round-trip) interleaved reps. Bulk IQR: control 23,819..24,685, fix 379,514..390,887. Deferrals per leg, read from the server's own counter:
Pre-flight: the control deferred 128 of 128 spanning groups — every one. The harness was demonstrably exercising the path this PR removes, on the control side, before either leg was timed. Harness
plus a fourth I added for this shape: the hash-tag pairs are verified live against Script is on the bench box at Two caveats, stated plainlyThe +51.1% is not "the −52.4% is fixed." I did not run the measurement that produced −52.4%, so I can't put the two percentages on the same axis and claim a remainder. What I measured is the delta above, on a rig I verified. If you want the regression declared closed, the honest way is to re-run the original #513 harness on both sides. The win tracks parks, not SPSC pushes. The commit message credits removing cross-shard sends. My own pipeline sweep on this hardware puts per-message cost at roughly zero and per-park at 2.49 CPU%/kops — and every deferral here is a batch boundary, hence a park. 7,938 → 0 parks is what moved the number. Same fix, same magnitude; I'd just describe the mechanism differently, and it matters for predicting where else this pattern pays. Conflict worth flagging before this mergesThis PR rewrites the regions the L4 shared-read-plane work touches — Evidence only — not a review verdict, not an approval. |
TinDang97
added a commit
that referenced
this pull request
Sep 1, 2026
* docs(benchmark): first Moon-vs-Redis matrix since v0.6.0, and what it corrects BENCHMARK.md backed v0.6.0 and its headline KV rows dated from 2026-04-15 on v0.1.6. Eight releases and 112 src/ commits later nothing had re-measured it, so §1 and the README were quoting ratios no one had reproduced in five months. §2.12 re-measures v0.8.7 (d63ffcd) on both GCE arches using §2.11's interleaved method — legs alternate every rep, Redis 7.0.15 is restarted and re-measured every rep as a live drift control, request counts scale with depth, provenance written into the CSV. 266 rows per arch, 0 failures, floors 0.2-3.6%. The peak numbers hold and are in fact better than published (GET 2.40x x86 / 2.29x ARM vs the recorded 1.72x/2.20x). The FRAMING does not: GET and plain SET are the only two commands on the inline byte path, and every other family runs 0.40-0.67x Redis from p=8 up, identically on both architectures. That is not inferred — `SET k v` runs 2.08x Redis and `SET k v EX 100`, the same work with one option that disqualifies it from the fast path, runs 0.87x. §2.13 dates the deficit. v0.6.0 rebuilt on the same host already ran 0.51-0.76x at p>=8, so the report never regressed — it simply never computed those ratios, including in §2.11, which ran this exact grid three weeks ago and reduced it only to Moon-vs-Moon deltas. But a further 9-21% write-path loss did land: ten of twelve rows regressed against floors of 0.4-1.6%, and GET, the only read in the grid, lost nothing. A 9-point rebuild sweep shows three steps rather than one cliff, which is why this was not bisected: bisect assumes a single step change and would have returned the 08-19 drop as the whole cause, missing two thirds of the loss. Four mechanisms were proposed and tested to destruction rather than shipped as plausible-sounding prose — the intercept chain shrank (28->26 gates), the cmd_len==4 guard theory reverses under an INCR-vs-INCRBY probe, the with_shard_db fallback is never taken, and ShardSlice did not grow. What the profile can support is stated; what fat LTO makes unattributable is stated too. README gains the same scope caveat, and the p=1 busy-poll claim now records that 1.65-1.66x needs dedicated cores. Raw data in tmp/: kvab-{x86,arm}.csv, versab-x86.csv, sweep-x86.csv, probe-arm.csv, lenprobe.csv. author: Tin Dang * perf(dispatch): ordinary keyspace commands skip 21 intercept gates BENCHMARK.md 2.12 measured every non-inlined command family at 0.40-0.67x Redis from p=8 upward while GET and plain SET win 1.78-2.40x. Profiling the gap showed +5.5 points of per-command cost sitting in the dispatch/intercept region relative to v0.6.0. Reading the handler explains part of it. Every command walks 26 intercept gates before reaching dispatch. Only six carry a `cmd_len` pre-guard -- added deliberately, per the comment at the call site, because the bodies are "too large for rustc to inline" -- and twelve are `async fn`s, so an INCR builds and polls twelve futures purely to be told "not mine" twelve times. That optimisation was applied to six of twenty-six and never finished. `CommandFlags` is a u16 with bits 0..13 used, so `NO_INTERCEPT` costs a free bit and no memory: it rides in the same COMMAND_META entry the arity check already reads. One lookup and one bit test replace 21 gate calls. The gates and their ORDER are untouched. The ordering comments above them are not decoration -- they record the bugs that fixed each position (ACL above every privileged intercept, workspace rewrite above every key-reader, MULTI queue below ACL, MONITOR below both). Reordering them to save a branch would re-open those, so the chain is guarded, not rearranged. The four STATE gates -- ACL, cluster routing, readonly, disk-full -- apply to every command whatever its name and are deliberately left unguarded. The bit's sense is inverted on purpose. "This command IS intercepted" is fail-open: a new intercept whose command nobody flagged would be silently skipped, and no throughput test would notice. Unmarked means slow path, so adding either a command or an intercept can cost speed and never correctness. 67 plain keyspace commands are marked; everything else keeps the full chain. The drift guard was verified to work rather than assumed: marking WAIT -- which try_handle_wait provably claims -- makes tests/intercept_flag_drift.rs fail with that name, and unmarking it makes it pass again. `is_inline_intercepted` (shared.rs) was considered for collapsing onto this bit and rejected: it answers the narrower "does this intercept touch keys" for moon#507, and !NO_INTERCEPT is a strict superset. Expected recovery is ~3-5% of cycles. It does not close 0.43x -> 1.0x; the larger cost is the ~10.7% of Frame/Bytes lifecycle that the inline GET/SET path skips and every other command pays. Refs #507 author: Tin Dang * perf(command): answer "is this reply an error" from a borrow, not a clone The monoio local write path built `response_frame` by cloning the `DispatchResult`'s `Frame` and then used it for exactly one thing: `matches!(response_frame, Frame::Error(_))`. `response_frame` had those two occurrences in the whole file and no others, so the clone produced nothing but work. For a `Frame::Array` reply that is a deep clone -- a fresh `FrameVec` box plus one `Bytes` refcount bump per element, plus the matching drops -- paid per command on the general write path. Wave 0 measured that path at 0.68x of single-threaded Redis at p8 with ZERO cross-shard hops, so every cycle here is single-thread execution cost. Adds `DispatchResult::is_error()`, a borrow-only `matches!` over both variants, and calls it at the one site. Semantics are unchanged by construction: the old expression and the new one test the same discriminant over the same two variants. Refs: tmp/WAVE1_B_FRAME.md section 2.1 (Wave 2-A, stage 1 of 3) author: Tin Dang * perf(storage): Database::set borrows its key instead of taking it `set(&mut self, key: Bytes, entry: Entry)` never moved the `Bytes` anywhere. Reading the whole body: `spill_inflight_forget(&key)`, `entry_overhead(&key, ..)`, `hash_expiry_index_note_value(&key, ..)`, `CompactKey::from(key.as_ref())` (which copies the bytes either way), `ColdIndex::remove(&key)`, and both expiry-index writers -- every one takes `&[u8]`. The owned parameter existed only as a signature. Its cost was paid at the call sites: `db.set(key.clone(), entry)` in every write command, i.e. one `shared_v_clone` on the way in and one `shared_v_drop` on the way out per command, for a refcount whose value is never read. Wave 0 measured moon `--shards 1` at 0.68x of single-threaded Redis at p8 on exactly these families, on a leg with zero cross-shard hops -- so this is straight single-thread execution cost. `set`, `set_string` and `set_string_with_expiry` now take `&[u8]`. That removes 32 `Some(k) => k.clone()` key extractions across the string, hash, list, set and sorted-set write paths. Six sites genuinely need ownership (the key outlives the borrow) and keep their clone; the compiler identified every one of them rather than a grep. It also deletes a real allocation, not just a refcount, from RESTORE, COPY, RENAME, MOVE, the cold-tier promote, WAL v3 replay and replication apply: each was building a throwaway `Bytes::copy_from_slice(key)` only to satisfy the old signature. Semantics are unchanged -- `set`'s body is byte-for-byte the same work with `&key` rewritten to `key`. Both runtimes check clean; clippy clean on both legs; 5110 lib tests pass. Refs: tmp/WAVE1_B_FRAME.md section 2.3 (Wave 2-A, stage 2 of 3) author: Tin Dang * perf(shard): ship the cross-shard read fast path enabled by default `--cross-shard-fast-path` now defaults to `auto` instead of `off`. At `--shards 8` on a populated keyspace it serves 100% of foreign reads on the calling thread and takes parks/cmd for GET at p=1 from 0.87336 to 0.00023 -- same binary, one flag apart, measured from INFO stats (total_dispatch_cross_read_fast / total_dispatch_cross_spsc / total_remote_awaits_parked). docs/internal/cross-shard-cost-model.md prices a park at ~24.9 core-us and at 85% of p=1 cost, so this is the largest single lever on the cross-shard read path. Against that fit it predicts 2.586 -> 0.413 CPU%/kops for cross-shard reads at p=1. Reads only. A cross-shard write still parks: the gate is `!is_write(cmd)`, and INCR/LPUSH/SADD/HSET/SET were measured unchanged at 0.875 parks/cmd with the flag on. The write side needs its own mechanism and is not addressed here. Why it shipped `off`, and why that reading was wrong. The evidence was moon#768's -8.61% CPU/op, a doubled s8 p16 variance, and the standing puzzle that #768 measured 50.5% of reads served in place where the model predicted 87.5%. All three come from one cause: the path declines a key that is not resident, because dispatch_read cannot consult the cold tier (the moon#610 class), and a declined read falls back to the SPSC hop. So the measured "in-place rate" was tracking the benchmark's key HIT rate, not the mechanism -- and a wandering hit rate is exactly the run-to-run variance that held the default down. Reproduced against DBSIZE: 63,114 keys resident gives 62.9% in place, 86,396 gives 86.2%, 98,169 gives 98.2%, 100,000 gives 100.0%. Populate to saturation before A/B-ing this flag, and report DBSIZE with the result. `auto` is a real policy, not an alias for `on`: it declines where the path cannot fire -- `--shards 1`, where every key is local and the branch is unreachable, and the tokio leg, where handler_sharded has no fast-path site (moon#776). A switch that cannot change behaviour must not read as enabled. `on` forces it regardless and still gets main.rs's tokio no-op warning. The policy is a pure function (`db_plane::resolve_cross_shard_fast_path`) so it is unit-tested rather than asserted at the call site, and it joins xshard_cleanup_shape's LIVE_SYMBOLS -- deleting it would silently revert the default while leaving the flag parsable, which is the defect that surface test exists to catch. `--cross-shard-fast-path off` is the rollback and is pinned by l4_cross_shard_read_fastpath::the_fast_path_stays_dark_when_the_flag_is_off. No gate in the fast path was weakened. In particular the moon#507/#512 `pending_mask` ordering gate is untouched: a foreign read is still served in place only when this connection has no in-flight remote work on that shard. Counter ratios were taken on macOS, which is legitimate for ratios and never for wall time; no throughput number is claimed here. The Linux A/B recipe is in tmp/WAVE2_B_PARALLEL.md. author: Tin Dang * docs(benchmark): measure NO_INTERCEPT on Linux and record the void first attempt The intercept-gate skip landed with an estimate (~3-5% of cycles) and no measurement. This records the real Linux/ARM A/B: INCR +11.5%/+16.8% at p8/p64, HSET +6.5/+12.0/+11.9%, LPUSH +8.7/+10.9%, SADD +7.6/+11.0%, geometric mean +3.9% over 18 cells with base/ni distributions disjoint at p64. Larger than the 3-5% predicted. GET/SET are bimodal in both arms and carry no signal -- the inline byte path serves them. The FIRST run of this A/B was void and the failure is worth recording: nisrv.sh reused `pkill -9 -x moon` from legsrv.sh, but its binaries are moon-base / moon-ni, so the pattern matched nothing and 16 servers accumulated on one SO_REUSEPORT port. Every arm was then load-balanced across a blend of both binaries, presenting as a 26-39% noise floor, a monotonic 83k->30k decay, and a perfectly null result (geomean 1.0023x, 18/18 within noise). Fixing the kill moved the same commit from "no effect" to "+8-17%, distributions disjoint". author: Tin Dang * docs(internal): record an eighth dead end and two measurement traps The cross-shard cost model exists for negative knowledge. Four additions, each from a measurement taken this wave. Dead end 8 -- shared guard for reads on the SPSC execute arms. All four SPSC execute arms take `s.databases.write(db_idx)` for every command, reads included, so an SPSC-routed read holds the owner's database exclusively for its whole execution: exactly the condition under which a foreign reader's try_read declines and diverts to the SPSC path it was avoiding. The argument predicts a self-sustaining loop. Implemented (shared guard + dispatch_read for hot, read-supported commands, exclusive otherwise) and measured: in-place rate 62.7% -> 62.9% at --shards 8 p=1. Premise refuted; the change was reverted rather than shipped, because a hot-path change with no measured effect is cost without evidence. Trap -- redis-benchmark's built-in tests mostly use ONE key, and -r cannot change it. Verified by FLUSHDB + run + DBSIZE against a live server: lpush, rpush, lpop, rpop, sadd, spop, hset and zadd touch a single literal key with or without -r (-r randomises the element, not the key). Only set, get, incr and mset take a randomised key, and only when -r is passed. For a shared-nothing server that is fatal to any scaling claim: one key is owned by one shard, so sN/s1 ~= 1.0 is the architecturally correct answer, not a finding. A 12-family matrix built on `redis-benchmark -t` has 8 families whose answer is fixed before the server starts. Trap -- a p=1 leg that is not CPU-bound measures the network, and its ratios collapse toward 1.0. The tell is a per-family throughput spread far narrower at p=1 than at p=64 on the same server: 1.29x versus 13.7x on one recent ARM matrix, with every p=1 leg 3-4x below what section 7 records for the same configuration at c=200. New section 8 -- the fast path's measured effect (parks/cmd 0.87336 -> 0.00023), why the old 50.5%-in-place puzzle was the benchmark's key hit rate rather than the mechanism, the closed-form model confirmed to four significant figures against INFO counters at three pipeline depths, mean park depth 12-18 at c=50 (so the 24.9 core-us constant is per-park CPU, not a serialized wait), and the finding that the multi-key coordinator path increments NONE of these counters: MSET of 4 uniform keys reads as 0.0016 parks/cmd -- "already optimal" -- while issuing 1.5-1.8 cross-thread notifies per command at every pipeline depth, and gaining only 2.6x from p=1 to p=64 where every other family gains 7-30x. author: Tin Dang * perf(command): format INCR's new value with itoa, not a per-op String `incrby_internal` stored its result as Entry::new_string(Bytes::from(new_val.to_string())) which allocates a `String` on the command hot path -- the allocation CLAUDE.md forbids by name in `src/command/` -- and then hands it to `CompactValue`, which copies the digits out and frees it immediately. A counter of twelve digits or fewer inlines into the 12-byte SSO payload, so the allocation was never even where the value ended up. INCR is the command the campaign profiled. `itoa::Buffer` formats into a stack buffer instead. To take it by reference the storage layer gains `CompactValue::from_slice` and `Entry::new_string_from_slice{,_with_expiry}` -- the same branch `from_redis_value` takes for `RedisValue::String`, minus the owned `Bytes` the caller had to build first. Nothing is lost by borrowing: both arms copy the bytes anyway, and the heap arm's supposed zero-copy `Bytes::into::<Vec<u8>>()` only applies at refcount 1, which a slice of a shared read buffer never is. Tested red first: the constructors are checked against `Entry::new_string` at every length from 0 to 32 bytes and at nine i64 magnitudes, because the 12/13-byte SSO boundary and both i64 extremes sit inside the range an INCR can reach. The guard was then mutated (heap arm truncating one byte) and confirmed to fail, so it is not vacuous. INCR itself is covered end to end on both the plain and the TTL-preserving arm, which use different constructors. Refs: tmp/WAVE1_B_FRAME.md section 2.5 (Wave 2-A, stage 3 of 3) author: Tin Dang * fix(config): resolve --cross-shard-fast-path against the RESOLVED shard count The previous commit sited the resolver call early in `main`, before shard setup, and passed it `config.shards`. That is the raw CLI value, and `--shards 0` -- auto-detect, the default deployment shape -- leaves it at `0` until `num_shards` is computed ~250 lines later. `auto` therefore saw `0`, returned `false`, and the fast path shipped DISABLED on exactly the multi-core hosts it was written for. A `--shards 8` operator got it; an operator who set nothing got nothing, and no log line, counter, or test said so. Move the resolve-and-set block to immediately after `record_shard_count(num_shards)` -- which reads the resolved count for the same reason -- and pass `num_shards`. Nothing between the two points re-execs or spawns a shard: `malloc_respawn` runs at the top of `main`, and the shard threads start after both. Guarded at two levels: - `resolve_cross_shard_fast_path("auto", 0, true) == Ok(false)` pins the pure function's answer for an unresolved count. `0` declining is the safe direction; the contract is that the caller must not hand it one. - A fourth case in the L4 integration suite spawns with `--shards 0` and NO `--cross-shard-fast-path` argument, then asserts `total_dispatch_cross_read_fast` moves. This is the case the existing three could not see: they all pass `--shards 4` plus an explicit flag, and all three stay GREEN with the bug reintroduced -- verified by building the mutated binary and running the suite against it. It skips with a message, rather than passing vacuously, where auto-detect resolves to one shard and there is no foreign read to serve. `stat()` grows a section-aware sibling because `num_shards` is reported in `INFO server`, not `INFO stats`, and reading the wrong section returns 0 -- which in this file would have read as "the fast path never fired". author: Tin Dang * perf(protocol): parse a flat multibulk in one pass, not two `parse()` walked the request bytes twice. `validate_frame` found every CRLF and computed every argument offset in order to return the frame's total length -- and threw all of the offsets away. `parse_frame_zerocopy` then walked the same bytes again to re-derive exactly those offsets. For a top-level `*N` of `$`-bulks, which is the shape of essentially every client command, `scan_flat_multibulk` now records each argument's span into a stack `SmallVec<[(u32,u32); 16]>` as it validates, and `parse_flat_multibulk` builds the `Frame` straight from the spans: one `memchr` walk and one `strict_atoi` per token instead of two, and no recursive non-inlinable call per element. The profile in tmp/CAMPAIGN_S8_CONTEXT.md attributes 3.77% to `parse_frame_zerocopy` alone, with `validate_frame`'s own cost folded into `protocol::parse` under fat LTO and therefore not separable. The fast path DECLINES on anything it does not handle exactly -- incomplete input, a malformed count or length, a negative count, a null bulk, a nested or non-bulk element, an over-limit count or payload, a RESP3 container, an inline command -- and the untouched two-pass path handles all of them with their existing error kinds and offsets. Declining more often than necessary is always safe; answering differently never is. It also mirrors one piece of leniency that looks like a bug and is not: `validate_frame` advances `pos += len + 2` past a bulk payload without checking those two bytes are CRLF, so `*1\r\n$1\r\naXY` parses. The scanner does not check them either. Verifying them would have made the fast path stricter than the path it replaces. Verification, in the order it happened: - The differential test came first and failed for the right reason (`parse_reference_two_pass` and the scanner did not exist). - `parse_reference_two_pass` is the pre-change pipeline, compiled only under `cfg(test)` / `feature = "fuzzing"`. The test compares it against `parse()` on ~50 hand-picked inputs AND every truncation of each, under four `ParseConfig`s including degenerate limits, across argc 0-20 x payload 0-300, draining whole pipelines rather than one frame. Comparison is on the frame, on the `Display` of the error (which carries the wire fault name, message and offset), and on the bytes consumed. - Five deliberate mutations of the scanner were each confirmed to make those tests fail, so the differential is not vacuous. - A new `resp_parse_fused` fuzz target runs the same differential and is registered in BOTH matrices in `.github/workflows/fuzz.yml`. It found a REAL divergence in 90 seconds on its first run (see below), then ran 1,975,081 executions clean after the fix. The bug it found: `strict_atoi` reads a lone `-` (and `-0`) as ZERO, while `parse()`'s `is_null_multibulk` gate keys on the raw byte `buf[1] == b'-'` and not on the parsed count. So `*-\r\n` is silently consumed by the two-pass path with no frame reported, where the scanner's `count < 0` test let it through as an empty array. The scanner now declines on the byte. Nothing released is affected -- the fast path had not shipped -- but the shape is a trap for any future fast path over these bytes, so it is recorded in CHANGELOG and pinned by four corpus entries. Blast radius is `src/protocol/parse.rs` only; no signature changed anywhere else. Refs: tmp/WAVE1_B_FRAME.md section 4 stage 2 / alternative C author: Tin Dang * fix(protocol): bound the fast path's span reserve by the buffer, not the wire `scan_flat_multibulk` reserved `SmallVec::with_capacity(count)` where `count` comes straight off the wire, bounded only by `config.max_array_length` -- 1Mi by default. `*1048576\r\n` is ten bytes and would have reserved 8 MiB before the scan reached the first element and discovered the frame was incomplete. A client can repeat that at line rate. The two-pass path this replaces never had the amplification: it reaches `FrameVec::with_capacity(count)` only inside `parse_frame_zerocopy`, which runs after `validate_frame` has proved the entire frame is present, so the bytes in the buffer bound the count for free. Moving the allocation ahead of the walk is what created the hole, so the scanner has to re-derive the bound itself. `span_capacity` caps the reserve at `buf.len() / 6`. Six bytes is the shortest an element can be -- `$0\r\n` plus the two trailing bytes every bulk is charged -- so the cap can never under-allocate a scan that goes on to succeed, and `SmallVec` grows regardless if it somehow did. Found by reading back the diff of the commit before it, not by a test, which is why the test came second here rather than first. It is pinned now: the attack shape, the degenerate `(usize::MAX, 0)` case, and an assertion that four real commands still get capacity for every argument. Not present in any release -- the fast path landed one commit ago on this branch. author: Tin Dang * docs(storage): separate the two paragraphs of Database::set's doc comment The borrowed-key rationale added in 7e147ff ran straight on from the PERF-08 paragraph with no `///` between them, so rustdoc rendered the two as one. Comment only. author: Tin Dang * test(bench): add the argc>4 parse pair, so the FrameVec spill can be measured `FrameVec` is `Box<SmallVec<[Frame; 4]>>`, so `with_capacity(count)` heap-spills past four elements: a `*5` command pays two allocations where a `*3` pays one. tmp/CAMPAIGN_S8_CONTEXT.md records `SET k v` at 2.08x and `SET k v EX 100` at 0.87x against Redis and attributes the whole step to the inline byte path -- which is certainly the dominant term, since one command qualifies for that path and the other does not. But a second, independent step sits at exactly the same argc boundary and nothing has ever separated the two. `parse_set_ex_5arg` (`*5`) pairs with the existing `parse_set_single` (`*3`). `parse_hset_4arg` and `parse_hset_6arg` are the control the tmp/WAVE1_B_FRAME.md section 2.6 probe asked for: same command, same work per argument, only the argument count differs -- and the inline path never touches HSET, so a step between 4 and 6 cannot be blamed on it or on command identity. No numbers are claimed. These are instruments; they must be run on a Linux host, and this branch was developed on macOS. author: Tin Dang * chore(release): cut v0.8.8 Promote [Unreleased] to [0.8.8] and bump 0.8.7 -> 0.8.8. The train is 63 merged PRs across 80 commits since v0.8.7, touching 23 issues: wire-level parity against a live redis-server, the search-surface correctness wave, and the first end-to-end measurement of --shards 8 against io-threads 8. Adds BENCHMARK.md 2.14, which reports all three dimensions of that comparison including the two where moon does not win, and retracts two earlier claims in-tree with their raw data kept: - "moon gains nothing from eight shards" (s8/s1 = 0.97x) was a harness artifact. redis-benchmark -t lpush|sadd|hset|zadd drives ONE literal key and -r randomises the element, not the key, so eight of twelve families were asked to parallelise a single key. Re-run with explicit __rand_int__ keys behind a DBSIZE >= 50000 guard proven to fire, real scaling is 1.42x / 2.14x / 3.79x. - The "0.90x per-key memory win" measured redis-benchmark's default 3-byte value. Both legs sat below their own arithmetic floor, which is impossible. Re-measured at 8/64/256-byte values under a key+value+24 floor check -- verified to reject both historical numbers before being trusted -- the win is a band, not a trend, and exists only below the 12-byte CompactValue inline cutoff. Measured, on both architectures: throughput 1.26x (ARM) / 1.32x (x86) at p=8 and 2.74x / 2.91x at p=64; a tie at p=1, because the rig is bimodal for Redis as well as for moon; CPU per op a tie at 10.55 vs 11.33 us, inside Redis's own 11.9% spread. Memory is NOT won: 1.16x worse at 64-byte values and 1.26x worse on idle RSS. The executive summary states both losses as losses. Gates: scripts/ci-local.sh --full PASS (13/13 legs, tree fingerprint unchanged, client-compat PASS=368 FAIL=0 WAIVED=50); hosted dispatch matrix green (Check Windows, MSRV 1.94, Memory steady-state); oracle sweep against live redis-server 8.6.1 -- test-consistency.sh 457/458 and test-commands.sh 516/517, the single red row in each being the known #536 ROLE offset divergence, with no new divergence from either the rewritten RESP parser or the changed Database::set. Discloses #536 as a known divergence riding this release. author: Tin Dang * perf(storage): stop reserving 16 segments per empty DashTable An empty `DashTable` reserved a 16-segment first slab to hold ONE segment. `size_of::<Segment<CompactKey, CompactEntry>>()` is 3,456 B, so `DashTable::new()` allocated 55,296 B to store 3,456 B — 93.75% waste. moon builds `--databases` (16) of these PER SHARD at boot, all empty, so every shard reserved 884,736 B for segments that never exist on an idle server. Measured with a counting global allocator (tests/shard_idle_alloc_attribution.rs), for `--shards N --appendonly no --disk-offload disable`, default features: per-shard term before after one empty Database 55,432 B 3,592 B 16 Databases (per shard) 893,976 B 64,536 B ChannelMesh::new (unchanged) 135,984 B 135,984 B MODEL TOTAL per extra shard 1,030,432 B 200,992 B The 16 empty databases were 84% of ALL per-shard heap reservation — four times the entire N(N-1) SPSC mesh, which is the term that had previously been blamed. The first slab is now sized to demand: 1 segment for `new()`, exactly `dir_size` for `with_capacity()` (that path also rounded up to the fixed slab AND spread its segments across ~log2(dir_size) slabs; it is now one right-sized allocation). The doubling growth is unchanged and reaches the old curve by the fifth slab, so a table that fills sees the same amortised behaviour. Slabs are still never reallocated, so segment pointers stay stable across growth — covered by a new 20,000-key test that walks several slab boundaries and re-reads every key. WHAT IS NOT CLAIMED: an RSS number. Reserved bytes are an UPPER BOUND on resident — untouched pages of a fresh mapping never become resident, and this change removes mostly-untouched tail. A directional macOS A/B (3 interleaved reps, same tree, one constant apart) moved idle RSS at `--shards 8` by -1.07/-1.84/-1.82 MiB and showed NO change at `--shards 1`, but macOS has no jemalloc `background_thread` and different retention, so that number is not publishable. The Linux figure must be re-measured on the benchmark host. Red/green: `empty_table_does_not_over_reserve_segment_slots` and `presized_table_does_not_over_reserve_segment_slots` fail on the parent commit (16 slots reserved for 1 live segment). The integration gate was attacked by reverting the constant in place and confirming it reports the exact pre-fix figure (1,030,432 B over a 262,144 B budget) before being restored. Verified: 5,129 lib tests green (monoio, default features); `cargo check --no-default-features --features runtime-tokio,jemalloc --all-targets` green; `cargo clippy --all-targets` zero warnings; `cargo fmt --check` clean; server boots and serves 20,001 keys across dbs 0/1/2/15 at both s1 and s8. No new unsafe in `src/`. author: Tin Dang
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
1 participant
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.
Second half of #513. #721 (A1) landed single-owner routing; this is the genuinely spanning case, where the filed −52.4% still lives — untagged keys span with probability
1 − 1/shards, so most real interleavings still cut the batch.A spanning multi-key READ is decomposed per owner shard, dispatched inside the existing
PipelineBatchSlotted, and folded back into one client reply.READS ONLY
MGET(Gather) andEXISTS(SumInteger) are the only commands admitted.MSET/DEL/UNLINKare A2b and deliberately absent — they add per-part AOF serialization, per-part replication, the group-commit barrier and tracking invalidation. Landing them together would make a durability bug and a throughput change bisect to one commit.Why write loss cannot recur
The requirement is per-key: for C1 before C2 in one connection's byte stream both touching
k, C1's effect onkmust be visible to C2.khas exactly one owner shard — a pure function of the key bytes — and ownership does not move mid-batch. So every operation onkis placed either intoremote_groups[s]in loop order (executed by one thread in vector order) or inline against the local slice in loop order. The two cannot mix for one key.#507 was a violation of the first case specifically:
coordinate_multi_keyused a second, immediate push onto the same SPSC ring, overtaking still-buffered entries. A2a removes that second path for the splittable reads; it does not add one.One decision function, not two predicates
multikey_placementreturnsSlotted | Fanout | Coordinator, consumed by both the guard and the routing sites. #708's own commit message flags "two answers to the same question" as the hazard, and #721 already needed the halves to agree.Everything else keeps waiting bit-for-bit: inline-intercepted commands (declared keys don't describe touched state), keyless aggregations, any untrustworthy mask (>64 shards, non-string key position,
AtPlusComputed), workspace connections (keys rewritten below the guard), and the non-decomposableMSETNX/BITOP/COPY.Sits below the #500
txn_multikey_writerefusal, and respects thehandler_sharded/handler_monoionum_shards <= 1asymmetry — the same asymmetry that made tokio lose multi-keyDELin #500.The new metric is load-bearing, not telemetry
A fall in the defer counter alone is also what a "fix" that merely stopped waiting would produce — that is #507 reopened.
pco16asserts both halves: zero deferrals AND that the command took the route which justifies not waiting.Verification (release build,
MOON_BINpinned)pco16red/green proven by mutation: forcing spanning reads back toCoordinatorgives 32 deferrals over 32 interleavings on every spanning pair; with the change, 0.pco1–pco15unchanged; 16/16 green.--all-targets, including--no-default-features --features runtime-tokio,jemalloc.What is NOT claimed
No throughput number. The −52.4% was measured on Linux; the evidence here is placement counts on macOS. The size of the A2a win stays a hypothesis until
bench-single-owner-multikey.pyruns a spanning shape on a Linux host.Partial failure is unchanged, not improved: a backpressure reject on one shard errors that part while another may have applied —
coordinate_multi_keyhas the identical exposure today for spanning writes.