code review 9492044: net/http: simplify transfer body; reduces allocations too

net/http: simplify transfer body; reduces allocations too

benchmark                                   old ns/op    new ns/op    delta
BenchmarkServerFakeConnNoKeepAlive              14431        14247   -1.28%
BenchmarkServerFakeConnWithKeepAlive            11618        11357   -2.25%
BenchmarkServerFakeConnWithKeepAliveLite         6735         6427   -4.57%
BenchmarkServerHandlerTypeLen                    8842         8740   -1.15%
BenchmarkServerHandlerNoLen                      8001         7828   -2.16%
BenchmarkServerHandlerNoType                     8270         8227   -0.52%
BenchmarkServerHandlerNoHeader                   6148         5920   -3.71%

benchmark                                  old allocs   new allocs    delta
BenchmarkServerFakeConnNoKeepAlive                 30           29   -3.33%
BenchmarkServerFakeConnWithKeepAlive               25           24   -4.00%
BenchmarkServerFakeConnWithKeepAliveLite           10            9  -10.00%
BenchmarkServerHandlerTypeLen                      18           17   -5.56%
BenchmarkServerHandlerNoLen                        15           14   -6.67%
BenchmarkServerHandlerNoType                       16           15   -6.25%
BenchmarkServerHandlerNoHeader                     10            9  -10.00%

benchmark                                   old bytes    new bytes    delta
BenchmarkServerFakeConnNoKeepAlive               2557         2492   -2.54%
BenchmarkServerFakeConnWithKeepAlive             2260         2194   -2.92%
BenchmarkServerFakeConnWithKeepAliveLite         1092         1026   -6.04%
BenchmarkServerHandlerTypeLen                    1941         1875   -3.40%
BenchmarkServerHandlerNoLen                      1898         1832   -3.48%
BenchmarkServerHandlerNoType                     1906         1840   -3.46%
BenchmarkServerHandlerNoHeader                   1092         1026   -6.04%

Update Issue 5195

[bradfitz, 2013-05-20 03:57:05]

Hello golang-dev@googlegroups.com,

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

[DMorsing, 2013-05-20 13:58:38]

LGTM

https://codereview.appspot.com/9492044/diff/5001/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/9492044/diff/5001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:1824: ioutil.NopCloser(strings.NewReader("")),
NopCloser doesn't need a reader if the read method is never called. Just pass it
nil.

[bradfitz, 2013-05-20 14:24:02]

*** Submitted as https://code.google.com/p/go/source/detail?r=d1d76fc0ab6a ***

net/http: simplify transfer body; reduces allocations too

benchmark                                   old ns/op    new ns/op    delta
BenchmarkServerFakeConnNoKeepAlive              14431        14247   -1.28%
BenchmarkServerFakeConnWithKeepAlive            11618        11357   -2.25%
BenchmarkServerFakeConnWithKeepAliveLite         6735         6427   -4.57%
BenchmarkServerHandlerTypeLen                    8842         8740   -1.15%
BenchmarkServerHandlerNoLen                      8001         7828   -2.16%
BenchmarkServerHandlerNoType                     8270         8227   -0.52%
BenchmarkServerHandlerNoHeader                   6148         5920   -3.71%

benchmark                                  old allocs   new allocs    delta
BenchmarkServerFakeConnNoKeepAlive                 30           29   -3.33%
BenchmarkServerFakeConnWithKeepAlive               25           24   -4.00%
BenchmarkServerFakeConnWithKeepAliveLite           10            9  -10.00%
BenchmarkServerHandlerTypeLen                      18           17   -5.56%
BenchmarkServerHandlerNoLen                        15           14   -6.67%
BenchmarkServerHandlerNoType                       16           15   -6.25%
BenchmarkServerHandlerNoHeader                     10            9  -10.00%

benchmark                                   old bytes    new bytes    delta
BenchmarkServerFakeConnNoKeepAlive               2557         2492   -2.54%
BenchmarkServerFakeConnWithKeepAlive             2260         2194   -2.92%
BenchmarkServerFakeConnWithKeepAliveLite         1092         1026   -6.04%
BenchmarkServerHandlerTypeLen                    1941         1875   -3.40%
BenchmarkServerHandlerNoLen                      1898         1832   -3.48%
BenchmarkServerHandlerNoType                     1906         1840   -3.46%
BenchmarkServerHandlerNoHeader                   1092         1026   -6.04%

