-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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
x/net/http2: client can't fully utilize network resource #37373
Comments
Curious if you just tried to walk backwards through the connections? |
@fraenkel |
@ochanism Your fix was to randomly start somewhere in the list. The current code walks forward. Since connections are added to the end, in most cases, the next connection with availability will be the last one. Obviously it all depends on which streams in which connections are closed. Ultimately the bigger issue is that one has no way of configuring your new option from net/http. Now if this is only for use from net/http2 its less of an issue. Its also related to #34511 |
@fraenkel |
Your selection will have similar issues in practice. Randomly selecting won't solve optimizing the network traffic across multiple connections. The underlying locking is a bit loose and writes currently lock reads. Which means depending on how loaded (# of streams) a connection is, and how much data is being sent back, the network may appear to be utilized but you are still dealing with lots of contention. The fact that you are creating a new rand for each request does not provide any guarantees over the selection across the connections. |
@fraenkel |
/cc @bradfitz @tombergan |
@cagedmantis @bradfitz @tombergan |
It sounds like a knob we wouldn't want to expose to users. How would you document how to tune it? I'd rather just see it be automatically fast instead. |
@bradfitz I think the MinConcurrentConns field has a similar role to the MaxIdleConns field in HTTP transport. (Not exact same, but similar) I've been running a server with this library in production, and there was a huge gain in terms of throughput. When I used the vanilla net/http library, an HTTP client uses only one or two TCP connections even if there is huge traffic. When I set strictMaxConcurrentStreams to false, I expected that the number of TCP connections would be increased if there is burst traffic. But it didn't work as I expected. What do you think about this? Do you have any idea? |
We've noticed this in rclone too rclone/rclone#3631 (comment) When uploading 4 files at once to Google Drive
So that is 50% of the speed using HTTP/2. My conjecture is that would go faster if we could persuade the HTTP/2 transport to use more TCP connections but I don't know how to do that. |
I can reproduce similar values with following snippet, by changing the package main
import (
"fmt"
"io/ioutil"
"log"
"net/http"
"net/http/httptest"
"strings"
"sync"
"time"
)
func handler(w http.ResponseWriter, r *http.Request) {
start := time.Now()
n := 1 << 20
chunk := strings.Repeat("x", 1024)
for i := 0; i < n; i++ {
_, err := fmt.Fprintf(w, chunk)
if err != nil {
log.Printf("Error writing %v", err)
}
}
size := float64(n) / 1024
duration := time.Since(start).Seconds()
log.Printf("Server: %f MBps", size/duration)
}
func main() {
sv := httptest.NewUnstartedServer(http.HandlerFunc(handler))
sv.EnableHTTP2 = true
sv.StartTLS()
var wg sync.WaitGroup
tr, ok := sv.Client().Transport.(*http.Transport)
if !ok {
panic("transport unsupported")
}
tr.DisableKeepAlives = false
workers := 4
for i := 1; i <= workers; i++ {
wg.Add(1)
i := i
go func() {
defer wg.Done()
start := time.Now()
resp, err := sv.Client().Get(sv.URL + "/")
if err != nil {
log.Fatal(err)
}
defer resp.Body.Close()
data, err := ioutil.ReadAll(resp.Body)
if err != nil {
log.Printf("Error reading %v", err)
}
size := float64(len(data)) / (1024 * 1024)
duration := time.Since(start).Seconds()
log.Printf("Client%d: %f MBps ", i, size/duration)
}()
}
wg.Wait()
}
Setting |
I hope this bug is not forgotten, it's a pretty important one... |
* Disabled http2 due to http2 streaming in net/http2 performance issues golang/go#37373 * RCLONE_S3_UPLOAD_CONCURRENCY, RCLONE_S3_CHUNK_SIZE to improve parallelism and download efficiency Signed-off-by: mzardab <mzardab@redhat.com>
* Disabled http2 due to http2 streaming in net/http2 performance issues golang/go#37373 * RCLONE_S3_UPLOAD_CONCURRENCY, RCLONE_S3_CHUNK_SIZE to improve parallelism and download efficiency Signed-off-by: mzardab <mzardab@redhat.com>
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
I ran an http2 webdav server and I made a client with golang x/net/http2 for a test.
The client uploaded a 1 MB file infinitely with a few hundred goroutine.
MaxConcurrentStreams of the server was 100 and StrictMaxConcurrentStreams of the client was false.
I measured the throughput between the server and the client on AWS.
And network bandwidth between the server and the client was 150 Gbps in the infrastructure level.
What did you expect to see?
I expected that the throughput would be around 150 Gbps.
What did you see instead?
The measured throughput was 90 Gbps.
Proposal
I figured out what caused this. I modified the http2 connection pool a little as shown in the below link.
(ochanism/net@b2cc93d)
With the above code, I could achieve 150 Gbps throughput.
For achieving high throughput, multiple TCP connections are needed.
But the current version of the http2 connection pool increases TCP connections only when there is no available TCP connection (CurrentMaxStreams == MaxConcurrentStreams for all the connections).
I reduced MaxConcurrentStreams value at the server-side, but the number of TCP connections wasn't increased than I expected.
So I added a MinConcurrentConns field to http2 Transport.
MinConcurrentConns is the minimum number of TCP connections and ClientConnPool tries to maintain MinConcurrentConns TCP connections at least. If 0, ClientConnPool creates a TCP connection only when needed.
Please review my proposal.
The text was updated successfully, but these errors were encountered: