Avoid unnecessary red node allocations in ReduceOneNodeOrTokenAsync by replacing .Single() with .First()

[nareshjo]

This pull request was generated by the VS Perf Rel AI Agent. Please review this AI-generated PR with extra care! For more information, visit our wiki. Please share feedback with TIP Insights

• Issue: AbstractSimplificationService.ReduceOneNodeOrTokenAsync uses .Single() on ChildNodesAndTokens() which forces enumeration of ALL children of a parent node to verify uniqueness, creating red syntax node wrappers (ExpressionStatementSyntax) for every child via GetRedElement → CreateRed, even after the matching annotated child has already been found.

  Evidence:

1. Stack trace shows allocation type TypeAllocated!Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionStatementSyntax (red node wrapper)
2. Source code at line 224 shows: .ChildNodesAndTokens().Single(c => c.HasAnnotation(annotation)) which enumerates all remaining children after the match to verify uniqueness
3. The annotation is a freshly created new SyntaxAnnotation() which is guaranteed unique by construction (_id assigned via Interlocked.Increment), making the uniqueness check redundant
4. This code is in a triply-nested hot path: Parallel.ForEachAsync × foreach reducers × do-while HasMoreWork

TypeAllocated!Microsoft.CodeAnalysis.CSharp.Syntax.ExpressionStatementSyntax
clr.dll!JIT_New
microsoft.codeanalysis.csharp.dll!ExpressionStatementSyntax.CreateRed
microsoft.codeanalysis.dll!SyntaxNode.GetRedElement
microsoft.codeanalysis.dll!ChildSyntaxList.ItemInternal
microsoft.codeanalysis.dll!ChildSyntaxList+EnumeratorImpl.get_Current
microsoft.codeanalysis.workspaces.dll!System.Linq.Enumerable.Single[SyntaxNodeOrToken]
microsoft.codeanalysis.workspaces.dll!AbstractSimplificationService.ReduceOneNodeOrTokenAsync

• Issue type: AVOID unnecessary full enumeration when searching for a guaranteed-unique element on hot path

• Proposed fix: Change .Single() to .First() on line 224 to stop enumeration at the first match instead of continuing through all remaining children.
  The annotation is guaranteed unique by construction (new SyntaxAnnotation() assigns a unique _id via Interlocked.Increment), so .First() returns the same element as .Single().
  This is a minimal and safe fix that eliminates red node allocations for all children after the match while maintaining identical behavior.

Best practices wiki
See related failure in PRISM
ADO work item

Microsoft Reviewers: Open in CodeFlow

[ToddGrun]

[ToddGrun]

Makes sense to me, but someone from Roslyn should take a look. @jasonmalinowski

[CyrusNajmabadi]

We should have a debug assert that this only has one item in it. It's fine then to switch the code to be First.

That said. isntead of explicitly getting the children and finding the one with the annotation, can we not just directly ask for the annotated node/token? I thought there was a helper on nodes for that. That seems better as it can avoid the greens entirely, not creating the red versions for the children before the match.

[CyrusNajmabadi]

The code should change to: replacedParent.GetAnnotatedNodesAndTokens(annotation).Single()

that said, i looked at GetAnnotatedNodesAndTokens and it could be more efficient. Right now it is:

return this.DescendantNodesAndTokensAndSelf(n => n.ContainsAnnotations, descendIntoTrivia: true)
                .Where(t => t.HasAnnotation(annotation));

this will realize the red children of a node unnecessarily so that it can call Func<SyntaxNode, bool>? descendIntoChildren on them. we should have an internal version of this helper that checks the green node first and sees if it can skip that. @ToddGrun, something you might want to look into.

@nareshjo what perf hit do you see here, and hwat perf benefit did you observe from this change?

[ToddGrun]

The code should change to: replacedParent.GetAnnotatedNodesAndTokens(annotation).Single()

It feels like we should have a replace method that returns the new node in the tree, and not require the caller to do this annotation dance.

