Pin solutions when doing fix all operation #72909
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -93,7 +93,7 @@ private partial class SuggestedActionsSource : IAsyncSuggestedActionsSource | |||
| // especially important as we are sending disparate requests for diagnostics, and we do not want the | |||
| // individual diagnostic requests to redo all the work to run source generators, create skeletons, | |||
| // etc. | |||
| using var _1 = RemoteKeepAliveSession.Create(document.Project.Solution, _listener); | |||
| using var _1 = await RemoteKeepAliveSession.CreateAsync(document.Project.Solution, cancellationToken).ConfigureAwait(false); | |||
There was a problem hiding this comment.
the async entrypoint is nicer for async clients, ensuring that we don't proceed until actually makign the remote host client, and issuing the keepalive call. it also means an async caller doesn't need access to an async listener.
| @@ -20,26 +20,31 @@ namespace Microsoft.CodeAnalysis.CodeFixesAndRefactorings; | |||
| /// </summary> | |||
| internal static class DefaultFixAllProviderHelpers | |||
| { | |||
| public static async Task<CodeAction?> GetFixAsync<TFixAllContext>( | |||
| public static CodeAction GetFix<TFixAllContext>( | |||
There was a problem hiding this comment.
view with whitespace off.
| /// <summary> | ||
| /// Gets a new <see cref="FixAllContext"/> with the given cancellationToken. | ||
| /// </summary> | ||
| public FixAllContext WithCancellationToken(CancellationToken cancellationToken) |
There was a problem hiding this comment.
moved into the generic With helper below.
| @@ -39,7 +45,7 @@ internal static class DefaultFixAllProviderHelpers | |||
| return null; | |||
|
|
|||
| return CodeAction.Create( | |||
| title, c => Task.FromResult(solution)); | |||
| title, _ => Task.FromResult(solution)); | |||
There was a problem hiding this comment.
@mavasani Aside: why do we do all teh expensive comptuation above, instead of doing it in the async callback passed to CodeAction.Create?
| @@ -40,32 +42,52 @@ internal static class DocumentBasedFixAllProviderHelpers | |||
| var workItemCount = fixAllKind == FixAllKind.CodeFix ? 3 : 2; | |||
| progressTracker.AddItems(fixAllContexts.Length * workItemCount); | |||
There was a problem hiding this comment.
this file is hte meat of hte change.
| // 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); |
There was a problem hiding this comment.
this is the first large scale operation we do. where we process the entire initial solution to determine the first pass of documents changed. for this, we want things pinned so nothing drops between local and oop.
| currentSolution = newRoot != null | ||
| ? currentSolution.WithDocumentSyntaxRoot(docId, newRoot) | ||
| : currentSolution.WithDocumentText(docId, newText!); | ||
| } |
There was a problem hiding this comment.
we then go and form the next full solution, with all those initial changes applied.
| // 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); |
There was a problem hiding this comment.
we then do a final cleanup pass on that next-solution. this will again be a ton of work, with lots of oop calls. so this also needs to pin the solution during this time.
| { | ||
| // First, compute and apply the fixes. | ||
| var docIdToNewRootOrText = await getFixedDocumentsAsync(fixAllContext, progressTracker).ConfigureAwait(false); | ||
| // TODO: consider computing this in parallel. |
There was a problem hiding this comment.
i will, but i'd like to put in another PR :)
src/Workspaces/Core/Portable/CodeFixesAndRefactorings/DocumentBasedFixAllProviderHelpers.cs
Show resolved
Hide resolved
|
@jasonmalinowski For review when you get back. |
Previously fix-all would do thingsl ike the following:
This is normally not a problem, but it opens ourselves up to an issue (which i repro'ed) when the operation is very long running. Namely, we sync the solution over, do an op on OOP with it, then come back. Before the next operation calls out to oop, oop drops that solution. So on the next call, we have to sync it again. this is normally cheap in terms of the sync, but it means we get a fresh solution instance on the oop side. This fresh solution will not have the cached compilations from the last call, causing them to be recreated. With SG and skeletons, this can eb super expensive.
The fix here, before doing very many operations on the same solution, is to pin that solution on the remote side. That way, it won't get dropped between calls, and we can benefit from all teh compilations and wahtnot staying cached.
This shaved off a couple of minutes doing a 'fix all in solution' change that i was trying out.