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

Restore EETypeNode to the state before we deleted reflection blocking #94287

Merged
merged 3 commits into from
Nov 6, 2023

Conversation

MichalStrehovsky
Copy link
Member

Contributes to #91704.

When we deleted reflection blocking in #85810 we had to update EETypeNode/ConstructedEETypeNode to ensure any MethodTable gets 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 optimized the Foo MethodTable to unconstructed one) but we still had to generate metadata for reflection stack sake.

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. Considering interfaces constructed doesn't result in pretty much any regressions and kind of makes sense (they don't have vtables and their interface list is not much cost).

Cc @dotnet/ilc-contrib

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.
@ghost
Copy link

ghost commented Nov 2, 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.

When we deleted reflection blocking in #85810 we had to update EETypeNode/ConstructedEETypeNode to ensure any MethodTable gets 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 optimized the Foo MethodTable to unconstructed one) but we still had to generate metadata for reflection stack sake.

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. Considering interfaces constructed doesn't result in pretty much any regressions and kind of makes sense (they don't have vtables and their interface list is not much cost).

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@jkotas
Copy link
Member

jkotas commented Nov 2, 2023

Asserts in checked build

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.

Thanks!

@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

/azp run runtime-nativeaot-outerloop

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky MichalStrehovsky merged commit 1ddb513 into dotnet:main Nov 6, 2023
127 of 135 checks passed
@MichalStrehovsky MichalStrehovsky deleted the nounconstructed branch November 6, 2023 22:16
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 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