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

Remove RuntimeHelpers.IsReference from NativeAOT #105118

Merged
merged 5 commits into from
Aug 20, 2024

Conversation

MichalPetryka
Copy link
Contributor

Should improve codegen, removed redundant VM code.

cc @MichalStrehovsky

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 18, 2024
Copy link
Contributor

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

@danmoseley
Copy link
Member

@MichalPetryka unrelated to this PR, would you mind dropping me an email? both danmose and joperezr @ microsoft.

@@ -49,7 +49,7 @@ public static partial class Activator
try
{
// Call the default constructor on the allocated instance.
if (RuntimeHelpers.IsReference<T>())
if (!typeof(T).IsValueType)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MichalStrehovsky Can we assume that typeof(T).IsValueType is always recognized as an intrinsic by the toolchain and that typeof(T) won't introduce any undesirable additional reflection dependencies?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is in activator implementation, I'm not too worried about possibility of more reflection dependencies - we're already allocating the type and it better be seen as reflected on and allocated.

But if the PR description has a claim of "Should improve codegen", I would want to see that claim backed up by disassembly of before/after code for a value type and for a ref type to ensure there's no surprise regression.

@agocke
Copy link
Member

agocke commented Aug 19, 2024

@MichalPetryka Could you grab some code samples to verify the results?

@jkotas
Copy link
Member

jkotas commented Aug 20, 2024

This is producing exact same code before and after on simple tests:

using System.Runtime.CompilerServices;
using System.Diagnostics.CodeAnalysis;

m1();
m2();
m3<object>();
m3<KeyValuePair<object,object>>();

[MethodImpl(MethodImplOptions.NoInlining)]
void m1()
{
    Activator.CreateInstance<DateTime>();
}

[MethodImpl(MethodImplOptions.NoInlining)]
void m2()
{
   Activator.CreateInstance<object>();
}

[MethodImpl(MethodImplOptions.NoInlining)]
void m3<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] T>()
{
   Activator.CreateInstance<T>();
}

Disassembly of relevant methods:

repro!repro_Program____Main___g__m1_0_0:
00007ff7`bea1a4f0 c3              ret

repro!S_P_CoreLib_System_Activator__CreateInstance<System___Canon>:
00007ff7`bea3a570 55              push    rbp
00007ff7`bea3a571 56              push    rsi
00007ff7`bea3a572 53              push    rbx
00007ff7`bea3a573 4883ec30        sub     rsp,30h
00007ff7`bea3a577 488d6c2440      lea     rbp,[rsp+40h]
00007ff7`bea3a57c 48894de8        mov     qword ptr [rbp-18h],rcx
00007ff7`bea3a580 488bd9          mov     rbx,rcx
00007ff7`bea3a583 488b7308        mov     rsi,qword ptr [rbx+8]
00007ff7`bea3a587 488d0db279faff  lea     rcx,[repro!S_P_CoreLib_System_Activator__MissingConstructorMethod (00007ff7`be9e1f40)]
00007ff7`bea3a58e 483bf1          cmp     rsi,rcx
00007ff7`bea3a591 741d            je      repro!S_P_CoreLib_System_Activator__CreateInstance<System___Canon>+0x40 (00007ff7`bea3a5b0)
00007ff7`bea3a593 488b4310        mov     rax,qword ptr [rbx+10h]
00007ff7`bea3a597 488b0b          mov     rcx,qword ptr [rbx]
00007ff7`bea3a59a ffd0            call    rax
00007ff7`bea3a59c 488bd8          mov     rbx,rax
00007ff7`bea3a59f 488bcb          mov     rcx,rbx
00007ff7`bea3a5a2 ffd6            call    rsi
00007ff7`bea3a5a4 90              nop
00007ff7`bea3a5a5 488bc3          mov     rax,rbx
00007ff7`bea3a5a8 4883c430        add     rsp,30h
00007ff7`bea3a5ac 5b              pop     rbx
00007ff7`bea3a5ad 5e              pop     rsi
00007ff7`bea3a5ae 5d              pop     rbp
00007ff7`bea3a5af c3              ret
...

repro!repro_Program____Main___g__m3_0_2<S_P_CoreLib_System_Collections_Generic_KeyValuePair_2<System___Canon__System___Canon>>:
00007ff7`bea3a560 c3              ret

Copy link
Member

@MichalStrehovsky MichalStrehovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! (And thank you Jan for checking the disassembly!).

@MichalStrehovsky MichalStrehovsky merged commit 6802ff2 into dotnet:main Aug 20, 2024
86 of 88 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Sep 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants