Make fix-all code action more parallel #73356

CyrusNajmabadi · 2024-05-06T21:22:26Z

No description provided.

CyrusNajmabadi · 2024-05-06T21:22:46Z

src/Workspaces/Core/Portable/CodeFixes/FixAllOccurrences/DocumentBasedFixAllProvider.cs

-        IProgress<CodeAnalysisProgress> progressTracker)
-    {
-        // First, determine the diagnostics to fix.
-        var diagnostics = await DetermineDiagnosticsAsync(fixAllContext, progressTracker).ConfigureAwait(false);


this method was inlined.

CyrusNajmabadi · 2024-05-06T21:22:53Z

src/Workspaces/Core/Portable/CodeFixes/FixAllOccurrences/DocumentBasedFixAllProvider.cs

-        var diagnostics = await DetermineDiagnosticsAsync(fixAllContext, progressTracker).ConfigureAwait(false);
-
-        // Second, get the fixes for all the diagnostics, and apply them to determine the new root/text for each doc.
-        return await GetFixedDocumentsAsync(fixAllContext, progressTracker, diagnostics).ConfigureAwait(false);


this method was inlined.

CyrusNajmabadi · 2024-05-06T21:23:03Z

src/Workspaces/Core/Portable/CodeFixes/FixAllOccurrences/DocumentBasedFixAllProvider.cs

-    /// </summary>
-    private static async Task<ImmutableDictionary<Document, ImmutableArray<Diagnostic>>> DetermineDiagnosticsAsync(FixAllContext fixAllContext, IProgress<CodeAnalysisProgress> progressTracker)
-    {
-        using var _ = progressTracker.ItemCompletedScope();


this went away. the caller is responsible for this now.

CyrusNajmabadi · 2024-05-06T21:24:02Z

src/Workspaces/Core/Portable/CodeFixesAndRefactorings/DocumentBasedFixAllProviderHelpers.cs

-        var workItemCount = fixAllKind == FixAllKind.CodeFix ? 3 : 2;
-        progressTracker.AddItems(fixAllContexts.Length * workItemCount);
+        // One work item for each context.  Then one final item to for cleaning the solution.
+        progressTracker.AddItems(fixAllContexts.Length + 1);


the math computation was originally wrong. it was not hte case that we did 3 items for fixes per context, and 2 per refactoring per context. Because this was just broken. i simplified this. the computation of progress length is done here, and the updating happens here as well. this is my preference for progress as having length computation and item-finish calls separated means it's too easy to get wrong.

CyrusNajmabadi · 2024-05-06T21:24:15Z

src/Workspaces/Core/Portable/CodeFixesAndRefactorings/DocumentBasedFixAllProviderHelpers.cs

+        progressTracker.AddItems(fixAllContexts.Length + 1);
+
+        var (dirtySolution, changedRootDocumentIds) = await GetInitialUncleanedSolutionAsync().ConfigureAwait(false);
+        return await CleanSolutionAsync(dirtySolution, changedRootDocumentIds).ConfigureAwait(false);


broke into two nested functions to help out there.

view with whitespace off.

CyrusNajmabadi · 2024-05-06T21:25:11Z

src/Workspaces/Core/Portable/CodeRefactorings/FixAllOccurences/DocumentBasedFixAllProvider.cs

    {
        Contract.ThrowIfFalse(fixAllContext.Scope is FixAllScope.Document or FixAllScope.Project
            or FixAllScope.ContainingMember or FixAllScope.ContainingType);

        var cancellationToken = fixAllContext.CancellationToken;

-        using var _1 = progressTracker.ItemCompletedScope();


we definitely weren't doing 'two' "completed" calls here. so this was entirely busted for progress.

CyrusNajmabadi · 2024-05-06T21:25:55Z

src/Workspaces/Core/Portable/CodeFixesAndRefactorings/DocumentBasedFixAllProviderHelpers.cs

+                        fixAllContext.Scope is FixAllScope.Document or FixAllScope.Project or FixAllScope.ContainingMember or FixAllScope.ContainingType);
+
+                    var singleContextDocIdToNewRootOrText = await args.getFixedDocumentsAsync(fixAllContext).ConfigureAwait(false);
+                    callback(singleContextDocIdToNewRootOrText);


i'm likely going to change this to a callback as well, to avoid this intermediary dictionary.

CyrusNajmabadi · 2024-05-06T21:31:42Z

src/Workspaces/Core/Portable/CodeFixes/FixAllOccurrences/DocumentBasedFixAllProvider.cs

-                await foreach (var (docId, nodeOrText) in results)
-                    docIdToNewRootOrText[docId] = nodeOrText;
-
-                return docIdToNewRootOrText;


no need for a producerconsumer here anymore. we're not making the intermediary dictionary. we're just calling back into callback with the doc data produced.

CyrusNajmabadi · 2024-05-06T21:32:05Z

src/Workspaces/Core/Portable/CodeRefactorings/FixAllOccurences/DocumentBasedFixAllProvider.cs

        // Process all documents in parallel to get the change for each doc.
        var documentsAndSpansToFix = await fixAllContext.GetFixAllSpansAsync(cancellationToken).ConfigureAwait(false);

-        return await ProducerConsumer<(DocumentId, (SyntaxNode? node, SourceText? text))>.RunParallelAsync(
+        await RoslynParallel.ForEachAsync(


no need for a producerconsumer here anymore. we're not making the intermediary dictionary. we're just calling back into callback with the doc data produced.

CyrusNajmabadi · 2024-05-06T21:49:12Z

src/Workspaces/Core/Portable/CodeFixesAndRefactorings/DocumentBasedFixAllProviderHelpers.cs

-        var workItemCount = fixAllKind == FixAllKind.CodeFix ? 3 : 2;
-        progressTracker.AddItems(fixAllContexts.Length * workItemCount);
+        // One work item for each context.
+        progressTracker.AddItems(fixAllContexts.Length);


the math computation was originally wrong. it was not hte case that we did 3 items for fixes per context, and 2 per refactoring per context. Because this was just broken. i simplified this. the computation of progress length is done here, and the updating happens here as well. this is my preference for progress as having length computation and item-finish calls separated means it's too easy to get wrong.

CyrusNajmabadi · 2024-05-06T21:57:50Z

src/Workspaces/Core/Portable/WorkspacesResources.resx

@@ -492,4 +492,7 @@
  <data name="Unexpected_value_0_in_DocumentKinds_array" xml:space="preserve">
    <value>Unexpected value '{0}' in DocumentKinds array.</value>
  </data>
+  <data name="Cleaning_fixed_documents" xml:space="preserve">
+    <value>Cleaning fixed documents</value>


i don't love this text. perhaps "Running code cleanup on fixed documents"?

CyrusNajmabadi · 2024-05-06T23:04:00Z

src/Workspaces/Core/Portable/CodeFixesAndRefactorings/DocumentBasedFixAllProviderHelpers.cs

+                    var dirtyDocument = args.dirtySolution.GetRequiredDocument(documentId);
+                    var cleanedDocument = await PostProcessCodeAction.Instance.PostProcessChangesAsync(dirtyDocument, cancellationToken).ConfigureAwait(false);
+                    var cleanedText = await cleanedDocument.GetValueTextAsync(cancellationToken).ConfigureAwait(false);
+                    callback((dirtyDocument.Id, cleanedText));


i have a followup PR that moves this cleanup step to OOP. so we can benefit from .net core execution wins.

CyrusNajmabadi · 2024-05-06T23:42:53Z

@ToddGrun ptal.

ToddGrun · 2024-05-07T14:13:25Z

src/Workspaces/Core/Portable/CodeFixesAndRefactorings/DocumentBasedFixAllProviderHelpers.cs

+                return dirtySolution;
+
+            // Clear out the progress so far.  We're starting a new progress pass for the final cleanup.
+            progressTracker.Report(CodeAnalysisProgress.Clear());


progressTracker.Report(CodeAnalysisProgress.Clear());

Out of curiosity, how is this expressed to the user? Does a bar fill during GetInitialUncleanedSolutionAsync, empty here, and then fill again during this method?

Yes. Exactly

ToddGrun

CyrusNajmabadi · 2024-05-10T04:39:09Z

@jasonmalinowski For review when you get back.

Make fix-all code action more parallel

479790e

CyrusNajmabadi requested a review from a team as a code owner May 6, 2024 21:22

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 6, 2024

CyrusNajmabadi requested a review from ToddGrun May 6, 2024 21:22

CyrusNajmabadi commented May 6, 2024

View reviewed changes

Simplify

83971bf

CyrusNajmabadi commented May 6, 2024

View reviewed changes

CyrusNajmabadi added 3 commits May 6, 2024 14:39

Simplify

84947a2

Simplify

03fd9b8

Update progress

8d62cd8

CyrusNajmabadi commented May 6, 2024

View reviewed changes

CyrusNajmabadi added 2 commits May 6, 2024 14:49

Remove comment

b32db78

REmove

afd60a8

CyrusNajmabadi commented May 6, 2024

View reviewed changes

change string

8470ca1

CyrusNajmabadi commented May 6, 2024

View reviewed changes

CyrusNajmabadi mentioned this pull request May 6, 2024

Run final fix-all code-cleanup phase out of process. #73357

Draft

ToddGrun reviewed May 7, 2024

View reviewed changes

ToddGrun approved these changes May 7, 2024

View reviewed changes

CyrusNajmabadi merged commit ce1d093 into dotnet:main May 7, 2024
25 checks passed

CyrusNajmabadi deleted the parallelFixAll branch May 7, 2024 14:45

dotnet-policy-service bot added this to the Next milestone May 7, 2024

dotnet-bot mentioned this pull request May 14, 2024

[Automated] PRs inserted in VS build main-34913.43 #73460

Closed

dotnet-bot mentioned this pull request May 21, 2024

[Automated] PRs inserted in VS build feature.debugger.main-34920.221 #73610

Closed

Cosifne modified the milestones: Next, 17.11 P2 May 28, 2024

dotnet-bot mentioned this pull request Jun 27, 2024

[Automated] PRs inserted in VS build feature.dotnetVS-35026.97 #74176

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make fix-all code action more parallel #73356

Make fix-all code action more parallel #73356

CyrusNajmabadi commented May 6, 2024

CyrusNajmabadi May 6, 2024

CyrusNajmabadi May 6, 2024

CyrusNajmabadi May 6, 2024

CyrusNajmabadi May 6, 2024

CyrusNajmabadi May 6, 2024

CyrusNajmabadi May 6, 2024

CyrusNajmabadi May 6, 2024

CyrusNajmabadi May 6, 2024

CyrusNajmabadi May 6, 2024

CyrusNajmabadi May 6, 2024

CyrusNajmabadi May 6, 2024

CyrusNajmabadi May 6, 2024

CyrusNajmabadi May 6, 2024

CyrusNajmabadi commented May 6, 2024

ToddGrun May 7, 2024

CyrusNajmabadi May 7, 2024

ToddGrun left a comment

CyrusNajmabadi commented May 10, 2024

Make fix-all code action more parallel #73356

Make fix-all code action more parallel #73356

Conversation

CyrusNajmabadi commented May 6, 2024

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

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 May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented May 10, 2024