Don't use PreviousSpan method, because it's slow #8807
Conversation
| owner is CSharpStatementLiteralSyntax && | ||
| owner.Parent is CSharpCodeBlockSyntax && | ||
| owner.PreviousSpan() is CSharpTransitionSyntax) | ||
| owner.TryGetPreviousSibling(out var transition) && transition is CSharpTransitionSyntax) |
There was a problem hiding this comment.
This wasn't flagged by PRISM, but removing all usages of PreviousSpan from the LSP editor just makes sense.
|
|
||
| if (owner is CSharpStatementLiteralSyntax && | ||
| owner.PreviousSpan() is { } prevNode && | ||
| owner.TryGetPreviousSibling(out var prevNode) && |
There was a problem hiding this comment.
As above, this wasn't flagged.
src/Razor/src/Microsoft.VisualStudio.Editor.Razor/BraceSmartIndenter.cs
Outdated
Show resolved
Hide resolved
src/Compiler/Microsoft.AspNetCore.Razor.Language/src/Legacy/LegacySyntaxNodeExtensions.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpFormattingPassBase.cs
Outdated
Show resolved
Hide resolved
|
Ping @dotnet/razor-tooling, after a bit of back-and-forth this is a tooling exclusive change |
| var outerNode = owner.GetOutermostNode(); | ||
| if (outerNode is not null && | ||
| outerNode.TryGetPreviousSibling(out var whiteSpace) && whiteSpace.ContainsOnlyWhitespace() && | ||
| whiteSpace.TryGetPreviousSibling(out var comment) && comment is MarkupCommentBlockSyntax) |
There was a problem hiding this comment.
Is this accurate? Previously, it checked literal.PreviousSpan()?.Parent but the parent isn't checked anymore.
There was a problem hiding this comment.
Yep. PreviousSpan would return the close angle of the comment, so needed to get to the parent to check if it was a comment. In this case TryGetPreviousSibling is not only faster, it more closely matches what the code wants to do anyway.
The scenario is covered by a couple of tests here: https://github.com/dotnet/razor/blob/main/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Formatting/HtmlFormattingTest.cs#L1020
4 participants
Fixes #8781 and https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1828376
Context on benchmarks:
I benchmarked a few scenarios with this one, because its a little hard to guess at what the scenario is that PRISM found. All of the benchmarks simulate a full "format" operation, so for each line of the file, they find the first non-whitespace character, and then try to find the node that owns it. This is what the real formatting engine does (multiple times)
ShouldFormatrepeatedly on the same document, it would have some benefit from caching.TryGetPreviousSiblingmethod, which would both have to iterate the tree. After looking at the benchmarks it didn't make enough of an impact to be worthwhile IMO, so left the existing code as its easy to read and debug.The next four benchmarks, that end in "NewSyntaxTree", mirror the above scenarios but the parsing of the syntax tree is part of the iteration. This means absolute times are longer, but levels the playing field, so that there is no benefit from caching at all, including any lazy evaluation the syntax nodes may do.
The "DocType" field specifies which of the four documents I ran with. "Before" and "After" have a
<div>element with 100 attributes, at the start or end of the document, respectively. "BeforeNested" and "AfterNested" have 100 nested<div>elements at the start or end. In all cases, the extra div elements would have had to be scanned by thePreviousSpan()and theTryGetPrevousSibling()methods.Summary:
In the best case, the new code performs on par with the existing code for CPU, and better for memory. In the worst case, it performs much better than the existing code. Given reality is presumably somewhere in the middle, I consider the new code to be enough of a win, and if PRISM continues to find issues we can always take a second look at things.
Benchmark results: