Skip to content

Optimize ResourceContainer/AssemblyLoadEventHandler methods, remove allocs #9822

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

Merged
merged 17 commits into from
Feb 3, 2025

Conversation

h3xds1nz
Copy link
Member

@h3xds1nz h3xds1nz commented Sep 20, 2024

Description

Optimizes two rather important startup methods in ResourceContainer class, the handler for runtime assembly load events - OnAssemblyLoadEventHandler and GetResourceManagerWrapper. This saves several milliseconds upon startup.

  • I retracted from using StringComparer.OrdinalIgnoreCase as it would be slower by ~60ns in the final solution.
  • For parsing the assembly name from safe input (RuntimeAssembly), I've re-used the Optimize retrieval of simple name from Assembly.FullName, fix up faulty methods #9739 and added a new method for Version/PublicKeyToken
  • I've originally used ArrayPool<char> for the unlikely event of needing an array (256+ chars) but seems redundant to initialize it this early, and we shall not really need to allocate an array anyways in 99% cases.
  • Using AlternativeLookup, this now becomes fully allocation-free given that usually only 1 entry is in the dictionary.
  • It is safe to assume that ToLowerInvariant produces same-length output, the own function's sanity check and several other dotnet runtime libraries (plus tons of user code) depend on this behavior of per-character folding.
  • Some of the comments (e.g. about GACed assemblies) could have been removed, but I've kept them in.
  • We seal the class for additional perf benefit as it is derived.

Do note that these benchmarks measure throughput and allocations (accurate), the savings are in milliseconds this early.
Allocations are dependent on presence of public key / assembly name length, this just illustrates general scenario.

Single throughput of AssemblyLoadHandler

Method assembly Mean [ns] Error [ns] StdDev [ns] Code Size [B] Gen0 Allocated [B]
Original Syste(...)7798e [89] 636.3 ns 6.57 ns 6.14 ns 5,880 B 0.0896 1504 B
PR__EDIT Syste(...)7798e [89] 121.6 ns 0.44 ns 0.39 ns 5,052 B - -

Standard app startup (28 libraries loaded)

Method assembly Mean [ns] Error [ns] StdDev [ns] Code Size [B] Gen0 Allocated [B]
Original Syste(...)7798e [89] 17,579.6 ns 151.20 ns 141.43 ns 8,030 B 2.6245 44352 B
PR__EDIT Syste(...)7798e [89] 3,395.5 ns 5.43 ns 4.81 ns 3,166 B - -

Customer Impact

Improved startup performance, decreased startup allocations.

Regression

No.

Testing

Local build, running few sample apps using resources and verifying functionality.

Risk

Low, I believe I didn't achieve any change of the behavior, even unintentionally.

Microsoft Reviewers: Open in CodeFlow

@h3xds1nz h3xds1nz requested review from a team as code owners September 20, 2024 09:29
@dotnet-policy-service dotnet-policy-service bot added PR metadata: Label to tag PRs, to facilitate with triage Community Contribution A label for all community Contributions labels Sep 20, 2024
Copy link
Contributor

@edwardneal edwardneal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! Does the benchmark handle the startup of an app with something bound to an embedded image resource? There's a call to Assembly.Load in the call stack of ResourceContainer.GetPart (GetPart -> GetPartCore -> GetResourceManagerWrapper -> BaseUriHelper.GetLoadedAssembly -> Assembly.Load) and it feels like this is going to be the largest performance killer in the worst-case scenario; I'm curious to see whether it's triggered under normal circumstances.

@JeremyKuhne
Copy link
Member

@adamsitnik wanted to point this out to you as it is doing some assembly name parsing in a hot path for WPF.

@himgoyalmicro himgoyalmicro merged commit 444cf2f into dotnet:main Feb 3, 2025
8 checks passed
@himgoyalmicro
Copy link
Contributor

Thank you @h3xds1nz for your continuous contributions 😄

@h3xds1nz
Copy link
Member Author

h3xds1nz commented Feb 3, 2025

@himgoyalmicro Thank you, happy this one is in <3

h3xds1nz added a commit to h3xds1nz/wpf that referenced this pull request Feb 3, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Mar 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Community Contribution A label for all community Contributions PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants