Skip to content

Commit

Permalink
Remove semantic model keep-alive code in completion (#73844)
Browse files Browse the repository at this point in the history
* Remove semantic model keep-alive code in completion

There are quite a few providers that don't request/use the semantic model during their computations, and in the case where they are being used to commit / GetDescription, the model would be obtained/calculated unnecessarilly.

I don't believe the keep alive mechanism here is necessary anymore, as Solution.OnSemanticModelObtained caches the model when it's obtained for the active document in the solution.

Additionally, I switched a couple places in CommitManager to use JTF.Runn instead of WaitAndGetResult
  • Loading branch information
ToddGrun authored Jun 26, 2024
1 parent e79b9b5 commit 5aeb42b
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 17 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ private AsyncCompletionData.CommitResult Commit(

// This might be an item promoted by us, make sure we restore it to the original state first.
roslynItem = Helpers.DemoteItem(roslynItem);
CompletionChange change;
CompletionChange change = null!;

// We met an issue when external code threw an OperationCanceledException and the cancellationToken is not canceled.
// Catching this scenario for further investigations.
Expand All @@ -229,7 +229,10 @@ private AsyncCompletionData.CommitResult Commit(
if (_textView is IDebuggerTextView)
roslynItem = ImportCompletionItem.MarkItemToAlwaysFullyQualify(roslynItem);

change = completionService.GetChangeAsync(document, roslynItem, commitCharacter, cancellationToken).WaitAndGetResult(cancellationToken);
_threadingContext.JoinableTaskFactory.Run(async () =>
{
change = await completionService.GetChangeAsync(document, roslynItem, commitCharacter, cancellationToken).ConfigureAwait(true);
});
}
catch (OperationCanceledException e) when (e.CancellationToken != cancellationToken && FatalError.ReportAndCatch(e))
{
Expand Down Expand Up @@ -319,7 +322,11 @@ private AsyncCompletionData.CommitResult Commit(
var spanToFormat = triggerSnapshotSpan.TranslateTo(subjectBuffer.CurrentSnapshot, SpanTrackingMode.EdgeInclusive);

// Note: C# always completes synchronously, TypeScript is async
var changes = formattingService.GetFormattingChangesAsync(currentDocument, subjectBuffer, spanToFormat.Span.ToTextSpan(), cancellationToken).WaitAndGetResult(cancellationToken);
var changes = ImmutableArray<TextChange>.Empty;
_threadingContext.JoinableTaskFactory.Run(async () =>
{
changes = await formattingService.GetFormattingChangesAsync(currentDocument, subjectBuffer, spanToFormat.Span.ToTextSpan(), cancellationToken).ConfigureAwait(true);
});
subjectBuffer.ApplyChanges(changes);
}
}
Expand Down
10 changes: 4 additions & 6 deletions src/Features/Core/Portable/Completion/CompletionService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -214,14 +214,14 @@ public virtual TextSpan GetDefaultCompletionListSpan(SourceText text, int caretP

var extensionManager = document.Project.Solution.Workspace.Services.GetRequiredService<IExtensionManager>();

// We don't need SemanticModel here, just want to make sure it won't get GC'd before CompletionProviders are able to get it.
(document, var semanticModel) = await GetDocumentWithFrozenPartialSemanticsAsync(document, cancellationToken).ConfigureAwait(false);
document = await GetDocumentWithFrozenPartialSemanticsAsync(document, cancellationToken).ConfigureAwait(false);

var description = await extensionManager.PerformFunctionAsync(
provider,
cancellationToken => provider.GetDescriptionAsync(document, item, options, displayOptions, cancellationToken),
defaultValue: null,
cancellationToken).ConfigureAwait(false);
GC.KeepAlive(semanticModel);

return description;
}

Expand All @@ -245,8 +245,7 @@ public virtual async Task<CompletionChange> GetChangeAsync(
{
var extensionManager = document.Project.Solution.Workspace.Services.GetRequiredService<IExtensionManager>();

// We don't need SemanticModel here, just want to make sure it won't get GC'd before CompletionProviders are able to get it.
(document, var semanticModel) = await GetDocumentWithFrozenPartialSemanticsAsync(document, cancellationToken).ConfigureAwait(false);
document = await GetDocumentWithFrozenPartialSemanticsAsync(document, cancellationToken).ConfigureAwait(false);

var change = await extensionManager.PerformFunctionAsync(
provider,
Expand All @@ -256,7 +255,6 @@ public virtual async Task<CompletionChange> GetChangeAsync(
if (change == null)
return CompletionChange.Create(new TextChange(new TextSpan(), ""));

GC.KeepAlive(semanticModel);
Debug.Assert(item.Span == change.TextChange.Span || item.IsComplexTextEdit);
return change;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,7 @@ internal virtual async Task<CompletionList> GetCompletionsAsync(
ImmutableHashSet<string>? roles = null,
CancellationToken cancellationToken = default)
{
// We don't need SemanticModel here, just want to make sure it won't get GC'd before CompletionProviders are able to get it.
(document, var semanticModel) = await GetDocumentWithFrozenPartialSemanticsAsync(document, cancellationToken).ConfigureAwait(false);
document = await GetDocumentWithFrozenPartialSemanticsAsync(document, cancellationToken).ConfigureAwait(false);

var text = await document.GetValueTextAsync(cancellationToken).ConfigureAwait(false);
var completionListSpan = GetDefaultCompletionListSpan(text, caretPosition);
Expand Down Expand Up @@ -115,8 +114,6 @@ internal virtual async Task<CompletionList> GetCompletionsAsync(
var augmentingContexts = await ComputeNonEmptyCompletionContextsAsync(
document, caretPosition, trigger, options, completionListSpan, augmentingProviders, sharedContext, cancellationToken).ConfigureAwait(false);

GC.KeepAlive(semanticModel);

// Providers are ordered, but we processed them in our own order. Ensure that the
// groups are properly ordered based on the original providers.
var completionProviderToIndex = GetCompletionProviderToIndex(providers);
Expand Down Expand Up @@ -172,19 +169,20 @@ static async Task<ImmutableArray<CompletionProvider>> GetAugmentingProvidersAsyn
}

/// <summary>
/// Returns a document with frozen partial semantic unless we already have a complete compilation available.
/// Returns a document with frozen partial semantic model unless caller is test code require full semantics.
/// Getting full semantic could be costly in certain scenarios and would cause significant delay in completion.
/// In most cases we'd still end up with complete document, but we'd consider it an acceptable trade-off even when
/// we get into this transient state.
/// </summary>
private async Task<(Document document, SemanticModel? semanticModel)> GetDocumentWithFrozenPartialSemanticsAsync(Document document, CancellationToken cancellationToken)
private async Task<Document> GetDocumentWithFrozenPartialSemanticsAsync(Document document, CancellationToken cancellationToken)
{
if (_suppressPartialSemantics)
{
return (document, await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false));
var _ = await document.GetRequiredSemanticModelAsync(cancellationToken).ConfigureAwait(false);
return document;
}

return await document.GetFullOrPartialSemanticModelAsync(cancellationToken).ConfigureAwait(false);
return document.WithFrozenPartialSemantics(cancellationToken);
}

private static bool ValidatePossibleTriggerCharacterSet(CompletionTriggerKind completionTriggerKind, IEnumerable<CompletionProvider> triggeredProviders,
Expand Down

0 comments on commit 5aeb42b

Please sign in to comment.