Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use JIT intrinsic for UTF8 encoding in Utf8.TryWrite's AppendLiteral #89376

Merged
merged 2 commits into from Jul 24, 2023

Conversation

stephentoub
Copy link
Member

@EgorBo, the right things don't seem to be happening here, or else I'm misunderstanding how it's supposed to work. I still see a call to ReadUtf8 in the asm.

@ghost
Copy link

ghost commented Jul 24, 2023

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

Issue Details

@EgorBo, the right things don't seem to be happening here, or else I'm misunderstanding how it's supposed to work. I still see a call to ReadUtf8 in the asm.

Author: stephentoub
Assignees: stephentoub
Labels:

area-System.Text.Encoding

Milestone: -

@EgorBo
Copy link
Member

EgorBo commented Jul 24, 2023

@stephentoub do you have a repro? I clonned your branch locally and checked codegen for:

        [MethodImpl(MethodImplOptions.NoInlining)]
        internal static void Egor(ref TryWriteInterpolatedStringHandler h)
        {
            h.AppendLiteral("hello world some long string bla bla bla dewwqedewfwef");
        }

and it looks ok - no ReadUtf8 calls and copying is done using avx2

@stephentoub
Copy link
Member Author

@EgorBo:

private byte[] _buffer = new byte[1000];
private uint _id;

[Benchmark]
public bool TryWrite() => Utf8.TryWrite(_buffer, $"Id: {_id}", out _);

@EgorBo
Copy link
Member

EgorBo commented Jul 24, 2023

@EgorBo:

private byte[] _buffer = new byte[1000];
private uint _id;

[Benchmark]
public bool TryWrite() => Utf8.TryWrite(_buffer, $"Id: {_id}", out _);

Interesting, Checked codegen for TryWrite looks ok:

; Assembly listing for method Program:TryWrite():bool:this (FullOpts)
G_M42047_IG01:  ;; offset=0000H
       push     rdi
       push     rsi
       push     rbx
       sub      rsp, 64
       vxorps   xmm4, xmm4, xmm4
       vmovdqa  xmmword ptr [rsp+20H], xmm4
       vmovdqa  xmmword ptr [rsp+30H], xmm4
       mov      rbx, rcx
G_M42047_IG02:  ;; offset=001AH
       mov      rcx, gword ptr [rbx+08H]
       test     rcx, rcx
       jne      SHORT G_M42047_IG04
G_M42047_IG03:  ;; offset=0023H
       xor      rdx, rdx
       xor      eax, eax
       jmp      SHORT G_M42047_IG05
G_M42047_IG04:  ;; offset=0029H
       lea      rdx, bword ptr [rcx+10H]
       mov      eax, dword ptr [rcx+08H]
G_M42047_IG05:  ;; offset=0030H
       xor      ecx, ecx
       cmp      eax, 4
       setge    cl
       xor      r8, r8
       mov      gword ptr [rsp+20H], r8
G_M42047_IG06:  ;; offset=0040H
       mov      dword ptr [rsp+28H], r8d
       mov      byte  ptr [rsp+2CH], cl
       mov      byte  ptr [rsp+2DH], 0
       mov      bword ptr [rsp+30H], rdx
       mov      dword ptr [rsp+38H], eax
       test     ecx, ecx
       je       SHORT G_M42047_IG12
G_M42047_IG07:  ;; offset=005BH
       mov      ecx, dword ptr [rsp+28H]
       mov      esi, dword ptr [rsp+38H]
       cmp      ecx, esi
       ja       SHORT G_M42047_IG14
       mov      edi, ecx
       add      rdi, bword ptr [rsp+30H]
       sub      esi, ecx
       jns      SHORT G_M42047_IG08
       mov      rdx, 0x1DF80300008      ; ''
       mov      rcx, rdx
       call     [System.Diagnostics.Debug:Fail(System.String,System.String)]
G_M42047_IG08:  ;; offset=0085H
       mov      edx, -1
       cmp      esi, 4
       jl       SHORT G_M42047_IG09
       mov      dword ptr [rdi], 0x203A6449 ;;; <----- "hi: " in utf8
       mov      edx, 4
G_M42047_IG09:  ;; offset=009AH
       test     edx, edx
       jl       SHORT G_M42047_IG10
       add      edx, dword ptr [rsp+28H]
       mov      dword ptr [rsp+28H], edx
       jmp      SHORT G_M42047_IG11
G_M42047_IG10:  ;; offset=00A8H
       mov      byte  ptr [rsp+2CH], 0
       jmp      SHORT G_M42047_IG12
G_M42047_IG11:  ;; offset=00AFH
       mov      edx, dword ptr [rbx+10H]
       lea      rcx, [rsp+20H]
       call     [System.Text.Unicode.Utf8+TryWriteInterpolatedStringHandler:AppendFormatted[uint](uint):bool:this]
G_M42047_IG12:  ;; offset=00BDH
       xor      eax, eax
       cmp      byte  ptr [rsp+2CH], 0
       setne    al
G_M42047_IG13:  ;; offset=00C7H
       add      rsp, 64
       pop      rbx
       pop      rsi
       pop      rdi
       ret      
G_M42047_IG14:  ;; offset=00CFH
       call     [System.ThrowHelper:ThrowArgumentOutOfRangeException()]
       int3     
; Total bytes of code 214

@EgorBo
Copy link
Member

EgorBo commented Jul 24, 2023

Ok, reproduces for me when I run your benchmark in Release using BDN - checking now

@EgorBo
Copy link
Member

EgorBo commented Jul 24, 2023

Interesting, when I run the benchmark with --envvars DOTNET_TieredPGO:0 it works as expected - I no longer see ReadUtf8 call and the result is >2x faster

@EgorBo
Copy link
Member

EgorBo commented Jul 24, 2023

Looks like stale Static PGO problem, --envvars DOTNET_ReadyToRun:0 also helps
image

@EgorBo
Copy link
Member

EgorBo commented Jul 24, 2023

Ah, stop. There is a general problem with PGO 😢

IsKnownConstant is always false in Tier0 because we never inline - it means that Dynamic PGO reports blocks under IsKnownConstant as always cold, some opts don't kick in for such blocks...

@stephentoub
Copy link
Member Author

I can workaround it here by removing the IsKnownConstant, but that seems like something we need to fix in general for 8 :-)

@stephentoub stephentoub requested a review from EgorBo July 24, 2023 13:38
@stephentoub stephentoub merged commit deeeb55 into dotnet:main Jul 24, 2023
165 of 168 checks passed
@stephentoub stephentoub deleted the utf8jit branch July 24, 2023 16:24
@dotnet dotnet locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants