-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Description
I think there is a functional issue here as well, but I discovered it just from the performance impact. To repro sync https://github.com/aspnet/jitbench and follow the first 3 steps in the README. In step 2 you don't need to replace anything. The important step is 3, it is generating the ASP.Net store from scratch using dotnet store command.
Expected: step 3 completes with only a modest perf overhead relative to the time it takes to crossgen the assemblies (apparently about ~45s on my machine)
Actual: step 3 is still running after 20 minutes, not sure how long it goes
I'm pretty sure the bug is right here:
sdk/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.ComposeStore.targets
Line 108 in b5aa4a2
Properties="MSBuildProjectExtensionsPath=$(ComposeWorkingDir)\%(PackageReference.Identity)_$([System.String]::Copy('%(PackageReference.Version)').Replace('*','-'))\;" |
The StoreResolver target will be invoked N^2 times to make a full matrix of MSBuildProjectPathExtensionsPath on every project X StorePackageName on every project. I'm guessing the intent was to batch on properties of PackageReferencesToStore rather than to batch on PackageReference.
FWIW, while I was investigating the issue above I found that the scheme of invoking MSBuild on the current project to get target parallelism from project parallelism can have some nasty perf side-effects of its own. Before running the desired target MSBuild re-evaluates all the initial global items and properties. One of the initial global items in this project is the set of all source files which includes a globing pattern of **/*. This means in addition to the desired work, MSBuild also does a complete file system walk of the project directory to compute an item list that will never be used. For project directories containing M arbitrary files and N files to crossgen, together with the issue above performance is currently O(N^2 * M) whereas you'd hope for O(N) or O(N + M).