Update Fortunes to use Razor templating#1801
Merged
DamianEdwards merged 9 commits intomainfrom Mar 13, 2023
Merged
Conversation
sebastienros
reviewed
Mar 1, 2023
src/BenchmarksApps/TechEmpower/PlatformBenchmarks/PlatformBenchmarks.csproj
Show resolved
Hide resolved
Updates the Fortunes implementations for Minimal and Platform to use Razor .cshtml templates with custom base classes intended for rendering using the RazorSlices package (no MVC).
306fe89 to
aba8713
Compare
roji
reviewed
Mar 10, 2023
src/BenchmarksApps/TechEmpower/PlatformBenchmarks/BenchmarkApplication.Fortunes.cs
Outdated
Show resolved
Hide resolved
src/BenchmarksApps/TechEmpower/PlatformBenchmarks/PlatformBenchmarks.csproj
Show resolved
Hide resolved
src/BenchmarksApps/TechEmpower/PlatformBenchmarks/BenchmarkApplication.Fortunes.cs
Outdated
Show resolved
Hide resolved
BrennanConroy
approved these changes
Mar 10, 2023
src/BenchmarksApps/TechEmpower/PlatformBenchmarks/ChunkedBufferWriter.cs
Outdated
Show resolved
Hide resolved
roji
approved these changes
Mar 11, 2023
davidfowl
reviewed
Mar 12, 2023
davidfowl
reviewed
Mar 12, 2023
davidfowl
reviewed
Mar 12, 2023
src/BenchmarksApps/TechEmpower/PlatformBenchmarks/ChunkedBufferWriter.cs
Outdated
Show resolved
Hide resolved
src/BenchmarksApps/TechEmpower/PlatformBenchmarks/ChunkedBufferWriter.cs
Outdated
Show resolved
Hide resolved
sebastienros
approved these changes
Mar 13, 2023
benaadams
reviewed
Mar 13, 2023
| } | ||
|
|
||
| private void OutputFortunes(PipeWriter pipeWriter, List<Fortune> model) | ||
| private ValueTask OutputFortunes<TModel>(PipeWriter pipeWriter, TModel model, SliceFactory<TModel> templateFactory) |
Contributor
There was a problem hiding this comment.
Might as well just return Task here and then pass through in the other methods if you are just going to await in those ones?
Member
There was a problem hiding this comment.
Those methods also need to await the LoadFortunesRows() calls so you can't just return the Task.
Contributor
There was a problem hiding this comment.
nvm you have to await anyway
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
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.
UPDATED: This has been updated on top of @roji's UTF8 changes. Please take a look!
Updates the Fortunes implementations for Minimal and Platform to use Razor .cshtml templates with custom base classes intended for rendering without MVC.
Minimal Results
The new Fortunes Razor implementation for Minimal APIs shows an 88% improvement, possibly due to a large reduction in lock contention and more efficient rendering (e.g. no
int.ToString(), optimizedHtmlEncodersettings, etc.):Platform Results
The new Platform-level Fortunes Razor implementation shows a drop of only 2.4% from the previous non-templated platform implementation from before @roji's UTF8 changes, and still above 460K RPS: