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.

Sign up for GitHub

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

Jump to bottom

Convert interval-tree type to a struct #73676

Merged
merged 8 commits into from
May 25, 2024
Merged

Convert interval-tree type to a struct #73676

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
975ba0a
Convert type to a struct
CyrusNajmabadi May 23, 2024
423874f
Simpler
CyrusNajmabadi May 23, 2024
0f2acd9
Simplify
CyrusNajmabadi May 23, 2024
7fd9575
Simplify
CyrusNajmabadi May 23, 2024
69060d8
Simplify
CyrusNajmabadi May 24, 2024
01115ee
Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Coll…
CyrusNajmabadi May 24, 2024
8ac12be
Update src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Coll…
CyrusNajmabadi May 24, 2024
bb87e4d
Remove
CyrusNajmabadi May 24, 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
47 changes: 17 additions & 30 deletions src/EditorFeatures/Core/Shared/Tagging/Utilities/TagSpanIntervalTree.TagNode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,49 +2,36 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using Microsoft.CodeAnalysis.Editor.Shared.Extensions;
using Microsoft.VisualStudio.Text;
using Microsoft.VisualStudio.Text.Tagging;

namespace Microsoft.CodeAnalysis.Editor.Shared.Tagging;

internal partial class TagSpanIntervalTree<TTag>
{
private class TagNode(ITagSpan<TTag> ts, SpanTrackingMode trackingMode)
private readonly struct TagNode(ITagSpan<TTag> tagSpan, SpanTrackingMode trackingMode)
{
public readonly TTag Tag = ts.Tag;
public readonly ITrackingSpan Span = ts.Span.CreateTrackingSpan(trackingMode);
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: i intentionally removed the up front allocation of the tracking span. The tracking is done on demand, only for the short period of times where we have a tag and want to map it forward, and haven't produced the updated the new tags yet. I tried typing as well, and didn't see any of the tracking even showing up. This is likely because we only ever have a short distance between the original text snapshot and the latest one. So tracking from the original to the latest is extremely fast and simple without hte need for a dedicated allocated tracking span.

Copy link
Member Author

Choose a reason for hiding this comment

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

i also didn't move to a model where we compute and cache this on demand because i wanted to make this a struct, and having this be a mutable struct was non-trivial in terms of ensuring it could mutate through all the interfaces and dispatch pattern we work with.

private SnapshotSpan _snapshotSpan = ts.Span;
private readonly ITagSpan<TTag> _originalTagSpan = tagSpan;
private readonly SpanTrackingMode _trackingMode = trackingMode;

private SnapshotSpan GetSnapshotSpan(ITextSnapshot textSnapshot)
{
var localSpan = _snapshotSpan;
if (localSpan.Snapshot == textSnapshot)
{
return localSpan;
}
else if (localSpan.Snapshot != null)
{
_snapshotSpan = default;
}

return default;
}
public TTag Tag => _originalTagSpan.Tag;

internal int GetStart(ITextSnapshot textSnapshot)
{
var localSpan = this.GetSnapshotSpan(textSnapshot);
return localSpan.Snapshot == textSnapshot
? localSpan.Start
: this.Span.GetStartPoint(textSnapshot);
}
public TagSpan<TTag> GetTranslatedTagSpan(ITextSnapshot textSnapshot)
=> new(GetTranslatedSpan(textSnapshot), Tag);

internal int GetLength(ITextSnapshot textSnapshot)
public SnapshotSpan GetTranslatedSpan(ITextSnapshot textSnapshot)
{
var localSpan = this.GetSnapshotSpan(textSnapshot);
var localSpan = _originalTagSpan.Span;

return localSpan.Snapshot == textSnapshot
? localSpan.Length
: this.Span.GetSpan(textSnapshot).Length;
? localSpan
: localSpan.TranslateTo(textSnapshot, _trackingMode);
}

public int GetStart(ITextSnapshot textSnapshot)
Copy link
Contributor

Choose a reason for hiding this comment

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

GetStart

Could this just be a GetSpan method instead, encouraging callers to not need to call both of these (which might help in the case where a translation is necessary)

Copy link
Contributor

Choose a reason for hiding this comment

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

Duh, that's what GetTranslatedSpan is. I guess the question is why even have GetStart/GetLength

Copy link
Member Author

Choose a reason for hiding this comment

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

the way interval trees work, we have a lot of algorithms that don't need the full span. that said, i'll clean this up a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if they don't need the full span, isn't calling GetTranslatedSpan the same cost as calling GetStart or GetLength individually?

The thought I had was if the GetStart/GetLength methods were removed, then callers that need one will just call the GetTranslatedSpan and use the appropriate Length/Start. Callers that need both can be trivially changed to just call GetTranslatedSpan once and use that result to get both the length and start.

Copy link
Member Author

Choose a reason for hiding this comment

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

i think we need to chat.

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: my primary goals in all of these are not to make large scale refactorings to the components, but to address the stuff that profiles are calling out as a problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I get that. This is definitely better, and I'm going to go ahead and approve.

Maybe I'm a bit confused on what this object is doing, I might take a peek once your changes are in.

=> GetTranslatedSpan(textSnapshot).Start;

public int GetLength(ITextSnapshot textSnapshot)
=> GetTranslatedSpan(textSnapshot).Length;
}
}
4 changes: 2 additions & 2 deletions src/EditorFeatures/Core/Shared/Tagging/Utilities/TagSpanIntervalTree.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,11 @@ private void AppendIntersectingSpansInSortedOrder(SnapshotSpan snapshotSpan, Seg
snapshotSpan.Start, snapshotSpan.Length, ref intersectingIntervals.AsRef(), new IntervalIntrospector(snapshot));

