Interval tree refactorings. #73846

CyrusNajmabadi · 2024-06-04T21:07:49Z

These are some small-scale refactorings to the interval tree refactoring to prepare for a larger piece of work coming next.

The future change is that we want to provide a new impl of the interval tree that is much faster and more space efficient than the existing binary-tree form for clients (like tagging) that know all the intervals up front and which can produce the tree in one go. That tree will actually be stored directly in a flat, immutable, array, in a much more compact form as it doesn't have to ever change.

The changes in this pr are as follows:

Many features were updated to compute their spans up front, and then create the interval tree once at the end. Not all features could do this though, as we have a small handful of features that genuinely make use of the mutable nature of our binary-interval-trees to build up state incrementally that they query while building.
The core interval tree necessary functionality was extracted out into an interface.
Much of the operation on that type were extracted out to run on that interface instead, so that we can have the same rich surface area with the different 'flat' impl.
Our existing trees were renamed to have 'Binary' in the name to indicate to clients what form they are getting.

CyrusNajmabadi · 2024-06-04T21:09:09Z

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

@@ -27,7 +27,7 @@ internal sealed partial class TagSpanIntervalTree<TTag>(SpanTrackingMode spanTra
    public static readonly TagSpanIntervalTree<TTag> Empty = new(SpanTrackingMode.EdgeInclusive);

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


renaming to add 'Binary' for clarity here. The upcoming interval tree will probably be called FlatArrayIntervalTree or something like that.

CyrusNajmabadi · 2024-06-04T21:11:11Z

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

@@ -57,10 +57,10 @@ private static TagSpan<TTag> GetTranslatedTagSpan(TagSpan<TTag> originalTagSpan,
            : new(GetTranslatedSpan(originalTagSpan, textSnapshot, trackingMode), originalTagSpan.Tag);

    public bool HasSpanThatContains(SnapshotPoint point)
-        => _tree.HasIntervalThatContains(point.Position, length: 0, new IntervalIntrospector(point.Snapshot, _spanTrackingMode));
+        => _tree.Algorithms.HasIntervalThatContains(point.Position, length: 0, new IntervalIntrospector(point.Snapshot, _spanTrackingMode));


these helpers could not be extensions for two reasons:

the types themselves have some existing intance methods that have hte same name. and we never look up extensions if we find a matching instance name (even if it fails).

The extensions were on a TIntervalTree so that they can operate without calling through an interface once instantiated. but the inference required there is beyond what C# can do today. So i used the pattern of asking the instance for a wrapper struct that itself then exposes the operations. The property handles the initial part of generic inference, meaning that the only inference has to happen on the TIntrospector type, which works fine.

CyrusNajmabadi · 2024-06-04T21:11:31Z

...p/Portable/ConvertToRawString/ConvertInterpolatedStringToRawStringCodeRefactoringProvider.cs

-        var interpolationInteriorSpans = new TextSpanIntervalTree();
-        var restrictedSpans = new TextSpanIntervalTree();
+        var interpolationInteriorSpans = new SegmentedList<TextSpan>();
+        var restrictedSpans = new SegmentedList<TextSpan>();


moving to tree construction at end, instead of as we go along.

tree construction at end. this will move to the FlatArrayIntervalTree in next pr.

CyrusNajmabadi · 2024-06-04T21:11:57Z

src/VisualStudio/Core/Def/DocumentOutline/DocumentOutlineViewModel.cs

+        // models given any position in the file with any particular text snapshot.
+        using var _ = SegmentedListPool.GetPooledList<DocumentSymbolDataViewModel>(out var models);
+        AddAllModels(newViewModelItems, models);
+        var intervalTree = BinaryIntervalTree.Create(new IntervalIntrospector(), models);


tree construction at end. this will move to the FlatArrayIntervalTree in next pr.

CyrusNajmabadi · 2024-06-04T21:12:12Z

src/VisualStudio/Core/Def/InheritanceMargin/InheritanceGlyphManager.cs

+    /// <summary>
+    /// Mutable.  Must only be accessed from the UI thread.
+    /// </summary>
+    private SimpleBinaryIntervalTree<GlyphData, GlyphDataIntrospector> _glyphDataTree;


tree construction at end. this will move to the FlatArrayIntervalTree in next pr.

CyrusNajmabadi · 2024-06-04T21:12:23Z

src/Workspaces/Core/Portable/Classification/ClassifierHelper.cs

                finalParts.Add(part);
            }
        }

+        var semanticPartsTree = new SimpleBinaryIntervalTree<TClassifiedSpan, TClassifiedSpanIntervalIntrospector>(default, values: semanticSpans);


tree construction at end. this will move to the FlatArrayIntervalTree in next pr.

CyrusNajmabadi · 2024-06-04T21:12:48Z

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

+/// example checking if an interval 'contains', 'intersects', or 'overlaps' with a requested span.
+/// </summary>
+internal delegate bool TestInterval<T, TIntrospector>(T value, int start, int length, in TIntrospector introspector)
+    where TIntrospector : struct, IIntervalIntrospector<T>;


new core types taht power the BinaryIntervalTree, and will power the upcoming FlatArrayIntervalTree.

CyrusNajmabadi · 2024-06-04T21:13:19Z

...orkspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/IntervalTreeAlgorithms`2.cs

+/// involved here is too complex for C# to handle (specifically using a <c>TIntervalTree</c> type), which would make
+/// ergonomics extremely painful as the callsites would have to pass three type arguments along explicitly.
+/// </summary>
+internal readonly struct IntervalTreeAlgorithms<T, TIntervalTree>(TIntervalTree tree) where TIntervalTree : IIntervalTree<T>


all the algorithms that will work fine on any interval tree impl (binary, or flat array). extracted out from the Binary version.

CyrusNajmabadi · 2024-06-04T21:13:58Z

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

+    /// <summary>
+    /// Provides access to lots of common algorithms on this interval tree.
+    /// </summary>
+    public IntervalTreeAlgorithms<T, BinaryIntervalTree<T>> Algorithms => new(this);


this is how we get type inference to work. a user just calls .Algorithms, and the T and TIntervalTree types are already solved for them on the Algorithms wrapper they get back.

CyrusNajmabadi · 2024-06-04T21:14:04Z

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

@@ -101,102 +104,7 @@ public static IntervalTree<T> CreateFromSorted<TIntrospector>(in TIntrospector i
        return node;
    }

-    protected static bool Contains<TIntrospector>(T value, int start, int length, in TIntrospector introspector)


moved to the Algorithms type.

CyrusNajmabadi · 2024-06-04T21:14:22Z

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

-    private int FillWithIntervalsThatMatch<TIntrospector>(
-        int start, int length, TestInterval<TIntrospector> testInterval,
+    int IIntervalTree<T>.FillWithIntervalsThatMatch<TIntrospector>(
+        int start, int length, TestInterval<T, TIntrospector> testInterval,


switched to explicit impl as nothing should call this directly, just the algorithms type.

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

ToddGrun · 2024-06-04T21:33:27Z

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

+    int FillWithIntervalsThatMatch<TIntrospector>(
+        int start, int length, TestInterval<T, TIntrospector> testInterval,
+        ref TemporaryArray<T> builder, in TIntrospector introspector,
+        bool stopAfterFirst)


bool stopAfterFirst

maybe use a max find count?

keeping the same logic for now. will change after flag goes in.

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

src/VisualStudio/Core/Def/DocumentOutline/DocumentOutlineViewModel.cs

…odel.cs

ToddGrun · 2024-06-04T22:12:43Z

src/VisualStudio/Core/Def/InheritanceMargin/InheritanceGlyphManager.cs

@@ -130,7 +134,7 @@ public void SetSnapshotAndUpdate(ITextSnapshot snapshot, IList<ITextViewLine> ne
        {
            // Go through all the existing visuals and invalidate or transform as appropriate.
            var allGlyphData = _glyphDataTree.ToImmutableArray();
-            _glyphDataTree.ClearInPlace();
+            _glyphDataTree = new(new GlyphDataIntrospector(), values: null);


new GlyphDataIntrospector(

Does this object hold state? If not, would it be worth only having one of these?

ToddGrun

CyrusNajmabadi added 17 commits June 4, 2024 12:57

Remove Clear method from interval trees

eca0bb5

Move code to initializing interval tree at end

182c222

Move code to initializing interval tree at end

c84f885

Move code to initializing interval tree at end

1368a36

Rename interval trees to indicate they use a binary tree internally

f300853

IN progress

70f097b

In progress

dfd72e7

Update simple binary tree

99771b6

Workspace compiling

3f7fb88

Add docs

2d8d581

Move and doc type

208c467

Move and doc type

c59d72c

Docs

663d1fe

Rename type

b78a75f

Rename type

70bf293

Delete dead code

b11bf5e

Make explicit impl

f9f2c87

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

CyrusNajmabadi commented Jun 4, 2024

View reviewed changes

ToddGrun reviewed Jun 4, 2024

View reviewed changes

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/IIntervalTree`1.cs Outdated Show resolved Hide resolved

ToddGrun reviewed Jun 4, 2024

View reviewed changes

src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/IIntervalTree`1.cs Outdated Show resolved Hide resolved

CyrusNajmabadi added 2 commits June 4, 2024 14:39

Fix

d73ad9b

Fix

6fae781

CyrusNajmabadi marked this pull request as ready for review June 4, 2024 21:54

CyrusNajmabadi requested a review from a team as a code owner June 4, 2024 21:54

CyrusNajmabadi commented Jun 4, 2024

View reviewed changes

src/VisualStudio/Core/Def/DocumentOutline/DocumentOutlineViewModel.cs Outdated Show resolved Hide resolved

Update src/VisualStudio/Core/Def/DocumentOutline/DocumentOutlineViewM…

4d43964

…odel.cs

ToddGrun reviewed Jun 4, 2024

View reviewed changes

ToddGrun approved these changes Jun 4, 2024

View reviewed changes

CyrusNajmabadi enabled auto-merge June 4, 2024 22:23

CyrusNajmabadi merged commit bc5ea08 into dotnet:main Jun 4, 2024
25 checks passed

dotnet-policy-service bot added this to the Next milestone Jun 4, 2024

CyrusNajmabadi deleted the flatIntervalTree branch June 5, 2024 00:28

This was referenced Jun 6, 2024

[Automated] PRs inserted in VS build main-35005.113 #73865

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35006.210 #73890

Closed

[Automated] PRs inserted in VS build 20240610.19 #73926

Closed

jjonescz modified the milestones: Next, 17.11 P3 Jun 24, 2024

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

Interval tree refactorings. #73846

Interval tree refactorings. #73846

CyrusNajmabadi commented Jun 4, 2024 •

edited

Loading

CyrusNajmabadi Jun 4, 2024

CyrusNajmabadi Jun 4, 2024

CyrusNajmabadi Jun 4, 2024

CyrusNajmabadi Jun 4, 2024

CyrusNajmabadi Jun 4, 2024

CyrusNajmabadi Jun 4, 2024

CyrusNajmabadi Jun 4, 2024

CyrusNajmabadi Jun 4, 2024

CyrusNajmabadi Jun 4, 2024

CyrusNajmabadi Jun 4, 2024

CyrusNajmabadi Jun 4, 2024

CyrusNajmabadi Jun 4, 2024

ToddGrun Jun 4, 2024

CyrusNajmabadi Jun 4, 2024

ToddGrun Jun 4, 2024

ToddGrun left a comment

Interval tree refactorings. #73846

Interval tree refactorings. #73846

Conversation

CyrusNajmabadi commented Jun 4, 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

ToddGrun left a comment

Choose a reason for hiding this comment

CyrusNajmabadi commented Jun 4, 2024 •

edited

Loading