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: reduce allocations of (*clientConnReadLoop).handleReponse by 10% #37853

Closed
rs opened this issue Mar 14, 2020 · 4 comments
Closed
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Performance

Comments

@rs
Copy link
Contributor

rs commented Mar 14, 2020

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

1.14

What did you do?

In our deployment, (*clientConnReadLoop).handleReponse is the 4th method creating the most allocations.

By applying the same slice trick as in textproto.(*Reader).ReadMIMEHeader, we can save about 10% of the allocations:

name                                   old time/op    new time/op    delta
ClientResponseHeaders/___0_Headers-16    82.3µs ± 7%    76.4µs ± 4%   -7.18%  (p=0.000 n=10+10)
ClientResponseHeaders/__10_Headers-16     101µs ± 2%      99µs ± 3%   -2.00%  (p=0.016 n=8+10)
ClientResponseHeaders/_100_Headers-16     213µs ± 2%     202µs ± 4%   -4.96%  (p=0.000 n=9+9)
ClientResponseHeaders/1000_Headers-16    2.28ms ± 1%    2.15ms ± 2%   -5.58%  (p=0.000 n=8+10)

name                                   old alloc/op   new alloc/op   delta
ClientResponseHeaders/___0_Headers-16    4.60kB ± 0%    4.60kB ± 0%     ~     (p=0.201 n=10+10)
ClientResponseHeaders/__10_Headers-16    9.01kB ± 0%    8.66kB ± 0%   -3.96%  (p=0.000 n=10+10)
ClientResponseHeaders/_100_Headers-16    54.4kB ± 0%    48.4kB ± 0%  -11.01%  (p=0.000 n=10+10)
ClientResponseHeaders/1000_Headers-16     702kB ± 0%     595kB ± 0%  -15.28%  (p=0.000 n=10+9)

name                                   old allocs/op  new allocs/op  delta
ClientResponseHeaders/___0_Headers-16      57.0 ± 0%      56.0 ± 0%   -1.75%  (p=0.000 n=10+10)
ClientResponseHeaders/__10_Headers-16       135 ± 0%       123 ± 0%   -8.89%  (p=0.000 n=10+10)
ClientResponseHeaders/_100_Headers-16       786 ± 0%       679 ± 0%  -13.61%  (p=0.000 n=10+10)
ClientResponseHeaders/1000_Headers-16     8.14k ± 0%     7.11k ± 0%  -12.65%  (p=0.000 n=10+10)

Here is the patch:

diff --git a/http2/transport.go b/http2/transport.go
index 81778be..e4fb025 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -1892,7 +1892,9 @@ func (rl *clientConnReadLoop) handleResponse(cs *clientStream, f *MetaHeadersFra
                return nil, errors.New("malformed response from server: malformed non-numeric status pseudo header")
        }

