-
Notifications
You must be signed in to change notification settings - Fork 5k
[release/9.0-staging] [mono] Switch generic instance cache back to GHashTable; improve ginst hash function #113316
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
[release/9.0-staging] [mono] Switch generic instance cache back to GHashTable; improve ginst hash function #113316
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. please get a code review. we will take for consideration in 9.0.x
@kg @vitek-karas @jeffschwMSFT Today is code complete for the April Release. If you want this fix included in this release, please get it approved by Tactics and merge it before 4pm PT. |
Approved by tactics over email |
Backport of #113287 to release/9.0-staging
/cc @kg
Customer Impact
Multiple customers have reported seeing significantly slower iOS/MacCatalyst AOT compilation times in .NET 9 compared to .NET 8. We found the slowdown was specific to ARM macs and was due to ARM specific inefficiencies with a hashtable we introduced early on in the .NET 9 cycle.
AOT build times on Apple hardware regressed significantly in .NET 9 and 10, i.e. from ~50 seconds to ~250 seconds.
This change will revert to the hashtable container used for the generic instance cache in .NET 8.0 to address a performance regression introduced by changing to a different container in 9. Also improves the hash function used for the cache (the existing one was suboptimal.)
Regression
Can be traced back to #102039
Testing
Manual benchmarks were performed by @steveisok and @kotlarmilos on Apple hardware. I also performed measurements on Ampere ARM64 and AMD x64 hardware to verify that the performance characteristics of the alternative container used here are acceptable.
Running the test @rolfbjarne used in #110406 (comment) shows we get back to the expected range:
Risk
Low. We used this container in .NET 8 without issue. May cause a slight startup hit for the WASM target specifically since that motivated the original change, but profiling data showed that at most this container accounted for 3% of CPU time in the startup profile we were targeting, so any regression should be in the neighborhood of 5ms or less.
IMPORTANT: If this backport is for a servicing release, please verify that:
release/X.0-staging
, notrelease/X.0
.Package authoring no longer needed in .NET 9
IMPORTANT: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version.
Keep in mind that we still need package authoring in .NET 8 and older versions.