Reduce lots of allocations in TagSpanIntervalTree #73715

CyrusNajmabadi · 2024-05-25T20:53:20Z

Followup to #73713.

Moved to pooled collections, and callers pass in the collections they want to fill.

Before:

After:

CyrusNajmabadi · 2024-05-25T22:12:24Z

...eatures/Core/Classification/CopyPasteAndPrintingClassificationBufferTaggerProvider.Tagger.cs

@@ -159,8 +159,7 @@ public IEnumerable<TagSpan<IClassificationTag>> GetAllTags(NormalizedSnapshotSpa
                    arg: default).ConfigureAwait(false);
            });

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


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

CyrusNajmabadi · 2024-05-25T22:13:03Z

src/EditorFeatures/Core/Shared/Tagging/Utilities/TagSpanIntervalTree.cs

-    public ITextBuffer Buffer => _textBuffer;
-
-    public SpanTrackingMode SpanTrackingMode => _spanTrackingMode;


removed these two properties. they are not needed or appropriate to expose off of this type.

CyrusNajmabadi · 2024-05-25T22:16:08Z

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

@@ -141,7 +146,7 @@ private TagSpanIntervalTree<TTag> GetTagTree(ITextSnapshot snapshot, ImmutableDi
        {
            return tagTrees.TryGetValue(snapshot.TextBuffer, out var tagTree)
                ? tagTree
-                : new TagSpanIntervalTree<TTag>(snapshot.TextBuffer, _dataSource.SpanTrackingMode);
+                : TagSpanIntervalTree<TTag>.Empty;


singleton instead of alloc.

CyrusNajmabadi · 2024-05-25T22:16:30Z

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

+                // For efficiency, just grab the old tags, remap them to the current snapshot, and place them in the
+                // newTags buffer.  This is a safe mutation of this buffer as the caller doesn't use it after this point
+                // and instead immediately clears it.
+                oldTagTree.AddAllSpans(snapshot, newTags);


oogy, but safe, optimization.

CyrusNajmabadi · 2024-05-25T22:16:42Z

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

            }
            else
            {
                // We do have spans to invalidate. Get the set of old tags that don't intersect with those and add the new tags.
                using var _1 = _tagSpanSetPool.GetPooledObject(out var nonIntersectingTagSpans);
-                AddNonIntersectingTagSpans(spansToInvalidate, oldTagTree, nonIntersectingTagSpans);


CyrusNajmabadi · 2024-05-25T22:19:22Z

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

-            var latestSpans = latestTree.GetSpans(snapshot);
-            var previousSpans = previousTree.GetSpans(snapshot);
+            using var _1 = ArrayBuilder<TagSpan<TTag>>.GetInstance(out var latestSpans);
+            using var _2 = ArrayBuilder<TagSpan<TTag>>.GetInstance(out var previousSpans);


added pooling fo rthese as well. this also means the iteration is allocfree.

Why even mess with enumerators? We can just index directly into them now, right?

my god. you madman! will def change that :D

actually, it didn't make it much better afaict. i still need to track which builder and which index i'm working with. And... that's just an enumerator.

CyrusNajmabadi · 2024-05-25T22:21:33Z

src/EditorFeatures/Test/Tagging/TagSpanIntervalTreeTests.cs

@@ -17,117 +20,126 @@ namespace Microsoft.CodeAnalysis.Editor.UnitTests.Tagging;
 [UseExportProvider]
 public class TagSpanIntervalTreeTests
 {
-    private static TagSpanIntervalTree<ITextMarkerTag> CreateTree(string text, params Span[] spans)
+    private static (TagSpanIntervalTree<ITextMarkerTag>, ITextBuffer) CreateTree(string text, params Span[] spans)


just test code, mechanically updated now that the api changed.

CyrusNajmabadi · 2024-05-25T22:22:51Z

src/EditorFeatures/Core/Shared/Tagging/Utilities/TagSpanIntervalTree.cs

-            static (args, tags) => [email protected](args.snapshotSpan, tags),
-            (@this: this, snapshotSpan));
+    public bool HasSpanThatIntersects(SnapshotPoint point)
+        => _tree.HasIntervalThatIntersectsWith(point.Position, new IntervalIntrospector(point.Snapshot, _spanTrackingMode));


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.

CyrusNajmabadi · 2024-05-25T22:23:15Z

src/EditorFeatures/Core/Shared/Tagging/Utilities/TagSpanIntervalTree.cs


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


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

CyrusNajmabadi · 2024-05-25T22:24:11Z

src/EditorFeatures/Core/Shared/Tagging/Utilities/TagSpanIntervalTree.cs

    }

-    private TagSpan<TTag> GetTranslatedITagSpan(TagSpan<TTag> originalTagSpan, ITextSnapshot textSnapshot)


removed. no need for this (the interface version) and GetTranslatedTagSpan now that everything moved to being TagSpan based.

ToddGrun · 2024-05-25T22:50:43Z

src/EditorFeatures/Core/Shared/Tagging/Utilities/TagSpanIntervalTree.cs

+
+        return spans.Count == 0
+            ? NormalizedSnapshotSpanCollection.Empty
+            : new(spans);


new(spans);

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

yup! is tehre an alternative?

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?

yeah, i can live with this for now :)

