Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

As part of this, we use APIs from System.Text.Unicode.Utf8 to encode UTF-8 strings, which increases performance and safety, and reduces duplicate code.

Is this needed to introduce the new APIs?

System.Text.Unicode.Utf8 change introduces a new dependency for System.Reflection.Metadata on .NET Framework that will be an extra work to push through the system. It would be better to avoid bundling the two changes together in a single PR.

As part of this, we use APIs from System.Text.Unicode.Utf8 to encode UTF-8 strings, which increases performance and safety, and reduces duplicate code.

Is this needed to introduce the new APIs?

The other PR is approved and ready to merge #111292. After the merge and rebase this branch against main, those commits will disappear.

The other PR is approved and ready to merge #111292. After the merge and rebase this branch against main, those commits will disappear.

The change that introduces System.Reflection.Metadata dependency on Microsoft.Bcl.Memory won't disappear.

Switched back to the old WriteUTF8 implementation for downlevel frameworks, which was rewritten to use spans and remove unsafe code. On modern .NET we still use System.Text.Unicode.Utf8.

Tests pass locally. This is ready for review.

WriteUTF8 implementation for downlevel frameworks, which was rewritten to use spans and remove unsafe code

What''s the performance regression introduced by this rewrite on .NET Framework?

Our primary interest in removing unsafe code is on latest .NET. It is fine to keep unsafe code for .NET Framework if it is required for decent performance.

I updated the function to use unsafe code and wrote a benchmark to compare it with my initial safe edition. We cannot compare it with the existing unsafe implementation since the functions don't have the same signature and semantics. The numbers look promising so I switched to the unsafe edition:

BenchmarkDotNet v0.14.0, Windows 10 (10.0.19045.5796/22H2/2022Update)
Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
  [Host]     : .NET Framework 4.8.1 (4.8.9310.0), X64 RyuJIT VectorSize=256
  DefaultJob : .NET Framework 4.8.1 (4.8.9310.0), X64 RyuJIT VectorSize=256

// See https://aka.ms/new-console-template for more information
using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;
using System.Runtime.InteropServices;
using System.Text;

BenchmarkRunner.Run<Utf8Bench>();

public class Utf8Bench
{
    public string TestString = null!;

    public byte[] TestBytes = new byte[2048];

    [Params(16, 128)]
    public int N { get; set; }

    [GlobalSetup]
    public void Setup()
    {
        var sb = new StringBuilder();
        for (int i = 0; i < N; i++)
        {
            sb.Append('a');
        }
        for (int i = 0; i < N; i++)
        {
            sb.Append('Θ');
        }
        for (int i = 0; i < N; i++)
        {
            sb.Append("😂");
        }
        TestString = sb.ToString();
        TestBytes = new byte[2048];
    }

    [Benchmark(Baseline = true)]
    public int Safe()
    {
        WriteUtf8Safe(TestString.AsSpan(), TestBytes, out int charsRead, out int bytesWritten, true);
        return charsRead + bytesWritten;
    }

    [Benchmark]
    public int Unsafe()
    {
        WriteUtf8Unsafe(TestString.AsSpan(), TestBytes, out int charsRead, out int bytesWritten, true);
        return charsRead + bytesWritten;
    }

    public static void WriteUtf8Safe(ReadOnlySpan<char> source, Span<byte> destination, out int charsRead, out int bytesWritten, bool allowUnpairedSurrogates)
    {
        // Copy from PR
    }

    public static unsafe void WriteUtf8Unsafe(ReadOnlySpan<char> source, Span<byte> destination, out int charsRead, out int bytesWritten, bool allowUnpairedSurrogates)
    {
        // Copy from PR
    }
}

We are trying to minimize our use of unsafe throughout, so I expect we'd want to avoid bringing that in here. @jkotas @AaronRobinsonMSFT can you advise on this PR for whether we should pursue getting it into approvable state for .NET 11?

We are trying to minimize our use of unsafe throughout, so I expect we'd want to avoid bringing that in here.

The unsafe code is used in .NET Framework polyfills for the most part. There is not much we can do about that without significantly regressing performance of this library on .NET Framework.

These APIs are expected to improve Roslyn performance once Roslyn switches over to use them. Like with any performance related change, I would like to see some numbers that show (1) the refactoring required by these APIs is not regressing Roslyn performance and (2) switching Roslyn to use these APIs is improving Roslyn performance.

@teo-tsirpanis This PR is awaiting action for collecting performance benchmarks and comparing the results.

(1) the refactoring required by these APIs is not regressing Roslyn performance

The most significant refactoring is the change to the UTF-8 encoder implementation. For modern .NET, I expect it to be faster, since it's using inbox APIs that are vectorized. For .NET Framework, if we end up with the rewritten unsafe implementation, I don't expect performance to differ very much, since the implementations are quite similar (https://www.diffchecker.com/VMVUZR1g/). It should also be noted that Roslyn in Visual Studio no longer runs on .NET Framework by default (dotnet/roslyn#77914).

(2) switching Roslyn to use these APIs is improving Roslyn performance

In #100418, @jaredpar said:

Using the [new APIs] I've been able to significantly improve the allocation profile of VBCSCompiler. For building a solution the scale of compilers.slnf (~500 compilation events, large, small and medium projects) I've been able to remove ~200MB of LOH for byte[] and reduce GC pause time by 1.5%.

I would like to see the numbers for the implementation in this PR and including walk clock time.

It sounds like there's an assumption that we need to bring this new API surface to the .NET Framework polyfill. Why is that the case?

It sounds like there's an assumption that we need to bring this new API surface to the .NET Framework polyfill. Why is that the case?

This is a question for Roslyn (@jaredpar). These APIs are designed to optimize Roslyn. If Roslyn is fine with missing these APIs on netfx, we can certainly drop them there.

I would like to see the numbers for the implementation in this PR and including walk clock time.

I wrote a benchmark of BlobBuilder.WriteUTF8 similar to my previous one, comparing between SRM from NuGet and a local build of this branch, running on .NET Framework and .NET 10. I also included writing ASCII-only strings, which is what the overwhelming majority of metadata identifiers are going to be in a compilation.

.NET Framework

BenchmarkDotNet v0.15.8, Windows 10 (10.0.19045.6456/22H2/2022Update)
Intel Core i7-7700HQ CPU 2.80GHz (Max: 2.81GHz) (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
  [Host]     : .NET Framework 4.8.1 (4.8.9310.0), X64 RyuJIT VectorSize=256
  DefaultJob : .NET Framework 4.8.1 (4.8.9310.0), X64 RyuJIT VectorSize=256

.NET 10

BenchmarkDotNet v0.15.8, Windows 10 (10.0.19045.6456/22H2/2022Update)
Intel Core i7-7700HQ CPU 2.80GHz (Max: 2.81GHz) (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET SDK 10.0.102
  [Host] : .NET 10.0.2 (10.0.2, 10.0.225.61305), X64 RyuJIT x86-64-v3

Job=InProcess  Toolchain=InProcessEmitToolchain  

Sources are attached. I manually changed the TFM, the UseStockSrm property, and the path to the local SRM assembly, and concatenated the results into the tables above.