From 4392014c578bdc85d87a2b324a77b74340b0cec1 Mon Sep 17 00:00:00 2001 From: David Federman Date: Fri, 22 May 2026 17:18:27 -0700 Subject: [PATCH 1/2] Dedup target list in ProjectGraph.ExpandDefaultTargets to prevent graph explosion ProjectGraph.ExpandDefaultTargets now unconditionally dedupes its output via a hybrid fast/slow path. The downstream BFS cross-products entry-targets against matching PRT items, so any N-duplicate entry list at one hop becomes ~N^2 propagations at the next; over BFS depth D this is N^D edges. Duplicates arise from PRT marker double-emission and from explicit literal duplicates in Targets metadata. ExpandDefaultTargets has a zero-allocation fast path (inline O(n^2) scan for n <= 8) returning the input unchanged when no marker or duplicate is found, and a HashSet-backed slow path otherwise. Dedup is OrdinalIgnoreCase, first-occurrence wins, matching the existing post-BFS dedup at the public GetTargetLists boundary -- so no consumer-observable behavior changes and no ChangeWave is needed. BFS hot path moved off ImmutableList/ImmutableList: ProjectGraphBuildRequest.RequestedTargets, ExpandDefaultTargets, TargetsToPropagate.FromProjectAndEntryTargets, and GetApplicableTargetsForReference all flow string[] end-to-end. TargetsToPropagate collapses two ImmutableLists to one flat TargetSpecification[] + outer-build count. LINQ removed from the per-edge loop in FromProjectAndEntryTargets. ImmutableList is retained only at the public GetTargetLists boundary and in targetLists[node] where the AddRange chain actually derives each version from the prior. 11 new tests (22 with TFMs) cover dedup behavior plus end-to-end GetTargetLists smoke at depth 12 with the duplicate-marker shape and depth 6 with the common single-marker shape. End-to-end GetTargetLists(["Build"]) benchmark on realistic .NET-shape graphs: 19-51% faster and 24-31% lower allocation across small/medium/large trees and linear chains; the pathological duplicate-marker shape is 187x faster and 172x lower allocation at 50 nodes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ...ctGraph_ExpandDefaultTargetsDedup_Tests.cs | 313 ++++++++++++++++++ src/Build/Graph/ProjectGraph.cs | 170 ++++++++-- src/Build/Graph/ProjectInterpretation.cs | 111 +++++-- 3 files changed, 528 insertions(+), 66 deletions(-) create mode 100644 src/Build.UnitTests/Graph/ProjectGraph_ExpandDefaultTargetsDedup_Tests.cs diff --git a/src/Build.UnitTests/Graph/ProjectGraph_ExpandDefaultTargetsDedup_Tests.cs b/src/Build.UnitTests/Graph/ProjectGraph_ExpandDefaultTargetsDedup_Tests.cs new file mode 100644 index 00000000000..044bd829a32 --- /dev/null +++ b/src/Build.UnitTests/Graph/ProjectGraph_ExpandDefaultTargetsDedup_Tests.cs @@ -0,0 +1,313 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using Microsoft.Build.Evaluation; +using Microsoft.Build.Execution; +using Microsoft.Build.Shared; +using Microsoft.Build.UnitTests; +using Shouldly; +using Xunit; + +namespace Microsoft.Build.Graph.UnitTests +{ + /// + /// Regression coverage for the dedup inside . + /// + /// Background: GetTargetLists performs a BFS over the graph and calls + /// ExpandDefaultTargets once per traversed edge. Each call's output becomes the + /// next hop's input. If the propagated list contains duplicates from any source — + /// marker expansion against duplicate-bearing + /// <ProjectReferenceTargets Targets="…"/> metadata, multiple PRT items + /// independently emitting marker-containing metadata for the same entry target, an + /// explicit literal duplicate like Targets="Build;Build;Build", or upstream + /// propagation accumulating duplicates — the next hop's + /// + /// iterates each duplicate entry target against every matching PRT, multiplying the + /// propagated count by N per hop and producing N^depth growth. + /// + /// The fix dedupes inside ExpandDefaultTargets regardless of input shape, so + /// the result handed back to the BFS is always duplicate-free. The outer post-BFS + /// dedup in GetTargetLists still runs and collapses each per-node final list, + /// so the publicly-observable result of GetTargetLists is unchanged — this is a + /// purely structural / performance fix. + /// + public class ProjectGraph_ExpandDefaultTargetsDedup_Tests + { + private readonly ITestOutputHelper _output; + + public ProjectGraph_ExpandDefaultTargetsDedup_Tests(ITestOutputHelper output) + { + _output = output; + } + + /// + /// Marker expansion produces duplicates: a literal Build in the input plus + /// the that expands to default + /// targets [Build, X]. Result is deduped to [Build, X]. + /// + [Fact] + public void Dedupes_WhenMarkerExpansionProducesDuplicates() + { + ProjectItemInstance edge = CreateEdge(); + string[] input = ["Build", MSBuildConstants.DefaultTargetsMarker]; + List defaultTargets = ["Build", "X"]; + + string[] result = ProjectGraph.ExpandDefaultTargets(input, defaultTargets, edge); + + result.ShouldBe(["Build", "X"]); + } + + /// + /// The PRT-or-default marker path: two markers in the input each expand to the + /// edge's Targets metadata (A;B;A), producing the geometric amplification + /// shape this fix prevents. Verify the per-call result collapses to the unique set. + /// + [Fact] + public void Dedupes_PRTOrDefaultMarker_WhenTargetsMetadataDuplicatesExpansion() + { + ProjectItemInstance edge = CreateEdgeWithTargetsMetadata("A;B;A"); + string[] input = [ + MSBuildConstants.ProjectReferenceTargetsOrDefaultTargetsMarker, + MSBuildConstants.ProjectReferenceTargetsOrDefaultTargetsMarker]; + List defaultTargets = ["DefaultsShouldNotBeUsed"]; + + string[] result = ProjectGraph.ExpandDefaultTargets(input, defaultTargets, edge); + + result.ShouldBe(["A", "B"]); + } + + /// + /// Explicit non-marker duplicates in the input must also be deduped. Without this, + /// a <ProjectReferenceTargets Include="Build" Targets="Build;Build;Build"/> + /// shape would explode geometrically across BFS hops even though no marker is + /// involved — + /// iterates each entry target against every matching PRT, so an N-duplicate + /// arriving at an edge becomes N^2 propagated at the next hop. + /// + [Fact] + public void Dedupes_ExplicitNonMarkerDuplicates() + { + ProjectItemInstance edge = CreateEdge(); + string[] input = ["Build", "Build", "Build"]; + List defaultTargets = ["DefaultsShouldNotBeUsed"]; + + string[] result = ProjectGraph.ExpandDefaultTargets(input, defaultTargets, edge); + + result.ShouldBe(["Build"]); + } + + /// + /// Dedup is case-insensitive ordinal (matching the equality semantics used by both + /// the outer post-BFS dedup at ProjectGraph.GetTargetLists and + /// ProjectGraphBuildRequest.Equals). First-occurrence wins. + /// + [Fact] + public void Dedupes_IgnoresCase() + { + ProjectItemInstance edge = CreateEdge(); + string[] input = ["Build", MSBuildConstants.DefaultTargetsMarker]; + List defaultTargets = ["BUILD", "x"]; + + string[] result = ProjectGraph.ExpandDefaultTargets(input, defaultTargets, edge); + + result.ShouldBe(["Build", "x"]); + } + + /// + /// Fast path: when no marker is present and no duplicate is detected, the original + /// input reference is returned unchanged — zero allocations beyond the lookup HashSet. + /// + [Fact] + public void NoMarkerNoDuplicates_ReturnsSameInstance() + { + ProjectItemInstance edge = CreateEdge(); + string[] input = ["Build", "X", "Y"]; + List defaultTargets = ["ShouldNotBeUsed"]; + + string[] result = ProjectGraph.ExpandDefaultTargets(input, defaultTargets, edge); + + result.ShouldBeSameAs(input); + } + + /// + /// Marker is expanded but the resulting list has no duplicates. The buffer is + /// allocated (because we crossed the marker), but no entries are dropped. + /// + [Fact] + public void MarkerExpanded_NoDuplicates_ReturnsExpandedList() + { + ProjectItemInstance edge = CreateEdge(); + string[] input = [MSBuildConstants.DefaultTargetsMarker]; + List defaultTargets = ["A", "B"]; + + string[] result = ProjectGraph.ExpandDefaultTargets(input, defaultTargets, edge); + + result.ShouldBe(["A", "B"]); + } + + /// + /// Edge case: a marker that expands to an empty defaultTargets list. The + /// marker is consumed and the result is empty. + /// + [Fact] + public void MarkerExpandsToEmptyDefaults_ReturnsEmptyList() + { + ProjectItemInstance edge = CreateEdge(); + string[] input = [MSBuildConstants.DefaultTargetsMarker]; + List defaultTargets = []; + + string[] result = ProjectGraph.ExpandDefaultTargets(input, defaultTargets, edge); + + result.ShouldBeEmpty(); + } + + /// + /// Edge case: every entry collapses to the same target. Verifies the buffer copies + /// the unique prefix correctly when the prefix is length 1. + /// + [Fact] + public void AllEntriesCollapseToSingleton() + { + ProjectItemInstance edge = CreateEdge(); + string[] input = ["Build", MSBuildConstants.DefaultTargetsMarker, MSBuildConstants.DefaultTargetsMarker]; + List defaultTargets = ["Build"]; + + string[] result = ProjectGraph.ExpandDefaultTargets(input, defaultTargets, edge); + + result.ShouldBe(["Build"]); + } + + /// + /// Verifies the order of first-occurrence wins across a mix of literal, marker + /// expansion, and post-marker literal entries. + /// + [Fact] + public void PreservesFirstOccurrenceOrder() + { + ProjectItemInstance edge = CreateEdge(); + string[] input = ["First", MSBuildConstants.DefaultTargetsMarker, "Last"]; + List defaultTargets = ["Middle", "First"]; + + string[] result = ProjectGraph.ExpandDefaultTargets(input, defaultTargets, edge); + + // "First" (literal at index 0) precedes "Middle" (from marker expansion); the + // second "First" inside defaultTargets is dropped; "Last" follows. + result.ShouldBe(["First", "Middle", "Last"]); + } + + /// + /// End-to-end smoke: at depth 12 with the post-PR-#13427 vcxproj shape (two + /// PRT(Build) items where one item embeds the marker twice inside its + /// Targets metadata), the graph build completes correctly and the per-node final + /// target list contains each target at most once. With the fix in place the BFS + /// runs in sub-second time; without the fix it explodes geometrically. + /// + [Fact] + public void GetTargetLists_DuplicateMarkerPRT_StaysBoundedAcrossChain() + { + using TestEnvironment env = TestEnvironment.Create(_output); + + const int depth = 12; + IReadOnlyDictionary> targetLists = + BuildChainAndGetTargetLists(env, depth, DuplicateMarkerPRTContent); + + foreach (KeyValuePair> pair in targetLists) + { + IGrouping? firstDup = pair.Value + .GroupBy(t => t, StringComparer.OrdinalIgnoreCase) + .FirstOrDefault(g => g.Count() > 1); + + firstDup.ShouldBeNull( + $"per-node final target list must be duplicate-free, but '{firstDup?.Key}' appeared {firstDup?.Count()} times on {pair.Key.ProjectInstance.FullPath}"); + } + } + + /// + /// Sanity: a single-PRT(Build)-with-single-marker chain at depth 6 produces one + /// Build per node — verifies the common case is unaffected. + /// + [Fact] + public void GetTargetLists_SingleMarkerPRT_PropagatesLinearly() + { + using TestEnvironment env = TestEnvironment.Create(_output); + + const int depth = 6; + IReadOnlyDictionary> targetLists = + BuildChainAndGetTargetLists(env, depth, SingleMarkerPRTContent); + + foreach (KeyValuePair> pair in targetLists) + { + pair.Value.Count(t => t.Equals("Build", StringComparison.OrdinalIgnoreCase)) + .ShouldBe(1, $"single-marker chain should resolve to one Build per node on {pair.Key.ProjectInstance.FullPath}"); + } + } + + private static ProjectItemInstance CreateEdge() => CreateEdgeWithTargetsMetadata(targetsMetadata: null); + + private static ProjectItemInstance CreateEdgeWithTargetsMetadata(string? targetsMetadata) + { + ProjectInstance projectInstance = new Project().CreateProjectInstance(); + ProjectItemInstance edge = new ProjectItemInstance(projectInstance, "ProjectReference", "ref.csproj", projectInstance.FullPath); + if (!string.IsNullOrEmpty(targetsMetadata)) + { + edge.SetMetadata("Targets", targetsMetadata); + } + + return edge; + } + + private static IReadOnlyDictionary> BuildChainAndGetTargetLists( + TestEnvironment env, + int depth, + string extraContent) + { + // Chain: 1 -> 2 -> 3 -> ... -> depth. + var edges = new Dictionary(); + for (int i = 1; i < depth; i++) + { + edges[i] = [i + 1]; + } + edges[depth] = []; + + ProjectGraph graph = Helpers.CreateProjectGraph( + env: env, + dependencyEdges: edges, + extraContentForAllNodes: extraContent); + + return graph.GetTargetLists(["Build"]); + } + + // Mirrors the post-PR-#13427 vcxproj state: + // - PRT(Build, "marker;GetNativeManifest;_GCTODI") + // - PRT(Build, "BGS;marker;GetNativeManifest;BC;BL;marker;GetNativeManifest;_GCTODI") + // The second item embeds the marker TWICE because its authoring source prepends to + // $(ProjectReferenceTargetsForBuild) after Common already populated the property with + // one marker. Net per Build entry: 3 markers => 3 Builds emitted per visit => 3^n + // growth without the dedup. + private const string DuplicateMarkerPRTContent = $""" + + + + + + + + + + + + + """; + + // Pre-PR-#13427 single-PRT shape: one PRT(Build) item with one marker. Common case. + private const string SingleMarkerPRTContent = $""" + + + + """; + } +} diff --git a/src/Build/Graph/ProjectGraph.cs b/src/Build/Graph/ProjectGraph.cs index d717694d641..2022bdf0d40 100644 --- a/src/Build/Graph/ProjectGraph.cs +++ b/src/Build/Graph/ProjectGraph.cs @@ -621,7 +621,7 @@ public IReadOnlyDictionary> GetTargetLis // If no targets were specified, use every project's default targets. foreach (ProjectGraphNode entryPointNode in EntryPointNodes) { - var entryTargets = ImmutableList.CreateRange(entryPointNode.ProjectInstance.DefaultTargets); + string[] entryTargets = entryPointNode.ProjectInstance.DefaultTargets.ToArray(); var entryEdge = new ProjectGraphBuildRequest(entryPointNode, entryTargets); encounteredEdges.Add(entryEdge); edgesToVisit.Enqueue(entryEdge); @@ -636,7 +636,7 @@ public IReadOnlyDictionary> GetTargetLis { foreach (ProjectGraphNode entryPointNode in EntryPointNodes) { - var entryTargets = ImmutableList.CreateRange(entryPointNode.ProjectInstance.DefaultTargets); + string[] entryTargets = entryPointNode.ProjectInstance.DefaultTargets.ToArray(); var entryEdge = new ProjectGraphBuildRequest(entryPointNode, entryTargets); encounteredEdges.Add(entryEdge); edgesToVisit.Enqueue(entryEdge); @@ -663,7 +663,7 @@ public IReadOnlyDictionary> GetTargetLis { // Build a specific project with its default targets. ProjectGraphNode node = GetNodeForProject(project); - ProjectGraphBuildRequest entryEdge = new(node, ImmutableList.CreateRange(node.ProjectInstance.DefaultTargets)); + ProjectGraphBuildRequest entryEdge = new(node, node.ProjectInstance.DefaultTargets.ToArray()); encounteredEdges.Add(entryEdge); edgesToVisit.Enqueue(entryEdge); isSolutionTraversalTarget = true; @@ -725,14 +725,14 @@ public IReadOnlyDictionary> GetTargetLis // Queue the project references for visitation, if the edge hasn't already been traversed. foreach (var referenceNode in node.ProjectReferences) { - var applicableTargets = targetsToPropagate.GetApplicableTargetsForReference(referenceNode); + string[] applicableTargets = targetsToPropagate.GetApplicableTargetsForReference(referenceNode); - if (applicableTargets.IsEmpty) + if (applicableTargets.Length == 0) { continue; } - var expandedTargets = ExpandDefaultTargets( + string[] expandedTargets = ExpandDefaultTargets( applicableTargets, referenceNode.ProjectInstance.DefaultTargets, Edges[(node, referenceNode)]); @@ -797,39 +797,143 @@ void ThrowOnEmptyTargetNames(ICollection targetNames) } } - private static ImmutableList ExpandDefaultTargets(ImmutableList targets, List defaultTargets, ProjectItemInstance graphEdge) + internal static string[] ExpandDefaultTargets(string[] targets, List defaultTargets, ProjectItemInstance graphEdge) { - var i = 0; - while (i < targets.Count) + // Dedup the BFS-edge target list. The N^depth graph-explosion behavior this + // function defends against has two sources: (1) marker expansion injecting a + // duplicate-bearing payload — including markers from a previously-deduped hop + // re-expanding against the next hop's PRT shape; and (2) literal duplicates in + // the input list propagating through FromProjectAndEntryTargets, where each + // duplicated entry target re-matches the same PRT items at the next hop and + // multiplies the propagated count. Bounding the result here breaks the chain + // at the source for both, so we dedup unconditionally rather than only on + // marker expansion. + // + // The outer post-BFS dedup in GetTargetLists still runs and collapses each + // per-node final list, so the publicly-observable result is unchanged. The + // benefit of dedup here is purely structural: smaller per-edge requestedTargets + // means fewer distinct ProjectGraphBuildRequest entries in encounteredEdges + // and far less BFS thrash. + // + // Fast path: for small inputs (the typical post-first-hop BFS shape — count + // 1-3 in real .NET builds) do an inline O(n^2) scan that detects markers and + // duplicates in place. If neither is present, return targets unchanged with + // zero allocation. Above the threshold we fall through to the HashSet-backed + // slow path; the O(n^2) cost would dominate the HashSet allocation otherwise. + int count = targets.Length; + if (count == 0) { - if (targets[i].Equals(MSBuildConstants.DefaultTargetsMarker, StringComparison.OrdinalIgnoreCase)) + return targets; + } + + const int InlineScanThreshold = 8; + if (count <= InlineScanThreshold) + { + for (int i = 0; i < count; i++) { - targets = targets - .RemoveAt(i) - .InsertRange(i, defaultTargets); - i += defaultTargets.Count; + string target = targets[i]; + if (target.Equals(MSBuildConstants.DefaultTargetsMarker, StringComparison.OrdinalIgnoreCase) || + target.Equals(MSBuildConstants.ProjectReferenceTargetsOrDefaultTargetsMarker, StringComparison.OrdinalIgnoreCase)) + { + return ExpandDefaultTargetsSlow(targets, defaultTargets, graphEdge); + } + + for (int j = 0; j < i; j++) + { + if (string.Equals(targets[j], target, StringComparison.OrdinalIgnoreCase)) + { + return ExpandDefaultTargetsSlow(targets, defaultTargets, graphEdge); + } + } } - else if (targets[i].Equals(MSBuildConstants.ProjectReferenceTargetsOrDefaultTargetsMarker, StringComparison.OrdinalIgnoreCase)) - { - var targetsString = graphEdge.GetMetadataValue(ItemMetadataNames.ProjectReferenceTargetsMetadataName); - var expandedTargets = string.IsNullOrEmpty(targetsString) - ? defaultTargets - : ExpressionShredder.SplitSemiColonSeparatedList(targetsString).ToList(); + return targets; + } + + return ExpandDefaultTargetsSlow(targets, defaultTargets, graphEdge); + } - targets = targets - .RemoveAt(i) - .InsertRange(i, expandedTargets); + private static string[] ExpandDefaultTargetsSlow(string[] targets, List defaultTargets, ProjectItemInstance graphEdge) + { + // Slow path. Reached when (a) input contains at least one marker or duplicate + // and was small enough for the fast path to detect it, or (b) input is larger + // than the inline-scan threshold and we go straight here. + // + // The lazy buffer pattern keeps allocations to the minimum necessary: if the + // input turns out to be marker-and-dup-free (only possible in the large-count + // case (b)) we never allocate the result List, only the HashSet. + int count = targets.Length; + var seen = new HashSet(count, StringComparer.OrdinalIgnoreCase); + List result = null; + + for (int i = 0; i < count; i++) + { + string target = targets[i]; - i += expandedTargets.Count; + if (target.Equals(MSBuildConstants.DefaultTargetsMarker, StringComparison.OrdinalIgnoreCase)) + { + EnsureBuffer(targets, i, ref result); + foreach (string defaultTarget in defaultTargets) + { + if (seen.Add(defaultTarget)) + { + result.Add(defaultTarget); + } + } + } + else if (target.Equals(MSBuildConstants.ProjectReferenceTargetsOrDefaultTargetsMarker, StringComparison.OrdinalIgnoreCase)) + { + EnsureBuffer(targets, i, ref result); + string targetsString = graphEdge.GetMetadataValue(ItemMetadataNames.ProjectReferenceTargetsMetadataName); + if (string.IsNullOrEmpty(targetsString)) + { + foreach (string defaultTarget in defaultTargets) + { + if (seen.Add(defaultTarget)) + { + result.Add(defaultTarget); + } + } + } + else + { + foreach (string expandedTarget in ExpressionShredder.SplitSemiColonSeparatedList(targetsString)) + { + if (seen.Add(expandedTarget)) + { + result.Add(expandedTarget); + } + } + } + } + else if (seen.Add(target)) + { + result?.Add(target); } else { - i++; + // First duplicate in plain-target territory. Materialize the verbatim + // unique prefix into the buffer; subsequent unique entries will be added + // through the buffer above. + EnsureBuffer(targets, i, ref result); } } - return targets; + return result?.ToArray() ?? targets; + + static void EnsureBuffer(string[] targets, int currentIndex, ref List result) + { + if (result is not null) + { + return; + } + + result = new List(targets.Length); + for (int k = 0; k < currentIndex; k++) + { + result.Add(targets[k]); + } + } } internal ProjectInstance DefaultProjectInstanceFactory( @@ -863,7 +967,7 @@ internal static ProjectInstance StaticProjectInstanceFactory( private struct ProjectGraphBuildRequest : IEquatable { - public ProjectGraphBuildRequest(ProjectGraphNode node, ImmutableList targets) + public ProjectGraphBuildRequest(ProjectGraphNode node, string[] targets) { Node = node ?? throw new ArgumentNullException(nameof(node)); RequestedTargets = targets ?? throw new ArgumentNullException(nameof(targets)); @@ -871,18 +975,18 @@ public ProjectGraphBuildRequest(ProjectGraphNode node, ImmutableList tar public ProjectGraphNode Node { get; } - public ImmutableList RequestedTargets { get; } + public string[] RequestedTargets { get; } public readonly bool Equals(ProjectGraphBuildRequest other) { if (Node != other.Node - || RequestedTargets.Count != other.RequestedTargets.Count) + || RequestedTargets.Length != other.RequestedTargets.Length) { return false; } // Target order is important - for (var i = 0; i < RequestedTargets.Count; i++) + for (int i = 0; i < RequestedTargets.Length; i++) { if (!RequestedTargets[i].Equals(other.RequestedTargets[i], StringComparison.OrdinalIgnoreCase)) { @@ -895,7 +999,7 @@ public readonly bool Equals(ProjectGraphBuildRequest other) public override readonly bool Equals(object obj) { - return !(obj is null) && obj is ProjectGraphBuildRequest graphNodeWithTargets && Equals(graphNodeWithTargets); + return obj is ProjectGraphBuildRequest graphNodeWithTargets && Equals(graphNodeWithTargets); } public override readonly int GetHashCode() @@ -903,8 +1007,8 @@ public override readonly int GetHashCode() unchecked { const int salt = 397; - var hashCode = Node.GetHashCode() * salt; - for (var i = 0; i < RequestedTargets.Count; i++) + int hashCode = Node.GetHashCode() * salt; + for (int i = 0; i < RequestedTargets.Length; i++) { hashCode *= salt; hashCode ^= StringComparer.OrdinalIgnoreCase.GetHashCode(RequestedTargets[i]); diff --git a/src/Build/Graph/ProjectInterpretation.cs b/src/Build/Graph/ProjectInterpretation.cs index 12ae373485d..66e7733f885 100644 --- a/src/Build/Graph/ProjectInterpretation.cs +++ b/src/Build/Graph/ProjectInterpretation.cs @@ -466,17 +466,37 @@ private static void RemoveFromPropertyDictionary( public readonly struct TargetsToPropagate { - private readonly ImmutableList _outerBuildTargets; - private readonly ImmutableList _allTargets; + // Outer build targets occupy the first _outerBuildTargetCount entries of _allTargets, + // followed by inner build (non-outer-build) targets. Non-multitargeting projects use + // the full array because they act as both outer and inner builds. + private readonly TargetSpecification[] _allTargets; + private readonly int _outerBuildTargetCount; - private TargetsToPropagate(ImmutableList outerBuildTargets, ImmutableList nonOuterBuildTargets) + private TargetsToPropagate(List outerBuildTargets, List nonOuterBuildTargets) { - _outerBuildTargets = outerBuildTargets; + int outerCount = outerBuildTargets?.Count ?? 0; + int innerCount = nonOuterBuildTargets?.Count ?? 0; + int total = outerCount + innerCount; - // This is used as the list of entry targets for both inner builds and non-multitargeting projects. - // It represents the concatenation of outer build targets and non outer build targets, in this order. - // Non-multitargeting projects use these targets because they act as both outer and inner builds. - _allTargets = outerBuildTargets.AddRange(nonOuterBuildTargets); + _outerBuildTargetCount = outerCount; + + if (total == 0) + { + _allTargets = []; + return; + } + + // Single backing array; each source list is copied exactly once. + TargetSpecification[] combined = new TargetSpecification[total]; + if (outerCount > 0) + { + outerBuildTargets.CopyTo(combined, 0); + } + if (innerCount > 0) + { + nonOuterBuildTargets.CopyTo(combined, outerCount); + } + _allTargets = combined; } /// @@ -489,10 +509,10 @@ private TargetsToPropagate(ImmutableList outerBuildTargets, /// Project containing the PRT protocol /// Targets with which will get called /// - public static TargetsToPropagate FromProjectAndEntryTargets(ProjectInstance project, ImmutableList entryTargets) + public static TargetsToPropagate FromProjectAndEntryTargets(ProjectInstance project, string[] entryTargets) { - ImmutableList.Builder targetsForOuterBuild = ImmutableList.CreateBuilder(); - ImmutableList.Builder targetsForInnerBuild = ImmutableList.CreateBuilder(); + List targetsForOuterBuild = null; + List targetsForInnerBuild = null; ICollection projectReferenceTargets = project.GetItems(ItemTypeNames.ProjectReferenceTargets); @@ -505,41 +525,66 @@ public static TargetsToPropagate FromProjectAndEntryTargets(ProjectInstance proj string targetsMetadataValue = projectReferenceTarget.GetMetadataValue(ItemMetadataNames.ProjectReferenceTargetsMetadataName); bool skipNonexistentTargets = MSBuildStringIsTrue(projectReferenceTarget.GetMetadataValue("SkipNonexistentTargets")); bool targetsAreForOuterBuild = MSBuildStringIsTrue(projectReferenceTarget.GetMetadataValue(ProjectReferenceTargetIsOuterBuildMetadataName)); - TargetSpecification[] targets = ExpressionShredder.SplitSemiColonSeparatedList(targetsMetadataValue) - .Select(t => new TargetSpecification(t, skipNonexistentTargets)).ToArray(); - if (targetsAreForOuterBuild) - { - targetsForOuterBuild.AddRange(targets); - } - else + + // Append directly into the destination list. SemiColonTokenizer is a + // struct enumerator, so foreach avoids boxing it through IEnumerable. + // This skips the LINQ Select state machine, its captured-locals closure, + // and an intermediate TargetSpecification[] vs the previous + // .Select(...).ToArray() + AddRange pattern. + ref List dest = ref targetsAreForOuterBuild ? ref targetsForOuterBuild : ref targetsForInnerBuild; + dest ??= []; + foreach (string target in ExpressionShredder.SplitSemiColonSeparatedList(targetsMetadataValue)) { - targetsForInnerBuild.AddRange(targets); + dest.Add(new TargetSpecification(target, skipNonexistentTargets)); } } } } - return new TargetsToPropagate(targetsForOuterBuild.ToImmutable(), targetsForInnerBuild.ToImmutable()); + return new TargetsToPropagate(targetsForOuterBuild, targetsForInnerBuild); } - public ImmutableList GetApplicableTargetsForReference(ProjectGraphNode projectGraphNode) + public string[] GetApplicableTargetsForReference(ProjectGraphNode projectGraphNode) { - ImmutableList RemoveNonexistentTargetsIfSkippable(ImmutableList targets) + int end = projectGraphNode.ProjectType switch { - // Keep targets that are non-skippable or that exist but are skippable. - return targets - .Where(t => !t.SkipIfNonexistent || projectGraphNode.ProjectInstance.Targets.ContainsKey(t.Target)) - .Select(t => t.Target) - .ToImmutableList(); + ProjectType.OuterBuild => _outerBuildTargetCount, + ProjectType.InnerBuild => _allTargets.Length, + ProjectType.NonMultitargeting => _allTargets.Length, + _ => throw new ArgumentOutOfRangeException(), + }; + + if (end == 0) + { + return []; } - return projectGraphNode.ProjectType switch + // Keep targets that are non-skippable or that exist but are skippable. The common + // case is "every target passes the filter", so pre-size a string[] and trim only + // if something was actually skipped. + string[] result = new string[end]; + int writeIndex = 0; + for (int i = 0; i < end; i++) { - ProjectType.InnerBuild => RemoveNonexistentTargetsIfSkippable(_allTargets), - ProjectType.OuterBuild => RemoveNonexistentTargetsIfSkippable(_outerBuildTargets), - ProjectType.NonMultitargeting => RemoveNonexistentTargetsIfSkippable(_allTargets), - _ => throw new ArgumentOutOfRangeException(), - }; + TargetSpecification t = _allTargets[i]; + if (!t.SkipIfNonexistent || projectGraphNode.ProjectInstance.Targets.ContainsKey(t.Target)) + { + result[writeIndex++] = t.Target; + } + } + + if (writeIndex == end) + { + return result; + } + + if (writeIndex == 0) + { + return []; + } + + Array.Resize(ref result, writeIndex); + return result; } } From 58f0b654ca2b4392bdc6537fd9e9183573313a50 Mon Sep 17 00:00:00 2001 From: David Federman Date: Tue, 26 May 2026 11:47:54 -0700 Subject: [PATCH 2/2] Address PR #13855 review feedback - Wrap unit tests in TestEnvironment.Create(_output) to match the in-file end-to-end test pattern and the sibling ProjectGraph_Tests convention, ensuring evaluation state from in-memory Project instances doesn't leak across tests via the global ProjectCollection (Copilot bot suggestion). - Defer the empty-list allocation in FromProjectAndEntryTargets until the first target is actually appended, avoiding an empty List allocation when a matched PRT item has empty Targets metadata (Copilot bot suggestion). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- ...ectGraph_ExpandDefaultTargetsDedup_Tests.cs | 18 ++++++++++++++++++ src/Build/Graph/ProjectInterpretation.cs | 2 +- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/Build.UnitTests/Graph/ProjectGraph_ExpandDefaultTargetsDedup_Tests.cs b/src/Build.UnitTests/Graph/ProjectGraph_ExpandDefaultTargetsDedup_Tests.cs index 044bd829a32..885497dbc60 100644 --- a/src/Build.UnitTests/Graph/ProjectGraph_ExpandDefaultTargetsDedup_Tests.cs +++ b/src/Build.UnitTests/Graph/ProjectGraph_ExpandDefaultTargetsDedup_Tests.cs @@ -52,6 +52,8 @@ public ProjectGraph_ExpandDefaultTargetsDedup_Tests(ITestOutputHelper output) [Fact] public void Dedupes_WhenMarkerExpansionProducesDuplicates() { + using TestEnvironment env = TestEnvironment.Create(_output); + ProjectItemInstance edge = CreateEdge(); string[] input = ["Build", MSBuildConstants.DefaultTargetsMarker]; List defaultTargets = ["Build", "X"]; @@ -69,6 +71,8 @@ public void Dedupes_WhenMarkerExpansionProducesDuplicates() [Fact] public void Dedupes_PRTOrDefaultMarker_WhenTargetsMetadataDuplicatesExpansion() { + using TestEnvironment env = TestEnvironment.Create(_output); + ProjectItemInstance edge = CreateEdgeWithTargetsMetadata("A;B;A"); string[] input = [ MSBuildConstants.ProjectReferenceTargetsOrDefaultTargetsMarker, @@ -91,6 +95,8 @@ public void Dedupes_PRTOrDefaultMarker_WhenTargetsMetadataDuplicatesExpansion() [Fact] public void Dedupes_ExplicitNonMarkerDuplicates() { + using TestEnvironment env = TestEnvironment.Create(_output); + ProjectItemInstance edge = CreateEdge(); string[] input = ["Build", "Build", "Build"]; List defaultTargets = ["DefaultsShouldNotBeUsed"]; @@ -108,6 +114,8 @@ public void Dedupes_ExplicitNonMarkerDuplicates() [Fact] public void Dedupes_IgnoresCase() { + using TestEnvironment env = TestEnvironment.Create(_output); + ProjectItemInstance edge = CreateEdge(); string[] input = ["Build", MSBuildConstants.DefaultTargetsMarker]; List defaultTargets = ["BUILD", "x"]; @@ -124,6 +132,8 @@ public void Dedupes_IgnoresCase() [Fact] public void NoMarkerNoDuplicates_ReturnsSameInstance() { + using TestEnvironment env = TestEnvironment.Create(_output); + ProjectItemInstance edge = CreateEdge(); string[] input = ["Build", "X", "Y"]; List defaultTargets = ["ShouldNotBeUsed"]; @@ -140,6 +150,8 @@ public void NoMarkerNoDuplicates_ReturnsSameInstance() [Fact] public void MarkerExpanded_NoDuplicates_ReturnsExpandedList() { + using TestEnvironment env = TestEnvironment.Create(_output); + ProjectItemInstance edge = CreateEdge(); string[] input = [MSBuildConstants.DefaultTargetsMarker]; List defaultTargets = ["A", "B"]; @@ -156,6 +168,8 @@ public void MarkerExpanded_NoDuplicates_ReturnsExpandedList() [Fact] public void MarkerExpandsToEmptyDefaults_ReturnsEmptyList() { + using TestEnvironment env = TestEnvironment.Create(_output); + ProjectItemInstance edge = CreateEdge(); string[] input = [MSBuildConstants.DefaultTargetsMarker]; List defaultTargets = []; @@ -172,6 +186,8 @@ public void MarkerExpandsToEmptyDefaults_ReturnsEmptyList() [Fact] public void AllEntriesCollapseToSingleton() { + using TestEnvironment env = TestEnvironment.Create(_output); + ProjectItemInstance edge = CreateEdge(); string[] input = ["Build", MSBuildConstants.DefaultTargetsMarker, MSBuildConstants.DefaultTargetsMarker]; List defaultTargets = ["Build"]; @@ -188,6 +204,8 @@ public void AllEntriesCollapseToSingleton() [Fact] public void PreservesFirstOccurrenceOrder() { + using TestEnvironment env = TestEnvironment.Create(_output); + ProjectItemInstance edge = CreateEdge(); string[] input = ["First", MSBuildConstants.DefaultTargetsMarker, "Last"]; List defaultTargets = ["Middle", "First"]; diff --git a/src/Build/Graph/ProjectInterpretation.cs b/src/Build/Graph/ProjectInterpretation.cs index 66e7733f885..b993b48990a 100644 --- a/src/Build/Graph/ProjectInterpretation.cs +++ b/src/Build/Graph/ProjectInterpretation.cs @@ -532,9 +532,9 @@ public static TargetsToPropagate FromProjectAndEntryTargets(ProjectInstance proj // and an intermediate TargetSpecification[] vs the previous // .Select(...).ToArray() + AddRange pattern. ref List dest = ref targetsAreForOuterBuild ? ref targetsForOuterBuild : ref targetsForInnerBuild; - dest ??= []; foreach (string target in ExpressionShredder.SplitSemiColonSeparatedList(targetsMetadataValue)) { + dest ??= []; dest.Add(new TargetSpecification(target, skipNonexistentTargets)); } }