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

Performance Regression: GetCustomAttributes() Allocations Increased from netcoreapp2.1 #11637

Closed
Tracked by #93172 ...
NickCraver opened this issue Dec 9, 2018 · 4 comments
Labels
Milestone

Comments

@NickCraver
Copy link
Member

I'm not sure how much time I'll have to dig here, so filing an issue up front:
Allocations have increased and performance has regressed on certain Attribute paths sometime between netcoreapp2.1 (2.1.6) and netcoreapp3.0. I was adding the benchmark set I created for dotnet/coreclr#20779 and dotnet/coreclr#20795 to the new dotnet/performance repo in dotnet/performance#177 and too illustrate the utility, I was getting a screenshot for a tweet and...hey I found regressions. So this whole dotnet/performance thing is already paying off!

Here's a run comparing net472, netcoreapp2.1 (2.1.6) and netcoreapp3.0 (3.0.0-preview-27122-01 specifically):
image

Note the performance and allocation regressions specifically in these 4 benchmarks:

To reproduce this, in the dotnet/performance repo:

C:\git\NickCraver\performance\src\benchmarks\micro (master)
λ dotnet run -c Release -f netcoreapp3.0 -- --runtimes netcoreapp2.1 netcoreapp3.0 --filter System.Reflection.Attributes.GetCustom*

You can see in the initial benchmarks of dotnet/coreclr#20779 that the regression predates that PR, but that's the only date bound I have at the moment - we'll need to dig into history here and see what happened. Luckily, benchmarks in dotnet/performance being used for regression testing should prevent this sort of thing in the future and that's awesome. Kudos to @adamsitnik for putting that together...I never would have seen this otherwise.

cc @benaadams @danmosemsft @jkotas @stephentoub

@jkotas
Copy link
Member

jkotas commented Dec 10, 2018

This is regression introduce by dotnet/coreclr#9189 . Bulk of the regression is in FilterCustomAttributeRecord method.

This change modified ModuleHandle.ResolveMethodHandleInternal(decoratedModule.GetNativeHandle(), caRecord.tkCtor) to much more expensive ctor = decoratedModule.ResolveMethod(caRecord.tkCtor, attributeType.GenericTypeArguments, null).MethodHandle.GetMethodInfo().

@AviAvni Is it something you can take a look at?

NickCraver referenced this issue in NickCraver/coreclr Dec 10, 2018
…only generic attributes

This is a trivial quick-fix for #21456 where regressions between 2.1 and 3.0 were discovered on most attibute pathways due to the allocation overhead in the generic-supporting pathways. The workaround is to simply not take that slow/expensive path for non-generics.

While I'd like to optimize `RuntimeModule.ResolveMethod` further, there's a public surface area in play there that makes the changes non-trivial. There, we'll have to choose overhead on the public path (which may still be a net win), or duplication in code for another path.
@NickCraver
Copy link
Member Author

I opened up a PR that short-circuits out of the costly path generics currently need in dotnet/coreclr#21462 - hopefully that's an acceptable quick improvement, though ResolveMethod() can still get a lot of love.

@AviAvni
Copy link
Contributor

AviAvni commented Dec 10, 2018

@jkotas the fix look great thanks @NickCraver

adamsitnik referenced this issue in dotnet/coreclr Dec 10, 2018
…attributes) (#21462)

* Fix for #21456 - restrict increased generic attribute allocations to only generic attributes

This is a trivial quick-fix for #21456 where regressions between 2.1 and 3.0 were discovered on most attibute pathways due to the allocation overhead in the generic-supporting pathways. The workaround is to simply not take that slow/expensive path for non-generics.

While I'd like to optimize `RuntimeModule.ResolveMethod` further, there's a public surface area in play there that makes the changes non-trivial. There, we'll have to choose overhead on the public path (which may still be a net win), or duplication in code for another path.

* Update comments
@NickCraver
Copy link
Member Author

Closing out - fixed via dotnet/coreclr#21462 :)

Thanks everyone!

morganbr referenced this issue in dotnet/coreclr Dec 14, 2018
…attributes) (#21462)

* Fix for #21456 - restrict increased generic attribute allocations to only generic attributes

This is a trivial quick-fix for #21456 where regressions between 2.1 and 3.0 were discovered on most attibute pathways due to the allocation overhead in the generic-supporting pathways. The workaround is to simply not take that slow/expensive path for non-generics.

While I'd like to optimize `RuntimeModule.ResolveMethod` further, there's a public surface area in play there that makes the changes non-trivial. There, we'll have to choose overhead on the public path (which may still be a net win), or duplication in code for another path.

* Update comments
@msftgits msftgits transferred this issue from dotnet/coreclr Jan 31, 2020
@msftgits msftgits added this to the 3.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants