Rewrite resolveprojectreferences MSBuild calls to reduce the amount of batching done

[baronfel]

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:

• GetTargetPath
• default or explicitly-specified build targets
• GetNativeManifest

Testing

None additional, as this should be a not-publicly-visible change.

Notes

[Copilot]

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);
                    }
                }

[ViktorHofer]

This makes sense to me. I noticed the same pattern a while ago but thought that it was intentional.

[baronfel]

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.

[baronfel]

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.