Reduce allocations in semantic tokens #9280

davidwengier · 2023-09-13T03:34:13Z

Fixes https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1869868

Saw this in one of the slow completion traces. A little unorthodox method here, as our benchmarks weren't well suited for this sort of work. Instead I used the new ability to profile a test, and just added one test that produced a lot of tokens.

Results as reported by the VS profiler:

…tions

davidwengier · 2023-09-13T03:36:02Z

...icrosoft.AspNetCore.Razor.LanguageServer/Semantic/Services/RazorSemanticTokensInfoService.cs

@@ -175,7 +183,7 @@ private static List<SemanticRange> CombineSemanticRanges(List<SemanticRange> ran
            var tokenModifiers = csharpResponse[i + 4];

            var semanticRange = CSharpDataToSemanticRange(lineDelta, charDelta, length, tokenType, tokenModifiers, previousSemanticRange);
-            if (_documentMappingService.TryMapToHostDocumentRange(generatedDocument, semanticRange.Range, out var originalRange))
+            if (_documentMappingService.TryMapToHostDocumentRange(generatedDocument, CopyValues(semanticRange, rangePool), out var originalRange))


A new API here that returned something other than Range would further reduce allocations, but seemed like a pretty big change for this late in the process. In future ideally we'll be able to move to auto-generated protocol types that aren't classes anyway, which will solve this too.

Looked at this a bit more and its definitely a big change, just with how much churn it is in tests that use Moq 😁

I take it back, wasn't too bad :)
#9285

davidwengier · 2023-09-13T03:40:42Z

...Microsoft.AspNetCore.Razor.LanguageServer/Semantic/Services/TagHelperSemanticRangeVisitor.cs

@@ -40,11 +40,13 @@ public static List<SemanticRange> VisitAllNodes(RazorCodeDocument razorCodeDocum
            rangeAsTextSpan = range.AsRazorTextSpan(sourceText);
        }

-        var visitor = new TagHelperSemanticRangeVisitor(razorCodeDocument, rangeAsTextSpan, razorSemanticTokensLegend, colorCodeBackground);
+        using var _ = ArrayBuilderPool<SemanticRange>.GetPooledObject(out var builder);


The resizing of this array shows up in the profile as being a slight problem, but with only one test run being profiled there is no pooling benefit, so hopefully in the real world it goes away.

davidwengier · 2023-09-13T03:41:59Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Extensions/RangeExtensions.cs

-            var lineCount = sourceText.Lines.Count;
-            if (line > lineCount ||
-                (line == lineCount && character > 0))
+            if (!sourceText.TryGetAbsolutePosition(line, character, out var absolutePosition))


This logic just moved to SourceTextExtensions, which is a better place anyway, as we can use it in more places to possibly solve some other PRISM bugs for out of range issues, since it's the only thing that currently does the correct "allow line to be count + 1" logic.

davidwengier · 2023-09-13T06:17:11Z

A thought: Would it be a better change if SemanticRange just swapped to using Roslyn's LinePositionSpan instead of Range? It's the same shape, just a struct.

(which I eventually want to remove and just have it call the new one)

ToddGrun · 2023-09-13T13:10:16Z

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Extensions/SourceTextExtensions.cs

+            (line == lineCount && character > 0))
+        {
+            return false;
+        }


nit: I would probably write this something like below (easier for my old brain to read and probably fewer conditions to evaluate in common case)

if (line >= lineCount) { if (line > lineCount || character >0) { return false; } // LSP spec allowed a Range to end one line past the end, and character 0. SourceText does not, so we adjust to the final char position absoluteIndex = sourceText.Length; } else { absoluteIndex = sourceText.Lines[line].Start + character; }

I'm going to leave this for now, since its just a move, and I find that harder to read 😛

I could be convinced about the less conditions though, so I'll have a go at this in a follow up PR, to clean up more Position and Range stuff. This probably needs more logic anyway - just because the line number is valid, doesn't mean the character position on that line is, and there is no check for line > 0.

...icrosoft.AspNetCore.Razor.LanguageServer/Semantic/Services/RazorSemanticTokensInfoService.cs

ToddGrun · 2023-09-13T13:19:20Z

...icrosoft.AspNetCore.Razor.LanguageServer/Semantic/Services/RazorSemanticTokensInfoService.cs

@@ -159,13 +162,18 @@ private static List<SemanticRange> CombineSemanticRanges(List<SemanticRange> ran
            return null;
        }

-        var razorRanges = new List<SemanticRange>();
+        using var _ = ArrayBuilderPool<SemanticRange>.GetPooledObject(out var razorRanges);
+        razorRanges.SetCapacityIfLarger(csharpResponse.Length / TokenSize);


TokenSize

Look at that beautiful constant! :)

ToddGrun · 2023-09-13T13:26:36Z

    if (FromRazor && !other.FromRazor)

If we're embracing CompareTo, might as well use it here too #Closed

