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.

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

Ensure taggers that compute frozen partial data eventually move to a 'correct' final state. #72878

Merged
merged 36 commits into from
Apr 5, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
db6b65e
Initial work to support frozen partial tagging
CyrusNajmabadi Apr 4, 2024
0dab34b
Update tests
CyrusNajmabadi Apr 4, 2024
209a95c
Add tests
CyrusNajmabadi Apr 4, 2024
1411434
Update tests
CyrusNajmabadi Apr 4, 2024
be9305d
in progress
CyrusNajmabadi Apr 4, 2024
303982a
Cancellation series
CyrusNajmabadi Apr 4, 2024
06116b2
comments
CyrusNajmabadi Apr 4, 2024
b976155
move docs
CyrusNajmabadi Apr 4, 2024
4e3d729
Simplify
CyrusNajmabadi Apr 4, 2024
d869168
ASserts
CyrusNajmabadi Apr 4, 2024
7c8bf35
Update src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProv…
CyrusNajmabadi Apr 4, 2024
3ec3530
Simplify
CyrusNajmabadi Apr 4, 2024
f554737
Merge branch 'frozenPartialTagging' of https://github.com/CyrusNajmab…
CyrusNajmabadi Apr 4, 2024
24c34e5
move helpers
CyrusNajmabadi Apr 4, 2024
3de1764
indentatin
CyrusNajmabadi Apr 4, 2024
84c7aab
REmove type
CyrusNajmabadi Apr 4, 2024
5469dee
Docs
CyrusNajmabadi Apr 4, 2024
766085b
REmove unused usings
CyrusNajmabadi Apr 4, 2024
d9c8454
Consistentcy
CyrusNajmabadi Apr 4, 2024
02bd1ee
lint
CyrusNajmabadi Apr 4, 2024
4db1689
Merge remote-tracking branch 'upstream/main' into frozenPartialTagging
CyrusNajmabadi Apr 5, 2024
54485a1
Use disposal source
CyrusNajmabadi Apr 5, 2024
bb87d24
Switch to any
CyrusNajmabadi Apr 5, 2024
f4a0212
simplify logic
CyrusNajmabadi Apr 5, 2024
ba2fb3a
move option lower, and inline method
CyrusNajmabadi Apr 5, 2024
2717585
do the same for highlighting
CyrusNajmabadi Apr 5, 2024
5ae41b5
voidresult
CyrusNajmabadi Apr 5, 2024
05d0c6f
Update src/EditorFeatures/Core/ReferenceHighlighting/ReferenceHighlig…
CyrusNajmabadi Apr 5, 2024
df8d68a
voidresult
CyrusNajmabadi Apr 5, 2024
492c3fc
Merge branch 'frozenPartialTagging' of https://github.com/CyrusNajmab…
CyrusNajmabadi Apr 5, 2024
f31e40f
Simplify
CyrusNajmabadi Apr 5, 2024
6e8da4c
REvert
CyrusNajmabadi Apr 5, 2024
b78d0a8
revert
CyrusNajmabadi Apr 5, 2024
c58a604
remove redundant check
CyrusNajmabadi Apr 5, 2024
a9b19d3
Docs
CyrusNajmabadi Apr 5, 2024
0e8d413
Update src/EditorFeatures/Core/Tagging/AbstractAsynchronousTaggerProv…
CyrusNajmabadi Apr 5, 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
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ private static async Task<IEnumerable<ITagSpan<BraceHighlightTag>>> ProduceTagsA

var context = new TaggerContext<BraceHighlightTag>(
buffer.CurrentSnapshot.GetRelatedDocumentsWithChanges().FirstOrDefault(),
buffer.CurrentSnapshot, new SnapshotPoint(buffer.CurrentSnapshot, position));
buffer.CurrentSnapshot,
frozenPartialSemantics: false,
new SnapshotPoint(buffer.CurrentSnapshot, position));
await producer.GetTestAccessor().ProduceTagsAsync(context);

