Skip to content

Commit

Permalink
Use static CoreClrAssemblyLoader for SDK resolvers (#6864)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
marcin-krystianc authored Sep 20, 2021
1 parent cb055d2 commit 57f14a7
Show file tree
Hide file tree
Showing 2 changed files with 6 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ internal class ProjectCacheService
/// i.e. falling back to FileSystem.Default.
/// </summary>
private sealed class DefaultMSBuildFileSystem : MSBuildFileSystemBase { }

// Use NullableBool to make it work with Interlock.CompareExchange (doesn't accept bool?).
// Assume that if one request is a design time build, all of them are.
// Volatile because it is read by the BuildManager thread and written by one project cache service thread pool thread.
Expand Down Expand Up @@ -195,7 +195,7 @@ Assembly LoadAssembly(string resolverPath)
#if !FEATURE_ASSEMBLYLOADCONTEXT
return Assembly.LoadFrom(resolverPath);
#else
return _loader.LoadFromPath(resolverPath);
return s_loader.LoadFromPath(resolverPath);
#endif
}

Expand All @@ -213,7 +213,7 @@ IEnumerable<Type> GetTypes<T>(Assembly assembly)
}

#if FEATURE_ASSEMBLYLOADCONTEXT
private static readonly CoreClrAssemblyLoader _loader = new CoreClrAssemblyLoader();
private static readonly CoreClrAssemblyLoader s_loader = new CoreClrAssemblyLoader();
#endif

public void PostCacheRequest(CacheRequest cacheRequest)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ namespace Microsoft.Build.BackEnd.SdkResolution
internal class SdkResolverLoader
{
#if FEATURE_ASSEMBLYLOADCONTEXT
private readonly CoreClrAssemblyLoader _loader = new CoreClrAssemblyLoader();
private static readonly CoreClrAssemblyLoader s_loader = new CoreClrAssemblyLoader();
#endif

private readonly string IncludeDefaultResolver = Environment.GetEnvironmentVariable("MSBUILDINCLUDEDEFAULTSDKRESOLVER");
Expand All @@ -35,7 +35,7 @@ internal class SdkResolverLoader
internal virtual IList<SdkResolver> LoadResolvers(LoggingContext loggingContext,
ElementLocation location)
{
var resolvers = !String.Equals(IncludeDefaultResolver, "false", StringComparison.OrdinalIgnoreCase) ?
var resolvers = !String.Equals(IncludeDefaultResolver, "false", StringComparison.OrdinalIgnoreCase) ?
new List<SdkResolver> {new DefaultSdkResolver()}
: new List<SdkResolver>();

Expand Down Expand Up @@ -192,7 +192,7 @@ protected virtual Assembly LoadResolverAssembly(string resolverPath, LoggingCont
#if !FEATURE_ASSEMBLYLOADCONTEXT
return Assembly.LoadFrom(resolverPath);
#else
return _loader.LoadFromPath(resolverPath);
return s_loader.LoadFromPath(resolverPath);
#endif
}

Expand Down

0 comments on commit 57f14a7

Please sign in to comment.