Skip to content
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

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

Merged
merged 21 commits into from
May 25, 2024
Merged

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

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
a625300
In progress
CyrusNajmabadi May 24, 2024
45198ea
More ui work
CyrusNajmabadi May 24, 2024
da8522c
Comments
CyrusNajmabadi May 24, 2024
594cf95
in progres
CyrusNajmabadi May 24, 2024
96813b9
IN progress
CyrusNajmabadi May 24, 2024
1dfb46e
IN progress
CyrusNajmabadi May 24, 2024
6b3608c
Spin
CyrusNajmabadi May 24, 2024
3ad1639
Done
CyrusNajmabadi May 24, 2024
1d544fb
Cleanup
CyrusNajmabadi May 24, 2024
4d1998a
Cleanup
CyrusNajmabadi May 24, 2024
ba6cbe9
Add docs
CyrusNajmabadi May 24, 2024
4e01406
Reverse
CyrusNajmabadi May 24, 2024
13b9493
Remove wrapper
CyrusNajmabadi May 24, 2024
5723ae1
Update src/EditorFeatures/Core/Classification/Semantic/AbstractSemant…
CyrusNajmabadi May 24, 2024
5ef1b2a
fix comments
CyrusNajmabadi May 24, 2024
4eed867
use correct token
CyrusNajmabadi May 24, 2024
addb628
Merge branch 'lessMainThreadSwitch' of https://github.com/CyrusNajmab…
CyrusNajmabadi May 24, 2024
2b83bd9
Add docs
CyrusNajmabadi May 24, 2024
0e6342d
Text image helper
CyrusNajmabadi May 24, 2024
e08f41f
use text images
CyrusNajmabadi May 24, 2024
900ed3d
Bring back bt
CyrusNajmabadi May 24, 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
71 changes: 37 additions & 34 deletions ...ore/Classification/Semantic/AbstractSemanticOrEmbeddedClassificationViewTaggerProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,6 @@ internal abstract class AbstractSemanticOrEmbeddedClassificationViewTaggerProvid
private readonly IGlobalOptionService _globalOptions;
private readonly ClassificationType _type;

// 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 sealed override TaggerTextChangeBehavior TextChangeBehavior => TaggerTextChangeBehavior.TrackTextChanges;
protected sealed override ImmutableArray<IOption2> Options { get; } = [SemanticColorizerOptionsStorage.SemanticColorizer];

protected AbstractSemanticOrEmbeddedClassificationViewTaggerProvider(
Expand Down Expand Up @@ -137,8 +134,9 @@ public async Task ProduceTagsAsync(
if (document == null)
return;

var currentSemanticVersion = await document.Project.GetDependentSemanticVersionAsync(cancellationToken).ConfigureAwait(false);
var classified = await TryClassifyContainingMemberSpanAsync(
context, document, spanToTag.SnapshotSpan, classificationService, options, cancellationToken).ConfigureAwait(false);
context, document, spanToTag.SnapshotSpan, classificationService, options, currentSemanticVersion, cancellationToken).ConfigureAwait(false);
Copy link
Member Author

Choose a reason for hiding this comment

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

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).

if (classified)
{
return;
Expand All @@ -147,7 +145,7 @@ public async Task ProduceTagsAsync(
// We weren't able to use our specialized codepaths for semantic classifying.
// Fall back to classifying the full span that was asked for.
await ClassifySpansAsync(
context, document, spanToTag.SnapshotSpan, classificationService, options, cancellationToken).ConfigureAwait(false);
context, document, spanToTag.SnapshotSpan, classificationService, options, currentSemanticVersion, cancellationToken).ConfigureAwait(false);
}

