Skip to content

Remove blocking sync calls in rename #76521

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

Merged
CyrusNajmabadi merged 3 commits into from
Dec 19, 2024
Merged

Remove blocking sync calls in rename #76521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e158050
Remove blocking sync calls in rename
CyrusNajmabadi Dec 19, 2024
f997952
Remove test only code
CyrusNajmabadi Dec 19, 2024
b0c4f83
Remove unneessary check
CyrusNajmabadi Dec 19, 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
4 changes: 2 additions & 2 deletions src/EditorFeatures/Core/RenameTracking/RenameTrackingCodeRefactoringProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,10 @@ public RenameTrackingCodeRefactoringProvider(

public override Task ComputeRefactoringsAsync(CodeRefactoringContext context)
{
var (document, span, cancellationToken) = context;
var (document, span, _) = context;

var (action, renameSpan) = RenameTrackingTaggerProvider.TryGetCodeAction(
document, span, _refactorNotifyServices, _undoHistoryRegistry, cancellationToken);
document, span, _refactorNotifyServices, _undoHistoryRegistry);

if (action != null)
context.RegisterRefactoring(action, renameSpan);
Expand Down
15 changes: 5 additions & 10 deletions ...itorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.RenameTrackingCodeAction.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,8 @@ protected override Task<ImmutableArray<CodeActionOperation>> ComputeOperationsAs
IProgress<CodeAnalysisProgress> progress, CancellationToken cancellationToken)
{
// Invoked directly without previewing.
if (_renameTrackingCommitter == null)
{
if (!TryInitializeRenameTrackingCommitter(cancellationToken))
{
return SpecializedTasks.EmptyImmutableArray<CodeActionOperation>();
}
}
if (_renameTrackingCommitter == null && !TryInitializeRenameTrackingCommitter())
return SpecializedTasks.EmptyImmutableArray<CodeActionOperation>();

var committerOperation = new RenameTrackingCommitterOperation(_renameTrackingCommitter, _threadingContext);
return Task.FromResult(ImmutableArray.Create<CodeActionOperation>(committerOperation));
Expand All @@ -74,7 +69,7 @@ protected override Task<ImmutableArray<CodeActionOperation>> ComputeOperationsAs
protected override async Task<IEnumerable<CodeActionOperation>> ComputePreviewOperationsAsync(CancellationToken cancellationToken)
{
if (!_globalOptions.GetOption(RenameTrackingOptionsStorage.RenameTrackingPreview, _document.Project.Language) ||
!TryInitializeRenameTrackingCommitter(cancellationToken))
!TryInitializeRenameTrackingCommitter())
{
return await SpecializedTasks.EmptyEnumerable<CodeActionOperation>().ConfigureAwait(false);
}
Expand All @@ -84,14 +79,14 @@ protected override async Task<IEnumerable<CodeActionOperation>> ComputePreviewOp
return [new ApplyChangesOperation(solutionSet.RenamedSolution)];
}

