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

Reduce allocations in semantic tokens #9280

Merged
merged 6 commits into from
Sep 14, 2023
Merged

Reduce allocations in semantic tokens #9280

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
95828e2
Create a test with a large file for profiling purposes
davidwengier Sep 13, 2023
5ee99ea
Use a struct instead of protocol types during semantic tokens calcula…
davidwengier Sep 13, 2023
ae876c4
Rename method to match the one in PositionExtensions
davidwengier Sep 13, 2023
759c68a
Tweak the large file test so C# tokens arrive out-of-order
davidwengier Sep 13, 2023
0207c14
Fast path if only one type of tokens is needed
davidwengier Sep 13, 2023
e5f6954
Comments
davidwengier Sep 13, 2023
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
5 changes: 3 additions & 2 deletions ...Microsoft.AspNetCore.Razor.Microbenchmarks/LanguageServer/RazorSemanticTokensBenchmark.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.IO;
using System.Linq;
using System.Threading;
Expand Down Expand Up @@ -141,7 +142,7 @@ public TestRazorSemanticTokensInfoService(
}

// We can't get C# responses without significant amounts of extra work, so let's just shim it for now, any non-Null result is fine.
internal override Task<List<SemanticRange>> GetCSharpSemanticRangesAsync(
internal override Task<ImmutableArray<SemanticRange>?> GetCSharpSemanticRangesAsync(
RazorCodeDocument codeDocument,
TextDocumentIdentifier textDocumentIdentifier,
Range razorRange,
Expand All @@ -151,7 +152,7 @@ internal override Task<List<SemanticRange>> GetCSharpSemanticRangesAsync(
CancellationToken cancellationToken,
string previousResultId = null)
{
return Task.FromResult(new List<SemanticRange>());
return Task.FromResult<ImmutableArray<SemanticRange>?>(ImmutableArray<SemanticRange>.Empty);
}
}
}
17 changes: 9 additions & 8 deletions ...NetCore.Razor.Microbenchmarks/LanguageServer/RazorSemanticTokensRangeEndpointBenchmark.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

using System;
using System.Collections.Generic;
using System.Collections.Immutable;
using System.IO;
using System.Linq;
using System.Threading;
Expand Down Expand Up @@ -56,7 +57,7 @@ public class RazorSemanticTokensRangeEndpointBenchmark : RazorLanguageServerBenc
[Params(0, 100, 1000)]
public int NumberOfCsSemanticRangesToReturn { get; set; }

private static List<SemanticRange> PregeneratedRandomSemanticRanges { get; set; }
private static ImmutableArray<SemanticRange>? PregeneratedRandomSemanticRanges { get; set; }

[GlobalSetup]
public async Task InitializeRazorSemanticAsync()
Expand Down Expand Up @@ -93,7 +94,7 @@ public async Task InitializeRazorSemanticAsync()

var random = new Random();
var codeDocument = await DocumentContext.GetCodeDocumentAsync(CancellationToken);
PregeneratedRandomSemanticRanges = new List<SemanticRange>(NumberOfCsSemanticRangesToReturn);
var builder = ImmutableArray.CreateBuilder<SemanticRange>(NumberOfCsSemanticRangesToReturn);
for (var i = 0; i < NumberOfCsSemanticRangesToReturn; i++)
{
var startLine = random.Next(Range.Start.Line, Range.End.Line);
Expand All @@ -103,11 +104,11 @@ public async Task InitializeRazorSemanticAsync()
? random.Next(startChar, codeDocument.Source.Lines.GetLineLength(startLine))
: random.Next(0, codeDocument.Source.Lines.GetLineLength(endLine));

PregeneratedRandomSemanticRanges.Add(
new SemanticRange(random.Next(),
new Range { Start = new Position(startLine, startChar), End = new Position(endLine, endChar) },
0, fromRazor: false));
builder.Add(
new SemanticRange(random.Next(), startLine, startChar, endLine, endChar, 0, fromRazor: false));
}

PregeneratedRandomSemanticRanges = builder.DrainToImmutable();
}

[Benchmark(Description = "Razor Semantic Tokens Range Endpoint")]
Expand Down Expand Up @@ -161,7 +162,7 @@ public TestCustomizableRazorSemanticTokensInfoService(
}

// We can't get C# responses without significant amounts of extra work, so let's just shim it for now, any non-Null result is fine.
internal override Task<List<SemanticRange>> GetCSharpSemanticRangesAsync(
internal override Task<ImmutableArray<SemanticRange>?> GetCSharpSemanticRangesAsync(
RazorCodeDocument codeDocument,
TextDocumentIdentifier textDocumentIdentifier,
Range razorRange,
Expand All @@ -171,7 +172,7 @@ internal override Task<List<SemanticRange>> GetCSharpSemanticRangesAsync(
CancellationToken cancellationToken,
string previousResultId = null)
{
return Task.FromResult(PregeneratedRandomSemanticRanges);
return Task.FromResult<ImmutableArray<SemanticRange>?>(PregeneratedRandomSemanticRanges);
}
}
}
20 changes: 6 additions & 14 deletions src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Extensions/RangeExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,8 +129,8 @@ public static TextSpan AsTextSpan(this Range range, SourceText sourceText)
throw new ArgumentNullException(nameof(sourceText));
}

var start = GetAbsolutePosition(range.Start, sourceText);
var end = GetAbsolutePosition(range.End, sourceText);
var start = GetAbsoluteIndex(range.Start, sourceText);
var end = GetAbsoluteIndex(range.End, sourceText);

var length = end - start;
if (length < 0)
Expand All @@ -140,24 +140,16 @@ public static TextSpan AsTextSpan(this Range range, SourceText sourceText)

return new TextSpan(start, length);

static int GetAbsolutePosition(Position position, SourceText sourceText, [CallerArgumentExpression(nameof(position))] string? argName = null)
static int GetAbsoluteIndex(Position position, SourceText sourceText, [CallerArgumentExpression(nameof(position))] string? argName = null)
{
var line = position.Line;
var character = position.Character;
var lineCount = sourceText.Lines.Count;
if (line > lineCount ||
(line == lineCount && character > 0))
if (!sourceText.TryGetAbsoluteIndex(line, character, out var absolutePosition))
{
throw new ArgumentOutOfRangeException($"{argName} ({line},{character}) matches or exceeds SourceText boundary {lineCount}.");
throw new ArgumentOutOfRangeException($"{argName} ({line},{character}) matches or exceeds SourceText boundary {sourceText.Lines.Count}.");
}

// 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
if (line == lineCount)
{
return sourceText.Length;
}

return sourceText.Lines[line].Start + character;
return absolutePosition;
}
}

Expand Down
23 changes: 23 additions & 0 deletions src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Extensions/SourceTextExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,4 +201,27 @@ public static bool NonWhitespaceContentEquals(this SourceText source, SourceText

return null;
}

public static bool TryGetAbsoluteIndex(this SourceText sourceText, int line, int character, out int absoluteIndex)
{
absoluteIndex = 0;
var lineCount = sourceText.Lines.Count;
if (line > lineCount ||
(line == lineCount && character > 0))
{
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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;
            }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


// 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
if (line == lineCount)
{
absoluteIndex = sourceText.Length;
}
else
{
absoluteIndex = sourceText.Lines[line].Start + character;
}

return true;
}
}
Loading