Refers to: src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/Services/SemanticRange.cs:64 in ae876c4. [](commit_id = ae876c4, deletion_comment = False)

davidwengier · 2023-09-13T22:22:54Z

If we're embracing CompareTo, might as well use it here too

I prefer the readability of the current code. The equivalent logic would be (!FromRazor).CompareTo(!other.FromRazor) and I'd spend an hour trying to work out what it's doing. Hell, it took me half an hour of experimenting in SharpLab to work out thats what it would be.

ToddGrun · 2023-09-13T22:31:15Z

...icrosoft.AspNetCore.Razor.LanguageServer/Semantic/Services/RazorSemanticTokensInfoService.cs

+        newList.SetCapacityIfLarger(razorRanges.Length + csharpRanges.Length);
+
+        newList.AddRange(razorRanges);
+        newList.AddRange(csharpRanges);

        // Because SemanticToken data is generated relative to the previous token it must be in order.
        // We have a guarantee of order within any given language server, but the interweaving of them can be quite complex.


We have a guarantee of order within any given language server

Is this comment not accurate (and thus the need for you to sort in the just csharp case above?)

Strictly speaking its not wrong, but certainly misleading. Will update.

Within any given language server the order is guaranteed, due to the nature of the offset based data, but that just means its in order when we get it back from the C# server. Once we've done the re-mapping, a using directive which would be at the top of the generated C# file, can produce a classified span in the middle of a .razor document (as the @using can appear anywhere).

ToddGrun

Follow up to #9280 This creates a struct based API for document mapping, and moves semantic tokens to it. I did it in a source-compatible way so we can upgrade existing features as necessary. I suspect most won't get the benefit that semantic tokens gets, as most things just ferry ranges and positions around, and don't process them much. Logged #9284 to follow up though. Commit-at-a-time might be easiest. Results for semantic tokens are pretty good though: ![image](https://github.com/dotnet/razor/assets/754264/7316e5db-0e90-4b32-a807-d4ee5d40741a)

davidwengier added 2 commits September 13, 2023 13:22

Create a test with a large file for profiling purposes

95828e2

Use a struct instead of protocol types during semantic tokens calcula…

5ee99ea

…tions

davidwengier requested review from DustinCampbell, maryamariyan and ToddGrun September 13, 2023 03:34

davidwengier requested a review from a team as a code owner September 13, 2023 03:34

davidwengier commented Sep 13, 2023

View reviewed changes

Rename method to match the one in PositionExtensions

ae876c4

(which I eventually want to remove and just have it call the new one)

ToddGrun reviewed Sep 13, 2023

View reviewed changes

...icrosoft.AspNetCore.Razor.LanguageServer/Semantic/Services/RazorSemanticTokensInfoService.cs Outdated Show resolved Hide resolved

ToddGrun reviewed Sep 13, 2023

View reviewed changes

davidwengier added 2 commits September 14, 2023 07:36

Tweak the large file test so C# tokens arrive out-of-order

759c68a

Fast path if only one type of tokens is needed

0207c14

ToddGrun reviewed Sep 13, 2023

View reviewed changes

Comments

e5f6954

ToddGrun approved these changes Sep 14, 2023

View reviewed changes

davidwengier merged commit 3282025 into dotnet:main Sep 14, 2023

davidwengier deleted the SemanticTokensAllocations branch September 14, 2023 00:30

ghost added this to the Next milestone Sep 14, 2023

davidwengier mentioned this pull request Sep 14, 2023

Further reduce allocations in semantic tokens #9285

Merged

allisonchou mentioned this pull request Sep 19, 2023

[Automated] PRs inserted in VS build main-34118.363 #9300

Closed

Cosifne modified the milestones: Next, 17.8 P3 Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce allocations in semantic tokens #9280

Reduce allocations in semantic tokens #9280

davidwengier commented Sep 13, 2023

davidwengier Sep 13, 2023 •

edited

Loading

davidwengier Sep 13, 2023

davidwengier Sep 14, 2023

davidwengier Sep 13, 2023

davidwengier Sep 13, 2023

davidwengier commented Sep 13, 2023

ToddGrun Sep 13, 2023

davidwengier Sep 13, 2023

ToddGrun Sep 13, 2023

ToddGrun commented Sep 13, 2023 •

edited

Loading

davidwengier commented Sep 13, 2023

ToddGrun Sep 13, 2023

davidwengier Sep 13, 2023

ToddGrun left a comment

Reduce allocations in semantic tokens #9280

Reduce allocations in semantic tokens #9280

Conversation

davidwengier commented Sep 13, 2023

davidwengier Sep 13, 2023 • 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

davidwengier commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ToddGrun commented Sep 13, 2023 • edited Loading

davidwengier commented Sep 13, 2023

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

davidwengier Sep 13, 2023 •

edited

Loading

ToddGrun commented Sep 13, 2023 •

edited

Loading