-
Notifications
You must be signed in to change notification settings - Fork 241
Stop UTF8 decoding/reencoding in Fortunes platform #1806
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
Conversation
src/BenchmarksApps/TechEmpower/PlatformBenchmarks/BenchmarkApplication.Fortunes.cs
Outdated
Show resolved
Hide resolved
Wow just the increase from not sorting is surprising (although I guess it shouldn't be). As for sorting over the Utf8 bytes, I have to imagine that's something we have implemented in the framework somewhere already, or at least a utility we can call into? |
As for the sorting, I asked internally and got an answer from @stephentoub that |
Thanks @DamianEdwards, will do all that tomorrow, and look at the other benchmarks too. |
ca9205a
to
1c7da39
Compare
New Fortunes version up, with sorting and with direct HTML encoding into our output buffer (no additional copy via stackalloc). This gives us a 6.94% increase, really nice! Will clean this up and look at the other benchmarks.
|
Silly me, I forgot that Fortunes is the only benchmark involving strings. Cleaned this PR up, this should be ready for merging. |
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.
Nice improvement! I'll rebase my Razor PR on these changes and hopefully the improvement sticks 😄
src/BenchmarksApps/TechEmpower/PlatformBenchmarks/BenchmarkApplication.Fortunes.cs
Outdated
Show resolved
Hide resolved
src/BenchmarksApps/TechEmpower/PlatformBenchmarks/BenchmarkApplication.Fortunes.cs
Show resolved
Hide resolved
@DamianEdwards did a bit more cleanup, holding FYI I'm getting lots of variance in benchmark results: I just got 523k RPS (+13.28%), whereas in a previous run before this change I got only +2%... Hopefully there isn't something screwing up our stability here /cc @sebastienros. But in any case this is definitely an improvement, regardless of exactly how much. Will go ahead and merge. |
I've found this too for the Fortunes benchmark when doing the Razor Slices PR. I agree it would be nice to understand why it's so noisy as like you saw, sometimes you can get a really good run that is more than the value of the improvement you're trying to achieve, which makes it harder to judge if it's really better or not. |
Here's a hacky attempt to do UTF8 all the way in Fortunes; since PostgreSQL delivers text encoded in UTF8, it's silly to decode it to .NET UTF-16 strings, only to re-encode it back to UTF8 for sending to the web client. tl;dr this seems to bring a 4.19% throughput increase.
Npgsql already supports reading strings as raw binary data:
This all works pretty well; the one problem is the the Fortunes scenario mandates that we sort the returned message. I'm not sure if there's a built-in way to compare UTF8 strings (as byte[]), maybe it's OK to compare byte (wink wink)? IIRC other scenarios don't have sorting requirements so this may be more appropriate there.
For now I removed sorting from both the baseline and the UTF8 version for an apples-to-apples comparison. 4.19% is maybe a bit disappointing, I'll take a look with perfivew to ensure everything s what it should be.
Results (remember: both are without sortng and so not actual Fortunes results):
/cc @DamianEdwards @davidfowl @ajcvickers
/cc @vonzshik @NinoFloris @Brar