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

[NativeAOT] Reflection type refactoring #93440

Merged
merged 52 commits into from
Oct 31, 2023
Merged

[NativeAOT] Reflection type refactoring #93440

merged 52 commits into from
Oct 31, 2023

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Oct 13, 2023

Contributes to #91704

Design:

  • RuntimeType - sealed light-weight System.Type, similar to CoreCLR RuntimeType. It has just two fields MethodTable* and lazily initialized pointer to the full reflection. The light-weight method and properties are implemented using MethodTable*, the rest initializes the full reflection RuntimeTypeInfo lazily and calls it to do the work.

  • RuntimeTypeInfo - internal, lazily created full reflection. It is similar to the Cache attached to RuntimeType in CoreCLR. The reflection-free mode is implemented by blocking creation of RuntimeTypeInfo and throwing instead.

Some reflection micro-benchmark may regress with this change on native AOT. It is expected due to the extra indirections, but it makes the reflection more pay-for-play and a bit closer to how it is implemented in CoreCLR.

@ghost
Copy link

ghost commented Oct 13, 2023

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

Issue Details

Contributes to #91704

Design:

  • RuntimeType - sealed light-weight System.Type, similar to CoreCLR RuntimeType. It has just two fields MethodTable* and lazily initialized pointer to the full reflection. The light-weight method and properties are implemented using MethodTable*, the rest initializes the full reflection RuntimeTypeInfo lazily and calls it to do the work.

  • MetadataOnlyType - "runtime type" without MethodTable*. It always calls into the full reflection RuntimeTypeInfo to do the work.

  • RuntimeTypeInfo - internal, lazily created full reflection. It is similar to the Cache attached to RuntimeType in CoreCLR. The reflection-free mode is implemented by blocking creation of RuntimeTypeInfo and throwing instead.

Author: jkotas
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@ghost ghost assigned jkotas Oct 13, 2023
@jkotas jkotas marked this pull request as draft October 13, 2023 04:12
@jkotas
Copy link
Member Author

jkotas commented Oct 13, 2023

I am not sure whether I like the RuntimeType and MetadataOnlyType split. It has been a bug farm, especially in the shared code. For example, there are number of checks like type is RuntimeType that tend to be broken when both RuntimeType and MetadataOnlyType represent RuntimeType.

@MichalStrehovsky
Copy link
Member

I am not sure whether I like the RuntimeType and MetadataOnlyType split. It has been a bug farm, especially in the shared code. For example, there are number of checks like type is RuntimeType that tend to be broken when both RuntimeType and MetadataOnlyType represent RuntimeType.

Hm, it was worth a shot. Maybe RuntimeType with a null type handle would still work. My concern with going that route was around another set of bugs (assuming we can get a type handle from a RuntimeType) but maybe there's fewer of those in the end.

reflection-free mode is implemented by blocking creation

If reflection-free mode didn't survive this refactor I wouldn't be too sad :).

@EgorBo
Copy link
Member

EgorBo commented Oct 13, 2023

Does it mean we'll be able to enable some of the NonGC-related opts for RuntimeType now which currently only work on CoreCLR?

@jkotas
Copy link
Member Author

jkotas commented Oct 13, 2023

Does it mean we'll be able to enable some of the NonGC-related opts for RuntimeType now which currently only work on CoreCLR?

Yes, it is the goal for #91704

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.

Looks good to the extent I can reasonably review!

@jkotas
Copy link
Member Author

jkotas commented Oct 31, 2023

/azp run runtime-nativeaot-outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jkotas
Copy link
Member Author

jkotas commented Oct 31, 2023

The outerloop failures are unrelated. Fixing them in #94213.

@jkotas jkotas merged commit 41e4d65 into dotnet:main Oct 31, 2023
173 of 175 checks passed
@jkotas jkotas deleted the reflection branch October 31, 2023 16:58
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request Nov 2, 2023
Contributes to dotnet#91704.

When we deleted reflection blocking in dotnet#85810 we had to update `EETypeNode`/`ConstructedEETypeNode` to ensure any `MethodTable` also has metadata (constructed or not). This was needed because of how reflection was structured - we couldn't create a `RuntimeType` without knowing about the metadata. After the refactor in dotnet#93440, metadata is no longer a prerequisite to constructing a `RuntimeType`.

The compiler can go back to optimizing away the metadata. This affects things like `typeof(Foo) == bar.GetType()`. No metadata is needed for this (and we do optimized the `Foo` MethodTable to unconstructed one) but we still had to generate metadata.

Besides the rollback of EEType to the previous shape, this also has a bugfix for dotnet#91988 that was found later - interface types used in cast/dispatch should be considered constructed.