@nareshjo what perf hit do you see here, and hwat perf benefit did you observe from this change?

This would be good to know. For example, there is another Single call a couple lines below the one changed that looks to be walking the full set of descendants from the document root, which I would this is way worse than just realizing a single node's children.

[nareshjo]

@CyrusNajmabadi, @ToddGrun, thanks for reviewing. This is 18th highest allocation contributor for latest VS releases with allocation rates of 305 MB/s at 50th percentile and 496 at 90th. It also shows up in list of high CPU usage. Allocation/CPU issues affect the responsiveness of VS in general.

Just to give a bit of context, this PR came from our internal Perf AI agent which tries to address issues identified in latest VS releases based on perf telemetry. I submitted it just because of some gaps in that system ATM regarding submissions to GitHub repos.
It is instructed to focus on the exact hot paths from the real world traces and try to make safe, minimal fixes that reduce the reported allocation without taking on much behavioural risk, instead of making broader structural changes unless it's very confident it understands the surrounding code and impact.
That's what it did in this case but it's possible there might be more optimisation opportunities here, like the ones you mentioned, but I think it was trying to balance potential benefit against the risk of making a broader change.

Since this is currently a fairly high-ranking allocation and CPU issue as per real-world data, the idea here is to land the low-risk improvement first and leave any broader follow-up optimisations open if reviewers think they’re worth pursuing.

I ran some benchmarks with original code and proposed fix with the help of profiler copilot and BenchmarkDotnet (something we are trying to incorporate and include in the PR descriptions from agent upfront to help reviewers better understand impact of change).
It does show significant improvements in both allocations and performance by changing Single to First (more when a match is found near start/middle, as expected). Here are results:

Allocation reduction

• Count = number of relevant direct sibling children under replacedParent being scanned
• Position = location of the uniquely annotated child within that direct-child sequence

Count	Position	Single Alloc	First Alloc	Reduction
10	First	2 KB	1 KB	50% less
10	Middle	2 KB	1 KB	50% less
10	Last	2 KB	2 KB	same
50	First	4 KB	2 KB	50% less
50	Middle	4 KB	3 KB	25% less
50	Last	4 KB	4 KB	same
200	First	12 KB	5 KB	58% less
200	Middle	12 KB	9 KB	25% less
200	Last	12 KB	12 KB	same

Perf speedup

Count	Position	Single	First	Speedup
200	First	26.2 μs	16.7 μs	1.57× faster
200	Middle	28.3 μs	23.7 μs	1.20× faster
50	First	8.2 μs	5.8 μs	1.42× faster

One option could be to take a simpler yet effective optimisation like this with any other minor tweaks as you see appropriate and if this issue still shows up as high allocation/CPU issue in future releases then try to do more extensive optimisations.

[nareshjo]

The code should change to: replacedParent.GetAnnotatedNodesAndTokens(annotation).Single()

It feels like we should have a replace method that returns the new node in the tree, and not require the caller to do this annotation dance.

@nareshjo what perf hit do you see here, and hwat perf benefit did you observe from this change?

This would be good to know. For example, there is another Single call a couple lines below the one changed that looks to be walking the full set of descendants from the document root, which I would this is way worse than just realizing a single node's children.

@ToddGrun
The allocation trace shows System.Linq.Enumerable.Single[Microsoft.CodeAnalysis.SyntaxNodeOrToken] which matches the first .Single() call that this fix proposes to change to .First(). That would explain why second one was not considered in scope for fix.
I did a quick search and couldn't find any recent failures which match that second .Single() call. So I guess maybe that path doesn't get used as much and hence optimising it although could be good, may not have as much real world impact.

[nareshjo]

We should have a debug assert that this only has one item in it. It's fine then to switch the code to be First.

Would someone on Roslyn be able to help check that? I am not familiar with process to set up a test environment and do that validation. Or I can give it a try if you can point me to any docs which cover that.
Or were you suggesting to leverage some existing (or new unit tests) in debug mode to check that?