Rewrite resolveprojectreferences MSBuild calls to reduce the amount of batching done#12631
Rewrite resolveprojectreferences MSBuild calls to reduce the amount of batching done#12631
Conversation
baronfel
added
the
Area: Tasks
There was a problem hiding this comment.
Pull request overview
This PR attempts to optimize the ResolveProjectReferences target by reducing MSBuild task batching overhead. Instead of using batching directly in the MSBuild task calls (which can result in multiple task invocations), the PR precomputes three item lists (_ProjectsForGetTargetPath, _ProjectsForCommandLineBuild, _ProjectsForNativeManifest) with all necessary metadata attached, allowing each MSBuild task to be called just once per logical build phase.
However, the PR contains several critical bugs that prevent it from compiling and would cause runtime failures:
- Compilation error: The new ExpandItemList method is declared as static but attempts to access the instance property TargetAndPropertyListSeparators, which is a compile-time error in C#
- Critical logic bug: The UndefineProperties handling was incorrectly changed, causing project-specific properties to be silently dropped when global properties are also specified
- Critical syntax bug: RemoveProperties concatenation was changed from direct string concatenation to semicolon-separated concatenation, breaking the expected format
- Moderate bug: Dictionary capacity calculation has an operator precedence error
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| src/Tasks/Microsoft.Common.CurrentVersion.targets | Restructures ResolveProjectReferences target to precompute item lists, reducing batching; contains critical RemoveProperties concatenation bugs |
| src/Tasks/MSBuild.cs | Adds ExpandItemList helper method and refactors property/target parsing; contains critical compilation error and logic bugs |
| src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/MSBuild.cs | Parallel changes to MSBuild.cs for intrinsic task implementation; contains same critical bugs |
Comments suppressed due to low confidence (4)
src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/MSBuild.cs:484
- This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
foreach (string p in Properties)
{
// Split each property according to the separators
string[] expandedPropertyValues = ExpandItemList(p);
// Add the resultant properties to the final list
foreach (string property in expandedPropertyValues)
{
expandedProperties.Add(property);
}
}
src/Build/BackEnd/Components/RequestBuilder/IntrinsicTasks/MSBuild.cs:501
- This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
foreach (string t in Targets)
{
// Split each target according to the separators
string[] expandedTargetValues = ExpandItemList(t);
// Add the resultant targets to the final list
foreach (string target in expandedTargetValues)
{
expandedTargets.Add(target);
}
}
src/Tasks/MSBuild.cs:439
- This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
foreach (string t in Properties)
{
// Split each property according to the separators
string[] expandedPropertyValues = ExpandItemList(t);
// Add the resultant properties to the final list
foreach (string property in expandedPropertyValues)
{
expandedProperties.Add(property);
}
}
src/Tasks/MSBuild.cs:457
- This foreach loop immediately maps its iteration variable to another variable - consider mapping the sequence explicitly using '.Select(...)'.
foreach (string t in Targets)
{
// Split each target according to the separators
string[] expandedTargetValues = ExpandItemList(t);
// Add the resultant targets to the final list
foreach (string target in expandedTargetValues)
{
expandedTargets.Add(target);
}
}
c895e75 to
7e0b7c9
Compare
|
This makes sense to me. I noticed the same pattern a while ago but thought that it was intentional. |
| <ItemGroup> | ||
| <_ProjectsForGetTargetPath Include="@(_MSBuildProjectReferenceExistent)" | ||
| Condition="'%(_MSBuildProjectReferenceExistent.BuildReference)' == 'true' and '@(ProjectReferenceWithConfiguration)' != '' and ('$(BuildingInsideVisualStudio)' == 'true' or '$(BuildProjectReferences)' != 'true') and '$(VisualStudioVersion)' != '10.0' and '@(_MSBuildProjectReferenceExistent)' != ''" | ||
| Properties="%(_MSBuildProjectReferenceExistent.SetConfiguration);%(_MSBuildProjectReferenceExistent.SetPlatform);%(_MSBuildProjectReferenceExistent.SetTargetFramework)" | ||
| RemoveProperties="%(_MSBuildProjectReferenceExistent.GlobalPropertiesToRemove);$(_GlobalPropertiesToRemoveFromProjectReferences)" /> | ||
| </ItemGroup> |
There was a problem hiding this comment.
The changes to this file are the actual operational fixes - creating the Items ahead of time so that only one invocation of each MSBuild Task below fires. You should see this 'create then execute' pattern once for each step of the ResolveProjectReferences Target.
However, this pre-computation creates Items with Metadata that are present and empty (or effectively empty, for example ;; is an empty list when expanded), but the current per-project-Metadata-processing routines in the MSBuild Task implementations assumed that any non-null-or-empty string meant that there would be values to overwrite, so this refactoring resulted in a lot of additional chatter in the binlogs.
Because of this, I also had to go update the MSBuild Task invocations to check if the values of the metadata were effectively empty or not before doing any processing or logging.
| string[] undefinePropertiesArray = ExpandItemList(TargetAndPropertyListSeparators ?? SemicolonSeparatorArray, RemoveProperties); | ||
| if (undefinePropertiesArray.Length > 0) |
There was a problem hiding this comment.
All of the changes to metadata handling are because of the presence of the not-null-but-semantically-empty metadata values. In each case we expand them out and trim empty values and do comparisons on that list to determine if any modifications need to be made. And because TargetAndPropertyListSeparators is an instance value we can't reference it directly from ExpandItemList, etc etc.
…ed in on Project TaskItems via metadata
baronfel
force-pushed
the
optimize-ResolveProjectReferences-MSBuild-Task-batching
branch
from
7e0b7c9 to
05e19c6
Compare
|
This pull request has been automatically closed because it has been open for more than 180 days with no recent activity. If you believe this work is still relevant, please feel free to reopen or create a new pull request. Thank you for your contribution! Note 🔒 Integrity filter blocked 39 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
|
This pull request has been automatically closed because it has been open for more than 180 days with no recent activity. If you believe this work is still relevant, please feel free to reopen or create a new pull request. Thank you for your contribution! Note 🔒 Integrity filter blocked 43 itemsThe following items were blocked because they don't meet the GitHub integrity level.
To allow these resources, lower tools:
github:
min-integrity: approved # merged | approved | unapproved | none
|
Context
When writing microsoft/vstest#15295, @rainersigwald pointed out to me that we suffer from the same anti-pattern with regards to usage of the MSBuild Task in ResolveProjectReferences, which is a very-commonly-called Target.
When batching is used directly in the MSBuild Task call, it can lead to repeated invocations of the Task, which have small but noticeable overheads. The fix is to push as much information about the customization of the individual Project builds being requested as possible into the Item descriptions of the projects themselves - we do this by precomputing a few item lists, one for each MSBuild call section.
This may increase overall memory usage, so it's worth measuring against the reduction in overhead from the batched Task calls. In my investigations of a recent partner team build, it's very common to end up with at least 2 batches per logical Task request in this particular Target:
Changes Made
Precompute Project Item lists for each of the three steps of ResolveProjectReference:
Testing
None additional, as this should be a not-publicly-visible change.
Notes