IBufferWriterExtensions.Write : actively harmful

Start with CommunityToolkit.HighPerformance/Extensions/IBufferWriterExtensions.cs around the T-to-T and byte overloads, then run the supplied MalignWriter reproduction against the toolkit and BuffersExtensions implementations. Done means the BCL implementation is preferred, bounded or under-delivering writers complete successfully, and the overload ambiguity and silent byte overload hijack are addressed as described.

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

1. BuffersExtensions is preferred
2. 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

• CommunityToolkit.Common
• CommunityToolkit.Diagnostics
• CommunityToolkit.HighPerformance
• CommunityToolkit.Mvvm (aka MVVM Toolkit)

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