Fix unnecessary casts of generic types to specific types in analyzer driver #76917

CyrusNajmabadi · 2025-01-24T19:56:31Z

Found when investigating traces involving lots of analyzers in large projects:

Related pr here: #76894

Todd is fixing up heavy CPU on the dictionary lookups. But we're also seeing enough significance in the use of IEnumerable everywhere here. The main reason the code was written to use IEnuemrable was because some codepths used ImmutableArrays and some ArrayBuilders. I've since separated these all out to consistently use one or the other (immutable when we copute and cache the data, and AB when we build up scratch results we want to process). This is very valuable as it means we're not boxing immutable arrays, nor are we getting boxed iterators as we keep walking the syntax and iop trees.

Doing this revealed some GNARLY code in the analyze driver for processing both SyntaxNode and IOps in a similar fashion. I"ve refactored that code heavily to be entirely typesafe and to not require the core code to have runtime type checks of the generic values within it to customize behavior.

In the scrolling speedometer test, the Roslyn CodeAnalysis process shows about 25% of CPU spent in this method. Of that, a surprising amount of it (11.2%) is spent in ImmutableSegmentedDictionary.TryGetValue. Debugging through this code, it appears this is because that is called O(m x n) times where m is the number of nodes to analyze and n is the number of items in groupedActions.GroupedActionsByAnalyzer. Instead, add a hook into GroupedAnalyzerActions to allow a mapping of kind -> analyzers. This can be used by executeNodeActionsByKind to get a much quicker way to determine whether the analyzer can contribute for the node in question. Only publishing this early so Cyrus can take a peek, as I still need to do a bit of debugging around these changes. Once Cyrus and I think the changes have merit, I will create a test insertion and publish the speedometer results once those are available. Only if all that goes well will I promote this PR out of draft mode.

…values.

…taxNodeActions

CyrusNajmabadi · 2025-01-24T19:58:01Z

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

@@ -777,10 +859,6 @@ private void ExecuteBlockActionsCore<TBlockStartAction, TBlockAction, TNodeActio

            var blockEndActions = PooledHashSet<TBlockAction>.GetInstance();
            var blockActions = PooledHashSet<TBlockAction>.GetInstance();
-            var executableNodeActions = ArrayBuilder<TNodeAction>.GetInstance();
-            var syntaxNodeActions = executableNodeActions as ArrayBuilder<SyntaxNodeAnalyzerAction<TLanguageKindEnum>>;
-            var operationActions = executableNodeActions as ArrayBuilder<OperationAnalyzerAction>;


these two lines were super wacky. we'd make the bulder, then decide if we were the codepath that was running for syntax ndoes or for operations. and hten special case behavior lower down.

CyrusNajmabadi · 2025-01-24T19:58:46Z

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

-            var executableNodeActions = ArrayBuilder<TNodeAction>.GetInstance();
-            var syntaxNodeActions = executableNodeActions as ArrayBuilder<SyntaxNodeAnalyzerAction<TLanguageKindEnum>>;
-            var operationActions = executableNodeActions as ArrayBuilder<OperationAnalyzerAction>;
-            ImmutableArray<IOperation> operationBlocks = executableBlocks[0] is IOperation ? (ImmutableArray<IOperation>)(object)executableBlocks : ImmutableArray<IOperation>.Empty;


similarly, if this was hte syntax node case, we'd to runtime checks to make this an empty array, or else we'd do runtime casts to convert an argument to the right type. now the callers just pass the right array along.

CyrusNajmabadi · 2025-01-24T19:59:25Z

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

@@ -792,75 +870,13 @@ private void ExecuteBlockActionsCore<TBlockStartAction, TBlockAction, TNodeActio

            // Include the stateful actions.
            foreach (var startAction in startActions)
-            {
-                if (startAction is CodeBlockStartAnalyzerAction<TLanguageKindEnum> codeBlockStartAction)


we'd do runtime checks to then have syntaxnode-vs-operation specific behavior. now, we just pass the data we need to the 'addActions' function, and each caller does the specific logic they want in a properly strongly typed fashion.

CyrusNajmabadi · 2025-01-24T20:00:18Z

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


            using var _ = PooledDelegates.GetPooledFunction((d, ct, arg) => arg.self.IsSupportedDiagnostic(arg.analyzer, d, ct), (self: this, analyzer), out Func<Diagnostic, CancellationToken, bool> isSupportedDiagnostic);

            // Execute stateful executable node analyzers, if any.
            if (executableNodeActions.Any())
-            {
-                if (syntaxNodeActions != null)


same case, we'd specialize what we wanted to do based on what data we were passed.

this was the code that motived the pr int eh first place and move us to spans. i want to be able to get us off of IEnuemrable, but code like (IEnumerable<SyntaxNode>)getNodesToAnalyze(executableBlocks); just abuses generics so much this isn't possible.

CyrusNajmabadi · 2025-01-24T20:00:49Z

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

-                declaredNode, declaredSymbol, executableCodeBlocks, (codeBlocks) => codeBlocks.SelectMany(
-                    cb =>
+            Debug.Assert(!executableCodeBlocks.IsEmpty);
+            var executableNodeActions = ArrayBuilder<SyntaxNodeAnalyzerAction<TLanguageKindEnum>>.GetInstance();


moved this up here so that we can infer the type arguments to ExecuteBlockActionsCore from just the args passed in.

CyrusNajmabadi · 2025-01-24T20:01:28Z

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

+                executableNodeActions,
+                addActions: static (startAction, codeBlockEndActions, syntaxNodeActions, args, cancellationToken) =>
+                {
+                    var (@this, codeBlockStartActions, analyzer, declaredNode, declaredSymbol, executableCodeBlocks, semanticModel, getKind, filterSpan, isGeneratedCode) = args;


pattern here is that all passed in lambdas are static (so we don't continually allocate for them), but they are passed an 'args' tuple containing any data they need. this args tuple is then unpackaged as the first step exactly how it is packaged up below.

CyrusNajmabadi · 2025-01-24T20:01:58Z

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

+
+                    var codeBlockScope = new HostCodeBlockStartAnalysisScope<TLanguageKindEnum>(startAction.Analyzer);
+                    var blockStartContext = new AnalyzerCodeBlockStartAnalysisContext<TLanguageKindEnum>(
+                        codeBlockScope, declaredNode, declaredSymbol, semanticModel, @this.AnalyzerOptions, filterSpan, isGeneratedCode, cancellationToken);


all this code was hte same code we had below in ExecuteBlockActionsCore. but we don't need runtime checks to determine to execute it. we know we want to execute exactly this code because we're teh syntax-node case.

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

ToddGrun · 2025-01-24T21:35:45Z

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

-                    }
-                }
-            }
+                addActions(startAction, blockEndActions, executableNodeActions, args, cancellationToken);


s

nit: plural seems weird here

the lambdas end up doing things like:

data.blockEndActions?.AddAll(data.scope.CodeBlockEndActions); data.syntaxNodeActions?.AddRange(data.scope.SyntaxNodeActions);

So plural seemed sensible to me. I don't feel hugely strongly.

CyrusNajmabadi · 2025-01-24T21:37:48Z

@dotnet/roslyn-compiler ptal. This is also a precursor step to some more work todd and i are doing to take some of these core hot hot hot loops and reduce costs/allocaitons there. This means updating them to not use IEnumerables as those both box ImmutableArrays, and they have expensive costs for enumeration. These are things we've seen in traces involving large files with lots of analyzers. Thanks!

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

…slyn into driverGenerics

CyrusNajmabadi · 2025-01-27T22:03:19Z

...mpilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.GroupedAnalyzerActionsForAnalyzer.cs

-                        var analyzerActionsByKind = !nodeActions.IsEmpty ?
-                            AnalyzerExecutor.GetNodeActionsByKind(nodeActions) :
-                            ImmutableSegmentedDictionary<TLanguageKindEnum, ImmutableArray<SyntaxNodeAnalyzerAction<TLanguageKindEnum>>>.Empty;
+                        var analyzerActionsByKind = AnalyzerExecutor.GetNodeActionsByKind(nodeActions);


swithcing to a temp array builder allows us to only accept ArrayBuilders in GetNodeActionsByKind instead of needing to take an IEnumerable ot handle ArrayBuilders or ImmutableArrays. This also avoids intermediary copies of data.

CyrusNajmabadi · 2025-01-27T22:04:19Z

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

+            Debug.Assert(!executableCodeBlocks.IsEmpty);
+
+            // The actions we discover in 'addActions' and then execute in 'executeActions'.
+            var ephemeralActions = ArrayBuilder<SyntaxNodeAnalyzerAction<TLanguageKindEnum>>.GetInstance();


this was pulled out of the callee as it prevented being able to do inference on the type arguments passed to it.

CyrusNajmabadi · 2025-01-27T22:05:38Z

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

-                        data => data.action(data.context),
-                        (action: codeBlockAction.Action, context: context),
+                        static data => data.action(data.context),
+                        argument: (action: codeBlockAction.Action, context),


made lambdas explicitly static to make it clear they must not capture naything (since that would defeat the purpose). my followup pr applies this to all the lambdas in this file.

CyrusNajmabadi · 2025-01-27T22:06:02Z

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

        {
-            return _syntaxNodeActions.OfType<SyntaxNodeAnalyzerAction<TLanguageKindEnum>>().ToImmutableArray();


linq all over the place.

CyrusNajmabadi · 2025-01-27T22:06:15Z

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

@@ -836,8 +842,6 @@ internal readonly ImmutableArray<SyntaxNodeAnalyzerAction<TLanguageKindEnum>> Ge
                    builder.Add(syntaxNodeAction);
                }
            }
-
-            return builder.ToImmutableAndFree();


copies all over the place. we realy want to avoid that while doing our processing.

CyrusNajmabadi · 2025-01-27T22:07:24Z

@RikkiGibson @jcouv this is ready for eyes.

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

CyrusNajmabadi · 2025-01-27T22:10:29Z

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

+                    var (@this, startActions, analyzer, declaredNode, operationBlocks, declaredSymbol, operations, semanticModel, filterSpan, isGeneratedCode, ephemeralActions) = args;
+                    var scope = new HostOperationBlockStartAnalysisScope(startAction.Analyzer);
+                    var startContext = new AnalyzerOperationBlockStartAnalysisContext(
+                        scope, operationBlocks, declaredSymbol, semanticModel.Compilation, @this.AnalyzerOptions,


note: some code uses semanticModel.Compilation, and some code uses this.Compilation. It would probably be better if we did things consistently. I would like to move that to a followup.

CyrusNajmabadi · 2025-01-27T22:12:17Z

@dotnet/roslyn-compiler this is ready for review.

CyrusNajmabadi · 2025-02-01T18:37:24Z

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

-            foreach (var blockAction in blockActions)
-            {
-                var codeBlockAction = blockAction as CodeBlockAnalyzerAction;
-                if (codeBlockAction != null)


all this logic moved into the executeBlockActions callbacks, so that it can all be strongly typed, without needing runtime type checks to determine what to do.

CyrusNajmabadi · 2025-02-04T18:04:18Z

@RikkiGibson @jcouv @jjonescz this is ready for review.

jcouv

LGTM Thanks (iteration 52)

ToddGrun and others added 12 commits January 23, 2025 15:10

Use some existing helpers to create a dictionary with ImmutableArray …

841bcbc

…values.

Early return to avoid the added array walk when not necessary

ec95ec4

Merge branch 'dotnet:main' into dev/toddgrun/ReduceCPUUnderExecuteSyn…

805be72

…taxNodeActions

Fix abuse of generics in analyzer driver

072f4c7

Add asserts

f038a00

Remove type checking

24cbdac

in progress

1639264

pass args

c4aa312

Make inference work

4fc71a8

Free

2ba4b34

pass immutable arrays

dcf2344

dotnet-issue-labeler bot added Area-Analyzers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 24, 2025

Indentation

85fd6fa

CyrusNajmabadi commented Jan 24, 2025

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs Outdated Show resolved Hide resolved

CyrusNajmabadi requested a review from ToddGrun January 24, 2025 20:02

Strong typing

cf94f86

CyrusNajmabadi marked this pull request as ready for review January 24, 2025 21:35

CyrusNajmabadi requested a review from a team as a code owner January 24, 2025 21:35

ToddGrun reviewed Jan 24, 2025

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs Show resolved Hide resolved

CyrusNajmabadi added 3 commits January 26, 2025 15:10

Merge branch 'main' into driverGenerics

62eb73a

Merge branch 'driverGenerics' of https://github.com/CyrusNajmabadi/ro…

a559788

…slyn into driverGenerics

Merge remote-tracking branch 'upstream/main' into driverGenerics

589ef48

CyrusNajmabadi marked this pull request as ready for review January 27, 2025 21:59

CyrusNajmabadi commented Jan 27, 2025

View reviewed changes

RikkiGibson self-assigned this Jan 27, 2025

CyrusNajmabadi commented Jan 27, 2025

View reviewed changes

src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs Show resolved Hide resolved

CyrusNajmabadi commented Jan 27, 2025

View reviewed changes

CyrusNajmabadi and others added 5 commits January 31, 2025 16:01

Merge remote-tracking branch 'upstream/main' into driverGenerics

8d9ba29

In progress

d134178

Inline moree

dba6f04

Simplifiy

7069af4

Simplifiy

a01c5f5

CyrusNajmabadi commented Feb 1, 2025

View reviewed changes

jjonescz approved these changes Feb 4, 2025

View reviewed changes

jcouv approved these changes Feb 4, 2025

View reviewed changes

CyrusNajmabadi merged commit 91cc9dc into dotnet:main Feb 4, 2025
24 checks passed

CyrusNajmabadi deleted the driverGenerics branch February 4, 2025 21:59

dotnet-policy-service bot added this to the Next milestone Feb 4, 2025

This was referenced Feb 6, 2025

[Automated] PRs inserted in VS build main-35805.68 #77073

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35805.254 #77079

Closed

[Automated] PRs inserted in VS build feature.d18initial-10311.01 #77169

Closed

333fred mentioned this pull request Feb 18, 2025

Merge main to runtime async branch #77265

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unnecessary casts of generic types to specific types in analyzer driver #76917

Fix unnecessary casts of generic types to specific types in analyzer driver #76917

CyrusNajmabadi commented Jan 24, 2025 •

edited

Loading

CyrusNajmabadi Jan 24, 2025

CyrusNajmabadi Jan 24, 2025

CyrusNajmabadi Jan 24, 2025

CyrusNajmabadi Jan 24, 2025

CyrusNajmabadi Jan 24, 2025

CyrusNajmabadi Jan 24, 2025

CyrusNajmabadi Jan 24, 2025

ToddGrun Jan 24, 2025

CyrusNajmabadi Jan 24, 2025

CyrusNajmabadi commented Jan 24, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025 •

edited

Loading

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi commented Jan 27, 2025

CyrusNajmabadi Jan 27, 2025

CyrusNajmabadi commented Jan 27, 2025

CyrusNajmabadi Feb 1, 2025

CyrusNajmabadi commented Feb 4, 2025

jcouv left a comment

		{
		return _syntaxNodeActions.OfType<SyntaxNodeAnalyzerAction<TLanguageKindEnum>>().ToImmutableArray();

Fix unnecessary casts of generic types to specific types in analyzer driver #76917

Fix unnecessary casts of generic types to specific types in analyzer driver #76917

Conversation

CyrusNajmabadi commented Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 24, 2025

Choose a reason for hiding this comment

CyrusNajmabadi Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 27, 2025

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 27, 2025

Choose a reason for hiding this comment

CyrusNajmabadi commented Feb 4, 2025

jcouv left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jan 24, 2025 •

edited

Loading

CyrusNajmabadi Jan 27, 2025 •

edited

Loading