Update Issue 5195

R=golang-dev, daniel.morsing
CC=golang-dev
https://codereview.appspot.com/9492044

https://codereview.appspot.com/9492044/diff/5001/src/pkg/net/http/server.go
File src/pkg/net/http/server.go (right):

https://codereview.appspot.com/9492044/diff/5001/src/pkg/net/http/server.go#n...
src/pkg/net/http/server.go:1824: ioutil.NopCloser(strings.NewReader("")),
On 2013/05/20 13:58:38, DMorsing wrote:
> NopCloser doesn't need a reader if the read method is never called. Just pass
it
> nil.

Done.

[DMorsing, 2013-05-20 15:06:39]

Second pass over this made me realise that ioutil.Discard implements ReaderFrom.
io.Copy prioritizes ReaderFroms over WriterTos, so the embedding change would be
a no-op.

Perhaps it's a good idea to switch the priorities in io.Copy. ReadFrom requires
that a buffer be created for the reader to write into, while WriteTo will use
the buffer internal to the reader.

[bradfitz, 2013-05-20 15:11:49]

On Mon, May 20, 2013 at 8:06 AM, <daniel.morsing@gmail.com> wrote:

> Second pass over this made me realise that ioutil.Discard implements
> ReaderFrom. io.Copy prioritizes ReaderFroms over WriterTos, so the
> embedding change would be a no-op.
>
> Perhaps it's a good idea to switch the priorities in io.Copy. ReadFrom
> requires that a buffer be created for the reader to write into, while
> WriteTo will use the buffer internal to the reader.
>

Interesting.

At least ioutil.Discard's ReadFrom ends up re-using a blackhole buffer, so
amortized, there's no allocation, but there are channel operations for now
(until we have a Cache type or similar).

I wonder whether changing the ReaderFrom/WriterTo order could break any
programs.  It makes me nervous from a Go1 compatibility promise angle at
least.

[minux1, 2013-05-20 15:17:19]

On Mon, May 20, 2013 at 11:06 PM, <daniel.morsing@gmail.com> wrote:

> Second pass over this made me realise that ioutil.Discard implements
> ReaderFrom. io.Copy prioritizes ReaderFroms over WriterTos, so the
> embedding change would be a no-op.
>
> Perhaps it's a good idea to switch the priorities in io.Copy. ReadFrom
> requires that a buffer be created for the reader to write into, while
> WriteTo will use the buffer internal to the reader.

It's also possible for Writers that implement ReadFrom to have its own write
buffer so that it could pass it to the Reader and avoid extra garbage.

[DMorsing, 2013-05-20 16:22:53]

On Mon, May 20, 2013 at 5:16 PM, minux <minux.ma@gmail.com> wrote:
>
> On Mon, May 20, 2013 at 11:06 PM, <daniel.morsing@gmail.com> wrote:
>>
>> Second pass over this made me realise that ioutil.Discard implements
>> ReaderFrom. io.Copy prioritizes ReaderFroms over WriterTos, so the
>> embedding change would be a no-op.
>>
>> Perhaps it's a good idea to switch the priorities in io.Copy. ReadFrom
>> requires that a buffer be created for the reader to write into, while
>> WriteTo will use the buffer internal to the reader.
>
> It's also possible for Writers that implement ReadFrom to have its own write
> buffer so that it could pass it to the Reader and avoid extra garbage.

Hopefully, types that implement ReaderFrom and WriterTo have buffers
internally that they can subslice, otherwise the rationale behind
implementing them would be moot. If both Reader and Writer implement
WriterTo and ReaderFrom, choosing between them should give the same
cost.

Which makes me wonder, Why does ioutil.Discard even implement the
ReaderFrom? Wouldn't it be better to just implement the buffer cache
for io.Copy?

[bradfitz, 2013-05-20 17:58:30]

On Mon, May 20, 2013 at 9:22 AM, Daniel Morsing
<daniel.morsing@gmail.com>wrote:

