-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Double Evaluation Fix #2595
Double Evaluation Fix #2595
Conversation
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.
dd50373
to
3eb23d3
Compare
* 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.
3eb23d3
to
bb44739
Compare
@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 Evaluation: Time (ms)
Evaluation: Memory (bytes)
Build: Time (ms)
Build: Memory (bytes)
|
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 <Project>
<PropertyGroup>
<RestoreTaskAssemblyFile>D:\src\NuGet.Client\artifacts\NuGet.Build.Tasks\15.0\bin\Debug\net45\NuGet.Build.Tasks.dll</RestoreTaskAssemblyFile>
</PropertyGroup>
</Project> |
<TargetFrameworks Condition="'$(TargetFrameworks)' == ''">$(TargetFramework)</TargetFrameworks> | ||
<HasSingleTargetFramework>true</HasSingleTargetFramework> | ||
<HasSingleTargetFramework Condition="'$(IsCrossTargetingBuild)' == 'true'">false</HasSingleTargetFramework> | ||
<IsRidAgnostic>true</IsRidAgnostic> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this hard-coded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to get the SDK tests running with this change. cc @dsplaisted
<TargetFrameworks Condition="'$(TargetFrameworks)' == ''">$(TargetFramework)</TargetFrameworks> | ||
<HasSingleTargetFramework>true</HasSingleTargetFramework> | ||
<HasSingleTargetFramework Condition="'$(IsCrossTargetingBuild)' == 'true'">false</HasSingleTargetFramework> | ||
<IsRidAgnostic>true</IsRidAgnostic> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
<TargetFrameworks Condition="'$(TargetFrameworks)' != ''">$(TargetFrameworks)</TargetFrameworks> | ||
<TargetFrameworks Condition="'$(TargetFrameworks)' == ''">$(TargetFramework)</TargetFrameworks> | ||
<HasSingleTargetFramework>true</HasSingleTargetFramework> | ||
<HasSingleTargetFramework Condition="'$(IsCrossTargetingBuild)' == 'true'">false</HasSingleTargetFramework> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is always going to be false when we land in the non-cross-targeting version of GetTargetFrameworks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above and also the comment about too much logic going in to NuGet on the NuGet PR.
Handle IsRidAgnostic
Condition="'@(_ProjectReferenceTargetFrameworkPossibilities->Count())' != '0'"> | ||
<GetReferenceNearestTargetFrameworkTask AnnotatedProjectReferences="@(_ProjectReferenceTargetFrameworkPossibilities)" | ||
CurrentProjectTargetFramework="$(ReferringTargetFrameworkForProjectReferences)" | ||
CurrentProjectName="MSBuildProjectName" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$(MSBuildProjectName) ?
Fix MSBuildProjectName
Updated numbers: Evaluation: Time (ms)
Evaluation: Memory (bytes)
Build: Time (ms)
Build: Memory (bytes)
|
@@ -1,4 +1,4 @@ | |||
# The `ProjectReference` Protocol | |||
# The `ProjectReference` Protocol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
<UndefineProperties Condition="!$(_ProjectReferenceTargetFrameworkProperties.Contains(`ProjectHasSingleTargetFramework=true`))">%(_MSBuildProjectReferenceExistent.UndefineProperties);ProjectHasSingleTargetFramework</UndefineProperties> | ||
</_MSBuildProjectReferenceExistent> | ||
<ItemGroup> | ||
<AnnotatedProjects Include="@(_ProjectReferenceTargetFrameworkPossibilities)" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
.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
Replaces #2472 and #2458
Still WIP. I need to get the NuGet task pulled down for bootstrap build.