private bool TryInitializeRenameTrackingCommitter(CancellationToken cancellationToken)
private bool TryInitializeRenameTrackingCommitter()
{
if (_document.TryGetText(out var text))
{
var textBuffer = text.Container.GetTextBuffer();
if (textBuffer.Properties.TryGetProperty(typeof(StateMachine), out StateMachine stateMachine))
{
if (!stateMachine.CanInvokeRename(out _, cancellationToken: cancellationToken))
if (!stateMachine.CanInvokeRename(out _))
Copy link
Contributor Author

@CyrusNajmabadi CyrusNajmabadi Dec 19, 2024

Choose a reason for hiding this comment

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

this caller passed in false, so its behavior is unchanged.

{
// The rename tracking could be dismissed while a codefix is still cached
// in the lightbulb. If this happens, do not perform the rename requested
Expand Down
21 changes: 11 additions & 10 deletions src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.StateMachine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ private void Buffer_Changed(object sender, TextContentChangedEventArgs e)
public void UpdateTrackingSessionIfRenamable()
{
ThreadingContext.ThrowIfNotOnUIThread();
if (this.TrackingSession.IsDefinitelyRenamableIdentifier())
if (this.TrackingSession.IsDefinitelyRenamableIdentifierFastCheck())
{
this.TrackingSession.CheckNewIdentifier(this, Buffer.CurrentSnapshot);
TrackingSessionUpdated();
Expand Down Expand Up @@ -214,7 +214,7 @@ public bool ClearTrackingSession()
previousTrackingSession.Cancel();

// If there may have been a tag showing, then actually clear the tags.
if (previousTrackingSession.IsDefinitelyRenamableIdentifier())
if (previousTrackingSession.IsDefinitelyRenamableIdentifierFastCheck())
{
TrackingSessionCleared(previousTrackingSession.TrackingSpan);
}
Expand All @@ -229,7 +229,7 @@ public bool ClearVisibleTrackingSession()
{
ThreadingContext.ThrowIfNotOnUIThread();

if (this.TrackingSession != null && this.TrackingSession.IsDefinitelyRenamableIdentifier())
if (this.TrackingSession != null && this.TrackingSession.IsDefinitelyRenamableIdentifierFastCheck())
{
var document = Buffer.CurrentSnapshot.GetOpenDocumentInCurrentContextWithChanges();
if (document != null)
Expand Down Expand Up @@ -271,7 +271,7 @@ internal int StoreCurrentTrackingSessionAndGenerateId()

public bool CanInvokeRename(
[NotNullWhen(true)] out TrackingSession trackingSession,
bool isSmartTagCheck = false, bool waitForResult = false, CancellationToken cancellationToken = default)
bool isSmartTagCheck = false)
{
// This needs to be able to run on a background thread for the diagnostic.

Expand All @@ -280,14 +280,15 @@ public bool CanInvokeRename(
return false;

return TryGetSyntaxFactsService(out var syntaxFactsService) && TryGetLanguageHeuristicsService(out var languageHeuristicsService) &&
trackingSession.CanInvokeRename(syntaxFactsService, languageHeuristicsService, isSmartTagCheck, waitForResult, cancellationToken);
trackingSession.CanInvokeRename(syntaxFactsService, languageHeuristicsService, isSmartTagCheck);
}

internal (CodeAction action, TextSpan renameSpan) TryGetCodeAction(
Document document, SourceText text, TextSpan userSpan,
Document document,
SourceText text,
TextSpan userSpan,
IEnumerable<IRefactorNotifyService> refactorNotifyServices,
ITextUndoHistoryRegistry undoHistoryRegistry,
CancellationToken cancellationToken)
ITextUndoHistoryRegistry undoHistoryRegistry)
{
try
{
Expand All @@ -299,7 +300,7 @@ public bool CanInvokeRename(
// engine will know that the document changed and not display the lightbulb anyway.

if (Buffer.AsTextContainer().CurrentText == text &&
CanInvokeRename(out var trackingSession, waitForResult: true, cancellationToken: cancellationToken))
Copy link
Contributor Author

@CyrusNajmabadi CyrusNajmabadi Dec 19, 2024

Choose a reason for hiding this comment

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

Step 2. this caller passed in true. So it appears as if its behavior changed. HOwever, it did not. Here's why. 'TryGetCodeAction' is called from https://github.com/dotnet/roslyn/pull/76521/files#diff-872822bf2183010393ff714b053784bc454bcbe417f0afa13dcad53f02bd25d8R119. Specifically this code:

            if (document != null && document.TryGetText(out var text))
            {
                var textBuffer = text.Container.TryGetTextBuffer();
                if (textBuffer != null &&
                    textBuffer.Properties.TryGetProperty(typeof(StateMachine), out StateMachine stateMachine) &&
                    stateMachine.CanInvokeRename(out _, cancellationToken: cancellationToken))
                {
                    return stateMachine.TryGetCodeAction(
                        document, text, textSpan, refactorNotifyServices, undoHistoryRegistry, cancellationToken);
                }

Notice that in order for this TryGetCodeAction to be called, 'CanInvokeRename' must return true. And 'CanInvokeRename' passes 'false' for waitForResult. Which means that the task must already be complete. Which means that it won't matter at all if TryGetCodeAction passes 'true' for waitForResult.

As such, we can remove this flag as it is never relevant.

CanInvokeRename(out var trackingSession))
{
var snapshotSpan = trackingSession.TrackingSpan.GetSpan(Buffer.CurrentSnapshot);

Expand All @@ -318,7 +319,7 @@ public bool CanInvokeRename(

return default;
}
catch (Exception e) when (FatalError.ReportAndPropagateUnlessCanceled(e, cancellationToken))
catch (Exception e) when (FatalError.ReportAndPropagateUnlessCanceled(e))
{
throw ExceptionUtilities.Unreachable();
}
Expand Down
12 changes: 5 additions & 7 deletions src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.TrackingSession.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,10 @@ async Task<bool> DetermineIfNewIdentifierBindsAsync(Task<TriggerIdentifierKind>
}
}

internal bool IsDefinitelyRenamableIdentifier()
internal bool IsDefinitelyRenamableIdentifierFastCheck()
{
// This needs to be able to run on a background thread for the CodeFix
return IsRenamableIdentifier(_isRenamableIdentifierTask, waitForResult: false, cancellationToken: CancellationToken.None);
return IsRenamableIdentifierFastCheck(_isRenamableIdentifierTask, out _);
Copy link
Contributor Author

@CyrusNajmabadi CyrusNajmabadi Dec 19, 2024

Choose a reason for hiding this comment

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

this caller passed in 'false' so it's behavior is unchanged.

}

public void Cancel()
Expand Down Expand Up @@ -259,13 +259,11 @@ private TriggerIdentifierKind DetermineIfRenamableSymbol(ISymbol symbol, Documen
internal bool CanInvokeRename(
ISyntaxFactsService syntaxFactsService,
IRenameTrackingLanguageHeuristicsService languageHeuristicsService,
bool isSmartTagCheck,
bool waitForResult,
CancellationToken cancellationToken)
bool isSmartTagCheck)
{
if (IsRenamableIdentifier(_isRenamableIdentifierTask, waitForResult, cancellationToken))
if (IsRenamableIdentifierFastCheck(_isRenamableIdentifierTask, out var triggerIdentifierKind))
{
var isRenamingDeclaration = _isRenamableIdentifierTask.Result == TriggerIdentifierKind.RenamableDeclaration;
var isRenamingDeclaration = triggerIdentifierKind == TriggerIdentifierKind.RenamableDeclaration;
var newName = TrackingSpan.GetText(TrackingSpan.TextBuffer.CurrentSnapshot);
var comparison = isRenamingDeclaration || syntaxFactsService.IsCaseSensitive ? StringComparison.Ordinal : StringComparison.OrdinalIgnoreCase;

Expand Down
52 changes: 17 additions & 35 deletions src/EditorFeatures/Core/RenameTracking/RenameTrackingTaggerProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
using Microsoft.CodeAnalysis.ErrorReporting;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Shared.TestHooks;
using Microsoft.CodeAnalysis.SQLite.Interop;
using Microsoft.CodeAnalysis.Text;
using Microsoft.VisualStudio.Text;
using Microsoft.VisualStudio.Text.Editor;
Expand Down Expand Up @@ -103,63 +104,44 @@ internal static bool ResetRenameTrackingStateWorker(Workspace workspace, Documen

public static (CodeAction action, TextSpan renameSpan) TryGetCodeAction(
Document document, TextSpan textSpan,
IEnumerable<IRefactorNotifyService> refactorNotifyServices,
ITextUndoHistoryRegistry undoHistoryRegistry,
CancellationToken cancellationToken)
IEnumerable<IRefactorNotifyService> refactorNotifyServices,
ITextUndoHistoryRegistry undoHistoryRegistry)
{
try
{
if (document != null && document.TryGetText(out var text))
{
var textBuffer = text.Container.TryGetTextBuffer();
if (textBuffer != null &&
textBuffer.Properties.TryGetProperty(typeof(StateMachine), out StateMachine stateMachine) &&
stateMachine.CanInvokeRename(out _, cancellationToken: cancellationToken))
Copy link
Contributor Author

@CyrusNajmabadi CyrusNajmabadi Dec 19, 2024

Choose a reason for hiding this comment

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

note: this line was removed as it is redundant. stateMachine.TryGetCodeAction performs this as well. The prior logic was effectively:

"if i know i can invoke, using a fast check, then invoke using the slow check". but the slow check always passes if hte fast check passes. So I change the "invoke using slow check" inside to "invoke using fast check" and just moved this outside fast check as it's now duplicative.

textBuffer.Properties.TryGetProperty(typeof(StateMachine), out StateMachine stateMachine))
{
return stateMachine.TryGetCodeAction(
document, text, textSpan, refactorNotifyServices, undoHistoryRegistry, cancellationToken);
document, text, textSpan, refactorNotifyServices, undoHistoryRegistry);
Copy link
Contributor Author

@CyrusNajmabadi CyrusNajmabadi Dec 19, 2024

Choose a reason for hiding this comment

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

Restating what was stated above. We know this single call is gated on the call above to CanInvokeRename. That call always did a non-blocking check of the BG 'is renamable' task. So CanInvokeRename returning true proves the task must already be compelte. Which means this code doesn't need to ever do a blocking wait itself.

}
}

return default;
}
catch (Exception e) when (FatalError.ReportAndPropagateUnlessCanceled(e, cancellationToken, ErrorSeverity.General))
catch (Exception e) when (FatalError.ReportAndPropagateUnlessCanceled(e, ErrorSeverity.General))
{
throw ExceptionUtilities.Unreachable();
}
}

internal static bool IsRenamableIdentifier(Task<TriggerIdentifierKind> isRenamableIdentifierTask, bool waitForResult, CancellationToken cancellationToken)
internal static bool IsRenamableIdentifierFastCheck(
Copy link
Contributor

@sharwell sharwell Dec 19, 2024

Choose a reason for hiding this comment

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

💡 Consider renaming these back (removing the FastCheck suffix) since there is no corresponding "slow check"

Copy link
Contributor Author

@CyrusNajmabadi CyrusNajmabadi Dec 19, 2024

Choose a reason for hiding this comment

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

i liked the FastCheck as it indicated to the caller that this wasn't a guaranteed check, but just a quick one that could be innacurate.

Task<TriggerIdentifierKind> isRenamableIdentifierTask, out TriggerIdentifierKind identifierKind)
{
if (isRenamableIdentifierTask.Status == TaskStatus.RanToCompletion && isRenamableIdentifierTask.Result != TriggerIdentifierKind.NotRenamable)
if (isRenamableIdentifierTask.Status == TaskStatus.RanToCompletion)
{
return true;
}
else if (isRenamableIdentifierTask.Status == TaskStatus.Canceled)
{
return false;
}
else if (waitForResult)
Copy link
Contributor Author

@CyrusNajmabadi CyrusNajmabadi Dec 19, 2024

Choose a reason for hiding this comment

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

Step 1. There was a boolean here that said if we wanted to block on computing this task or not. However, only one caller only ever passed true for this (all other callers got the default value of 'false' that was passed in as an optional argument.

{
return WaitForIsRenamableIdentifier(isRenamableIdentifierTask, cancellationToken);
}
else
{
return false;
var kind = isRenamableIdentifierTask.Result;
if (kind != TriggerIdentifierKind.NotRenamable)
{
identifierKind = kind;
return true;
}
}
}

internal static bool WaitForIsRenamableIdentifier(Task<TriggerIdentifierKind> isRenamableIdentifierTask, CancellationToken cancellationToken)
{
try
{
return isRenamableIdentifierTask.WaitAndGetResult_CanCallOnBackground(cancellationToken) != TriggerIdentifierKind.NotRenamable;
}
catch (OperationCanceledException e) when (e.CancellationToken != cancellationToken || cancellationToken == CancellationToken.None)
{
// We passed in a different cancellationToken, so if there's a race and
// isRenamableIdentifierTask was cancelled, we'll get a OperationCanceledException
return false;
}
identifierKind = default;
return false;
}
}
21 changes: 5 additions & 16 deletions src/EditorFeatures/Test/RenameTracking/RenameTrackingTaggerProviderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -874,32 +874,21 @@ End Enum
[WpfFact, WorkItem("http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/1028072")]
public void RenameTrackingDoesNotThrowAggregateException()
{
var waitForResult = false;
var notRenamable = Task.FromResult(RenameTrackingTaggerProvider.TriggerIdentifierKind.NotRenamable);
Assert.False(RenameTrackingTaggerProvider.IsRenamableIdentifier(notRenamable, waitForResult, CancellationToken.None));
Assert.False(RenameTrackingTaggerProvider.IsRenamableIdentifierFastCheck(notRenamable, out _));

var source = new TaskCompletionSource<RenameTrackingTaggerProvider.TriggerIdentifierKind>();
Assert.False(RenameTrackingTaggerProvider.IsRenamableIdentifier(source.Task, waitForResult, CancellationToken.None));
Assert.False(RenameTrackingTaggerProvider.IsRenamableIdentifierFastCheck(source.Task, out _));
source.TrySetResult(RenameTrackingTaggerProvider.TriggerIdentifierKind.RenamableReference);
Assert.True(RenameTrackingTaggerProvider.IsRenamableIdentifier(source.Task, waitForResult, CancellationToken.None));
Assert.True(RenameTrackingTaggerProvider.IsRenamableIdentifierFastCheck(source.Task, out _));

source = new TaskCompletionSource<RenameTrackingTaggerProvider.TriggerIdentifierKind>();
source.TrySetCanceled();
Assert.False(RenameTrackingTaggerProvider.IsRenamableIdentifier(source.Task, waitForResult, CancellationToken.None));
Assert.False(RenameTrackingTaggerProvider.WaitForIsRenamableIdentifier(source.Task, CancellationToken.None));
Assert.False(RenameTrackingTaggerProvider.IsRenamableIdentifierFastCheck(source.Task, out _));

source = new TaskCompletionSource<RenameTrackingTaggerProvider.TriggerIdentifierKind>();
source.TrySetException(new OperationCanceledException());
Assert.False(RenameTrackingTaggerProvider.IsRenamableIdentifier(source.Task, waitForResult, CancellationToken.None));
Assert.False(RenameTrackingTaggerProvider.WaitForIsRenamableIdentifier(source.Task, CancellationToken.None));
Assert.False(RenameTrackingTaggerProvider.WaitForIsRenamableIdentifier(source.Task, new CancellationTokenSource().Token));

source = new TaskCompletionSource<RenameTrackingTaggerProvider.TriggerIdentifierKind>();
Assert.Throws<OperationCanceledException>(() => RenameTrackingTaggerProvider.WaitForIsRenamableIdentifier(source.Task, new CancellationToken(canceled: true)));
var thrownException = new Exception();
source.TrySetException(thrownException);
var caughtException = Assert.Throws<Exception>(() => RenameTrackingTaggerProvider.WaitForIsRenamableIdentifier(source.Task, CancellationToken.None));
Assert.Same(thrownException, caughtException);
Assert.False(RenameTrackingTaggerProvider.IsRenamableIdentifierFastCheck(source.Task, out _));
}

[WpfFact]
Expand Down
Loading