-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Workload resolver caching #16414
Workload resolver caching #16414
Changes from all commits
4588742
d32d9f4
76ac9dd
ce483b3
a86a56c
d0d06dc
aaf7028
4da9b22
0af475b
9da9305
a1de9fd
77f444c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ | |
using System.IO; | ||
using System.Linq; | ||
using System.Reflection; | ||
using Microsoft.NET.Sdk.WorkloadMSBuildSdkResolver; | ||
|
||
#nullable disable | ||
|
||
|
@@ -19,7 +20,6 @@ namespace Microsoft.DotNet.MSBuildSdkResolver | |
// 2. Nevertheless, in the IDE, project re-evaluation can create new instances for each evaluation. | ||
// | ||
// As such, all state (instance or static) must be guarded against concurrent access/updates. | ||
// Caches of minimum versions, compatible SDKs are static to benefit multiple IDE evaluations. | ||
// VSSettings are also effectively static (singleton instance that can be swapped by tests). | ||
|
||
public sealed class DotNetMSBuildSdkResolver : SdkResolver | ||
|
@@ -32,6 +32,8 @@ public sealed class DotNetMSBuildSdkResolver : SdkResolver | |
private readonly Func<string, string> _getEnvironmentVariable; | ||
private readonly NETCoreSdkResolver _netCoreSdkResolver; | ||
|
||
private static CachingWorkloadResolver _staticWorkloadResolver = new CachingWorkloadResolver(); | ||
|
||
public DotNetMSBuildSdkResolver() | ||
: this(Environment.GetEnvironmentVariable, VSSettings.Ambient) | ||
{ | ||
|
@@ -44,40 +46,49 @@ public DotNetMSBuildSdkResolver(Func<string, string> getEnvironmentVariable, VSS | |
_netCoreSdkResolver = new NETCoreSdkResolver(getEnvironmentVariable, vsSettings); | ||
} | ||
|
||
private sealed class CachedResult | ||
private sealed class CachedState | ||
{ | ||
public string DotnetRoot; | ||
public string MSBuildSdksDir; | ||
public string NETCoreSdkVersion; | ||
public IDictionary<string, string> PropertiesToAdd; | ||
public CachingWorkloadResolver WorkloadResolver; | ||
} | ||
|
||
public override SdkResult Resolve(SdkReference sdkReference, SdkResolverContext context, SdkResultFactory factory) | ||
{ | ||
string dotnetRoot = null; | ||
string msbuildSdksDir = null; | ||
string netcoreSdkVersion = null; | ||
IDictionary<string, string> propertiesToAdd = null; | ||
IDictionary<string, SdkResultItem> itemsToAdd = null; | ||
List<string> warnings = null; | ||
CachingWorkloadResolver workloadResolver = null; | ||
|
||
if (context.State is CachedResult priorResult) | ||
if (context.State is CachedState priorResult) | ||
{ | ||
dotnetRoot = priorResult.DotnetRoot; | ||
msbuildSdksDir = priorResult.MSBuildSdksDir; | ||
netcoreSdkVersion = priorResult.NETCoreSdkVersion; | ||
propertiesToAdd = priorResult.PropertiesToAdd; | ||
workloadResolver = priorResult.WorkloadResolver; | ||
} | ||
|
||
if (msbuildSdksDir == null) | ||
if (context.IsRunningInVisualStudio) | ||
{ | ||
// These are overrides that are used to force the resolved SDK tasks and targets to come from a given | ||
// base directory and report a given version to msbuild (which may be null if unknown. One key use case | ||
// for this is to test SDK tasks and targets without deploying them inside the .NET Core SDK. | ||
msbuildSdksDir = _getEnvironmentVariable("DOTNET_MSBUILD_SDK_RESOLVER_SDKS_DIR"); | ||
netcoreSdkVersion = _getEnvironmentVariable("DOTNET_MSBUILD_SDK_RESOLVER_SDKS_VER"); | ||
workloadResolver = _staticWorkloadResolver; | ||
} | ||
|
||
if (workloadResolver == null) | ||
{ | ||
workloadResolver = new CachingWorkloadResolver(); | ||
} | ||
|
||
if (msbuildSdksDir == null) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: why do we have the same condition twice? Can we combine this and line 139? |
||
{ | ||
string dotnetExeDir = _netCoreSdkResolver.GetDotnetExeDirectory(); | ||
dotnetRoot = _netCoreSdkResolver.GetDotnetExeDirectory(); | ||
string globalJsonStartDir = Path.GetDirectoryName(context.SolutionFilePath ?? context.ProjectFilePath); | ||
var resolverResult = _netCoreSdkResolver.ResolveNETCoreSdkDirectory(globalJsonStartDir, context.MSBuildVersion, context.IsRunningInVisualStudio, dotnetExeDir); | ||
var resolverResult = _netCoreSdkResolver.ResolveNETCoreSdkDirectory(globalJsonStartDir, context.MSBuildVersion, context.IsRunningInVisualStudio, dotnetRoot); | ||
|
||
if (resolverResult.ResolvedSdkDirectory == null) | ||
{ | ||
|
@@ -89,6 +100,20 @@ public override SdkResult Resolve(SdkReference sdkReference, SdkResolverContext | |
msbuildSdksDir = Path.Combine(resolverResult.ResolvedSdkDirectory, "Sdks"); | ||
netcoreSdkVersion = new DirectoryInfo(resolverResult.ResolvedSdkDirectory).Name; | ||
|
||
// These are overrides that are used to force the resolved SDK tasks and targets to come from a given | ||
// base directory and report a given version to msbuild (which may be null if unknown. One key use case | ||
// for this is to test SDK tasks and targets without deploying them inside the .NET Core SDK. | ||
var msbuildSdksDirFromEnv = _getEnvironmentVariable("DOTNET_MSBUILD_SDK_RESOLVER_SDKS_DIR"); | ||
var netcoreSdkVersionFromEnv = _getEnvironmentVariable("DOTNET_MSBUILD_SDK_RESOLVER_SDKS_VER"); | ||
if (!string.IsNullOrEmpty(msbuildSdksDirFromEnv)) | ||
{ | ||
msbuildSdksDir = msbuildSdksDirFromEnv; | ||
} | ||
if (!string.IsNullOrEmpty(netcoreSdkVersionFromEnv)) | ||
{ | ||
netcoreSdkVersion = netcoreSdkVersionFromEnv; | ||
} | ||
|
||
if (IsNetCoreSDKSmallerThanTheMinimumVersion(netcoreSdkVersion, sdkReference.MinimumVersion)) | ||
{ | ||
return Failure( | ||
|
@@ -135,12 +160,23 @@ public override SdkResult Resolve(SdkReference sdkReference, SdkResolverContext | |
} | ||
} | ||
|
||
context.State = new CachedResult | ||
context.State = new CachedState | ||
{ | ||
DotnetRoot = dotnetRoot, | ||
MSBuildSdksDir = msbuildSdksDir, | ||
NETCoreSdkVersion = netcoreSdkVersion | ||
NETCoreSdkVersion = netcoreSdkVersion, | ||
PropertiesToAdd = propertiesToAdd, | ||
WorkloadResolver = workloadResolver | ||
}; | ||
|
||
// First check if requested SDK resolves to a workload SDK pack | ||
var workloadResult = workloadResolver.Resolve(sdkReference.Name, dotnetRoot, netcoreSdkVersion); | ||
|
||
if (workloadResult is not CachingWorkloadResolver.NullResolutionResult) | ||
{ | ||
return workloadResult.ToSdkResult(sdkReference, factory); | ||
} | ||
|
||
string msbuildSdkDir = Path.Combine(msbuildSdksDir, sdkReference.Name, "Sdk"); | ||
if (!Directory.Exists(msbuildSdkDir)) | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,226 @@ | ||
// Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
// Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
||
using Microsoft.Build.Framework; | ||
using System; | ||
using System.Collections.Generic; | ||
using System.Diagnostics; | ||
using System.IO; | ||
using System.Linq; | ||
using Microsoft.NET.Sdk.WorkloadManifestReader; | ||
using System.Collections.Immutable; | ||
|
||
#if NET | ||
using Microsoft.DotNet.Cli; | ||
#else | ||
using Microsoft.DotNet.DotNetSdkResolver; | ||
#endif | ||
|
||
#nullable disable | ||
|
||
namespace Microsoft.NET.Sdk.WorkloadMSBuildSdkResolver | ||
{ | ||
|
||
// This class contains workload SDK resolution logic which will be used by both .NET SDK MSBuild and Full Framework / Visual Studio MSBuild. | ||
// | ||
// Keeping this performant in Visual Studio is tricky, as VS performs a lot of evaluations, but they are not linked by an MSBuild "Submission ID", | ||
// so the state caching support provided by MSBuild for SDK Resolvers doesn't really help. Additionally, multiple instances of the SDK resolver | ||
// may be created, and the same instance may be called on multiple threads. So state needs to be cached staticly and be thread-safe. | ||
// | ||
// To keep the state static, the MSBuildSdkResolver keeps a static reference to the CachingWorkloadResolver that is used if the build is inside | ||
// Visual Studio. To keep it thread-safe, the body of the Resolve method is all protected by a lock statement. This avoids having to make | ||
// the classes consumed by the CachingWorkloadResolver (the manifest provider and workload resolver) thread-safe. | ||
// | ||
// A resolver should not over-cache and return out-of-date results. For workloads, the resolution could change due to: | ||
// - Installation, update, or uninstallation of a workload | ||
// - Resolved SDK changes (either due to an SDK installation or uninstallation, or a global.json change) | ||
// For SDK or workload installation actions, we expect to be running under a new process since Visual Studio will have been restarted. | ||
// For global.json changes, the Resolve method takes parameters for the dotnet root and the SDK version. If those values have changed | ||
// from the previous call, the cached state will be thrown out and recreated. | ||
class CachingWorkloadResolver | ||
{ | ||
private sealed record CachedState | ||
{ | ||
public string DotnetRootPath { get; init; } | ||
public string SdkVersion { get; init; } | ||
public IWorkloadManifestProvider ManifestProvider { get; init; } | ||
public IWorkloadResolver WorkloadResolver { get; init; } | ||
public ImmutableDictionary<string, ResolutionResult> CachedResults { get; init; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Originally this code was supposed to be fully thread-safe, using immutable object patterns. It's probably not necessary anymore now that everything happens in a lock. But if it's not a big deal perf-wise it seems cleaner to leave it as is. |
||
|
||
public CachedState() | ||
{ | ||
CachedResults = ImmutableDictionary.Create<string, ResolutionResult>(StringComparer.OrdinalIgnoreCase); | ||
} | ||
} | ||
|
||
public object _lockObject { get; } = new object(); | ||
private CachedState _cachedState; | ||
private readonly bool _enabled; | ||
|
||
|
||
public CachingWorkloadResolver() | ||
{ | ||
// Support opt-out for workload resolution | ||
_enabled = true; | ||
var envVar = Environment.GetEnvironmentVariable("MSBuildEnableWorkloadResolver"); | ||
if (envVar != null) | ||
{ | ||
if (envVar.Equals("false", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
_enabled = false; | ||
} | ||
} | ||
|
||
if (_enabled) | ||
{ | ||
string sentinelPath = Path.Combine(Path.GetDirectoryName(typeof(CachingWorkloadResolver).Assembly.Location), "DisableWorkloadResolver.sentinel"); | ||
if (File.Exists(sentinelPath)) | ||
{ | ||
_enabled = false; | ||
} | ||
} | ||
} | ||
|
||
public record ResolutionResult() | ||
{ | ||
public SdkResult ToSdkResult(SdkReference sdkReference, SdkResultFactory factory) | ||
{ | ||
switch (this) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. super-nit: Would this be prettier with virtual methods instead of switching on type? My experience with records is limited, feel free to tell me that this is a common pattern and I'll shut up :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know, this was my first time using some new C# language features :). My impression is that this is a standard pattern in functional programming. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if objects do not have state. using virtual methods or pattern matching is pretty much the same thing. However, I prefer OO style when available just for the consistency. Plus even with a better pattern matching and record. C# is still hard to do functional style programming without discriminated union and other convenient syntax. You end up with generic inside generic and pattern matching all the time for few lines of F#. See https://www.manning.com/books/functional-programming-in-c-sharp . |
||
{ | ||
case SinglePathResolutionResult r: | ||
return factory.IndicateSuccess(r.Path, sdkReference.Version); | ||
case MultiplePathResolutionResult r: | ||
return factory.IndicateSuccess(r.Paths, sdkReference.Version); | ||
case EmptyResolutionResult r: | ||
return factory.IndicateSuccess(Enumerable.Empty<string>(), sdkReference.Version, r.propertiesToAdd, r.itemsToAdd); | ||
case NullResolutionResult: | ||
return null; | ||
} | ||
|
||
throw new InvalidOperationException("Unknown resolutionResult type: " + this.GetType()); | ||
} | ||
} | ||
|
||
public sealed record SinglePathResolutionResult( | ||
string Path | ||
) : ResolutionResult; | ||
|
||
public sealed record MultiplePathResolutionResult( | ||
IEnumerable<string> Paths | ||
) : ResolutionResult; | ||
|
||
public sealed record EmptyResolutionResult( | ||
IDictionary<string, string> propertiesToAdd, | ||
IDictionary<string, SdkResultItem> itemsToAdd | ||
) : ResolutionResult; | ||
|
||
public sealed record NullResolutionResult() : ResolutionResult; | ||
|
||
private static ResolutionResult Resolve(string sdkReferenceName, IWorkloadManifestProvider manifestProvider, IWorkloadResolver workloadResolver) | ||
{ | ||
if (sdkReferenceName.Equals("Microsoft.NET.SDK.WorkloadAutoImportPropsLocator", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
List<string> autoImportSdkPaths = new List<string>(); | ||
foreach (var sdkPackInfo in workloadResolver.GetInstalledWorkloadPacksOfKind(WorkloadPackKind.Sdk)) | ||
{ | ||
string sdkPackSdkFolder = Path.Combine(sdkPackInfo.Path, "Sdk"); | ||
string autoImportPath = Path.Combine(sdkPackSdkFolder, "AutoImport.props"); | ||
if (File.Exists(autoImportPath)) | ||
{ | ||
autoImportSdkPaths.Add(sdkPackSdkFolder); | ||
} | ||
} | ||
// Call Distinct() here because with aliased packs, there may be duplicates of the same path | ||
return new MultiplePathResolutionResult(autoImportSdkPaths.Distinct()); | ||
} | ||
else if (sdkReferenceName.Equals("Microsoft.NET.SDK.WorkloadManifestTargetsLocator", StringComparison.OrdinalIgnoreCase)) | ||
{ | ||
List<string> workloadManifestPaths = new List<string>(); | ||
foreach (var manifestDirectory in manifestProvider.GetManifestDirectories()) | ||
{ | ||
var workloadManifestTargetPath = Path.Combine(manifestDirectory, "WorkloadManifest.targets"); | ||
if (File.Exists(workloadManifestTargetPath)) | ||
{ | ||
workloadManifestPaths.Add(manifestDirectory); | ||
} | ||
} | ||
return new MultiplePathResolutionResult(workloadManifestPaths); | ||
} | ||
else | ||
{ | ||
var packInfo = workloadResolver.TryGetPackInfo(sdkReferenceName); | ||
if (packInfo != null) | ||
{ | ||
if (Directory.Exists(packInfo.Path)) | ||
{ | ||
return new SinglePathResolutionResult(Path.Combine(packInfo.Path, "Sdk")); | ||
} | ||
else | ||
{ | ||
var itemsToAdd = new Dictionary<string, SdkResultItem>(); | ||
itemsToAdd.Add("MissingWorkloadPack", | ||
new SdkResultItem(sdkReferenceName, | ||
metadata: new Dictionary<string, string>() | ||
{ | ||
{ "Version", packInfo.Version } | ||
})); | ||
|
||
Dictionary<string, string> propertiesToAdd = new Dictionary<string, string>(); | ||
return new EmptyResolutionResult(propertiesToAdd, itemsToAdd); | ||
} | ||
} | ||
} | ||
return new NullResolutionResult(); | ||
} | ||
|
||
public ResolutionResult Resolve(string sdkReferenceName, string dotnetRootPath, string sdkVersion) | ||
{ | ||
if (!_enabled) | ||
{ | ||
return new NullResolutionResult(); | ||
} | ||
|
||
ResolutionResult resolutionResult; | ||
|
||
lock (_lockObject) | ||
{ | ||
if (_cachedState == null || | ||
_cachedState.DotnetRootPath != dotnetRootPath || | ||
_cachedState.SdkVersion != sdkVersion) | ||
{ | ||
var workloadManifestProvider = new SdkDirectoryWorkloadManifestProvider(dotnetRootPath, sdkVersion); | ||
var workloadResolver = WorkloadResolver.Create(workloadManifestProvider, dotnetRootPath, sdkVersion); | ||
|
||
_cachedState = new CachedState() | ||
{ | ||
DotnetRootPath = dotnetRootPath, | ||
SdkVersion = sdkVersion, | ||
ManifestProvider = workloadManifestProvider, | ||
WorkloadResolver = workloadResolver | ||
}; | ||
} | ||
|
||
if (!_cachedState.CachedResults.TryGetValue(sdkReferenceName, out resolutionResult)) | ||
{ | ||
resolutionResult = Resolve(sdkReferenceName, _cachedState.ManifestProvider, _cachedState.WorkloadResolver); | ||
|
||
_cachedState = _cachedState with | ||
{ | ||
CachedResults = _cachedState.CachedResults.Add(sdkReferenceName, resolutionResult) | ||
}; | ||
} | ||
} | ||
|
||
return resolutionResult; | ||
} | ||
} | ||
} | ||
|
||
|
||
// Add attribute to support init-only properties on .NET Framework | ||
#if !NET | ||
namespace System.Runtime.CompilerServices | ||
{ | ||
public class IsExternalInit { } | ||
} | ||
#endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Looks like this is unused if not running in VS. Initialize lazily?