Double Evaluation Fix #2595

AndyGerlicher · 2017-10-05T20:28:54Z

Still WIP. I need to get the NuGet task pulled down for bootstrap build.

In order to avoid batching the _GetProjectReferenceTargetFrameworkProperties target for each reference, the ProjectReference protocol can be amended to return an item from GetTargetFrameworkProperties instead of a semicolon-delimited list of key-value pairs. This allows a single build request to be sent to the engine, and allows resolving references in parallel on multiprocess builds.

The previous commit is a breaking change to the ProjectReference protocol, requiring a .NET Core SDK change to return the now-expected structured data rather than the old semicolon-delimited string. That means, for example, that MSBuild v15.5 couldn't build a solution containing a full framework project that referenced a .NET Core SDK 2.0.0 project. To avoid this, reconstruct the new structured data from the old return value: * Allow the MSBuild engine to split the returned value on `;` and return multiple values. * Batch over metadata common to those values to reconstruct a single item with the complete string return value. * Parse that string into structured metadata on a single item, as though the project had returned a single item with metadata. * Remove the now-redundant individual-valued items. * Continue as before with adjusting the reference items based on the metadata.

In preparation for describing the changes to return values I'd like to make, I need to document what the old ones were.

Document the more-succinct-but-compatibility-breaking single-item-with-metadata return possibility for GetTargetFrameworkProperties.

* Call GetTargetFrameworks target to identify target frameworks in referenced projects. Since TargetFrameworks is not set, this will avoid a 2nd evaluation when the project single targets. * Adds dependency on NuGet AssignReferenceProperties task.

AndyGerlicher · 2017-10-08T03:55:11Z

@davkean FYI

I've got some initial benchmark results for this, and it's a bit mixed. Evaluation time and memory shouldn't really be impacted. The diff here I would contribute mostly to noise. The memory seems to be about 15k more. Makes sense that with a tiny standard deviation on the small projects (GC called right before so likely no GC during test) we would see a delta. It's more to parse, few more objects materialized, etc. Surprised it would be that much, but while statistically significant I don't think it's real world significant.

Build time (design time build listed) is more interesting. For smaller projects, it's clearly not faster. And I'm not really surprised. It's an extra task from a non-msbuild dll (not ngen'd here). Many builds will end up with that dll loaded anyway (from Restore, etc.) so it's not always an extra dll, but still it's not free. And this affects all builds regardless of SDK status. I'm also a little worried this will set off RPS having the extra dll.

The good news on build time is a lot of projects should see an improvement. 30%-50% in the P2P and Picasso test cases which is great. I don't have any spinning drives to test on but I assume this could be even better there when globbing is involved.

The mixed news is testing Roslyn.sln. Doing an incremental build on the solution msbuild /v:m Roslyn.sln went from 335 evaluations down to 165 (good), but with /m the time was reliably 27s -> 26s. Which is not that spectacular. Without /m the times was a little more pronounced at 1:36 -> 1:23. Which after calculating is 13% so that is pretty good, just not as much as I was hoping for. Perhaps it will be more for design-time builds, but I couldn't get that working.

Evaluation: Time (ms)

Test	Overall	Significant δ	Value
DotnetConsoleProject	🔴	yes	30.395 -> 30.5164 (0.399%)
DotnetWebProject	🔴	yes	49.611 -> 49.6851 (0.149%)
DotnetMvcProject	🔴	yes	86.4774 -> 86.5475 (0.081%)
Picasso	✅	yes	266.2479 -> 256.1184 (-3.805%)
SmallP2POldCsproj	✅	yes	42.6504 -> 41.8708 (-1.828%)
SmallP2PNewCsproj	🔴	yes	82.5688 -> 82.6155 (0.057%)
Generated_100_100_v150	🔴	yes	1254.7972 -> 1290.9703 (2.883%)
LargeP2POldCsproj	✅	yes	753.0709 -> 751.137 (-0.257%)
roslyn	✅	yes	5370.7354 -> 5334.5412 (-0.674%)

Evaluation: Memory (bytes)

