Describe the bug
Context: IBufferWriter<T> and the sizeHint in GetSpan/GetMemory. While the docs mention that the result should be at least this size, this largely relates to the original spec when this was minSize (or similar); in reality, it is not assumed that the sizeHint is always respected, and consuming code typically tests the buffer and applies fallback behaviour. For an example, see BuffersExtensions
The sizeHint as a hint rather than a demand is important for scenarios where a transport has page size limits, and can honour reasonable requests, but not excessive requests; the caller can still ask for what it would like, but typically settles for what it gets. It is also possible for the consumer to ask for minimal sizes and hope that it gets much, much more, but this has performance implications (fragmentation, multiple resize chains, etc). The point here is that the loop is mandatory and the hint policy inside it is the BCL's / provider's business to tune - either policy is correct, whereas demanding one contiguous span is not.
The implementation here is actively hostile; BuffersExtensions is exposed via the System.Memory package so is already available, and does the right thing, specifically: when oversized, it uses a second method (inline-optimized for "it fits", pathological case doesn't inline) that loops copying down in slices.
This means this method achieves nothing useful, and can be actively harmful.
Other issues:
- active overload ambiguity on the
T version if both namespaces in-play (and T is not byte)
- there's a silent overload hijack on the
byte-version, with the broken version taking precedence
- THIS HITS ALL TFMs - it is not specific to down-level and is not gated by the
#if
Recommendations:
- on the
T-to-T version, mark [Obsolete] citing the BuffersExtensions version, redirect the work via BuffersExtensions, and remove the this, making it no-longer an extension method (no runtime API break; build-time API break intentional)
- potentially also tweak the T-to-bytes version to proxy via the same after the type-punning
- fix the
Write<T>(this IBufferWriter<byte> writer, T value) version similarly
- (optional, perf related) possibly add a byte-to-byte version to avoid the hijack via byte-to-T, or add a
if (typeof(T) == typeof(byte)) test internally and let the JIT worry about it; both options still leave the hijack, note, but at least it is a hijack to a "good" version and the JIT may be able to see through the inline; the question is whether to add a new API and let the compiler deal with it, or let the JIT deal with the switch at runtime; either approach still hopes the JIT will inline
(I've audited runtimes targeted by this package; the "correct" version is always available)
Regression
(unchanged behaviour back to Microsoft.Toolkit.HighPerformance 7.1.2)
Steps to reproduce
using System;
using System.Buffers;
using CommunityToolkit.HighPerformance; // <-- delete this line and the first test passes
// Repro: CommunityToolkit.HighPerformance.IBufferWriterExtensions.Write<T>(IBufferWriter<byte>, ReadOnlySpan<T>)
// out-competes System.Buffers.BuffersExtensions.Write<T>(IBufferWriter<T>, ReadOnlySpan<T>) for byte writers
// (concrete receiver beats generic receiver), and it demands the whole payload as a single contiguous
// span instead of looping, so any writer that hands out bounded segments blows up.
//
// net472 (System.Memory 4.6.3), run under mono:
// w.Write(span) [throws on big hint] FAIL OutOfMemoryException, calls: GetSpan(20)
// w.Write(span) [under-delivers ] FAIL ArgumentException, calls: GetSpan(20)
// BuffersExtensions.Write [throws on big hint] FAIL OutOfMemoryException, calls: GetSpan(0) Advance(8) GetSpan(12)
// BuffersExtensions.Write [under-delivers ] OK wrote 20, calls: GetSpan(0) Advance(8) GetSpan(12) Advance(8) GetSpan(4) Advance(4)
// toolkit .Write [throws on big hint] FAIL OutOfMemoryException, calls: GetSpan(20)
// toolkit .Write [under-delivers ] FAIL ArgumentException, calls: GetSpan(20)
//
// i.e. `w.Write(span)` == the toolkit method, never the BCL one. The under-delivering writer is the
// clean discriminator: the BCL loops and completes, the toolkit asks once and dies. (The throwing
// writer also kills the netfx BCL build, because System.Memory 4.6.3's WriteMultiSegment hints the
// remaining length; the current runtime version calls GetSpan() with no hint and survives - on
// net10.0 the two BCL rows are OK with calls: GetSpan(0) Advance(8) x3.)
internal static class Program
{
private static void Main()
{
byte[] payload = new byte[20];
// (demonstrates silent hijack)
// whatever `w.Write(span)` binds to, with `using CommunityToolkit.HighPerformance;` in scope
Run("w.Write(span) ", w => w.Write(new ReadOnlySpan<byte>(payload)));
// the BCL method, called explicitly
Run("BuffersExtensions.Write", w => BuffersExtensions.Write<byte>(w, payload));
// the toolkit method, called explicitly (fully qualified so we can remove the using directive)
Run("toolkit .Write ", w => CommunityToolkit.HighPerformance.IBufferWriterExtensions.Write<byte>(w, new ReadOnlySpan<byte>(payload)));
}
private static void Run(string label, Action<IBufferWriter<byte>> write)
{
foreach (bool throwOnBigHint in new[] { true, false })
{
MalignWriter writer = new MalignWriter(throwOnBigHint);
string mode = throwOnBigHint ? "throws on big hint" : "under-delivers ";
try
{
write(writer);
Console.WriteLine($"{label} [{mode}] OK wrote {writer.Written}, calls: {writer.Calls}");
}
catch (Exception ex)
{
Console.WriteLine($"{label} [{mode}] FAIL {ex.GetType().Name}, calls: {writer.Calls}");
}
}
}
}
// Hands out at most 8 bytes at a time. Per the IBufferWriter<T> docs, GetSpan "can throw if the
// requested buffer size is not available" - so throwOnBigHint:true is a conforming writer, and
// throwOnBigHint:false is the sloppier variant plenty of hand-rolled writers actually implement.
internal sealed class MalignWriter : IBufferWriter<byte>
{
private const int SegmentSize = 8;
private readonly bool throwOnBigHint;
private byte[] current = new byte[SegmentSize];
private int used;
public MalignWriter(bool throwOnBigHint) => this.throwOnBigHint = throwOnBigHint;
public int Written { get; private set; }
public string Calls { get; private set; } = "";
public Span<byte> GetSpan(int sizeHint = 0)
{
Calls += $"GetSpan({sizeHint}) ";
if (sizeHint > SegmentSize && throwOnBigHint)
{
throw new OutOfMemoryException($"cannot supply {sizeHint} contiguous bytes");
}
if (used == current.Length)
{
current = new byte[SegmentSize];
used = 0;
}
// never more than the current segment, whatever was asked for
return new Span<byte>(current, used, current.Length - used);
}
public Memory<byte> GetMemory(int sizeHint = 0) => throw new NotSupportedException();
public void Advance(int count)
{
Calls += $"Advance({count}) ";
used += count;
Written += count;
}
}
Expected behavior
BuffersExtensions is preferred
- writers that return less than
sizeHint from GetSpan/GetMemory still work
Screenshots
No response
IDE and version
Other
IDE version
(not IDE related; all TFMs/runtimes, all builds)
Nuget packages
Nuget package version(s)
8.4.0
Additional context
No response
Help us help you
Yes, I'd like to be assigned to work on this item
Describe the bug
Context:
IBufferWriter<T>and thesizeHintinGetSpan/GetMemory. While the docs mention that the result should be at least this size, this largely relates to the original spec when this wasminSize(or similar); in reality, it is not assumed that thesizeHintis always respected, and consuming code typically tests the buffer and applies fallback behaviour. For an example, seeBuffersExtensionsThe
sizeHintas a hint rather than a demand is important for scenarios where a transport has page size limits, and can honour reasonable requests, but not excessive requests; the caller can still ask for what it would like, but typically settles for what it gets. It is also possible for the consumer to ask for minimal sizes and hope that it gets much, much more, but this has performance implications (fragmentation, multiple resize chains, etc). The point here is that the loop is mandatory and the hint policy inside it is the BCL's / provider's business to tune - either policy is correct, whereas demanding one contiguous span is not.The implementation here is actively hostile;
BuffersExtensionsis exposed via the System.Memory package so is already available, and does the right thing, specifically: when oversized, it uses a second method (inline-optimized for "it fits", pathological case doesn't inline) that loops copying down in slices.This means this method achieves nothing useful, and can be actively harmful.
Other issues:
Tversion if both namespaces in-play (andTis notbyte)byte-version, with the broken version taking precedence#ifRecommendations:
T-to-Tversion, mark[Obsolete]citing theBuffersExtensionsversion, redirect the work viaBuffersExtensions, and remove thethis, making it no-longer an extension method (no runtime API break; build-time API break intentional)Write<T>(this IBufferWriter<byte> writer, T value)version similarlyif (typeof(T) == typeof(byte))test internally and let the JIT worry about it; both options still leave the hijack, note, but at least it is a hijack to a "good" version and the JIT may be able to see through the inline; the question is whether to add a new API and let the compiler deal with it, or let the JIT deal with the switch at runtime; either approach still hopes the JIT will inline(I've audited runtimes targeted by this package; the "correct" version is always available)
Regression
(unchanged behaviour back to Microsoft.Toolkit.HighPerformance 7.1.2)
Steps to reproduce
Expected behavior
BuffersExtensionsis preferredsizeHintfromGetSpan/GetMemorystill workScreenshots
No response
IDE and version
Other
IDE version
(not IDE related; all TFMs/runtimes, all builds)
Nuget packages
Nuget package version(s)
8.4.0
Additional context
No response
Help us help you
Yes, I'd like to be assigned to work on this item