Commits on Sep 13, 2026

1. 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
   CommunityToolkit/dotnet#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.

   

   mgravell authored Sep 13, 2026
   Configuration menu

   • View commit details
   • Copy full SHA for c1a317b
   • Browse repository at this point

   Copy the full SHA

   c1a317b View commit details

   Browse the repository at this point in the history