Test	Overall	Significant δ	Value
DotnetConsoleProject	🔴	yes	4944663 -> 4959678 (0.304%)
DotnetWebProject	🔴	yes	6840943 -> 6861401 (0.299%)
DotnetMvcProject	🔴	yes	9028362 -> 9043496 (0.168%)
Picasso	🔴	yes	35430020 -> 35436938 (0.02%)
SmallP2POldCsproj	✅	yes	6034210 -> 5993157 (-0.68%)
SmallP2PNewCsproj	🔴	yes	10577958 -> 10602931 (0.236%)
Generated_100_100_v150	🔴	yes	147579824 -> 147587200 (0.005%)
LargeP2POldCsproj	🔴	yes	86536590 -> 86568520 (0.037%)
roslyn	🔴	yes	585084467 -> 585118839 (0.006%)

Build: Time (ms)

Test	Overall	Significant δ	Value
DotnetConsoleProject	🔴	yes	7.5323 -> 7.6717 (1.851%)
DotnetWebProject	🔴	yes	9.2563 -> 9.39 (1.444%)
DotnetMvcProject	🔴	yes	10.0831 -> 10.2063 (1.222%)
Picasso	✅	yes	2382.0946 -> 1020.6393 (-57.154%)
SmallP2POldCsproj	✅	yes	84.8296 -> 48.2654 (-43.103%)
SmallP2PNewCsproj	✅	yes	87.373 -> 53.141 (-39.179%)

Build: Memory (bytes)

Test	Overall	Significant δ	Value
DotnetConsoleProject	🔴	yes	1151919 -> 1174937 (1.998%)
DotnetWebProject	✅	yes	1302788 -> 1302090 (-0.054%)
DotnetMvcProject	🔴	yes	1476974 -> 1482962 (0.405%)
Picasso	✅	yes	277497653 -> 125792697 (-54.669%)
SmallP2POldCsproj	✅	yes	9756337 -> 6211117 (-36.338%)
SmallP2PNewCsproj	✅	yes	10009262 -> 6611995 (-33.941%)

AndyGerlicher · 2017-10-08T04:05:49Z

FYI this will be CI red until NuGet/NuGet.Client#1737 goes in and publishes and I can update the package version. In #2594 I updated our bootstrap to pull that task from a package so it should be just a version bump.

If anyone wants to set this up locally, run cibuild --boostrap-only and edit NuGet.targets to match the one in the NuGet PR. Then add a Directory.Build.props with this (replacing the path to where you built NuGet):

<Project>
<PropertyGroup>
 <RestoreTaskAssemblyFile>D:\src\NuGet.Client\artifacts\NuGet.Build.Tasks\15.0\bin\Debug\net45\NuGet.Build.Tasks.dll</RestoreTaskAssemblyFile>
</PropertyGroup>
</Project>

nguerrera · 2017-10-09T17:26:00Z

src/Tasks/Microsoft.Common.CrossTargeting.targets

+        <TargetFrameworks Condition="'$(TargetFrameworks)' == ''">$(TargetFramework)</TargetFrameworks>
+        <HasSingleTargetFramework>true</HasSingleTargetFramework>
+        <HasSingleTargetFramework Condition="'$(IsCrossTargetingBuild)' == 'true'">false</HasSingleTargetFramework>
+        <IsRidAgnostic>true</IsRidAgnostic>


Why is this hard-coded?

See https://github.com/dotnet/sdk/blob/d7730b73a5bea6c1e7f5d178711322082c93d96e/src/Tasks/Microsoft.NET.Build.Tasks/build/Microsoft.NET.Sdk.Common.targets#L71-L73

and dotnet/sdk#696, which might regress.

We need to get the SDK tests running with this change. cc @dsplaisted

nguerrera · 2017-10-09T17:27:24Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+        <TargetFrameworks Condition="'$(TargetFrameworks)' == ''">$(TargetFramework)</TargetFrameworks>
+        <HasSingleTargetFramework>true</HasSingleTargetFramework>
+        <HasSingleTargetFramework Condition="'$(IsCrossTargetingBuild)' == 'true'">false</HasSingleTargetFramework>
+        <IsRidAgnostic>true</IsRidAgnostic>


nguerrera · 2017-10-09T17:38:24Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