I'm seeing 0.1 - 0.7% size saving.
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request Nov 3, 2023
With dotnet#93440 it became possible to place `RuntimeType` instances in the frozen region. This adds support for generating frozen `RuntimeType` instances within the compiler. We give these to RyuJIT whenever it asks for them.

* New `FrozenObjectNode` descendant that represents a `RuntimeType` instance. We have two flavors - one wraps constructed and the other wraps necessary `MethodTable`s (we have a need for freezing both kinds to keep our ability to optimize `RuntimeTypes` used in comparisons only).
* We hand out references to these whenever RyuJIT needs them.
* At `MethodTable` emission time, we check whether a frozen `RuntimeType` for this `MethodTable` was generated. If so, we pre-populate `MethodTable`’s `WritableData` section with a reloc to the `RuntimeType`. Otherwise we use null as usual.
* At runtime for `GetTypeFromEEType`, if `WritableData` is non-null, we just return that. Otherwise we create a pinned `RuntimeType` instance and write it into `WritableData`. In the future, we should allocate this on a frozen heap.

Old codegen for `Console.WriteLine(typeof(Program))`:

```asm
sub         rsp,28h
lea         rcx,[repro_Program::`vftable' (07FF7D03154E0h)]
call        S_P_CoreLib_Internal_Runtime_CompilerHelpers_LdTokenHelpers__GetRuntimeType (07FF7D0253290h)
mov         rcx,rax
call        System_Console_System_Console__WriteLine_11 (07FF7D0262AC0h)
nop
add         rsp,28h
ret
```

New codegen:

```asm
sub         rsp,28h
lea         rcx,[__RuntimeType_repro_Program (07FF7A218EC50h)]
call        System_Console_System_Console__WriteLine_11 (07FF7A20F2680h)
nop
add         rsp,28h
ret
```

I’ll do cctor preinitialization in a subsequent PR to keep things short.
Contributes to dotnet#91704.
MichalStrehovsky added a commit that referenced this pull request Nov 6, 2023
With #93440 it became possible to place `RuntimeType` instances in the frozen region. This adds support for generating frozen `RuntimeType` instances within the compiler. We give these to RyuJIT whenever it asks for them.

* New `FrozenObjectNode` descendant that represents a `RuntimeType` instance. We have two flavors - one wraps constructed and the other wraps necessary `MethodTable`s (we have a need for freezing both kinds to keep our ability to optimize `RuntimeTypes` used in comparisons only).
* We hand out references to these whenever RyuJIT needs them.
* At `MethodTable` emission time, we check whether a frozen `RuntimeType` for this `MethodTable` was generated. If so, we pre-populate `MethodTable`’s `WritableData` section with a reloc to the `RuntimeType`. Otherwise we use null as usual.
* At runtime for `GetTypeFromEEType`, if `WritableData` is non-null, we just return that. Otherwise we create a pinned `RuntimeType` instance and write it into `WritableData`. In the future, we should allocate this on a frozen heap.

Old codegen for `Console.WriteLine(typeof(Program))`:

```asm
sub         rsp,28h
lea         rcx,[repro_Program::`vftable' (07FF7D03154E0h)]
call        S_P_CoreLib_Internal_Runtime_CompilerHelpers_LdTokenHelpers__GetRuntimeType (07FF7D0253290h)
mov         rcx,rax
call        System_Console_System_Console__WriteLine_11 (07FF7D0262AC0h)
nop
add         rsp,28h
ret
```

New codegen:

```asm
sub         rsp,28h
lea         rcx,[__RuntimeType_repro_Program (07FF7A218EC50h)]
call        System_Console_System_Console__WriteLine_11 (07FF7A20F2680h)
nop
add         rsp,28h
ret
```

I’ll do cctor preinitialization in a subsequent PR to keep things short.
Contributes to #91704.
MichalStrehovsky added a commit that referenced this pull request Nov 6, 2023
…#94287)

Contributes to #91704.

When we deleted reflection blocking in #85810 we had to update `EETypeNode`/`ConstructedEETypeNode` to ensure any `MethodTable` also has metadata (constructed or not). This was needed because of how reflection was structured - we couldn't create a `RuntimeType` without knowing about the metadata. After the refactor in #93440, metadata is no longer a prerequisite to constructing a `RuntimeType`.

The compiler can go back to optimizing away the metadata. This affects things like `typeof(Foo) == bar.GetType()`. No metadata is needed for this (and we do optimize the `Foo` MethodTable to unconstructed one) but we still had to generate metadata.

Besides the rollback of EEType to the previous shape, this also has a bugfix for #91988 that was found later - interface types used in cast/dispatch should be considered constructed.

I'm seeing 0.1 - 0.7% size saving.
@ghost ghost locked as resolved and limited conversation to collaborators Nov 30, 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.

3 participants