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.

Sign up for GitHub

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

Jump to bottom

SDK Resolver assemblies leak AssemblyLoadContexts #6842

Closed
rainersigwald opened this issue Sep 13, 2021 · 4 comments · Fixed by #6864
Closed

SDK Resolver assemblies leak AssemblyLoadContexts #6842

rainersigwald opened this issue Sep 13, 2021 · 4 comments · Fixed by #6864
Assignees
@marcin-krystianc
Labels
Area: API performance triaged
Milestone
VS 17.0

Comments

@rainersigwald
Copy link
Member

The SDK resolver loader should cache by assembly path much like the task loader does, and evidently does not today:

Originally posted by @marcin-krystianc in #5037 (comment)

New ALC are created because SdkResolvers need to be loaded for each evaluated project.
There is a cache for them called CachingSdkResolverService (in the EvaluationContext) so they are loaded only once per each instance of that cache. If it was possible to use single EvaluationContext for the whole lifetime of our long running service that would fix the leak from ALCs, unfortunately it is not possible as it creates other memory leaks (e.g. from the EngineFileUtilities).
@rainersigwald rainersigwald added this to the 17.0 milestone Sep 13, 2021
@rainersigwald rainersigwald mentioned this issue Sep 13, 2021
Closed
3 tasks
@marcin-krystianc
Copy link
Contributor

One possible fix would be to make the CoreClrAssemblyLoader in the SdkResolverLoader to be a static field (https://github.com/dotnet/msbuild/blob/main/src/Build/BackEnd/Components/SdkResolution/SdkResolverLoader.cs#L20). Is it what you have in mind ?

@rainersigwald
Copy link
Member Author

@marcin-krystianc Yes, I think that would do the trick. It looks like the other uses are in statics:

msbuild/src/Shared/TypeLoader.cs

Line 24 in f566ba1

private static readonly CoreClrAssemblyLoader s_coreClrAssemblyLoader;

msbuild/src/Build/BackEnd/Components/ProjectCache/ProjectCacheService.cs

Line 216 in f566ba1

private static readonly CoreClrAssemblyLoader _loader = new CoreClrAssemblyLoader();

Are you interested in sending a PR to fix this?

@marcin-krystianc marcin-krystianc mentioned this issue Sep 17, 2021
Merged
@marcin-krystianc
Copy link
Contributor

Are you interested in sending a PR to fix this?

Done in #6864

@rainersigwald
Copy link
Member Author

Awesome, thank you!

AR-May pushed a commit that referenced this issue Sep 20, 2021
@marcin-krystianc
Use static CoreClrAssemblyLoader for SDK resolvers (#6864)
57f14a7 
We want to use static for CoreClrAssemblyLoader so each unique SDK resolver assembly is loaded into memory and JITted only once.

Fixes #6842 (comment)

### Context

We use static for the `CoreClrAssemblyLoader` field so each unique SDK resolver assembly is loaded into memory and JITted only once. All subsequent load requests will return assembly from the assembly loader's cache instead of loading it again from disk. This change increases the performance of SDK resolution and at the same time avoids leaking memory due to loading the same SDK resolver assembly multiple times and never unloading it.

### Changes Made

The `CoreClrAssemblyLoader` field in the `SdkResolverLoader` class was changed from non-static to static member. The same instance of `CoreClrAssemblyLoader` will be used by all instances of `SdkResolverLoader`. It is consistent now with other uses of `CoreClrAssemblyLoader` in msbuild.

### Testing

Tested manually using repro from #5037 (comment)

### Notes

Alternative approach would be to use collectible `CoreClrAssemblyLoader` / `AssemblyLoadContext` - that would fix the leak as well but it would be less performant as it wouldn't benefit from re-using already loaded and JITed assemblies.
@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
Assignees

@marcin-krystianc marcin-krystianc

Labels
Area: API performance triaged
Projects
None yet
Milestone
VS 17.0
Development

Successfully merging a pull request may close this issue.

Use static CoreClrAssemblyLoader in the SdkResolverLoader marcin-krystianc/msbuild
3 participants
@marcin-krystianc @rainersigwald @AR-May