> On Mon, May 20, 2013 at 5:16 PM, minux <minux.ma@gmail.com> wrote:
> >
> > On Mon, May 20, 2013 at 11:06 PM, <daniel.morsing@gmail.com> wrote:
> >>
> >> Second pass over this made me realise that ioutil.Discard implements
> >> ReaderFrom. io.Copy prioritizes ReaderFroms over WriterTos, so the
> >> embedding change would be a no-op.
> >>
> >> Perhaps it's a good idea to switch the priorities in io.Copy. ReadFrom
> >> requires that a buffer be created for the reader to write into, while
> >> WriteTo will use the buffer internal to the reader.
> >
> > It's also possible for Writers that implement ReadFrom to have its own
> write
> > buffer so that it could pass it to the Reader and avoid extra garbage.
>
> Hopefully, types that implement ReaderFrom and WriterTo have buffers
> internally that they can subslice, otherwise the rationale behind
> implementing them would be moot. If both Reader and Writer implement
> WriterTo and ReaderFrom, choosing between them should give the same
> cost.
>
> Which makes me wonder, Why does ioutil.Discard even implement the
> ReaderFrom?


Because it's safe.


> Wouldn't it be better to just implement the buffer cache
> for io.Copy?
>

I discarded https://codereview.appspot.com/7206048/ (maybe prematurely?)
but opened https://code.google.com/p/go/issues/detail?id=5509 at least.
 The thing I didn't like about the io.Copy one (in addition to adding yet
another buffered-channel-as-cache) was that it's potentially unsafe, if a
Reader/Writer retains a reference to the p []byte arg, which they really
shouldn't do.

[DMorsing, 2013-05-20 19:28:20]

On Mon, May 20, 2013 at 7:58 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> On Mon, May 20, 2013 at 9:22 AM, Daniel Morsing <daniel.morsing@gmail.com>
> wrote:
>>
>> On Mon, May 20, 2013 at 5:16 PM, minux <minux.ma@gmail.com> wrote:
>> >
>> > On Mon, May 20, 2013 at 11:06 PM, <daniel.morsing@gmail.com> wrote:
>> >>
>> >> Second pass over this made me realise that ioutil.Discard implements
>> >> ReaderFrom. io.Copy prioritizes ReaderFroms over WriterTos, so the
>> >> embedding change would be a no-op.
>> >>
>> >> Perhaps it's a good idea to switch the priorities in io.Copy. ReadFrom
>> >> requires that a buffer be created for the reader to write into, while
>> >> WriteTo will use the buffer internal to the reader.
>> >
>> > It's also possible for Writers that implement ReadFrom to have its own
>> > write
>> > buffer so that it could pass it to the Reader and avoid extra garbage.
>>
>> Hopefully, types that implement ReaderFrom and WriterTo have buffers
>> internally that they can subslice, otherwise the rationale behind
>> implementing them would be moot. If both Reader and Writer implement
>> WriterTo and ReaderFrom, choosing between them should give the same
>> cost.
>>
>> Which makes me wonder, Why does ioutil.Discard even implement the
>> ReaderFrom?
>
>
> Because it's safe.
>
>>
>> Wouldn't it be better to just implement the buffer cache
>> for io.Copy?
>
>
> I discarded https://codereview.appspot.com/7206048/ (maybe prematurely?) but
> opened https://code.google.com/p/go/issues/detail?id=5509 at least.  The
> thing I didn't like about the io.Copy one (in addition to adding yet another
> buffered-channel-as-cache) was that it's potentially unsafe, if a
> Reader/Writer retains a reference to the p []byte arg, which they really
> shouldn't do.
>

Ok, That makes removal of ReaderTo from ioutil.Discard out of the
question. That makes it tempting to switch the priorities in io.Copy.

There are other reasons to switch priorities. If we have well behaved
WriterTos that keep their data in memory and issues Write with one big
slice, the size of the data will be known before it's processed by the
Writer. An example is bytes.Buffer. If we use its ReaderFrom method,
it might read multiple times, having to allocate whenever it runs out
of buffer. If we issue one big write to it, it will only allocate
once.

I'll send out a CL later where we can move this discussion.