Remove ResolveXHandle indirections #45292

benaadams · 2020-11-28T00:13:53Z

benaadams · 2020-11-28T01:29:54Z

Current method

; Assembly listing for method ModuleHandle:ResolveTypeHandleInternal(RuntimeModule,int,ref,ref):RuntimeType
; Lcl frame size = 248

; Total bytes of code 602, prolog size 55, PerfScore 179.03, instruction count 158 (MethodHash=dc0f0d48) for method ModuleHandle:ResolveTypeHandleInternal(RuntimeModule,int,ref,ref):RuntimeType
; ============================================================

New method

; Assembly listing for method ModuleHandle:ResolveTypeHandleInternal(RuntimeModule,int):RuntimeType
; Lcl frame size = 216

; Total bytes of code 368, prolog size 48, PerfScore 110.13, instruction count 101 (MethodHash=240fc648) for method ModuleHandle:ResolveTypeHandleInternal(RuntimeModule,int):RuntimeType
; ============================================================

GrabYourPitchforks · 2020-11-28T02:34:59Z

The existing code path contains calls to helpers which no-op if given null inputs.

What is the benefit of this change? Does it measurably increase perf for some scenario?

jkotas · 2020-11-28T03:22:47Z

Could you please share numbers for what this is improving?

benaadams · 2020-11-28T03:58:36Z

Just looking into where time is going for FilterCustomAttributeRecord for #45121 and noticed there is a bunch of unneeded work 2 x CopyRuntimeTypeHandles 2 x ChkCastAny, 2 x ConvertToTypeHandleArray; extra 32 bytes of stack cleared etc

However FilterCustomAttributeRecord never calls it with any generic arguments so all this can be skipped

Details

-; Lcl frame size = 248
+; Lcl frame size = 216
 
G_..._IG01:
        push     rbp
        push     r15
        push     r14
...
        push     rdi
        push     rsi
        push     rbx
-       sub      rsp, 248
-       lea      rbp, [rsp+130H]
+       sub      rsp, 216
+       lea      rbp, [rsp+110H]
        xorps    xmm4, xmm4
-       movaps   xmmword ptr [rbp-90H], xmm4
-       movaps   xmmword ptr [rbp-80H], xmm4
        movaps   xmmword ptr [rbp-70H], xmm4
        movaps   xmmword ptr [rbp-60H], xmm4
+       movaps   xmmword ptr [rbp-50H], xmm4
        xor      rax, rax
-       mov      qword ptr [rbp-50H], rax
+       mov      qword ptr [rbp-40H], rax
        mov      gword ptr [rbp+10H], rcx
-       mov      ebx, edx
-       mov      rsi, r8
-       mov      rdi, r9

+       mov      esi, edx
        mov      rcx, gword ptr [rbp+10H]
        call     [ModuleHandle:ValidateModulePointer(RuntimeModule)]
-       lea      rcx, bword ptr [rbp-58H]
+       lea      rcx, bword ptr [rbp-50H]
        mov      rdx, gword ptr [rbp+10H]
        call     [ModuleHandle:GetMetadataImport(RuntimeModule):MetadataImport]
-       mov      rcx, qword ptr [rbp-50H]
-       mov      edx, ebx
+       mov      rcx, qword ptr [rbp-48H]
+       mov      edx, esi
        call     [MetadataImport:_IsValidToken(long,int):bool]
        test     al, al
-       je       G_M62135_IG21
+       je       G_M14775_IG07

-       test     rsi, rsi
-       jne      SHORT G_M62135_IG05

        xor      rcx, rcx
-       jmp      SHORT G_M62135_IG06

-       mov      rcx, rsi
-       call     [Object:MemberwiseClone():Object:this]
-       mov      rcx, rax

-       call     [CORINFO_HELP_READYTORUN_CHKCAST]
-       mov      rsi, rax
-       test     rdi, rdi
-       jne      SHORT G_M62135_IG08

-       xor      rcx, rcx
-       jmp      SHORT G_M62135_IG09

-       mov      rcx, rdi
-       call     [Object:MemberwiseClone():Object:this]
-       mov      rcx, rax

-       call     [CORINFO_HELP_READYTORUN_CHKCAST]
-       mov      rdi, rax
-       lea      rdx, [rbp-40H]
-       mov      gword ptr [rbp+20H], rsi
-       mov      rcx, rsi
-       call     [RuntimeTypeHandle:CopyRuntimeTypeHandles(ref,byref):ref]
-       mov      r14, rax
-       lea      rdx, [rbp-48H]
-       mov      gword ptr [rbp+28H], rdi
-       mov      rcx, rdi
-       call     [RuntimeTypeHandle:CopyRuntimeTypeHandles(ref,byref):ref]
-       mov      gword ptr [rbp-60H], r14
-       test     r14, r14
-       je       SHORT G_M62135_IG11

