From 23b087ab92587a923080843c48bbae060ddc0c56 Mon Sep 17 00:00:00 2001 From: mgravell Date: Sun, 13 Sep 2026 07:30:18 +0100 Subject: [PATCH 1/7] Do not assume GetSpan honoured the size hint IBufferWriter.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. --- src/StackExchange.Redis/MessageWriter.cs | 246 +++++++++++++----- .../MessageWriterBenchmarks.cs | 73 ++++++ .../BufferWriterHintTests.cs | 149 +++++++++++ 3 files changed, 399 insertions(+), 69 deletions(-) create mode 100644 tests/StackExchange.Redis.Benchmarks/MessageWriterBenchmarks.cs create mode 100644 tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs diff --git a/src/StackExchange.Redis/MessageWriter.cs b/src/StackExchange.Redis/MessageWriter.cs index 93e63b58d..be3a7782b 100644 --- a/src/StackExchange.Redis/MessageWriter.cs +++ b/src/StackExchange.Redis/MessageWriter.cs @@ -164,14 +164,21 @@ internal void WriteHeader(RedisCommand command, int arguments) // *{argCount}\r\n = 3 + MaxInt32TextLen // ${cmd-len}\r\n = precomputed // {cmd}\r\n = precomputed - var span = _writer.GetSpan(commandBytes.Length + 3 + Format.MaxInt32TextLen); - span[0] = (byte)'*'; - - int offset = WriteRaw(span, arguments + 1, offset: 1); - commandBytes.CopyTo(span.Slice(offset)); - offset += commandBytes.Length; - - _writer.Advance(offset); + if (TryGetSpan(_writer, commandBytes.Length + 3 + Format.MaxInt32TextLen, out var span)) + { + span[0] = (byte)'*'; + int offset = WriteRaw(span, arguments + 1, offset: 1); + commandBytes.CopyTo(span.Slice(offset)); + _writer.Advance(offset + commandBytes.Length); + } + else + { + // the command bytes are already framed, so they can go straight through the looping write + Span scratch = stackalloc byte[MaxPrefixScratch]; + scratch[0] = (byte)'*'; + _writer.Write(scratch.Slice(0, WriteRaw(scratch, arguments + 1, offset: 1))); + _writer.Write(commandBytes); + } } internal void WriteHeader(RedisCommand command, int arguments, ReadOnlySpan commandBytes) @@ -186,29 +193,40 @@ internal void WriteHeader(RedisCommand command, int arguments, ReadOnlySpan scratch = stackalloc byte[MaxPrefixScratch]; + scratch[0] = (byte)'*'; + int offset = WriteRaw(scratch, arguments + 1, offset: 1); + scratch[offset++] = (byte)'$'; + offset = WriteRaw(scratch, commandBytes.Length, offset: offset); + _writer.Write(scratch.Slice(0, offset)); + _writer.Write(commandBytes); + WriteCrlf(_writer); + } } internal static void WriteMultiBulkHeader(IBufferWriter writer, long count) { // *{count}\r\n = 3 + MaxInt32TextLen - var span = writer.GetSpan(3 + Format.MaxInt32TextLen); + var scratch = new PrefixScratch(writer, 3 + Format.MaxInt32TextLen, stackalloc byte[MaxPrefixScratch]); + var span = scratch.Span; span[0] = (byte)'*'; - int offset = WriteRaw(span, count, offset: 1); - writer.Advance(offset); + scratch.Commit(WriteRaw(span, count, offset: 1)); } internal static void WriteMultiBulkHeader(IBufferWriter writer, long count, RespPrefix prefix) { // *{count}\r\n = 3 + MaxInt32TextLen - var span = writer.GetSpan(3 + Format.MaxInt32TextLen); + var scratch = new PrefixScratch(writer, 3 + Format.MaxInt32TextLen, stackalloc byte[MaxPrefixScratch]); + var span = scratch.Span; span[0] = (byte)prefix; if ((prefix is RespPrefix.Map or RespPrefix.Attribute) & count > 0) { @@ -218,8 +236,101 @@ internal static void WriteMultiBulkHeader(IBufferWriter writer, long count paramName: nameof(count), message: $"{type} data must be in pairs; got {count}"); } - int offset = WriteRaw(span, count, offset: 1); - writer.Advance(offset); + scratch.Commit(WriteRaw(span, count, offset: 1)); + } + + /// + /// The largest fixed-size burst any single write below needs: a prefix byte, two int32 text forms and + /// their CRLFs, or a prefix plus an int64 text form and CRLF - whichever is larger. + /// + private const int MaxPrefixScratch = 8 + Format.MaxInt32TextLen + Format.MaxInt32TextLen; + + /// A SHA1 hash as a bulk string: $40\r\n plus 40 hex characters plus CRLF. + private const int Sha1BulkStringLength = 47; + + /// As , but also covering a formatted int64 or double payload. + private const int MaxValueScratch = 7 + Format.MaxDoubleTextLen > 7 + Format.MaxInt64TextLen + ? 7 + Format.MaxDoubleTextLen + : 7 + Format.MaxInt64TextLen; + + /// + /// Obtain a span of at least bytes, reporting whether the writer obliged. + /// + /// + /// + /// sizeHint is advisory. The "should be at least this size" wording in the docs is left over + /// from when the parameter was called minSize; the guarantee is one element, and consuming code is + /// expected to test what it got and fall back. The BCL does exactly that - + /// loops and copies in slices rather than demanding one contiguous block - which is why the fallbacks here + /// hand their bytes to instead of asking again. + /// See . + /// + /// + /// The point of the looser contract is transports with page limits: they 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. + /// + /// + private static bool TryGetSpan(IBufferWriter writer, int length, out Span span) + { + span = writer.GetSpan(length); + return span.Length >= length; + } + + /// + /// A destination for a short, fixed-size burst: the writer's own span when it is long enough, and a stack + /// buffer when it is not. + /// + /// + /// + /// Keeps the zero-copy path for the overwhelmingly common case while staying correct when the hint is not + /// honoured, which explains. On the fallback path the composed bytes go through + /// , which loops until everything is written. + /// + /// + /// Writing past a short span was an at best, and - where the + /// destination length was handed to an encoder rather than derived from the span - a buffer overrun. + /// + /// + private readonly ref struct PrefixScratch + { + private readonly IBufferWriter _writer; + private readonly Span _span; + private readonly bool _direct; + + public PrefixScratch(IBufferWriter writer, int length, Span fallback) + { + Debug.Assert(fallback.Length >= length, "fallback too small for the requested burst"); + _writer = writer; + if (TryGetSpan(writer, length, out var span)) + { + _span = span; + _direct = true; + } + else + { + _span = fallback; + _direct = false; + } + } + + /// Where to compose the bytes. + public Span Span => _span; + + /// Hand of to the writer. + public void Commit(int bytes) + { + if (_direct) + { + _writer.Advance(bytes); + } + else + { + _writer.Write(_span.Slice(0, bytes)); + } + } } private static ReadOnlySpan NullBulkString => "$-1\r\n"u8; @@ -247,10 +358,10 @@ internal static void WriteUnifiedPrefixedString(IBufferWriter writer, byte } else { - var span = writer.GetSpan(3 + Format.MaxInt32TextLen); + var scratch = new PrefixScratch(writer, 3 + Format.MaxInt32TextLen, stackalloc byte[MaxPrefixScratch]); + var span = scratch.Span; span[0] = (byte)'$'; - int bytes = WriteRaw(span, totalLength, offset: 1); - writer.Advance(bytes); + scratch.Commit(WriteRaw(span, totalLength, offset: 1)); if (prefixLength != 0) writer.Write(prefix); if (encodedLength != 0) WriteRaw(writer, value, encodedLength); @@ -270,10 +381,7 @@ internal static int WriteCrlf(Span span, int offset) [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static void WriteCrlf(IBufferWriter writer) { - var span = writer.GetSpan(2); - span[0] = (byte)'\r'; - span[1] = (byte)'\n'; - writer.Advance(2); + writer.Write("\r\n"u8); } [MethodImpl(MethodImplOptions.AggressiveInlining)] @@ -393,11 +501,16 @@ internal static unsafe void WriteRaw(IBufferWriter writer, string value, i fixed (char* cPtr = value) { int totalBytes; - if (expectedLength <= MaxQuickEncodeSize) + + // NOTE the second condition is load-bearing, not belt-and-braces: GetSpan's size is a hint, and + // expectedLength is handed to the encoder as the destination capacity. Against a shorter span that + // is a buffer OVERRUN rather than an exception - it writes past the end of someone else's memory. + // When the writer declines, fall through to the encoder loop below, which sizes every write from + // span.Length and so copes with whatever it is given. + if (expectedLength <= MaxQuickEncodeSize && TryGetSpan(writer, expectedLength, out var quick)) { // encode directly in one hit - var span = writer.GetSpan(expectedLength); - fixed (byte* bPtr = &MemoryMarshal.GetReference(span)) + fixed (byte* bPtr = &MemoryMarshal.GetReference(quick)) { totalBytes = Encoding.UTF8.GetBytes( cPtr, @@ -467,19 +580,15 @@ private static void WriteUnifiedPrefixedBlob(IBufferWriter writer, byte[]? } else { - var span = writer.GetSpan(3 + - Format - .MaxInt32TextLen); // note even with 2 max-len, we're still in same text range + // note even with 2 max-len, we're still in same text range + var scratch = new PrefixScratch(writer, 3 + Format.MaxInt32TextLen, stackalloc byte[MaxPrefixScratch]); + var span = scratch.Span; span[0] = (byte)'$'; - int bytes = WriteRaw(span, prefix.LongLength + value.LongLength, offset: 1); - writer.Advance(bytes); + scratch.Commit(WriteRaw(span, prefix.LongLength + value.LongLength, offset: 1)); writer.Write(prefix); writer.Write(value); - - span = writer.GetSpan(2); - WriteCrlf(span, 0); - writer.Advance(2); + WriteCrlf(writer); } } @@ -490,11 +599,10 @@ private static void WriteUnifiedInt64(IBufferWriter writer, long value) // ${asc-len}\r\n = 4/5 (asc-len at most 2 digits) // {asc}\r\n = MaxInt64TextLen + 2 - var span = writer.GetSpan(7 + Format.MaxInt64TextLen); - + var scratch = new PrefixScratch(writer, 7 + Format.MaxInt64TextLen, stackalloc byte[MaxPrefixScratch]); + var span = scratch.Span; span[0] = (byte)'$'; - var bytes = WriteRaw(span, value, withLengthPrefix: true, offset: 1); - writer.Advance(bytes); + scratch.Commit(WriteRaw(span, value, withLengthPrefix: true, offset: 1)); } private static void WriteUnifiedUInt64(IBufferWriter writer, ulong value) @@ -506,13 +614,13 @@ private static void WriteUnifiedUInt64(IBufferWriter writer, ulong value) var len = Format.FormatUInt64(value, valueSpan); // ${asc-len}\r\n = 4/5 (asc-len at most 2 digits) // {asc}\r\n = {len} + 2 - var span = writer.GetSpan(7 + len); + var scratch = new PrefixScratch(writer, 7 + len, stackalloc byte[MaxValueScratch]); + var span = scratch.Span; span[0] = (byte)'$'; int offset = WriteRaw(span, len, withLengthPrefix: false, offset: 1); valueSpan.Slice(0, len).CopyTo(span.Slice(offset)); offset += len; - offset = WriteCrlf(span, offset); - writer.Advance(offset); + scratch.Commit(WriteCrlf(span, offset)); } private static void WriteUnifiedDouble(IBufferWriter writer, double value) @@ -523,13 +631,13 @@ private static void WriteUnifiedDouble(IBufferWriter writer, double value) // ${asc-len}\r\n = 4/5 (asc-len at most 2 digits) // {asc}\r\n = {len} + 2 - var span = writer.GetSpan(7 + len); + var scratch = new PrefixScratch(writer, 7 + len, stackalloc byte[MaxValueScratch]); + var span = scratch.Span; span[0] = (byte)'$'; int offset = WriteRaw(span, len, withLengthPrefix: false, offset: 1); valueSpan.Slice(0, len).CopyTo(span.Slice(offset)); offset += len; - offset = WriteCrlf(span, offset); - writer.Advance(offset); + scratch.Commit(WriteCrlf(span, offset)); #else // fallback: drop to string WriteUnifiedPrefixedString(writer, null, Format.ToString(value)); @@ -540,11 +648,10 @@ internal static void WriteInteger(IBufferWriter writer, long value) { // note: client should never write integer; only server does this // :{asc}\r\n = MaxInt64TextLen + 3 - var span = writer.GetSpan(3 + Format.MaxInt64TextLen); - + var scratch = new PrefixScratch(writer, 3 + Format.MaxInt64TextLen, stackalloc byte[MaxValueScratch]); + var span = scratch.Span; span[0] = (byte)':'; - var bytes = WriteRaw(span, value, withLengthPrefix: false, offset: 1); - writer.Advance(bytes); + scratch.Commit(WriteRaw(span, value, withLengthPrefix: false, offset: 1)); } private static void WriteUnifiedBlob(IBufferWriter writer, byte[]? value) @@ -570,20 +677,20 @@ private static void WriteUnifiedSpan(IBufferWriter writer, ReadOnlySpan writer, in ReadOnly private static void WriteUnifiedSequenceIterator(IBufferWriter writer, ReadOnlySequenceSegmentIterator seq) { - var span = writer.GetSpan(3 + Format.MaxInt32TextLen); + var scratch = new PrefixScratch(writer, 3 + Format.MaxInt32TextLen, stackalloc byte[MaxPrefixScratch]); + var span = scratch.Span; span[0] = (byte)'$'; - int bytes = WriteRaw(span, seq.Length, offset: 1); - writer.Advance(bytes); + scratch.Commit(WriteRaw(span, seq.Length, offset: 1)); while (seq.TryNext(out var memory)) { @@ -650,7 +757,8 @@ internal void WriteSha1AsHex(byte[]? value) { // $40\r\n = 5 // {40 bytes}\r\n = 42 - var span = writer.GetSpan(47); + var scratch = new PrefixScratch(writer, Sha1BulkStringLength, stackalloc byte[Sha1BulkStringLength]); + var span = scratch.Span; span[0] = (byte)'$'; span[1] = (byte)'4'; span[2] = (byte)'0'; @@ -668,7 +776,7 @@ internal void WriteSha1AsHex(byte[]? value) span[offset++] = (byte)'\r'; span[offset++] = (byte)'\n'; - writer.Advance(offset); + scratch.Commit(offset); } else { diff --git a/tests/StackExchange.Redis.Benchmarks/MessageWriterBenchmarks.cs b/tests/StackExchange.Redis.Benchmarks/MessageWriterBenchmarks.cs new file mode 100644 index 000000000..3dabebe95 --- /dev/null +++ b/tests/StackExchange.Redis.Benchmarks/MessageWriterBenchmarks.cs @@ -0,0 +1,73 @@ +using System; +using System.Buffers; +using BenchmarkDotNet.Attributes; +using BenchmarkDotNet.Configs; + +namespace StackExchange.Redis.Benchmarks; + +// Formatting only - no server. Exists to check that verifying GetSpan's size hint did not cost anything on +// the path that matters: the writer's own span is still used directly whenever it is long enough, so the +// added work should be one length comparison per burst plus a stackalloc that SkipLocalsInit makes free. +// +// Run this on both sides of the change; the numbers are only meaningful as a before/after pair. +[Config(typeof(CustomConfig))] +[MemoryDiagnoser] +[GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory)] +[CategoriesColumn] +public class MessageWriterBenchmarks +{ + private readonly RedisKey _key = "user:1"; + private readonly RedisValue _short = "marc"; + private RedisValue _long; + private RedisValue[] _several = null!; + private readonly Reusable _target = new(); + + [GlobalSetup] + public void Setup() + { + _long = new string('x', 4096); + _several = ["marc", "EX", 300, 1.5, long.MinValue]; + } + + private MessageWriter Writer => new(null, CommandMap.Default, _target); + + [BenchmarkCategory("KeyValue"), Benchmark] + public int KeyValue() + { + _target.Reset(); + Message.Create(0, CommandFlags.None, RedisCommand.SET, _key, _short).WriteTo(Writer); + return _target.Written; + } + + [BenchmarkCategory("Mixed"), Benchmark] + public int MixedValueShapes() + { + _target.Reset(); + Message.Create(0, CommandFlags.None, RedisCommand.SET, _key, _several).WriteTo(Writer); + return _target.Written; + } + + [BenchmarkCategory("Large"), Benchmark] + public int LargeValue() + { + _target.Reset(); + Message.Create(0, CommandFlags.None, RedisCommand.SET, _key, _long).WriteTo(Writer); + return _target.Written; + } + + /// A reusable buffer writer, so buffer management is not part of the measurement. + private sealed class Reusable : IBufferWriter + { + private readonly byte[] _buffer = new byte[16 * 1024]; + + public int Written { get; private set; } + + public void Reset() => Written = 0; + + public void Advance(int count) => Written += count; + + public Memory GetMemory(int sizeHint = 0) => new(_buffer, Written, _buffer.Length - Written); + + public Span GetSpan(int sizeHint = 0) => new(_buffer, Written, _buffer.Length - Written); + } +} diff --git a/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs b/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs new file mode 100644 index 000000000..2e0dc8882 --- /dev/null +++ b/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs @@ -0,0 +1,149 @@ +using System; +using System.Buffers; +using System.Collections.Generic; +using System.Text; +using Xunit; + +namespace StackExchange.Redis.Tests; + +/// +/// Writing must not assume honoured the size hint. +/// +/// +/// +/// The hint is a hint. CycleBuffer.GetUncommittedMemory caps it at 1k, sizes fresh segments from the +/// committed total rather than the hint, and - the case with no floor at all - hands back a dangling recycled +/// segment as-is, whatever length that happens to be. So a writer may legitimately return less than asked. +/// +/// +/// These use a writer that returns deliberately short spans, which is what the buffer does intermittently +/// under concurrency. Reported as ArgumentOutOfRangeException from MessageWriter.WriteUnifiedSpan. +/// +/// +public class BufferWriterHintTests +{ + /// + /// An that honours at most bytes per span, and + /// detects writes that run past the span it handed out. + /// + /// + /// The span comes from an isolated scratch array with a canary beyond it, checked on every Advance. A + /// plain growing buffer cannot catch this: an overrun would land harmlessly in the slack that follows, + /// and the test would pass while the real writer corrupts whatever is next in the pool. + /// + private sealed class StingyWriter(int max) : IBufferWriter + { + private const int Canary = 64; + private const byte CanaryByte = 0xCD; + + /// A generous but finite ceiling, so the "unconstrained" baseline stays allocatable. + private const int Unconstrained = 64 * 1024; + + private readonly int _max = Math.Min(max, Unconstrained); + private byte[] _scratch = []; + private int _handedOut; + private readonly List _written = []; + + public ReadOnlySpan Written => _written.ToArray(); + + public void Advance(int count) + { + if (count < 0 || count > _handedOut) + { + throw new InvalidOperationException($"Advance({count}) but only {_handedOut} was handed out"); + } + + for (var i = _handedOut; i < _scratch.Length; i++) + { + if (_scratch[i] != CanaryByte) + { + throw new InvalidOperationException( + $"buffer overrun: {i - _handedOut + 1} byte(s) written past a span of {_handedOut}"); + } + } + + for (var i = 0; i < count; i++) _written.Add(_scratch[i]); + _handedOut = 0; + } + + public Memory GetMemory(int sizeHint = 0) => Hand(sizeHint).AsMemory(0, _handedOut); + + public Span GetSpan(int sizeHint = 0) => Hand(sizeHint).AsSpan(0, _handedOut); + + private byte[] Hand(int sizeHint) + { + _handedOut = Math.Max(1, Math.Min(_max, sizeHint <= 0 ? _max : sizeHint)); + _scratch = new byte[_handedOut + Canary]; + _scratch.AsSpan(_handedOut).Fill(CanaryByte); + return _scratch; + } + } + + /// + /// Built directly rather than through TestHarness.Write, which pins the database to -1 and so + /// cannot issue the key-bearing commands this needs. + /// + private static string Render(int max, string command, params object[] args) + { + var writer = new StingyWriter(max); + var message = new RedisDatabase.ExecuteMessage(CommandMap.Default, 0, CommandFlags.None, command, args); + message.WriteTo(new MessageWriter(null, CommandMap.Default, writer)); + return Encoding.UTF8.GetString(writer.Written.ToArray()).Replace("\r\n", "|"); + } + + [Theory] + // 8 is below every fixed-size hint in the writer; 16 and 64 straddle the length-prefix sites; 520 is + // just under the 5 + MaxInt32TextLen + 512 that the quick-span path asks for + [InlineData(8)] + [InlineData(16)] + [InlineData(64)] + [InlineData(520)] + public void ShortSpansStillProduceCorrectFrames(int max) + { + var expected = Render(64 * 1024, "SET", "mykey", "myvalue"); + Assert.Equal("*3|$3|SET|$5|mykey|$7|myvalue|", expected); + Assert.Equal(expected, Render(max, "SET", "mykey", "myvalue")); + } + + [Theory] + [InlineData(8, 500)] + [InlineData(8, 512)] + [InlineData(8, 513)] + [InlineData(520, 512)] // the reported shape: payloads straddling the 512 boundary + [InlineData(520, 5000)] + public void ShortSpansStillProduceCorrectFramesForLargeValues(int max, int payload) + { + var value = new string('x', payload); + var expected = Render(64 * 1024, "SET", "mykey", value); + Assert.Equal(expected, Render(max, "SET", "mykey", value)); + } + + [Theory] + [InlineData(8)] + [InlineData(16)] + [InlineData(40)] + public void ShortSpansStillProduceCorrectFramesForEveryValueShape(int max) + { + // one of each writer path: int64, uint64-range, double, byte[], empty, and a long string + object[] args = + [ + "mykey", + 1234567890, + long.MinValue, + ulong.MaxValue, + 1.5d, + new byte[] { 1, 2, 3, 250 }, + "", + new string('x', 600), + ]; + + Assert.Equal(Render(64 * 1024, "SET", args), Render(max, "SET", args)); + } + + [Fact] + public void TheBaselineIsItselfCorrect() + { + // guards against both sides being equally wrong + Assert.Equal("*4|$3|SET|$5|mykey|$3|1.5|$2|-1|", Render(64 * 1024, "SET", "mykey", 1.5d, -1)); + } +} From f95d20743081e7f04fd64e27664f340c39514e73 Mon Sep 17 00:00:00 2001 From: mgravell Date: Sun, 13 Sep 2026 09:27:26 +0100 Subject: [PATCH 2/7] 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. --- src/StackExchange.Redis/MessageWriter.cs | 176 ++++++++++++------ .../BufferWriterHintTests.cs | 123 +++++++++++- 2 files changed, 234 insertions(+), 65 deletions(-) diff --git a/src/StackExchange.Redis/MessageWriter.cs b/src/StackExchange.Redis/MessageWriter.cs index be3a7782b..298180549 100644 --- a/src/StackExchange.Redis/MessageWriter.cs +++ b/src/StackExchange.Redis/MessageWriter.cs @@ -164,7 +164,9 @@ internal void WriteHeader(RedisCommand command, int arguments) // *{argCount}\r\n = 3 + MaxInt32TextLen // ${cmd-len}\r\n = precomputed // {cmd}\r\n = precomputed - if (TryGetSpan(_writer, commandBytes.Length + 3 + Format.MaxInt32TextLen, out var span)) + var needed = commandBytes.Length + 3 + Format.MaxInt32TextLen; + var span = _writer.GetSpan(needed); + if (span.Length >= needed) { span[0] = (byte)'*'; int offset = WriteRaw(span, arguments + 1, offset: 1); @@ -173,11 +175,7 @@ internal void WriteHeader(RedisCommand command, int arguments) } else { - // the command bytes are already framed, so they can go straight through the looping write - Span scratch = stackalloc byte[MaxPrefixScratch]; - scratch[0] = (byte)'*'; - _writer.Write(scratch.Slice(0, WriteRaw(scratch, arguments + 1, offset: 1))); - _writer.Write(commandBytes); + WriteHeaderSlow(_writer, arguments, commandBytes); } } @@ -193,7 +191,9 @@ internal void WriteHeader(RedisCommand command, int arguments, ReadOnlySpan= needed) { span[0] = (byte)'*'; int offset = WriteRaw(span, arguments + 1, offset: 1); @@ -202,32 +202,17 @@ internal void WriteHeader(RedisCommand command, int arguments, ReadOnlySpan scratch = stackalloc byte[MaxPrefixScratch]; - scratch[0] = (byte)'*'; - int offset = WriteRaw(scratch, arguments + 1, offset: 1); - scratch[offset++] = (byte)'$'; - offset = WriteRaw(scratch, commandBytes.Length, offset: offset); - _writer.Write(scratch.Slice(0, offset)); - _writer.Write(commandBytes); - WriteCrlf(_writer); + WriteHeaderUnframedSlow(_writer, arguments, commandBytes); } } internal static void WriteMultiBulkHeader(IBufferWriter writer, long count) { - // *{count}\r\n = 3 + MaxInt32TextLen - var scratch = new PrefixScratch(writer, 3 + Format.MaxInt32TextLen, stackalloc byte[MaxPrefixScratch]); - var span = scratch.Span; - span[0] = (byte)'*'; - scratch.Commit(WriteRaw(span, count, offset: 1)); + WriteCountPrefix(writer, (byte)'*', count); } internal static void WriteMultiBulkHeader(IBufferWriter writer, long count, RespPrefix prefix) { - // *{count}\r\n = 3 + MaxInt32TextLen - var scratch = new PrefixScratch(writer, 3 + Format.MaxInt32TextLen, stackalloc byte[MaxPrefixScratch]); - var span = scratch.Span; - span[0] = (byte)prefix; if ((prefix is RespPrefix.Map or RespPrefix.Attribute) & count > 0) { if ((count & 1) != 0) Throw(prefix, count); @@ -236,7 +221,7 @@ internal static void WriteMultiBulkHeader(IBufferWriter writer, long count paramName: nameof(count), message: $"{type} data must be in pairs; got {count}"); } - scratch.Commit(WriteRaw(span, count, offset: 1)); + WriteCountPrefix(writer, (byte)prefix, count); } /// @@ -245,6 +230,77 @@ internal static void WriteMultiBulkHeader(IBufferWriter writer, long count /// private const int MaxPrefixScratch = 8 + Format.MaxInt32TextLen + Format.MaxInt32TextLen; + /// + /// Write a {prefix}{count}\r\n header - the * of a multi-bulk and the $ of a bulk + /// string are the same shape. + /// + /// + /// Sized for an int64: , + /// WriteUnifiedPrefixedBlob and WriteUnifiedSequenceIterator all format a long, and + /// previously asked for int32 width. Unreachable in practice - it needs a value above int.MaxValue + /// - but the direct path could be handed exactly the 14 bytes it asked for and then write up to 23. + /// + private static void WriteCountPrefix(IBufferWriter writer, byte prefix, long count) + { + const int Needed = 3 + Format.MaxInt64TextLen; + var span = writer.GetSpan(Needed); + if (span.Length >= Needed) + { + span[0] = prefix; + writer.Advance(WriteRaw(span, count, offset: 1)); + } + else + { + WriteCountPrefixSlow(writer, prefix, count); + } + } + + /// + /// The fallback when the writer declined the hint: compose in a stack buffer and let the looping write + /// place it. + /// + /// + /// Deliberately separate and not inlined. A stackalloc in the caller's cold branch changes + /// codegen for the whole method: written inline, these fallbacks cost ~20% on the simplest command shape. + /// It is the same "inline-optimized for it-fits, pathological case doesn't inline" split the BCL uses, and + /// that describes. + /// + [MethodImpl(MethodImplOptions.NoInlining)] + private static void WriteCountPrefixSlow(IBufferWriter writer, byte prefix, long count) + { + Span scratch = stackalloc byte[MaxValueScratch]; + scratch[0] = prefix; + writer.Write(scratch.Slice(0, WriteRaw(scratch, count, offset: 1))); + } + + /// + [MethodImpl(MethodImplOptions.NoInlining)] + private static void WriteHeaderSlow(IBufferWriter writer, int arguments, ReadOnlySpan commandBytes) + { + // the command bytes are already framed, so they can go straight through the looping write + WriteCountPrefixSlow(writer, (byte)'*', arguments + 1); + writer.Write(commandBytes); + } + + /// + [MethodImpl(MethodImplOptions.NoInlining)] + private static void WriteHeaderUnframedSlow(IBufferWriter writer, int arguments, ReadOnlySpan commandBytes) + { + WriteCountPrefixSlow(writer, (byte)'*', arguments + 1); + WriteCountPrefixSlow(writer, (byte)'$', commandBytes.Length); + writer.Write(commandBytes); + WriteCrlf(writer); + } + + /// + [MethodImpl(MethodImplOptions.NoInlining)] + private static void WriteUnifiedSpanSlow(IBufferWriter writer, ReadOnlySpan value) + { + WriteCountPrefixSlow(writer, (byte)'$', value.Length); + writer.Write(value); + WriteCrlf(writer); + } + /// A SHA1 hash as a bulk string: $40\r\n plus 40 hex characters plus CRLF. private const int Sha1BulkStringLength = 47; @@ -302,16 +358,19 @@ private readonly ref struct PrefixScratch public PrefixScratch(IBufferWriter writer, int length, Span fallback) { - Debug.Assert(fallback.Length >= length, "fallback too small for the requested burst"); _writer = writer; + + // both are sliced to exactly length: the slice costs nothing (every write is bounds-checked + // anyway) but it holds in Release, where a Debug.Assert does not. This is the PR's own thesis + // applied to its own buffer - do not trust that it is as long as you think. if (TryGetSpan(writer, length, out var span)) { - _span = span; + _span = span.Slice(0, length); _direct = true; } else { - _span = fallback; + _span = fallback.Slice(0, length); _direct = false; } } @@ -358,10 +417,7 @@ internal static void WriteUnifiedPrefixedString(IBufferWriter writer, byte } else { - var scratch = new PrefixScratch(writer, 3 + Format.MaxInt32TextLen, stackalloc byte[MaxPrefixScratch]); - var span = scratch.Span; - span[0] = (byte)'$'; - scratch.Commit(WriteRaw(span, totalLength, offset: 1)); + WriteCountPrefix(writer, (byte)'$', totalLength); if (prefixLength != 0) writer.Write(prefix); if (encodedLength != 0) WriteRaw(writer, value, encodedLength); @@ -381,7 +437,20 @@ internal static int WriteCrlf(Span span, int offset) [MethodImpl(MethodImplOptions.AggressiveInlining)] internal static void WriteCrlf(IBufferWriter writer) { - writer.Write("\r\n"u8); + // NOTE the hint stays at 2. BuffersExtensions.Write asks with no hint at all, and CycleBuffer's + // "hint <= 0" branch returns anything non-empty - so a 1-byte segment tail would split the CRLF + // across segments rather than rolling to a fresh one. Protocol-correct either way, but it is a + // fragmentation change on a per-bulk-string path, and not one to make by accident. + if (TryGetSpan(writer, 2, out var span)) + { + span[0] = (byte)'\r'; + span[1] = (byte)'\n'; + writer.Advance(2); + } + else + { + writer.Write("\r\n"u8); // loops; a writer this stingy is pathological, but legal + } } [MethodImpl(MethodImplOptions.AggressiveInlining)] @@ -580,11 +649,7 @@ private static void WriteUnifiedPrefixedBlob(IBufferWriter writer, byte[]? } else { - // note even with 2 max-len, we're still in same text range - var scratch = new PrefixScratch(writer, 3 + Format.MaxInt32TextLen, stackalloc byte[MaxPrefixScratch]); - var span = scratch.Span; - span[0] = (byte)'$'; - scratch.Commit(WriteRaw(span, prefix.LongLength + value.LongLength, offset: 1)); + WriteCountPrefix(writer, (byte)'$', prefix.LongLength + value.LongLength); writer.Write(prefix); writer.Write(value); @@ -599,7 +664,7 @@ private static void WriteUnifiedInt64(IBufferWriter writer, long value) // ${asc-len}\r\n = 4/5 (asc-len at most 2 digits) // {asc}\r\n = MaxInt64TextLen + 2 - var scratch = new PrefixScratch(writer, 7 + Format.MaxInt64TextLen, stackalloc byte[MaxPrefixScratch]); + var scratch = new PrefixScratch(writer, 7 + Format.MaxInt64TextLen, stackalloc byte[MaxValueScratch]); var span = scratch.Span; span[0] = (byte)'$'; scratch.Commit(WriteRaw(span, value, withLengthPrefix: true, offset: 1)); @@ -677,24 +742,20 @@ private static void WriteUnifiedSpan(IBufferWriter writer, ReadOnlySpan quick; + if (value.Length <= MaxQuickSpanSize && (quick = writer.GetSpan(needed)).Length >= needed) + { + quick[0] = (byte)'$'; + writer.Advance(AppendToSpan(quick, value, 1)); + } + else + { + // too big for one span, or the writer declined the hint + WriteUnifiedSpanSlow(writer, value); + } } } @@ -725,10 +786,7 @@ private static void WriteUnifiedSequence(IBufferWriter writer, in ReadOnly private static void WriteUnifiedSequenceIterator(IBufferWriter writer, ReadOnlySequenceSegmentIterator seq) { - var scratch = new PrefixScratch(writer, 3 + Format.MaxInt32TextLen, stackalloc byte[MaxPrefixScratch]); - var span = scratch.Span; - span[0] = (byte)'$'; - scratch.Commit(WriteRaw(span, seq.Length, offset: 1)); + WriteCountPrefix(writer, (byte)'$', seq.Length); while (seq.TryNext(out var memory)) { diff --git a/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs b/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs index 2e0dc8882..9666ff3ba 100644 --- a/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs +++ b/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs @@ -2,6 +2,7 @@ using System.Buffers; using System.Collections.Generic; using System.Text; +using RESPite.Messages; using Xunit; namespace StackExchange.Redis.Tests; @@ -44,7 +45,15 @@ private sealed class StingyWriter(int max) : IBufferWriter private int _handedOut; private readonly List _written = []; - public ReadOnlySpan Written => _written.ToArray(); + /// Everything advanced so far, with the outstanding span checked first. + public byte[] Written + { + get + { + CheckCanary(); + return _written.ToArray(); + } + } public void Advance(int count) { @@ -53,17 +62,33 @@ public void Advance(int count) throw new InvalidOperationException($"Advance({count}) but only {_handedOut} was handed out"); } + CheckCanary(); + for (var i = 0; i < count; i++) _written.Add(_scratch[i]); + + // retire the scratch rather than just zeroing the count: what is left in it is written data, and + // a later canary check would otherwise read that as corruption + _scratch = []; + _handedOut = 0; + } + + /// + /// Verify nothing was written past the span we handed out. + /// + /// + /// Called on Advance, on the next hand-out, and on reading the result - not just on Advance, because + /// a span written past and then abandoned without advancing would otherwise slip through, and this + /// harness is the load-bearing part of these tests. + /// + private void CheckCanary() + { for (var i = _handedOut; i < _scratch.Length; i++) { if (_scratch[i] != CanaryByte) { throw new InvalidOperationException( - $"buffer overrun: {i - _handedOut + 1} byte(s) written past a span of {_handedOut}"); + $"buffer overrun: first byte past a span of {_handedOut} is at +{i - _handedOut}"); } } - - for (var i = 0; i < count; i++) _written.Add(_scratch[i]); - _handedOut = 0; } public Memory GetMemory(int sizeHint = 0) => Hand(sizeHint).AsMemory(0, _handedOut); @@ -72,6 +97,8 @@ public void Advance(int count) private byte[] Hand(int sizeHint) { + CheckCanary(); // the previous span, if it was abandoned rather than advanced + _handedOut = Math.Max(1, Math.Min(_max, sizeHint <= 0 ? _max : sizeHint)); _scratch = new byte[_handedOut + Canary]; _scratch.AsSpan(_handedOut).Fill(CanaryByte); @@ -88,7 +115,7 @@ private static string Render(int max, string command, params object[] args) var writer = new StingyWriter(max); var message = new RedisDatabase.ExecuteMessage(CommandMap.Default, 0, CommandFlags.None, command, args); message.WriteTo(new MessageWriter(null, CommandMap.Default, writer)); - return Encoding.UTF8.GetString(writer.Written.ToArray()).Replace("\r\n", "|"); + return Encoding.UTF8.GetString(writer.Written).Replace("\r\n", "|"); } [Theory] @@ -146,4 +173,88 @@ public void TheBaselineIsItselfCorrect() // guards against both sides being equally wrong Assert.Equal("*4|$3|SET|$5|mykey|$3|1.5|$2|-1|", Render(64 * 1024, "SET", "mykey", 1.5d, -1)); } + + // ---- paths the message shapes above do not reach ------------------------------------------------ + // Called directly: these are internal, and routing to them through a message would pin the test to + // whichever command happens to use them today. + + private static string RenderDirect(int max, Action> write) + { + var writer = new StingyWriter(max); + write(writer); + return Encoding.UTF8.GetString(writer.Written).Replace("\r\n", "|"); + } + + [Theory] + [InlineData(8)] + [InlineData(46)] // one short of the 47 this path needs, so the fallback is forced + [InlineData(47)] + public void Sha1AsHexSurvivesShortSpans(int max) + { + // the tightest fit in the writer: 47 requested, exactly 47 written, no slack at all + var hash = new byte[20]; + for (var i = 0; i < hash.Length; i++) hash[i] = (byte)(i * 11); + + static Action> Write(byte[] hash) + => w => new MessageWriter(null, CommandMap.Default, w).WriteSha1AsHex(hash); + + var expected = RenderDirect(64 * 1024, Write(hash)); + Assert.Equal(42, expected.Replace("|", "\r\n").Length - 5); // $40 CRLF + 40 hex + CRLF + Assert.Equal(expected, RenderDirect(max, Write(hash))); + } + + [Theory] + [InlineData(8)] + [InlineData(16)] + public void KeyspacePrefixesSurviveShortSpans(int max) + { + // WithKeyPrefix / channel prefixes: a real user-facing path, and prefixed writes are two-part + var prefix = Encoding.UTF8.GetBytes("tenant7:"); + + static Action> WriteString(byte[] prefix) + => w => MessageWriter.WriteUnifiedPrefixedString(w, prefix, "user:1"); + + Assert.Equal("$14|tenant7:user:1|", RenderDirect(64 * 1024, WriteString(prefix))); + Assert.Equal("$14|tenant7:user:1|", RenderDirect(max, WriteString(prefix))); + } + + [Theory] + [InlineData(8)] + [InlineData(16)] + public void MultiBulkHeaderWithPrefixSurvivesShortSpans(int max) + { + static Action> Write() + => w => MessageWriter.WriteMultiBulkHeader(w, 4, RespPrefix.Map); + + Assert.Equal("%2|", RenderDirect(64 * 1024, Write())); + Assert.Equal("%2|", RenderDirect(max, Write())); + } + + [Theory] + [InlineData(8)] + [InlineData(16)] + public void IntegerSurvivesShortSpans(int max) + { + // server-side only (toys/StackExchange.Redis.Server), but it is the same shape + static Action> Write() + => w => MessageWriter.WriteInteger(w, long.MinValue); + + Assert.Equal(":-9223372036854775808|", RenderDirect(64 * 1024, Write())); + Assert.Equal(":-9223372036854775808|", RenderDirect(max, Write())); + } + + [Theory] + [InlineData(8)] + [InlineData(16)] + [InlineData(22)] // one short of the int64 width a long count can need + public void CountPrefixIsSizedForALong(int max) + { + // WriteMultiBulkHeader(long), the blob prefix and the sequence iterator all format a long; they + // previously asked for int32 width, which is 14 against a possible 23 + static Action> Write() + => w => MessageWriter.WriteMultiBulkHeader(w, long.MaxValue); + + Assert.Equal("*9223372036854775807|", RenderDirect(64 * 1024, Write())); + Assert.Equal("*9223372036854775807|", RenderDirect(max, Write())); + } } From ea80ec8d6d380085507913530e5b818c58b483d4 Mon Sep 17 00:00:00 2001 From: mgravell Date: Sun, 13 Sep 2026 09:33:32 +0100 Subject: [PATCH 3/7] 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. --- .../MessageWriterBenchmarks.cs | 41 +++++++++++++++++-- 1 file changed, 37 insertions(+), 4 deletions(-) diff --git a/tests/StackExchange.Redis.Benchmarks/MessageWriterBenchmarks.cs b/tests/StackExchange.Redis.Benchmarks/MessageWriterBenchmarks.cs index 3dabebe95..f78d18092 100644 --- a/tests/StackExchange.Redis.Benchmarks/MessageWriterBenchmarks.cs +++ b/tests/StackExchange.Redis.Benchmarks/MessageWriterBenchmarks.cs @@ -5,11 +5,15 @@ namespace StackExchange.Redis.Benchmarks; -// Formatting only - no server. Exists to check that verifying GetSpan's size hint did not cost anything on -// the path that matters: the writer's own span is still used directly whenever it is long enough, so the -// added work should be one length comparison per burst plus a stackalloc that SkipLocalsInit makes free. +// Formatting only - no server. Two writers, because they answer different questions: // -// Run this on both sides of the change; the numbers are only meaningful as a before/after pair. +// Reusable - always returns the remainder of a 16KB buffer, so the hint is always honoured and the +// fallback branch is never entered. This measures what the CHECK costs on the hot path. +// Stingy - never returns more than 64 bytes, so every fixed-size burst takes the fallback. This +// measures what the fallback costs, and does not run at all before the fix (it throws). +// +// Run on both sides of the change; the numbers are only meaningful as a before/after pair, and only the +// Reusable ones can be compared against a build that predates the fix. [Config(typeof(CustomConfig))] [MemoryDiagnoser] [GroupBenchmarksBy(BenchmarkLogicalGroupRule.ByCategory)] @@ -21,6 +25,7 @@ public class MessageWriterBenchmarks private RedisValue _long; private RedisValue[] _several = null!; private readonly Reusable _target = new(); + private readonly Stingy _stingy = new(); [GlobalSetup] public void Setup() @@ -55,6 +60,34 @@ public int LargeValue() return _target.Written; } + [BenchmarkCategory("Fallback"), Benchmark] + public int KeyValue_ShortSpans() + { + _stingy.Reset(); + Message.Create(0, CommandFlags.None, RedisCommand.SET, _key, _short) + .WriteTo(new MessageWriter(null, CommandMap.Default, _stingy)); + return _stingy.Written; + } + + /// Hands out at most 64 bytes at a time, so every fixed-size burst takes the fallback. + private sealed class Stingy : IBufferWriter + { + private const int Max = 64; + private readonly byte[] _buffer = new byte[64 * 1024]; + + public int Written { get; private set; } + + public void Reset() => Written = 0; + + public void Advance(int count) => Written += count; + + public Memory GetMemory(int sizeHint = 0) => new(_buffer, Written, Available); + + public Span GetSpan(int sizeHint = 0) => new(_buffer, Written, Available); + + private int Available => Math.Min(Max, _buffer.Length - Written); + } + /// A reusable buffer writer, so buffer management is not part of the measurement. private sealed class Reusable : IBufferWriter { From d71263415ccc34b67c88033dafd113304f2118c5 Mon Sep 17 00:00:00 2001 From: mgravell Date: Sun, 13 Sep 2026 10:00:11 +0100 Subject: [PATCH 4/7] 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. --- src/StackExchange.Redis/MessageWriter.cs | 7 ++ .../BufferWriterHintTests.cs | 81 ++++++++++++++++++- 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/src/StackExchange.Redis/MessageWriter.cs b/src/StackExchange.Redis/MessageWriter.cs index 298180549..dd691dadd 100644 --- a/src/StackExchange.Redis/MessageWriter.cs +++ b/src/StackExchange.Redis/MessageWriter.cs @@ -328,6 +328,13 @@ private static void WriteUnifiedSpanSlow(IBufferWriter writer, ReadOnlySpa /// dangling recycled segment exactly as it is, whatever length that happens to be. All legitimate; the /// callers were wrong. /// + /// + /// Note that a false result is not side-effect free: GetSpan has already run, and on + /// CycleBuffer that may have trimmed the active segment and raised PageComplete. That is + /// fine - the span we then decline is simply left uncommitted, and the subsequent + /// asks again and fills from wherever the writer is now - but it + /// does mean this must not be used to speculatively "probe" a writer for capacity. + /// /// private static bool TryGetSpan(IBufferWriter writer, int length, out Span span) { diff --git a/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs b/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs index 9666ff3ba..b9bcf8992 100644 --- a/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs +++ b/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs @@ -71,6 +71,11 @@ public void Advance(int count) _handedOut = 0; } + // Two ways this harness is deliberately stricter than a real writer, both of which only matter if a + // caller misbehaves: a partial Advance discards the unadvanced tail rather than keeping it at the + // write position (uncommitted bytes, so nothing legitimate can observe the difference), and a second + // Advance without an intervening hand-out throws rather than silently double-counting. + /// /// Verify nothing was written past the span we handed out. /// @@ -136,7 +141,9 @@ public void ShortSpansStillProduceCorrectFrames(int max) [InlineData(8, 500)] [InlineData(8, 512)] [InlineData(8, 513)] - [InlineData(520, 512)] // the reported shape: payloads straddling the 512 boundary + // NOTE 520 does NOT stress the string path: a 512-byte string asks for exactly 512, which a 520-cap + // writer honours, so these pass on main too. Kept as regression cover, not as evidence. + [InlineData(520, 512)] [InlineData(520, 5000)] public void ShortSpansStillProduceCorrectFramesForLargeValues(int max, int payload) { @@ -145,6 +152,24 @@ public void ShortSpansStillProduceCorrectFramesForLargeValues(int max, int paylo Assert.Equal(expected, Render(max, "SET", "mykey", value)); } + [Theory] + // The reported crash shape: WriteUnifiedSpan with a sizeable BINARY value, which asks for + // 5 + MaxInt32TextLen + length - up to 528 - and previously wrote it unchecked. byte[] takes a + // different route from string, and the value-shapes test only reaches it with four bytes. + [InlineData(8, 400)] + [InlineData(400, 400)] // asks 416, gets 400 + [InlineData(520, 512)] // asks 528, gets 520 - straddles MaxQuickSpanSize with no slack + [InlineData(520, 513)] // one over, so the quick path is skipped and the prefix path runs + [InlineData(64, 4096)] + public void ShortSpansStillProduceCorrectFramesForLargeBinaryValues(int max, int payload) + { + var value = new byte[payload]; + for (var i = 0; i < value.Length; i++) value[i] = (byte)(i % 251); + + var expected = Render(64 * 1024, "SET", "mykey", value); + Assert.Equal(expected, Render(max, "SET", "mykey", value)); + } + [Theory] [InlineData(8)] [InlineData(16)] @@ -249,12 +274,62 @@ static Action> Write() [InlineData(22)] // one short of the int64 width a long count can need public void CountPrefixIsSizedForALong(int max) { - // WriteMultiBulkHeader(long), the blob prefix and the sequence iterator all format a long; they - // previously asked for int32 width, which is 14 against a possible 23 + // DEFENSIVE, not a fix for a reachable bug. WriteMultiBulkHeader takes a long and hints + // 3 + MaxInt32TextLen (14) on main, which is short of the 23 a long can need - but nothing can + // reach it: argument counts come from array lengths, so they are int-bounded in practice. The + // shared WriteCountPrefix here sizes for the parameter type rather than for today's callers, + // and this pins that down so a future long-valued caller cannot reintroduce a short hint. static Action> Write() => w => MessageWriter.WriteMultiBulkHeader(w, long.MaxValue); Assert.Equal("*9223372036854775807|", RenderDirect(64 * 1024, Write())); Assert.Equal("*9223372036854775807|", RenderDirect(max, Write())); } + + // ---- the repo's own writer, with no synthetic writer involved ------------------------------------ + + /// + /// BlockBuffer under-delivers deterministically, so this needs no contrived writer at all. + /// + /// + /// BlockBuffer.GetBuffer clamps the hint to [16, 128] and then hands back whatever is left in the + /// block - its own comment says so: "this isn't an actual max, just a max of what we guarantee; we give + /// the caller whatever is left in the buffer". So any request above 128 is under-served whenever the + /// block has between 128 and 527 bytes remaining, which covers the quick-span path (up to 528) and the + /// string encode (up to 512). No concurrency, no recycled segments, no CycleBuffer - just capacity. + /// + /// writes through MessageWriter.BlockBuffer, so this is the shipped + /// path end to end. + /// + /// + [Theory] + [InlineData(400)] + [InlineData(450)] + [InlineData(500)] + public void BlockBufferUnderDeliversWithoutAnyContrivedWriter(int size) + { + var harness = new TestHarness(); + var value = new string('x', size); + + // several arguments in a row, so the block fills and a later one lands in the 128..527 window + object[] args = [value, value, value, value, value, value]; + + var frame = harness.Write("ECHO", args); + var text = Encoding.UTF8.GetString(frame).Replace("\r\n", "|"); + + Assert.StartsWith("*7|$4|ECHO|", text); + Assert.Equal(6, CountOccurrences(text, "$" + size + "|" + value + "|")); + } + + private static int CountOccurrences(string haystack, string needle) + { + var count = 0; + for (var i = haystack.IndexOf(needle, StringComparison.Ordinal); i >= 0; + i = haystack.IndexOf(needle, i + needle.Length, StringComparison.Ordinal)) + { + count++; + } + + return count; + } } From e93aecdbcbbfdd24eafc2cc4d1c1ce476504cc1a Mon Sep 17 00:00:00 2001 From: mgravell Date: Sun, 13 Sep 2026 10:00:58 +0100 Subject: [PATCH 5/7] 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. --- .../BufferWriterHintTests.cs | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs b/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs index b9bcf8992..f09ab3ca3 100644 --- a/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs +++ b/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs @@ -154,13 +154,18 @@ public void ShortSpansStillProduceCorrectFramesForLargeValues(int max, int paylo [Theory] // The reported crash shape: WriteUnifiedSpan with a sizeable BINARY value, which asks for - // 5 + MaxInt32TextLen + length - up to 528 - and previously wrote it unchecked. byte[] takes a - // different route from string, and the value-shapes test only reaches it with four bytes. - [InlineData(8, 400)] - [InlineData(400, 400)] // asks 416, gets 400 - [InlineData(520, 512)] // asks 528, gets 520 - straddles MaxQuickSpanSize with no slack - [InlineData(520, 513)] // one over, so the quick path is skipped and the prefix path runs - [InlineData(64, 4096)] + // 5 + MaxInt32TextLen + length and previously wrote it unchecked. byte[] takes a different route from + // string, and the value-shapes test only reaches it with four bytes. + // + // The hint carries int32-text slack it rarely uses, so the cap has to be below the ACTUAL frame length + // (1 + digits + 2 + length + 2) to overrun, not merely below the hint. Cases marked (!) fail unfixed; + // the rest are boundary cover that passes either way. + [InlineData(8, 400)] // (!) asks 416, gets 8, writes 408 + [InlineData(400, 400)] // (!) asks 416, gets 400, writes 408 - overruns by 8 + [InlineData(519, 512)] // (!) $512\r\n...\r\n is exactly 520; one byte short + [InlineData(520, 512)] // exactly fits, so the slack in the hint absorbs the shortfall + [InlineData(520, 513)] // over MaxQuickSpanSize, so the looping big-value branch runs + [InlineData(64, 4096)] // likewise, well over public void ShortSpansStillProduceCorrectFramesForLargeBinaryValues(int max, int payload) { var value = new byte[payload]; From dfcb600d9b3268d295294dc51eee1a440ae40a78 Mon Sep 17 00:00:00 2001 From: mgravell Date: Sun, 13 Sep 2026 11:16:52 +0100 Subject: [PATCH 6/7] 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. --- src/StackExchange.Redis/MessageWriter.cs | 39 ++++++++++++------- .../MessageWriterBenchmarks.cs | 12 ++++-- .../BufferWriterHintTests.cs | 6 ++- 3 files changed, 37 insertions(+), 20 deletions(-) diff --git a/src/StackExchange.Redis/MessageWriter.cs b/src/StackExchange.Redis/MessageWriter.cs index dd691dadd..832810af3 100644 --- a/src/StackExchange.Redis/MessageWriter.cs +++ b/src/StackExchange.Redis/MessageWriter.cs @@ -224,12 +224,6 @@ internal static void WriteMultiBulkHeader(IBufferWriter writer, long count WriteCountPrefix(writer, (byte)prefix, count); } - /// - /// The largest fixed-size burst any single write below needs: a prefix byte, two int32 text forms and - /// their CRLFs, or a prefix plus an int64 text form and CRLF - whichever is larger. - /// - private const int MaxPrefixScratch = 8 + Format.MaxInt32TextLen + Format.MaxInt32TextLen; - /// /// Write a {prefix}{count}\r\n header - the * of a multi-bulk and the $ of a bulk /// string are the same shape. @@ -277,8 +271,10 @@ private static void WriteCountPrefixSlow(IBufferWriter writer, byte prefix [MethodImpl(MethodImplOptions.NoInlining)] private static void WriteHeaderSlow(IBufferWriter writer, int arguments, ReadOnlySpan commandBytes) { - // the command bytes are already framed, so they can go straight through the looping write - WriteCountPrefixSlow(writer, (byte)'*', arguments + 1); + // the command bytes are already framed, so they can go straight through the looping write. + // WriteCountPrefix, not …Slow: the ask that was declined was larger than this one, so the writer + // may well be able to take 23 and let the prefix go in place + WriteCountPrefix(writer, (byte)'*', arguments + 1); writer.Write(commandBytes); } @@ -286,17 +282,27 @@ private static void WriteHeaderSlow(IBufferWriter writer, int arguments, R [MethodImpl(MethodImplOptions.NoInlining)] private static void WriteHeaderUnframedSlow(IBufferWriter writer, int arguments, ReadOnlySpan commandBytes) { - WriteCountPrefixSlow(writer, (byte)'*', arguments + 1); - WriteCountPrefixSlow(writer, (byte)'$', commandBytes.Length); + WriteCountPrefix(writer, (byte)'*', arguments + 1); + WriteCountPrefix(writer, (byte)'$', commandBytes.Length); writer.Write(commandBytes); WriteCrlf(writer); } - /// + /// + /// Write a bulk string in pieces rather than as one burst. + /// + /// + /// Unlike the …Slow methods this is not only a fallback: + /// sends every value over MaxQuickSpanSize here because no single span could hold it, so for large + /// binary values this is the normal path. It therefore uses the checked , + /// which writes the length prefix in place when the writer can take 23 bytes; going straight to + /// would compose into a stack buffer and hand it to a hintless + /// , costing a copy and risking a prefix split across segments. + /// [MethodImpl(MethodImplOptions.NoInlining)] - private static void WriteUnifiedSpanSlow(IBufferWriter writer, ReadOnlySpan value) + private static void WriteUnifiedSpanPiecewise(IBufferWriter writer, ReadOnlySpan value) { - WriteCountPrefixSlow(writer, (byte)'$', value.Length); + WriteCountPrefix(writer, (byte)'$', value.Length); writer.Write(value); WriteCrlf(writer); } @@ -304,7 +310,10 @@ private static void WriteUnifiedSpanSlow(IBufferWriter writer, ReadOnlySpa /// A SHA1 hash as a bulk string: $40\r\n plus 40 hex characters plus CRLF. private const int Sha1BulkStringLength = 47; - /// As , but also covering a formatted int64 or double payload. + /// + /// The largest fixed-size burst any single compose-then-write below needs: a prefix byte, a formatted + /// int64 or double payload, and the framing CRLFs - whichever is larger. + /// private const int MaxValueScratch = 7 + Format.MaxDoubleTextLen > 7 + Format.MaxInt64TextLen ? 7 + Format.MaxDoubleTextLen : 7 + Format.MaxInt64TextLen; @@ -761,7 +770,7 @@ private static void WriteUnifiedSpan(IBufferWriter writer, ReadOnlySpanHands out at most 64 bytes at a time, so every fixed-size burst takes the fallback. + /// Hands out at most 8 bytes at a time, so every fixed-size burst takes the fallback. private sealed class Stingy : IBufferWriter { - private const int Max = 64; + private const int Max = 8; private readonly byte[] _buffer = new byte[64 * 1024]; public int Written { get; private set; } diff --git a/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs b/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs index f09ab3ca3..507e9b60a 100644 --- a/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs +++ b/tests/StackExchange.Redis.Tests/BufferWriterHintTests.cs @@ -124,8 +124,10 @@ private static string Render(int max, string command, params object[] args) } [Theory] - // 8 is below every fixed-size hint in the writer; 16 and 64 straddle the length-prefix sites; 520 is - // just under the 5 + MaxInt32TextLen + 512 that the quick-span path asks for + // The largest ask for this shape is 23 (WriteHeader wants framed command bytes + 3 + MaxInt32TextLen; + // WriteCountPrefix wants 3 + MaxInt64TextLen), so: 8 declines everything, 16 declines the 23-byte asks + // only, and 64 declines NOTHING - it is here as a control, not as pressure. 520 likewise for this + // payload; it only bites on the quick-span path, which needs a value to be large. [InlineData(8)] [InlineData(16)] [InlineData(64)] From 668b4b75809a7a5bb2cd45bb02e0dcd96ada84b1 Mon Sep 17 00:00:00 2001 From: mgravell Date: Sun, 13 Sep 2026 13:27:17 +0100 Subject: [PATCH 7/7] 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. --- src/StackExchange.Redis/MessageWriter.cs | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/src/StackExchange.Redis/MessageWriter.cs b/src/StackExchange.Redis/MessageWriter.cs index 832810af3..82870f1a9 100644 --- a/src/StackExchange.Redis/MessageWriter.cs +++ b/src/StackExchange.Redis/MessageWriter.cs @@ -267,7 +267,15 @@ private static void WriteCountPrefixSlow(IBufferWriter writer, byte prefix writer.Write(scratch.Slice(0, WriteRaw(scratch, count, offset: 1))); } - /// + /// + /// The fallback when the writer declined the hint, writing the header in pieces instead of as one burst. + /// + /// + /// Not inlined, for the reason given on . Note that the pieces go + /// through the checked : the ask that was just declined was larger + /// than its 23 bytes, so the writer may well still take those in place. Nothing is composed in a stack + /// buffer here - that only happens if is itself declined in turn. + /// [MethodImpl(MethodImplOptions.NoInlining)] private static void WriteHeaderSlow(IBufferWriter writer, int arguments, ReadOnlySpan commandBytes) { @@ -278,7 +286,7 @@ private static void WriteHeaderSlow(IBufferWriter writer, int arguments, R writer.Write(commandBytes); } - /// + /// [MethodImpl(MethodImplOptions.NoInlining)] private static void WriteHeaderUnframedSlow(IBufferWriter writer, int arguments, ReadOnlySpan commandBytes) { @@ -292,12 +300,20 @@ private static void WriteHeaderUnframedSlow(IBufferWriter writer, int argu /// Write a bulk string in pieces rather than as one burst. /// /// + /// /// Unlike the …Slow methods this is not only a fallback: /// sends every value over MaxQuickSpanSize here because no single span could hold it, so for large /// binary values this is the normal path. It therefore uses the checked , /// which writes the length prefix in place when the writer can take 23 bytes; going straight to /// would compose into a stack buffer and hand it to a hintless /// , costing a copy and risking a prefix split across segments. + /// + /// + /// It is still , but not for the reason given on + /// - there is no stackalloc left in here to perturb the + /// caller's codegen. The reason now is simply that 's hot path is the + /// small-value one, and this body has no business being in that frame. + /// /// [MethodImpl(MethodImplOptions.NoInlining)] private static void WriteUnifiedSpanPiecewise(IBufferWriter writer, ReadOnlySpan value)