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.

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

Lower priority of NuGet-based MSBuild project SDK resolver to run after .NET SDK resolver #4443

Merged
merged 1 commit into from
Feb 9, 2022

Conversation

jeffkl
Copy link
Contributor

@jeffkl jeffkl commented Feb 4, 2022

Bug

Fixes: NuGet/Home#11566

Regression? Last working version:

Description

Lowering the priority of the NuGet-based MSBuild project SDK resolver will allow built-in SDKs like Microsoft.NET.Sdk to be resolved quicker in Visual Studio and command-line builds. This will break any customer who is overriding the built-in SDKs with a NuGet package, but we believe it's worth breaking this functionality for the benefit of performance. At this time, there is no known customer who is doing this either.

The .NET SDK resolver's priority is 5000:
https://github.com/dotnet/sdk/blob/04d33f15114be2b5180c01124618e44e40403e44/src/Resolvers/Microsoft.DotNet.MSBuildSdkResolver/MSBuildSdkResolver.cs#L31

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation
    I'll need to update this page once this is merged to reflect the new ordering.

    • Documentation PR or issue filled
    • OR
    • N/A

@jeffkl jeffkl requested a review from a team as a code owner February 4, 2022 23:48
@jeffkl
Copy link
Contributor Author

jeffkl commented Feb 4, 2022

I also did an experimental insertion here: https://devdiv.visualstudio.com/DevDiv/_git/VS/pullrequest/378019?_a=overview

I installed the build, did some manual verification, and everything works

@jeffkl
Copy link
Contributor Author

jeffkl commented Feb 4, 2022

/cc @davkean

@zivkan
Copy link
Member

zivkan commented Feb 5, 2022

Are the perf changes measurable? For example, what's the perf difference for OrchardCore (or another solution with a LARGE number of projects) when running a target that doesn't exist, or a no-op restore?

@jeffkl
Copy link
Contributor Author

jeffkl commented Feb 5, 2022

Are the perf changes measurable? For example, what's the perf difference for OrchardCore (or another solution with a LARGE number of projects) when running a target that doesn't exist, or a no-op restore?

The performance increase is going to be in evaluation of projects which I should be able to measure with some effort. Command-line based builds shouldn't really be that much faster but loading large solutions in Visual Studio that don't use NuGet-based MSBuild project SDKs should be better depending on:

  1. If the MSBuild SDK resolution APIs are performant (the calls to SDK resolvers could be improved so we're experimenting with making that faster)
  2. The .NET SDK resolver which will now run before the NuGet SDK resolver has to be performant (it uses native code to parse global.json but suffers from some of the thread safety issues where it might be parsing the file a lot more than expected)

I think the MSBuild repo has some tooling to generate performance numbers, I can look into how to get this change into a state where I can run their suite. I also already checked in another change to make the NuGet SDK resolver better so that might skew the numbers. But this does speak to the fact that I'm going to have to do some quantitative measurements of 17.1 vs 17.2 to show if this work made improvements and if so, how much.

@zivkan
Copy link
Member

zivkan commented Feb 7, 2022

The performance increase is going to be in evaluation of projects which I should be able to measure with some effort.

If it requires effort (more so than just Measure-Command { & .\patched\dotnet msbuild -t:DoesNotExist }), or the Measure-Command test doesn't show any perf improvement, that suggests to me that it won't make a noticeable difference to customers. In VS where there's a lot of other things happening as well, it might be more impactful, but I'm just curious at a "high level" if it's going to make CI builds just a tiny bit faster or not.

@jeffkl
Copy link
Contributor Author

jeffkl commented Feb 7, 2022

If it requires effort (more so than just Measure-Command { & .\patched\dotnet msbuild -t:DoesNotExist }), or the Measure-Command test doesn't show any perf improvement, that suggests to me that it won't make a noticeable difference to customers. In VS where there's a lot of other things happening as well, it might be more impactful, but I'm just curious at a "high level" if it's going to make CI builds just a tiny bit faster or not.

Command-line based builds are fairly optimized where SDK resolvers are only called once per unique SDK. The bigger gain will definitely come in Visual Studio if:

  1. You have a large solution with lots of projects
  2. You are not using any NuGet-based MSBuild project SDKs

To really test this, I would need a large solution that only uses .NET SDKs, load it in VS, and measure the time it took to do all of the design-time builds (really just looking at evaluations since DTB are fast). I do expect this scenario to be faster with a bigger payoff for larger solutions. DDRITs on my experimental insertion did pass so its definitely not slower. Maybe I can look at those results to get a decent measurement?

@jeffkl jeffkl merged commit c29f95c into dev Feb 9, 2022
@jeffkl jeffkl deleted the dev-jeffkl-lower-nugetsdkresolver-priority branch February 9, 2022 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DCR]: NuGet-based MSBuild project SDK resolver should run after .NET SDK resolver
4 participants