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

RAR: Obtain assembly names from the SDK #8634

Closed
Tracked by #8422
ladipro opened this issue Apr 5, 2023 · 0 comments · Fixed by #8688
Closed
Tracked by #8422

RAR: Obtain assembly names from the SDK #8634

ladipro opened this issue Apr 5, 2023 · 0 comments · Fixed by #8688

Comments

@ladipro
Copy link
Member

ladipro commented Apr 5, 2023

This issue tracks implementing the optimization per https://github.com/dotnet/msbuild/blob/main/documentation/design/rar-core-scenarios.md#obtain-assembly-names-from-the-sdk

Parent user story: #8422

The SDK is currently already passing relevant metadata such as AssemblyVersion and PublicKeyToken, so there is no need for RAR to open the file and parse its .NET metadata tables to get this information. This, together with the fact that SDK references are marked with ExternallyResolved so they cannot have dependencies outside of the primary set, means that there is no need to cache anything about these assemblies. Everything RAR needs comes (or can come if it's not there already) from the Assemblies parameter, explicitly provided on each invocation. Note, it may make sense to keep a cache in memory but it definitely doesn't make sense to save it to disk.

If we do this, then in the warm and hot scenarios where the per project disk cache exists, we use it only to cache data about NuGet references and project references, significantly reducing its size. By eliminating per-reference I/O for most references, RAR would see a significant performance boost.

@ladipro ladipro self-assigned this Apr 5, 2023
Forgind pushed a commit that referenced this issue May 5, 2023
Fixes #8634

Context
For SDK/workload references, the SDK is currently already passing relevant metadata such as AssemblyVersion and PublicKeyToken, so there is no need for RAR to open these files and parse their .NET metadata tables to get this information. Also, barring corrupted installs, these files are guaranteed to exist on disk so there is no need for RAR to do any I/O on them (file-exists, get-time-stamp, ..)

Changes Made
Tweaked RAR to trust the item metadata passed by the SDK. The feature is gated on the presence of the FrameworkReferenceName metadatum which is not documented and is used only by the relevant SDK tasks, to my best knowledge.

SDK does not specify the Culture component of assembly names so it's assumed to be neutral. This has passed all relevant validation so far. If non-neutral assemblies are ever used as references, we'll need to define a metadatum for SDK to set.

Testing
Existing and new unit tests.
Experimental insertion with an assert that assembly names calculated from item metadata are equivalent to those extracted from assembly files (verifies that the Culture=neutral assumption is correct).
Checked all assemblies specified in all FrameworkList.xml files shipped in the SDK (+ workloads) and verified that all of them are culture neutral.
Perf results
RAR micro-benchmark where the task is invoked with parameters corresponding to building a simple AspNetCore app:

13.13 ms -> 3.27 ms per invocation without StateFile cache.
15.03 ms -> 5.13 ms per invocation with StateFile cache.
RAR task duration as reported with /clp:PerformanceSummary when building a simple AspNetCore app with MSBuild server enabled:

20 ms -> 10 ms.
Notes
The change is behind a 17.8 changewave.
No changes have been made to per-project caching (via the StateFile parameter) to reduce scope. Changes to the per-project cache file will be done in another PR.
@AR-May AR-May added the triaged label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment