code review 49570044: net/http: reuse client connections earlier when Content...

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.

[bradfitz, 2014-01-29 09:23:32]

Hello adg@golang.org (cc: golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[adg, 2014-01-29 10:04:18]

Apologies if this comment misses the entire point of the change.

https://codereview.appspot.com/49570044/diff/40001/src/pkg/net/http/transport...
File src/pkg/net/http/transport_test.go (right):

https://codereview.appspot.com/49570044/diff/40001/src/pkg/net/http/transport...
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.

[bradfitz, 2014-01-29 10:06:52]

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

[adg, 2014-01-29 10:08:23]

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

[bradfitz, 2014-01-29 10:14:08]

Hello adg@golang.org (cc: golang-codereviews@googlegroups.com),

Please take another look.

[adg, 2014-01-29 10:15:46]

LGTM

[bradfitz, 2014-01-29 10:23:48]

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

[dvyukov, 2014-01-29 11:18:05]

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=d92b32d188ec18b61690d3c8e74c02e0...

[bradfitz, 2014-01-29 11:20:11]

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

[dvyukov, 2014-01-29 11:25:25]

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=d92b32d188ec18b61690d3c8e74c02e0...
>>
>>
>> https://codereview.appspot.com/49570044/
>
>

[bradfitz, 2014-01-29 11:38:50]

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=d92b32d188ec18b61690d3c8e74c02e0...
> >>
> >>
> >> https://codereview.appspot.com/49570044/
> >
> >
>

[dvyukov, 2014-01-29 11:45:38]

Both builders agree on this change:
http://goperfd.appspot.com/perfdetail?commit=d92b32d188ec18b61690d3c8e74c02e0...


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=d92b32d188ec18b61690d3c8e74c02e0...
>> >>
>> >>
>> >> https://codereview.appspot.com/49570044/
>> >
>> >

[bradfitz, 2014-01-29 12:04:14]

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=d92b32d188ec18b61690d3c8e74c02e0...
>
>
> 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=d92b32d188ec18b61690d3c8e74c02e0...
> >> >>
> >> >>
> >> >> https://codereview.appspot.com/49570044/
> >> >
> >> >
>