Convert interval-tree type to a struct #73676
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| { | ||
| public readonly TTag Tag = ts.Tag; | ||
| public readonly ITrackingSpan Span = ts.Span.CreateTrackingSpan(trackingMode); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@ToddGrun ptal. |
| } | ||
|
|
||
| public int GetStart(ITextSnapshot textSnapshot) |
There was a problem hiding this comment.
Duh, that's what GetTranslatedSpan is. I guess the question is why even have GetStart/GetLength
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
i think we need to chat.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/IntervalTree`1.cs
Outdated
Show resolved
Hide resolved
…ections/IntervalTree`1.cs
src/Workspaces/SharedUtilitiesAndExtensions/Compiler/Core/Collections/IntervalTree`1.cs
Outdated
Show resolved
Hide resolved
…ections/IntervalTree`1.cs
This drops us from a total of 4.7% of all allocations:
Down to just 2.1%