Performance rework #22
Merged
Conversation
This file contains 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
add rst_stream api and make it a cast
…e-condition Andymck/fix trailers close race condition
|
Interesting, if I change the req and resp sizes in the tests to small numbers, all tests pass. So something about "large" payloads (just like 270 kb) is somehow broken. |
|
maybe some of my buffered receive code is broken? |
|
Ah, that'd be a good place to start. I hope to dig in some today. |
|
Narrowing down the exact size that triggers it might be helpful |
|
32753 bytes which is roughly 2x the configured max frame size. |
|
I tried increasing the max frame size and that didn't do anything, so likely unrelated. |
|
Interesting/weird thing I've noticed so far when tracing whats going on, when the request is large it is sending it twice (headers and all, not just the body). |
|
Meh, no that isn't the case.. I need more sleep. |
|
Heh, modified the echo test to send a large body and added a print of the frames the client gets back: |
|
Meh, didn't change the window size in the test. With the window size updated its just: |
|
That too was a misconfigure. Setting the frame size on the client to not be over the size on the server I see the server handling the body and then at least attempting (I see the calls to |
|
@Vagabond ok, maybe getting somewhere. I got it to work by basically forcing it to send (apply the stream actions) here: |
|
Oh, hmm. That's interesting. Aren't we violating the spec there? This is saying, due to a stream race, that we can't send as many bytes as we wanted to. How many streams were active on this session? Does the client agree on the window sizes, or do we have an accounting bug? |
|
There is only 1 active stream. Do you mean violating the spec with my change? Because, yes, it is just to debug and find where things might be going wrong. My guess would be an accounting bug. It is attempting to send everything (I see the |
|
Is there a way to see what the client thinks the window size is, or why the server decided it oversent? Maybe you can print BytesSent and NewSWS in the clause there to see how much they disagree? Can you add a unit test to the test suite for this? Also, do we know if this bug happens with a different HTTP/2 client? |
|
Yea, I can do all that in a bit.
And yea, I first encountered the issue when doing interop tests with Go's grpc client.
…On Thu, Nov 16, 2023, at 07:43, Andrew Thompson wrote:
Is there a way to see what the client thinks the window size is, or why the server decided it oversent? Maybe you can print BytesSent and NewSWS in the clause there to see how much they disagree?
Can you add a unit test to the test suite for this?
Also, do we know if this bug happens with a different HTTP/2 client?
—
Reply to this email directly, view it on GitHub <#22 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAI3A3ZG7PV6HCLEMASRJ3YEYRB3AVCNFSM6AAAAAAX7TSVNCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMJUGU4DKNZZGY>.
You are receiving this because you modified the open/close state.Message ID: ***@***.***>
|
|
But it also occurs with our chatterbox's client? |
|
Oh, yea, I recreated with a chatterbox test. |
|
NewSWS is 32707 and BytesSent is 32828 (the size of the whole body). |
|
Seems like its updating the send window and then just never doing anything again. |
|
So it decides it can send everything but then finds if it did send everything (it hasn't actually sent, just decided to) it would be more than allowed? |
|
Wait, 32707 is the send window size 65535 minus the body 32828. So is it comparing the updated window as though it had been sent against what is to be sent? |
|
Seems like double counting, right? Should it instead of checking the current socket send window against BytesSent? Or is it meant to check if new send window would be less than 0 then it should not send? If either of those is the case, I'm then still not sure how to test the case that it does actually end up in a state that it should go into the |
|
Yeah, this code does look wrong. I wonder if it should be |
|
The intent here, is because each stream can be handled independently, is to check that there's not a race condition where we'd send more than we were allowed by the windowing system. Given there's only one stream active this code is clearly wrong somewhere as there's no other stream to race against. |
|
Or possibly we can simply check NewSWS is not < 0? |
|
Right, my thought was |
|
Ah crap, turns out with |
|
nevermind, just didnt reset everyting. it works |
|
so that fixes it, or? |
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.
This PR is a collection of changes to improve the reliability and performance of chatterbox so that it can be used in a high performance context. There are a lot of changes here (and a lot of commits), which reflects the nature of how the branch was developed. Depending on the preference of the reviewer we could break this branch up into smaller PRs (although for many changes this may prove difficult). Certainly the commit history should be squashed, although I am currently leaving it for reference.
Now, I'll provide some information about the issues we encountered and how I resolved them. We are running a high throughput grpc (using grpcbox) service that uses a lot of persistant streams and unary requests. We would see the h2_connection back up under load with large mailboxes, and we'd also see http/2 connections get closed with no explanation under load.
Inspecting how chatterbox works, I learned that the h2_connection handled almost all of the http/2 message processing and routing, as well as the socket reading. This results in an incast condition where a connection with many streams will put far too much load on the h2_connection process, resulting in an effective stall of the connection.
To fix this, I made the h2_stream_set (the structure that tracks all the streams on the connection) into a record holding an ETS table, a set of atomic counters, the http/2 socket, the pid of the connection, etc. I then provide this stream_set record to all the streams and allowed the streams to make changes to the stream set themselves.
I also moved the socket recv out of the connection process into a dedicated sub process that sits in a tight receive loop reading and dispatching incoming http/2 frames. Almost all frame handling is done in this process, and incoming DATA frames are routed directly to the corresponding stream pid, bypassing the h2_connection process entirely.
Similarly, all the socket sends are done by the process wanting to send, rather than serializing them via the connection process (this relies on the assumption that a tcp/ssl send() is atomically sent and not interleaved with other sends).
Some http/2 protocol design issues have some concurrency limits that have been addressed:
Another issue, that we believe was causing our connection drops under load, was that chatterbox would allow streams to be created independent of headers being sent on that stream. Grpcbox used this, and under load I believe that messages to create streams and send headers would be interleaved in the connection's mailbox, leading to headers being sent on a "closed" stream, which is treated as a fatal connection error. I've removed the APIs that allow a stream to be created without headers being sent now, and thus stream creation and headers being sent are now both done atomically.
Stream GC also appeared to be an issue under high stream counts. I was unable to see why the GC was done the way it was and so, in an effort to reduce memory usage, I simply delete streams marked garbage immediately. Everything still seems to work with that change, so I left it in (interestingly there appears to be no way to set garbage_on_end on the server side of a connection).
There are a few unresolved issues:
http2_spec_8_1_SUITE ==> sends_second_headers_with_no_end_stream). I haven't figured out why.systemmemory being used that I don't fully understandThat said, this changeset allows us to run our grpcbox service at much higher scales, more reliably, with less CPU usage and so we are running this in production currently. I will update this PR if we find other issues under load, or determine the source of the memory usage.