+        <TargetFrameworks Condition="'$(TargetFrameworks)' != ''">$(TargetFrameworks)</TargetFrameworks>
+        <TargetFrameworks Condition="'$(TargetFrameworks)' == ''">$(TargetFramework)</TargetFrameworks>
+        <HasSingleTargetFramework>true</HasSingleTargetFramework>
+        <HasSingleTargetFramework Condition="'$(IsCrossTargetingBuild)' == 'true'">false</HasSingleTargetFramework>


I think this is always going to be false when we land in the non-cross-targeting version of GetTargetFrameworks.

nguerrera

See above and also the comment about too much logic going in to NuGet on the NuGet PR.

Handle IsRidAgnostic

nguerrera · 2017-10-10T22:47:31Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

-                                   Condition="'@(_ProjectReferenceTargetFrameworkPossibilities->Count())' != '0'">
+    <GetReferenceNearestTargetFrameworkTask AnnotatedProjectReferences="@(_ProjectReferenceTargetFrameworkPossibilities)"
+                                            CurrentProjectTargetFramework="$(ReferringTargetFrameworkForProjectReferences)"
+                                            CurrentProjectName="MSBuildProjectName"


$(MSBuildProjectName) ?

Fix MSBuildProjectName

AndyGerlicher · 2017-10-10T23:08:31Z

Updated numbers:

Evaluation: Time (ms)

Test	Overall	Significant δ	Value
DotnetConsoleProject	✅	yes	29.0593 -> 28.9913 (-0.234%)
DotnetWebProject	✅	yes	44.2305 -> 44.1433 (-0.197%)
DotnetMvcProject	🔴	yes	50.4046 -> 50.653 (0.493%)
Picasso	✅	yes	256.6368 -> 255.6237 (-0.395%)
SmallP2POldCsproj	🔴	yes	42.4037 -> 42.7942 (0.921%)
SmallP2PNewCsproj	🔴	yes	77.9943 -> 78.3615 (0.471%)
Generated_100_100_v150	✅	yes	1260.2155 -> 1257.19 (-0.24%)
LargeP2POldCsproj	🔴	yes	742.7307 -> 744.3613 (0.22%)
roslyn	✅	yes	3306.3346 -> 3275.4293 (-0.935%)

Evaluation: Memory (bytes)

Test	Overall	Significant δ	Value
DotnetConsoleProject	✅	yes	4895467 -> 4893821 (-0.034%)
DotnetWebProject	🔴	yes	6561947 -> 6565272 (0.051%)
DotnetMvcProject	🔴	yes	7408101 -> 7414780 (0.09%)
Picasso	✅	yes	35314766 -> 35290989 (-0.067%)
SmallP2POldCsproj	✅	yes	6017226 -> 5984496 (-0.544%)
SmallP2PNewCsproj	🔴	yes	10358236 -> 10366868 (0.083%)
Generated_100_100_v150	🔴	yes	147536691 -> 147553504 (0.011%)
LargeP2POldCsproj	🔴	yes	86257330 -> 86264699 (0.009%)
roslyn	👌	no	407241006 -> 408094764 (0.21%)

Build: Time (ms)

Test	Overall	Significant δ	Value
DotnetConsoleProject	✅	yes	7.597 -> 7.563 (-0.448%)
DotnetWebProject	✅	yes	9.2922 -> 9.2719 (-0.218%)
DotnetMvcProject	🔴	yes	9.9721 -> 10.0969 (1.251%)
Picasso	✅	yes	2309.7216 -> 1015.8582 (-56.018%)
SmallP2POldCsproj	✅	yes	84.6579 -> 48.6706 (-42.509%)
SmallP2PNewCsproj	✅	yes	82.0603 -> 50.1221 (-38.92%)

Build: Memory (bytes)

Test	Overall	Significant δ	Value
DotnetConsoleProject	::ok_hand:	no	1155168 -> 1154402 (-0.066%)
DotnetWebProject	🔴	yes	1279572 -> 1281203 (0.127%)
DotnetMvcProject	🔴	yes	1452351 -> 1457735 (0.371%)
Picasso	✅	yes	275855704 -> 125280131 (-54.585%)
SmallP2POldCsproj	✅	yes	9746673 -> 6192245 (-36.468%)
SmallP2PNewCsproj	✅	yes	9733262 -> 6400177 (-34.244%)

