Remove excess async state machines in completion #11620
Conversation
C# completion list is currently split across three "rewriter" objects that are async. However, none of them need to be async if they're passed a RazorCodeDocument. So, this change acquires a RazorCodeDocument up front before calling the rewriters. With this in place, the rewriters can be synchronous.
Now the completion list rewriters are synchronous, the RazorCodeDocument can be acquired by callers of RewriteCSharpResponseAsync and passed to it. Then, RewriteCSharpResponseAsync can be made synchronous.
The only reason this method was async was to call the IDocumentMappingService extension method, GetPositionInfoAsync. However, the reason *that* extension method is async is to call GetCodeDocumentAsync(). Ultimately, this method can be made synchronous by passing RazorCodeDocument to it.
This method is async just to call DocumentContext.GetCodeDocumentAsync and the only caller has a RazorCodeDocument in hand. So, this method can be removed in favor of the synchronous version.
This change audits and refactors to the GetCompletionListAsync methods on CompletionListProvider, RazorCompletionListProvider and DelegatedCompletionListProvider in favor of removing async state machines where possible.
| internal class RazorCompletionEndpoint( | ||
| CompletionListProvider completionListProvider, | ||
| CompletionTriggerAndCommitCharacters completionTriggerAndCommitCharacters, | ||
| ITelemetryReporter? telemetryReporter, |
There was a problem hiding this comment.
ℹ️ Only tests and benchmarks were calling the RazorCompletionEndpoint with a null telemetry reporter.
|
|
||
| var autoShownCompletion = completionContext.InvokeKind != VSInternalCompletionInvokeKind.Explicit; | ||
| if (autoShownCompletion && !_optionsMonitor.CurrentValue.AutoShowCompletion) | ||
| var sourceText = await documentContext.GetSourceTextAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
ℹ️ I've reordered some of the checks to push fast checks ahead of checks that require async work.
There was a problem hiding this comment.
Do any of these changes apply to the cohost completion endpoint? Though even if they do, I'm fine if you want to leave that for me, I suspect I need to audit that, and compare to this endpoint anyway
There was a problem hiding this comment.
I didn't actually look at the co-hosting endpoint. I was primarily looking at callers of CompletionListProvider.GetCompletionListAsync(...). For co-hosting, that's RemoteCompletionService.
There was a problem hiding this comment.
The options checking would be in the endpoint, which is why I thought of it, but I just looked and the ordering of the checks there is fine.
|
|
||
| namespace Microsoft.CodeAnalysis.Razor.Completion.Delegation; | ||
|
|
||
| internal interface IDelegatedCSharpCompletionResponseRewriter |
There was a problem hiding this comment.
💡 Eventually, this should be removed and the implementations of DesignTimeHelperResolutionRewriter, SnippetResponseRewriter and TextEditResponseRewriter should be inlined into DelegateCompletionHelper.RewriteCSharpResponse(...). It's extremely wasteful to loop through all of the completion items and update them three times.
There was a problem hiding this comment.
Completion was the first thing moved to a "single server" model, and there is a lot of over-abstraction IMO. I almost removed CompletionListProvider itself in my PR, because it's essentially useless. We'll get it all eventually :)
| codeDocument, | ||
| absoluteIndex, | ||
| completionContext, | ||
| documentContext.GetTextDocumentIdentifierAndVersion(), |
There was a problem hiding this comment.
ℹ️ Note that this is now the only reason a DocumentContext is passed through. Perhaps we can find a way to remove this in future.
|
|
||
| // The completion list is cached and can be retrieved via this result id to enable the resolve completion functionality. | ||
| var razorResolveContext = new RazorCompletionResolveContext(documentContext.FilePath, razorCompletionItems); | ||
| var filePath = codeDocument.Source.FilePath.AssumeNotNull(); |
There was a problem hiding this comment.
👀 This means that any RazorCodeDocument passed in must have a RazorSourceDocument with an accurate file path. Both ProjectSnapshotManager and co-hosting guarantee this fact, but it required tweaking to some tests.
|
|
||
| var autoShownCompletion = completionContext.InvokeKind != VSInternalCompletionInvokeKind.Explicit; | ||
| if (autoShownCompletion && !_optionsMonitor.CurrentValue.AutoShowCompletion) | ||
| var sourceText = await documentContext.GetSourceTextAsync(cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
Do any of these changes apply to the cohost completion endpoint? Though even if they do, I'm fine if you want to leave that for me, I suspect I need to audit that, and compare to this endpoint anyway
|
I merged my PR. Looks like no conflicts! |
3 participants
A lot of the code to compute completion lists is asynchronous in order to call
DocumentContext.GetCodeDocumentAsync(...). However, many of these calls can be made synchronous if theRazorCodeDocumentis computed up front and passed in. In addition, I've updated some methods that have both synchronous and asynchronous paths to returnValueTask.I hope that I don't conflict too badly with @davidwengier's completion work.
CI Build: https://dev.azure.com/dnceng/internal/_build/results?buildId=2662887&view=results
Test Insertion: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/619102