Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(924)

Issue 9492044: code review 9492044: net/http: simplify transfer body; reduces allocations too (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 10 months ago by bradfitz
Modified:
10 years, 10 months ago
Reviewers:
minux1, DMorsing
CC:
golang-dev
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : diff -r 647f336edfe8 https://go.googlecode.com/hg/ #

Patch Set 3 : diff -r 647f336edfe8 https://go.googlecode.com/hg/ #

Total comments: 2

Patch Set 4 : diff -r 9b75b7eb2191 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -14 lines) Patch
M src/pkg/net/http/server.go View 1 2 3 1 chunk +9 lines, -1 line 0 comments Download
M src/pkg/net/http/transfer.go View 1 4 chunks +3 lines, -13 lines 0 comments Download

Messages

Total messages: 9
bradfitz
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
10 years, 10 months ago (2013-05-20 03:57:05 UTC) #1
DMorsing
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 ...
10 years, 10 months ago (2013-05-20 13:58:38 UTC) #2
bradfitz
*** Submitted as https://code.google.com/p/go/source/detail?r=d1d76fc0ab6a *** net/http: simplify transfer body; reduces allocations too benchmark old ns/op ...
10 years, 10 months ago (2013-05-20 14:24:02 UTC) #3
DMorsing
Second pass over this made me realise that ioutil.Discard implements ReaderFrom. io.Copy prioritizes ReaderFroms over ...
10 years, 10 months ago (2013-05-20 15:06:39 UTC) #4
bradfitz
On Mon, May 20, 2013 at 8:06 AM, <daniel.morsing@gmail.com> wrote: > Second pass over this ...
10 years, 10 months ago (2013-05-20 15:11:49 UTC) #5
minux1
On Mon, May 20, 2013 at 11:06 PM, <daniel.morsing@gmail.com> wrote: > Second pass over this ...
10 years, 10 months ago (2013-05-20 15:17:19 UTC) #6
DMorsing
On Mon, May 20, 2013 at 5:16 PM, minux <minux.ma@gmail.com> wrote: > > On Mon, ...
10 years, 10 months ago (2013-05-20 16:22:53 UTC) #7
bradfitz
On Mon, May 20, 2013 at 9:22 AM, Daniel Morsing <daniel.morsing@gmail.com>wrote: > On Mon, May ...
10 years, 10 months ago (2013-05-20 17:58:30 UTC) #8
DMorsing
10 years, 10 months ago (2013-05-20 19:28:20 UTC) #9
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b