return context.TagSpans;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ protected AbstractSemanticOrEmbeddedClassificationViewTaggerProvider(

protected sealed override TaggerDelay EventChangeDelay => TaggerDelay.Short;

/// <summary>
/// We support frozen partial semantics, so we can quickly get classification items without building SG docs. We
/// will still run a tagging pass after the frozen-pass where we run again on non-frozen docs.
/// </summary>
protected sealed override bool SupportsFrozenPartialSemantics => true;

protected sealed override ITaggerEventSource CreateEventSource(ITextView textView, ITextBuffer subjectBuffer)
{
this.ThreadingContext.ThrowIfNotOnUIThread();
Expand Down Expand Up @@ -92,7 +98,19 @@ protected sealed override Task ProduceTagsAsync(
if (isLspSemanticTokensEnabled)
return Task.CompletedTask;

var classificationOptions = _globalOptions.GetClassificationOptions(document.Project.Language);
// Don't block getting classifications on building the full compilation. This may take a significant amount
// of time and can cause a very latency sensitive operation (copying) to block the user while we wait on this
// work to happen.
//
// It's also a better experience to get classifications to the user faster versus waiting a potentially large
// amount of time waiting for all the compilation information to be built. For example, we can classify types
// that we've parsed in other files, or partially loaded from metadata, even if we're still parsing/loading.
// For cross language projects, this also produces semantic classifications more quickly as we do not have to
// wait on skeletons to be built.
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 of this comment.

var classificationOptions = _globalOptions.GetClassificationOptions(document.Project.Language) with
{
FrozenPartialSemantics = context.FrozenPartialSemantics,
};
return ClassificationUtilities.ProduceTagsAsync(
context, spanToTag, classificationService, _typeMap, classificationOptions, _type, cancellationToken);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,19 +49,6 @@ public static async Task ProduceTagsAsync(
if (document == null)
return;

// Don't block getting classifications on building the full compilation. This may take a significant amount
// of time and can cause a very latency sensitive operation (copying) to block the user while we wait on this
// work to happen.
//
// It's also a better experience to get classifications to the user faster versus waiting a potentially
// large amount of time waiting for all the compilation information to be built. For example, we can
// classify types that we've parsed in other files, or partially loaded from metadata, even if we're still
// parsing/loading. For cross language projects, this also produces semantic classifications more quickly
// as we do not have to wait on skeletons to be built.

document = document.WithFrozenPartialSemantics(cancellationToken);
options = options with { ForceFrozenPartialSemanticsForCrossProcessOperations = true };

var classified = await TryClassifyContainingMemberSpanAsync(
context, document, spanToTag.SnapshotSpan, classificationService, typeMap, options, type, cancellationToken).ConfigureAwait(false);
if (classified)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ internal sealed partial class ReferenceHighlightingViewTaggerProvider(

protected override TaggerDelay EventChangeDelay => TaggerDelay.Medium;

/// <summary>
/// We support frozen partial semantics, so we can quickly get reference highlights without building SG docs. We
/// will still run a tagging pass after the frozen-pass where we run again on non-frozen docs.
/// </summary>
protected override bool SupportsFrozenPartialSemantics => true;

protected override ITaggerEventSource CreateEventSource(ITextView textView, ITextBuffer subjectBuffer)
{
// Note: we don't listen for OnTextChanged. Text changes to this buffer will get
Expand Down Expand Up @@ -129,7 +135,10 @@ protected override Task ProduceTagsAsync(
}

// Otherwise, we need to go produce all tags.
var options = _globalOptions.GetHighlightingOptions(document.Project.Language);
var options = _globalOptions.GetHighlightingOptions(document.Project.Language) with
{
FrozenPartialSemantics = context.FrozenPartialSemantics,
};
return ProduceTagsAsync(context, caretPosition, document, options, cancellationToken);
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ private sealed partial class TagSource
/// up event change notifications and only dispatch one recomputation every <see cref="EventChangeDelay"/>
/// to actually produce the latest set of tags.
/// </summary>
private readonly AsyncBatchingWorkQueue<bool, VoidResult> _eventChangeQueue;
private readonly AsyncBatchingWorkQueue<(bool highPriority, bool frozenPartialSemantics)> _eventChangeQueue;

#endregion

Expand Down Expand Up @@ -164,10 +164,10 @@ public TagSource(
//
// PERF: Use AsyncBatchingWorkQueue<bool, VoidResult> instead of AsyncBatchingWorkQueue<bool> because
// the latter has an async state machine that rethrows a very common cancellation exception.
_eventChangeQueue = new AsyncBatchingWorkQueue<bool, VoidResult>(
_eventChangeQueue = new AsyncBatchingWorkQueue<(bool highPriority, bool frozenPartialSemantics)>(
dataSource.EventChangeDelay.ComputeTimeDelay(),
ProcessEventChangeAsync,
EqualityComparer<bool>.Default,
EqualityComparer<(bool highPriority, bool frozenPartialSemantics)>.Default,
asyncListener,
_disposalTokenSource.Token);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -168,13 +168,17 @@ private void OnEventSourceChanged(object? _1, TaggerEventArgs _2)
=> EnqueueWork(highPriority: false);

private void EnqueueWork(bool highPriority)
=> _eventChangeQueue.AddWork(highPriority, _dataSource.CancelOnNewWork);
=> EnqueueWork(highPriority, _dataSource.SupportsFrozenPartialSemantics);

private ValueTask<VoidResult> ProcessEventChangeAsync(ImmutableSegmentedList<bool> changes, CancellationToken cancellationToken)
private void EnqueueWork(bool highPriority, bool frozenPartialSemantics)
Copy link
Member Author

Choose a reason for hiding this comment

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

@sharwell for eyes on thsi threading logic.

=> _eventChangeQueue.AddWork((highPriority, frozenPartialSemantics), _dataSource.CancelOnNewWork);

private async ValueTask ProcessEventChangeAsync(ImmutableSegmentedList<(bool highPriority, bool frozenPartialSemantics)> changes, CancellationToken cancellationToken)
{
// If any of the requests was high priority, then compute at that speed.
var highPriority = changes.Contains(true);
return new ValueTask<VoidResult>(RecomputeTagsAsync(highPriority, cancellationToken));
var highPriority = changes.Contains(x => x.highPriority);
var frozenPartialSemantics = changes.Contains(t => t.frozenPartialSemantics);
Copy link
Member

Choose a reason for hiding this comment

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

);

if we have one request of (highPriority: true, frozenPartialSemantics: false) and another (highPriority: false, frozenPartialSemantics: true), then we'd recompute with a mix setting of (true, true), is that intended? If so, pls add comment to clearify that. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

will add comment.

await RecomputeTagsAsync(highPriority, frozenPartialSemantics, cancellationToken).ConfigureAwait(false);
}

/// <summary>
Expand All @@ -191,11 +195,14 @@ private ValueTask<VoidResult> ProcessEventChangeAsync(ImmutableSegmentedList<boo
/// <param name="highPriority">
/// If this tagging request should be processed as quickly as possible with no extra delays added for it.
/// </param>
private async Task<VoidResult> RecomputeTagsAsync(bool highPriority, CancellationToken cancellationToken)
private async Task RecomputeTagsAsync(
bool highPriority,
bool frozenPartialSemantics,
CancellationToken cancellationToken)
{
await _dataSource.ThreadingContext.JoinableTaskFactory.SwitchToMainThreadAsync(cancellationToken).NoThrowAwaitable();
if (cancellationToken.IsCancellationRequested)
return default;
return;

// if we're tagging documents that are not visible, then introduce a long delay so that we avoid
// consuming machine resources on work the user isn't likely to see. ConfigureAwait(true) so that if
Expand All @@ -211,12 +218,12 @@ await _visibilityTracker.DelayWhileNonVisibleAsync(
_dataSource.ThreadingContext, _dataSource.AsyncListener, _subjectBuffer, DelayTimeSpan.NonFocus, cancellationToken).NoThrowAwaitable(captureContext: true);

if (cancellationToken.IsCancellationRequested)
return default;
return;
}

_dataSource.ThreadingContext.ThrowIfNotOnUIThread();
if (cancellationToken.IsCancellationRequested)
return default;
return;

using (Logger.LogBlock(FunctionId.Tagger_TagSource_RecomputeTags, cancellationToken))
{
Expand All @@ -234,15 +241,22 @@ await _visibilityTracker.DelayWhileNonVisibleAsync(
await TaskScheduler.Default;

if (cancellationToken.IsCancellationRequested)
return default;
return;

if (frozenPartialSemantics)
{
spansToTag = spansToTag.SelectAsArray(ds => new DocumentSnapshotSpan(
ds.Document?.WithFrozenPartialSemantics(cancellationToken),
ds.SnapshotSpan));
}

// Create a context to store pass the information along and collect the results.
var context = new TaggerContext<TTag>(
oldState, spansToTag, caretPosition, textChangeRange, oldTagTrees);
oldState, frozenPartialSemantics, spansToTag, caretPosition, textChangeRange, oldTagTrees);
await ProduceTagsAsync(context, cancellationToken).ConfigureAwait(false);

if (cancellationToken.IsCancellationRequested)
return default;
return;

// Process the result to determine what changed.
var newTagTrees = ComputeNewTagTrees(oldTagTrees, context);
Expand All @@ -251,7 +265,7 @@ await _visibilityTracker.DelayWhileNonVisibleAsync(
// 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;
return;

// 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.
Expand All @@ -275,9 +289,14 @@ await _visibilityTracker.DelayWhileNonVisibleAsync(
// Once we've computed tags, pause ourselves if we're no longer visible. That way we don't consume any
// machine resources that the user won't even notice.
PauseIfNotVisible();
}

return default;
// If we were computing with frozen partial semantics here, enqueue work to compute *without* frozen
// partial snapshots so we move to accurate results shortly. Note: when the queue goes to process this
// message, if it sees any other events asking for frozen-partial-semantics, it will process in that
// mode again, kicking the can down the road to finally end with non-frozen-partial computation
if (frozenPartialSemantics)
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
this.EnqueueWork(highPriority, frozenPartialSemantics: false);
}
}

private ImmutableArray<DocumentSnapshotSpan> GetSpansAndDocumentsToTag()
Expand Down Expand Up @@ -560,8 +579,8 @@ private DiffResult ComputeDifference(
!this.CachedTagTrees.TryGetValue(buffer, out _))
{
// Compute this as a high priority work item to have the lease amount of blocking as possible.
_dataSource.ThreadingContext.JoinableTaskFactory.Run(() =>
this.RecomputeTagsAsync(highPriority: true, _disposalTokenSource.Token));
_dataSource.ThreadingContext.JoinableTaskFactory.Run(async () =>
await this.RecomputeTagsAsync(highPriority: true, _dataSource.SupportsFrozenPartialSemantics, _disposalTokenSource.Token).ConfigureAwait(false));
}

_firstTagsRequest = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,11 @@ internal abstract partial class AbstractAsynchronousTaggerProvider<TTag> where T
/// </summary>
protected virtual bool CancelOnNewWork { get; }

/// <summary>
/// Whether or not this tagger would like to use frozen-partial snapshots to compute tags. TODO: doc more before submitting.
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
/// </summary>
protected virtual bool SupportsFrozenPartialSemantics { get; }
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved

protected virtual void BeforeTagsChanged(ITextSnapshot snapshot)
{
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,9 @@ protected override TaggerTextChangeBehavior TextChangeBehavior
protected override SpanTrackingMode SpanTrackingMode
=> _callback.SpanTrackingMode;

protected override bool SupportsFrozenPartialSemantics
=> _callback.SupportsFrozenPartialSemantics;

protected override ITaggerEventSource CreateEventSource(ITextView textView, ITextBuffer subjectBuffer)
=> _callback.CreateEventSource(textView, subjectBuffer);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,9 @@ SingleViewportTaggerProvider CreateSingleViewportTaggerProvider(ViewPortToTag vi
/// <inheritdoc cref="AbstractAsynchronousTaggerProvider{TTag}.SpanTrackingMode"/>
protected virtual SpanTrackingMode SpanTrackingMode => SpanTrackingMode.EdgeExclusive;

/// <inheritdoc cref="AbstractAsynchronousTaggerProvider{TTag}.SupportsFrozenPartialSemantics"/>
protected virtual bool SupportsFrozenPartialSemantics { get; }

/// <summary>
/// Indicates whether a tagger should be created for this text view and buffer.
/// </summary>
Expand Down
20 changes: 13 additions & 7 deletions src/EditorFeatures/Core/Tagging/TaggerContext.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,14 @@

#nullable disable

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Threading;
using Microsoft.CodeAnalysis.Collections;
using Microsoft.CodeAnalysis.Editor.Shared.Tagging;
using Microsoft.CodeAnalysis.Text;
using Microsoft.CodeAnalysis.Text.Shared.Extensions;
using Microsoft.VisualStudio.Text;
using Microsoft.VisualStudio.Text.Tagging;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.Editor.Tagging;

Expand All @@ -26,6 +22,7 @@ internal class TaggerContext<TTag> where TTag : ITag
internal ImmutableArray<SnapshotSpan> _spansTagged;
public readonly SegmentedList<ITagSpan<TTag>> TagSpans = [];

public bool FrozenPartialSemantics { get; }
public ImmutableArray<DocumentSnapshotSpan> SpansToTag { get; }
public SnapshotPoint? CaretPosition { get; }

Expand All @@ -48,22 +45,31 @@ internal class TaggerContext<TTag> where TTag : ITag

// For testing only.
internal TaggerContext(
Document document, ITextSnapshot snapshot,
Document document,
ITextSnapshot snapshot,
bool frozenPartialSemantics,
SnapshotPoint? caretPosition = null,
TextChangeRange? textChangeRange = null)
: this(state: null, [new DocumentSnapshotSpan(document, snapshot.GetFullSpan())],
caretPosition, textChangeRange, existingTags: null)
: this(
state: null,
frozenPartialSemantics,
[new DocumentSnapshotSpan(document, snapshot.GetFullSpan())],
caretPosition,
textChangeRange,
existingTags: null)
{
}

internal TaggerContext(
object state,
bool frozenPartialSemantics,
ImmutableArray<DocumentSnapshotSpan> spansToTag,
SnapshotPoint? caretPosition,
TextChangeRange? textChangeRange,
ImmutableDictionary<ITextBuffer, TagSpanIntervalTree<TTag>> existingTags)
{
this.State = state;
this.FrozenPartialSemantics = frozenPartialSemantics;
this.SpansToTag = spansToTag;
this.CaretPosition = caretPosition;
this.TextChangeRange = textChangeRange;
Expand Down
4 changes: 3 additions & 1 deletion src/EditorFeatures/Test/Options/GlobalOptionsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Diagnostics.Analyzers.NamingStyles;
using Microsoft.CodeAnalysis.DocumentationComments;
using Microsoft.CodeAnalysis.DocumentHighlighting;
using Microsoft.CodeAnalysis.Editor;
using Microsoft.CodeAnalysis.Editor.UnitTests;
using Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces;
Expand Down Expand Up @@ -165,7 +166,8 @@ private static bool IsStoredInGlobalOptions(PropertyInfo property, string? langu
property.DeclaringType == typeof(AddImportPlacementOptions) && property.Name == nameof(AddImportPlacementOptions.UsingDirectivePlacement) && language == LanguageNames.VisualBasic ||
property.DeclaringType == typeof(DocumentFormattingOptions) && property.Name == nameof(DocumentFormattingOptions.FileHeaderTemplate) ||
property.DeclaringType == typeof(DocumentFormattingOptions) && property.Name == nameof(DocumentFormattingOptions.InsertFinalNewLine) ||
property.DeclaringType == typeof(ClassificationOptions) && property.Name == nameof(ClassificationOptions.ForceFrozenPartialSemanticsForCrossProcessOperations) ||
property.DeclaringType == typeof(ClassificationOptions) && property.Name == nameof(ClassificationOptions.FrozenPartialSemantics) ||
property.DeclaringType == typeof(HighlightingOptions) && property.Name == nameof(HighlightingOptions.FrozenPartialSemantics) ||
property.DeclaringType == typeof(BlockStructureOptions) && property.Name == nameof(BlockStructureOptions.IsMetadataAsSource));

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ private static async Task<List<IContainerStructureTag>> GetTagsFromWorkspaceAsyn
var provider = workspace.ExportProvider.GetExportedValue<AbstractStructureTaggerProvider>();

var document = workspace.CurrentSolution.GetDocument(hostdoc.Id);
var context = new TaggerContext<IContainerStructureTag>(document, view.TextSnapshot);
var context = new TaggerContext<IContainerStructureTag>(document, view.TextSnapshot, frozenPartialSemantics: false);
await provider.GetTestAccessor().ProduceTagsAsync(context);

return context.TagSpans.Select(x => x.Tag).OrderBy(t => t.OutliningSpan.Value.Start).ToList();
Expand Down
Loading
Loading