private async Task<bool> TryClassifyContainingMemberSpanAsync(
Expand All @@ -156,39 +154,39 @@ private async Task<bool> TryClassifyContainingMemberSpanAsync(
SnapshotSpan snapshotSpan,
IClassificationService classificationService,
ClassificationOptions options,
VersionStamp currentSemanticVersion,
CancellationToken cancellationToken)
{
var range = context.TextChangeRange;
if (range == null)
{
// There was no text change range, we can't just reclassify a member body.
return false;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

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.


// there was top level edit, check whether that edit updated top level element
if (!document.SupportsSyntaxTree)
return false;

var lastSemanticVersion = (VersionStamp?)context.State;
if (lastSemanticVersion != null)
{
var currentSemanticVersion = await document.Project.GetDependentSemanticVersionAsync(cancellationToken).ConfigureAwait(false);
if (lastSemanticVersion.Value != currentSemanticVersion)
{
// A top level change was made. We can't perform this optimization.
return false;
}
}
// No cached state, so we can't check if the edits were just inside a member.
if (context.State is null)
return false;

var (lastSemanticVersion, lastTextSnapshot) = ((VersionStamp, ITextSnapshot))context.State;

// if a top level change was made. We can't perform this optimization.
if (lastSemanticVersion != currentSemanticVersion)
return false;

var service = document.GetRequiredLanguageService<ISyntaxFactsService>();

// perf optimization. Check whether all edits since the last update has happened within
// a member. If it did, it will find the member that contains the changes and only refresh
// that member. If possible, try to get a speculative binder to make things even cheaper.
// perf optimization. Check whether all edits since the last update has happened within a member. If it did, it
// will find the member that contains the changes and only refresh that member. If possible, try to get a
// speculative binder to make things even cheaper.

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);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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


var root = await document.GetRequiredSyntaxRootAsync(cancellationToken).ConfigureAwait(false);

var changedSpan = new TextSpan(range.Value.Span.Start, range.Value.NewLength);
var member = service.GetContainingMemberDeclaration(root, changedSpan.Start);
if (member == null || !member.FullSpan.Contains(changedSpan))
{
Expand Down Expand Up @@ -221,7 +219,7 @@ private async Task<bool> TryClassifyContainingMemberSpanAsync(

// re-classify only the member we're inside.
await ClassifySpansAsync(
context, document, subSpanToTag, classificationService, options, cancellationToken).ConfigureAwait(false);
context, document, subSpanToTag, classificationService, options, currentSemanticVersion, cancellationToken).ConfigureAwait(false);
return true;
}

Expand All @@ -231,6 +229,7 @@ private async Task ClassifySpansAsync(
SnapshotSpan snapshotSpan,
IClassificationService classificationService,
ClassificationOptions options,
VersionStamp currentSemanticVersion,
CancellationToken cancellationToken)
{
try
Expand All @@ -243,29 +242,33 @@ private async Task ClassifySpansAsync(
// that we preserve that same behavior in OOP if we end up computing the tags there.
options = options with { FrozenPartialSemantics = context.FrozenPartialSemantics };

var span = snapshotSpan.Span;
var snapshot = snapshotSpan.Snapshot;

if (_type == ClassificationType.Semantic)
{
await classificationService.AddSemanticClassificationsAsync(
document, snapshotSpan.Span.ToTextSpan(), options, classifiedSpans, cancellationToken).ConfigureAwait(false);
document, span.ToTextSpan(), options, classifiedSpans, cancellationToken).ConfigureAwait(false);
}
else if (_type == ClassificationType.EmbeddedLanguage)
{
await classificationService.AddEmbeddedLanguageClassificationsAsync(
document, snapshotSpan.Span.ToTextSpan(), options, classifiedSpans, cancellationToken).ConfigureAwait(false);
document, span.ToTextSpan(), options, classifiedSpans, cancellationToken).ConfigureAwait(false);
}
else
{
throw ExceptionUtilities.UnexpectedValue(_type);
}

foreach (var span in classifiedSpans)
context.AddTag(ClassificationUtilities.Convert(_typeMap, snapshotSpan.Snapshot, span));

var version = await document.Project.GetDependentSemanticVersionAsync(cancellationToken).ConfigureAwait(false);
foreach (var classifiedSpan in classifiedSpans)
context.AddTag(ClassificationUtilities.Convert(_typeMap, snapshot, classifiedSpan));

// Let the context know that this was the span we actually tried to tag.
context.SetSpansTagged([snapshotSpan]);
context.State = version;

// Store teh semantic version and snapshot we used to produce these tags. We can use this in the future
// to try to limit what we classify, if all edits were made within a single member.
context.State = (currentSemanticVersion, snapshot);
}
}
catch (Exception e) when (FatalError.ReportAndPropagateUnlessCanceled(e, cancellationToken))
Expand Down
4 changes: 0 additions & 4 deletions src/EditorFeatures/Core/EditAndContinue/ActiveStatementTaggerProvider.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


protected override TaggerDelay EventChangeDelay => TaggerDelay.NearImmediate;

protected override ITaggerEventSource CreateEventSource(ITextView? textView, ITextBuffer subjectBuffer)
Expand Down
57 changes: 57 additions & 0 deletions src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.BufferToTagTree.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Immutable;
using System.Diagnostics.CodeAnalysis;
using System.Runtime.CompilerServices;
using System.Threading;
using Microsoft.CodeAnalysis.Editor.Shared.Tagging;
using Microsoft.VisualStudio.Text;

namespace Microsoft.CodeAnalysis.Editor.Tagging;

