-
Notifications
You must be signed in to change notification settings - Fork 18k
net/http: reduce allocations in (Header).clone #29915
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
Comments
I don't think there's a need to raise an issue for this. Assuming there already are benchmarks that would cover this optimisation, you can just send a CL with the change and the benchmark numbers showing the improvement. |
CC @bradfitz for Note that number of allocations doesn't necessarily correlate with overall throughput or latency. (Sometimes, collecting the allocation is less work for the program than avoiding it.) In this case, the proposed change introduces a time/space tradeoff: if the user stores one particular slice, the may end up pinning the entire larger slice where before most of it could be collected. (You'd want to support a change with benchmarks either way.) |
Sounds fine to me. I was actually thinking about this recently when reviewing something else. Go ahead and send a change. |
@NWilson - did you want to send a CL for this ? |
Change https://golang.org/cl/166498 mentions this issue: |
Brilliant, thanks so much for putting together the PR! I maybe would have done it eventually... but I've not contributed to Go before, and I just know from each experience that a first-time patch to a new project takes at least a few hours to learn the ropes and get set up with a new build & code review system, and just didn't have the time to spare. I'm happy with the CL above, thank you! |
I realized there are at least other two copies of that function in the go tree, so the CL is incomplete-ish (technically one of them -in h2_bundle.go- is bundled, so I can't fix it in the CL). |
@NWilson - We accept github PRs now btw. So you don't need to setup gerrit. |
Change https://golang.org/cl/167678 mentions this issue: |
Closing, fixed in https://golang.org/cl/166498 |
@NWilson, that change hasn't been submitted. Re-opening. |
Change https://golang.org/cl/173658 mentions this issue: |
Oops, thanks @bradfitz |
What version of Go are you using (
go version
)?go version go1.11.2 linux/amd64
Does this issue reproduce with the latest release?
Yes
What did you do?
When using net/http's server, and profiling with pprof shows that the "Header.clone" function takes up a lot of allocations (4% of my application's allocations, actually).
Solution
Could we improve the function to perform fewer allocations!
(NB. Note that Reader.ReadMIMEHeader in net/textproto already uses exactly this same trick, of using three-argument slicing to build the http.Header using a single
[]string
. So the pattern already exists in the codebase.)The text was updated successfully, but these errors were encountered: