NuGetSdkResolver adds significant overhead to evaluation and should not be used

[davkean]

For evaluation time, The NuGetSdkResolver is extremely slow.

Based on investigation in CoreFX, I found:

• On restored state, it added 180-400ms overhead to a single project evaluation
• On unrestored state, it added 1000-6000ms overhead to a single project evaluation

I've looked at the original design, and it's built on a incorrect premise; that its performance impact will only affect the loading state of projects that use it. The image it shows where projects are being loaded in the background via Asynchronous Solution Load (ASL) was removed from the product in 15.5.

If a project opts into this resolver, it has the following effects:

• On solution load, blocks interaction with VS until it has completed downloading & resolving the SDK.
• Increases the time it takes for the project-system to send design-time build results to Roslyn, resulting in delayed IntelliSense results
• Increases the time it takes for designers to open
• Increases the time it takes for tests to be discovered
• Increases the time it takes for VS to react to changing the active configuration
• Increases the time and blocks interaction with VS when making changes to a project, such as adding/removing a file
• Increases design-time builds for every project that references it
• Increases the time for above when switching branches

When we designed SDK resolvers, it was explicitly called out that due to them being used during evaluation - that they must be extremely fast and must not hit the network. While this only hits the network on unrestored state, it still has a large negative impact on evaluation time when the package is already downloaded.

This is the entire reason that NuGet restore does not run during the build while inside Visual Studio.

Rogue resolvers can cause Visual Studio and other IDEs to be blamed for performance and UI delay issues, please remove or change the design of this resolver to play nicely with Visual Studio.

[davkean]

Alternatively, I could add a new API to MSBuild that the project system could call to pre-resolve SDKs so they don't happen during evaluation. That would be a bit more work but is entirely possible.

Yes, this is the approach we should be taking - no matter how fast we make NuGet restore, it will never be fast enough.

[CyrusNajmabadi]

I'm trying to get some time to fix the perf issues in the case when the packages are already on disk:

What happens when teh packages are not on disk?

[davkean]

It hits the network.

[jeromelaban]

@CyrusNajmabadi from what I remember, it can also freezes VS. I have to run msbuild /r /t:Restore to fix that.

[CyrusNajmabadi]

ok. so network on hte UI thread basically has to be a complete no-no. for one thing, i have terrible network access. For anotehr, this blocks loading vs. meaning i can't even do any work while this is happening because VS must be killed.

[dsplaisted]

I have a proposal to avoid the network access on the UI thread. See the "NuGet SDK Resolver" section in this proposal: https://github.com/dotnet/designs/pull/104

Basically we are going to update the MSBuild SDK resolver interface so that a resolver can return no resolved SDK paths without failing. A resolver will also be able to return items that should be added to the evaluation.

We can leverage this to fix the issue where the NuGet resolver downloads files on the UI thread. In VS, the resolver should run in a mode that does not allow downloading SDKs. Rather, if it can't find the SDK locally, it should return success and include a MissingMSBuildSDK item describing the SDK it would have downloaded. The VS project system should check for those items on evaluation, and if there are any of them, it should spin off a non-UI thread task to download them.

[marcpopMSFT]

@dsplaisted with the optional workload work getting in, is it now possible for project system to update to handle this differently?

[dsplaisted]

This has been unblocked since the new SDK resolver support went in to MSBuild. We would need to make changes to the NuGet SDK Resolver, and the project system would also need to make changes. It would be nice to do that for 5.0.200 / VS 16.9.

[davkean]

This is another issue caused by it: https://developercommunity.visualstudio.com/t/VisualStudio-was-hang-when-open-the-wpf-/1429662. We have ~57 threads all blocked behind the NuGet resolver.

[davkean]

This is another issue caused by this: https://developercommunity.visualstudio.com/t/vs-performance-issue-ide-hangs-when-changing-file/1643242.

[aortiz-msft]

NuGet Epic to address perf issues in the NuGetSDKResolver: https://github.com/NuGet/Home/issues/11441.

Please feel free to reach out to @jeffkl directly.