ToddGrun · 2024-05-26T01:56:31Z

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/IntervalTree`1.cs

        {
-            var (currentNode, firstTime) = tuple;
-            if (currentNode != null)
+            if (root == null)


if (root == null)

not sure if this is common, but this could be pulled outside the worker too

ToddGrun

CyrusNajmabadi added 24 commits May 25, 2024 12:10

in progress

acbd624

Merge branch 'tagSpan' into allIntervalTreeWork

994f6fe

Merge branch 'intervalTreeSpan' into allIntervalTreeWork

c4a9f59

Do not hold onto text buffer

acaf0f5

Simplify

e47a164

Simplify

f62585d

Simplify

fa9d227

Simplify

0d4d851

Better pooling

1093fc7

remove method

fb52a1a

inline

4df5f39

Simplify

4196af4

Docs

02f27fc

less allocations

49f23ae

Pass along snapshot, not buffer

f3b677b

use new helper. avoid allocs

9fca484

reuse buffer

74e6911

Merge branch 'tagSpan' into allIntervalTreeWork

36ff803

pool objects

8acf1bb

remove method

f37f24b

Use existing helper

488d6cf

move helper to tests

367c625

doc comment

0291f82

inline

3d6ae3d

CyrusNajmabadi requested a review from a team as a code owner May 25, 2024 20:53

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

Merge remote-tracking branch 'upstream/main' into allIntervalTreeWork

9f7f16d

CyrusNajmabadi commented May 25, 2024

View reviewed changes

rename

46e4d70

CyrusNajmabadi commented May 25, 2024

View reviewed changes

ToddGrun reviewed May 25, 2024

View reviewed changes

CyrusNajmabadi added 5 commits May 25, 2024 16:52

Merge branch 'main' into allIntervalTreeWork

42d410b

pass by ref

244da42

Make static

7209a05

Remove allocations

acb8324

pull out of loop

bb50d0e

ToddGrun reviewed May 26, 2024

View reviewed changes

ToddGrun approved these changes May 26, 2024

View reviewed changes

CyrusNajmabadi added 2 commits May 25, 2024 19:09

Move to segmented list

8b2386c

empty on null root

b46bc3d

CyrusNajmabadi enabled auto-merge May 26, 2024 02:11

CyrusNajmabadi merged commit f117182 into dotnet:main May 26, 2024
25 checks passed

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

CyrusNajmabadi deleted the allIntervalTreeWork branch May 26, 2024 04:14

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce lots of allocations in TagSpanIntervalTree #73715

Reduce lots of allocations in TagSpanIntervalTree #73715

CyrusNajmabadi commented May 25, 2024 •

edited

Loading

CyrusNajmabadi May 25, 2024

CyrusNajmabadi May 25, 2024

CyrusNajmabadi May 25, 2024

CyrusNajmabadi May 25, 2024

CyrusNajmabadi May 25, 2024

CyrusNajmabadi May 25, 2024

ToddGrun May 26, 2024

CyrusNajmabadi May 26, 2024

CyrusNajmabadi May 26, 2024

CyrusNajmabadi May 25, 2024

CyrusNajmabadi May 25, 2024

CyrusNajmabadi May 25, 2024

CyrusNajmabadi May 25, 2024

ToddGrun May 25, 2024

CyrusNajmabadi May 25, 2024

ToddGrun May 26, 2024

CyrusNajmabadi May 26, 2024

ToddGrun May 26, 2024

CyrusNajmabadi May 26, 2024

ToddGrun left a comment

		public ITextBuffer Buffer => _textBuffer;

		public SpanTrackingMode SpanTrackingMode => _spanTrackingMode;

		}

		private TagSpan<TTag> GetTranslatedITagSpan(TagSpan<TTag> originalTagSpan, ITextSnapshot textSnapshot)

Reduce lots of allocations in TagSpanIntervalTree #73715

Reduce lots of allocations in TagSpanIntervalTree #73715

Conversation

CyrusNajmabadi commented May 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented May 25, 2024 •

edited

Loading