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: reduce allocations in (Header).clone #29915

Closed
NWilson opened this issue Jan 24, 2019 · 13 comments
Closed

net/http: reduce allocations in (Header).clone #29915

NWilson opened this issue Jan 24, 2019 · 13 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@NWilson
Copy link

NWilson commented Jan 24, 2019

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.)

--- /tmp/header-v1.go   2019-01-24 11:18:01.182761218 +0000
+++ /tmp/header-v2.go   2019-01-24 11:17:42.367569268 +0000
@@ -1,9 +1,17 @@
 func (h Header) clone() Header {
  h2 := make(Header, len(h))
+ sliceLen := 0
+ for _, vv := range h {
+  sliceLen += len(vv)
+ }
+ slice := make([]string, sliceLen)
+ sliceLen = 0
  for k, vv := range h {
-  vv2 := make([]string, len(vv))
+  vv2 := slice[sliceLen:sliceLen+len(vv):sliceLen+len(vv)]
+  sliceLen += len(vv)
   copy(vv2, vv)
   h2[k] = vv2
  }
  return h2
 }
@mvdan
Copy link
Member

mvdan commented Jan 24, 2019

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.

@bcmills bcmills changed the title Reduce number of allocations in HTTP server net/http: reduce allocations in (Header).clone Jan 29, 2019
@bcmills
Copy link
Contributor

bcmills commented Jan 29, 2019

CC @bradfitz for net/http.

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.)

@bcmills bcmills added this to the Unplanned milestone Jan 29, 2019
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 29, 2019
@bradfitz
Copy link
Contributor

Sounds fine to me. I was actually thinking about this recently when reviewing something else.

Go ahead and send a change.

@bcmills bcmills added the NeedsFix The path to resolution is known, but the work has not been done. label Jan 29, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 29, 2019
@agnivade
Copy link
Contributor

agnivade commented Mar 8, 2019

@NWilson - did you want to send a CL for this ?

@gopherbot
Copy link

Change https://golang.org/cl/166498 mentions this issue: net/http: reduce allocations in Header.clone()

@NWilson
Copy link
Author

NWilson commented Mar 11, 2019

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!

@CAFxX
Copy link
Contributor

CAFxX commented Mar 12, 2019

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).

@agnivade
Copy link
Contributor

@NWilson - We accept github PRs now btw. So you don't need to setup gerrit.

@gopherbot
Copy link

Change https://golang.org/cl/167678 mentions this issue: http2: minimize allocations in cloneHeader

@NWilson
Copy link
Author

NWilson commented Apr 24, 2019

Closing, fixed in https://golang.org/cl/166498

@NWilson NWilson closed this as completed Apr 24, 2019
@bradfitz
Copy link
Contributor

@NWilson, that change hasn't been submitted. Re-opening.

@bradfitz bradfitz reopened this Apr 24, 2019
@gopherbot
Copy link

Change https://golang.org/cl/173658 mentions this issue: net/http: export Header.Clone, reduce its allocations, use it everywhere

@NWilson
Copy link
Author

NWilson commented Apr 24, 2019

Oops, thanks @bradfitz

@golang golang locked and limited conversation to collaborators Apr 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Development

No branches or pull requests

7 participants