foreach (var tagNode in intersectingIntervals)
result.Add(new TagSpan<TTag>(tagNode.Span.GetSpan(snapshot), tagNode.Tag));
result.Add(tagNode.GetTranslatedTagSpan(snapshot));
}

public IEnumerable<ITagSpan<TTag>> GetSpans(ITextSnapshot snapshot)
=> _tree.Select(tn => new TagSpan<TTag>(tn.Span.GetSpan(snapshot), tn.Tag));
=> _tree.Select(tn => tn.GetTranslatedTagSpan(snapshot));

public bool IsEmpty()
=> _tree.IsEmpty();
Expand Down
24 changes: 12 additions & 12 deletions src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/IntervalTree`1.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using System.Diagnostics.CodeAnalysis;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Text;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.Shared.Collections;
Expand Down Expand Up @@ -54,10 +55,11 @@ protected static bool Contains<TIntrospector>(T value, int start, int length, in
var otherStart = start;
var otherEnd = start + length;

var thisEnd = GetEnd(value, in introspector);
var thisStart = introspector.GetStart(value);
var thisEnd = thisStart + introspector.GetLength(value);

// make sure "Contains" test to be same as what TextSpan does
// TODO(cyrusn): This doesn't actually seem to match what TextSpan.Contains does. It doesn't specialize empty
// length in any way. Preserving this behavior for now, but we should consider changing this.
if (length == 0)
{
return thisStart <= otherStart && otherEnd < thisEnd;
Expand All @@ -69,13 +71,7 @@ protected static bool Contains<TIntrospector>(T value, int start, int length, in
private static bool IntersectsWith<TIntrospector>(T value, int start, int length, in TIntrospector introspector)
where TIntrospector : struct, IIntervalIntrospector<T>
{
var otherStart = start;
var otherEnd = start + length;

var thisEnd = GetEnd(value, in introspector);
var thisStart = introspector.GetStart(value);

return otherStart <= thisEnd && otherEnd >= thisStart;
return thisSpan.IntersectsWith(otherSpan);
CyrusNajmabadi marked this conversation as resolved.
Show resolved Hide resolved
}

private static bool OverlapsWith<TIntrospector>(T value, int start, int length, in TIntrospector introspector)
Expand All @@ -84,13 +80,13 @@ private static bool OverlapsWith<TIntrospector>(T value, int start, int length,
var otherStart = start;
var otherEnd = start + length;

var thisEnd = GetEnd(value, in introspector);
var thisStart = introspector.GetStart(value);
var thisEnd = thisStart + introspector.GetLength(value);

// TODO(cyrusn): This doesn't actually seem to match what TextSpan.OverlapsWith does. It doesn't specialize empty
// length in any way. Preserving this behavior for now, but we should consider changing this.
if (length == 0)
{
return thisStart < otherStart && otherStart < thisEnd;
}

var overlapStart = Math.Max(thisStart, otherStart);
var overlapEnd = Math.Min(thisEnd, otherEnd);
Expand Down Expand Up @@ -371,6 +367,10 @@ public IEnumerator<T> GetEnumerator()
IEnumerator IEnumerable.GetEnumerator()
=> this.GetEnumerator();

protected static TextSpan GetSpan<TIntrospector>(T value, in TIntrospector introspector)
where TIntrospector : struct, IIntervalIntrospector<T>
=> new(introspector.GetStart(value), introspector.GetLength(value));

protected static int GetEnd<TIntrospector>(T value, in TIntrospector introspector)
where TIntrospector : struct, IIntervalIntrospector<T>
=> introspector.GetStart(value) + introspector.GetLength(value);
Expand Down