Avoid re-running all codeaction requests at low priority. #74083
Conversation
The ICodeActionRequestPriorityProviderExtensions.MatchesPriority method which takes in a CodeFixProvider determines whether a CodeFixProvider is run at a certain priority. This method previously always returned true at low priority, indicating that all CodeFixProviders should be run during the low priority calculations (a 2nd time for all Default/High providers). The reason it did this was to support de-prioritization of slow analyzers. Instead of running all codeFixProviders during low priority, we instead run 1) CodeFixProviders who's RequestPriority is low 2) CodeFixProviders who have an entry in their FixableDiagnosticIds that matches an entry from SupportedDiagnostics of a de-prioritized analyzer.
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
…nningUnnecessarily
…nningUnnecessarily
| internal sealed class SuggestedActionPriorityProvider( | ||
| CodeActionRequestPriority priority, | ||
| ConcurrentSet<DiagnosticAnalyzer> lowPriorityAnalyzers) | ||
| ConcurrentSet<DiagnosticAnalyzer> lowPriorityAnalyzers, | ||
| ConcurrentSet<string> lowPriorityAnalyzerSupportedDiagnosticIds) |
There was a problem hiding this comment.
feeling like i want these two sets merged into one type. don't feel strongly about it.
There was a problem hiding this comment.
Changed to a struct that contains both ConcurrentSets.
| @@ -234,12 +234,13 @@ private void OnTextViewClosed(object sender, EventArgs e) | |||
| return null; | |||
|
|
|||
| var lowPriorityAnalyzers = new ConcurrentSet<DiagnosticAnalyzer>(); | |||
| var lowPriorityAnalyzerSupportedDiagnosticIds = new ConcurrentSet<string>(); | |||
There was a problem hiding this comment.
intentional that this is not read?
There was a problem hiding this comment.
It's the same as lowPriorityAnalyzers. Read/Writes on both happen inside each of the SuggestedActionPriorityProviders.
| @@ -125,7 +126,7 @@ private partial class SuggestedActionsSource : IAsyncSuggestedActionsSource | |||
| var allSets = GetCodeFixesAndRefactoringsAsync( | |||
| state, requestedActionCategories, document, | |||
| range, selection, | |||
| new SuggestedActionPriorityProvider(priority, lowPriorityAnalyzers), | |||
| new SuggestedActionPriorityProvider(priority, lowPriorityAnalyzers, lowPriorityAnalyzerSupportedDiagnosticIds), | |||
There was a problem hiding this comment.
same question. it's interesting that it is captured by SuggestedActionPriorityProvider, so presumably writen to. but where it is read isn't clear to me yet.
| { | ||
| // 'Low' priority can be used for two types of code fixers: | ||
| // 1. Those which explicitly set their 'RequestPriority' to 'Low' and | ||
| // 2. Those which can fix diagnostics for expensive analyzers which were de-prioritized | ||
| // to 'Low' priority bucket to improve lightbulb population performance. | ||
| // Hence, when processing the 'Low' Priority bucket, we accept fixers with any RequestPriority, | ||
| // as long as they can fix a diagnostic from an analyzer that was executed in the 'Low' bucket. | ||
| // Hence, when processing the 'Low' Priority bucket and the priority provider indicates |
There was a problem hiding this comment.
perhaps mention that the first case was handled by the prior if statement. so we def have a case where the codefixprovider is higher priority than the fix priority itself.
(actually, it might be worthwhile to actually validate the codefixprovider priority here as well(
There was a problem hiding this comment.
Updated the comment and added a check to verify that the codeFixProvider's RequestPriority is either High or Default
| return true; | ||
| } | ||
|
|
||
| return provider.Priority == codeFixProvider.RequestPriority; | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| internal sealed class DefaultCodeActionRequestPriorityProvider(CodeActionRequestPriority? priority = null) : ICodeActionRequestPriorityProvider |
There was a problem hiding this comment.
i hate this optional value here.
There was a problem hiding this comment.
Would prefer to not change as part of this PR. Let me know if you feel strongly otherwise.
There was a problem hiding this comment.
It was a general gripe. You don't have to change it at all.
…orityCodeActionsRunningUnnecessarily
…nningUnnecessarily
The ICodeActionRequestPriorityProviderExtensions.MatchesPriority method which takes in a CodeFixProvider determines whether a CodeFixProvider is run at a certain priority. This method previously always returned true at low priority, indicating that all CodeFixProviders should be run during the low priority calculations (a 2nd time for all Default/High providers). The reason it did this was to support de-prioritization of slow analyzers.
Instead of running all codeFixProviders during low priority, we instead run