Removing unsafe code

[ladeak]

Summary

Methods in StringUtilities.cs could have implementation without 'unsafe' keyword. It would allow HttpUtilities.GetHeaderName to also become safe as .NET 9 supports passing Span<T> (to generic type parameters).

Motivation and goals

Code becomes somewhat safer, no pointer arithmetic required, no 'pinning' by fixed would be required.

In scope

TryGetAsciiString could use Span<T> instead of char* and byte*, and methods using this could also become safe.

Risks / unknowns

It would still require a fair amount of Unsafe.As<> and Vector256.LoadUnsafe, MemoryMarshal.Cast etc. The code is perf sensitive, but I believe the safe code would be still on-par with perf.

Other consideration

Could also consider adding Vector512 support on the way.

[ladeak]

Also very interesting: when I compare the perf of the current implementation to something like on a machine that does not support Avx512, I get equivalent or slightly (0.3ns) better performance when the input string is a valid 10-char value:

private SearchValues<byte> sw = SearchValues.Create([1,2, ... 127]);

public string DotNetAscii(ReadOnlySpan<byte> input)
{
    if (input.ContainsAnyExcept(sw))
        throw new InvalidOperationException();
    return string.Create(input.Length, input, static (buffer, state) =>
    {
        Ascii.ToUtf16(state, buffer, out _);
    });
}

Although this is clearly parsing the input twice.

[BrennanConroy]

👍

I should probably revive https://github.com/dotnet/aspnetcore/compare/main...brecon/utf16 and see if unsafe can be removed as well.

[BrennanConroy]

BTW, we also have https://github.com/dotnet/aspnetcore/issues/4720 But there are probably other areas of the code that could remove unsafe. e.g. https://github.com/dotnet/aspnetcore/blob/main/src/Shared/QueryStringEnumerable.cs#L104

[ladeak]

Is this an area that community members could take a part of and help with the lead of maintainers?

[BrennanConroy]

Sure :) We should list out the areas and see which ones are good to work on. My really quick scan yesterday while reviving my old branch showed PathNormalizer and QueryStringEnumerable as 2 we can look into.

[ladeak]

Let me have a look at one of these suggested files in the coming days.

[ladeak]

@BrennanConroy , I find 2 PathNormalizer types (one in Kestrel, one in Shared). I assume you are referring to the one in Kestrel? (although the code is really similar).

[ladeak]

I will continue with PathNormalizer in the coming days.

[ladeak]

@BrennanConroy do you have other suggestions for converting unsafe code (other than StringUtilities.TryGetAsciiString)?

[BrennanConroy]

other than StringUtilities.TryGetAsciiString

That method doesn't exist anymore 😆

I don't see anything obvious, IIS and HttpSys have a bit of unsafe, but a lot of that probably isn't fixable. There are a couple random ones sprinkled around the repo that might be possible to change https://github.com/search?q=repo%3Adotnet%2Faspnetcore+%22unsafe+%22+-path%3A%2F%5Esrc%5C%2FDataProtection%5C%2F%2F&type=code

[ladeak]

The only thing might be interesting is BufferExtensions

I think this is using unsafe to avoid a few boundary checks upon writing. I will test if it is safe to use the span directly, but I think it should be.