Update tagging to avoid jumping back to the UI thread when finished. #73699

CyrusNajmabadi · 2024-05-24T19:48:07Z

This takes us from two UI jumps per ProduceTagsAsync call to just one.

CyrusNajmabadi · 2024-05-24T19:48:42Z

...s/Core/Classification/Semantic/AbstractSemanticOrEmbeddedClassificationViewTaggerProvider.cs

        var classified = await TryClassifyContainingMemberSpanAsync(
-            context, document, spanToTag.SnapshotSpan, classificationService, options, cancellationToken).ConfigureAwait(false);
+            context, document, spanToTag.SnapshotSpan, classificationService, options, currentSemanticVersion, cancellationToken).ConfigureAwait(false);


i removed the text-change-tracking that tagging did (which required the UI thread), and made it something only the SemanticClassifier does (since it is the only feature that needs it).

CyrusNajmabadi · 2024-05-24T19:49:27Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource.cs

@@ -88,6 +88,12 @@ private sealed partial class TagSource

        #endregion

+        #region Mutable state.  Only accessed from _eventChangeQueue
+
+        private object? _state_accessOnlyFromEventChangeQueueCallback;


This is a move/rename. We used to require this have UI thread affinity. But there's no need for that. It's only read/written from the _eventChangeQueue callbacks, so it is safe to have no jumps for this.

CyrusNajmabadi · 2024-05-24T19:49:34Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource.cs

@@ -124,9 +130,7 @@ private sealed partial class TagSource
        /// <summary>
        /// accumulated text changes since last tag calculation
        /// </summary>
-        private TextChangeRange? _accumulatedTextChanges_doNotAccessDirectly;


removed entirely.

CyrusNajmabadi · 2024-05-24T19:50:19Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource.cs

+                _dataSource.ThreadingContext.ThrowIfNotOnUIThread();
+                PauseIfNotVisible();
+                ResumeIfVisible();
+            };


previously, we would jump back to the UI thread after reporting tags to pause ourselves. Now, we just pause/unpause based on the events we get from the visibility service. This matches what we just did in anvbars.

CyrusNajmabadi · 2024-05-24T19:50:42Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

-                // Then switch back to the UI thread to update our state and kick off the work to notify the editor.
-                await _dataSource.ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken).NoThrowAwaitable();
-                if (cancellationToken.IsCancellationRequested)
-                    return default;


this is hte main win. we're not switching back to the ui thread when done.

CyrusNajmabadi · 2024-05-24T19:51:05Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

-                await _dataSource.ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken).NoThrowAwaitable();
-                if (cancellationToken.IsCancellationRequested)
-                    return default;
-
                // Once we assign our state, we're uncancellable.  We must report the changed information
                // to the editor.  The only case where it's ok not to is if the tagger itself is disposed.
                cancellationToken = CancellationToken.None;

                this.CachedTagTrees = newTagTrees;


WIP: this is an immutable dictionary. need to ensure that this is safe for all the reading threads.

CyrusNajmabadi · 2024-05-24T20:40:29Z

...s/Core/Classification/Semantic/AbstractSemanticOrEmbeddedClassificationViewTaggerProvider.cs

-        {
-            // There was no text change range, we can't just reclassify a member body.
-            return false;
-        }


instead of having this range be tracked by the context, and passed in. we can fairly trivially compute it if we store the previous snapshot we computed against, and determine the change ranges between it and the current snapshot we're classifying.

CyrusNajmabadi · 2024-05-24T20:42:12Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

-            var oldTagTrees = this.CachedTagTrees;
-            this.CachedTagTrees = ImmutableDictionary<ITextBuffer, TagSpanIntervalTree<TTag>>.Empty;
+            var oldTagTrees = Interlocked.Exchange(
+                ref _cachedTagTrees_mayChangeFromAnyThread, ImmutableDictionary<ITextBuffer, TagSpanIntervalTree<TTag>>.Empty);


because _cachedTagTrees may be mutated from several different locations (and threads) we have to switch to save ways of doing that atomically.

CyrusNajmabadi · 2024-05-24T20:43:34Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

-                return;
+            // Everything we're passing in here is synchronous.  So we can assert that this must complete synchronously
+            // as well.
+            var (oldTagTrees, newTagTrees) = CompareAndSwapTagTreesAsync(


core helper is this CompareAndSwapTagTreesAsync function. It performs the normal compare-and-swap algorithm, reading teh current value of _cachedTagTrees, calling a user-provided function with it, and attempting to then update that field with the new result as long as it didn't change in between. If it did change, it spins until it finally wins and then returns the before/after state.

CyrusNajmabadi · 2024-05-24T20:43:45Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

+            var (oldTagTrees, newTagTrees) = CompareAndSwapTagTreesAsync(
+                oldTagTrees =>
+                {
+                    if (oldTagTrees.TryGetValue(buffer, out var treeForBuffer))


inverted these checks for clarity.

CyrusNajmabadi · 2024-05-24T20:44:31Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

            }
        }

+        private async Task<(ImmutableDictionary<ITextBuffer, TagSpanIntervalTree<TTag>> oldTagTrees, ImmutableDictionary<ITextBuffer, TagSpanIntervalTree<TTag>> newTagTrees)>


i really wish i could have an alias for ImmutableDictionary<ITextBuffer, TagSpanIntervalTree<TTag>>, but it references TTag which is a generic type parameter of this type.

CyrusNajmabadi · 2024-05-24T21:04:24Z

src/EditorFeatures/Core/EditAndContinue/ActiveStatementTaggerProvider.cs

@@ -40,10 +40,6 @@ internal partial class ActiveStatementTaggerProvider(
    [Import(AllowDefault = true)] ITextBufferVisibilityTracker? visibilityTracker,
    IAsynchronousOperationListenerProvider listenerProvider) : AsynchronousTaggerProvider<ITextMarkerTag>(threadingContext, globalOptions, visibilityTracker, listenerProvider.GetListener(FeatureAttribute.Classification))
 {
-    // We want to track text changes so that we can try to only reclassify a method body if
-    // all edits were contained within one.
-    protected override TaggerTextChangeBehavior TextChangeBehavior => TaggerTextChangeBehavior.TrackTextChanges;


this was never used. this asked the tagger infrastructure to track this info. but then it was never used in this tagger.

Note: it's clear that this was just a copy/paste from the semantic-classifier

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.BufferToTagTree.cs

CyrusNajmabadi · 2024-05-24T21:07:08Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource.cs

@@ -225,8 +235,11 @@ void Connect()

                _eventSource.Changed += OnEventSourceChanged;

-                if (_dataSource.TextChangeBehavior.HasFlag(TaggerTextChangeBehavior.TrackTextChanges))


removed the top flag.

CyrusNajmabadi · 2024-05-24T21:10:00Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

            }
+
+            return default;
        }


core CompareAndSwap helper.

CyrusNajmabadi · 2024-05-24T21:10:27Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

            bool highPriority,
            bool frozenPartialSemantics,
+            bool calledFromJtfRun,


new addition. want to make sure we dont' do unnecessary hops when in a JTF call.

CyrusNajmabadi · 2024-05-24T21:10:40Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

            CancellationToken cancellationToken)
        {
+            // Jump to the main thread, as we have to grab the spans to tag and the caret point.
            await _dataSource.ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken).NoThrowAwaitable();


existing UI jump that we need to preserve.

CyrusNajmabadi · 2024-05-24T22:18:45Z

Note this is a complimentary PR to #67259. That pr makes it so that we can batch as many taggers together and have them all execute their UI work at the same time.

...s/Core/Classification/Semantic/AbstractSemanticOrEmbeddedClassificationViewTaggerProvider.cs

src/EditorFeatures/Core/Tagging/TaggerTextChangeBehavior.cs

jasonmalinowski · 2024-05-24T21:39:03Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource.cs

