-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Description
Is your feature request related to a problem? Please describe.
@DamianEdwards pointed me at this review of the macbook w/ M4 Max processor over the weekend (timestamp is directly to the comparison I'll be talking about).
The repo in question is https://github.com/alexziskind1/machine_tests.
The gist is that a very large (over 100k) number of C# source files are generated, and then dotnet restore
followed by dotnet build
are called, and this is treated as a performance test.
I took binlogs on my Windows and WSL Ubuntu instances, and Damian took binlogs on his Ubuntu machine and his Mac, all with eval profiling enabled. This is accomplished by adding -bl
to enable binlog generation, and --profileevaluation
to get nice graphs of where eval time is spent. As seasoned MSBuild experts might expect, the problem is file globbing here - the vast majority of the time in the build is spent in (on my Windows machine):
- SDK Default Compile Item Include globbing
- Csc - this is out of our control, compiling 100k files just takes time
However, in the test thread @alexziskind1 is doing, he's evaluating 5 times total (times on my Windows box):
dotnet restore
- once before the
Restore
target is executed, withMSBuildRestoreSessionId
andMSBuildIsRestoring
properties set (45s) - once as a side effect of the
_FilterRestoreGraphProjectInputItems
task when it calls theMSBuild
Task on the same projects'_IsProjectRestoreSupported
Target, but with an additional Global PropertyExcludeRestorePackageImports
set (45s)
- once before the
- dotnet build (with implicit restore because
--no-restore
is not passed in)- the same two from above, and
- the 'normal' evaluation of the project without any of the Restore-related properties set (45s)
On my machine, this means that we spend ~2.25 minutes just on evaluations, almost all of which is globbing, and ~2m on actual compilation (Csc Task wall-clock time).
Describe the solution you'd like
Restore
doesn't generally need to do Default Item expansion (this is pretty common on other commands that do MSBuild evaluation like dotnet run
, so the dotnet cli
should add the --restoreproperty:EnableDefaultItems=false
argument to the command line that it constructs for any implicit restore it asks MSBuild to perform.
If we wanted to be very defensive here, we could also
- enlighten the
dotnet
CLI about the MSBuild--restoreproperty
CLI argument like we have for the MSBuild--property
CLI argument - parse any user-supplied input for this argument, and if the user has specified a
EnableDefaultItems
value do not add our own
Additional context
If we do this, then on my machine at least every Restore-related evaluation goes from ~45s to ~300ms. That's a huge savings, amortized.
Activity
rainersigwald commentedon Jun 16, 2025
@jeffkl have you observed any problems from NuGet's static-graph glob avoidance that might inform this?
jeffkl commentedon Jun 16, 2025
I remember a handful of complaints over the years about globbing not expanding on anything except
*.*proj
. I don't think I've ever seen a case where a user expects globbing to work on a<PackageReference />
since those are statically declared and not backed by anything on disk. From what I can remember, the only time globbing didn't work as expected for people was on a<ProjectReference />
in a traversal project because their projects didn't end inproj
for whatever reason.If we could make globbing only work for certain items that could be a potential way to disable other globs during evaluation. Lazy evaluation of items also but I think the regular restore paths it might not help since we're not using APIs.
I would not recommend setting global properties like
EnableDefaultItems=false
since it wouldn't help with users' random items. To really speed up evaluation for restore, disabling globbing is the way to go.baronfel commentedon Jun 16, 2025
If we don't want to toggle everything, we can also disable the items responsible for most of the pain:
None of these should impact Restore.
ViktorHofer commentedon Jun 17, 2025
+100
baronfel commentedon Jun 17, 2025
@rainersigwald I'm happy to disable globbing entirely for restore, but it appears the only mechanism for that is an env var. We need a way to toggle that behavior for restore only from the SDK. Got any thoughts?
rainersigwald commentedon Jun 17, 2025
We could expose that in the API, I think--IIRC there may be some caches we'll have to be careful to additionally key on "globs disallowed".
But I fairly strongly suspect that we could get very good results with just the few item types you named above; I'd want to see evidence otherwise. @ViktorHofer what makes you think custom-item globs are causing significant pain?
ViktorHofer commentedon Jun 18, 2025
My reaction here was to build on top of what we already have (nuget static graph restore's opt-out) and avoid a potential diff between static graph and "normal" restore. I think this comment capture the concerns nicely: https://github.com/NuGet/NuGet.Client/blob/772ee13d2bafaa1414d90dbfbd77e0941115ef19/src/NuGet.Core/NuGet.Build.Tasks.Console/MSBuildFeatureFlags.cs#L65
baronfel commentedon Jun 23, 2025
I implemented this in #49526 and can confirm that this does seem to impact SDK scenarios around Restore - particular in RID-specific restores where the RID isn't in the Project but is passed from the command line. Checking out what's up with that.
EDIT: this is just user error the
-rp
flag explicitly states that it is exclusive with-p
, so the SDK should send all SDK-defined-p
as-rp
if any-rp
are sent at all. Working on that now as well.If that doesn't prove fruitful, then there is at least one other pathway to get this benefit I can see:
-restore
to an otherwise-reasonable build command line. This is a case where we could set the environment variable flag that @jeffkl and @ViktorHofer mention.