Code review - Issue 49570044: code review 49570044: net/http: reuse client connections earlier when Content...https://codereview.appspot.com/2014-01-29T12:04:14+00:00rietveld
Message from unknown
2014-01-29T09:23:11+00:00bradfitzurn:md5:f5c15e524a50840a70d643a9bdf4133e
Message from unknown
2014-01-29T09:23:13+00:00bradfitzurn:md5:ef5570fd481b123418f999bbeb21d317
Message from unknown
2014-01-29T09:23:29+00:00bradfitzurn:md5:58ffb5e88df3f35881fbf2519369a03a
Message from bradfitz@golang.org
2014-01-29T09:23:32+00:00bradfitzurn:md5:7611d8486272d225c4b88325e25ef150
Hello adg@golang.org (cc: golang-codereviews@googlegroups.com),
I'd like you to review this change to
https://go.googlecode.com/hg/
Message from adg@golang.org
2014-01-29T10:04:18+00:00adgurn:md5:4dba2a0fc841fe4139b20950c4aadea3
Apologies if this comment misses the entire point of the change.
https://codereview.appspot.com/49570044/diff/40001/src/pkg/net/http/transport_test.go
File src/pkg/net/http/transport_test.go (right):
https://codereview.appspot.com/49570044/diff/40001/src/pkg/net/http/transport_test.go#newcode300
src/pkg/net/http/transport_test.go:300: defer res.Body.Close()
This is an erroneous client, though, isn't it? Do the close explicitly after the Read instead of deferring it.
Message from bradfitz@golang.org
2014-01-29T10:06:52+00:00bradfitzurn:md5:b528f0f3c8e2e05d87dce4e8df1cba96
I explicitly don't want to eagerly close the bodies in the test, since
we're counting connections used.
But I do want to clean up before the test itself finishes (for afterTest),
hence the defer.
I originally had a slice of io.Closers I was appending to but realized I
just want defer.
Want a comment?
On Jan 29, 2014 11:04 AM, <adg@golang.org> wrote:
> Apologies if this comment misses the entire point of the change.
>
>
> https://codereview.appspot.com/49570044/diff/40001/src/
> pkg/net/http/transport_test.go
> File src/pkg/net/http/transport_test.go (right):
>
> https://codereview.appspot.com/49570044/diff/40001/src/
> pkg/net/http/transport_test.go#newcode300
> src/pkg/net/http/transport_test.go:300: defer res.Body.Close()
> This is an erroneous client, though, isn't it? Do the close explicitly
> after the Read instead of deferring it.
>
> https://codereview.appspot.com/49570044/
>
Message from adg@golang.org
2014-01-29T10:08:23+00:00adgurn:md5:7dc793c058b717b97863039758bede79
Yeah I think the comment would help. I thought that might be the case, but
wasn't sure. Outside of the context of the CL, it would be totally
mysterious.
On 29 January 2014 21:06, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> I explicitly don't want to eagerly close the bodies in the test, since
> we're counting connections used.
>
> But I do want to clean up before the test itself finishes (for afterTest),
> hence the defer.
>
> I originally had a slice of io.Closers I was appending to but realized I
> just want defer.
>
> Want a comment?
> On Jan 29, 2014 11:04 AM, <adg@golang.org> wrote:
>
>> Apologies if this comment misses the entire point of the change.
>>
>>
>> https://codereview.appspot.com/49570044/diff/40001/src/
>> pkg/net/http/transport_test.go
>> File src/pkg/net/http/transport_test.go (right):
>>
>> https://codereview.appspot.com/49570044/diff/40001/src/
>> pkg/net/http/transport_test.go#newcode300
>> src/pkg/net/http/transport_test.go:300: defer res.Body.Close()
>> This is an erroneous client, though, isn't it? Do the close explicitly
>> after the Read instead of deferring it.
>>
>> https://codereview.appspot.com/49570044/
>>
>
Message from unknown
2014-01-29T10:14:04+00:00bradfitzurn:md5:00c5d58c6a0b247a34469cca86a144e4
Message from bradfitz@golang.org
2014-01-29T10:14:08+00:00bradfitzurn:md5:e244fd18184b2a104e7b7bfc078969ba
Hello adg@golang.org (cc: golang-codereviews@googlegroups.com),
Please take another look.
Message from adg@golang.org
2014-01-29T10:15:46+00:00adgurn:md5:f5275627f77bb5f591624e548ee01841
LGTM
Message from unknown
2014-01-29T10:23:39+00:00bradfitzurn:md5:f7c098ab06fc5d06455debf2ac2eafc8
Message from bradfitz@golang.org
2014-01-29T10:23:48+00:00bradfitzurn:md5:6c87ff11db3f9290080298c929038f1c
*** Submitted as https://code.google.com/p/go/source/detail?r=d92b32d188ec ***
net/http: reuse client connections earlier when Content-Length is set
Set EOF on the final Read of a body with a Content-Length, which
will cause clients to recycle their connection immediately upon
the final Read, rather than waiting for another Read or Close
(neither of which might come). This happens often when client
code is simply something like:
err := json.NewDecoder(resp.Body).Decode(&dest)
...
Then there's usually no subsequent Read. Even if the client
calls Close (which they should): in Go 1.1, the body was
slurped to EOF, but in Go 1.2, that was then treated as a
Close-before-EOF and the underlying connection was closed.
But that's assuming the user even calls Close. Many don't.
Reading to EOF also causes a connection be reused. Now the EOF
arrives earlier.
This CL only addresses the Content-Length case. A future CL
will address the chunked case.
LGTM=adg
R=adg
CC=golang-codereviews
https://codereview.appspot.com/49570044
Message from dvyukov@google.com
2014-01-29T11:18:05+00:00dvyukovurn:md5:962661c64fdb43b682073fe556bbd860
FYI
---------- Forwarded message ----------
Date: Wed, Jan 29, 2014 at 3:13 PM
Subject: Perf changes on windows-amd64 by net/http: reuse client connections earlier when Content-Length is set
Change d92b32d188ec caused perf changes on windows-amd64:
net/http: reuse client connections earlier when Content-Length is set
Set EOF on the final Read of a body with a Content-Length, which
will cause clients to recycle their connection immediately upon
the final Read, rather than waiting for another Read or Close
(neither of which might come). This happens often when client
code is simply something like:
err := json.NewDecoder(resp.Body).Decode(&dest)
...
Then there's usually no subsequent Read. Even if the client
calls Close (which they sh
http://code.google.com/p/go/source/detail?r=d92b32d188ec
http-1 old new delta
allocated 7003 5466 -21.95
gc-pause-total 3663 2902 -20.78
http-8 old new delta
allocated 11402 9821 -13.87
gc-pause-total 10038 8537 -14.95
http://goperfd.appspot.com/perfdetail?commit=d92b32d188ec18b61690d3c8e74c02e03da66105&commit0=9c65fe4ce5a23c834205b5767f2bc766922da6f1&kind=builder&builder=windows-amd64
Message from bradfitz@golang.org
2014-01-29T11:20:11+00:00bradfitzurn:md5:db351acfa74971230786d09451c8884e
Where is the source to that benchmark?
On Wed, Jan 29, 2014 at 12:18 PM, <dvyukov@google.com> wrote:
> FYI
>
>
> ---------- Forwarded message ----------
> Date: Wed, Jan 29, 2014 at 3:13 PM
> Subject: Perf changes on windows-amd64 by net/http: reuse client
> connections earlier when Content-Length is set
>
>
> Change d92b32d188ec caused perf changes on windows-amd64:
>
>
> net/http: reuse client connections earlier when Content-Length is set
>
> Set EOF on the final Read of a body with a Content-Length, which
> will cause clients to recycle their connection immediately upon
> the final Read, rather than waiting for another Read or Close
> (neither of which might come). This happens often when client
> code is simply something like:
>
> err := json.NewDecoder(resp.Body).Decode(&dest)
> ...
>
> Then there's usually no subsequent Read. Even if the client
> calls Close (which they sh
>
> http://code.google.com/p/go/source/detail?r=d92b32d188ec
>
> http-1 old new delta
> allocated 7003 5466 -21.95
> gc-pause-total 3663 2902 -20.78
>
> http-8 old new delta
> allocated 11402 9821 -13.87
> gc-pause-total 10038 8537 -14.95
>
> http://goperfd.appspot.com/perfdetail?commit=
> d92b32d188ec18b61690d3c8e74c02e03da66105&commit0=
> 9c65fe4ce5a23c834205b5767f2bc766922da6f1&kind=builder&
> builder=windows-amd64
>
>
> https://codereview.appspot.com/49570044/
>
Message from dvyukov@google.com
2014-01-29T11:25:25+00:00dvyukovurn:md5:026e997031b72729eaed86fbb2a5ae0e
https://code.google.com/p/goperfd/source/browse/bench/http/http.go
On Wed, Jan 29, 2014 at 3:20 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Where is the source to that benchmark?
>
>
>
> On Wed, Jan 29, 2014 at 12:18 PM, <dvyukov@google.com> wrote:
>>
>> FYI
>>
>>
>> ---------- Forwarded message ----------
>> Date: Wed, Jan 29, 2014 at 3:13 PM
>> Subject: Perf changes on windows-amd64 by net/http: reuse client
>> connections earlier when Content-Length is set
>>
>>
>> Change d92b32d188ec caused perf changes on windows-amd64:
>>
>>
>> net/http: reuse client connections earlier when Content-Length is set
>>
>> Set EOF on the final Read of a body with a Content-Length, which
>> will cause clients to recycle their connection immediately upon
>> the final Read, rather than waiting for another Read or Close
>> (neither of which might come). This happens often when client
>> code is simply something like:
>>
>> err := json.NewDecoder(resp.Body).Decode(&dest)
>> ...
>>
>> Then there's usually no subsequent Read. Even if the client
>> calls Close (which they sh
>>
>> http://code.google.com/p/go/source/detail?r=d92b32d188ec
>>
>> http-1 old new delta
>> allocated 7003 5466 -21.95
>> gc-pause-total 3663 2902 -20.78
>>
>> http-8 old new delta
>> allocated 11402 9821 -13.87
>> gc-pause-total 10038 8537 -14.95
>>
>>
>> http://goperfd.appspot.com/perfdetail?commit=d92b32d188ec18b61690d3c8e74c02e03da66105&commit0=9c65fe4ce5a23c834205b5767f2bc766922da6f1&kind=builder&builder=windows-amd64
>>
>>
>> https://codereview.appspot.com/49570044/
>
>
Message from bradfitz@golang.org
2014-01-29T11:38:50+00:00bradfitzurn:md5:f6452b0e6f6b372383fb628bdd7d20e5
Hmmm. I'm surprised.
It's not obvious why there are fewer allocations.
On Jan 29, 2014 12:25 PM, "Dmitry Vyukov" <dvyukov@google.com> wrote:
> https://code.google.com/p/goperfd/source/browse/bench/http/http.go
>
>
> On Wed, Jan 29, 2014 at 3:20 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > Where is the source to that benchmark?
> >
> >
> >
> > On Wed, Jan 29, 2014 at 12:18 PM, <dvyukov@google.com> wrote:
> >>
> >> FYI
> >>
> >>
> >> ---------- Forwarded message ----------
> >> Date: Wed, Jan 29, 2014 at 3:13 PM
> >> Subject: Perf changes on windows-amd64 by net/http: reuse client
> >> connections earlier when Content-Length is set
> >>
> >>
> >> Change d92b32d188ec caused perf changes on windows-amd64:
> >>
> >>
> >> net/http: reuse client connections earlier when Content-Length is set
> >>
> >> Set EOF on the final Read of a body with a Content-Length, which
> >> will cause clients to recycle their connection immediately upon
> >> the final Read, rather than waiting for another Read or Close
> >> (neither of which might come). This happens often when client
> >> code is simply something like:
> >>
> >> err := json.NewDecoder(resp.Body).Decode(&dest)
> >> ...
> >>
> >> Then there's usually no subsequent Read. Even if the client
> >> calls Close (which they sh
> >>
> >> http://code.google.com/p/go/source/detail?r=d92b32d188ec
> >>
> >> http-1 old new delta
> >> allocated 7003 5466 -21.95
> >> gc-pause-total 3663 2902 -20.78
> >>
> >> http-8 old new delta
> >> allocated 11402 9821 -13.87
> >> gc-pause-total 10038 8537 -14.95
> >>
> >>
> >>
> http://goperfd.appspot.com/perfdetail?commit=d92b32d188ec18b61690d3c8e74c02e03da66105&commit0=9c65fe4ce5a23c834205b5767f2bc766922da6f1&kind=builder&builder=windows-amd64
> >>
> >>
> >> https://codereview.appspot.com/49570044/
> >
> >
>
Message from dvyukov@google.com
2014-01-29T11:45:38+00:00dvyukovurn:md5:578908cbaccdfb4f5747c35a3411b0f3
Both builders agree on this change:
http://goperfd.appspot.com/perfdetail?commit=d92b32d188ec18b61690d3c8e74c02e03da66105&builder=linux-amd64&benchmark=http
On Wed, Jan 29, 2014 at 3:38 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Hmmm. I'm surprised.
>
> It's not obvious why there are fewer allocations.
>
> On Jan 29, 2014 12:25 PM, "Dmitry Vyukov" <dvyukov@google.com> wrote:
>>
>> https://code.google.com/p/goperfd/source/browse/bench/http/http.go
>>
>>
>> On Wed, Jan 29, 2014 at 3:20 PM, Brad Fitzpatrick <bradfitz@golang.org>
>> wrote:
>> > Where is the source to that benchmark?
>> >
>> >
>> >
>> > On Wed, Jan 29, 2014 at 12:18 PM, <dvyukov@google.com> wrote:
>> >>
>> >> FYI
>> >>
>> >>
>> >> ---------- Forwarded message ----------
>> >> Date: Wed, Jan 29, 2014 at 3:13 PM
>> >> Subject: Perf changes on windows-amd64 by net/http: reuse client
>> >> connections earlier when Content-Length is set
>> >>
>> >>
>> >> Change d92b32d188ec caused perf changes on windows-amd64:
>> >>
>> >>
>> >> net/http: reuse client connections earlier when Content-Length is set
>> >>
>> >> Set EOF on the final Read of a body with a Content-Length, which
>> >> will cause clients to recycle their connection immediately upon
>> >> the final Read, rather than waiting for another Read or Close
>> >> (neither of which might come). This happens often when client
>> >> code is simply something like:
>> >>
>> >> err := json.NewDecoder(resp.Body).Decode(&dest)
>> >> ...
>> >>
>> >> Then there's usually no subsequent Read. Even if the client
>> >> calls Close (which they sh
>> >>
>> >> http://code.google.com/p/go/source/detail?r=d92b32d188ec
>> >>
>> >> http-1 old new delta
>> >> allocated 7003 5466 -21.95
>> >> gc-pause-total 3663 2902 -20.78
>> >>
>> >> http-8 old new delta
>> >> allocated 11402 9821 -13.87
>> >> gc-pause-total 10038 8537 -14.95
>> >>
>> >>
>> >>
>> >> http://goperfd.appspot.com/perfdetail?commit=d92b32d188ec18b61690d3c8e74c02e03da66105&commit0=9c65fe4ce5a23c834205b5767f2bc766922da6f1&kind=builder&builder=windows-amd64
>> >>
>> >>
>> >> https://codereview.appspot.com/49570044/
>> >
>> >
Message from bradfitz@golang.org
2014-01-29T12:04:14+00:00bradfitzurn:md5:4a4622065c787dec09456c458c01adef
I see....
tl;dr: ioutil.ReadAll uses a bytes.Buffer + calls its ReadFrom.
bytes.Buffer.ReadFrom tries to grow a slice before a subsequent Read if
capacity is too low. Now it gets the info (the EOF) in one Read, so it
never needs to grow the slice.
I can reproduce with:
$ go test -v -run=none -bench=ClientServer$ -benchtime=2s
-memprofile=mem.prof
Before:
$ go tool pprof --alloc_space http.test mem.prof
(pprof) top
Total: 406.5 MB
74.0 18.2% 18.2% 74.0 18.2% *bytes.makeSlice*
71.5 17.6% 35.8% 71.5 17.6% newselect
51.5 12.7% 48.5% 56.0 13.8%
net/textproto.(*Reader).ReadMIMEHeader
35.0 8.6% 57.1% 109.5 26.9% io/ioutil.readAll
25.0 6.2% 63.2% 25.0 6.2% net/textproto.MIMEHeader.Set
14.0 3.4% 66.7% 61.5 15.1% net/http.(*persistConn).readLoop
14.0 3.4% 70.1% 14.0 3.4% net/url.parse
13.5 3.3% 73.4% 13.5 3.3% makemap_c
13.5 3.3% 76.8% 96.5 23.7% net/http.(*persistConn).roundTrip
13.0 3.2% 80.0% 25.0 6.2% net/http.NewRequest
After:
$ go tool pprof --alloc_space http.test mem.prof
(pprof) top
Total: 321.0 MB
60.5 18.8% 18.8% 60.5 18.8% newselect
53.5 16.7% 35.5% 63.5 19.8%
net/textproto.(*Reader).ReadMIMEHeader
40.0 12.5% 48.0% 41.5 12.9% io/ioutil.readAll
18.0 5.6% 53.6% 18.0 5.6% makemap_c
17.0 5.3% 58.9% 17.0 5.3% net/textproto.MIMEHeader.Set
14.5 4.5% 63.4% 72.0 22.4% net/http.(*persistConn).readLoop
14.5 4.5% 67.9% 14.5 4.5% net/url.parse
12.5 3.9% 71.8% 27.5 8.6% net/http.NewRequest
12.0 3.7% 75.5% 83.0 25.9% net/http.(*persistConn).roundTrip
12.0 3.7% 79.3% 44.0 13.7% net/http.ReadRequest
On Wed, Jan 29, 2014 at 12:45 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
> Both builders agree on this change:
>
> http://goperfd.appspot.com/perfdetail?commit=d92b32d188ec18b61690d3c8e74c02e03da66105&builder=linux-amd64&benchmark=http
>
>
> On Wed, Jan 29, 2014 at 3:38 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > Hmmm. I'm surprised.
> >
> > It's not obvious why there are fewer allocations.
> >
> > On Jan 29, 2014 12:25 PM, "Dmitry Vyukov" <dvyukov@google.com> wrote:
> >>
> >> https://code.google.com/p/goperfd/source/browse/bench/http/http.go
> >>
> >>
> >> On Wed, Jan 29, 2014 at 3:20 PM, Brad Fitzpatrick <bradfitz@golang.org>
> >> wrote:
> >> > Where is the source to that benchmark?
> >> >
> >> >
> >> >
> >> > On Wed, Jan 29, 2014 at 12:18 PM, <dvyukov@google.com> wrote:
> >> >>
> >> >> FYI
> >> >>
> >> >>
> >> >> ---------- Forwarded message ----------
> >> >> Date: Wed, Jan 29, 2014 at 3:13 PM
> >> >> Subject: Perf changes on windows-amd64 by net/http: reuse client
> >> >> connections earlier when Content-Length is set
> >> >>
> >> >>
> >> >> Change d92b32d188ec caused perf changes on windows-amd64:
> >> >>
> >> >>
> >> >> net/http: reuse client connections earlier when Content-Length is set
> >> >>
> >> >> Set EOF on the final Read of a body with a Content-Length, which
> >> >> will cause clients to recycle their connection immediately upon
> >> >> the final Read, rather than waiting for another Read or Close
> >> >> (neither of which might come). This happens often when client
> >> >> code is simply something like:
> >> >>
> >> >> err := json.NewDecoder(resp.Body).Decode(&dest)
> >> >> ...
> >> >>
> >> >> Then there's usually no subsequent Read. Even if the client
> >> >> calls Close (which they sh
> >> >>
> >> >> http://code.google.com/p/go/source/detail?r=d92b32d188ec
> >> >>
> >> >> http-1 old new delta
> >> >> allocated 7003 5466 -21.95
> >> >> gc-pause-total 3663 2902 -20.78
> >> >>
> >> >> http-8 old new delta
> >> >> allocated 11402 9821 -13.87
> >> >> gc-pause-total 10038 8537 -14.95
> >> >>
> >> >>
> >> >>
> >> >>
> http://goperfd.appspot.com/perfdetail?commit=d92b32d188ec18b61690d3c8e74c02e03da66105&commit0=9c65fe4ce5a23c834205b5767f2bc766922da6f1&kind=builder&builder=windows-amd64
> >> >>
> >> >>
> >> >> https://codereview.appspot.com/49570044/
> >> >
> >> >
>