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

x/net/http2: allow setting MaxConcurrentStreams on client #63196

Open
Rohsichan opened this issue Sep 24, 2023 · 7 comments
Open

x/net/http2: allow setting MaxConcurrentStreams on client #63196

Rohsichan opened this issue Sep 24, 2023 · 7 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@Rohsichan
Copy link

Rohsichan commented Sep 24, 2023

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

$ go version
go version go1.19.13 linux/amd64

Does this issue reproduce with the latest release?

yes

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/root/.cache/go-build"
GOENV="/root/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/root/go/pkg/mod"
GOOS="linux"
GOPATH="/root/go"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.19.13"
GCCGO="gccgo"
GOAMD64="v1"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOWORK=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -Wl,--no-gc-sections -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build2198329798=/tmp/go-build -gno-record-gcc-switches"

What did you do?

I downloaded 9M files using 100 goroutines from http2.transport.

What did you expect to see?

There are currently more than 100 active streams in http2. I thought the connection would be newly opened, but it didn't.

What did you see instead?

After checking the http2.transport connection pool, we are reusing a single connection.
The netstat -nap check shows that there is 1 connection, rx full, tx full a lot.

The following modifications improve performance by creating connections in units of 20 active streams.

func (cc *ClientConn) State() ClientConnState {
	cc.wmu.Lock()
	maxConcurrent := cc.maxConcurrentStreams
	if !cc.seenSettings {
		maxConcurrent = 0
	}
	cc.wmu.Unlock()

+	// cc.mu.Lock()
+	// defer cc.mu.Unlock()
	return ClientConnState{
		Closed:               cc.closed,
		Closing:              cc.closing || cc.singleUse || cc.doNotReuse || cc.goAway != nil,
		StreamsActive:        len(cc.streams),
		StreamsReserved:      cc.streamsReserved,
		StreamsPending:       cc.pendingRequests,
		LastIdle:             cc.lastIdle,
		MaxConcurrentStreams: maxConcurrent,
	}
}
func (cc *ClientConn) idleStateLocked() (st clientConnIdleState) {
	if cc.singleUse && cc.nextStreamID > 1 {
		return
	}
	var maxConcurrentOkay bool
	if cc.t.StrictMaxConcurrentStreams {
		// We'll tell the caller we can take a new request to
		// prevent the caller from dialing a new TCP
		// connection, but then we'll block later before
		// writing it.
		maxConcurrentOkay = true
	} else {
		maxConcurrentOkay = int64(len(cc.streams)+cc.streamsReserved+1) <= int64(cc.maxConcurrentStreams)
	}

	st.canTakeNewRequest = cc.goAway == nil && !cc.closed && !cc.closing && maxConcurrentOkay &&
		!cc.doNotReuse &&
		int64(cc.nextStreamID)+2*int64(cc.pendingRequests) < math.MaxInt32 &&
		!cc.tooIdleLocked()

+	if st.canTakeNewRequest {
+		if cc.State().StreamsActive > 20 {
+			st.canTakeNewRequest = false
+		}
+	}
	return
}

As the number of active streams increases, http2 becomes slower and slower.

@thanm thanm added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 25, 2023
@thanm thanm added this to the Backlog milestone Sep 25, 2023
@thanm thanm changed the title affected/package: x/net/http2 transport is very slow x/net/http2: unexpected reuse of single connection Sep 25, 2023
@thanm thanm added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Sep 25, 2023
@thanm
Copy link
Contributor

thanm commented Sep 25, 2023

Thanks for the report. Could you please post (via playground link) the code you are using?

@thanm
Copy link
Contributor

thanm commented Sep 25, 2023

@neild @tombergan per owners

@Rohsichan
Copy link
Author

Rohsichan commented Sep 26, 2023

The test code is here.
https://go.dev/play/p/et4HPzU3wLq?v=goprev

I checked 1 connection on my machine.
The response time was 10 seconds in total.
In http1.1, the response time is within 2 seconds.

http2

time is 10.555606421

http1.1

time is 1.013303016s

@Rohsichan
Copy link
Author

Please let me know if you need any more information.

@seankhliao
Copy link
Member

note that the server can set MaxConcurrentStreams, similar performance improvements can be achieved through #47840

@seankhliao seankhliao changed the title x/net/http2: unexpected reuse of single connection x/net/http2: allow setting MaxConcurrentStreams on client Jan 28, 2024
@seankhliao seankhliao removed the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jan 28, 2024
@Rohsichan
Copy link
Author

I think we need a way to set MaxConcurrentStreams on the client. (http2.transport)

In addition to the go server, I am using a server such as go reverse proxy, nginx, etc.
If Golang becomes a client, it is the same.

The MaxReadFrameSize fluctuates and the numbers remain the same.

@Rohsichan
Copy link
Author

I'm looking for something new and sharing it with you.

My current computer is cpu 56core.
You have set runtime.GOMAXPROCS(3).
http2 10 seconds -> 4 seconds changed.
http1 1 second -> 4 seconds changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

3 participants