Code review - Issue 9492044: code review 9492044: net/http: simplify transfer body; reduces allocations toohttps://codereview.appspot.com/2013-05-20T19:28:20+00:00rietveld
Message from unknown
2013-05-20T03:56:54+00:00bradfitzurn:md5:8b3b409ed5d4196b685e8c342c26c33e
Message from unknown
2013-05-20T03:56:57+00:00bradfitzurn:md5:85c847f0019eb90cea096f412b5557ea
Message from unknown
2013-05-20T03:57:01+00:00bradfitzurn:md5:0588160f1f30ec92eb3901a11035a6e3
Message from bradfitz@golang.org
2013-05-20T03:57:05+00:00bradfitzurn:md5:f8f00c3825d75e7e3525ebc491831402
Hello golang-dev@googlegroups.com,
I'd like you to review this change to
https://go.googlecode.com/hg/
Message from daniel.morsing@gmail.com
2013-05-20T13:58:38+00:00DMorsingurn:md5:48239d2ca74e6bc92e416706c96c34b2
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#newcode1824
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.
Message from unknown
2013-05-20T14:23:55+00:00bradfitzurn:md5:0da47067355ec89090217d4018c38ce4
Message from bradfitz@golang.org
2013-05-20T14:24:02+00:00bradfitzurn:md5:66230094e12b2f19555e063d8fc55817
*** 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#newcode1824
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.
Message from daniel.morsing@gmail.com
2013-05-20T15:06:39+00:00DMorsingurn:md5:5cc8ce49a6aa4e955d4a26a3af1173b3
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.
Message from bradfitz@golang.org
2013-05-20T15:11:49+00:00bradfitzurn:md5:a7590eded164984c97a218674dd387f0
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.
Message from minux.ma@gmail.com
2013-05-20T15:17:19+00:00minux1urn:md5:2bd45d568e94a80185e580ae78aa6228
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.
Message from daniel.morsing@gmail.com
2013-05-20T16:22:53+00:00DMorsingurn:md5:5ddee103f84df8af8adef1f669bd534c
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?
Message from bradfitz@golang.org
2013-05-20T17:58:30+00:00bradfitzurn:md5:ee3ff8d7d03322c1ef0da7e4793238c7
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.
Message from daniel.morsing@gmail.com
2013-05-20T19:28:20+00:00DMorsingurn:md5:35de995b91837abb3da7c699b87ab259
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.