See https://github.com/dotnet/sdk/pull/418/files

dsplaisted · 2017-10-11T00:52:14Z

documentation/ProjectReference-Protocol.md

@@ -1,4 +1,4 @@
-# The `ProjectReference` Protocol
+# The `ProjectReference` Protocol


Are the changes to this file up-to-date after the changes to the protocol and interface with NuGet that were made in this PR?

dsplaisted · 2017-10-11T00:55:07Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

-    build against the most appropriate target for the referring target framework.
+    Builds the GetTargetFrameworks target of all existent project references to get a list
+    of all supported TargetFrameworks of the referenced projects. For each project, call
+    GetReferenceNearestTargetFrameworkTask to determine the closest match. This allows a


Reword this to say "Call GetReferenceNearestTargetFrameworkTask to determine the closest match for each project." This way it won't imply that the task is called once for project.

dsplaisted · 2017-10-11T00:57:42Z

src/Tasks/Microsoft.Common.CurrentVersion.targets

-        <UndefineProperties Condition="!$(_ProjectReferenceTargetFrameworkProperties.Contains(`ProjectHasSingleTargetFramework=true`))">%(_MSBuildProjectReferenceExistent.UndefineProperties);ProjectHasSingleTargetFramework</UndefineProperties>
-      </_MSBuildProjectReferenceExistent>
+    <ItemGroup>
+      <AnnotatedProjects Include="@(_ProjectReferenceTargetFrameworkPossibilities)"


I'd suggest adding a comment here explaining this logic (possibly linking to dotnet/sdk#416)

Doesn't look like the package is signed so this likely won't work.

This is needed to get the import to get the automatic import of NuGet.targets.

AndyGerlicher · 2017-10-11T06:44:29Z

.NET Core doesn't care if the dlls are delay-signed, but we're not going to get a green build for full framework until it's signed.