[ladipro]

I'm looking at this issue, trying to break it down into units of work across the affected components. A couple of observations:

1. Avoid downloading NuGet packages during evaluation I'm actually not sure if it should be built on top of the new SDK resolver support. If the SDK cannot be resolved, there is no point in continuing evaluation. Instead of defining items on the evaluated project to indicate which NuGet packages need to be restored, the resolver may as well just throw an exception with this info, which the project system would catch. Maybe I'm not understanding the flow correctly, though, will need to follow up.

The work needed here:

• From NuGet SDK resolver:

  • Detect that it's running in VS and it's not allowed to restore. The flag for VS is already there as SdkResolverContext.IsRunningInVisualStudio but presumably we'll want a way to opt-in/out to experiment so we may need an additional flag.
  • If the above holds true, indicate the unresolved package via an exception or via ItemsToAdd in the new SDK interface.
  • Overall, this is a fairly small work item.

• From MSBuild:

  • Depending on the exact mechanism of indicating the need to restore, MSBuild may need to aggregate the information from all resolver invocations. This is because <Project Sdk="Package1/Version1;Package2/Version2"> is perfectly legal, whether intended to or not.
  • Very small amount of work.

• From the project system:

  • This would depend on the desired UX. We may want to have VS automatically restore the missing packages and then retry evaluating the affected project, all within the context of solution load. Or finish loading the solution leaving the affected projects 'unloaded' and then asynchronously pop a dialog? Or take the user directly to the VS NuGet Manager? Or something else. Very much an open question, feedback welcome.

2. Avoid JITting There is indeed some non-trivial JITting cost when a process runs SDK resolution for the first time. For VS, the first project loaded after VS starts pays the price. On command-line it's generally every build (i.e. every MSBuild.exe invocation). On my machine I measured it at 45 ms for Microsoft.Build.NuGetSdkResolver.dll and 85 ms for Microsoft.DotNet.MSBuildSdkResolver.dll. The latter has nothing to do with NuGet SDK resolution but it's always invoked and because it's more expensive to JIT than the NuGet resolver, if we decide to address JITting it would make sense to prioritize fixing the base SDK resolver first.

I have played with prototyping a change to NGEN Microsoft.Build.NuGetSdkResolver.dll + its dependency Microsoft.Deployment.DotNet.Releases and it's not straightforward. MSBuild uses Assembly.LoadFrom, which immediately disqualifies the native image from being loaded. To be able to Assembly.Load by full name, we 1) Need to know the full name and 2) Need add a binding redirect to devenv.exe.config and MSBuild.exe.config so the CLR binder can find the file. For this we need to know the assembly version and in some cases this appears to be hitting layering & versioning unfortunacies, i.e. MSBuild generally doesn't know the specific .NET SDK version it's going to be part of. Not unsolvable but likely P2 in the grand scheme of things.

[dsplaisted]

1. I'm actually not sure if it should be built on top of the new SDK resolver support. If the SDK cannot be resolved, there is no point in continuing evaluation. Instead of defining items on the evaluated project to indicate which NuGet packages need to be restored, the resolver may as well just throw an exception with this info, which the project system would catch. Maybe I'm not understanding the flow correctly, though, will need to follow up.

The resolver needs to communicate this information to Visual Studio. You could probably include it as data on an exception, but adding items seems cleaner to me. Also if you do it via items, then I don't think you need to do the work in MSBuild to aggregate the information about which packages need to be downloaded, as there will just be multiple items with the information from the different resolver invocations.

[ladipro]

True, I think in general it's uglier to rely on structured data attached to an exception. On the other hand, if the resolver fails in this way, conceptually there's no value in evaluating the project. Other than the items added via ItemsToAdd it's pretty much useless. Perf wouldn't be an issue as this an error path. But what I'm kind of debating is whether a dummy ProjectInstance fits the CPS internal model or not. I.e. if it's good for CPS to always receive a valid object or if it's a potential source of confusion downstream. (ex: Would it be good or bad for such a ProjectInstance to be persisted to project system's evaluation cache?)