Reduce allocations from enumerator allocations in AnalyzerExecutor.ExecuteSyntaxNodeActions #75940

ToddGrun · 2024-11-15T22:24:34Z

The scrolling speedometer test shows about 1% of CodeAnalysis process allocations due to allocating enumerators in AnalyzerExecutor.ExecuteSyntaxNodeAction. This code currently uses both ArrayBuilder and ImmutableArray, casting down to the common IEnumerable for certain operations. Enumerating then causes an allocation due to not being able to use the struct based enumerator using strongly typed classes allows.

Instead, I've changed the common method to take in a ArrayBuilder and to enumerate on that. This required some callers to copy into a new ArrayBuilder instance, but at least it avoids the enumerator allocations.

Created an insertion PR to verify this improves things, as there is the potential for this approach to actually make things worse if the ArrayBuilder being copied into doesn't go back into the pool due to exceeding the ArrayPool size threshold.

https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_build/results?buildId=10583050&view=results

…n usage The scrolling speedometer test shows about 1% of CodeAnalysis process allocations due to allocating enumerators in AnalyzerExecutor.ExecuteSyntaxNodeAction. This code currently uses both ArrayBuilder and ImmutableArray, casting down to the common IEnumerable for certain operations. Enumerating then causes an allocation due to not being able to use the struct based enumerator using strongly typed classes allows. Instead, I've changed the common method to take in a ReadOnlySpan and to enumerate on that. However, that requires the callers to be able to generate a ReadOnlySpan. ImmutableArray can be done via ImmutableCollectionsMarshal, but ArrayBuilder presents a problem as there is no way (that I know of) to get to the underlying ImmutableArray.Builder's array. To work around this, I've pulled out a minimal subset of code from ArrayBuilder and stripped it's underlying ImmutableArray.Builder and had it hold the array and size directly.

src/Dependencies/PooledObjects/SpannableArrayBuilder.cs

ToddGrun · 2024-11-15T23:22:03Z

Commit 2 test insertion PR:

https://devdiv.visualstudio.com/DevDiv/_build/results?buildId=10568083&view=results

src/Dependencies/PooledObjects/SpannableArrayBuilder.cs

…numerate

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

src/Dependencies/PooledObjects/SpannableArrayBuilder.cs

…zerExecutor

AlekseyTs · 2024-11-16T02:27:19Z

src/Dependencies/PooledObjects/SpannableArrayBuilder.cs

+    }
+
+    public ReadOnlySpan<T> AsSpan()
+        => new(_items, 0, Count);


=> new(_items, 0, Count);

I am not sure whether we would want to have a collection like that.

The span might no longer reflect the content of the builder after EnsureCapacity is called.

Same issue with the ref returned by the indexer.

When T is a reference type, performance characteristics for indexing could be worse than for array builder due to a type check performed by the ref _items[index] operation.

We might want to look for an alternative solution to the problem that we are trying to address.
#Closed

For example, picking either ArrayBuilder or ImmutableArray for the input.

This isn't the first time I've wished we had a way to convert an ArrayBuilder to a span to pass to an API. Currently, there is no way to do that without copying the data into another array.

If this change is considered extremely objectionable, I could limit the scope of this change to AnalyzerExecutor.ExecuteSyntaxNodeActions and AnalyzerExecutor.ExecuteOperationActions such that both take in ArrayBuilders and modify the ImmutableArray callers to copy their data into an ArrayBuilder before invoking.

However, I thought this approach was better as it not only fixed these callers to not require copying data around to avoid the allocations, but also allowed us to generally solve the problem of being able to call span based APIs from callers who need ArrayBuilder like behaviors.

I view the AsSpan as similar to what ImmutableCollectionsMarshal.AsArray and CollectionsMarshal.AsArray provide. Perhaps it would be better to not put the AsSpan directly on the class, but rather in a Marshal like class to indicate that extra care need be taken while the span is in use?

Went ahead and pushed some changes over in commit 6 to use a marshal class mechanism similar to what List/ImmutableArray and our Segmented collections do. I don't think SpannableArrayBuilder remains a good name for the collection class, but I couldn't think of a more applicable name right now.

