-
Notifications
You must be signed in to change notification settings - Fork 199
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.
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
Improve formatting performance #8243
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had no idea the Children
property was not part of the generated syntax node code, and was committing such crimes. Shocked pikachu.
Tooling side looks fine to me. Thanks for the awesome improvements!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done review pass (commit 4). I only looked at the compiler changes, not the IDE side.
src/Compiler/Microsoft.AspNetCore.Razor.Language/src/Legacy/LegacySyntaxNodeExtensions.cs
Outdated
Show resolved
Hide resolved
...piler/Microsoft.AspNetCore.Razor.Language/src/Legacy/LegacySyntaxNodeExtensions.NodeStack.cs
Outdated
Show resolved
Hide resolved
...piler/Microsoft.AspNetCore.Razor.Language/src/Legacy/LegacySyntaxNodeExtensions.NodeStack.cs
Outdated
Show resolved
Hide resolved
...piler/Microsoft.AspNetCore.Razor.Language/src/Legacy/LegacySyntaxNodeExtensions.NodeStack.cs
Outdated
Show resolved
Hide resolved
...piler/Microsoft.AspNetCore.Razor.Language/src/Legacy/LegacySyntaxNodeExtensions.NodeStack.cs
Outdated
Show resolved
Hide resolved
src/Compiler/Microsoft.AspNetCore.Razor.Language/src/Legacy/LegacySyntaxNodeExtensions.cs
Show resolved
Hide resolved
src/Compiler/Microsoft.AspNetCore.Razor.Language/src/Legacy/LegacySyntaxNodeExtensions.cs
Outdated
Show resolved
Hide resolved
src/Compiler/Microsoft.AspNetCore.Razor.Language/src/Legacy/LegacySyntaxNodeExtensions.cs
Outdated
Show resolved
Hide resolved
src/Compiler/Microsoft.AspNetCore.Razor.Language/src/Legacy/LegacySyntaxNodeExtensions.cs
Outdated
Show resolved
Hide resolved
src/Compiler/Microsoft.AspNetCore.Razor.Language/src/Syntax/MarkupEndTagSyntax.cs
Outdated
Show resolved
Hide resolved
77b1d49
to
4e3fc99
Compare
4e3fc99
to
aebbdf5
Compare
@333fred @dotnet/razor-compiler - This is ready for another review. |
Thanks! |
The "large file" formatting test that I recently checked in is slow - really slow.
That's far too slow for a file that's only ~40 KB in size -- and that's running on Release bits.
Firing up PerfView revealed that the bulk of the time is spent in an internal extension method in the compiler:
LegacySpanExtensions.PreviousSpan(...)
, and it's not hard to see why. This method causes the entire subtree of the targetSyntaxNode
to be realized by callingLastOrDefault(...)
on the result ofLegacySpanExtensions.FlattenSpans(...)
. Even worse, much of that subtree is represented by nodes that are created each timeMarkupStartTagSyntax.Children
andMarkupEndTagSyntax.Children
are called. I've made the following changes, which greatly improve formatting performance -- especially on large inputs.Here is a summary of the changes:
IsXXXSpanKind(...)
extension methods now use hash sets rather than arrays for look up.IsSpanKind(...)
no longer calls all of the otherIsXXXSpanKind(...)
methods. Instead, it uses its own hash set for look up that is unioned with the others.MarkupStartTagSyntax.Children
,MarkupEndTagSyntax.Children
,MarkupTagHelperStartTagSyntax.Children
andMarkupTagHelperEndTagSyntax.Children
are now cached on their red nodes to avoid recomputing them (and allocating new nodes!) on every access.NextSpan
andPreviousSpan
now cache their results in aConditionalWeakTable
.PreviousSpan
calls a privateFlattenSpansInReverse(...)
method that is written to be more efficient than callingFlattenSpans.LastOrDefault(...)
.LocateOwner
no longer boxes struct-based collections before iterating them. This also allows each iteration to use each collection's struct-based enumerator.With these changes, the test runs much faster. It's now much more realistic.
Benchmarks
The formatting benchmarks are also improved, especially on larger inputs and bigger jobs. The improvements are as much as 50% on the final rows in the tables below.
RazorCSharpFormattingBenchmark (main)
RazorCSharpFormattingBenchmark (with changes)