Conversation
StackExchange.Redis v3's async PhysicalConnection read loop reads, then parses/dispatches, then reads again - all on one path. Under concurrent load this caps how much data accumulates per parse pass at whatever a single physical read returns (~5KB observed), versus the pre-RESPite Pipe-backed reader's ~29-31KB, because nothing kept reading while the old design's consumer was busy. That gap is the root cause of a concurrent bulk-string (GET) throughput ceiling that integer replies don't hit, reported against 3.3.0 vs 2.9.11. ReadAllAsync now runs a background filler task that keeps reading into the shared CycleBuffer independently of the parse loop, synchronized by a lock plus a bounded semaphore wake signal. This needed no changes to CycleBuffer's own single-writer semantics: its existing lease/discard tolerance (already covered by CanDiscardSafely) is sufficient once the filler and parser are genuinely mutually excluded and the filler never holds more than one outstanding, uncommitted read at a time. Also fixes a latent bug this surfaced: Segment.Recycle() never reset StartTrimCount before returning the segment to the shared static spare slot, so a segment recycled via AppendOrRecycle's search-exhausted path (the only path that skips Untrim()) could later be handed out by Segment.Create() as a "fresh" segment with a stale trim offset. Pre- existing and unrelated to this change; only the new stress test's heavy segment churn hit it reliably enough to notice. Bytes committed per parse cycle: ~5KB -> ~23-28KB, closing most of the gap to the pre-RESPite ~29-31KB. Verified via a new CycleBuffer concurrency stress test (two real threads, lock-synchronized, running byte-pattern verification), the full RESPite.Tests and StackExchange.Redis.Tests suites, and a continuous byte-for-byte correctness check built into ad hoc load testing (zero corruption across millions of real GETs).
If the parse loop gives up (a parse fault, ForceReconnect, or the filler's own clean EOF), the filler could still be mid-cycle: it might commit a read into _readBuffer after the parse loop's finally block has already wiped it, or race that unsynchronized wipe outright, since _readBuffer is a multi-field struct. _readLoopDoomed is set before the wipe (now itself lock-protected) and checked by the filler both before requesting its next buffer and after its current read completes, before combining (committing) it - so it stops cleanly rather than touching a buffer that's gone or racing the wipe itself.
System.Range/System.Index aren't available on net481, and RESPite.Tests multi-targets it. mem.Span[..chunkLen] -> mem.Span.Slice(0, chunkLen). Verified: dotnet build Build.csproj -c Release /p:CI=true (all TFMs, whole solution) now succeeds; previously this only surfaced on CI (net10.0-only local runs don't build net481).
ReadAllSync had the same single-threaded batching ceiling as the pre-fix async loop, for the same reason: nothing kept reading while this thread was busy parsing/dispatching. Give it its own background filler thread, mirroring ReadAllAsync/FillBufferAsync, so DedicatedThreads connections get the same batching-depth recovery - on top of the reader thread that mode already pays for, not layered as a separate cost. Preserves the existing mid-flight sync->async transition (ShouldTransitionToAsync): the filler thread is explicitly joined before handing the buffer off to ReadAllAsync's own filler, since two independent fillers racing the same CycleBuffer would violate the single-writer assumption both designs rely on. Leaves a note on FillBufferAsync flagging an open question for a future architecture that moves parsing/dispatch out of the read loop entirely: that might recover the same batching depth without needing a second thread at all, since the second thread exists specifically to stop dispatch work from competing with re-issuing reads on one thread.
…ismatch _readLoopDoomed was checked outside _readBufferLock before touching _readBuffer in both fillers. Between the check and taking the lock, the parse loop's own exit path could doom-and-Release() (or doom-and-wipe) in between, so the filler could then get a lease from an already-released buffer - and since CycleBuffer.Release() hands segments back to the shared *static* spare slot regardless of any outstanding lease, that lease could point at memory another connection's CycleBuffer now owns. Moving the check inside the same lock that guards the touch it gates makes the two mutually exclusive. Also closed a related gap the ForceReconnect exit path had on both readers: Release() was called before _readLoopDoomed was set (only finally set it), so a ForceReconnect break - unlike the filler's own clean-EOF break, where the filler has already stopped by construction - could still race a genuinely-running filler. Doom is now set before Release() on that path too. SignalFillProgress's CurrentCount==0-then-Release() is safe today only because it's never called concurrently with itself; documented that invariant and added a SemaphoreFullException catch with Debug.Fail, so a future second caller fails loudly in debug/test instead of the exception silently vanishing into _fillerFault. The sync filler thread was inheriting ThreadPriority.AboveNormal from the parser thread it was copied from. The parser does the higher-value, latency-sensitive work (dispatch, TCS completion, user callbacks) and should win any priority-based scheduling contest under load; dropped the filler to Normal.
The prior fix closed "take a new lease after Release()" by moving the _readLoopDoomed check inside the lock, but not "a lease taken before Release() runs, still being written into": Release() hands segments back to the shared static spare regardless of any outstanding lease, so a filler read still in flight when Release() runs can go on writing into memory another connection's CycleBuffer has since taken over. The post-read doom check stops the *commit*, not the write that already landed. My first attempt at a fix - awaiting/joining the filler before Release() - was itself broken: the filler can only unblock via new data or the stream being disposed, and that disposal is exactly what RecordConnectionFailed (called right after) is about to do. Waiting for the filler first deadlocks against the very teardown that would unblock it - confirmed by 8 MovedUnitTests failures (SocketClosed, 0-read) once that landed, all in the ForceReconnect-into-retry path. Fixed properly instead: only call Release() when _fillerDone proves the filler has already, definitely, fully stopped (set in its own finally, right before it returns - a stronger guarantee than doomed, which only stops a *new* lease or commit). When it isn't yet safe, skip the recycle and let the existing plain wipe in finally - which only drops the field's value rather than returning anything to the shared pool - handle cleanup instead, same as the fault/cancellation paths already do. No waiting, no deadlock risk, and the corruption window closes without a behavioural cost on the common path. Verified: RESPite.Tests, StackExchange.Redis.Tests (multiple full runs, including the specific tests that caught the deadlock), full all-TFM build.
Allow uploading of hidden files from .nupkgs directory.
mgravell
marked this pull request as ready for review
September 23, 2026 10:28
This branch was successfully deployed
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.
Summary
An external comparison (2.9.11 vs 3.3.0) reported that concurrent GET (bulk-string) throughput
plateaus around 390-400k ops/s in 3.3.0 while 2.9.11 keeps scaling; INCR (integer replies) is
unaffected. This bisected to the root mechanism and mitigates it, for both the default async
reader and the opt-in sync/
DedicatedThreadsreader.PhysicalConnection.ReadAllAsync's async loop reads, then parses/dispatches,then reads again, all on one path. Under concurrent load this caps how much data accumulates
per parse pass at whatever a single physical read returns (~5KB observed on this box), versus
the pre-RESPite
Pipelines.Sockets.Unofficial-backed reader's ~29-31KB, because nothing keptreading while the old design's consumer was busy parsing.
ReadAllSync(used only whenDedicatedThreadsis enabled) had the identical ceiling for the identical reason.ReadAllAsyncandReadAllSyncnow run a background filler (aTaskand adedicated
Thread, respectively) that keeps reading into the sharedCycleBufferindependently of the parse loop, synchronized by a lock plus a bounded semaphore wake signal.
For
DedicatedThreadsthis is a second dedicated thread per connection, on top of the one thatmode already pays for to avoid ThreadPool starvation - not a separate cost layered on that
existing trade-off. No changes needed to
CycleBuffer's own single-writer semantics - itsexisting lease/discard tolerance (already covered by
CanDiscardSafely) is sufficient once thefiller and parser are genuinely mutually excluded and the filler never holds more than one
outstanding, uncommitted read at a time. The sync path additionally has to join its filler
thread before handing the buffer off to
ReadAllAsync, on the (rarely used) mid-flightsync-to-async transition - see the remaAsync:
┌───────────────┬─────────┬─────────┬─────────┬──────────┬───────────┐
│ scenario │ 2.9.11 │ 3.3.0 │ 3.3.7 │ vs 3.3.0 │ vs 2.9.11 │
├───────────────┼─────────┼─────────┼─────────┼──────────┼───────────┤
│ get-1k-conc64 │ 707,644 │ 387,189 │ 814,369 │ +110.3% │ +15.1% │
├───────────────┼─────────┼─────────┼─────────┼──────────┼───────────┤
│ get-8k-conc64 │ 178,535 │ 147,490 │ 321,983 │ +118.3% │ +80.3% │
├───────────────┼─────────┼─────────┼─────────┼──────────┼───────────┤
│ get-8k-pipe32 │ 197,306 │ 211,779 │ 227,258 │ +7.3% │ +15.2% │
└───────────────┴─────────┴─────────┴─────────┴──────────┴───────────┘
Sync is the same shape: get-1k-conc64 +61.6%, get-8k-conc64 +130.6%.
Not merely restored — get-1k-conc64 now beats 2.9.11 by 15%, and get-8k-conc64 by 80%. The 8 KiB case gains more than the 1 KiB one it was diagnosed on, exactly as your PR predicted.
The mechanism confirms the diagnosis independently. Commands per recv(), measured server-side:
┌───────────────┬────────┬───────┬───────┐
│ scenario │ 2.9.11 │ 3.3.0 │ 3.3.7 │
├───────────────┼────────┼───────┼───────┤
│ get-1k-conc64 │ 15.2 │ 5.1 │ 16.4 │
├───────────────┼────────┼───────┼───────┤
│ get-8k-conc64 │ 3.7 │ 2.2 │ 10.1 │
├───────────────┼────────┼───────┼───────┤
│ incr-conc64 │ 16.4 │ 18.2 │ 18.4 │
└───────────────┴────────┴───────┴───────┘
Batching depth collapsed in 3.3.0 and is restored — and at 8 KiB it's now 2.7× better than 2.9.11 ever managed. incr-conc64 is flat throughout, which is why INCR never regressed.
What it costs — your stated trade-off is real and measurable
┌────────────┬───────┬───────────┬───────────┬─────────┐
│ scenario │ api │ 3.3.0 p50 │ 3.3.7 p50 │ delta │
├────────────┼───────┼───────────┼───────────┼─────────┤
│ incr-seq │ async │ 20.4µs │ 20.8µs │ +0.32µs │
├────────────┼───────┼───────────┼───────────┼─────────┤
│ incr-seq │ sync │ 23.5µs │ 24.6µs │ +1.04µs │
├────────────┼───────┼───────────┼───────────┼─────────┤
│ get-8k-seq │ async │ 23.4µs │ 25.2µs │ +1.73µs │
├────────────┼───────┼───────────┼───────────┼─────────┤
│ get-8k-seq │ sync │ 26.3µs │ 29.0µs │ +2.62µs │
└────────────┴───────┴───────────┴───────────┴─────────┘rks on
ReadAllSync.Segment.Recycle()neverreset
StartTrimCountbefore returning a segment to the shared static spare slot, so asegment recycled via
AppendOrRecycle's search-exhausted path could later be handed out bySegment.Create()as a "fresh" segment with a stale trim offset. Unrelated to this change;only the new stress test's heavy segment churn hit it reliably enough to notice.
ForceReconnect,clean EOF), the filler now stops cleanly via a
_readLoopDoomedflag instead of potentiallycommitting into (or racing) a
_readBufferthat the parse loop's teardown has already wiped.Applies to both readers.
Results
Measured with RespFest (
github.com/seredis/respfest), the actual harness that surfaced theoriginal regression - 6 clients (2.9.11 and pinned 3.3.0, each sync + async, plus this branch
sync + async) x 8 scenarios, 96/96 cells, 0 errors. Reduced to 2 repeats x 8s measured (vs. the
harness's default 3 x 15s) to fit the time available; treat the sequential-latency reading below
as directionally right rather than final. Full report and raw JSON available on request.
get-1k-conc64(async)get-8k-conc64(async)get-8k-pipe32(async)incr-conc64(async)Concurrent GET roughly doubles at both 1KiB and 8KiB payloads (not just the 1KiB shape the
regression was diagnosed on), on both the async- and sync-calling rows (RespFest's "sync" is the
caller's calling convention - blocking
StringGet()vsawait StringGetAsync()- notDedicatedThreads; both still ride this same fix internally), and now exceeds 2.9.11 rather thantrailing it. INCR concurrent, never broken, stays flat.
The one real, honest cost: sequential (single-caller, one op in flight) GET latency is
consistently ~1-3us higher across every sequential scenario and both APIs (e.g.
get-1k-seqp5017.9us -> 18.8us async, 21.4us -> 24.2us sync). Each individual gap sits close to the run's own
noise floor, but the direction is consistent everywhere it could show up, and it has a clean
explanation: decoupling read from parse into a separate filler task adds one thread-hop-and-signal
round trip per reply. A concurrent caller earns that back many times over as the filler races
ahead; a lone sequential caller gets no such benefit and just pays the toll.
incr-seqwas flatto marginally affected the same way - consistent with the mechanism, not a separate problem.
Also verified: a continuous byte-for-byte correctness check built into ad hoc load testing (every
GET verified against the known value) - 0 corruption across millions of real ops, independent of
the RespFest run above (which also reported 0 errors on every cell).
Sync/
DedicatedThreadsreaderSeparate RespFest run, same box, comparing this branch's
DedicatedThreadsmode before and afterthe fix (48/48 cells, 0 errors), plus the plain async/
sync-calling-convention row for context:DedicatedThreads(before)DedicatedThreads(after, this PR)get-1k-conc64get-8k-conc64get-8k-pipe32incr-conc64Same shape as the async fix: concurrent GET improves substantially (+37% at 1KiB, +77% at 8KiB),
INCR concurrent is flat, and there's a small honest sequential-latency cost from the same
thread-hop mechanism (e.g.
get-8k-seqp50 31.2us -> 34.0us;incr-seqp50 28.1us -> 28.4us).The one difference in kind, not degree: idle CPU usage per op is measurably higher on the
sequential/low-throughput scenarios (e.g.
incr-seq62 -> 90 CPU us/op) because a seconddedicated thread is now parked waiting on I/O even when nothing is happening - a fixed cost that's
already amortised away well before
conc64throughput.Test plan
CycleBufferTests.ConcurrentFillAndParse_PreservesDataUnderStress(two real threads,lock-synchronized, running byte-pattern verification) - clean across 60+ repeated runs
RESPite.Tests: 1718/1718 passingStackExchange.Redis.Testsagainst the docker topology: 6470/6470 passing (runmultiple times, including the
DedicatedThreads-specific suite explicitly)dotnet build Build.csproj -c Release /p:CI=true: cleanreader table above; separately, 48/48 cells, 0 errors, sync/
DedicatedThreadsreader table aboveNot done / follow-ups worth knowing about
FillBufferAsyncflagging an open question for a future architecture (e.g. av4 that moves parsing/dispatch out of the read loop entirely, onto whatever consumes the parsed
results): that might recover the same batching depth without needing a second thread at all,
since the second thread here exists specifically to stop dispatch work from competing with
re-issuing reads on one thread. Untested hypothesis, not a finding - worth revisiting once that
groundwork exists, to see whether the dual-reader split in this PR can then be undone.
powersave, notperformance, and this box's 14 CPUs required overriding RespFest's ownchecked-in profile, which assumes a 24-thread machine) - worth a clean run on dedicated hardware
before treating the numbers as final, even though the shape of the result (ceiling gone,
small sequential-latency trade-off) should hold.
Some side-by-side runs:
Async:
Sync is the same shape: get-1k-conc64 +61.6%, get-8k-conc64 +130.6%.
Not merely restored — get-1k-conc64 now beats 2.9.11 by 15%, and get-8k-conc64 by 80%. The 8 KiB case gains more than the 1 KiB one it was diagnosed on, exactly as your PR predicted.
The mechanism confirms the diagnosis independently. Commands per recv(), measured server-side:
Batching depth collapsed in 3.3.0 and is restored — and at 8 KiB it's now 2.7× better than 2.9.11 ever managed. incr-conc64 is flat throughout, which is why INCR never regressed.
What it costs — the stated trade-off is real and measurable