-       header := make(http.Header)
+       regularFields := f.RegularFields()
+       strs := make([]string, len(regularFields))
+       header := make(http.Header, len(regularFields))
        res := &http.Response{
                Proto:      "HTTP/2.0",
                ProtoMajor: 2,
@@ -1900,7 +1902,7 @@ func (rl *clientConnReadLoop) handleResponse(cs *clientStream, f *MetaHeadersFra
                StatusCode: statusCode,
                Status:     status + " " + http.StatusText(statusCode),
        }
-       for _, hf := range f.RegularFields() {
+       for _, hf := range regularFields {
                key := http.CanonicalHeaderKey(hf.Name)
                if key == "Trailer" {
                        t := res.Trailer
@@ -1912,7 +1914,18 @@ func (rl *clientConnReadLoop) handleResponse(cs *clientStream, f *MetaHeadersFra
                                t[http.CanonicalHeaderKey(v)] = nil
                        })
                } else {
-                       header[key] = append(header[key], hf.Value)
+                       vv := header[key]
+                       if vv == nil && len(strs) > 0 {
+                               // More than likely this will be a single-element key.
+                               // Most headers aren't multi-valued.
+                               // Set the capacity on strs[0] to 1, so any future append
+                               // won't extend the slice into the other strings.
+                               vv, strs = strs[:1:1], strs[1:]
+                               vv[0] = hf.Value
+                               header[key] = vv
+                       } else {
+                               header[key] = append(vv, hf.Value)
+                       }
                }
        }

Please tell me if it's worth a CL.

@ALTree ALTree added Performance NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Mar 14, 2020
@ALTree ALTree modified the milestone: Go1.15 Mar 14, 2020
@odeke-em
Copy link
Member

Hello @rs, great catching you here!

Thank you and Sounds good to me, please send a CL.

Perhaps let’s rename strs to valuesSlab, because that’s the parent slice that we’ll be using to create individual value slices each of capacity 1.

@rs
Copy link
Contributor Author

rs commented Mar 15, 2020

Thanks. For the strs naming, I used the same as in textproto.(*Reader).ReadMIMEHeader. I thought it would be more consistent.

@gopherbot
Copy link

Change https://golang.org/cl/223783 mentions this issue: http2: reduce allocations of (*clientConnReadLoop).handleReponse

@gopherbot
Copy link

Change https://golang.org/cl/224217 mentions this issue: net/http: update bundled x/net/http2

gopherbot pushed a commit that referenced this issue Mar 20, 2020
Updates bundled http2 to x/net git rev 63522dbf7

    http2: reduce allocations of (*clientConnReadLoop).handleReponse
    https://golang.org/cl/223783 (#37853)

    http2: remove unused errors
    https://golang.org/cl/220458

    http2: remove unused stream struct fields
    https://golang.org/cl/219857

    http2: fix typo in comment
    https://golang.org/cl/214602

    http2: workaround TCPConn CloseWrite not being supported on Plan 9
    https://golang.org/cl/209417 (#17906, #35904)

Change-Id: I0e48f32247938c3858170bf419624367d4faef4b
Reviewed-on: https://go-review.googlesource.com/c/go/+/224217
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@golang golang locked and limited conversation to collaborators Mar 20, 2021
dteh pushed a commit to dteh/fhttp that referenced this issue Jun 22, 2022
Reduce allocation by using 1 capacity slices out of a single
pre-allocated slice for header values, similarly to how it is done
in textproto.(*Reader).ReadMIMEHeader.

  name                                   old time/op    new time/op    delta
  ClientResponseHeaders/___0_Headers-16    82.3µs ± 7%    76.4µs ± 4%   -7.18%  (p=0.000 n=10+10)
  ClientResponseHeaders/__10_Headers-16     101µs ± 2%      99µs ± 3%   -2.00%  (p=0.016 n=8+10)
  ClientResponseHeaders/_100_Headers-16     213µs ± 2%     202µs ± 4%   -4.96%  (p=0.000 n=9+9)
  ClientResponseHeaders/1000_Headers-16    2.28ms ± 1%    2.15ms ± 2%   -5.58%  (p=0.000 n=8+10)

  name                                   old alloc/op   new alloc/op   delta
  ClientResponseHeaders/___0_Headers-16    4.60kB ± 0%    4.60kB ± 0%     ~     (p=0.201 n=10+10)
  ClientResponseHeaders/__10_Headers-16    9.01kB ± 0%    8.66kB ± 0%   -3.96%  (p=0.000 n=10+10)
  ClientResponseHeaders/_100_Headers-16    54.4kB ± 0%    48.4kB ± 0%  -11.01%  (p=0.000 n=10+10)
  ClientResponseHeaders/1000_Headers-16     702kB ± 0%     595kB ± 0%  -15.28%  (p=0.000 n=10+9)

  name                                   old allocs/op  new allocs/op  delta
  ClientResponseHeaders/___0_Headers-16      57.0 ± 0%      56.0 ± 0%   -1.75%  (p=0.000 n=10+10)
  ClientResponseHeaders/__10_Headers-16       135 ± 0%       123 ± 0%   -8.89%  (p=0.000 n=10+10)
  ClientResponseHeaders/_100_Headers-16       786 ± 0%       679 ± 0%  -13.61%  (p=0.000 n=10+10)
  ClientResponseHeaders/1000_Headers-16     8.14k ± 0%     7.11k ± 0%  -12.65%  (p=0.000 n=10+10)

Fixes golang/go#37853

Change-Id: I0bc6d879293a202a2742a06aca0b6dacfae7fc5f
Reviewed-on: https://go-review.googlesource.com/c/net/+/223783
Run-TryBot: Emmanuel Odeke <emm.odeke@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emm.odeke@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Performance
Projects
None yet
Development

No branches or pull requests

4 participants