Control rate of HTTP/1 writes #180
Merged
Conversation
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
This avoids bloating memory with unsent messages. This also helps improve the accuracy of our upload stats. Still needs tweaking to deal with websocket upgrade edge-cases.
…it for stream to complete. Remove hacks where 101 response completed a stream, no 1xx response should complete a stream.
JonathanHenson
approved these changes
Jan 31, 2020
bretambrose
reviewed
Jan 31, 2020
Remove the server-only `waiting_stream_list`. Do a simple scan over available streams to find the next one to work on. This is O(N) when pipelining, but I hear that no one does HTTP/1 pipelining, so in that case O(N=1). We can always re-optimize if we have data that pipelining is super popular and this is a hotspot.
bretambrose
approved these changes
Feb 3, 2020
bretambrose
added a commit
that referenced
this pull request
Feb 4, 2020
* Base statistics gathering and monitoring * Control rate of HTTP/1 writes (#180) * HTTP/1 only writes 1 message at a time. This avoids bloating memory with unsent messages. This also helps improve the accuracy of our upload stats. Still needs tweaking to deal with websocket upgrade edge-cases. * Install websocket handler the moment 101 response comes in, do not wait for stream to complete. Remove hacks where 101 response completed a stream, no 1xx response should complete a stream. * Simplify confusing list logic. Remove the server-only `waiting_stream_list`. Do a simple scan over available streams to find the next one to work on. This is O(N) when pipelining, but I hear that no one does HTTP/1 pipelining, so in that case O(N=1). We can always re-optimize if we have data that pipelining is super popular and this is a hotspot. * Update * Windows warning Co-authored-by: Michael Graeb <graebm@amazon.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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 IS NOT A MERGE TO MASTER. CI WILL FAIL. WORKS ON MY MACHINE THOUGH.
HTTP/1 was writing one 16KB message per event-loop tick, regardless of whether that data was making it to the network. This wastes memory on a slow network, and made it hard for us to measure the actual write throughput.
This PR makes it so HTTP/1 waits until a message makes it to the network before attempting to write another.
Also removed hacks that had the stream complete when a 101 Switching Protocols response came in. 1xx responses should never complete the stream. Websockets needed to adapt to this change and install its handler the moment the response came in, instead of waiting for the stream to end.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.