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

x/net/http2: reduce allocations of (*clientConnReadLoop).handleReponse by 10% #37853

rs opened this issue Mar 14, 2020 · 4 comments

Comments

@rs
Copy link
Contributor

@rs 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.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented Mar 14, 2020

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

@gopherbot gopherbot commented Mar 17, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Mar 19, 2020

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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants