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

Get rid of reflection blocked types #85810

Merged
merged 6 commits into from
May 8, 2023

Conversation

MichalStrehovsky
Copy link
Member

Fixes #72570.

Still need to delete workarounds that make things public in corelib but maybe this diff is large enough already?

We were gradually getting less and less from reflection blocking:

  • We stopped blocking things outside corelib (.NET Native blocked all of BCL; we don't).
  • We trim reflection metadata and that allows us to have method bodies without metadata.

With this, we should be able to get type definition-level reflection metadata for any MethodTable there is in the system.

This is a ~30 kB size regression for BasicMinimalApis. It is pretty much a wash for Hello World, because all the BCL cruft to handle reflection blocked types costs as much as the benefit of blocking.

Cc @dotnet/ilc-contrib

Fixes dotnet#72570.

Still need to delete workarounds that make things public in corelib but maybe this diff is large enough already?

We were gradually getting less and less from reflection blocking:

* We stopped blocking things outside corelib (.NET Native blocked all of BCL; we don't).
* We trim reflection metadata and that allows us to have method bodies without metadata.

With this, we should be able to get type definition-level reflection metadata for any MethodTable there is in the system.

This is a ~30 kB size regression for BasicMinimalApis. It is pretty much a wash for Hello World, because all the BCL cruft to handle reflection blocked types costs as much as the benefit of blocking.
@ghost
Copy link

ghost commented May 5, 2023

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

Issue Details

Fixes #72570.

Still need to delete workarounds that make things public in corelib but maybe this diff is large enough already?

We were gradually getting less and less from reflection blocking:

  • We stopped blocking things outside corelib (.NET Native blocked all of BCL; we don't).
  • We trim reflection metadata and that allows us to have method bodies without metadata.

With this, we should be able to get type definition-level reflection metadata for any MethodTable there is in the system.

This is a ~30 kB size regression for BasicMinimalApis. It is pretty much a wash for Hello World, because all the BCL cruft to handle reflection blocked types costs as much as the benefit of blocking.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: -
Labels:

area-NativeAOT-coreclr

Milestone: -

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
Copy link
Member Author

Unfortunately, the test failures are real. There is one other kind of MethodTables that take the "reflection blocked" route at runtime: MethodTables that are not actually user visible.

For example someType == typeof(NeverAllocated) would not create a constructed MethodTable and as a result would not get metadata. But we now need the metadata to create the RuntimeType instance since the reflection blocked path was deleted.

I'm fixing it by giving necessary MethodTables reflection metadata. It means they're now user visible and users can browse to them with e.g. Type.GetType. This should only be a problem for trim-unsafe code. I think the only problem with that would be GetUninitializedObject so I'm adding a new check for that to prevent these from landing on the GC heap (which would be a lot more unsafe than trimming-unsafe).

@MichalStrehovsky
Copy link
Member Author

This is an extra couple kB regression. It's not too bad.

@MichalStrehovsky
Copy link
Member Author

Okay, so on Linux the regression on Hello world is much worse - ~1,569 kB to 1,821 kB. Need to figure that out first 😭.

@MichalStrehovsky
Copy link
Member Author

The Linux regression was the same problem as #84156 (comment) with the SpanAction thing. @jkotas could you have a look at that commit? I made a small functional change in it because it looks like we were passing a wrong size of the buffer so I fixed that.

…oding.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

…oding.cs

Co-authored-by: Jan Kotas <jkotas@microsoft.com>
@MichalStrehovsky
Copy link
Member Author

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@MichalStrehovsky MichalStrehovsky merged commit 05a3d36 into dotnet:main May 8, 2023
222 of 236 checks passed
@MichalStrehovsky MichalStrehovsky deleted the deletereflblock branch May 8, 2023 06:35
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request May 9, 2023
Concept of reflection blocked internals was deleted in dotnet#85810. This is no longer needed.
jkotas pushed a commit that referenced this pull request May 9, 2023
* Stop special casing reflected-on internals

Concept of reflection blocked internals was deleted in #85810. This is no longer needed.

* Can no longer access ResourceSet outside corelib
@agocke
Copy link
Member

agocke commented May 9, 2023

Nice! Thanks for all the hard work here @MichalStrehovsky and @jkotas, much appreciated

MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request May 18, 2023
After dotnet#85810 we need to generate reflection metadata for owning types of static synchronized methods. Before that change, obtaining a RuntimeType for locking would obtain a reflection blocked type. We now need a named type and scanner must predict it.

Found in Pri0 testing.
jkotas pushed a commit that referenced this pull request May 18, 2023
After #85810 we need to generate reflection metadata for owning types of static synchronized methods. Before that change, obtaining a RuntimeType for locking would obtain a reflection blocked type. We now need a named type and scanner must predict it.

Found in Pri0 testing.
@ghost ghost locked as resolved and limited conversation to collaborators Jun 8, 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.

Investigate getting rid of reflection blocking
3 participants