Run phases of code cleanup in parallel #73386
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -16,7 +16,7 @@ namespace Microsoft.CodeAnalysis.IntroduceVariable; | |||
|
|
|||
| internal partial class AbstractIntroduceVariableService<TService, TExpressionSyntax, TTypeSyntax, TTypeDeclarationSyntax, TQueryExpressionSyntax, TNameSyntax> | |||
| { | |||
| internal abstract class AbstractIntroduceVariableCodeAction : CodeAction | |||
| private abstract class AbstractIntroduceVariableCodeAction : CodeAction | |||
There was a problem hiding this comment.
merge of #73392 into this PR. once that goes in, this will disappear from this PR.
| @@ -248,7 +250,7 @@ public Task<ImmutableArray<CodeActionOperation>> GetOperationsAsync(Cancellation | |||
|
|
|||
| if (operations != null) | |||
| { | |||
| return await this.PostProcessAsync(originalSolution, operations, cancellationToken).ConfigureAwait(false); | |||
| return await PostProcessAsync(originalSolution, operations, cancellationToken).ConfigureAwait(false); | |||
There was a problem hiding this comment.
became a static method.
|
|
||
| internal async Task<Solution> PostProcessChangesAsync( | ||
| internal static async Task<ImmutableArray<(DocumentId documentId, CodeCleanupOptions codeCleanupOptions)>> GetDocumentIdsAndOptionsAsync( | ||
| Solution solution, CodeCleanupOptionsProvider optionsProvider, ImmutableArray<DocumentId> documentIds, CancellationToken cancellationToken) |
There was a problem hiding this comment.
helper used here (for code actions that produce a solution (as opposed to a doc), and also in the FixAll-InSolution code. We want hte parallel benefits for both codepaths.
| { | ||
| // We do this in 4 serialized passes. This allows us to process all documents in parallel, while only forking | ||
| // the solution 4 times *total* (instead of 4 times *per* document). | ||
| progress.AddItems(documentIdsAndOptions.Length * 4); |
There was a problem hiding this comment.
this is the meat of hte change. 4 defined passes on the solution. not 4 passes per document.
| } | ||
| // Then do a pass where we cleanup semantics. | ||
| var semanticCleanedSolution = await CleanupSemanticsAsync( | ||
| syntaxCleanedSolution, documentIdsAndOptions, CodeAnalysisProgress.None, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
for features that produce a new solution (as oppposed to new document) they now get the benefit of this cleanup in parallel, and only 4 passes as well across all documents.
| progressTracker.Report(CodeAnalysisProgress.Clear()); | ||
| progressTracker.Report(CodeAnalysisProgress.Description(WorkspacesResources.Running_code_cleanup_on_fixed_documents)); | ||
|
|
||
| var cleanedSolution = await CodeAction.CleanSyntaxAndSemanticsAsync( |
There was a problem hiding this comment.
defers to the single helper to do the syntax and semantic cleanup passes.
| if (!document.SupportsSyntaxTree) | ||
| return document; | ||
|
|
||
| var cleanedSolution = await CleanupSyntaxAndSemanticsAsync( |
There was a problem hiding this comment.
cleaning a document just defers to the main N-pass helper, just processing this single document.
| // good state. The semantic cleanup passes may have introduced new nodes with elastic trivia that have to be | ||
| // cleaned. | ||
| static (document, options, cancellationToken) => CleanupSyntaxAsync(document, options, cancellationToken), | ||
| ]; |
There was a problem hiding this comment.
i really liked this. it emphasizes that we're doing a series of passes in a specific order.
| var solutionChanges = changedSolution.GetChanges(originalSolution); | ||
| var documentIds = solutionChanges | ||
| .GetProjectChanges() | ||
| .SelectMany(p => p.GetChangedDocuments(onlyGetDocumentsWithTextChanges: true).Concat(p.GetAddedDocuments())) |
There was a problem hiding this comment.
Probably not, these probably only affect code docs
There was a problem hiding this comment.
nope. those are never cleaned up. 'cleaning' is a specific C#/VB concept where we do final passes, applying formatting, and going and simplifying code that a code fixer annotated in the syntax trees.
| // Grab all the cleaned roots and produce the new solution snapshot from that. | ||
| var currentSolution = args.solution; | ||
| await foreach (var (documentId, newRoot) in stream) | ||
| currentSolution = currentSolution.WithDocumentSyntaxRoot(documentId, newRoot); |
There was a problem hiding this comment.
yes. we can totally do that. it's orthogonal to the work here, but i'm fully in support of it.
primarily, these prs ensure that we're not forking the solution N times and also getting compilatinos for each fork.
i will do this in a followup :)
|
@jasonmalinowski For review when you get back. |
Followup to #73383.
Prior to this PR, the way FixAll worked was that it would first call into every provider to produce the fixed document roots for all relevant files. This was done in parallel and would produce an updated solution.
We'd then go (in parallel) across all those fixed documents and woudl then clean them. This was inefficient because hte cleaning phase per document would fork the solution 4 times. First, to add missing imports for the doc. Second, to simplify any expanded constructs. Third, to case-correct VB files. Fourth, to format hte files (though this wouldn't incur compilation costs as it's purely syntactic). Each time it was forked, and the next cleaning operation was done on that document, that would incur all the costs to create compilations (including generators, skeletons, and any other semantic work).
This change fixes cleaning to be much smarter. Instead of 4 forks per document, tehre are 4 forks total. Specifically, we have the starting solution, we then add imports to all changed documents in parallel against that solution. We use the results of all those documents to produce the next solution. To that, we then simplify expanded constructs in parallel across all files, using htat to produce the next solution. We then finally do that for VB-case correction.