-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
Use Ascii.ToUtf16 in StringUtilities #56578
Conversation
/benchmark plaintext aspnet-citrine-win kestrel |
src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs
Outdated
Show resolved
Hide resolved
/benchmark plaintext aspnet-citrine-win kestrel |
Benchmark started for plaintext on aspnet-citrine-win with kestrel. Logs: link |
plaintext - aspnet-citrine-win
|
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.
LGTM 👍🏻 (code wise)
src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs
Outdated
Show resolved
Hide resolved
cpu goes up and rps goes down, latency goes up?
|
CPU and latency are very bouncy for this benchmark. RPS is within noise range. |
src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs
Outdated
Show resolved
Hide resolved
src/Servers/Kestrel/Core/src/Internal/Infrastructure/HttpUtilities.cs
Outdated
Show resolved
Hide resolved
21be0d5
to
8d76d61
Compare
8d76d61
to
18b966d
Compare
Switches to
Ascii.ToUtf16
over hand rolled vectorized code. This allows us to remove a nice chunk of unsafe+vectorized code in AspNetCore and take advantage of newer features like AVX512 without updating our code.One issue is that our old code would not allow the null character (
\0
) butAscii.ToUtf16
does. To fix this I moved the main null check to when we first iterate over the request line so it should result in an almost free null check (since we're already searching through the whole request) and preserve behavior.See dotnet/runtime#80366 for some discussion on the null character as well as some perf discussion around
Ascii.ToUtf16
vs. our vectorized code. TBF I ran on hardware without AVX512 so I didn't get the benefits from that code path.