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

[release/9.0-staging] [mono] Switch generic instance cache back to GHashTable; improve ginst hash function #113316

Merged

Conversation

github-actions[bot]
Copy link
Contributor

@github-actions github-actions bot commented Mar 10, 2025

Backport of #113287 to release/9.0-staging

/cc @kg

Customer Impact

  • Customer reported
  • Found internally

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

  • Yes
  • No

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:

.NET 8
real     0m32.759s
user    0m29.893s
sys      0m1.876s

.NET 9
real       0m36.223s
user      0m33.228s
sys        0m1.929s

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:

  • The PR target branch is release/X.0-staging, not release/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.

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a 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

@carlossanlop
Copy link
Member

@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.

@steveisok steveisok added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Mar 10, 2025
@steveisok
Copy link
Member

Approved by tactics over email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-VM-meta-mono Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants