Update our 'delay until visible' logic to not throw cancellation exceptions #68382
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -51,6 +55,9 @@ internal static class ITextBufferVisibilityTrackerExtensions | |||
| if (service is null) | |||
| return; | |||
|
|
|||
| if (cancellationToken.IsCancellationRequested) | |||
| return; | |||
|
|
|||
| await threadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken); | |||
There was a problem hiding this comment.
❗ Cancellation here (during the switch) will result in an exception
There was a problem hiding this comment.
unfortunately, no NoThrowAwaitable for MainThreadAwaitable. Hopefully the chance of getting the CT just in the switch is low. (seems like the majority case would be where we're actually delay'ing and an event comes in in that time).
There was a problem hiding this comment.
I've been discussing with Andrew a new GetResult(out Exception) method that would support this case and also support the feature from #67487.
src/EditorFeatures/Core/Workspaces/ITextBufferVisibilityTracker.cs
Outdated
Show resolved
Hide resolved
CyrusNajmabadi
force-pushed
the
noThrowDelay
branch
from
efd8fbc to
316b1f4
Compare
CyrusNajmabadi
force-pushed
the
noThrowDelay
branch
from
12929d7 to
485db1d
Compare
|
@sharwell ptal. |
src/EditorFeatures/Core/Workspaces/ITextBufferVisibilityTracker.cs
Outdated
Show resolved
Hide resolved
src/EditorFeatures/Core/Workspaces/ITextBufferVisibilityTracker.cs
Outdated
Show resolved
Hide resolved
| var callback = void () => visibilityChangedTaskSource.TrySetResult(true); | ||
| service.RegisterForVisibilityChanges(subjectBuffer, callback); | ||
|
|
||
| try |
There was a problem hiding this comment.
I assume utilizing CancellationToken.Register and doing something like:
cancellationToken.Register(callback);
and then sending in CancellationToken.None to the IAsynchronousOperationListener.Delay call isn't an option to prevent the exception from being thrown when Task.Delay is inside the listener.Delay call?
There was a problem hiding this comment.
This is a very nice idea. I wasn't able to apply it though here as we still need the CT for Expeditable.Delay. Maybe sam has thoughts on how to combine both of htese?
ghost
added this to the 
This is a small change (as part of a large set of work) to address taggers causing stress on the system due to excessive exceptions and lock contention (specifically on large core count machines).
The issue being addressed here is that taggers will constantly gets a stream of events coming in saying "something changed, update tags in the near future". The tagger then says "ok, but if i'm not visible, i'm going to delay that work till much later". This delaying is good for preventing unnecessary CPU work actually computing the tags, but is bad in that it is causing excess cancellation exceptions to be thrown. Specifically, the tagger puts itself to sleep when non-visible, but then gets canceled (throwing an exception) when the next event comes in. This "event-cancel-sleep, event-cancel-sleep, ..." can happen repeatedly, across all taggers (we have many).
These exceptions are not cheap themselves. For example, they take locks here to copy exception stack information: https://referencesource.microsoft.com/#mscorlib/system/exception.cs,a445c4e8ae46b283
While there is work already happening at the compiler layer to lighten this (by not always throwing), we can make things better in the interim by having our own code no throw here, but instead gracefully check for this, and pop up the async stack simply.