[Bug]: Pack target is causing unnecessary project evaluations which noticeably slows down a project's build

ViktorHofer commented 2 years ago

NuGet Product Used

dotnet.exe

Product Version

6.0.100

Worked before?

No response

Impact

It bothers me. A fix would be nice

Repro Steps & Context

When building a .NET project which sets the GeneratePackOnBuild switch or when invoking the Pack target, NuGet calls into the underlying projects via an MSBuild Exec task (i.e. based on the project.assets.json data) and set BuildProjectReferences=false as a global property. Because of the global property, additional project evaluations are triggered which slow down the overall build.

This is especially noticeable when projects multi-target. As an example, let's consider a project "A" that multi-targets for net6.0;net5.0;netstandard2.0;net48. Based on the the described issue, the evaluations double from 5 to 10:

Before:

Outer build net6.0;net5.0;netstandard2.0;net48 Inner build net6.0 Inner build net5.0 Inner build netstandard2.0 Inner build net48

After:

Outer build net6.0;net5.0;netstandard2.0;net48 Inner build net6.0 Inner build net5.0 Inner build netstandard2.0 Inner build net48 Outer build net6.0;net5.0;netstandard2.0;net48 with additional BuildProjectReferences global property Inner build net6.0 with additional BuildProjectReferences global property Inner build net5.0 with additional BuildProjectReferences global property Inner build netstandard2.0 with additional BuildProjectReferences global property Inner build net48 with additional BuildProjectReferences global property

Shouldn't it be possible to avoid these extra evaluations by sequencing the gathering of the required data into the already running outer build, which runs without the BuildProjectReferences global property?

Verbose Logs

No response

nkolev92 commented 2 years ago

The project version one is probably the most problematic one in your case right? The other 2 look like they actually need to get data from the inner build.

Building the project refs there probably introduces a regression for projects with deep graphs, right?

I'm trying to brainstorm here, but we might need to involve msbuild folks who may have some advice.

ViktorHofer commented 2 years ago

The project version one is probably the most problematic one in your case right? The other 2 look like they actually need to get data from the inner build.

I don't think it matters if just the outer or the inner builds are invoked. If I use GeneratePackageOnBuild or invoke the project via the Pack target without specifying the --no-build option, projects build and both outer and inner dimensions are invoked. What I wonder is why NuGet can't sequence the gathering of the data that it needs into the already running build (which doesn't specify BuildProjectRefeferences=false, as it builds) instead of triggering another set of evaluations and invocations with the additional BuildProjectReferences global property.

I'm trying to brainstorm here, but we might need to involve msbuild folks who may have some advice.

Yes, I think asking msbuild folks like @dsplaisted and @rainersigwald for help would be a good idea.

rainersigwald commented 2 years ago

Yeah, it feels like you should be able to do this by doing something like "add a target that sets a property like _NuGetTheBuildActuallyRan=true in the build process that doesn't run on Pack-with-no-build, then check for it and if it's set don't use BuildProjectReferences=false.

That said, evaluations are ideally not too expensive, especially if we've already read a bunch of files from disk. It might be better to spend dev time elsewhere, even though this is definitely wasted work.

ViktorHofer commented 2 years ago

In the case of dotnet/runtime libraries, project evaluations are quite expensive. The import and evaluation from both the msbuild layout under the sdk's folder and from the props and targets files from nuget packages + the Arcade SDK adds 200-250ms per project:

dotnet/runtime has about 80 out-of-band projects and they usually consist of the following tfms: net7.0;net6.0;netstandard2.0;net462. 5 builds per project 80 200ms = 80 seconds. I imagine that we aren't the only customer with a high number of projects which produce packages.

nkolev92 commented 2 years ago

I imagine that we aren't the only customer with a high number of projects which produce packages.

Probably not, but the biggest difference is the number of target frameworks that your repo has.

ViktorHofer commented 2 years ago

Three frameworks are quite common in .NET OSS repos. Many folks target .NETFramework and .NETCoreApp in addition to .NETStandard to avoid binding redirects or leverage new features.

You (NuGet) have such packages as well: https://www.nuget.org/packages/NuGet.Packaging/ :)

nkolev92 commented 2 years ago

I'm not saying it never happens, but in the big picture, nuget.client and runtime repos are not the most common ones.

Forgind commented 2 years ago

200-250ms sounds like a lot. Can you get an etw trace with the Microsoft-Build provider on? https://github.com/dotnet/msbuild/blob/main/documentation/specs/event-source.md

ViktorHofer commented 2 months ago

@Forgind sorry for not getting back to you on this. I just filed https://github.com/dotnet/runtime/issues/105139 to track this as it showed up again.

EDIT:

200-250ms sounds like a lot. Can you get an etw trace with the Microsoft-Build provider on?

FWIW, I just looked at a console sample app:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net9.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

</Project>

and that also spends 280ms in evaluation (warm state): dotnet msbuild /t:Build /clp:PerformanceSummary /v:m .\evalperfm.csproj -tl:off

NuGet / Home