internal partial class AbstractAsynchronousTaggerProvider<TTag>
{
private readonly struct BufferToTagTree(ImmutableDictionary<ITextBuffer, TagSpanIntervalTree<TTag>> map)
{
public static readonly BufferToTagTree Empty = new(ImmutableDictionary<ITextBuffer, TagSpanIntervalTree<TTag>>.Empty);

public readonly ImmutableDictionary<ITextBuffer, TagSpanIntervalTree<TTag>> Map = map;

public bool IsDefault => Map is null;

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));
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed this all. back to ImmutableDictionaries.


internal static BufferToTagTree InterlockedCompareExchange(ref BufferToTagTree location, BufferToTagTree value, BufferToTagTree comparand)
=> new(Interlocked.CompareExchange(ref Unsafe.AsRef(in location.Map), value.Map, comparand.Map));

public bool TryGetValue(ITextBuffer textBuffer, [NotNullWhen(true)] out TagSpanIntervalTree<TTag>? tagTree)
=> Map.TryGetValue(textBuffer, out tagTree);

public BufferToTagTree Add(ITextBuffer buffer, TagSpanIntervalTree<TTag> newTagTree)
=> new(Map.Add(buffer, newTagTree));

public BufferToTagTree SetItem(ITextBuffer buffer, TagSpanIntervalTree<TTag> newTagTree)
=> new(Map.SetItem(buffer, newTagTree));

public static bool operator ==(BufferToTagTree left, BufferToTagTree right)
=> left.Map == right.Map;

public static bool operator !=(BufferToTagTree left, BufferToTagTree right)
=> !(left == right);

public override bool Equals([NotNullWhen(true)] object? obj)
=> throw new NotSupportedException();

public override int GetHashCode()
=> throw new NotSupportedException();

public bool ContainsKey(ITextBuffer oldBuffer)
=> Map.ContainsKey(oldBuffer);
}
}
100 changes: 37 additions & 63 deletions src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProvider.TagSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Runtime.CompilerServices;
using System.Threading;
using Microsoft.CodeAnalysis.Editor.Shared.Extensions;
using Microsoft.CodeAnalysis.Editor.Shared.Tagging;
Expand All @@ -27,12 +29,12 @@ internal partial class AbstractAsynchronousTaggerProvider<TTag>
/// tagging infrastructure. It is the coordinator between <see cref="ProduceTagsAsync(TaggerContext{TTag}, CancellationToken)"/>s,
/// <see cref="ITaggerEventSource"/>s, and <see cref="ITagger{T}"/>s.</para>
///
/// <para>The <see cref="TagSource"/> is the type that actually owns the
/// list of cached tags. When an <see cref="ITaggerEventSource"/> says tags need to be recomputed,
/// the tag source starts the computation and calls <see cref="ProduceTagsAsync(TaggerContext{TTag}, CancellationToken)"/> to build
/// the new list of tags. When that's done, the tags are stored in <see cref="CachedTagTrees"/>. The
/// tagger, when asked for tags from the editor, then returns the tags that are stored in
/// <see cref="CachedTagTrees"/></para>
/// <para>The <see cref="TagSource"/> is the type that actually owns the list of cached tags. When an <see
/// cref="ITaggerEventSource"/> says tags need to be recomputed, the tag source starts the computation and calls
/// <see cref="ProduceTagsAsync(TaggerContext{TTag}, CancellationToken)"/> to build the new list of tags. When
/// that's done, the tags are stored in <see cref="_cachedTagTrees_mayChangeFromAnyThread"/>. The tagger, when asked
/// for tags from the editor, then returns the tags that are stored in <see
/// cref="_cachedTagTrees_mayChangeFromAnyThread"/></para>
///
/// <para>There is a one-to-many relationship between <see cref="TagSource"/>s
/// and <see cref="ITagger{T}"/>s. Special cases, like reference highlighting (which processes multiple
Expand Down Expand Up @@ -86,6 +88,20 @@ private sealed partial class TagSource
/// </summary>
private readonly CancellationSeries _nonFrozenComputationCancellationSeries;

/// <summary>
/// The last tag trees that we computed per buffer. Note: this can be read/written from any thread. Because of
/// that, we have to use safe operations to actually read or write it. This includes using looping "compare and
/// swap" algorithms to make sure that it is consistently moved forward no matter which thread is trying to
/// mutate it.
/// </summary>
private BufferToTagTree _cachedTagTrees_mayChangeFromAnyThread = BufferToTagTree.Empty;

#endregion

#region Mutable state. Only accessed from _eventChangeQueue

private object? _state_accessOnlyFromEventChangeQueueCallback;
Copy link
Member Author

Choose a reason for hiding this comment

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

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.


#endregion

#region Fields that can only be accessed from the foreground thread
Expand Down Expand Up @@ -121,13 +137,6 @@ private sealed partial class TagSource

#region Mutable state. Can only be accessed from the foreground thread

