Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Avoid x-targeting perf impact on vcxproj #1278

Merged
merged 1 commit into from
Oct 28, 2016
Merged

Avoid x-targeting perf impact on vcxproj #1278

merged 1 commit into from
Oct 28, 2016
+7 −2

Conversation

rainersigwald
Copy link
Member

Targeted mitigation for #1276.

VC++ noticed a dramatic regression in solution-load performance related to
the time cost of ResolveProjectReferences. That affects all projects, but
we can safely assume that a .vcxproj is NOT using cross-targeting, while
it's impossible to detect from the outside whether a .csproj is.

This commit avoids the regression for C++ projects by just not calling the
MSBuild task again for .vcxproj references.

/cc @nguerrera @olgaark @srivatsn

@rainersigwald rainersigwald added this to the Visual Studio 15 RC milestone Oct 27, 2016
@nguerrera
Copy link
Contributor

nguerrera commented Oct 27, 2016 •
edited
Loading

Was about to say looks good then saw the error is related to your change:

error MSB4116: The condition "'%(_MSBuildProjectReferenceExistent.Extension)' != '.vcxproj'" 
on the "_GetProjectReferenceTargetFrameworkProperties" target has a reference to item 
metadata. References to item metadata are not allowed in target conditions unless they
are part of an item transform.

@rainersigwald
Copy link
Member Author

Someday I will either internalize that restriction or remove it! Looking.

@rainersigwald
Avoid x-targeting perf impact on vcxproj
36a8c7f 
Targeted mitigation for dotnet#1276.

VC++ noticed a dramatic regression in solution-load performance related to
the time cost of ResolveProjectReferences. That affects all projects, but
we can safely assume that a .vcxproj is NOT using cross-targeting, while
it's impossible to detect from the outside whether a .csproj is.

This commit avoids the regression for C++ projects by just not calling the
MSBuild task again for .vcxproj references.
@rainersigwald rainersigwald force-pushed the vcxproj-avoid-negotiation branch from 7267638 to 36a8c7f Compare October 27, 2016 23:43
@rainersigwald rainersigwald reopened this Oct 27, 2016
@olgaark
Copy link
Collaborator

olgaark commented Oct 28, 2016

Looks good to me too. Thanks!

From: Nick Guerrera [mailto:[email protected]]
Sent: Thursday, October 27, 2016 4:47 PM
To: Microsoft/msbuild [email protected]
Cc: Olga Arkhipova [email protected]; Mention [email protected]
Subject: Re: [Microsoft/msbuild] Avoid x-targeting perf impact on vcxproj (#1278)

@nguerrera approved this pull request.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2Fmsbuild%2Fpull%2F1278%23pullrequestreview-6166985&data=02%7C01%7Colgaark%40microsoft.com%7C01dd8cf12371412f54d208d3fec39474%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636132088368420009&sdata=gJqmEIMwNknZ43HG5TTxZJb50DjQUt%2BajAZkvzR9E%2FI%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAVapcjtlphEDdo6trLuZnuoGgcW1owNzks5q4TgBgaJpZM4Ki67t&data=02%7C01%7Colgaark%40microsoft.com%7C01dd8cf12371412f54d208d3fec39474%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636132088368420009&sdata=avN67xXamEWwlsvMLXghKBNVa1PSNNgW25Vc1peoF3Q%3D&reserved=0.

@rainersigwald
Copy link
Member Author

I am confirming that a) this doesn't break cross-targeting for .csproj, and b) it does reduce time for .vcxproj.

@rainersigwald
Copy link
Member Author

Validated:

  • Significant reduction in _PerfIntellisenseInfo execution time on the customer project (.vcxproj)
  • Cross-targeting solution from @nguerrera returns a netstandard1.3 assembly when called from a .NET 4.6 project, and netstandard1.1 when called from a .NET 4.5 project.

@rainersigwald rainersigwald merged commit b6ff43b into dotnet:master Oct 28, 2016
@rainersigwald rainersigwald deleted the vcxproj-avoid-negotiation branch October 28, 2016 18:47
rainersigwald added a commit to rainersigwald/msbuild that referenced this pull request Mar 15, 2017
@rainersigwald
Ask ProjectReferences if they cross-target
69a3a3e 
Before cross-targeting was implemented, a project had a single output
that was queried in ResolveProjectReferences by either `GetTargetPath`
(in single-project-build scenarios) or the default target (`Build`) in
command line builds.

