Add Razor TagHelper Parsing Benchmarks #23627
Conversation
Benchmarks: | Method | Mean | Error | StdDev | Op/s | Allocated | |---------------------------------------- |------------:|----------:|----------:|---------:|----------:| | 'TagHelper Design Time Processing' | 2,331.51 us | 28.916 us | 27.048 us | 428.9 | 985.33 KB | | 'TagHelper Component Directive Parsing' | 90.46 us | 0.472 us | 0.394 us | 11,055.1 | 3.01 KB | Notes / Attributions: - `BlazorServerTagHelpers` is just a demo file concatonated from the basic `BlazorServer` files - `RazorDiagnosticJsonConverter` is copied from: https://github.com/dotnet/aspnetcore/blob/b7d9e8cfea97c9a3e6df871666d489556e2a3d0b/src/Shared/RazorShared/RazorDiagnosticJsonConverter.cs - `TagHelperDescriptorJsonConverter` is copied from: https://github.com/dotnet/aspnetcore-tooling/blob/73c96f1c00b90e0f9d4fdbfb0905376229ceb913/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Serialization/TagHelperDescriptorJsonConverter.cs - `taghelpers.json` was updated from: https://github.com/dotnet/aspnetcore-tooling/blob/73c96f1c00b90e0f9d4fdbfb0905376229ceb913/src/Razor/benchmarks/Microsoft.AspNetCore.Razor.Performance/taghelpers.json - `ReadTagHelpers` was copied over from https://github.com/dotnet/aspnetcore-tooling/blob/fef50ba62387cb87d34970b4c88290c233361927/src/Razor/benchmarks/Microsoft.AspNetCore.Razor.Performance/ProjectSystem/ProjectSnapshotManagerBenchmarkBase.cs#L83-L93 Fixes: https://github.com/dotnet/aspnetcore/issues/23454
TanayParikh
requested review from
ajaybhargavb,
NTaylorMullen,
noahbkim,
ToddGrun and
ryanbrandenburg
ghost
added
the
area-mvc
| @@ -247,8 +247,8 @@ public ComponentDirectiveVisitor(string filePath, IReadOnlyList<TagHelperDescrip | |||
| { | |||
| // If this is a child content tag helper, we want to add it if it's original type is in scope. | |||
| // E.g, if the type name is `Test.MyComponent.ChildContent`, we want to add it if `Test.MyComponent` is in scope. | |||
| TrySplitNamespaceAndType(typeName, out var typeNameTextSpan, out var _); | |||
| typeName = GetTextSpanContent(typeNameTextSpan, typeName); | |||
| TrySplitNamespaceAndType(typeName, out var namespaceTextSpan, out var _); | |||
There was a problem hiding this comment.
Are these just renames?
There was a problem hiding this comment.
To clarify that 3KB -> 1KB was a hypothetical if we weren't to do substringing at all. We've kept the substringing in place as we need to be able to extract the namespace from a fully qualified name. The point I was trying to make was that this area has already been optimized substantially (from 27.48KB to 3KB) so any further optimization will have diminishing returns. Note; the ETL's which instigated the issue were from prior to this change. |
|
|
||
| namespace Microsoft.VisualStudio.LanguageServices.Razor.Serialization | ||
| { | ||
| internal class RazorDiagnosticJsonConverter : JsonConverter |
There was a problem hiding this comment.
You shouldn't need to replicate this. We have a converter in the CodeAnalysis.Workspaces dll already.
There was a problem hiding this comment.
Removed, and updated the Performance.csproj with the file compilation references as we do elsewhere
...erf/Microsoft.AspNetCore.Razor.Performance/Serialization/TagHelperDescriptorJsonConverter.cs
Outdated
Show resolved
Hide resolved
| <None Include="MSN.cshtml" CopyToOutputDirectory="PreserveNewest" /> | ||
| <None Include="taghelpers.json" CopyToOutputDirectory="PreserveNewest" /> | ||
| <None Include="BlazorServerTagHelpers.razor" CopyToOutputDirectory="PreserveNewest" /> | ||
| <Compile Include="$(SharedSourceRoot)RazorShared\TagHelperDescriptorJsonConverter.cs"> |
There was a problem hiding this comment.
There was a problem hiding this comment.
is not updated to reflect the most recent: https://github.com/dotnet/aspnetcore-tooling/blob/73c96f1c00b90e0f9d4fdbfb0905376229ceb913/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Serialization/TagHelperDescriptorJsonConverter.cs
Is this update handled automatically, or do we have to do some transfer?
Fixes: https://github.com/dotnet/aspnetcore/issues/23454
Based on the ETL Traces from:
https://github.com/dotnet/aspnetcore/issues/23108
#23095
This issue turned out to already have been resolved from @ToddGrun's PR which brought major performance improvements in this specific area (6.5% less CPU time and 89% less memory allocation).
Baseline before Todd's changes:
After Todd's Changes:
With No Sub-stringing At All:
Which proves sub-stringing is not an issue.
Added some benchmarks for taghelper parsing so it can help us in potential future investigations.
Benchmarks:
Notes / Attributions:
BlazorServerTagHelpersis just a demo file concatonated from the basicBlazorServerfilesRazorDiagnosticJsonConverteris copied from: https://github.com/dotnet/aspnetcore/blob/b7d9e8cfea97c9a3e6df871666d489556e2a3d0b/src/Shared/RazorShared/RazorDiagnosticJsonConverter.csTagHelperDescriptorJsonConverteris copied from: https://github.com/dotnet/aspnetcore-tooling/blob/73c96f1c00b90e0f9d4fdbfb0905376229ceb913/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Serialization/TagHelperDescriptorJsonConverter.cstaghelpers.jsonwas updated from: https://github.com/dotnet/aspnetcore-tooling/blob/73c96f1c00b90e0f9d4fdbfb0905376229ceb913/src/Razor/benchmarks/Microsoft.AspNetCore.Razor.Performance/taghelpers.jsonReadTagHelperswas copied over from https://github.com/dotnet/aspnetcore-tooling/blob/fef50ba62387cb87d34970b4c88290c233361927/src/Razor/benchmarks/Microsoft.AspNetCore.Razor.Performance/ProjectSystem/ProjectSnapshotManagerBenchmarkBase.cs#L83-L93Fixes: https://github.com/dotnet/aspnetcore/issues/23454