search

dotnet / project-system

The .NET Project System for Visual Studio
MIT License
969 stars 387 forks source link

High CPU when target framework is empty #3670

Closed davkean closed 4 years ago

davkean commented 6 years ago

Opening the following projects causes VS to constantly consume CPU:

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

  <PropertyGroup>
    <TargetFramework></TargetFramework>
  </PropertyGroup>

</Project>

or 

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

  <PropertyGroup>
    <TargetFrameworks></TargetFrameworks>
  </PropertyGroup>

</Project>

Design time builds are constantly occurring in the background.

Also hit by https://github.com/dotnet/project-system/issues/3489 by AWS SDK.

davkean commented 6 years ago

Pretty sure this is dependency node.

This was hit by an MVP.

Pilchie commented 6 years ago

@davkean did you look at all? Is this an infinite restore loop?

jmarolf commented 6 years ago

just tried this scenario. Its infinite restore.

davkean commented 6 years ago

It didn't look like infinite restore - it was infinite design-time builds but that will likely be dependency node reinitializing over and over.

Pilchie commented 6 years ago

Oh, right - dupe of #3489 then?

davkean commented 6 years ago

Yep, dup'd it against this one.

eerhardt commented 6 years ago

I'm hitting this in dotnet/corefx after updating the .csproj's to SDK-style projects.

To repro:

  1. pull https://github.com/wtgodbe/corefx/tree/SdkProj (the branch from PR https://github.com/dotnet/corefx/pull/29831).
  2. Build once from the root of the repo on the commandline: build.cmd
  3. Open src\System.Collections\System.Collections.sln.

However, in my scenario I don't see any design-time builds continually happening using the Build Logging window. However, it is consuming a lot of CPU:

image

Pilchie commented 6 years ago

Can you collect an ETL trace during this period? https://github.com/dotnet/roslyn/wiki/Recording-performance-traces-with-PerfView

eerhardt commented 6 years ago

Yes, I'll get it tomorrow morning.

eerhardt commented 6 years ago

I took 2 traces.

  1. With the solution already loaded, and eating CPU in the background, I ran the trace for about 10 seconds.
  2. A full trace of loading VS, loading the solution, and then letting it run for ~30 seconds.

I uploaded them to the internal scratch file share: \\scratch2\scratch\eerhardt

Pilchie commented 6 years ago

@sharwell and/or @davkean - care to take a look?

davkean commented 6 years ago

@eerhardt Can you verify that you have a TargetFramework to verify if its just another symptom of that? This is slated for 16.0, so won't look at until then if its a dupe.

eerhardt commented 6 years ago

Yes, setting TargetFramework makes the CPU usage drop down to nothing when VS is in the background.

The only behavior difference I noticed from the originally described issue is:

Design time builds are constantly occurring in the background.

I wasn't seeing any design-time builds happening in the Build Logging window.

davkean commented 6 years ago

Thanks.

It might be good to sync up on CoreFx's requirements and why you can't use TargetFrameworks/TargetFramework. I've had your situation in the back of my mind, and want to address it and make it work for you out of the box.

eerhardt commented 6 years ago

Agreed. @weshaggard would have the best background knowledge.

I haven't completely ruled out that we can't use TargetFrameworks/TargetFramework yet. We are setting TargetFramework to workaround this issue. It's just that it is kind of a meaningless concept in the corefx repo's projects. In "normal" projects, it defines which version of netcoreapp/netstandard/netfx you want to reference. In corefx, we aren't referencing a framework. Instead, we are defining the framework.

davkean commented 6 years ago

Think of "TargetFramework" as simply another open-ended dimension at which you can specialize a configuration like Platform and Configuration, except it's treated slightly different:

You can call your target frameworks "foo" and "bar" if you'd want (assuming you fill in the details, TargetFrameworkIdentifier, TargetFrameworkVersion, etc that we produce from the friendly name)

weshaggard commented 6 years ago

Aren't there other assumptions made about those properties as well? For example doesn't Nuget interpret them?

davkean commented 6 years ago

Barring bugs, TargetFramework is supposed to be treated as a "user value". NuGet gets the full TFM that we auto-generate from TargetFramework, and is supposed to be only using TargetFramework in conditions within the generated props files.

davkean commented 6 years ago

My ultimate aim is to be able to able to specialize on other things; such as RID. It would be helpful if we could feed your requirements in so that what we develop around this works for you and you can stop working around MSBuild/VS

clairernovotny commented 6 years ago

You can call your target frameworks "foo" and "bar" if you'd want (assuming you fill in the details, TargetFrameworkIdentifier, TargetFrameworkVersion, etc that we produce from the friendly name)

This actually won't work because of NuGet validations. When I initially tried to implement multi-RID builds, I attempted it by using/abusing TFM's like netstandard2.0+win-x86. I had to monkey-patch a lot of targets to make it work despite setting NuGetTarketMoniker, TargetFrameworkIdentifier and TargetFrameworkVersion correctly. NuGet just blew up with unsupported/unknown TFM's.

clairernovotny commented 6 years ago

My ultimate aim is to be able to able to specialize on other things; such as RID. It would be helpful if we could feed your requirements in so that what we develop around this works for you and you can stop working around MSBuild/VS

I made this work today by making the outer loop get the RID's for the TFM and pass that into the inner loop: https://github.com/onovotny/MSBuildSdkExtras/blob/db55a020a7d7d0d7119ff01c3104a9dd62a51f11/Source/MSBuild.Sdk.Extras/Build/RIDs.targets#L23

It works and builds, in VS and the command-line, but the project system/IDE knows nothing about it. So no IntelliSense for that combination (to catch the define's for each RID).

davkean commented 6 years ago

It's not supposed to be interpreting that value, at least that was the original design - but there's going to be bugs here. I'm rewriting/removing our usage of TFM at the moment, and then I'll start pushing on other teams.

clairernovotny commented 6 years ago

@davkean it most certainly is interpreting/validating TFM's. Check out this horror for my first attempt at using TFM+RID: https://github.com/onovotny/MSBuildSdkExtras/pull/93

I had to rewrite the items that NuGet saw to remove the +RID and then it generated the props/targets incorrectly, so I needed to read its file and find/replace the source TFM with the TFM+RID to make props/targets work correctly.

What I have in that PR is functional, just a lot grosser than the solution based on a discussion with @jeffkl.

clairernovotny commented 6 years ago

Here's where NuGet Restore (at least) is validating the tfm itself:

https://github.com/NuGet/NuGet.Client/blob/b82a7274e5f58c8af2eb052b8ff98d3d29fed66e/src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/SpecValidationUtility.cs#L131

I would expect that it should be based on a NuGetTargetMoniker value, so I can define my own TFM and say "use netstandard2.0" for it. No dice today.

nguerrera commented 6 years ago

We did fully intend for TargetFramework do be open ended, but there are definitely issues. Another place where there's a problem is where we negotiate tfms between project references. It passes the TargetFrameworks to nuget to pick the "nearest" one. It would be prohibitively expensive to get all of the full tfms there in the current design. And even if we could there would cases where you have the same full TFM for different custom TF. I think the only out would be to require such P2Ps to skip the negotiation and manually assign the desired TF. There are bugs around doing that though, IIRC.

davkean commented 6 years ago

It passes the TargetFrameworks to nuget to pick the "nearest" one.

Ah, yeah, forgot about this case - we'd need the same translation logic in the outer build, or call into the inner build - which would be expensive.

clairernovotny commented 6 years ago

@davkean @nguerrera do either of you know off hand where this logic lives in the targets? Would save me some spelunking time.

nguerrera commented 6 years ago

https://github.com/Microsoft/msbuild/blob/ffa7f408e8d00bda677cfb0ec15d547acf2aea06/src/Tasks/Microsoft.Common.CurrentVersion.targets#L1571-L1582

nguerrera commented 6 years ago

Related: https://github.com/NuGet/Home/issues/5154

davkean commented 4 years ago

@ocallesp This is likely the cause: https://github.com/dotnet/project-system/issues/3670#issuecomment-400475289.

davkean commented 4 years ago

Have a look inside AggregateCrossTargetProjectContext where it persists the targetfamework.

drewnoakes commented 4 years ago

@ocallesp feel free to reach out if you would like to discuss this.

ocallesp commented 4 years ago

@davkean this issue is only reproducible in VS 2017. Not able to repro in VS 2019/master.