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

Allow preinitializing RuntimeType instances #94405

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

MichalStrehovsky
Copy link
Member

Unblock serializing RuntimeType instances from the static constructor interpreter.

With this we can now preinitialize static readonly Type s_foo = typeof(Foo) and RyuJIT will optimize reads of s_foo into loading a constant value.

Contributes to #91704.

Cc @dotnet/ilc-contrib

Unblock serializing `RuntimeType` instances from the static constructor interpreter.

With this we can now preinitialize `static readonly Type s_foo = typeof(Foo)` and RyuJIT will optimize reads of `s_foo` into loading a constant value.

Contributes to dotnet#91704.
@ghost
Copy link

ghost commented Nov 6, 2023

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

Issue Details

Unblock serializing RuntimeType instances from the static constructor interpreter.

With this we can now preinitialize static readonly Type s_foo = typeof(Foo) and RyuJIT will optimize reads of s_foo into loading a constant value.

Contributes to #91704.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@@ -8989,6 +8989,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
FALLTHROUGH;
case CORINFO_FIELD_STATIC_SHARED_STATIC_HELPER:
case CORINFO_FIELD_STATIC_ADDRESS:
case CORINFO_FIELD_STATIC_RELOCATABLE:
Copy link
Member Author

Choose a reason for hiding this comment

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

@EgorBo could you have a look at this? This affects a lot more than just RuntimeType and I might be missing why we weren't doing this before. (We might also want to do this for STATIC_RVA_ADDRESS.)

Copy link
Member

Choose a reason for hiding this comment

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

@MichalStrehovsky wow, it's actually a good catch! It should improve some things, e.g.:

static readonly string Fld = "Hello";

[MethodImpl(MethodImplOptions.NoInlining)]
static string Test() => Fld;
; Assembly listing for method Benchmarks:Test():System.String (FullOpts)
-      mov      rax, qword ptr [(reloc 0x400000000042c388)]
-      mov      rax, gword ptr [rax+0x08]
+      lea      rax, gword ptr [(reloc 0x400000000042c398)]      ; '"Hello"'
       ret      
; Total bytes of code 12

Copy link
Member

Choose a reason for hiding this comment

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

Basically, static readonly wasn't foldable for gc types for NAOT

@@ -689,6 +689,8 @@ class C3<T> : IInterface, IInterface<T>
static IInterface<object> s_c3a = new C3<object>();
static IInterface s_c3b = new C3<object>();

// Works around https://github.com/dotnet/runtime/pull/94342
[MethodImpl(MethodImplOptions.NoOptimization)]
Copy link
Member Author

Choose a reason for hiding this comment

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

This found a new bug. The Run method is essentially optimized to a throw. It's ridiculous how well this optimizes:

  • We figured out the value assigned to the static is read only using whole program view (the static is not marked read only in source code)
  • RyuJIT then figured out exact type information from the preinit data blob.
  • RyuJIT then devirtualized the call (into the wrong method, but that's the bug)
  • RyuJIT then folded the string constant comparison.

All that's left is the throw (and a couple garbage instructions I couldn't fully make sense of).

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky
Copy link
Member Author

@dotnet/ilc-contrib could someone have a look?

@jkotas
Copy link
Member

jkotas commented Nov 9, 2023

System.Threading.Tasks tests are failing with a message that we have not seen before:

[FAIL] System.Threading.Tasks.Tests.TaskRunSyncTests.TaskRunSyncTest10
Task wasn't RunSynchronously with TaskScheduler specified
   at System.Threading.Tasks.Tests.TaskRunSyncTest.RealRun() + 0x507

Is it a pre-existing problem or a regression introduced by this change?

@MichalPetryka
Copy link
Contributor

Is it a pre-existing problem or a regression introduced by this change?

I think I've seen this once on one of my PRs.

@MichalStrehovsky
Copy link
Member Author

Is it a pre-existing problem or a regression introduced by this change?

I think I've seen this once on one of my PRs.

This test sometimes fails, always with a failure mode that doesn't produce dumps. It never reproed for me locally.

@MichalStrehovsky
Copy link
Member Author

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Co-authored-by: Michal Strehovský <MichalStrehovsky@users.noreply.github.com>
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thank you!

@MichalStrehovsky MichalStrehovsky merged commit 1347af7 into dotnet:main Nov 13, 2023
@MichalStrehovsky MichalStrehovsky deleted the preinitrttype branch November 13, 2023 07:01
@github-actions github-actions bot locked and limited conversation to collaborators Dec 13, 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.

4 participants