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

Reduce lots of allocations in TagSpanIntervalTree #73715

Merged
merged 33 commits into from
May 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
acbd624
in progress
CyrusNajmabadi May 25, 2024
994f6fe
Merge branch 'tagSpan' into allIntervalTreeWork
CyrusNajmabadi May 25, 2024
c4a9f59
Merge branch 'intervalTreeSpan' into allIntervalTreeWork
CyrusNajmabadi May 25, 2024
acaf0f5
Do not hold onto text buffer
CyrusNajmabadi May 25, 2024
e47a164
Simplify
CyrusNajmabadi May 25, 2024
f62585d
Simplify
CyrusNajmabadi May 25, 2024
fa9d227
Simplify
CyrusNajmabadi May 25, 2024
0d4d851
Simplify
CyrusNajmabadi May 25, 2024
1093fc7
Better pooling
CyrusNajmabadi May 25, 2024
fb52a1a
remove method
CyrusNajmabadi May 25, 2024
4df5f39
inline
CyrusNajmabadi May 25, 2024
4196af4
Simplify
CyrusNajmabadi May 25, 2024
02f27fc
Docs
CyrusNajmabadi May 25, 2024
49f23ae
less allocations
CyrusNajmabadi May 25, 2024
f3b677b
Pass along snapshot, not buffer
CyrusNajmabadi May 25, 2024
9fca484
use new helper. avoid allocs
CyrusNajmabadi May 25, 2024
74e6911
reuse buffer
CyrusNajmabadi May 25, 2024
36ff803
Merge branch 'tagSpan' into allIntervalTreeWork
CyrusNajmabadi May 25, 2024
8acf1bb
pool objects
CyrusNajmabadi May 25, 2024
f37f24b
remove method
CyrusNajmabadi May 25, 2024
488d6cf
Use existing helper
CyrusNajmabadi May 25, 2024
367c625
move helper to tests
CyrusNajmabadi May 25, 2024
0291f82
doc comment
CyrusNajmabadi May 25, 2024
3d6ae3d
inline
CyrusNajmabadi May 25, 2024
9f7f16d
Merge remote-tracking branch 'upstream/main' into allIntervalTreeWork
CyrusNajmabadi May 25, 2024
46e4d70
rename
CyrusNajmabadi May 25, 2024
42d410b
Merge branch 'main' into allIntervalTreeWork
CyrusNajmabadi May 25, 2024
244da42
pass by ref
CyrusNajmabadi May 26, 2024
7209a05
Make static
CyrusNajmabadi May 26, 2024
acb8324
Remove allocations
CyrusNajmabadi May 26, 2024
bb50d0e
pull out of loop
CyrusNajmabadi May 26, 2024
8b2386c
Move to segmented list
CyrusNajmabadi May 26, 2024
b46bc3d
empty on null root
CyrusNajmabadi May 26, 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 @@ -159,8 +159,7 @@ await TotalClassificationAggregateTagger.AddTagsAsync(
arg: default).ConfigureAwait(false);
});

var cachedTags = new TagSpanIntervalTree<IClassificationTag>(snapshot.TextBuffer, SpanTrackingMode.EdgeExclusive, mergedTags);

var cachedTags = new TagSpanIntervalTree<IClassificationTag>(snapshot, SpanTrackingMode.EdgeExclusive, mergedTags);
Copy link
Member Author

Choose a reason for hiding this comment

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

changed the tree to take in the snapshot it initially corresponds to, not a random textbuffer it grabs CurrentSnapshot off of.

lock (_gate)
{
_cachedTaggedSpan = spanToTag;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,10 +93,10 @@ protected override async Task ProduceTagsAsync(
var position = caretPosition.Value;
var snapshot = snapshotSpan.Snapshot;

// See if the user is just moving their caret around in an existing tag. If so, we don't
// want to actually go recompute things. Note: this only works for containment. If the
// user moves their caret to the end of a highlighted reference, we do want to recompute
// as they may now be at the start of some other reference that should be highlighted instead.
// See if the user is just moving their caret around in an existing tag. If so, we don't want to actually go
// recompute things. Note: this only works for containment. If the user moves their caret to the end of a
// highlighted reference, we do want to recompute as they may now be at the start of some other reference that
// should be highlighted instead.
var onExistingTags = context.HasExistingContainingTags(new SnapshotPoint(snapshot, position));
if (onExistingTags)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,10 +122,10 @@ protected override Task ProduceTagsAsync(
return Task.CompletedTask;
}

// See if the user is just moving their caret around in an existing tag. If so, we don't
// want to actually go recompute things. Note: this only works for containment. If the
// user moves their caret to the end of a highlighted reference, we do want to recompute
// as they may now be at the start of some other reference that should be highlighted instead.
// See if the user is just moving their caret around in an existing tag. If so, we don't want to actually go
// recompute things. Note: this only works for containment. If the user moves their caret to the end of a
// highlighted reference, we do want to recompute as they may now be at the start of some other reference that
// should be highlighted instead.
var onExistingTags = context.HasExistingContainingTags(caretPosition);
if (onExistingTags)
{
Expand Down
266 changes: 134 additions & 132 deletions src/EditorFeatures/Core/Shared/Tagging/Utilities/TagSpanIntervalTree.cs
Original file line number Diff line number Diff line change
Expand Up @@ -21,66 +21,53 @@ namespace Microsoft.CodeAnalysis.Editor.Shared.Tagging;
/// tracked. That way you can query for intersecting/overlapping spans in a different snapshot
/// than the one for the tag spans that were added.
/// </summary>
internal sealed partial class TagSpanIntervalTree<TTag>(
ITextBuffer textBuffer,
SpanTrackingMode trackingMode,
IEnumerable<TagSpan<TTag>>? values1 = null,
IEnumerable<TagSpan<TTag>>? values2 = null) where TTag : ITag
internal sealed partial class TagSpanIntervalTree<TTag>(SpanTrackingMode spanTrackingMode) where TTag : ITag
{
private readonly ITextBuffer _textBuffer = textBuffer;
private readonly SpanTrackingMode _spanTrackingMode = trackingMode;
private readonly IntervalTree<TagSpan<TTag>> _tree = IntervalTree.Create(
new IntervalIntrospector(textBuffer.CurrentSnapshot, trackingMode),
values1, values2);

private static SnapshotSpan GetTranslatedSpan(
TagSpan<TTag> originalTagSpan, ITextSnapshot textSnapshot, SpanTrackingMode trackingMode)
// Tracking mode passed in here doesn't matter (since the tree is empty).
public static readonly TagSpanIntervalTree<TTag> Empty = new(SpanTrackingMode.EdgeInclusive);

private readonly SpanTrackingMode _spanTrackingMode = spanTrackingMode;
private readonly IntervalTree<TagSpan<TTag>> _tree = IntervalTree<TagSpan<TTag>>.Empty;

public TagSpanIntervalTree(
ITextSnapshot textSnapshot,
SpanTrackingMode trackingMode,
IEnumerable<TagSpan<TTag>>? values1 = null,
IEnumerable<TagSpan<TTag>>? values2 = null)
: this(trackingMode)
{
var localSpan = originalTagSpan.Span;

return localSpan.Snapshot == textSnapshot
? localSpan
: localSpan.TranslateTo(textSnapshot, trackingMode);
_tree = IntervalTree.Create(
new IntervalIntrospector(textSnapshot, trackingMode),
values1, values2);
}

private TagSpan<TTag> GetTranslatedITagSpan(TagSpan<TTag> originalTagSpan, ITextSnapshot textSnapshot)
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. no need for this (the interface version) and GetTranslatedTagSpan now that everything moved to being TagSpan based.

// Avoid reallocating in the case where we're on the same snapshot.
=> originalTagSpan.Span.Snapshot == textSnapshot
? originalTagSpan
: GetTranslatedTagSpan(originalTagSpan, textSnapshot, _spanTrackingMode);
private static SnapshotSpan GetTranslatedSpan(TagSpan<TTag> originalTagSpan, ITextSnapshot textSnapshot, SpanTrackingMode trackingMode)
// SnapshotSpan no-ops if you pass it the same snapshot that it is holding onto.
=> originalTagSpan.Span.TranslateTo(textSnapshot, trackingMode);

private TagSpan<TTag> GetTranslatedTagSpan(TagSpan<TTag> originalTagSpan, ITextSnapshot textSnapshot)
=> GetTranslatedTagSpan(originalTagSpan, textSnapshot, _spanTrackingMode);

private static TagSpan<TTag> GetTranslatedTagSpan(TagSpan<TTag> originalTagSpan, ITextSnapshot textSnapshot, SpanTrackingMode trackingMode)
// Avoid reallocating in the case where we're on the same snapshot.
=> originalTagSpan is TagSpan<TTag> tagSpan && tagSpan.Span.Snapshot == textSnapshot
? tagSpan
=> originalTagSpan.Span.Snapshot == textSnapshot
? originalTagSpan
: new(GetTranslatedSpan(originalTagSpan, textSnapshot, trackingMode), originalTagSpan.Tag);

public ITextBuffer Buffer => _textBuffer;

public SpanTrackingMode SpanTrackingMode => _spanTrackingMode;
Comment on lines -58 to -60
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 these two properties. they are not needed or appropriate to expose off of this type.


public bool HasSpanThatContains(SnapshotPoint point)
{
var snapshot = point.Snapshot;
Debug.Assert(snapshot.TextBuffer == _textBuffer);

return _tree.HasIntervalThatContains(point.Position, length: 0, new IntervalIntrospector(snapshot, _spanTrackingMode));
}
=> _tree.HasIntervalThatContains(point.Position, length: 0, new IntervalIntrospector(point.Snapshot, _spanTrackingMode));

public IReadOnlyList<TagSpan<TTag>> GetIntersectingSpans(SnapshotSpan snapshotSpan)
=> SegmentedListPool<TagSpan<TTag>>.ComputeList(
static (args, tags) => [email protected](args.snapshotSpan, tags),
(@this: this, snapshotSpan));
public bool HasSpanThatIntersects(SnapshotPoint point)
Copy link
Member Author

Choose a reason for hiding this comment

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

used to replace a call from the tagger that would get the full list of intersecting spans just to check if it was non-empty.

=> _tree.HasIntervalThatIntersectsWith(point.Position, new IntervalIntrospector(point.Snapshot, _spanTrackingMode));
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: the ref highlighting tagger uses both the existing HasSpanThatContains and the new HasSpanThatIntersects. There are comments in it indicating this is intentional. i don't love this, as i think it's likely one could be removed. but i'm preserving behavior here.


/// <summary>
/// Gets all the spans that intersect with <paramref name="snapshotSpan"/> in sorted order and adds them to
/// <paramref name="result"/>. Note the sorted chunk of items are appended to <paramref name="result"/>. This
/// means that <paramref name="result"/> may not be sorted if there were already items in them.
/// </summary>
private void AppendIntersectingSpansInSortedOrder(SnapshotSpan snapshotSpan, SegmentedList<TagSpan<TTag>> result)
public void AddIntersectingTagSpans(SnapshotSpan snapshotSpan, SegmentedList<TagSpan<TTag>> result)
Copy link
Member Author

Choose a reason for hiding this comment

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

replacement for GetSpans. No allocs. Caller passes in the value the ywant to fill.

{
var snapshot = snapshotSpan.Snapshot;
Debug.Assert(snapshot.TextBuffer == _textBuffer);

using var intersectingIntervals = TemporaryArray<TagSpan<TTag>>.Empty;
_tree.FillWithIntervalsThatIntersectWith(
Expand All @@ -92,8 +79,24 @@ ref intersectingIntervals.AsRef(),
result.Add(GetTranslatedTagSpan(tagSpan, snapshot, _spanTrackingMode));
}

public IEnumerable<TagSpan<TTag>> GetSpans(ITextSnapshot snapshot)
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 thsi expensive linq-based version that was consumed inefficiently.

=> _tree.Select(tn => GetTranslatedITagSpan(tn, snapshot));
/// <summary>
/// Gets all the tag spans in this tree, remapped to <paramref name="snapshot"/>, and returns them as a <see
/// cref="NormalizedSnapshotSpanCollection"/>.
/// </summary>
public NormalizedSnapshotSpanCollection GetSnapshotSpanCollection(ITextSnapshot snapshot)
Copy link
Member Author

Choose a reason for hiding this comment

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

several calls to GetSpans were replaced to a call to this. this version returns the right data they needed, while also pooling internal values needed to compute it.

{
if (this == Empty)
return NormalizedSnapshotSpanCollection.Empty;

using var _ = ArrayBuilder<SnapshotSpan>.GetInstance(out var spans);

foreach (var tagSpan in _tree)
spans.Add(GetTranslatedSpan(tagSpan, snapshot, _spanTrackingMode));

return spans.Count == 0
? NormalizedSnapshotSpanCollection.Empty
: new(spans);
Copy link
Contributor

Choose a reason for hiding this comment

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

new(spans);

nit: this ends up calling the IEnumerable ctor, right?

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! is tehre an alternative?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess not. There is an IList ctor for NormalizedSnaphotSpanCollection which doesn't use an enumerator, but it doesn't look like ArrayBuilder implements IList?

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, i can live with this for now :)

}

/// <summary>
/// Adds all the tag spans in <see langword="this"/> to <paramref name="tagSpans"/>, translating them to the given
Expand All @@ -102,7 +105,15 @@ public IEnumerable<TagSpan<TTag>> GetSpans(ITextSnapshot snapshot)
public void AddAllSpans(ITextSnapshot textSnapshot, HashSet<TagSpan<TTag>> tagSpans)
{
foreach (var tagSpan in _tree)
tagSpans.Add(GetTranslatedITagSpan(tagSpan, textSnapshot));
tagSpans.Add(GetTranslatedTagSpan(tagSpan, textSnapshot));
}

/// <inheritdoc cref="AddAllSpans(ITextSnapshot, HashSet{TagSpan{TTag}})"/>
/// <remarks>Spans will be added in sorted order</remarks>
public void AddAllSpans(ITextSnapshot textSnapshot, SegmentedList<TagSpan<TTag>> tagSpans)
{
foreach (var tagSpan in _tree)
tagSpans.Add(GetTranslatedTagSpan(tagSpan, textSnapshot));
}

/// <summary>
Expand All @@ -126,127 +137,118 @@ ref buffer.AsRef(),
new IntervalIntrospector(textSnapshot, _spanTrackingMode));

foreach (var tagSpan in buffer)
tagSpans.Remove(GetTranslatedITagSpan(tagSpan, textSnapshot));
tagSpans.Remove(GetTranslatedTagSpan(tagSpan, textSnapshot));
}
}

public bool IsEmpty()
=> _tree.IsEmpty();
Copy link
Member Author

Choose a reason for hiding this comment

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

not ever used.


public void AddIntersectingTagSpans(NormalizedSnapshotSpanCollection requestedSpans, SegmentedList<TagSpan<TTag>> tags)
{
AddIntersectingTagSpansWorker(requestedSpans, tags);
Copy link
Member Author

Choose a reason for hiding this comment

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

inlined.

DebugVerifyTags(requestedSpans, tags);
}

[Conditional("DEBUG")]
private static void DebugVerifyTags(NormalizedSnapshotSpanCollection requestedSpans, SegmentedList<TagSpan<TTag>> tags)
{
if (tags == null)
{
return;
}

foreach (var tag in tags)
{
var span = tag.Span;

if (!requestedSpans.Any(s => s.IntersectsWith(span)))
{
Contract.Fail(tag + " doesn't intersects with any requested span");
}
}
}

private void AddIntersectingTagSpansWorker(
NormalizedSnapshotSpanCollection requestedSpans,
SegmentedList<TagSpan<TTag>> tags)
{
const int MaxNumberOfRequestedSpans = 100;

// Special case the case where there is only one requested span. In that case, we don't
// need to allocate any intermediate collections
if (requestedSpans.Count == 1)
AppendIntersectingSpansInSortedOrder(requestedSpans[0], tags);
{
AddIntersectingTagSpans(requestedSpans[0], tags);
}
else if (requestedSpans.Count < MaxNumberOfRequestedSpans)
AddTagsForSmallNumberOfSpans(requestedSpans, tags);
{
foreach (var span in requestedSpans)
AddIntersectingTagSpans(span, tags);
Copy link
Member Author

Choose a reason for hiding this comment

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

inlined.

}
else
{
AddTagsForLargeNumberOfSpans(requestedSpans, tags);
}
}

private void AddTagsForSmallNumberOfSpans(
NormalizedSnapshotSpanCollection requestedSpans,
SegmentedList<TagSpan<TTag>> tags)
{
foreach (var span in requestedSpans)
AppendIntersectingSpansInSortedOrder(span, tags);
}
DebugVerifyTags(requestedSpans, tags);
return;

private void AddTagsForLargeNumberOfSpans(NormalizedSnapshotSpanCollection requestedSpans, SegmentedList<TagSpan<TTag>> tags)
{
// we are asked with bunch of spans. rather than asking same question again and again, ask once with big span
// which will return superset of what we want. and then filter them out in O(m+n) cost.
// m == number of requested spans, n = number of returned spans
var mergedSpan = new SnapshotSpan(requestedSpans[0].Start, requestedSpans[^1].End);
void AddTagsForLargeNumberOfSpans(NormalizedSnapshotSpanCollection requestedSpans, SegmentedList<TagSpan<TTag>> tags)
Copy link
Member Author

Choose a reason for hiding this comment

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

made into a local function.

{
// we are asked with bunch of spans. rather than asking same question again and again, ask once with big span
// which will return superset of what we want. and then filter them out in O(m+n) cost.
// m == number of requested spans, n = number of returned spans
var mergedSpan = new SnapshotSpan(requestedSpans[0].Start, requestedSpans[^1].End);

using var _1 = SegmentedListPool.GetPooledList<TagSpan<TTag>>(out var tempList);
using var _1 = SegmentedListPool.GetPooledList<TagSpan<TTag>>(out var tempList);

AppendIntersectingSpansInSortedOrder(mergedSpan, tempList);
if (tempList.Count == 0)
return;
AddIntersectingTagSpans(mergedSpan, tempList);
if (tempList.Count == 0)
return;

// Note: both 'requstedSpans' and 'tempList' are in sorted order.
// Note: both 'requestedSpans' and 'tempList' are in sorted order.

using var enumerator = tempList.GetEnumerator();
using var enumerator = tempList.GetEnumerator();

if (!enumerator.MoveNext())
return;
if (!enumerator.MoveNext())
return;

using var _2 = PooledHashSet<TagSpan<TTag>>.GetInstance(out var hashSet);
using var _2 = PooledHashSet<TagSpan<TTag>>.GetInstance(out var hashSet);

var requestIndex = 0;
while (true)
{
var currentTag = enumerator.Current;
var requestIndex = 0;
while (true)
{
var currentTag = enumerator.Current;

var currentRequestSpan = requestedSpans[requestIndex];
var currentTagSpan = currentTag.Span;
var currentRequestSpan = requestedSpans[requestIndex];
var currentTagSpan = currentTag.Span;

// The current tag is *before* the current span we're trying to intersect with. Move to the next tag to
// see if it intersects with the current span.
if (currentTagSpan.End < currentRequestSpan.Start)
{
// If there are no more tags, then we're done.
if (!enumerator.MoveNext())
return;
// The current tag is *before* the current span we're trying to intersect with. Move to the next tag to
// see if it intersects with the current span.
if (currentTagSpan.End < currentRequestSpan.Start)
{
// If there are no more tags, then we're done.
if (!enumerator.MoveNext())
return;

continue;
}
continue;
}

// The current tag is *after* teh current span we're trying to intersect with. Move to the next span to
// see if it intersects with the current tag.
if (currentTagSpan.Start > currentRequestSpan.End)
{
requestIndex++;
// The current tag is *after* teh current span we're trying to intersect with. Move to the next span to
// see if it intersects with the current tag.
if (currentTagSpan.Start > currentRequestSpan.End)
{
requestIndex++;

// If there are no more spans to intersect with, then we're done.
if (requestIndex >= requestedSpans.Count)
return;

continue;
}

// If there are no more spans to intersect with, then we're done.
if (requestIndex >= requestedSpans.Count)
return;
// This tag intersects the current span we're trying to intersect with. Ensure we only see and add a
// particular tag once.

continue;
if (currentTagSpan.Length > 0 &&
hashSet.Add(currentTag))
{
tags.Add(currentTag);
}

if (!enumerator.MoveNext())
break;
}
}
}

[Conditional("DEBUG")]
private static void DebugVerifyTags(NormalizedSnapshotSpanCollection requestedSpans, SegmentedList<TagSpan<TTag>> tags)
{
if (tags == null)
{
return;
}

// This tag intersects the current span we're trying to intersect with. Ensure we only see and add a
// particular tag once.
foreach (var tag in tags)
{
var span = tag.Span;

if (currentTagSpan.Length > 0 &&
hashSet.Add(currentTag))
if (!requestedSpans.Any(s => s.IntersectsWith(span)))
{
tags.Add(currentTag);
Contract.Fail(tag + " doesn't intersects with any requested span");
}

if (!enumerator.MoveNext())
break;
}
}
}
Loading
Loading