Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make fix-all code action more parallel #73356

Merged
merged 8 commits into from
May 7, 2024
Merged

Make fix-all code action more parallel #73356

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
479790e
Make fix-all code action more parallel
CyrusNajmabadi May 6, 2024
83971bf
Simplify
CyrusNajmabadi May 6, 2024
84947a2
Simplify
CyrusNajmabadi May 6, 2024
03fd9b8
Simplify
CyrusNajmabadi May 6, 2024
8d62cd8
Update progress
CyrusNajmabadi May 6, 2024
b32db78
Remove comment
CyrusNajmabadi May 6, 2024
afd60a8
REmove
CyrusNajmabadi May 6, 2024
8470ca1
change string
CyrusNajmabadi May 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 18 additions & 50 deletions src/Workspaces/Core/Portable/CodeFixes/FixAllOccurrences/DocumentBasedFixAllProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixesAndRefactorings;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Shared.Utilities;
using Microsoft.CodeAnalysis.Text;
Expand Down Expand Up @@ -73,55 +72,34 @@ public sealed override IEnumerable<FixAllScope> GetSupportedFixAllScopes()
fixAllContext.GetDefaultFixAllTitle(), fixAllContext, FixAllContextsHelperAsync);

private Task<Solution?> FixAllContextsHelperAsync(FixAllContext originalFixAllContext, ImmutableArray<FixAllContext> fixAllContexts)
=> DocumentBasedFixAllProviderHelpers.FixAllContextsAsync(originalFixAllContext, fixAllContexts,
originalFixAllContext.Progress,
this.GetFixAllTitle(originalFixAllContext),
DetermineDiagnosticsAndGetFixedDocumentsAsync);

private async Task<Dictionary<DocumentId, (SyntaxNode? node, SourceText? text)>> DetermineDiagnosticsAndGetFixedDocumentsAsync(
FixAllContext fixAllContext,
IProgress<CodeAnalysisProgress> progressTracker)
{
// First, determine the diagnostics to fix.
var diagnostics = await DetermineDiagnosticsAsync(fixAllContext, progressTracker).ConfigureAwait(false);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method was inlined.


// 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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method was inlined.

}

/// <summary>
/// Determines all the diagnostics we should be fixing for the given <paramref name="fixAllContext"/>.
/// </summary>
private static async Task<ImmutableDictionary<Document, ImmutableArray<Diagnostic>>> DetermineDiagnosticsAsync(FixAllContext fixAllContext, IProgress<CodeAnalysisProgress> progressTracker)
{
using var _ = progressTracker.ItemCompletedScope();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return await FixAllContextHelper.GetDocumentDiagnosticsToFixAsync(fixAllContext).ConfigureAwait(false);
}

/// <summary>
/// Attempts to fix all the provided <paramref name="diagnostics"/> returning, for each updated document, either
/// the new syntax root for that document or its new text. Syntax roots are returned for documents that support
/// them, and are used to perform a final cleanup pass for formatting/simplication/etc. Text is returned for
/// documents that don't support syntax.
/// </summary>
private async Task<Dictionary<DocumentId, (SyntaxNode? node, SourceText? text)>> GetFixedDocumentsAsync(
FixAllContext fixAllContext, IProgress<CodeAnalysisProgress> progressTracker, ImmutableDictionary<Document, ImmutableArray<Diagnostic>> diagnostics)
=> DocumentBasedFixAllProviderHelpers.FixAllContextsAsync(
originalFixAllContext,
fixAllContexts,
originalFixAllContext.Progress,
this.GetFixAllTitle(originalFixAllContext),
DetermineDiagnosticsAndGetFixedDocumentsAsync);

