code review 79110043: bufio: improve ReadFrom

bufio: improve ReadFrom

Writer.ReadFrom currently uses underlying writer's
ReadFrom method only when internal buffer is empty.
Also, it does not utilize given Reader's WriteTo
method even if it implements WriterTo. It's not
optional.

This patch is to use underlying value's ReadFrom and
WriteTo methods as long as they are available.

[ruiu, 2014-03-22 23:03:17]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[r, 2014-03-26 05:04:06]

We're in a feature freeze for 1.3. Please ping us after the 1.3 release.

[gobot, 2014-06-16 20:10:00]

R=rsc@golang.org (assigned by r@golang.org)

[ruiu, 2014-07-02 01:00:20]

Ping.

[rsc, 2014-07-02 01:15:47]

test, sorry

[rsc, 2014-07-02 01:22:08]

https://1349-161c4bc39310-tainted-dot-codereview.appspot.com/79110043/diff/60...
File src/pkg/bufio/bufio.go (left):

https://1349-161c4bc39310-tainted-dot-codereview.appspot.com/79110043/diff/60...
src/pkg/bufio/bufio.go:628: m, err = r.Read(b.buf[b.n:])
It seems to me that you could keep all the existing code and just move the
buffered == 0 check here, resulting in something like a 5-line diff.
You could set first := true before the loop and make the check

if first && b.Buffered() == 0 {
    first = false
    if w, ok := ...
}

and that would avoid doing the check each time through the loop.

The rewrite in the CL introduces a new helper method that seems to be
unnecessary, and despite that, ends up duplicating quite a bit of code. Just
moving the check here in the original would not duplicate anything.

[rsc, 2014-07-02 01:31:54]

another test, sorry

[rsc, 2014-07-02 01:41:16]

final test for now, sorry

[ruiu, 2014-07-02 23:54:00]

Thank you for reviewing. PTAL.

https://codereview.appspot.com/79110043/diff/60001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (left):

https://codereview.appspot.com/79110043/diff/60001/src/pkg/bufio/bufio.go#old...
src/pkg/bufio/bufio.go:628: m, err = r.Read(b.buf[b.n:])
Good point. This code does not even have to be in the loop. I moved it to the
beginning of the function and removed the helper function.

[pongad, 2014-07-03 04:41:42]

This only partly addresses the issue discussed in
https://groups.google.com/forum/#!msg/golang-nuts/oYYggbLntVE/DMsqEyIU-yIJ
Though I think if you move the two if statements into the end of the loop
similar to rsc's suggestion, it should solve the issue quite handsomely.
I am a little worried that we are using from WriteTo and ReadFrom. It seems like
if we are not careful reading a bufio.Reader into a bufio.Writer might try to
shortcut into each other's ReadTo/WriteFrom and end in infinite recursion.
Could you also post some benchmark numbers? I have two new benchmarks in
https://codereview.appspot.com/103710044/ that should benchmark for cases
discussed in the thread I linked above. Hopefully we will see some improvements.

Michael

[ruiu, 2014-07-03 23:30:04]

On 2014/07/03 04:41:42, pongad wrote:
> This only partly addresses the issue discussed in
> https://groups.google.com/forum/#!msg/golang-nuts/oYYggbLntVE/DMsqEyIU-yIJ
> Though I think if you move the two if statements into the end of the loop
> similar to rsc's suggestion, it should solve the issue quite handsomely.

Are you suggesting code like this?
https://codereview.appspot.com/106410043/diff/20001/src/pkg/bufio/bufio.go

> I am a little worried that we are using from WriteTo and ReadFrom. It seems
like
> if we are not careful reading a bufio.Reader into a bufio.Writer might try to
> shortcut into each other's ReadTo/WriteFrom and end in infinite recursion.

It wouldn't cause a infinite recursion. On each iteration, ReadFrom or WriteTo
methods are called against a bufio's underlying reader, so the recursion will
terminate at some point. (If the bufio wraps itself it won't stop, but it's an
apparent programmer's error.)

> Could you also post some benchmark numbers? I have two new benchmarks in
> https://codereview.appspot.com/103710044/ that should benchmark for cases
> discussed in the thread I linked above. Hopefully we will see some
improvements.

See the following result:

benchmark                     old ns/op     new ns/op     delta       
ReaderCopyOptimal             182           188           +3.30%
ReaderCopyUnoptimal           299           296           -1.00%
ReaderCopyNoWriteTo           12491         12453         -0.30%
ReaderWriteToOptimal          575           578           +0.52%
WriterCopyOptimal             179           180           +0.56%
WriterCopyUnoptimal           259           266           +2.70%
WriterCopyNoReadFrom          14053         13423         -4.48%
WriterReadFromNotEmpty        1835          1301          -29.10%
WriterReadFromSmallChunks     64928         67688         +4.25%
ReaderEmpty                   2490          2479          -0.44%
WriterEmpty                   2624          2526          -3.73%
WriterFlush                   19.2          19.4          +1.04%

[pongad, 2014-07-05 23:01:56]

You are right; my fears are banished.

I'm a little surprised that SmallChunks benchmark got worse, though the NotEmpty
benchmark looks encouraging.

If you want to change the description of the CL to also fix Issue 6373, we will
probably be able to shoot two issues with one CL. If that works out, I'll just
close my CL.

[dave_cheney.net, 2014-07-16 10:06:35]

On 2014/07/05 23:01:56, pongad wrote:
> You are right; my fears are banished.
> 
> I'm a little surprised that SmallChunks benchmark got worse, though the
NotEmpty
> benchmark looks encouraging.
> 
> If you want to change the description of the CL to also fix Issue 6373, we
will
> probably be able to shoot two issues with one CL. If that works out, I'll just
> close my CL.

I'd like to see these test additions in their own CL. I think they are of value.

As for this CL, i'm on the fence, ReaderFrom/WriterTo are subtle and there isn't
enough justification in this CL to warrant fixing something that isn't
demonstrably broken. 

If you were to add some benchmarks or tests, possibly that count the number of
invocations of ReaderFrom/WriteTo rather than just the bytes shoved through
them, I think that would be valuable.

[bradfitz, 2014-07-16 18:00:34]

R=gri

... since he's expressed concerns about the bufio package most recently.

[gri, 2014-07-16 21:38:59]

This seems unrelated to issue 7611 which has been fixed before. At least the CL
desc needs to be updated.

Also, I'm not sure I understand this change. For one, the test that you added
doesn't actually seem to test your changes. If I simply appy the changes to
bufio_test.go (w/o the bufio.go changes), the tests still pass. So the test is
either not correct or not exposing the problem.

Please clarify.

[ruiu, 2014-07-17 05:16:09]

Updated both the CL description and the test.
The test now correctly test my change to check if
ReadFrom or WriteTo is used as expected. Please
take another look.

[gri, 2014-07-21 18:11:53]

The test still does not test the change. If I remove the bufio.go changes and
run the tests, the tests pass.

[gobot, 2014-09-28 05:24:50]

R=gri@golang.org (assigned by satish.puranam@gmail.com)

[gri, 2014-09-30 17:49:50]

This CL needs to be updated first to match the new directory structure (no pkg
anymore in path).

[gobot, 2014-12-19 05:18:32]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/79110043 is best) in the description in your
new CL.

Thanks very much.