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

Change Load Behavior for NuGet Components #6126

Merged
merged 2 commits into from
Feb 8, 2021
Merged

Change Load Behavior for NuGet Components #6126

merged 2 commits into from
Feb 8, 2021

Conversation

brianrob
Copy link
Member

@brianrob brianrob commented Feb 5, 2021

Contributes to dotnet/sdk#15558.

Context

In a single MSBuild process, most of the NuGet binaries are being loaded into more than one AssemblyLoadContext. The ready to run code associated with these binaries can only be used in a single AssemblyLoadContext, which means that usage of the assembly in all other contexts must be jitted. This shows up as an extra 736 methods that get jitted (measured as 147.6ms).

Changes Made

  1. Change NuGetFrameworkWrapper to use Assembly.LoadFrom instead of Assembly.LoadFile when using reflection to load NuGet.Frameworks.dll. This ensures that the assembly is not loaded into the NULL loader context, and unifies the load with later loads of the same assembly.
  2. Add all of the NuGet assemblies to the list of well-known assemblies in MSBuildLoadContext, forcing them to all be loaded in the default AssemblyLoadContext. This is an important part of the fix because these loads occur after the load that occurs in NuGetFrameworkWrapper, and so by MSBuild taking a dependency on these NuGet assemblies, they should be treated as part of MSBuild itself, and not as part of a task using the task loader behavior.

Testing

  • Verified that basic build scenarios work.
  • Used COMPlus_ReadyToRunLogFile to confirm that R2R code is not rejected due to multiple loads in different AssemblyLoadContexts.
  • Captured before/after profiles to confirm wins.

Notes

This PR contains the following performance wins:

  • -736 drop in method jitting overall.
  • 147.6ms improvement in JIT time (15.5%)
  • 5.1% wall-clock latency improvement overall

cc: @stephentoub, @DamianEdwards, @marcpopMSFT

@brianrob brianrob requested a review from ladipro February 5, 2021 00:32
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Nice!

@brianrob
Copy link
Member Author

brianrob commented Feb 8, 2021

Thanks for the review @Forgind. Can you merge at your convenience? I don't appear to have permissions.

Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Thank you!

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Feb 8, 2021
@Forgind Forgind merged commit bba8fa9 into dotnet:master Feb 8, 2021
@brianrob brianrob deleted the nuget-load-context branch February 8, 2021 18:09
@ericstj
Copy link
Member

ericstj commented Feb 23, 2021

This introduced regressions in our build tasks. This is sharing NuGet unconditionally regardless of version which will pin NuGet to the version in the SDK. It’s also causing problems because NewtonsoftJson is not pinned but is a dependency of NuGet. This causes Newtonsoft to load in the tasks ALC and it doesn’t unify with the one used by NuGet even if the same version. This results in Type loading issues (method not found in the case we saw). Why not instead remove NuGet from the tasks which are carrying extra copies to achieve this perf improvement? That will have the added benefit of reducing disk footprint, assuming this unification is indeed legitimate.

Barring that it seems like some additional work needs to be done to selectively reuse based on some version rules and ensure the entire assembly closure will be reused, including Newtonsoft.Json.

@brianrob brianrob mentioned this pull request Mar 25, 2021
Forgind pushed a commit that referenced this pull request Mar 31, 2021
Fixes #6289

Context
#6126 changes the way that NuGet.Frameworks.dll is loaded to be used by NuGetFrameworkWrapper from a call to LoadFile to a call to LoadFrom. The path passed in to the Load*` call is the same in both cases, but the behavior is different when running on .NET Framework.

On .NET Framework, calling LoadFile results in IJW loading rules being followed, and a different copy of NuGet.Frameworks.dll
than the one requested being loaded. It also appears to match the one loaded into the LOAD context. Calling LoadFrom results in the specified assembly being loaded, but it doesn't match the copy of the assembly loaded into the LOAD context. Thus, it remains in the LOADFROM context instead of being promoted to the LOAD context.

Later on, there is a collision between code from the two instances of NuGet.Frameworks.dll that are loaded into the LOAD context and the LOADFROM context, and this is where the MissingMethodException is thrown.

Note, this does not happen on the .NET Core bootstrap build because the loader behavior is significantly simpler.

Changes Made
Choose the Load* API based on the target framework at build time. On .NET Core, use LoadFrom and on .NET Framework, use LoadFile. This type of precedent already exists in MSBuild where there is different load behavior for .NET Framework and .NET Core.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants