Skip to content
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

HTTP2 receiver window size does not seem to expand #1960

Open
rocallahan opened this issue Sep 30, 2019 · 11 comments
Open

HTTP2 receiver window size does not seem to expand #1960

rocallahan opened this issue Sep 30, 2019 · 11 comments
Labels
A-http2 Area: HTTP/2 specific. C-performance Category: performance. This is making existing behavior go faster.

Comments

@rocallahan
Copy link

This test program downloads a 281MB file from AWS S3 using hyper-rustls with HTTP2 enabled:
https://github.com/rocallahan/http2-window-size-bug/blob/master/src/main.rs
On a AWS c5d.9xl instance, it takes a few minutes. If you disable HTTP2 in the example, or use curl/wget, it takes a few seconds.

Calling http2_initial_stream_window_size to set SETTINGS_INITIAL_WINDOW_SIZE to 2^31 - 1 fixes the problem completely.

My understanding is that client stacks should automatically expand the HTTP2 stream window size while no backpressure is needed, like TCP does, but it appears that isn't happening.

If the intent is that hyper should not manage stream window sizes, okay, but in that case reqwest should.

@dvic
Copy link

dvic commented Sep 30, 2019

Related: #1813 (where HTTP is approx ~2x slower)

@seanmonstar
Copy link
Member

I believe it makes sense for hyper to use a better default, unless configured otherwise, similar to how in http1 it uses default read and write strategies to improve performance.

One such default could be to use BDP.

@seanmonstar seanmonstar added A-http2 Area: HTTP/2 specific. C-performance Category: performance. This is making existing behavior go faster. labels Oct 2, 2019
@rocallahan
Copy link
Author

Yes, I think BDP estimation would be a suitable fix.

@seanmonstar
Copy link
Member

Getting BDP may take a bit more work though, if we wanted, we could change hyper's defaults to just something bigger (1 MB?)...

@rocallahan
Copy link
Author

I don't know whether changing the default makes sense, but whatever the default is, at some point there needs to be a dynamic algorithm as well.

@seanmonstar
Copy link
Member

From the HTTP2 spec:

Flow control is defined to protect endpoints that are operating under resource constraints. [..] Deployments that do not require this capability can advertise a flow-control window of the maximum size [..] This effectively disables flow control for that receiver.

I read that as saying flow control is available if you need it, but if not, don't use it. So my opinion now is to select a better default for most applications (and those who need to constrain more can always pass a specific option).

Clients are typically receiving more data than they send, and typically have less connections in general. Servers however are usually sending more than receiving, and are more likely to be attacked or flooded. Additionally, due to the multiplexing nature, a stream probably shouldn't hog the entirety of a connections window.

Therefore, my suggestion:

  • Client default connection window of 5 MB, and stream window of 3 MB.
  • Server default connection window of 2MB, and stream window of 1 MB.
  • Work on BDP solution for the future.

The numbers I picked are kinda arbitrary, others could be suggested instead.

seanmonstar added a commit that referenced this issue Oct 8, 2019
Set default HTTP2 window sizes much larger values than the spec default.

ref #1960
@rocallahan
Copy link
Author

FWIW, using those settings in reqwest 0.9.22 gives me 12s download time for a ~500MB file from S3 to an EC2 instance, compared to 6s using wget or using 2^31 - 1 for both initial window sizes. So there is definitely room for improvement.

@seanmonstar
Copy link
Member

How'd you use those settings in reqwest? Custom build? Because the new settings aren't in a published version yet.

@khuey
Copy link
Contributor

khuey commented Oct 17, 2019

We maintain a fork (which is currently at https://github.com/khuey/reqwest/tree/downstream2)

@seanmonstar
Copy link
Member

The good news is even with those conservative defaults, it went from a few minutes to a few seconds! I've been adding some missing pieces to the h2 library that I'm hoping will allow implementing a BDP detector in hyper.

@seanmonstar
Copy link
Member

Initial adaptive window support using BDP is proposed in #2138.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-http2 Area: HTTP/2 specific. C-performance Category: performance. This is making existing behavior go faster.
Projects
None yet
Development

No branches or pull requests

4 participants