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

Make noop restore faster - speed up evaluations by calling MSBuild Static Graph apis #8791

Closed
jeffkl opened this issue Nov 4, 2019 · 26 comments · Fixed by NuGet/NuGet.Client#3109
Assignees
Labels
Partner:1ES Tenet:Performance Performance issues Type:DCR Design Change Request

Comments

@jeffkl
Copy link
Contributor

jeffkl commented Nov 4, 2019

Details about Problem

In MSBuild 16.0, a new Static Graph API was added for parsing project trees in a more efficient way. This new API also can express dependencies so that callers can build a graph that represents the tree. NuGet should adopt this new API to replace the way projects are being evaluated via target execution which would reduce command-line restores dramatically.

NuGet product used (NuGet.exe | VS UI | Package Manager Console | dotnet.exe): Any

NuGet version (x.x.x.xxx): Any

dotnet.exe --version (if appropriate): Any

VS version (if appropriate): Any

OS version (i.e. win10 v1607 (14393.321)): Any

Worked before? If so, with which NuGet version:

Detailed repro steps so we can see the same problem

  1. From the command-line, restore a large tree of projects.

The restore takes around 30 seconds for 700 projects. With Static Graph and other perf improvements, it takes less than 6 seconds.

Other suggested things

Verbose Logs

Please include verbose logs (NuGet.exe -verbosity detailed | dotnet.exe --verbosity diag | etc...)

Sample Project

Very helpful if you can zip a project and paste into this issue!

@japj
Copy link

japj commented Nov 5, 2019

Just a check, I had the impression that although the MSBuild static graph api is public/stable now, the implementation is still ongoing (looking at the MSBuild static graph related issue list).

Is the MSBuild implementation now sufficiently stable that static graph is working on all types of solutions and projects? (Or has this the potential to accidentally break nuget restore operations for certain solution/projects?)
perhaps I am missing some insight here? (Although I am very happy at the speed increase)

@jeffkl
Copy link
Contributor Author

jeffkl commented Nov 5, 2019

@cdmihai can you comment to the above message?

@cdmihai
Copy link

cdmihai commented Nov 5, 2019

So far static graph is used to successfully build (i.e. call the msbuild build target on) Orchard Core, Orchard classic, Roslyn's Compilers.sln, and the whole Cloudbuild repo (~700 projects, the largest repo we've tried it on). The graph is built following the ProjectReference items. To my knowledge that's what Nuget is also using to build its own graph, so in that respect whatever project graph Nuget produces, the static graph should produce as well.

That being said, these are some caveats that spring in mind:

  • graph structure is only based on evaluation time data, not build time data. If there's targets that massage the ProjectReference item at build time, static graph won't see that logic.
  • solution support has some missing cases: [Static graph] Instantiate graph from solution dotnet/msbuild#4463 (comment)
  • static graph has some special knowledge for multitargeting details and global property inference, which could deviate from reality
  • we've never tried it on C++ projects, but AFAIK, nuget doesn't cover C++

@japj
Copy link

japj commented Nov 6, 2019

I know there is a command line MSBuild switch for using static graph/isolation projects.
But shouldn’t there be a VisualStudio setting to turn this behavior on/off for trying out with projects?

Related to nuget support, that is implemented through some Microsoft common targets mechanism and nuget restore is “technically” available in c++ projects too (e.g. nuget MSBuild tools packages can be consumed in c++ projects, although the VS UI doesn’t handle it)