If all we want is a "special" type name. There is an option of deriving from List<T> and using the derived type instead of the List<T> directly. Basically, I do not think we should be creating yet another list implementation when we already have a standard collection type that fits our needs.

I mean fits in terms of behavior.

I'd like to better understand what the proposed change Aleksey is suggesting. Is it:

Change the four AnalyzerExecutor.Execute*Actions to take in a List rather than the current IEnumerable (or ROS that the PR has currently)

Change AnalyzerDriver.DeclarationAnalysisData to declare an objectpool and to use that to get DescendantNodesToAnalyze

Change invocations of AnalyzerExecutor.Execute*Actions to pass in a List. This will require some callers to copy into a temporary list as they have an ImmutableArray (see AnalyzerDriver.ExecuteDeclaringReferenceActions.executeOperationsActionsByKind)

I can try changing to this if there is a strong aversion to adding a new collection type (which appears to be the case). I don't love that those four methods would now take in a mutable type, or that we would need to do the copy from the ImmutableArray into a list, but it's all doable. Again, I liked the approach currently in this PR as I see a need for us to be able to get an array or ROS from a List or ArrayBuilder like object and we currently can't do that (with the exception that net core can do that for List)

I'd like to better understand what the proposed change Aleksey is suggesting. Is it:

If we really want to pass a span around and avoid copying data, I suggest using List<T> instead of introducing a custom collection implementation.

There are also alternatives to consider, like either passing ImmutableArrays, or or passing ArrayBuilders, or creating a copy to get a span.

I would be fine with either approach as long as it addresses the problem we are trying to address, doesn't introduce an inacceptable perf downside elsewhere, and doesn't add a custom List<T> implementation.

Still testing, but I went ahead and pushed out changes that remove the added collection type. As mentioned, there is some extra data being copied around, but it should avoid the allocations this PR concerns itself with.

I still do like the type that was introduced earlier in this PR and do see a value that it provides, but I'm sensitive that the compiler team might not want to add another data type used in their area.

src/Dependencies/PooledObjects/SpannableArrayBuilder.cs

…ImmutableArray/Segmented* collections

…thods. By doing that and a bit of copying data around, we don't need the new ArrayBuilder class in this PR.

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

jaredpar · 2024-11-19T17:54:40Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

                }
                else if (operationActions != null)
                {
+                    var operationsToAnalyzeBuilder = ArrayBuilder<IOperation>.GetInstance(nodesToAnalyzeBuilder.Count);
+                    foreach (var syntaxNode in nodesToAnalyzeBuilder)
+                        operationsToAnalyzeBuilder.Add((syntaxNode as IOperation)!);


Why (syntax as IOperation)! and not (IOperation)syntaxNode? If this does not actually implement IOperation shouldn't we find out here vs. null refing later in code?

jaredpar · 2024-11-19T17:55:53Z

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs

                if (syntaxNodeActions != null)
                {
                    Debug.Assert(getKind != null);

+                    var syntaxNodesToAnalyzeBuilder = ArrayBuilder<SyntaxNode>.GetInstance(nodesToAnalyzeBuilder.Count);
+                    foreach (var syntaxNode in nodesToAnalyzeBuilder)
+                        syntaxNodesToAnalyzeBuilder.Add((syntaxNode as SyntaxNode)!);


Why (syntax as SyntaxNode)! and not (SyntaxNode)syntaxNode? If this is not a SyntaxNode shouldn't we find out here vs. null refing later?

Direct cast to SyntaxNode doesn't compile. I will go ahead and change it to cast to object and then to SyntaxNode assuming that is preferable.

Btw, it's not completely obvious but the calling code either puts in IOperation or SyntaxNodes, and doesn't intermingle.

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs

AlekseyTs · 2024-11-19T19:02:47Z

It looks like the description of the change no longer matches the change.

ToddGrun · 2024-11-21T16:47:07Z

It looks like the description of the change no longer matches the change.

Modified to match current changes

AlekseyTs

LGTM (commit 13)

ToddGrun requested a review from a team as a code owner November 15, 2024 22:24

dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Nov 15, 2024

CyrusNajmabadi reviewed Nov 15, 2024

View reviewed changes