/// <summary>
/// accumulated text changes since last tag calculation
/// </summary>
private TextChangeRange? _accumulatedTextChanges_doNotAccessDirectly;
Copy link
Member Author

Choose a reason for hiding this comment

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

removed entirely.

private ImmutableDictionary<ITextBuffer, TagSpanIntervalTree<TTag>> _cachedTagTrees_doNotAccessDirectly = ImmutableDictionary.Create<ITextBuffer, TagSpanIntervalTree<TTag>>();
private object? _state_doNotAccessDirecty;

/// <summary>
/// Keep track of if we are processing the first <see cref="ITagger{T}.GetTags"/> request. If our provider returns
/// <see langword="true"/> for <see cref="AbstractAsynchronousTaggerProvider{TTag}.ComputeInitialTagsSynchronously"/>,
Expand Down Expand Up @@ -202,9 +211,13 @@ public TagSource(
// Create the tagger-specific events that will cause the tagger to refresh.
_eventSource = CreateEventSource();

// any time visibility changes, resume tagging on all taggers. Any non-visible taggers will pause
// themselves immediately afterwards.
_onVisibilityChanged = () => ResumeIfVisible();
// Any time visibility changes try to pause us if we're not visible, or resume us if we are.
_onVisibilityChanged = () =>
{
_dataSource.ThreadingContext.ThrowIfNotOnUIThread();
PauseIfNotVisible();
ResumeIfVisible();
};
Copy link
Member Author

Choose a reason for hiding this comment

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

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.


// Now hook up this tagger to all interesting events.
Connect();
Expand All @@ -225,8 +238,11 @@ void Connect()

_eventSource.Changed += OnEventSourceChanged;

if (_dataSource.TextChangeBehavior.HasFlag(TaggerTextChangeBehavior.TrackTextChanges))
Copy link
Member Author

Choose a reason for hiding this comment

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

removed the top flag.

if (_dataSource.TextChangeBehavior.HasFlag(TaggerTextChangeBehavior.RemoveAllTags) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

_dataSource.TextChangeBehavior

maybe

_dataSource.TextChangeBehavior != TaggerTextChangeBehavior.None

Copy link
Member Author

Choose a reason for hiding this comment

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

can change in followup.

_dataSource.TextChangeBehavior.HasFlag(TaggerTextChangeBehavior.RemoveTagsThatIntersectEdits))
{
_subjectBuffer.Changed += OnSubjectBufferChanged;
}

if (_dataSource.CaretChangeBehavior.HasFlag(TaggerCaretChangeBehavior.RemoveAllTagsOnCaretMoveOutsideOfTag))
{
Expand Down Expand Up @@ -270,8 +286,11 @@ void Disconnect()
_textView.Caret.PositionChanged -= OnCaretPositionChanged;
}

if (_dataSource.TextChangeBehavior.HasFlag(TaggerTextChangeBehavior.TrackTextChanges))
if (_dataSource.TextChangeBehavior.HasFlag(TaggerTextChangeBehavior.RemoveAllTags) ||
_dataSource.TextChangeBehavior.HasFlag(TaggerTextChangeBehavior.RemoveTagsThatIntersectEdits))
{
Comment on lines +289 to +291
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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!

_subjectBuffer.Changed -= OnSubjectBufferChanged;
}

_eventSource.Changed -= OnEventSourceChanged;

Expand Down Expand Up @@ -336,51 +355,6 @@ private ITaggerEventSource CreateEventSource()
return TaggerEventSources.Compose(optionChangedEventSources);
}

private TextChangeRange? AccumulatedTextChanges
{
get
{
_dataSource.ThreadingContext.ThrowIfNotOnUIThread();
return _accumulatedTextChanges_doNotAccessDirectly;
}

set
{
_dataSource.ThreadingContext.ThrowIfNotOnUIThread();
_accumulatedTextChanges_doNotAccessDirectly = value;
}
}

private ImmutableDictionary<ITextBuffer, TagSpanIntervalTree<TTag>> CachedTagTrees
{
get
{
_dataSource.ThreadingContext.ThrowIfNotOnUIThread();
return _cachedTagTrees_doNotAccessDirectly;
}

set
{
_dataSource.ThreadingContext.ThrowIfNotOnUIThread();
_cachedTagTrees_doNotAccessDirectly = value;
}
}

private object? State
{
get
{
_dataSource.ThreadingContext.ThrowIfNotOnUIThread();
return _state_doNotAccessDirecty;
}

set
{
_dataSource.ThreadingContext.ThrowIfNotOnUIThread();
_state_doNotAccessDirecty = value;
}
}

private void RaiseTagsChanged(ITextBuffer buffer, DiffResult difference)
{
_dataSource.ThreadingContext.ThrowIfNotOnUIThread();
Expand Down
Loading
Loading