+                if (_dataSource.TextChangeBehavior.HasFlag(TaggerTextChangeBehavior.RemoveAllTags) ||
+                    _dataSource.TextChangeBehavior.HasFlag(TaggerTextChangeBehavior.RemoveTagsThatIntersectEdits))
+                {


Is it easier to skip this condition and just always do the delegate remove? That way there's no risk of the conditions getting out of sync.

i wasn't 100% certain the semantics of dleegate removal. and i was a tiny bit concerned that might introduce more chance of a problem... i'd like to keep this way for now if htat's ok!

jasonmalinowski · 2024-05-24T21:47:16Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

+                // If we're being called from within a blocking JTF.Run call, we don't want to switch to the background
+                // if we can avoid it.
+                if (!calledFromJtfRun)
+                    await TaskScheduler.Default;


You can also do this:

roslyn/src/VisualStudio/Core/Def/ProjectSystem/VisualStudioProjectFactory.cs

Lines 80 to 87 in 14a96f9

// The rest of this method can be ran off the UI thread. We'll only switch though if the UI thread isn't already blocked -- the legacy project

// system creates project synchronously, and during solution load we've seen traces where the thread pool is sufficiently saturated that this

// switch can't be completed quickly. For the rest of this method, we won't use ConfigureAwait(false) since we're expecting VS threading

// rules to apply.

if (!_threadingContext.JoinableTaskContext.IsMainThreadBlocked())

{

await TaskScheduler.Default;

}

jasonmalinowski · 2024-05-24T21:55:23Z

...s/Core/Classification/Semantic/AbstractSemanticOrEmbeddedClassificationViewTaggerProvider.cs

+        var lastSourceText = lastTextSnapshot.AsText();
+        var currentSourceText = snapshotSpan.Snapshot.AsText();
+
+        var textChangeRanges = currentSourceText.GetChangeRanges(lastSourceText);
+        var collapsedRange = TextChangeRange.Collapse(textChangeRanges);
+
+        var changedSpan = new TextSpan(collapsedRange.Span.Start, collapsedRange.NewLength);


Do we only need the span in the end? Because we can be a bit friendlier here on costs I think. That GetChangeRanges call is only looking at the ITextVersions I think:

roslyn/src/EditorFeatures/Text/Extensions.SnapshotSourceText.cs

Line 384 in 14a96f9

INormalizedTextChangeCollection? changes = null;

If we extract out that then the state could just be the snapshot version.

yup. will do this. intiially it seemed complex. but it should be ok.

jasonmalinowski · 2024-05-24T22:07:39Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.BufferToTagTree.cs

+        public TagSpanIntervalTree<TTag> this[ITextBuffer buffer] => Map[buffer];
+
+        internal static BufferToTagTree InterlockedExchange(ref BufferToTagTree location, BufferToTagTree value)
+            => new(Interlocked.Exchange(ref Unsafe.AsRef(in location.Map), value.Map));


Is this just more sane to have the struct use a mutable field and then you don't need this directly? Or can dispense of the Unsafe.AsRef? I normally say that a mutable struct is terrible, but this is already effectively mutable so maybe it doesn't really matter.

removed this all. back to ImmutableDictionaries.

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.BufferToTagTree.cs

jasonmalinowski · 2024-05-24T22:25:30Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

+                            var newTagTree = new TagSpanIntervalTree<TTag>(
+                                buffer,
+                                treeForBuffer.SpanTrackingMode,
+                                allTags.Except(tagsToRemove, comparer: this));


Unrelated to your change but I wonder if we can/should make this processing cheaper. It seems we enumerate the tree twice and allocate that intermediate list and then create a whole new tree from scratch. I assume there's a better algorithm.

(assuming this even appears in a trace)

haven't seen it yet. we likely can optimize more here though.

jasonmalinowski · 2024-05-24T22:29:38Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

+                _dataSource.ThreadingContext.ThrowIfNotOnUIThread();
+                if (cancellationToken.IsCancellationRequested)
+                    return default;
+
                // Make a copy of all the data we need while we're on the foreground.  Then switch to a threadpool
                // thread to do the computation. Finally, once new tags have been computed, then we update our state
                // again on the foreground.
                var spansToTag = GetSpansAndDocumentsToTag();
                var caretPosition = _dataSource.GetCaretPoint(_textView, _subjectBuffer);


I wonder how hard it would be to also allow the editor to make this free-threaded.

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

…icOrEmbeddedClassificationViewTaggerProvider.cs Co-authored-by: Jason Malinowski <[email protected]>

…adi/roslyn into lessMainThreadSwitch

ToddGrun · 2024-05-24T23:51:32Z

src/EditorFeatures/Core/Tagging/TaggerTextChangeBehavior.cs

    /// </summary>
-    TrackTextChanges = 1 << 0,
+    RemoveTagsThatIntersectEdits = 1 << 1,


1 << 1

Intentionally starting at 2?

No. Can fix later :-)

