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

Remove defensive copies in System.Reflection.MetadataLoadContext #101161

Closed
wants to merge 2 commits into from

Conversation

hamarb123
Copy link
Contributor

These fields being marked readonly causes 25 uses to create a defensive copy in total (since it's generic, it has no such things as readonly members, meaning every access requires a defensive copy). Removing the readonly should resolve these.

@stephentoub
Copy link
Member

Can you highlight specific examples where this is causing meaurably worse performance / worse code generation?

@steveharter steveharter added needs-author-action An issue or pull request that requires more info or actions from the author. tenet-performance Performance related issue labels Apr 17, 2024
@hamarb123
Copy link
Contributor Author

Can you highlight specific examples where this is causing meaurably worse performance / worse code generation?

Looking at the definition of EcmaMethodDecoder : IMethodDecoder, it seems like it'd be 4 pointer sizes. So the longer methods that will not be inlined will copy 32 bytes first before they can call the method (on 64-bit). I can do some proper testing at some point, but I'll probably be unable to do much for approximately the next 1.5 weeks.

@dotnet-policy-service dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Apr 17, 2024
@buyaa-n buyaa-n added the needs-author-action An issue or pull request that requires more info or actions from the author. label May 8, 2024
@hamarb123
Copy link
Contributor Author

Can you highlight specific examples where this is causing meaurably worse performance / worse code generation?

This is probably not that bad really, so I'll close it, since the preference is to keep readonly where possible without meaningful performance difference (#101156 (comment)).

@dotnet-policy-service dotnet-policy-service bot removed the needs-author-action An issue or pull request that requires more info or actions from the author. label May 13, 2024
@hamarb123 hamarb123 closed this May 13, 2024
@stephentoub
Copy link
Member

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Reflection community-contribution Indicates that the PR has been added by a community member tenet-performance Performance related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants