Further cleanup and code reduction in analyzer driver. #76921
Conversation
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.
| SyntaxNode node, | ||
| ISymbol containingSymbol, | ||
| SemanticModel semanticModel, | ||
| ExecutionData executionData, |
There was a problem hiding this comment.
this subsumes the common 4 pieces of data passed to all these methods.
| node, containingSymbol, semanticModel, AnalyzerOptions, addDiagnostic, | ||
| isSupportedDiagnostic, filterSpan, isGeneratedCode, cancellationToken); | ||
| node, executionData.DeclaredSymbol, executionData.SemanticModel, AnalyzerOptions, addDiagnostic, | ||
| isSupportedDiagnostic, executionData.FilterSpan, executionData.IsGeneratedCode, cancellationToken); |
There was a problem hiding this comment.
note: i could push the ExecutionData struct into these context objects to make them smaller as well if that's desired.
| cancellationToken); | ||
| } | ||
|
|
||
| private readonly record struct ExecutionData( |
There was a problem hiding this comment.
The compiler's use of records is very restricted. It doesn't seem like we're making use of Equals on this new type, so it should just be a regular struct.
There was a problem hiding this comment.
sure. i can do that. though it seems very verbose. :)
There was a problem hiding this comment.
Sorry, it needs to be even more verbose ;-) We also don't do primary constructor captures.
This should have been flagged by an analyzer: dotnet/roslyn-analyzers#7138
Tagging @333fred to confirm
There was a problem hiding this comment.
what is the value in that verbosity? like... at least with records i can see not wanting the IL/metadata. But really this seems to be adding noise and hiding clear concepts in lots of code. :)
There was a problem hiding this comment.
note: the analyzer you are referencing disallows implicit capture. there is no implicit capture. it's all explicitly written to the explicit fields.
jcouv
left a comment
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (iteration 59)
5 participants
Followup to #76917
Primary change is making a new helper structure to capture many parameters taht are passed along to so many methods in these codepaths.