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

Make TypeComponentsCache trimmable #80726

Merged
merged 2 commits into from
Jan 19, 2023

Conversation

MichalStrehovsky
Copy link
Member

@MichalStrehovsky MichalStrehovsky commented Jan 17, 2023

Contributes to #80165.

Dispensing of reflection member infos is done through a member policies class. This is a generic class that has specialized implementations for each kind of member info.

It used a clever trick to get to the specific implementations. Just access MemberPolicies<EventInfo>.Default to get one for events or MemberPolicies<PropertyInfo>.Default to get one for properties. It was also absolutely not trimming friendly.

This change removes the clever trick and replaces it with good old fashioned parameter passing.

Cc @dotnet/ilc-contrib

Contributes to dotnet#80165.

The compiler is not able to get rid of the call to `PerNameQueryCache<M>.Factory` virtual method due to generic code sharing. Making this not shareable allows trimming it because now we can see `ConcurrentUnifier<StringKey, __Canon>.Factory` is never called (whereas `ConcurrentUnifier<__Canon, __Canon>.Factory` is called).

This is a ~5 kB regression for apps that do use reflection.

We could explore different strategies to make this trimmable and not be a size regression, but I'm not sure if it's really worth it. We'd like to get rid of this reflection stack implementation anyway and replace it with something more lightweight.
@ghost
Copy link

ghost commented Jan 17, 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 #80165.

The compiler is not able to get rid of the call to PerNameQueryCache<M>.Factory virtual method due to generic code sharing. Making this not shareable allows trimming it because now we can see ConcurrentUnifier<StringKey, __Canon>.Factory is never called (whereas ConcurrentUnifier<__Canon, __Canon>.Factory is called).

This is a ~5 kB regression for apps that do use reflection.

We could explore different strategies to make this trimmable and not be a size regression, but I'm not sure if it's really worth it. We'd like to get rid of this reflection stack implementation anyway and replace it with something more lightweight.

Cc @dotnet/ilc-contrib

Author: MichalStrehovsky
Assignees: MichalStrehovsky
Labels:

area-NativeAOT-coreclr

Milestone: -

// actually used in an app that is light on reflection use. But we do have other canonically equivalent
// ConcurrentUnifier instances in the program and their use could drag this implementation into the app.
// Making the ConcurrentUnifier instance unshareable gives the compiler the ability to unshare this.
private struct StringKey : IEquatable<StringKey>
Copy link
Contributor

@neon-sunset neon-sunset Jan 17, 2023

Choose a reason for hiding this comment

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

Could this be a readonly record struct? Or is it not allowed in System.Private.CoreLib?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't use records in CoreLib (or probably all of runtime repo in general). They add a lot more overhead than they're worth. Trading a couple lines of code for kilobytes of footprint for everyone who uses this is not a good tradeoff.

Recently: dotnet/aspnetcore#45604 (comment)

Copy link
Contributor

@neon-sunset neon-sunset Jan 17, 2023

Choose a reason for hiding this comment

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

My assumption was that this could be trimmed but I guess not. Time to read docs on trimming in more detail, thanks for the explanation.

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that a lot of the conveniences records bring is behind interfaces and virtual methods. E.g. ToString. It's hard to get rid of ToString.

@MichalStrehovsky MichalStrehovsky added the NO-REVIEW Experimental/testing PR, do NOT review it label Jan 17, 2023
@MichalStrehovsky
Copy link
Member Author

I'm marking this as no-review. As I was working on another piece of #80165 I came to realization that we'll probably need the components cache for nested types, but not the others, so this might require a different shape of the fix.

@MichalStrehovsky MichalStrehovsky removed the NO-REVIEW Experimental/testing PR, do NOT review it label Jan 18, 2023
@MichalStrehovsky
Copy link
Member Author

I've changed the way to do this and updated top-post with the description. This is ready for review.

@MichalStrehovsky MichalStrehovsky merged commit fa19917 into dotnet:main Jan 19, 2023
@MichalStrehovsky MichalStrehovsky deleted the typecache branch January 19, 2023 00:20
MichalStrehovsky added a commit to MichalStrehovsky/runtime that referenced this pull request Jan 19, 2023
Follow up to dotnet#80726. Needed a bit more laziness to really get everything out of the picture.
MichalStrehovsky added a commit that referenced this pull request Jan 19, 2023
Follow up to #80726. Needed a bit more laziness to really get everything out of the picture.
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
Contributes to dotnet#80165.

Dispensing of reflection member infos is done through a member policies class. This is a generic class that has specialized implementations for each kind of member info.

It used a clever trick to get to the specific implementations. Just access `MemberPolicies<EventInfo>.Default` to get one for events or `MemberPolicies<PropertyInfo>.Default` to get one for properties. It was also absolutely not trimming friendly.

This change removes the clever trick and replaces it with good old fashioned parameter passing.
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
Follow up to dotnet#80726. Needed a bit more laziness to really get everything out of the picture.
@ghost ghost locked as resolved and limited conversation to collaborators Feb 18, 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