Do not assume GetSpan honoured the size hint (#3220)
* Do not assume GetSpan honoured the size hint
IBufferWriter<T>.GetSpan(sizeHint) guarantees one element, not sizeHint. The
"should be at least this size" wording in the docs is left over from when the
parameter was called minSize; it is advisory, and consuming code is expected to
test what it got and fall back. The BCL does exactly that - BuffersExtensions
loops and copies in slices rather than demanding one contiguous block. See
https://github.com/CommunityToolkit/dotnet/issues/1208.
MessageWriter treated it as a guarantee throughout: take the span, write the
full amount into it, advance by that amount.
The looser contract exists for transports with page limits, which can honour
reasonable requests and refuse excessive ones. CycleBuffer is one - it caps the
hint at 1k, sizes a fresh segment from the committed total rather than from the
hint, and, the case with no floor at all, hands back a dangling recycled segment
exactly as it is, whatever length that happens to be. All legitimate; the
callers were wrong. It is also why this reproduces intermittently and under
concurrency rather than deterministically.
Reported as an ArgumentOutOfRangeException from WriteUnifiedSpan, but that is
the benign manifestation and not the only site. Every fixed-size write had the
same shape, and one of them is worse than an exception:
var span = writer.GetSpan(expectedLength);
fixed (byte* bPtr = &MemoryMarshal.GetReference(span))
Encoding.UTF8.GetBytes(cPtr, value.Length, bPtr, expectedLength);
writer.Advance(expectedLength);
expectedLength is passed to the encoder as the destination capacity rather than
being derived from the span, so against a short span that is a buffer OVERRUN -
it writes past the end of memory it does not own, silently. It now falls through
to the encoder loop below it, which sizes every write from span.Length and
already copes with whatever it is given.
Every other site checks the length inline and hands the rare case to a separate
NoInlining method that composes in a stack buffer and writes through
BuffersExtensions.Write. That split is load-bearing, not style: a stackalloc in
the caller's cold branch changes codegen for the whole method, and writing the
fallbacks inline cost 20% on the simplest command shape. It is the same
"inline-optimized for it-fits, pathological case doesn't inline" structure the
BCL uses.
Measured, formatting only, SET key value: 64.66ns before against 66.64ns after,
so +3.1% - small error bars, so real rather than noise. That is the honest price
of checking. Larger values come out ahead (203.7ns to 184.9ns), because sharing
one length-prefix helper removed duplicated work.
Sites fixed: both WriteHeader overloads, both WriteMultiBulkHeader overloads,
WriteUnifiedPrefixedString, WriteUnifiedPrefixedBlob, WriteUnifiedInt64,
WriteUnifiedUInt64, WriteUnifiedDouble, WriteInteger,
WriteUnifiedSequenceIterator, the SHA1 bulk-string path, WriteUnifiedSpan and
WriteCrlf. The encoder loop was already correct; the commented-out
WriteUnifiedSequence is left alone.
Tests use a writer that honours at most N bytes per span and places a canary
past the span it hands out, so an overrun is detected rather than landing
harmlessly in slack - a plain growing buffer passes while the real writer
corrupts the next thing in the pool. 13 tests across the value shapes; 6 fail
without the fix, with two distinct crash modes.
Full suite against the docker topology: 6172 passed, 150 skipped, 1 failed -
FailoverTests.SubscriptionsSurvivePrimarySwitchAsync, which fails identically on
main.
* Move the hint fallbacks out of line, and address review findings
Restores the NoInlining split that was lost to a git checkout during
benchmarking, plus the review findings:
- WriteUnifiedInt64 passed MaxPrefixScratch (30) while requesting 27; now
MaxValueScratch, which is what its doc comment says it is for.
- PrefixScratch slices both spans to length, so it bounds-checks in Release
rather than relying on a Debug.Assert that is gone there.
- WriteCountPrefix is sized for an int64: the multi-bulk header, the blob prefix
and the sequence iterator all format a long and previously asked for int32
width, so the direct path could be handed exactly 14 bytes and write 23.
- WriteCrlf keeps its hint of 2 rather than falling to BuffersExtensions.Write,
which asks with no hint at all and would split a CRLF across segments.
- The test harness checks its canary on hand-out and on reading the result, not
only on Advance, so a span written past and then abandoned cannot slip through.
- Coverage for WriteSha1AsHex (the tightest fit in the file), keyspace prefixes,
the prefixed multi-bulk header, WriteInteger, and a long-width count.
* Benchmark the fallback path too, and correct the performance claims
Adds a stingy writer to the benchmark, so the fallback branch is actually
exercised: the Reusable writer always returns the remainder of a 16KB buffer and
so only ever measures what the CHECK costs, which the previous table could not
be read as the cost of the fix generally.
Corrects the numbers, which were overstated in two directions:
- The -9.2% on LargeValue was partly measurement drift. main's own figure moved
between 187ns and 204ns across sessions, so the claim that sharing a
length-prefix helper made large values faster is not supported and is dropped.
- MixedValueShapes swung between -0.4% and +4.5% across runs. BenchmarkDotNet's
error bars measure within-run precision, not between-run reproducibility, and
at this scale the latter is several percent.
Only the SET key value figure is reproducible: 64.48ns against 66.42ns, so about
+3%, measured twice independently with matching filter sets. Note the filter set
matters - including the fallback benchmark in the same run moved the figure to
70.60ns, so before-and-after runs must measure the same set.
* Cover the reported binary shape; correct two overstated test labels
- add ShortSpansStillProduceCorrectFramesForLargeBinaryValues: byte[] takes a
different route from string through WriteUnifiedSpan, and the value-shapes
test only reached it with four bytes. 400/512/513 at caps of 8/400/520.
- note that [InlineData(520, 512)] on the string theory does not stress that
path - a 512-byte string asks for exactly 512, which a 520-cap writer
honours, so it passes unfixed. Kept as regression cover, not as evidence.
- relabel CountPrefixIsSizedForALong as defensive: main's long overload really
does hint int32 width, but argument counts come from array lengths, so no
caller can reach it.
- document that TryGetSpan returning false is not side-effect free, and the two
ways StingyWriter is stricter than a real writer.
* Make the binary straddle cases actually straddle
519/512 is the case that bites: the hint carries int32-text slack it rarely
uses, so a cap has to fall below the real frame length (1 + digits + 2 + len +
2 = 520 here), not merely below the hint, to overrun. Marked which cases fail
unfixed and which are boundary cover.
* Keep the in-place length prefix for large values; make the stingy writer stingy
Three findings from review:
1. WriteUnifiedSpanSlow was the NORMAL path for values over MaxQuickSpanSize,
not a fallback, and called WriteCountPrefixSlow unconditionally - so every
4KB byte[] composed its $4096\r\n into a stack buffer and handed it to a
hintless Write, costing a copy and risking the prefix splitting across
segments: the same fragmentation just fixed in WriteCrlf. The string path
never had this. Now calls the checked WriteCountPrefix, and is renamed
WriteUnifiedSpanPiecewise since '...Slow' described only one of its two
entry reasons. The two header fallbacks likewise: the ask that was declined
was larger than 23, so the smaller one may still land in place.
2. The Stingy benchmark writer capped at 64, but the largest ask in
SET user:1 marc is 23 - so it declined nothing and measured the direct path
twice. 16 forces the fallback but still does not fault on main; 8 does both.
Same correction to ShortSpansStillProduceCorrectFrames, whose comment
claimed 64 applied pressure.
3. MaxPrefixScratch was dead - every stackalloc uses MaxValueScratch or
Sha1BulkStringLength. Deleted, wording folded into MaxValueScratch.
WriteCountPrefixSlow is now reached only from WriteCountPrefix.
* Bring two doc comments up to date with the previous commit
- WriteUnifiedSpanPiecewise keeps NoInlining, but no longer for the reason
documented on WriteCountPrefixSlow: there is no stackalloc left in it to
perturb the caller's codegen. It is now simply about keeping the large-value
body out of the hot small-value WriteUnifiedSpan frame. Said so.
- WriteHeaderSlow/WriteHeaderUnframedSlow inherited a summary saying they
'compose in a stack buffer and let the looping write place it'. Since the
last commit they call the checked WriteCountPrefix, which only composes if it
is itself declined. The 'fallback when the writer declined the hint' half was
still accurate; only the mechanism half was stale. M
Marc Gravell committed
c1a317bddd1090c9c00d390ce59f2625ef2b89b2
Parent: 611e478
Committed by GitHub <noreply@github.com>
on 9/13/2026, 1:44:44 PM