private async Task DetermineDiagnosticsAndGetFixedDocumentsAsync(
FixAllContext fixAllContext, Action<(DocumentId documentId, (SyntaxNode? node, SourceText? text))> callback)
{
var cancellationToken = fixAllContext.CancellationToken;

using var _1 = progressTracker.ItemCompletedScope();
// First, determine the diagnostics to fix.
var diagnostics = await FixAllContextHelper.GetDocumentDiagnosticsToFixAsync(fixAllContext).ConfigureAwait(false);

// Second, get the fixes for all the diagnostics, and apply them to determine the new root/text for each doc.
if (diagnostics.IsEmpty)
return [];
return;

// Then, process all documents in parallel to get the change for each doc.
return await ProducerConsumer<(DocumentId, (SyntaxNode? node, SourceText? text))>.RunParallelAsync(
await RoslynParallel.ForEachAsync(
source: diagnostics.Where(kvp => !kvp.Value.IsDefaultOrEmpty),
produceItems: static async (kvp, callback, args, cancellationToken) =>
cancellationToken,
async (kvp, cancellationToken) =>
{
var (document, documentDiagnostics) = kvp;

var newDocument = await args.@this.FixAllAsync(args.fixAllContext, document, documentDiagnostics).ConfigureAwait(false);
var newDocument = await this.FixAllAsync(fixAllContext, document, documentDiagnostics).ConfigureAwait(false);
if (newDocument == null || newDocument == document)
return;

Expand All @@ -131,16 +109,6 @@ private static async Task<ImmutableDictionary<Document, ImmutableArray<Diagnosti
var text = newDocument.SupportsSyntaxTree ? null : await newDocument.GetValueTextAsync(cancellationToken).ConfigureAwait(false);

callback((document.Id, (node, text)));
},
consumeItems: static async (results, args, cancellationToken) =>
{
var docIdToNewRootOrText = new Dictionary<DocumentId, (SyntaxNode? node, SourceText? text)>();
await foreach (var (docId, nodeOrText) in results)
docIdToNewRootOrText[docId] = nodeOrText;

return docIdToNewRootOrText;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

},
args: (@this: this, fixAllContext),
cancellationToken).ConfigureAwait(false);
}).ConfigureAwait(false);
}
}
182 changes: 81 additions & 101 deletions src/Workspaces/Core/Portable/CodeFixesAndRefactorings/DocumentBasedFixAllProviderHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,13 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Runtime.CompilerServices;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeActions;
using Microsoft.CodeAnalysis.CodeFixes;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Remote;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.CodeAnalysis.Shared.Utilities;
using Microsoft.CodeAnalysis.Text;
using Roslyn.Utilities;
Expand All @@ -30,124 +28,106 @@ internal static class DocumentBasedFixAllProviderHelpers
ImmutableArray<TFixAllContext> fixAllContexts,
IProgress<CodeAnalysisProgress> progressTracker,
string progressTrackerDescription,
Func<TFixAllContext, IProgress<CodeAnalysisProgress>, Task<Dictionary<DocumentId, (SyntaxNode? node, SourceText? text)>>> getFixedDocumentsAsync)
Func<TFixAllContext, Action<(DocumentId documentId, (SyntaxNode? node, SourceText? text))>, Task> getFixedDocumentsAsync)
where TFixAllContext : IFixAllContext
{
var cancellationToken = originalFixAllContext.CancellationToken;

progressTracker.Report(CodeAnalysisProgress.Description(progressTrackerDescription));

var solution = originalFixAllContext.Solution;

// For code fixes, we have 3 pieces of work per project. Computing diagnostics, computing fixes, and applying fixes.
// For refactorings, we have 2 pieces of work per project. Computing refactorings, and applying refactorings.
var fixAllKind = originalFixAllContext.State.FixAllKind;
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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


var (dirtySolution, changedRootDocumentIds) = await GetInitialUncleanedSolutionAsync().ConfigureAwait(false);
return await CleanSolutionAsync(dirtySolution, changedRootDocumentIds).ConfigureAwait(false);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

broke into two nested functions to help out there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

view with whitespace off.


using var _1 = PooledDictionary<DocumentId, (SyntaxNode? node, SourceText? text)>.GetInstance(out var allContextsDocIdToNewRootOrText);
async Task<(Solution dirtySolution, ImmutableArray<DocumentId> changedRootDocumentIds)> GetInitialUncleanedSolutionAsync()
{
// First, iterate over all contexts, and collect all the changes for each of them. We'll be making a lot of
// calls to the remote server to compute diagnostics and changes. So keep a single connection alive to it
// so we never resync or recompute anything.
using var _2 = await RemoteKeepAliveSession.CreateAsync(solution, originalFixAllContext.CancellationToken).ConfigureAwait(false);

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

// TODO: consider computing this in parallel.
var singleContextDocIdToNewRootOrText = await getFixedDocumentsAsync(fixAllContext, progressTracker).ConfigureAwait(false);

// Note: it is safe to blindly add the dictionary for a particular context to the full dictionary. Each
// dictionary will only update documents within that context, and each context represents a distinct
// project, so these should all be distinct without collisions. However, to be very safe, we use an
// overwriting policy here to ensure nothing causes any problems here.
foreach (var kvp in singleContextDocIdToNewRootOrText)
allContextsDocIdToNewRootOrText[kvp.Key] = kvp.Value;
}
}
using var _ = await RemoteKeepAliveSession.CreateAsync(solution, cancellationToken).ConfigureAwait(false);

