Remove blocking sync calls in rename #76521
Conversation
ghost
added
Area-IDE
untriaged
| { | ||
| return false; | ||
| } | ||
| else if (waitForResult) |
There was a problem hiding this comment.
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.
| if (textBuffer != null && | ||
| textBuffer.Properties.TryGetProperty(typeof(StateMachine), out StateMachine stateMachine) && | ||
| stateMachine.CanInvokeRename(out _, cancellationToken: cancellationToken)) | ||
| stateMachine.CanInvokeRename(out _)) |
There was a problem hiding this comment.
this caller passed in false. so its behavior is unchanged.
| { | ||
| // 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 _); |
There was a problem hiding this comment.
this caller passed in 'false' so it's behavior is unchanged.
| if (textBuffer.Properties.TryGetProperty(typeof(StateMachine), out StateMachine stateMachine)) | ||
| { | ||
| if (!stateMachine.CanInvokeRename(out _, cancellationToken: cancellationToken)) | ||
| if (!stateMachine.CanInvokeRename(out _)) |
There was a problem hiding this comment.
this caller passed in false, so its behavior is unchanged.
| // 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)) |
There was a problem hiding this comment.
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.
CyrusNajmabadi
requested review from
JoeRobich,
ToddGrun and
jasonmalinowski
| { | ||
| return stateMachine.TryGetCodeAction( | ||
| document, text, textSpan, refactorNotifyServices, undoHistoryRegistry, cancellationToken); | ||
| document, text, textSpan, refactorNotifyServices, undoHistoryRegistry); |
There was a problem hiding this comment.
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.
| var textBuffer = text.Container.TryGetTextBuffer(); | ||
| if (textBuffer != null && | ||
| textBuffer.Properties.TryGetProperty(typeof(StateMachine), out StateMachine stateMachine) && | ||
| stateMachine.CanInvokeRename(out _, cancellationToken: cancellationToken)) |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| internal static bool IsRenamableIdentifier(Task<TriggerIdentifierKind> isRenamableIdentifierTask, bool waitForResult, CancellationToken cancellationToken) | ||
| internal static bool IsRenamableIdentifierFastCheck( |
There was a problem hiding this comment.
💡 Consider renaming these back (removing the FastCheck suffix) since there is no corresponding "slow check"
There was a problem hiding this comment.
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.
3 participants
The code change here is subtle, so i've doc'ed teh reasoning for it in the PR.