-       mov      rcx, gword ptr [rbp-60H]
-       cmp      dword ptr [rcx+8], 0
-       jne      SHORT G_M62135_IG12

-       xor      r8, r8
-       jmp      SHORT G_M62135_IG13

-       mov      r8, gword ptr [rbp-60H]
-       cmp      dword ptr [r8+8], 0
-       jbe      G_M62135_IG22
-       mov      r8, gword ptr [rbp-60H]
-       add      r8, 16

-       mov      gword ptr [rbp-68H], rax
-       test     rax, rax
-       je       SHORT G_M62135_IG15

-       mov      rcx, gword ptr [rbp-68H]
-       cmp      dword ptr [rcx+8], 0
-       jne      SHORT G_M62135_IG16

+       mov      gword ptr [rbp-40H], rcx
+       lea      rcx, [rbp+10H]
+       mov      rdx, gword ptr [rbp+10H]
+       mov      rdx, qword ptr [rdx+32]
+       lea      r8, [rbp-40H]
+       lea      r9, bword ptr [rbp-60H]
+       mov      qword ptr [r9], rcx
+       mov      qword ptr [r9+8], rdx
        xor      rcx, rcx
-       jmp      SHORT G_M62135_IG17

-       mov      rcx, gword ptr [rbp-68H]
-       cmp      dword ptr [rcx+8], 0
-       jbe      G_M62135_IG22
-       mov      rcx, gword ptr [rbp-68H]
-       add      rcx, 16

-       xor      rdx, rdx
-       mov      gword ptr [rbp-70H], rdx
-       lea      rdx, [rbp+10H]
-       mov      r9, gword ptr [rbp+10H]
-       mov      r9, qword ptr [r9+32]
-       mov      eax, dword ptr [rbp-40H]
-       mov      r10d, dword ptr [rbp-48H]
-       lea      r11, [rbp-70H]
-       lea      r14, bword ptr [rbp-80H]
-       mov      qword ptr [r14], rdx
-       mov      qword ptr [r14+8], r9
        mov      qword ptr [rsp+20H], rcx
-       mov      dword ptr [rsp+28H], r10d
-       mov      qword ptr [rsp+30H], r11
-       lea      rcx, bword ptr [rbp-80H]
-       mov      bword ptr [rbp-90H], rcx
-       mov      edx, ebx
-       mov      dword ptr [rbp-84H], edx
-       mov      qword ptr [rbp-98H], r8
-       mov      r9d, eax
-       mov      dword ptr [rbp-88H], r9d
-       lea      rcx, [rbp-F0H]

+       mov      dword ptr [rsp+28H], ecx
+       mov      qword ptr [rsp+30H], r8
+       lea      rcx, bword ptr [rbp-60H]
+       mov      bword ptr [rbp-70H], rcx
+       mov      edx, esi
+       mov      dword ptr [rbp-64H], edx
+       xor      r8, r8
+       mov      qword ptr [rbp-78H], r8
+       xor      r9d, r9d
+       mov      dword ptr [rbp-68H], r9d
+       lea      rcx, [rbp-D0H]

        call     [CORINFO_HELP_JIT_PINVOKE_BEGIN]
        mov      rax, qword ptr [(reloc)]
-       mov      rcx, bword ptr [rbp-90H]
-       mov      edx, dword ptr [rbp-84H]
-       mov      r8, qword ptr [rbp-98H]
-       mov      r9d, dword ptr [rbp-88H]
+       mov      rcx, bword ptr [rbp-70H]
+       mov      edx, dword ptr [rbp-64H]
+       mov      r8, qword ptr [rbp-78H]
+       mov      r9d, dword ptr [rbp-68H]

        call     qword ptr [rax]ModuleHandle:ResolveType(QCallModule,int,long,int,long,int,ObjectHandleOnStack)
-       lea      rcx, [rbp-F0H]
+       lea      rcx, [rbp-D0H]
        call     [CORINFO_HELP_JIT_PINVOKE_END]
-       mov      rax, gword ptr [rbp-70H]
+       mov      rax, gword ptr [rbp-40H]

        lea      rsp, [rbp-38H]
        pop      rbx
        pop      rsi

benaadams · 2020-11-28T04:20:36Z

CopyRuntimeTypeHandles, ChkCastAny and ConvertToTypeHandleArray all have early outs but they also don't inline so its 10 additional method calls per call to FilterCustomAttributeRecord

src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeModule.cs

src/coreclr/src/System.Private.CoreLib/src/System/RuntimeHandles.cs

jkotas · 2020-11-28T15:23:10Z