FYI we have mixed project type solutions (c# old/new style, sqldb, c++, powershell, Azure, SSIS, etc) incl. some customizations like using the Microsoft CentralPackageVersioning MSBuild Sdk.

@cdmihai
Copy link

cdmihai commented Nov 6, 2019

I know there is a command line MSBuild switch for using static graph/isolation projects.
But shouldn’t there be a VisualStudio setting to turn this behavior on/off for trying out with projects?

It depends if VS wants to explore this or not. /graph might make a solution build faster (when VS invokes msbuild on the solution and lets msbuild manage the graph). /isolate is more useful to ensure correctness (ensure the static graph does not have gaps, ensure that higher order build engines (BuildXL and Quickbuild) behave exactly like vanilla msbuild, etc) and to ensure that a project is built in isolation of other projects. VS could also replace it's own msbuild graph with static graph to reduce code duplication.

Related to nuget support, that is implemented through some Microsoft common targets mechanism and nuget restore is “technically” available in c++ projects too (e.g. nuget MSBuild tools packages can be consumed in c++ projects, although the VS UI doesn’t handle it)

FYI we have mixed project type solutions (c# old/new style, sqldb, c++, powershell, Azure, SSIS, etc) incl. some customizations like using the Microsoft CentralPackageVersioning MSBuild Sdk.

As long as projects use ProjectReference (with the standard metadata) to express dependencies, constructing a static graph should be fine, indifferent or project system, sdk, or homogeneity. Building the graph with vanilla msbuild correctness parity is another issue, but for nuget restore that feature is not used.

@rrelyea rrelyea changed the title Adopt MSBuild Static Graph Make noop restore faster - speed up evaluations by calling MSBuild Static Graph apis Dec 4, 2019
@zarenner
Copy link

zarenner commented Jan 9, 2020

@jeffkl is this in a state where we could test it now or soon (e.g. with a preview .net core sdk)? Very excited for this.
fyi @tedchamb @kevin-david

@jeffkl
Copy link
Contributor Author

jeffkl commented Jan 9, 2020

@zarenner we'll need to do a VS insertion and then it'll be available in internal previews. I expect to find some issues and fix them before the next release, 16.5 Preview 3.

@aolszowka
Copy link

Did this land in Preview 3? We still see pretty poor restore times for ~1300 C# Projects in 16.5 Preview 3. (Approximately 30-60 seconds). What do you need to help debug this?

@rrelyea
Copy link
Contributor

rrelyea commented Mar 5, 2020

@aolszowka - this requires an opt-in property, i believe. @jeffkl? Do we have a doc page for this yet?

@jeffkl
Copy link
Contributor Author

jeffkl commented Mar 5, 2020

Yes you need to set the RestoreUseStaticGraphEvaluation to true to opt into the feature.

<PropertyGroup>
  <RestoreUseStaticGraphEvaluation>true</RestoreUseStaticGraphEvaluation>
</PropertyGroup>

Or for one time use:

msbuild /t:Restore /p:RestoreUseStaticGraphEvaluation=true

@aolszowka
Copy link

Awesome; Big Wins for us on some solutions (largest win was 30 seconds -> 10 seconds for a ~1,000 C# Only Solution).

  1. Is there anything programmatic that we can gather to give higher quality evidence to show speed ups on the restore?
  2. Does this imply /graphBuild or is this separate from that flag?

@aolszowka
Copy link

Pinging the thread; any documentation that answers the questions?

  1. Is there anything programmatic that we can gather to give higher quality evidence to show speed ups on the restore?
  2. Does this imply /graphBuild or is this separate from that flag?

@cdmihai
Copy link

cdmihai commented Mar 17, 2020

Is there anything programmatic that we can gather to give higher quality evidence to show speed ups on the restore?

I'll defer to @jeffkl here, but you should see end to end build times go down if you have super long restores.

Does this imply /graphBuild or is this separate from that flag?

While fast restore does use static graph, it doesn't require the users to use /graph. It should be transparent for users. It does mean that you have to use ProjectReference to describe project dependencies.

@zarenner
Copy link

@jeffkl @rrelyea Finally gave this a try, and it shaved at least 10 seconds off for us 🎉
Thanks for all the work here.

We're still at ~6 seconds for ~500 projects / ~200 centrally managed packages, but I'm hoping that's small enough to convince my team to get rid of our ugly broken hacks for skipping restore entirely.

Do you know which 3.1 dotnet SDK this will make it into and when? Looks like it doesn't work in 3.1.201 as NuGet.Build.Tasks.Console.dll is missing.

@jeffkl
Copy link
Contributor Author

jeffkl commented Apr 24, 2020

@zarenner I think its only available in 5.0 preview 2. The insertion was made here:
dotnet/sdk#10869

https://dotnet.microsoft.com/download/dotnet/5.0

@zarenner
Copy link

Yeah that's where I tried it from. In #9267 looks like it was mentioned it might make it in 3.1.300? Is that still the case? /cc @nkolev92

@aolszowka
Copy link

@jeffkl

Is there anything programmatic that we can gather to give higher quality evidence to show speed ups on the restore?

@jeffkl
Copy link
Contributor Author

jeffkl commented Apr 24, 2020

The best way to measure it is to install MSBuild 16.5+ and restore from the root 3 times:

msbuild /t:restore
msbuild /t:restore
msbuild /t:Restore /p:RestoreUseStaticGraphEvaluation=true

The first restore might pull packages and isn't considered a no-op. The second and third restores are no-op restores without my optimization on and then with it on.

In a large repo with ~740 projects it used to take around 30 seconds and now it takes 7.

@nkolev92
Copy link
Member

Looking at it, I don't think 3.1.300 ever happened :(

@zarenner
Copy link

That's a bit disappointing. When's the earliest non-preview release you think we might be able to use it?

@nkolev92
Copy link
Member

@zarenner
Not sure yet. Started a discussion to get it into a 3.1.xxx branch: dotnet/sdk#11420

@nkolev92
Copy link
Member

nkolev92 commented Jun 3, 2020

The NuGet.Build.Tasks.Console side change has been ported to 3.1.400.

@nkolev92
Copy link
Member

nkolev92 commented Jun 3, 2020

Created & closed #9644 for release notes purposes.

@AraHaan
Copy link

AraHaan commented Feb 4, 2021

is this in the .NET 5.0.102 SDK too?

@zivkan
Copy link
Member

zivkan commented Feb 4, 2021

yes, but it's still opt-in: https://docs.microsoft.com/en-us/nuget/reference/msbuild-targets#restoring-with-msbuild-static-graph-evaluation
dotnet restore -p:RestoreUseStaticGraphEvaluation=true

@AraHaan
Copy link

AraHaan commented Feb 4, 2021

@zivkan thanks, been trying to fix an issue where dotnet restore sometimes takes a few minutes just to restore 3 analyzer packages, a package for the .NET Framework reference assemblies for the .NET Framework targets on my code, and a few other packages like the ones for my mbsuild task, and then also System.Text.Json v5.0.1 even when not using .NET 5 for it (when targeting .NET Framework / older versions of .NET Core / Standard 2.0) from within github actions making the workflow take 3~5 minutes to build when it could build in even less time.

Too bad I cant just cache the things that dotnet restore does inside of github actions for future use to save time (if nothing needs to be updated on the restore).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Partner:1ES Tenet:Performance Performance issues Type:DCR Design Change Request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants