Build interval tree from sorted nodes #73819
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
CyrusNajmabadi
changed the title
| @@ -32,13 +32,15 @@ internal sealed partial class TagSpanIntervalTree<TTag>(SpanTrackingMode spanTra | |||
| public TagSpanIntervalTree( | |||
| ITextSnapshot textSnapshot, | |||
| SpanTrackingMode trackingMode, | |||
| IEnumerable<TagSpan<TTag>>? values1 = null, | |||
| IEnumerable<TagSpan<TTag>>? values2 = null) | |||
| SegmentedList<TagSpan<TTag>> values) | |||
There was a problem hiding this comment.
made nothing optional. made it so only one set of values is passed. made it so it has to be a SegmentedList (so we can sort it easily).
| @@ -49,7 +49,6 @@ private sealed partial class TagSource | |||
| /// </summary> | |||
| private const int CoalesceDifferenceCount = 10; | |||
|
|
|||
| private static readonly ObjectPool<SegmentedList<TagSpan<TTag>>> s_tagSpanListPool = new(() => new(), trimOnFree: false); | |||
There was a problem hiding this comment.
switched to an existing pool we already have for SegmentedLists (which already has the "do not trim on free" concept).
| @@ -450,21 +460,21 @@ static void CheckSnapshot(ITextSnapshot snapshot) | |||
| foreach (var spanToTag in context.SpansToTag) | |||
| buffersToTag.Add(spanToTag.SnapshotSpan.Snapshot.TextBuffer); | |||
|
|
|||
| using var _2 = s_tagSpanListPool.GetPooledObject(out var newTagsInBuffer); | |||
| using var _2 = SegmentedListPool.GetPooledList<TagSpan<TTag>>(out var newTagsInBuffer_safeToMutate); | |||
There was a problem hiding this comment.
renamed to make it very clear that it's ok and expected for this to be mutated in the loops below (including past initially populating it).
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/IntervalTree`1.cs
Outdated
Show resolved
Hide resolved
| CreateFromSortedWorker(values, mid + 1, endExclusive, in introspector), | ||
| in introspector); | ||
|
|
||
| return node; |
There was a problem hiding this comment.
for anyone reviewing this. To convince yourself of correctness, simply see how 'Insert' works:
private static Node Insert<TIntrospector>(Node? root, Node newNode, int newNodeStart, in TIntrospector introspector)
where TIntrospector : struct, IIntervalIntrospector<T>
{
if (root == null)
{
return newNode;
}
Node? newLeft, newRight;
if (newNodeStart < introspector.GetSpan(root.Value).Start)
{
newLeft = Insert(root.Left, newNode, newNodeStart, in introspector);
newRight = root.Right;
}
else
{
newLeft = root.Left;
newRight = Insert(root.Right, newNode, newNodeStart, in introspector);
}
root.SetLeftRight(newLeft, newRight, in introspector);
var newRoot = root;
return Balance(newRoot, in introspector);
}The logic there is trivial. We simply examine the start of the element we're adding and the start of the node we have to determine down which path to place the new node. So that same 'sorting' is accomplished by an up front real sort of the items.
Then, once we've placed things down the correct path, 'set' the left/right elements (which picks the right height value for the node we're creating, as well as the right 'max end node'). That same 'setting' happens in the new code.
What chnages those is that we need no balancing. because we're always building from another sorted sub-node, so the tree is naturally balanced enough to efficiently search without any costly steps while building it.
There was a problem hiding this comment.
I need to think this through a bit, but I'll ask before I reach understanding of this code: Why couldn't this just store an array and not need the actual tree structure?
There was a problem hiding this comment.
We just had that conversation in person :-)
There was a problem hiding this comment.
Sometimes I just nod and act like I'm listening to you :)
| values1, values2); | ||
| // Sort the values by their start position. This is extremely fast (defer'ing to the runtime's sorting | ||
| // routines), and allows us to build the balanced tree directly without having to do any additional work. | ||
| values.Sort(static (t1, t2) => t1.Span.Start.Position - t2.Span.Start.Position); |
There was a problem hiding this comment.
Correct. The is to figure out left/right solely on start.
There was a problem hiding this comment.
Which you'll see is what Insert does as well.
| args.allTags.Clear(); | ||
| var (@this, e, tagsToRemove, allTagsList, allTagsSet) = args; | ||
|
|
||
| // Compre-and-swap loops until we can successfully update the tag trees. Clear out the collections |
| AddAll(in introspector, values2); | ||
| if (values != null) | ||
| { | ||
| foreach (var value in values) |
There was a problem hiding this comment.
I want to do that in follow-up (also where I get rid of trees)
|
|
||
| // Everything is sorted, and we're always building a node up from equal subtrees. So we're never unbalanced | ||
| // enough to require balancing here. | ||
| var balanceFactor = BalanceFactor(node); |
Drops us from:
to