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

Shuffle some MethodTable flags #85634

Merged
merged 2 commits into from
May 3, 2023
Merged

Conversation

MichalStrehovsky
Copy link
Member

  • IDynamicInterfaceCastableFlag cannot be set on types that have a component size, so move it from the valuable location in Flags to FlagsEx.
  • Move HasSealedVTableEntriesFlag from rare flags to flags. This flag is not so rare. All async state machine types set it, for example.
  • Delete IsAbstractClassFlag. This was introduce in .NET Native for GetUninitializedObject since accessing Type.IsAbstract could trigger a MissingMetadataException there. We got rid of that concept in NativeAOT because it cannot be reconciled with ILLink trimming. Rewrite the code to use Type.IsAbstract.

Saves 15 kB on BasicMinimalApis, which is nice. Also makes things faster since we avoid reading optional fields and rare flags.

Cc @dotnet/ilc-contrib

* `IDynamicInterfaceCastableFlag` cannot be set on types that have a component size, so move it from the valuable location in `Flags` to `FlagsEx`.
* Move `HasSealedVTableEntriesFlag` from rare flags to flags. This flag is not so rare. All async state machine types set it, for example.
* Delete `IsAbstractClassFlag`. This was introduce in .NET Native for `GetUninitializedObject` since accessing `Type.IsAbstract` could trigger a `MissingMetadataException` there. We got rid of that concept in NativeAOT because it cannot be reconciled with ILLink trimming. Rewrite the code to use `Type.IsAbstract`.

Saves 15 kB on BasicMinimalApis, which is nice. Also makes things faster since we avoid reading optional fields and rare flags.
@ghost
Copy link

ghost commented May 2, 2023

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

Issue Details
  • IDynamicInterfaceCastableFlag cannot be set on types that have a component size, so move it from the valuable location in Flags to FlagsEx.
  • Move HasSealedVTableEntriesFlag from rare flags to flags. This flag is not so rare. All async state machine types set it, for example.
  • Delete IsAbstractClassFlag. This was introduce in .NET Native for GetUninitializedObject since accessing Type.IsAbstract could trigger a MissingMetadataException there. We got rid of that concept in NativeAOT because it cannot be reconciled with ILLink trimming. Rewrite the code to use Type.IsAbstract.

Saves 15 kB on BasicMinimalApis, which is nice. Also makes things faster since we avoid reading optional fields and rare flags.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

@@ -319,6 +319,11 @@ public static void PrepareDelegate(Delegate d)
throw new NotSupportedException(SR.NotSupported_ManagedActivation);
}
Copy link
Member

Choose a reason for hiding this comment

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

(Unrelated to this PR)

We are missing a check for function pointers here. Could you please fix it in a follow up?

Copy link
Member

Choose a reason for hiding this comment

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

Activator.CreateInstance seems to be missing it too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not close to a computer now but I think Activator should be fine because there's no constructor to run and they're not a valuetype. I don't see it handling pointers either.

@MichalStrehovsky MichalStrehovsky merged commit 7afd85d into dotnet:main May 3, 2023
@MichalStrehovsky MichalStrehovsky deleted the mtflags branch May 3, 2023 07:05
@ghost ghost locked as resolved and limited conversation to collaborators Jun 2, 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.

2 participants