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.

Sign up for GitHub

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

Jump to bottom

x/net/http2: WINDOW_UPDATE sent rate too high and can't be configured #28732

Closed
crowfrog opened this issue Nov 12, 2018 · 10 comments
Closed

x/net/http2: WINDOW_UPDATE sent rate too high and can't be configured #28732

crowfrog opened this issue Nov 12, 2018 · 10 comments
Assignees
@neild
Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone
Unreleased

Comments

@crowfrog
Copy link

What version of Go are you using (go version)?

golang:1.10.0 linux

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

container: rhel
kubenet:

What did you do?

Currently golang http2 stack will send WINDOW_UPDATE back when receive a request with data.
It means a client will recevie a response and WINDOW_UPDATE when it send a request with data to golang http2 server.
sent one 'request' package and got two 'reponse' package, that will impact client performance in a high load traffic.
In fact, golang stack use 2^31 as default window size for connection. It is very big and updated too frequently is unnecessary.

What did you expect to see?

Change this behavior or make it configurable like other language http2 stack.
I think c++/java popular http2 stack have a configurable rate (default is 50%) used for sending WINDOS_UPDATE frame when the rate of left size/total is less than it.

What did you see instead?
@agnivade agnivade added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Nov 12, 2018
@agnivade agnivade added this to the Unplanned milestone Nov 12, 2018
@agnivade
Copy link
Contributor

/cc @rs @fraenkel @bradfitz

@crowfrog crowfrog changed the title x/net/http2: WINDOW_UPDATE sent rate to high and can't be configured x/net/http2: WINDOW_UPDATE sent rate too high and can't be configured Nov 13, 2018
@fraenkel
Copy link
Contributor

Most of the frameworks have chosen 50% as the deciding factor to send a WINDOW_UPDATE when reading DATA. nginx does it at the 25% mark.
I only saw one framework that allows you to tweak the value but there could be others, however I don't think it is needed.

I will go experiment and see how much benefit some change like this truly offers.

@bradfitz
Copy link
Contributor

Definitely room for improvement here. (There might even be a TODO about this in the code, IIRC) But let's not add configurability here.

@crowfrog
Copy link
Author

hi @fraenkel & @bradfitz,

Thanks a lot for your quick response! I am ok for non-configurable.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/150197 mentions this issue: http2: Send WindwUpdates when remaining bytes are below a threshold

@louygan
Copy link

louygan commented Dec 17, 2018

I could not find the change in the master.
when will this change merged to master?

@freeformz freeformz mentioned this issue Apr 8, 2022
Open
@seankhliao seankhliao mentioned this issue Aug 29, 2022
Closed
@dmitshur dmitshur modified the milestones: Unplanned, Unreleased Sep 20, 2022
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Sep 20, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/432038 mentions this issue: http2: Send WindowUpdates when remaining bytes are below a threshold

@bcmills
Copy link
Contributor

bcmills commented Sep 20, 2022

Reopening because https://go.dev/cl/432038 (which marked this issue fixed) was reverted.

@bcmills bcmills reopened this Sep 20, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/448055 mentions this issue: Revert "http2: Send WindowUpdates when remaining bytes are below a threshold"

gopherbot pushed a commit to golang/net that referenced this issue Nov 4, 2022
@neild
Revert "http2: Send WindowUpdates when remaining bytes are below a th…
d7f77dc 
…reshold"

This reverts commit 2e0b12c.

The calculation for when to return flow control doesn't properly take
data in server read buffers into account, resulting in flow control
credit being returned too quickly without backpressure.

Fixes golang/go#56315
For golang/go#28732

Change-Id: I573afd1a37d8a711da47f05f38f4de04183fb941
Reviewed-on: https://go-review.googlesource.com/c/net/+/448055
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
@neild neild reopened this Nov 4, 2022
@neild neild self-assigned this Nov 4, 2022
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/448155 mentions this issue: http2: refactor inbound flow control tracking

WeiminShang pushed a commit to WeiminShang/net that referenced this issue Nov 16, 2022
@fraenkel @neild
http2: Send WindowUpdates when remaining bytes are below a threshold
cbdb933 
Rather than send a WindowUpdate on every chunk of bytes read, allow them
to collect until we go past half the configured window size. Once the
threshold is reached, send a single WindowUpdate to reset the amount
back to the maximum amount configured.

Fixes golang/go#28732

Change-Id: I177f962ee0a9b8daa1c4817e0ab7698e828bad96
Reviewed-on: https://go-review.googlesource.com/c/net/+/150197
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Damien Neil <dneil@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
Reviewed-by: Damien Neil <dneil@google.com>
Run-TryBot: Damien Neil <dneil@google.com>
WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
@WeiminShang @gopherbot
http2: Send WindowUpdates when remaining bytes are below a threshold
3a96036 
This rolls-forward CL 150197 with an added fix for
TestProtocolErrorAfterGoAway.

Rather than send a WindowUpdate on every chunk of bytes read, allow them
to collect until we go past half the configured window size. Once the
threshold is reached, send a single WindowUpdate to reset the amount
back to the maximum amount configured.

Fixes golang/go#28732

Change-Id: Icee93dedf68d6166aa6fe0c3845d717e66586e73
Reviewed-on: https://go-review.googlesource.com/c/net/+/432038
Run-TryBot: Damien Neil <dneil@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Tatiana Bradley <tatiana@golang.org>
WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
@WeiminShang @neild
Revert "http2: Send WindowUpdates when remaining bytes are below a th…
72ef674 
…reshold"

This reverts commit 3a96036.

The calculation for when to return flow control doesn't properly take
data in server read buffers into account, resulting in flow control
credit being returned too quickly without backpressure.

Fixes golang/go#56315
For golang/go#28732

Change-Id: I573afd1a37d8a711da47f05f38f4de04183fb941
Reviewed-on: https://go-review.googlesource.com/c/net/+/448055
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Damien Neil <dneil@google.com>
Reviewed-by: Roland Shoemaker <roland@golang.org>
@golang golang locked and limited conversation to collaborators Jan 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@neild neild

Labels
FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
9 participants
@bradfitz @neild @fraenkel @agnivade @dmitshur @bcmills @louygan @gopherbot @crowfrog