ToddGrun · 2024-05-25T00:01:16Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource.cs

@@ -225,8 +238,11 @@ void Connect()

                _eventSource.Changed += OnEventSourceChanged;

-                if (_dataSource.TextChangeBehavior.HasFlag(TaggerTextChangeBehavior.TrackTextChanges))
+                if (_dataSource.TextChangeBehavior.HasFlag(TaggerTextChangeBehavior.RemoveAllTags) ||


_dataSource.TextChangeBehavior

maybe

_dataSource.TextChangeBehavior != TaggerTextChangeBehavior.None

can change in followup.

ToddGrun · 2024-05-25T00:27:22Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

+
+                // Compute the new tag trees, based on what the current tag trees are.  Intentionally CA(true) here so
+                // we stay on the UI thread if we're in a JTF blocking call.
+                var newTagTrees = await callback(oldTagTrees).ConfigureAwait(true);


ConfigureAwait(true);

Can you walk me through this a bit, going off the common path with ConfigureAwait always confuses me.

Specifically, the case I'm curious about is when we enter this method on the main thread and the callback switches to a different thread to do it's work.

With ConfigureAwait(true), we get switched back to the main thread upon the callback return, right? Whereas, if ConfigureAwait(false), we stay on whatever thread the work completed on (or is always a threadpool thread)?

If that's so, wouldn't that be an extra switch to main thread, perhaps unnecessarilly in the case where the CompareExchange was successful? Or does the caller of this method expect to be on the main thread upon it's return?

If that's so, wouldn't that be an extra switch to main thread, perhaps unnecessarilly in the case where the CompareExchange was successful? Or does the caller of this method expect to be on the main thread upon it's return?

The caller can be in one of two modes.

Normal threadpool/bg tagging. In this case, CA(true) is fine, as we come back to that context and continue i nthe bg.

blocking JTF call waiting for tags, for certain blocking editor APis (specifically, code-folding). In this case, we want to return to he ui thread whenever we can since that thread is already blocking on us. thsi allows that true thread to contribute progress to this work, instead of it just sitting there blocked, waiting on teh theradpool to finish work.

ToddGrun · 2024-05-25T00:36:12Z

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs

-                await TaskScheduler.Default;
+                // If we're being called from within a blocking JTF.Run call, we don't want to switch to the background
+                // if we can avoid it.
+                if (!calledFromJtfRun)


calledFromJtfRun

I don't suppose the highPriority flag could be used for this check instead?

no. The first call to get tags is also high-pri.

ToddGrun · 2024-05-25T00:56:24Z

src/EditorFeatures/Text/ITextImageHelpers.cs

+            {
+                if (changes != null)
+                {
+                    // Oops - more than one "textual" change between these snapshots, bail and try to find smallest changes span


// Oops - more than one "textual" change between these snapshots, bail and try to find smallest changes span

I know this comment isn't new in this PR, but I was trying to understand this code, and the "oops" part of this is confusing. I would think this would commonly happen if the versions sent in had some intermediary versions between.

Is it intended to instead call the GetChangeRanges method that accepts the forward parameter if you might have versions that aren't adjacent?

@jasonmalinowski may know moer her.e

CyrusNajmabadi added 3 commits May 24, 2024 12:23

In progress

a625300

More ui work

45198ea

Comments

da8522c

dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels May 24, 2024

CyrusNajmabadi commented May 24, 2024

View reviewed changes

CyrusNajmabadi added 5 commits May 24, 2024 12:58

in progres

594cf95

IN progress

96813b9

IN progress

1dfb46e

Spin

6b3608c

Done

3ad1639

CyrusNajmabadi commented May 24, 2024

View reviewed changes

CyrusNajmabadi added 2 commits May 24, 2024 14:03

Cleanup

1d544fb

Cleanup

4d1998a

CyrusNajmabadi commented May 24, 2024

View reviewed changes

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.BufferToTagTree.cs Outdated Show resolved Hide resolved

Add docs

ba6cbe9

CyrusNajmabadi commented May 24, 2024

View reviewed changes

jasonmalinowski approved these changes May 24, 2024

View reviewed changes

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs Outdated Show resolved Hide resolved

src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource_ProduceTags.cs Outdated Show resolved Hide resolved

CyrusNajmabadi and others added 8 commits May 24, 2024 15:43

Remove wrapper

13b9493

Update src/EditorFeatures/Core/Classification/Semantic/AbstractSemant…

5723ae1

…icOrEmbeddedClassificationViewTaggerProvider.cs Co-authored-by: Jason Malinowski <[email protected]>

fix comments

5ef1b2a

use correct token

4eed867

Merge branch 'lessMainThreadSwitch' of https://github.com/CyrusNajmab…

addb628

…adi/roslyn into lessMainThreadSwitch

Add docs

2b83bd9

Text image helper

0e6342d

use text images

e08f41f

CyrusNajmabadi requested a review from jasonmalinowski May 24, 2024 23:16

Bring back bt

900ed3d

CyrusNajmabadi enabled auto-merge May 24, 2024 23:27

ToddGrun reviewed May 24, 2024

View reviewed changes

ToddGrun reviewed May 25, 2024

View reviewed changes

build-analysis bot mentioned this pull request May 25, 2024

The active test run was aborted. Reason: Test host process crashed dotnet/dnceng#451

Open

3 tasks

CyrusNajmabadi merged commit 6c9936c into dotnet:main May 25, 2024
25 checks passed

dotnet-policy-service bot added this to the Next milestone May 25, 2024

CyrusNajmabadi deleted the lessMainThreadSwitch branch May 25, 2024 01:57

Cosifne modified the milestones: Next, 17.11 P2 May 28, 2024

This was referenced Jun 4, 2024

[Automated] PRs inserted in VS build main-35004.16 #73835

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35004.137 #73853

Closed

[Automated] PRs inserted in VS build feature.debugger.inlineValues-35005.76 #73866

Closed

dotnet-bot mentioned this pull request Jun 27, 2024

[Automated] PRs inserted in VS build feature.dotnetVS-35026.97 #74176

Closed

dibarbet mentioned this pull request Jul 25, 2024

Find-All-References results incomplete and unstable dotnet/vscode-csharp#7391

Closed

		@@ -225,8 +235,11 @@ void Connect()

		_eventSource.Changed += OnEventSourceChanged;

		if (_dataSource.TextChangeBehavior.HasFlag(TaggerTextChangeBehavior.TrackTextChanges))

	// The rest of this method can be ran off the UI thread. We'll only switch though if the UI thread isn't already blocked -- the legacy project
	// system creates project synchronously, and during solution load we've seen traces where the thread pool is sufficiently saturated that this
	// switch can't be completed quickly. For the rest of this method, we won't use ConfigureAwait(false) since we're expecting VS threading
	// rules to apply.
	if (!_threadingContext.JoinableTaskContext.IsMainThreadBlocked())
	{
	await TaskScheduler.Default;
	}

Update tagging to avoid jumping back to the UI thread when finished. #73699

Update tagging to avoid jumping back to the UI thread when finished. #73699

Conversation

CyrusNajmabadi commented May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented May 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyrusNajmabadi commented May 24, 2024 •

edited

Loading