-
Notifications
You must be signed in to change notification settings - Fork 10.5k
HttpResponseHeaders.CopyToFast move directly to next rather than if branching
#32337
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
HttpResponseHeaders.CopyToFast move directly to next rather than if branching
#32337
Conversation
if branchingif branching
4591a6c to
5b87ba3
Compare
36c9824 to
1d6783d
Compare
|
/azp run |
|
Commenter does not have sufficient privileges for PR 32337 in repo dotnet/aspnetcore |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
Looks like CI has its own issues |
5de29f2 to
3cd78db
Compare
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.
CopyToFast is used by HTTP/1.1, but HTTP/2 uses the generated Enumerator types when encoding HPack response headers.
Can this improvement be applied to how the generated Enumerator types decide next? Writing response headers in HTTP/2 is a hotspot in our current implementation. Multiplexing and HPack means that only one response can write headers at a time while other responses wait on a lock. Reducing the time in the lock is huge.
src/Servers/Kestrel/test/InMemory.FunctionalTests/BadHttpRequestTests.cs
Outdated
Show resolved
Hide resolved
Yes, though would do it in follow up |
3cd78db to
7609129
Compare
|
Rebased for merge conflict |
|
7609129 to
aaa9f22
Compare
|
Rebased to retrigger glitchy CI |
|
I've rerun the build a couple of times and it is consistently failing. The Windows build is timing out. |
Microbenchmarks validation is likely hanging. Something changed to make a single run of one or more of the the benchmarks hang.
|
|
In hung_dotnet.exe_2596_210608_031244.dmp I see a thread hung at ConsoleLifetime.OnProcessExit waiting for the host be be disposed. The hung process has a base directory of D:\workspace_work\1\s\artifacts\bin\Microsoft.AspNetCore.Server.Kestrel.Microbenchmarks\Release\net6.0\0 Main thread callstack: InMemoryTransportBenchmark looks like the only benchmark that creates a host. It does dispose it in GlobalCleanup, but maybe that's not happening or its failing? aspnetcore/src/Servers/Kestrel/perf/Microbenchmarks/InMemoryTransportBenchmark.cs Lines 45 to 59 in cee22ed
aspnetcore/src/Servers/Kestrel/perf/Microbenchmarks/InMemoryTransportBenchmark.cs Lines 84 to 88 in cee22ed
|
062eb9e to
24b4ede
Compare
|
Rebased, dropped the submodule changes, and added a commit that should help avoid the hang. Looking to get this merged to avoid conflicts with #33776. |
|
Any idea why the hang started showing up with this change? |
No, it' wasn't even in an affected benchmark. |
|
Nevermind, I found it: The benchmark is throwing an exception related to this change (Content-Length ordering changed), and then GlobalCleanup doesn't run. |
24b4ede to
fb34819
Compare
|
Is there a way to make benchmark errors easier to debug? A friendly error message would be much better than an unidentified freeze. |
|
I'm not sure if there is, but in most cases it's easy enough to run the benchmark in VS and see the issue |
|
Thanks @benaadams |
Can use
BitOperations.TrailingZeroCounton the set bits to determine which is the next header; rather than testing each bit in sequence via fall-through, droping x37ifstatements from theCopyToFastresponse header output method.Second commit is pulling
Content-Lengthto the first header in tests (as its a speciallong?outlier header)from
to
735 bytes less IL
871 bytes less asm