CopyRuntimeTypeHandles, ChkCastAny and ConvertToTypeHandleArray all have early outs but they also don't inline so its 10

Would it better to just add a check around these calls instead of duplicating the implementation?

IntPtr[] typeInstantiationContextHandles = null
int typeInstCount = 0;

if (typeInstantiationContext != null)
{
    typeInstantiationContext = (RuntimeTypeHandle[])typeInstantiationContext.Clone();
    typeInstantiationContextHandles = RuntimeTypeHandle.CopyRuntimeTypeHandles(typeInstantiationContext, out int typeInstCount);
}

Note that this check is sort of there already thanks to ?. operator.

benaadams · 2020-11-28T17:20:09Z

Would it better to just add a check around these calls instead of duplicating the implementation?

Have done this; but it pushes up the register pressure the stack usage for the common non-generic case? Probably not much in the scheme of things

benaadams · 2020-11-28T17:24:26Z

Could do the unification with ResolveMethodHandle also if you prefer moving for less methods?

jkotas · 2020-11-28T18:17:12Z

Yes, it would be nice for consistency

GrabYourPitchforks · 2020-11-28T23:06:26Z

I opened #45302 to track eliding the cast resulting from:

RuntimeTypeHandle[]? handles = (RuntimeTypeHandle[]?)originalHandles?.Clone();

benaadams · 2020-11-29T00:21:19Z

Yes, it would be nice for consistency

Did ResolveType, ResolveMethod, ResolveField

src/coreclr/src/System.Private.CoreLib/src/System/Reflection/CustomAttribute.cs

src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeModule.cs

src/coreclr/src/vm/runtimehandles.cpp

src/coreclr/src/System.Private.CoreLib/src/System/RuntimeHandles.cs

benaadams · 2020-11-29T19:12:11Z

windows x86 Release error is #45317

benaadams · 2020-11-29T20:32:17Z

😅 My Linq is bad for the tests

System.InvalidOperationException : The test method expected 1 parameter value, but 27 parameter values were provided.

benaadams · 2020-11-30T12:42:23Z

Rebased

benaadams · 2020-11-30T15:52:23Z

Seems happy with the crossgen revert

jkotas

LGTM otherwise

src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeModule.cs

jkotas · 2020-11-30T22:16:49Z

@GrabYourPitchforks @steveharter Could you please take a look as well as reflection area owners?

benaadams · 2020-12-01T01:08:50Z

Feed timeouts on same variants; rebased to trigger CI

benaadams · 2020-12-01T19:00:34Z

Rebased for CI

GrabYourPitchforks

Thanks!

src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeModule.cs

GrabYourPitchforks · 2020-12-01T21:53:15Z

The two comments I left were nits. Feel free to address them or leave them as-is at your discretion. I personally don't like target new, but there's seemingly no prohibition against it, so whatever. :)

benaadams · 2020-12-01T22:04:14Z

I personally don't like target new, but there's seemingly no prohibition against it, so whatever. :)

Could always go var instead... 😉

We only use var when it's obvious what the variable type is (e.g. var stream = new FileStream(...) not var stream = OpenStandardInput()).

Co-authored-by: Jan Kotas <[email protected]>

jkotas · 2020-12-02T17:25:34Z

Thank you!

Dotnet-GitSync-Bot added the area-System.Reflection label Nov 28, 2020

jkotas added the tenet-performance Performance related issue label Nov 28, 2020

jkotas reviewed Nov 28, 2020

View reviewed changes

src/coreclr/src/System.Private.CoreLib/src/System/Reflection/RuntimeModule.cs Outdated Show resolved Hide resolved

jkotas reviewed Nov 28, 2020

View reviewed changes

src/coreclr/src/System.Private.CoreLib/src/System/RuntimeHandles.cs Outdated Show resolved Hide resolved

benaadams mentioned this pull request Nov 28, 2020

Optimise GetCustomAttribute(s) #45121

Closed

jkotas reviewed Nov 28, 2020

View reviewed changes

src/coreclr/src/System.Private.CoreLib/src/System/RuntimeHandles.cs Outdated Show resolved Hide resolved

jkotas reviewed Nov 28, 2020

View reviewed changes

src/coreclr/src/System.Private.CoreLib/src/System/RuntimeHandles.cs Outdated Show resolved Hide resolved

benaadams force-pushed the ResolveType branch from 060e115 to 34df958 Compare November 28, 2020 21:54

benaadams changed the title ~~Skip fixed and GC.KeepAlive for ResolveTypeHandleInternal when no generics~~ Remove ResolveXHandle indirections Nov 28, 2020

benaadams force-pushed the ResolveType branch from 34df958 to 52defc6 Compare November 28, 2020 21:59