A cross-targeting project doesn't have a single output--it has one for
each of its `TargetFrameworks`. A ProjectReference, then, has to first
figure out which output to ask for--the one that's the best fit for the
current project.

The initial implementation of this was simple: unconditionally call
`GetTargetFrameworkProperties` on every ProjectReference, passing a
`ReferringTargetFramework`. Then, if the project cross-targets, specify
the resulting TF on future calls to the ProjectReference. If the project
has a single TF, do not specify it to avoid building it once (from the
solution  build) and again (from a reference, explicitly specifying the
one TF).

Unfortunately, adding a global property for `ReferringTargetFramework`
will result in a separate evaluation for the project. This means that
almost _every_ project in a solution will be evaluated at least
twice--once directly, with no special global properties, and once for
each unique `ReferringTargetFramework` that refers to it. This can be
particularly onerous for C++ projects (.vcxproj), where evaluation time
is nontrivial and doubling it is noticeable. That was mitigated by
special-casing vcxproj in dotnet#1278, but that is inelegant and doesn't
address the root cause.

This change alters the calling pattern for `ProjectReference`s. Instead
of asking all references for the correct TF, it breaks the problem into
multiple phases:

* Ask all references whether they cross-target, and split into
cross-targeting and non-cross-targeting lists.
* Perform the `GetTargetFrameworkProperties` query only on projects that
reported that they cross-targeted.
* Update (in place) the existing `_MSBuildProjectReferenceExistent` item
to pass or undefine the relevant properties based on whether the
reference cross-targets (and what TF is the best match).

Fixes dotnet#1276.
@rainersigwald rainersigwald mentioned this pull request Mar 15, 2017
Closed
rainersigwald added a commit to rainersigwald/msbuild that referenced this pull request Aug 4, 2017
@rainersigwald
Ask ProjectReferences if they cross-target
4b6c96a 
Before cross-targeting was implemented, a project had a single output
that was queried in ResolveProjectReferences by either `GetTargetPath`
(in single-project-build scenarios) or the default target (`Build`) in
command line builds.

A cross-targeting project doesn't have a single output--it has one for
each of its `TargetFrameworks`. A ProjectReference, then, has to first
figure out which output to ask for--the one that's the best fit for the
current project.

The initial implementation of this was simple: unconditionally call
`GetTargetFrameworkProperties` on every ProjectReference, passing a
`ReferringTargetFramework`. Then, if the project cross-targets, specify
the resulting TF on future calls to the ProjectReference. If the project
has a single TF, do not specify it to avoid building it once (from the
solution  build) and again (from a reference, explicitly specifying the
one TF).

Unfortunately, adding a global property for `ReferringTargetFramework`
will result in a separate evaluation for the project. This means that
almost _every_ project in a solution will be evaluated at least
twice--once directly, with no special global properties, and once for
each unique `ReferringTargetFramework` that refers to it. This can be
particularly onerous for C++ projects (.vcxproj), where evaluation time
is nontrivial and doubling it is noticeable. That was mitigated by
special-casing vcxproj in dotnet#1278, but that is inelegant and doesn't
address the root cause.

This change alters the calling pattern for `ProjectReference`s. Instead
of asking all references for the correct TF, it breaks the problem into
multiple phases:

* Ask all references whether they cross-target, and split into
cross-targeting and non-cross-targeting lists.
* Perform the `GetTargetFrameworkProperties` query only on projects that
reported that they cross-targeted.
* Update (in place) the existing `_MSBuildProjectReferenceExistent` item
to pass or undefine the relevant properties based on whether the
reference cross-targets (and what TF is the best match).

Fixes dotnet#1276.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nguerrera nguerrera nguerrera approved these changes

Assignees
No one assigned
Labels
performance
Projects
None yet
Milestone
Visual Studio 15 RC
Development

Successfully merging this pull request may close these issues.
5 participants
@rainersigwald @nguerrera @olgaark @dnfclas @Sarabeth-Jaffe-Microsoft