-
Notifications
You must be signed in to change notification settings - Fork 4k
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
made diagnostic tags to be removed faster and inserted slower. #8102
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,7 +75,7 @@ private sealed partial class TagSource : ForegroundThreadAffinitizedObject | |
|
||
#endregion | ||
|
||
public event Action<ICollection<KeyValuePair<ITextBuffer, NormalizedSnapshotSpanCollection>>> TagsChangedForBuffer; | ||
public event Action<ICollection<KeyValuePair<ITextBuffer, DiffResult>>> TagsChangedForBuffer; | ||
|
||
public event EventHandler Paused; | ||
public event EventHandler Resumed; | ||
|
@@ -112,6 +112,9 @@ public TagSource( | |
RecalculateTagsOnChanged(new TaggerEventArgs(TaggerDelay.Short)); | ||
} | ||
|
||
public TaggerDelay NewTagsNotificationDelay => _dataSource.NewTagsNotificationDelay; | ||
public TaggerDelay OldTagNotificationDelay => _dataSource.OldTagNotificationDelay; | ||
|
||
private ITaggerEventSource CreateEventSource() | ||
{ | ||
var eventSource = _dataSource.CreateEventSource(_textViewOpt, _subjectBuffer); | ||
|
@@ -260,7 +263,7 @@ public void Disconnect() | |
_eventSource.Changed -= OnChanged; | ||
} | ||
|
||
private void RaiseTagsChanged(ITextBuffer buffer, NormalizedSnapshotSpanCollection difference) | ||
private void RaiseTagsChanged(ITextBuffer buffer, DiffResult difference) | ||
{ | ||
this.AssertIsForeground(); | ||
if (difference.Count == 0) | ||
|
@@ -270,10 +273,10 @@ private void RaiseTagsChanged(ITextBuffer buffer, NormalizedSnapshotSpanCollecti | |
} | ||
|
||
RaiseTagsChanged(SpecializedCollections.SingletonCollection( | ||
new KeyValuePair<ITextBuffer, NormalizedSnapshotSpanCollection>(buffer, difference))); | ||
new KeyValuePair<ITextBuffer, DiffResult>(buffer, difference))); | ||
} | ||
|
||
private void RaiseTagsChanged(ICollection<KeyValuePair<ITextBuffer, NormalizedSnapshotSpanCollection>> collection) | ||
private void RaiseTagsChanged(ICollection<KeyValuePair<ITextBuffer, DiffResult>> collection) | ||
{ | ||
TagsChangedForBuffer?.Invoke(collection); | ||
} | ||
|
@@ -296,9 +299,12 @@ private static T NextOrDefault<T>(IEnumerator<T> enumerator) | |
/// <summary> | ||
/// Return all the spans that appear in only one of "latestSpans" or "previousSpans". | ||
/// </summary> | ||
private static IEnumerable<SnapshotSpan> Difference<T>(IEnumerable<ITagSpan<T>> latestSpans, IEnumerable<ITagSpan<T>> previousSpans, IEqualityComparer<T> comparer) | ||
private static DiffResult Difference<T>(IEnumerable<ITagSpan<T>> latestSpans, IEnumerable<ITagSpan<T>> previousSpans, IEqualityComparer<T> comparer) | ||
where T : ITag | ||
{ | ||
List<SnapshotSpan> added = null; | ||
List<SnapshotSpan> removed = null; | ||
|
||
var latestEnumerator = latestSpans.GetEnumerator(); | ||
var previousEnumerator = previousSpans.GetEnumerator(); | ||
try | ||
|
@@ -313,12 +319,12 @@ private static IEnumerable<SnapshotSpan> Difference<T>(IEnumerable<ITagSpan<T>> | |
|
||
if (latestSpan.Start < previousSpan.Start) | ||
{ | ||
yield return latestSpan; | ||
AddToList(ref added, latestSpan); | ||
latest = NextOrDefault(latestEnumerator); | ||
} | ||
else if (previousSpan.Start < latestSpan.Start) | ||
{ | ||
yield return previousSpan; | ||
AddToList(ref removed, previousSpan); | ||
previous = NextOrDefault(previousEnumerator); | ||
} | ||
else | ||
|
@@ -327,19 +333,19 @@ private static IEnumerable<SnapshotSpan> Difference<T>(IEnumerable<ITagSpan<T>> | |
// region to be conservative. | ||
if (previousSpan.End > latestSpan.End) | ||
{ | ||
yield return previousSpan; | ||
AddToList(ref removed, previousSpan); | ||
latest = NextOrDefault(latestEnumerator); | ||
} | ||
else if (latestSpan.End > previousSpan.End) | ||
{ | ||
yield return latestSpan; | ||
AddToList(ref added, latestSpan); | ||
previous = NextOrDefault(previousEnumerator); | ||
} | ||
else | ||
{ | ||
if (!comparer.Equals(latest.Tag, previous.Tag)) | ||
{ | ||
yield return latestSpan; | ||
AddToList(ref added, latestSpan); | ||
} | ||
|
||
latest = NextOrDefault(latestEnumerator); | ||
|
@@ -350,13 +356,13 @@ private static IEnumerable<SnapshotSpan> Difference<T>(IEnumerable<ITagSpan<T>> | |
|
||
while (latest != null) | ||
{ | ||
yield return latest.Span; | ||
AddToList(ref added, latest.Span); | ||
latest = NextOrDefault(latestEnumerator); | ||
} | ||
|
||
while (previous != null) | ||
{ | ||
yield return previous.Span; | ||
AddToList(ref removed, previous.Span); | ||
previous = NextOrDefault(previousEnumerator); | ||
} | ||
} | ||
|
@@ -365,6 +371,14 @@ private static IEnumerable<SnapshotSpan> Difference<T>(IEnumerable<ITagSpan<T>> | |
latestEnumerator.Dispose(); | ||
previousEnumerator.Dispose(); | ||
} | ||
|
||
return new DiffResult(added, removed); | ||
} | ||
|
||
private static void AddToList<T>(ref List<T> list, T item) | ||
{ | ||
list = list ?? new List<T>(); | ||
list.Add(item); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This optimization seems unnecessary. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why bad to have it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because it's a micro optimization that in general doesn't seem like it will do anything. Also, a better optimization would be to pool. These lists may be very large. And they're going to be created, and then thrown away immediately when the DiffResult is created. If we pool them, then we only ever have as many lists created as needed to service the max number of threads doing diffing simultaneously. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -90,10 +90,9 @@ private void RemoveAllTags() | |
|
||
var snapshot = _subjectBuffer.CurrentSnapshot; | ||
var oldTagTree = GetTagTree(snapshot, oldTagTrees); | ||
var newTagTree = GetTagTree(snapshot, this.CachedTagTrees); | ||
|
||
var difference = ComputeDifference(snapshot, newTagTree, oldTagTree); | ||
RaiseTagsChanged(snapshot.TextBuffer, difference); | ||
|
||
// everything from old tree is removed. | ||
RaiseTagsChanged(snapshot.TextBuffer, new DiffResult(added: null, removed: oldTagTree.GetSpans(snapshot).Select(s => s.Span))); | ||
} | ||
|
||
private void OnSubjectBufferChanged(object sender, TextContentChangedEventArgs e) | ||
|
@@ -193,11 +192,13 @@ private void RemoveTagsThatIntersectEdit(TextContentChangedEventArgs e) | |
var oldTagTrees = this.CachedTagTrees; | ||
this.CachedTagTrees = oldTagTrees.SetItem(snapshot.TextBuffer, newTagTree); | ||
|
||
// Grab our old tags. We might not have any, so in this case we'll just pretend it's | ||
// empty | ||
var oldTagTree = GetTagTree(snapshot, oldTagTrees); | ||
// Not sure why we are diffing when we already have tagsToRemove. is it due to _tagSpanComparer might return | ||
// different result than GetIntersectingSpans? | ||
// | ||
// treeForBuffer basically points to oldTagTrees. case where oldTagTrees not exist is already taken cared by | ||
// CachedTagTrees.TryGetValue. | ||
var difference = ComputeDifference(snapshot, newTagTree, treeForBuffer); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you're not using oldTagTrees, then remove the local. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oh. good catch. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
||
var difference = ComputeDifference(snapshot, newTagTree, oldTagTree); | ||
RaiseTagsChanged(snapshot.TextBuffer, difference); | ||
} | ||
|
||
|
@@ -594,7 +595,7 @@ private void ProcessNewTagTrees( | |
object newState, | ||
CancellationToken cancellationToken) | ||
{ | ||
var bufferToChanges = new Dictionary<ITextBuffer, NormalizedSnapshotSpanCollection>(); | ||
var bufferToChanges = new Dictionary<ITextBuffer, DiffResult>(); | ||
using (Logger.LogBlock(FunctionId.Tagger_TagSource_ProcessNewTags, cancellationToken)) | ||
{ | ||
foreach (var latestBuffer in newTagTrees.Keys) | ||
|
@@ -609,8 +610,7 @@ private void ProcessNewTagTrees( | |
else | ||
{ | ||
// It's a new buffer, so report all spans are changed | ||
var allSpans = new NormalizedSnapshotSpanCollection(newTagTrees[latestBuffer].GetSpans(snapshot).Select(t => t.Span)); | ||
bufferToChanges[latestBuffer] = allSpans; | ||
bufferToChanges[latestBuffer] = new DiffResult(added: newTagTrees[latestBuffer].GetSpans(snapshot).Select(t => t.Span), removed: null); | ||
} | ||
} | ||
|
||
|
@@ -619,8 +619,7 @@ private void ProcessNewTagTrees( | |
if (!newTagTrees.ContainsKey(oldBuffer)) | ||
{ | ||
// This buffer disappeared, so let's notify that the old tags are gone | ||
var allSpans = new NormalizedSnapshotSpanCollection(oldTagTrees[oldBuffer].GetSpans(oldBuffer.CurrentSnapshot).Select(t => t.Span)); | ||
bufferToChanges[oldBuffer] = allSpans; | ||
bufferToChanges[oldBuffer] = new DiffResult(added: null, removed: oldTagTrees[oldBuffer].GetSpans(oldBuffer.CurrentSnapshot).Select(t => t.Span)); | ||
} | ||
} | ||
} | ||
|
@@ -640,7 +639,7 @@ private void ProcessNewTagTrees( | |
|
||
private void UpdateStateAndReportChanges( | ||
ImmutableDictionary<ITextBuffer, TagSpanIntervalTree<TTag>> newTagTrees, | ||
Dictionary<ITextBuffer, NormalizedSnapshotSpanCollection> bufferToChanges, | ||
Dictionary<ITextBuffer, DiffResult> bufferToChanges, | ||
object newState) | ||
{ | ||
_workQueue.AssertIsForeground(); | ||
|
@@ -673,13 +672,12 @@ private void UpdateStateAndReportChanges( | |
RaiseTagsChanged(bufferToChanges); | ||
} | ||
|
||
private NormalizedSnapshotSpanCollection ComputeDifference( | ||
private DiffResult ComputeDifference( | ||
ITextSnapshot snapshot, | ||
TagSpanIntervalTree<TTag> latestSpans, | ||
TagSpanIntervalTree<TTag> previousSpans) | ||
{ | ||
return new NormalizedSnapshotSpanCollection( | ||
Difference(latestSpans.GetSpans(snapshot), previousSpans.GetSpans(snapshot), _dataSource.TagComparer)); | ||
return Difference(latestSpans.GetSpans(snapshot), previousSpans.GetSpans(snapshot), _dataSource.TagComparer); | ||
} | ||
|
||
/// <summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Threading; | ||
using Microsoft.CodeAnalysis.Editor.Shared.Tagging; | ||
using Microsoft.CodeAnalysis.Shared.TestHooks; | ||
using Microsoft.VisualStudio.Text; | ||
using Microsoft.VisualStudio.Text.Tagging; | ||
|
@@ -77,7 +78,7 @@ private void OnResumed(object sender, EventArgs e) | |
_batchChangeNotifier.Resume(); | ||
} | ||
|
||
private void OnTagsChangedForBuffer(ICollection<KeyValuePair<ITextBuffer, NormalizedSnapshotSpanCollection>> changes) | ||
private void OnTagsChangedForBuffer(ICollection<KeyValuePair<ITextBuffer, DiffResult>> changes) | ||
{ | ||
_tagSource.AssertIsForeground(); | ||
|
||
|
@@ -94,10 +95,27 @@ private void OnTagsChangedForBuffer(ICollection<KeyValuePair<ITextBuffer, Normal | |
} | ||
|
||
// Now report them back to the UI on the main thread. | ||
_batchChangeNotifier.EnqueueChanges(change.Value); | ||
|
||
// We ask to update UI immediately for removed tags | ||
NotifyEditors(change.Value.Removed, _tagSource.OldTagNotificationDelay); | ||
NotifyEditors(change.Value.Added, _tagSource.NewTagsNotificationDelay); | ||
} | ||
} | ||
|
||
private void NotifyEditors(NormalizedSnapshotSpanCollection changes, TaggerDelay delay) | ||
{ | ||
if (delay == TaggerDelay.NearImmediate) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bail immediately if changes is empty. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add foreground assertion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
{ | ||
// if delay is immediate, we let notifier knows about the change right away | ||
_batchChangeNotifier.EnqueueChanges(changes); | ||
return; | ||
} | ||
|
||
// if delay is anything more than that, we let notifier knows about the change after given delay | ||
// event notification is not cancellable. | ||
_tagSource.RegisterNotification(() => _batchChangeNotifier.EnqueueChanges(changes), (int)delay.ComputeTimeDelay(_subjectBuffer).TotalMilliseconds, CancellationToken.None); | ||
} | ||
|
||
public IEnumerable<ITagSpan<TTag>> GetTags(NormalizedSnapshotSpanCollection requestedSpans) | ||
{ | ||
return GetTagsWorker(requestedSpans, accurate: false, cancellationToken: CancellationToken.None); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,6 +70,16 @@ internal abstract partial class AbstractAsynchronousTaggerProvider<TTag> : Foreg | |
protected virtual IEnumerable<Option<bool>> Options => SpecializedCollections.EmptyEnumerable<Option<bool>>(); | ||
protected virtual IEnumerable<PerLanguageOption<bool>> PerLanguageOptions => SpecializedCollections.EmptyEnumerable<PerLanguageOption<bool>>(); | ||
|
||
/// <summary> | ||
/// This controls what delay tagger will use to let editor know about newly inserted tags | ||
/// </summary> | ||
protected virtual TaggerDelay NewTagsNotificationDelay => TaggerDelay.NearImmediate; | ||
|
||
/// <summary> | ||
/// This controls what delay tagger will use to let editor know about just deleted tags. | ||
/// </summary> | ||
protected virtual TaggerDelay OldTagNotificationDelay => TaggerDelay.NearImmediate; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OldTag is not the right name. This should be something like "RemovedTagNotificationDelay" vs "AddedTagNotificationDelay". There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should these both be NearImmediate by default? What does that do to existing taggers? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. NearImmediate is the existing behavior. |
||
|
||
#if DEBUG | ||
public readonly string StackTrace; | ||
#endif | ||
|
@@ -212,5 +222,19 @@ protected virtual Task ProduceTagsAsync(TaggerContext<TTag> context, DocumentSna | |
{ | ||
return SpecializedTasks.EmptyTask; | ||
} | ||
|
||
private struct DiffResult | ||
{ | ||
public NormalizedSnapshotSpanCollection Added { get; } | ||
public NormalizedSnapshotSpanCollection Removed { get; } | ||
|
||
public DiffResult(IEnumerable<SnapshotSpan> added, IEnumerable<SnapshotSpan> removed) | ||
{ | ||
Added = added != null ? new NormalizedSnapshotSpanCollection(added) : NormalizedSnapshotSpanCollection.Empty; | ||
Removed = removed != null ? new NormalizedSnapshotSpanCollection(removed) : NormalizedSnapshotSpanCollection.Empty; | ||
} | ||
|
||
public int Count => Added.Count + Removed.Count; | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much simpler to just 'new' these here. You could even pool them if you really wanted to since the DiffResult is going to make a copy of them in the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the reason I put that there was most of time, one of tree will be 0 length. so no reason that create new list. and DiffResult check it being null and use Normalized ...Empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed it to use pool. but I still think most of time, one of the list will be always 0