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 static graph restore the default experience. #9803

Open
1 task done
nkolev92 opened this issue Jul 15, 2020 · 16 comments
Open
1 task done

Make static graph restore the default experience. #9803

nkolev92 opened this issue Jul 15, 2020 · 16 comments
Labels
Area:RestoreStaticGraph Issues related to the Static Graph restore Functionality:Restore Partner:MSBuild Priority:2 Issues for the current backlog. Product:dotnet.exe Tenet:Performance Performance issues Type:DCR Design Change Request

Comments

@nkolev92
Copy link
Member

nkolev92 commented Jul 15, 2020

This is an epic tracking all things relevant to static graph restore and whether there are any blockers/concerns to making it the default.

For readers, you'll need Zenhub to see the linked issues.

Currently this contains all potential issues tagged as static graph.

This doesn't mean they all need to be fixed, just analyzed :)

Note

For customers moving to static graph

  • If you want to ensure that your restore is equivalent across static graph and regular restore consider running:
  1. Restore with static graph.
  2. Restore without static graph

When you run this, at the end of the second restore you should see something like:
All projects are up-to-date for restore.

@nkolev92 nkolev92 added Area:RestoreRepeatableBuild The lock file features Area:RestoreStaticGraph Issues related to the Static Graph restore Functionality:Restore Epic Tenet:Performance Performance issues Priority:2 Issues for the current backlog. and removed Area:RestoreRepeatableBuild The lock file features labels Jul 15, 2020
@nkolev92 nkolev92 changed the title Progress on making static graph restore the default experience. Make static graph restore the default experience. Jul 16, 2020
@jeffkl
Copy link
Contributor

jeffkl commented Jul 16, 2020

One potential issue is that static graph evaluation based restore doesn't call any of the older targets like _GenerateRestoreProjectPathItemsPerFramework. People shouldn't be using them as extension points since we prefixed them with _ but there's no enforcement. So if the default switches, these targets won't run any more and people's builds can start failing in very weird ways.

Telemetry would help us greatly here to know how many people are doing this. We might even be able to add a warning in a future release telling people that the targets are going away.

@nkolev92
Copy link
Member Author

nkolev92 commented Jul 16, 2020

That's a great point.
Definitely a smaller concern, but a possibility.

Think advertising the existence of the feature more broadly before making it a default is a good idea.

1ES repos will probably catch a large majority of the scenarios, but you can never have too many dogfooders :)

@jeffkl
Copy link
Contributor

jeffkl commented Jul 16, 2020

We are spinning up automation to make it the default for all 1ES customers by sending PRs to their repos. I'll update this issue with the problems we have getting people to adopt it.

@anangaur
Copy link
Member

anangaur commented Sep 9, 2020

What's the trigger, criteria before we make this default?

@nkolev92
Copy link
Member Author

nkolev92 commented Sep 9, 2020

The plan is to make it default in the next major release given that it's a big change.

The main criteria is correctness. We need static graph to not regress current functional scenarios.

Perf gains are something we'll look into (we have) as well, but numbers there have been overwhelmingly positive for large project counts, and on par for lesser project counts.

@nkolev92
Copy link
Member Author

nkolev92 commented Sep 9, 2020

Note for other readers.

Static graph is pretty safe to use.
We've found a very limited set of issues and most of those fail fast.
Unless you are customizing restore somehow, switching to static graph should just work.

@aolszowka
Copy link

FWIW you may break others who have custom Project Types wherein static graph does not work, I have filed a bug report at #10019

@nkolev92
Copy link
Member Author

@aolszowka The dup of the issue you linked in tracked here.

@nkolev92
Copy link
Member Author

Oh @jeffkl is already on it!

@nkolev92
Copy link
Member Author

I have updated the issue body calling out the way to test for the equivalence of these 2 experiences.

If you see a problem, do let us know by filling an issue.

@aolszowka
Copy link

What was the status on dog-fooding this? I can't believe we're this unique to have two issues found by just us #10307

@Piedone
Copy link

Piedone commented Aug 13, 2022

I've made some performance measurements, locally and under GitHub Actions (Ubuntu and Windows) with our Lombiq's Open-Source Orchard Core Extensions solution. This, since it uses Orchard Core, has hundreds of package dependencies with transitive ones included.

You can find my results here. I've seen some nice 8-15% speedups locally during dotnet restore. In GitHub Actions builds I was more interested in the overall impact on the dotnet build times (usually we don't have a separate restore) and while it does show up as a 0,5-4,5% speedup there too with an empty NuGet cache (what happens with GHA builds due to their fresh VMs, unlike local ones), the 3,5% slowdown for the full source Windows builds (which is the slowest) cancels out the aggregate compute time gains (and adds time waiting for the workflow to finish).

Could you check it out, please? It seems that turning this on doesn't help in the GHA case all in all. Though the margins we're talking about here are really small, so probably doing more measurements would turn the end result somewhat.

@jeffkl
Copy link
Contributor

jeffkl commented Aug 17, 2022

@Piedone thanks for the detailed performance measurements. During a full restore, I would not necessarily expect static graph-based restore to be too much faster. The main optimization that static graph-based restore uses is an out-of-proc system for evaluating all of the MSBuild projects which uses some special feature flags to improve performance as well as server GC. Once the projects have been evaluated, the same code runs to execute restore for both "regular" and static graph-based restore.

The more projects a repository has, the more I would expect static graph-based restore to speed things up. And of course no-op restores benefit the most since evaluating all of the MSBuild projects is the long pole and no packages are pulled and unzipped. For example, we have an internal repo with 750 projects where a full no-op restore used to take 35 seconds but now takes only 7 seconds. We also have a repository with 5,000 projects that would basically time out when restoring but now can restore in a few minutes.

We'll continue to improve restore performance in both scenarios and see if we can ever make static graph-based restore the default. But for now it will remain opt-in based on each individual repositories needs.

@Piedone
Copy link

Piedone commented Aug 17, 2022

I see, thanks for the explanation!

@ghost
Copy link

ghost commented Sep 2, 2022

Issue is missing Type label, remember to add a Type label

@ghost ghost added the missing-required-type The required type label is missing. label Sep 2, 2022
@nkolev92 nkolev92 added the Type:DCR Design Change Request label Sep 5, 2022
@ghost ghost removed the missing-required-type The required type label is missing. label Sep 5, 2022
@stan-sz
Copy link

stan-sz commented Feb 24, 2023

Just in case, linking dotnet/msbuild#8326

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area:RestoreStaticGraph Issues related to the Static Graph restore Functionality:Restore Partner:MSBuild Priority:2 Issues for the current backlog. Product:dotnet.exe Tenet:Performance Performance issues Type:DCR Design Change Request
Projects
None yet
Development

No branches or pull requests

7 participants