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

net/http: consider reusing buffers replacing io.Copy calls with io.CopyBuffer #12455

Closed
artyom opened this issue Sep 2, 2015 · 4 comments

Comments

Projects
None yet
3 participants
@artyom
Copy link
Contributor

commented Sep 2, 2015

As been shown in #12450 by crude patch to make io.Copy reuse its underlying 32k buffer(s) workloads intensively (ab)using net/http can see a significant reduce in GC pressure. Since changes to io.Copy has already been proposed and rejected, I'd like to discuss the prospect of replacing at least some of the io.Copy calls in net/http by io.CopyBuffer with reused byte slices.

The net/http uses io.Copy the most in standard library, and I believe updating it to use recently introduced io.CopyBuffer is worth doing.

go (master) $ find src/net/http -name \*.go -type f ! -name \*_test.go -exec cat '{}' \+ | grep -wc io.Copy
19

At least some of those calls are not subject to usual io.ReaderFrom/io.WriterTo optimizations.

I'm ready to implement changes in the upcoming days and propose a CL.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Sep 2, 2015

I don't have an opinion until I see a concrete proposal. Which io.Copy calls?

@artyom

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2015

@bradfitz one candidate is fallback path in net/http.response.ReadFrom, another is io.Copy used on liveSwitchReader when one makes use of http.CloseNotifier — httputil.ReverseProxy would gain most benefit from this.

benchmark                    old ns/op     new ns/op     delta
BenchmarkCloseNotified-4     309569        273466        -11.66%

benchmark                    old allocs     new allocs     delta
BenchmarkCloseNotified-4     50             48             -4.00%

benchmark                    old bytes     new bytes     delta
BenchmarkCloseNotified-4     36006         3089          -91.42%
@artyom

This comment has been minimized.

Copy link
Contributor Author

commented Sep 2, 2015

@gopherbot

This comment has been minimized.

Copy link

commented Sep 2, 2015

CL https://golang.org/cl/14177 mentions this issue.

@bradfitz bradfitz closed this in 6fd82d8 Sep 4, 2015

@golang golang locked and limited conversation to collaborators Sep 4, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.