-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Comments
This is regression introduce by dotnet/coreclr#9189 . Bulk of the regression is in FilterCustomAttributeRecord method. This change modified @AviAvni Is it something you can take a look at? |
…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.
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 |
@jkotas the fix look great thanks @NickCraver |
…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
Closing out - fixed via dotnet/coreclr#21462 :) Thanks everyone! |
…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
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 betweennetcoreapp2.1
(2.1.6) andnetcoreapp3.0
. I was adding the benchmark set I created for dotnet/coreclr#20779 and dotnet/coreclr#20795 to the newdotnet/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 wholedotnet/performance
thing is already paying off!Here's a run comparing
net472
,netcoreapp2.1
(2.1.6
) andnetcoreapp3.0
(3.0.0-preview-27122-01
specifically):Note the performance and allocation regressions specifically in these 4 benchmarks:
To reproduce this, in the
dotnet/performance
repo: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
The text was updated successfully, but these errors were encountered: