WIP: Reduce CPU costs under AnalyzerExecutor.ExecuteSyntaxNodeActions #76894
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.
dotnet-issue-labeler
bot
added
Area-Analyzers
untriaged
| } | ||
|
|
||
| analyzersByKind.Free(); | ||
| return result.ToImmutable(); |
There was a problem hiding this comment.
i could have sworn there was a helper that alreaday did the conversion (with all the value freeing) for this sort of Dict<K, arrayBuilder> type.
There was a problem hiding this comment.
I saw IDictionaryExtensions.MultiAdd and PooledBuilderExtensions.ToImmutableMultiDictionaryAndFree that I mentioned in the comments, but I couldn't get to those from the compiler. I definitely could have missed something, but I did look around.
There was a problem hiding this comment.
ah. just move to a common shared code location. we have tons. it will require compiler signoff. but given the niceness of the pattern, i think that should be fine.
There was a problem hiding this comment.
Found something. AddPooled and ToImmutableSegmentedDictionaryAndFree both exist under the compilers folder, I'm fine switching this to ImmutableSegmentedDictionary
| @@ -2611,10 +2611,21 @@ void executeNodeActions() | |||
|
|
|||
| void executeNodeActionsByKind(ArrayBuilder<SyntaxNode> nodesToAnalyze, GroupedAnalyzerActions groupedActions, bool arePerSymbolActions) | |||
| { | |||
| var analyzersForNodes = PooledHashSet<DiagnosticAnalyzer>.GetInstance(); | |||
There was a problem hiding this comment.
can this be lifted higher?
There was a problem hiding this comment.
Actually not. It depends on groupdActions which differs from the different callers.
There was a problem hiding this comment.
but you can clear the results between the calls. basically don't return to the pool just to fetch it out :) the code isn't concurrent, so you can use the same scratch buffer multiple times, right?
|
test insertion PR: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/605482 |
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.
*** before changes ***