// Next, go and insert those all into the solution so all the docs in this particular project point at
// the new trees (or text). At this point though, the trees have not been cleaned up. We don't cleanup
// the documents as they are created, or one at a time as we add them, as that would cause us to run
// cleanup on N different solution forks (which would be very expensive). Instead, by adding all the
// changed documents to one solution, and then cleaning *those* we only perform cleanup semantics on one
// forked solution.
var currentSolution = solution;
foreach (var (docId, (newRoot, newText)) in allContextsDocIdToNewRootOrText)
{
currentSolution = newRoot != null
? currentSolution.WithDocumentSyntaxRoot(docId, newRoot)
: currentSolution.WithDocumentText(docId, newText!);
return await ProducerConsumer<(DocumentId documentId, (SyntaxNode? node, SourceText? text))>.RunParallelAsync(
source: fixAllContexts,
produceItems: static async (fixAllContext, callback, args, cancellationToken) =>
{
using var _ = args.progressTracker.ItemCompletedScope();

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

await args.getFixedDocumentsAsync(fixAllContext, callback).ConfigureAwait(false);
},
consumeItems: static async (stream, args, cancellationToken) =>
{
var currentSolution = args.solution;
using var _ = ArrayBuilder<DocumentId>.GetInstance(out var changedRootDocumentIds);

// Next, go and insert those all into the solution so all the docs in this particular project
// point at the new trees (or text). At this point though, the trees have not been cleaned up.
// We don't cleanup the documents as they are created, or one at a time as we add them, as that
// would cause us to run cleanup on N different solution forks (which would be very expensive).
// Instead, by adding all the changed documents to one solution, and then cleaning *those* we
// only perform cleanup semantics on one forked solution.
await foreach (var (docId, (newRoot, newText)) in stream)
{
// If we produced a new root (as opposed to new text), keep track of that doc-id so that we
// can clean this doc later.
if (newRoot != null)
changedRootDocumentIds.Add(docId);

currentSolution = newRoot != null
? currentSolution.WithDocumentSyntaxRoot(docId, newRoot)
: currentSolution.WithDocumentText(docId, newText!);
}

return (currentSolution, changedRootDocumentIds.ToImmutableAndClear());
},
args: (getFixedDocumentsAsync, progressTracker, solution),
cancellationToken).ConfigureAwait(false);
}

async Task<Solution> CleanSolutionAsync(Solution dirtySolution, ImmutableArray<DocumentId> changedRootDocumentIds)
{
using var _1 = progressTracker.ItemCompletedScope();

if (changedRootDocumentIds.IsEmpty)
return dirtySolution;

// We're about to making a ton of calls to this new solution, including expensive oop calls to get up to
// date compilations, skeletons and SG docs. Create and pin this solution so that all remote calls operate
// on the same fork and do not cause the forked solution to be created and dropped repeatedly.
using var _2 = await RemoteKeepAliveSession.CreateAsync(currentSolution, originalFixAllContext.CancellationToken).ConfigureAwait(false);

var finalSolution = await CleanupAndApplyChangesAsync(
progressTracker,
currentSolution,
allContextsDocIdToNewRootOrText,
originalFixAllContext.CancellationToken).ConfigureAwait(false);

return finalSolution;
}
}

