dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.42k stars 4.76k forks source link

Remove unsafe perf quirks from System.Text.Encodings.Web #109896

Closed EgorBo closed 3 days ago

EgorBo commented 5 days ago

Contributes to https://github.com/dotnet/runtime/issues/94941 effort.

Should be ~zero performance difference as the JIT is smart enough to handle the safer equivalent, i.e.:

// New code:
static bool New(Span<byte> destination)
{
    return "&gt;"u8.TryCopyTo(destination);
}

// Old code:
static bool Old(Span<byte> destination)
{
    return TryWriteBytes(destination, (byte)'&', (byte)'g', (byte)'t', (byte)';');
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static bool TryWriteBytes(Span<byte> span, byte a, byte b, byte c, byte d)
{
    if (span.Length >= 4)
    {
        uint abcd32;
        if (BitConverter.IsLittleEndian)
        {
            abcd32 = ((uint)d << 24) | ((uint)c << 16) | ((uint)b << 8) | a;
        }
        else
        {
            abcd32 = ((uint)a << 24) | ((uint)b << 16) | ((uint)c << 8) | d;
        }
        Unsafe.WriteUnaligned<uint>(ref MemoryMarshal.GetReference(span), abcd32);
        return true;
    }
    else
    {
        return false;
    }
}

Codegen for New and Old:

; Method Program:New(System.Span`1[ubyte]):ubyte (FullOpts)
       mov      rax, bword ptr [rcx]
       mov      ecx, dword ptr [rcx+0x08]
       xor      edx, edx
       cmp      ecx, 4
       jl       SHORT G_M2274_IG04
       mov      dword ptr [rax], 0x3B746726
       mov      edx, 1
G_M2274_IG04:  ;; offset=0x0018
       mov      eax, edx
       ret      
; Total bytes of code: 27

; Method Program:Old(System.Span`1[ubyte]):ubyte (FullOpts)
       mov      rax, bword ptr [rcx]
       mov      ecx, dword ptr [rcx+0x08]
       cmp      ecx, 4
       jge      SHORT G_M16025_IG04
       xor      eax, eax
       jmp      SHORT G_M16025_IG05
G_M16025_IG04:
       mov      dword ptr [rax], 0x3B746726
       mov      eax, 1
G_M16025_IG05:
       ret      
; Total bytes of code: 27

Same for writing chars vs "str".TryCopyTo.

PS: SpanUtility.IsValidIndex can also be just removed (propagated at all uses) so we can remove the whole file, but it's out of scope for this PR since IsValidIndex is perfectly safe.

dotnet-policy-service[bot] commented 5 days ago

Tagging subscribers to this area: @dotnet/area-system-text-encodings-web See info in area-owners.md if you want to be subscribed.

EgorBo commented 5 days ago

ugh, .NET Framework 😐

MichalPetryka commented 5 days ago

How does the performance compare with Mono?

EgorBo commented 5 days ago

How does the performance compare with Mono?

This is a part of the global security initiative to improve code safety. We assume that extra nanoseconds in these APIs for mono workloads won't be a big deal. Not sure performance matters for quite old System.Text.Encodings.Web at all, perhaps, only for HtmlEncoder.