Projects that opt-out of GetTargetFrameworks must be preserved (e.g. C# -> C++ references).

NuGet.targets was added as a hard dependency in dotnet#2595, this change will explicitly import it in Microsoft.Common.CurrentVersion.targets.

* Get properties for ProjectReference in parallel In order to avoid batching the _GetProjectReferenceTargetFrameworkProperties target for each reference, the ProjectReference protocol can be amended to return an item from GetTargetFrameworkProperties instead of a semicolon-delimited list of key-value pairs. This allows a single build request to be sent to the engine, and allows resolving references in parallel on multiprocess builds. * Identify inner-build in consuming project * Call GetTargetFrameworks target to identify target frameworks in referenced projects. Since TargetFrameworks is not set, this will avoid a 2nd evaluation when the project single targets. * Adds dependency on NuGet GetReferenceNearestTargetFrameworkTask. * Add NuGet ImportAfter targets to bootstrapped build This is needed to get the import to get the automatic import of NuGet.targets.

* Explicitly import NuGet.targets NuGet.targets was added as a hard dependency in #2595, this change will explicitly import it in Microsoft.Common.CurrentVersion.targets. * Copy NuGet references for tests * Remove NuGet ImportAfter from bootstrap build

AndyGerlicher requested a review from nguerrera October 5, 2017 20:28

dnfclas added the cla-already-signed label Oct 5, 2017

nguerrera requested review from dsplaisted and davkean October 5, 2017 20:32

AndyGerlicher mentioned this pull request Oct 5, 2017

Cleanup bootstrap build #2594

Merged

rainersigwald added 5 commits October 6, 2017 13:59

Clarify existing ProjectReference protocol returns

8729689

In preparation for describing the changes to return values I'd like to make, I need to document what the old ones were.

Document new GetTargetFrameworkProperties return

984f381

Document the more-succinct-but-compatibility-breaking single-item-with-metadata return possibility for GetTargetFrameworkProperties.

Documentation for protocol change

c36bfb3

AndyGerlicher force-pushed the double-eval branch from dd50373 to 3eb23d3 Compare October 6, 2017 23:13

AndyGerlicher force-pushed the double-eval branch from 3eb23d3 to bb44739 Compare October 7, 2017 06:04

nguerrera reviewed Oct 9, 2017

View reviewed changes

nguerrera suggested changes Oct 9, 2017

View reviewed changes

davkean added the performance label Oct 10, 2017

Move logic out of NuGet

f066173

Handle IsRidAgnostic

nguerrera reviewed Oct 10, 2017

View reviewed changes

Update comments

caf92b2

Fix MSBuildProjectName

Fix issue when consuming project TFM is null.

62094fd

See https://github.com/dotnet/sdk/pull/418/files

dsplaisted approved these changes Oct 11, 2017

View reviewed changes

nguerrera approved these changes Oct 11, 2017

View reviewed changes

davkean mentioned this pull request Oct 11, 2017

.NET Core/.NET Standard build performance dotnet/project-system#2789

Closed

AndyGerlicher added 3 commits October 10, 2017 19:10

Update NuGet.Build.Tasks

21b9a95

Doesn't look like the package is signed so this likely won't work.

Add NuGet ImportAfter targets to bootstrapped build

15afc90

This is needed to get the import to get the automatic import of NuGet.targets.

Comment cleanup

00e2059

Preserve project references that opt-out

e3ee36f

Projects that opt-out of GetTargetFrameworks must be preserved (e.g. C# -> C++ references).

dsplaisted mentioned this pull request Oct 11, 2017

NuGet restore fails with .NET452 -> UAP ref (valid for UWP designer) NuGet/Home#5915

Closed

This was referenced Oct 11, 2017

Identify inner build in consuming project #2472

Closed

Get TargetFrameworks for ProjectReferences in parallel #2458

Closed

AndyGerlicher merged commit 57ae27c into dotnet:master Oct 11, 2017

davkean mentioned this pull request Oct 11, 2017

Avoid double evaluation of all ProjectReferences #1276

Closed

davkean added this to the MSBuild 15.5 milestone Oct 11, 2017

This was referenced Oct 12, 2017

MSBuild 15.5.153 (No Double Evaluation Fix) dotnet/cli#7813

Merged

MSBuild 15.5.154 dotnet/cli#7814

Merged

AndyGerlicher added a commit to AndyGerlicher/msbuild that referenced this pull request Oct 24, 2017

Explicitly import NuGet.targets

6d858d6

NuGet.targets was added as a hard dependency in dotnet#2595, this change will explicitly import it in Microsoft.Common.CurrentVersion.targets.

AndyGerlicher mentioned this pull request Oct 24, 2017

Explicitly import NuGet.targets #2663

Merged

AndyGerlicher deleted the double-eval branch June 26, 2018 20:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Double Evaluation Fix #2595

Double Evaluation Fix #2595

AndyGerlicher commented Oct 5, 2017

AndyGerlicher commented Oct 8, 2017

AndyGerlicher commented Oct 8, 2017

nguerrera Oct 9, 2017

nguerrera Oct 9, 2017

nguerrera Oct 9, 2017

nguerrera Oct 9, 2017

nguerrera Oct 9, 2017

nguerrera left a comment

nguerrera Oct 10, 2017

AndyGerlicher commented Oct 10, 2017

dsplaisted Oct 11, 2017

dsplaisted Oct 11, 2017

dsplaisted Oct 11, 2017

AndyGerlicher commented Oct 11, 2017

		@@ -1,4 +1,4 @@
		# The `ProjectReference` Protocol
		# The `ProjectReference` Protocol

Double Evaluation Fix #2595

Double Evaluation Fix #2595

Conversation

AndyGerlicher commented Oct 5, 2017

AndyGerlicher commented Oct 8, 2017

Evaluation: Time (ms)

Evaluation: Memory (bytes)

Build: Time (ms)

Build: Memory (bytes)

AndyGerlicher commented Oct 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguerrera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyGerlicher commented Oct 10, 2017

Evaluation: Time (ms)

Evaluation: Memory (bytes)

Build: Time (ms)

Build: Memory (bytes)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyGerlicher commented Oct 11, 2017