/// <summary>
/// Take all the fixed documents and format/simplify/clean them up (if the language supports that), and take the
/// resultant text and apply it to the solution. If the language doesn't support cleanup, then just take the
/// given text and apply that instead.
/// </summary>
private static async Task<Solution> CleanupAndApplyChangesAsync(
IProgress<CodeAnalysisProgress> progressTracker,
Solution currentSolution,
Dictionary<DocumentId, (SyntaxNode? node, SourceText? text)> docIdToNewRootOrText,
CancellationToken cancellationToken)
{
using var _1 = progressTracker.ItemCompletedScope();

if (docIdToNewRootOrText.Count == 0)
return currentSolution;

// Next, go and cleanup any trees we inserted. Once we clean the document, we get the text of it and insert
// that back into the final solution. This way we can release both the original fixed tree, and the cleaned
// tree (both of which can be much more expensive than just text).
//
// Do this in parallel across all the documents that were fixed.

return await ProducerConsumer<(DocumentId docId, SourceText sourceText)>.RunParallelAsync(
source: docIdToNewRootOrText,
produceItems: static async (tuple, callback, currentSolution, cancellationToken) =>
{
var (docId, (newRoot, _)) = tuple;
if (newRoot != null)
using var _2 = await RemoteKeepAliveSession.CreateAsync(dirtySolution, cancellationToken).ConfigureAwait(false);

// Next, go and cleanup any trees we inserted. Once we clean the document, we get the text of it and insert that
// back into the final solution. This way we can release both the original fixed tree, and the cleaned tree
// (both of which can be much more expensive than just text).
//
// Do this in parallel across all the documents that were fixed and resulted in a new tree (as opposed to new
// text).
return await ProducerConsumer<(DocumentId docId, SourceText sourceText)>.RunParallelAsync(
source: changedRootDocumentIds,
produceItems: static async (documentId, callback, currentSolution, cancellationToken) =>
{
var cleaned = await GetCleanedDocumentAsync(
currentSolution.GetRequiredDocument(docId), cancellationToken).ConfigureAwait(false);
callback(cleaned);
}
},
consumeItems: static async (results, currentSolution, _) =>
{
// Finally, apply the cleaned documents to the solution.
var finalSolution = currentSolution;
await foreach (var (docId, cleanedText) in results)
finalSolution = finalSolution.WithDocumentText(docId, cleanedText);

return finalSolution;
},
args: currentSolution,
cancellationToken).ConfigureAwait(false);

static async Task<(DocumentId docId, SourceText sourceText)> GetCleanedDocumentAsync(Document dirtyDocument, CancellationToken cancellationToken)
{
var cleanedDocument = await PostProcessCodeAction.Instance.PostProcessChangesAsync(dirtyDocument, cancellationToken).ConfigureAwait(false);
var cleanedText = await cleanedDocument.GetValueTextAsync(cancellationToken).ConfigureAwait(false);
return (dirtyDocument.Id, cleanedText);
var dirtyDocument = currentSolution.GetRequiredDocument(documentId);
var cleanedDocument = await PostProcessCodeAction.Instance.PostProcessChangesAsync(dirtyDocument, cancellationToken).ConfigureAwait(false);
var cleanedText = await cleanedDocument.GetValueTextAsync(cancellationToken).ConfigureAwait(false);
callback((dirtyDocument.Id, cleanedText));
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
consumeItems: static async (results, currentSolution, cancellationToken) =>
{
// Finally, apply the cleaned documents to the solution.
var finalSolution = currentSolution;
await foreach (var (docId, cleanedText) in results)
finalSolution = finalSolution.WithDocumentText(docId, cleanedText);

return finalSolution;
},
args: dirtySolution,
cancellationToken).ConfigureAwait(false);
}
}

Expand Down
Loading
Loading