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.

Sign up for GitHub

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

Jump to bottom

Investigate getting rid of reflection blocking #72570

Closed
MichalStrehovsky opened this issue Jul 21, 2022 · 1 comment · Fixed by #85810
Closed

Investigate getting rid of reflection blocking #72570

MichalStrehovsky opened this issue Jul 21, 2022 · 1 comment · Fixed by #85810
Labels
area-NativeAOT-coreclr
Milestone
8.0.0

Comments

@MichalStrehovsky
Copy link
Member

NativeAOT has a significant amount of the runtime written in C#. Keeping reflection metadata for it results in a lot of garbage being generated by the compiler since nobody is ever going to reflect on these things (and if they do, preventing that sounds like a good thing). We have a concept of reflection blocking for that that we inherited from .NET Native.

But now that IlcTrimMetadata=true is the default, there's little size incentive in keeping this - we should measure the impact. If it's not significant, we should stop doing reflection blocking.

We currently disable reflection blocking in the libs test because they do reflect on CoreLib's implementation details to test them. But it also means we're not running the tests in the configuration that the users will run. It could mask issues such as private reflection being done by the libraries themselves (not just by library tests).
@MichalStrehovsky MichalStrehovsky added this to the 7.0.0 milestone Jul 21, 2022
@agocke agocke modified the milestones: 7.0.0, 8.0.0 Jul 26, 2022
@MichalStrehovsky
Copy link
Member Author

Looked at this briefly.

The first thing we need to do is set the <DebuggerSupport>false</DebuggerSupport> feature switch, or this brings over a bunch of debugging type proxies (that are private, and therefore ignored with reflection blocking).

Once that's addressed, I see a 1.5% size regression for a small app. It's not too bad, but we can do better. A lot of it seems to be native layout related. We have things we can do there to make things better. We should do that first before tackling this.

With IlcTrimMetadata=false, the regression is like 15%, so we definitely need that mode to stick. If we backtrack on that for whatever reason, this is not feasible.

MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this issue Aug 10, 2022
@MichalStrehovsky
Don't excessively generate default constructors
9aca8d0 
Fixes dotnet#68660.

This is a piece of logic that originates from .NET Native. We didn't have a hard stance on `Activator.CreateInstance<T>` at that time, so we took a conservative approach of always generating the default constructors for all types that have type handle.

Since we now do static analysis and warn if `Activator` is used with something we couldn't analyze. We can trim more.

I would delete the whole table and logic, but I'm not sure if we can't still hit a problem when `CreateInstance<T>` is used with a reflection blocked type - this would affect private reflection within CoreLib (we don't think about reflection when doing `new T()`, so this can happen). With reflection blocking even if dataflow figures out that `BlockedFromReflectionType..ctor` should be reflectable, we won't reflection enable it.

We really should just get rid of reflection blocking (dotnet#72570). Then all of this can go away.
@MichalStrehovsky MichalStrehovsky mentioned this issue Aug 10, 2022
Merged
jkotas pushed a commit that referenced this issue Aug 11, 2022
@MichalStrehovsky
Don't excessively generate default constructors (#73683)
2594ec1 
Fixes #68660.

This is a piece of logic that originates from .NET Native. We didn't have a hard stance on `Activator.CreateInstance<T>` at that time, so we took a conservative approach of always generating the default constructors for all types that have type handle.

Since we now do static analysis and warn if `Activator` is used with something we couldn't analyze. We can trim more.

I would delete the whole table and logic, but I'm not sure if we can't still hit a problem when `CreateInstance<T>` is used with a reflection blocked type - this would affect private reflection within CoreLib (we don't think about reflection when doing `new T()`, so this can happen). With reflection blocking even if dataflow figures out that `BlockedFromReflectionType..ctor` should be reflectable, we won't reflection enable it.

We really should just get rid of reflection blocking (#72570). Then all of this can go away.
@MichalStrehovsky MichalStrehovsky mentioned this issue Dec 24, 2022
Closed
@MichalStrehovsky MichalStrehovsky mentioned this issue Feb 6, 2023
Merged
@jkotas jkotas mentioned this issue Apr 27, 2023
Merged
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this issue May 5, 2023
@MichalStrehovsky
Get rid of reflection blocked types
f70486b 
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.
@MichalStrehovsky MichalStrehovsky mentioned this issue May 5, 2023
Merged
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 5, 2023
MichalStrehovsky added a commit that referenced this issue May 8, 2023
@MichalStrehovsky
Get rid of reflection blocked types (#85810)
05a3d36 
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.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 8, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Jun 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area-NativeAOT-coreclr
Projects
AppModel
Archived in project
Milestone
8.0.0
Development

Successfully merging a pull request may close this issue.

Get rid of reflection blocked types MichalStrehovsky/runtime
2 participants
@agocke @MichalStrehovsky