Workload resolver caching #16414
Workload resolver caching #16414
Conversation
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
|
Looks like using the state object that MSBuild passes in the context isn't going to be enough. I added logging for when the existing SDK resolver was called, and when loading the SDK solution from this repo in VS, there were 346 calls to the resolver where the state passed in was null, and only 38 where it was non-null. |
|
Fixes dotnet/msbuild#6060 |
I don't think this actually fixes dotnet/msbuild#6060, rather it works around it to not make the problem any worse from adding new resolver logic. |
dotnet/msbuild#6060 was intended to address the problem of having to look up the sdk for each project in a solution and your fix looks like it targets that. Is there any residue left over that will need to be addressed in dotnet/msbuild#6060? |
Directory.Build.props
Outdated
| @@ -17,6 +17,10 @@ | |||
| <DisableImplicitPackageTargetFallback>true</DisableImplicitPackageTargetFallback> | |||
|
|
|||
| <!-- <ArtifactsShippingSymbolsDir>$(ArtifactsDir)symbols\$(Configuration)\Shipping</ArtifactsShippingSymbolsDir> --> | |||
|
|
|||
| <!-- Flag to enable using Serilog for debug logging for how SDK resolvers are called --> | |||
| <EnableSerilogLogging>false</EnableSerilogLogging> | |||
There was a problem hiding this comment.
The only usage is during development? You build with this flag enabled and then copy paste to VS, run it manually and see the result?
| } | ||
| } | ||
|
|
||
| #if !NET |
There was a problem hiding this comment.
This is an attribute which allows using init-only properties on .NET Framework.
| ); | ||
|
|
||
|
|
||
| #if USE_SERILOG |
There was a problem hiding this comment.
Any way to make this "more repeatable"? If another dev wants to improve the performance again. How should the new dev know what these means and how these properties should be used? What is the existing base line?
It is hard to make a unit test. But could you have a write up on how to use these logs? Something like you need to enable EnableSerilogLogging, copy the files over, and when running in VS then we should expect MSBuildSubmissionId to be the same all the time?
|
|
||
| #nullable disable | ||
|
|
||
| namespace Microsoft.NET.Sdk.WorkloadMSBuildSdkResolver |
There was a problem hiding this comment.
300 lines without unit test coverage. We should be able to test cache save and take.
| { | ||
| return null; | ||
| } | ||
| #if USE_SERILOG |
|
|
||
| if (workloadResolver == null) | ||
| { | ||
| workloadResolver = new WorkloadPartialResolver(); | ||
| } | ||
|
|
||
| if (msbuildSdksDir == null) |
There was a problem hiding this comment.
nit: why do we have the same condition twice? Can we combine this and line 139?
| <!-- To reduce dll load (cause RPS perf regression). Directly compile files from Microsoft.DotNet.SdkResolver, and the workload resolver --> | ||
| <ItemGroup> | ||
| <Compile Include="..\Microsoft.DotNet.SdkResolver\**\*.cs" LinkBase="Microsoft.DotNet.SdkResolver" /> | ||
| <Compile Include="..\Microsoft.NET.Sdk.WorkloadMSBuildSdkResolver\**\*.cs" LinkBase="Microsoft.NET.Sdk.WorkloadMSBuildSdkResolver" /> | ||
| <Compile Include="..\Microsoft.NET.Sdk.WorkloadMSBuildSdkResolver\WorkloadPartialResolver.cs" LinkBase="Microsoft.NET.Sdk.WorkloadMSBuildSdkResolver" /> |
There was a problem hiding this comment.
Why do we only need to include the partial resolver now? the other files aren't needed in VS?
There was a problem hiding this comment.
For VS / Full Framework MSBuild, we now only use one MSBuild workload resolver, which handles both the existing SDK resolution as well as resolving workloads. The WorkloadPartialResolver (which I'm renaming to CachingWorkloadResolver) contains the workload resolution logic that will be shared between the .NET (Core) and .NET Framework resolvers.
dsplaisted
force-pushed
the
workload-resolver-caching
branch
from
898d9eb
to
508a202
Compare
| @@ -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(); | |||
There was a problem hiding this comment.
nit: Looks like this is unused if not running in VS. Initialize lazily?
| // from the previous call, the cached state will be thrown out and recreated. | ||
| class CachingWorkloadResolver | ||
| { | ||
| private record CachedState |
| } | ||
| } | ||
|
|
||
| if (!_enabled) |
There was a problem hiding this comment.
The condition looks reversed.
| if (!_enabled) | |
| if (_enabled) |
| } | ||
| } | ||
|
|
||
| public record SinglePathResolutionResult( |
There was a problem hiding this comment.
nit: sealed (applies to all result records).
| 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.
Why is this an ImmutableDictionary as opposed to a regular Dictionary?
There was a problem hiding this comment.
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 SdkResult ToSdkResult(SdkReference sdkReference, SdkResultFactory factory) | ||
| { | ||
| switch (this) |
There was a problem hiding this comment.
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.
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.
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 .
|
|
||
| public object _lockObject { get; } = new object(); | ||
| private CachedState _cachedState; | ||
| private bool _enabled; |
dsplaisted
force-pushed
the
workload-resolver-caching
branch
from
508a202
to
77f444c
Compare
Co-authored-by: Matt Thalman <[email protected]>
Add caching to workload resolver results, and enable workload resolver by default.
Addresses the first two items from #13849.
Rolls the workload resolution into the MSBuildSdkResolver, so that it doesn't have to duplicate the lookup of which SDK to use.
The built-in MSBuild SDK Resolver state caching doesn't work well in Visual Studio, as there are a lot of evaluations which aren't linked with a build submission ID, which is what MSBuild uses to keep resolver state. So if called from Visual Studio, the resolver keeps a single static copy of the workload resolution state, which invalidates its cache if the resolved SDK changes.