Razor TagHelperDescriptor Hashing #24551
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1/2 PRs for https://github.com/dotnet/aspnetcore/issues/23170 The other PR will be in aspnetcore-tooling to utilize these changes.
TanayParikh
commented
Aug 4, 2020
| @@ -5,7 +5,6 @@ | |||
| using System.Collections.Generic; | |||
| using Microsoft.AspNetCore.Razor.Language; | |||
| using Newtonsoft.Json; | |||
| using Newtonsoft.Json.Linq; | |||
There was a problem hiding this comment.
Shared file update from tooling.
TanayParikh
commented
Aug 4, 2020
TanayParikh
added a commit
to dotnet/razor
that referenced
this pull request
Aug 4, 2020
2/2 PRs for https://github.com/dotnet/aspnetcore/issues/23170 Other PR: dotnet/aspnetcore#24551 - Used the project for a short period of time - Created a file, navigated between files, made some edits. - I ran into 238 cache misses (unique taghelper allocations) and 4612 cache hits - 95%+ cache hit - Scales up incredibly well. Number of TagHelperDescriptors unlikely to change unless - New files added - New assemblies / packages added ### Benchmarks | Method | Mean | Error | StdDev | Op/s | Allocated | |------------------------- |---------:|---------:|---------:|--------:|----------:| | 'TagHelpers GetHashCode' | 0.137 ms | 1.279 us | 1.196 us | 7,306.8 | 17.93 KB | | Method | Mean | Error | StdDev | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated | |----------------------------------------------------|---------:|----------:|----------:|------:|--------:|--------:|--------:|----------:| | 'Cached Razor TagHelper Roundtrip Serialization' | 3.243 ms | 0.0634 ms | 0.0593 ms | 308.4 | 42.9688 | 39.0625 | 35.1563 | 1.95 MB | | 'Uncached Razor TagHelper Roundtrip Serialization' | 4.891 ms | 0.1082 ms | 0.0959 ms | 204.4 | 78.1250 | 54.6875 | 39.0625 | 3.85 MB | Note `GetHashCode` has minimal perf overhead in comparison to the underlying serialization/deserialization cost. As a result we get 33% faster roundtrip serialization and half the memory utilization. | Method | Mean | Error | StdDev | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated | |------------------------------------------ |---------:|----------:|----------:|------:|--------:|--------:|--------:|----------:| | 'Before Razor TagHelper Serialization' | 1.782 ms | 0.0353 ms | 0.0378 ms | 561.3 | 35.1563 | 35.1563 | 35.1563 | 1.11 MB | | 'After Razor TagHelper Serialization' | 1.932 ms | 0.0385 ms | 0.0632 ms | 517.6 | 35.1563 | 35.1563 | 35.1563 | 1.13 MB | | 'Before Razor TagHelper Deserialization' | 4.198 ms | 0.0173 ms | 0.0161 ms | 238.2 | 39.0625 | 15.6250 | - | 3.28 MB | | 'After Razor TagHelper Deserialization' | 4.279 ms | 0.0215 ms | 0.0201 ms | 233.7 | 39.0625 | 15.6250 | - | 3.29 MB | Note, marginally higher CPU/Memory for unidirectional serialization/deserialization due to cost of hashing. Note this benchmark would not benefit for caching.
TanayParikh
requested review from
NTaylorMullen,
ryanbrandenburg and
ajaybhargavb
NTaylorMullen
approved these changes
Aug 4, 2020
src/Razor/Microsoft.AspNetCore.Razor.Language/src/TagHelperDescriptorCache.cs
Outdated
Show resolved
Hide resolved
src/Razor/Microsoft.AspNetCore.Razor.Language/src/TagHelperDescriptorCache.cs
Outdated
Show resolved
Hide resolved
NTaylorMullen
suggested changes
Aug 4, 2020
src/Razor/Microsoft.AspNetCore.Razor.Language/src/TagHelperDescriptorCache.cs
Outdated
Show resolved
Hide resolved
TanayParikh
changed the title
NTaylorMullen
approved these changes
Aug 4, 2020
|
Hello @TanayParikh! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
Pilchie
added
the
area-mvc
https://dev.azure.com/dnceng/public/_build/results?buildId=756677&view=ms.vss-test-web.build-test-results-tab&runId=23487428&resultId=102773&paneView=debug > Manifest resource: Microsoft.AspNetCore.Razor.Language.Test.TestFiles.taghelpers.json not found\nExpected: True\nActual: False
ajaybhargavb
reviewed
Aug 4, 2020
src/Razor/Microsoft.AspNetCore.Razor.Language/src/TagHelperDescriptorComparer.cs
Show resolved
Hide resolved
TanayParikh
added a commit
to dotnet/razor
that referenced
this pull request
Aug 5, 2020
* TagHelperDescriptor Hashing and Caching 2/2 PRs for https://github.com/dotnet/aspnetcore/issues/23170 Other PR: dotnet/aspnetcore#24551 - Used the project for a short period of time - Created a file, navigated between files, made some edits. - I ran into 238 cache misses (unique taghelper allocations) and 4612 cache hits - 95%+ cache hit - Scales up incredibly well. Number of TagHelperDescriptors unlikely to change unless - New files added - New assemblies / packages added ### Benchmarks | Method | Mean | Error | StdDev | Op/s | Allocated | |------------------------- |---------:|---------:|---------:|--------:|----------:| | 'TagHelpers GetHashCode' | 0.137 ms | 1.279 us | 1.196 us | 7,306.8 | 17.93 KB | | Method | Mean | Error | StdDev | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated | |----------------------------------------------------|---------:|----------:|----------:|------:|--------:|--------:|--------:|----------:| | 'Cached Razor TagHelper Roundtrip Serialization' | 3.243 ms | 0.0634 ms | 0.0593 ms | 308.4 | 42.9688 | 39.0625 | 35.1563 | 1.95 MB | | 'Uncached Razor TagHelper Roundtrip Serialization' | 4.891 ms | 0.1082 ms | 0.0959 ms | 204.4 | 78.1250 | 54.6875 | 39.0625 | 3.85 MB | Note `GetHashCode` has minimal perf overhead in comparison to the underlying serialization/deserialization cost. As a result we get 33% faster roundtrip serialization and half the memory utilization. | Method | Mean | Error | StdDev | Op/s | Gen 0 | Gen 1 | Gen 2 | Allocated | |------------------------------------------ |---------:|----------:|----------:|------:|--------:|--------:|--------:|----------:| | 'Before Razor TagHelper Serialization' | 1.782 ms | 0.0353 ms | 0.0378 ms | 561.3 | 35.1563 | 35.1563 | 35.1563 | 1.11 MB | | 'After Razor TagHelper Serialization' | 1.932 ms | 0.0385 ms | 0.0632 ms | 517.6 | 35.1563 | 35.1563 | 35.1563 | 1.13 MB | | 'Before Razor TagHelper Deserialization' | 4.198 ms | 0.0173 ms | 0.0161 ms | 238.2 | 39.0625 | 15.6250 | - | 3.28 MB | | 'After Razor TagHelper Deserialization' | 4.279 ms | 0.0215 ms | 0.0201 ms | 233.7 | 39.0625 | 15.6250 | - | 3.29 MB | Note, marginally higher CPU/Memory for unidirectional serialization/deserialization due to cost of hashing. Note this benchmark would not benefit for caching. * Add duplicate key safeguard in memory cache https://dev.azure.com/dnceng/public/_build/results?buildId=757405&view=ms.vss-test-web.build-test-results-tab&runId=23507082&resultId=100079&paneView=debug > System.ArgumentException : The key already existed in the dictionary.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
1/2 PRs for https://github.com/dotnet/aspnetcore/issues/23170
TagHelperDescriptorto minimize collisions.TagHelperDescriptormemory cache.The other PR utilizes these changes: dotnet/razor#2307