code review 6548047: bufio: Implement io.WriterTo for (*Reader) (issue 6548047)

[mch...@gmail.com]

Reviewers: golang-dev_googlegroups.com,

Message:
Hello golan...@googlegroups.com,

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


Description:
bufio: Implement io.WriterTo for (*Reader)

This is part 1 of 2 for issue 4028

Please review this at http://codereview.appspot.com/6548047/

Affected files:
M src/pkg/bufio/bufio.go
M src/pkg/bufio/bufio_test.go

[mch...@gmail.com]

On 2012/09/21 08:51:30, chaten wrote:
> Hello mailto:golan...@googlegroups.com,

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

Benchmark Data:
benchmark old ns/op new ns/op delta
BenchmarkReaderCopyOptimal 33495 9849 -70.60%
BenchmarkReaderCopyUnoptimal 70631 27041 -61.72%
BenchmarkReaderCopyOldImpl 51407 52970 +3.04%


http://codereview.appspot.com/6548047/

[Dave Cheney]

Updates issue 4028.

[Nigel Tao]

On 21 September 2012 18:55, Dave Cheney <da...@cheney.net> wrote:
> Updates issue 4028.

Drop the s: "Update issue 4028".

http://code.google.com/p/support/wiki/IssueTracker#Integration_with_version_control

[r...@golang.org]

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode378
src/pkg/bufio/bufio.go:378: // WriteTo first writes the contents of the
buffer into the writer,
This describes how good the implementation is. The reader should assume
it is good. The coment should say what the function's effect is.

// WriteTo copies data from the reader into the writer w until there's
no more
// data to write or an error occurs. It returns the number of bytes
written
// and the encountered error, if any.

(mostly lifted from io.WriterTo's docs)

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode382
src/pkg/bufio/bufio.go:382: func (b *Reader) WriteTo(wtr io.Writer) (n
int64, err error) {
Writers are almost always called w, not wtr.

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode388
src/pkg/bufio/bufio.go:388: if writerTo, ok := b.rd.(io.WriterTo); ok {
s/writerTo/r/
It's still a reader first and foremost.

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode389
src/pkg/bufio/bufio.go:389: j, err := writerTo.WriteTo(wtr)
j is a little strange for a count, especially since there's no i. The
next thing after n is usually nn or m in this code.

m, err := r.WriteTo(w)
n += m
return n, err

and then no else.
golang.org/doc/effective_go.html#if

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode396
src/pkg/bufio/bufio.go:396: b.fill()
The sequencing here is a little tricky. You want to delay processing the
read error until after you write what did get read, as a write error
then would correspond to an earlier input byte than the read error. I
think this would work:

for b.fill(); b.r < b.w; b.fill() {
m, err := b.writeTo(w)
n += m
if err != nil {
return n, err
}
}
if b.err == io.EOF {
b.err = nil
}
return n, b.readErr()

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode416
src/pkg/bufio/bufio.go:416: func (b *Reader) writeBufTo(wtr io.Writer)
(int64, error) {
writeTo is fine. the 'Buf' is unnecessary.
Also just call the writer w.

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go
File src/pkg/bufio/bufio_test.go (right):

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode767
src/pkg/bufio/bufio_test.go:767: input := make([]byte, 8096)
Interesting constant. Did you mean 8<<10 (8192)?

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode771
src/pkg/bufio/bufio_test.go:771: inputRdr := bytes.NewBuffer(input)
You never use this variable again. Eliminate it:

r := NewReader(bytes.NewReader(input))

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode772
src/pkg/bufio/bufio_test.go:772: outputWtr := new(bytes.Buffer)
w := new(bytes.Buffer)

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode775
src/pkg/bufio/bufio_test.go:775:
Can collapse the next 8 lines into

if n != int64(len(input)) || err != nil {
t.Fatalf("r.WriteTo(w) = %d, %v, want %d, nil", n, err, len(input))
}

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode786
src/pkg/bufio/bufio_test.go:786: t.Errorf("Invalid byte written at index
%d. Expected %d, was %d.", i, input[i], val)
Echo the values back in the same order as the comparison, and use Go
syntax when possible, like in the new Fatalf in the last comment.
t.Errorf("after write: out[%#x] = %#x, want %#x", i, val, input[i])

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode791
src/pkg/bufio/bufio_test.go:791: //Indirect reader masks any other
interface a reader might implement, like io.WriterTo
Please call this 'onlyReader'. The type name should appear verbatim,
without additional spaces or case changes:

// An onlyReader only implements io.Reader, no matter what other methods
// the underlying implementation may have.

http://codereview.appspot.com/6548047/

[brad...@golang.org]

https://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode378

src/pkg/bufio/bufio.go:378: // WriteTo first writes the contents of the
buffer into the writer,

On 2012/09/24 15:34:09, rsc wrote:
> (mostly lifted from io.WriterTo's docs)

Can this paragraph just be: ?

// WriteTo implements io.WriterTo.

I've seen that style in a few places.

https://codereview.appspot.com/6548047/

[Russ Cox]

Sure.

[mch...@gmail.com]

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode378

src/pkg/bufio/bufio.go:378: // WriteTo first writes the contents of the
buffer into the writer,
On 2012/09/24 15:34:09, rsc wrote:

> This describes how good the implementation is. The reader should
assume it is
> good. The coment should say what the function's effect is.

> // WriteTo copies data from the reader into the writer w until there's
no more
> // data to write or an error occurs. It returns the number of bytes
written
> // and the encountered error, if any.

> (mostly lifted from io.WriterTo's docs)

Didn't notice Tip io.WriterTo had docs, will go with brad's suggestion.

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode382
src/pkg/bufio/bufio.go:382: func (b *Reader) WriteTo(wtr io.Writer) (n
int64, err error) {

On 2012/09/24 15:34:09, rsc wrote:

> Writers are almost always called w, not wtr.

Noted

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode396
src/pkg/bufio/bufio.go:396: b.fill()

On 2012/09/24 15:34:09, rsc wrote:

> The sequencing here is a little tricky. You want to delay processing
the read
> error until after you write what did get read, as a write error then
would
> correspond to an earlier input byte than the read error. I think this
would
> work:

> for b.fill(); b.r < b.w; b.fill() {
> m, err := b.writeTo(w)
> n += m
> if err != nil {
> return n, err
> }
> }
> if b.err == io.EOF {
> b.err = nil
> }
> return n, b.readErr()

Fixed and added tests for the various permutations of errors.

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio.go#newcode416
src/pkg/bufio/bufio.go:416: func (b *Reader) writeBufTo(wtr io.Writer)
(int64, error) {

On 2012/09/24 15:34:09, rsc wrote:

> writeTo is fine. the 'Buf' is unnecessary.
> Also just call the writer w.

I'll rename it, but I don't think its the right decision. I feel like
the WriteTo code is less clear now that there is a private writeTo
method that only deals with the buffer. writeBufTo tells the reader that
it only is dealing with what is buffered.

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go
File src/pkg/bufio/bufio_test.go (right):

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode767
src/pkg/bufio/bufio_test.go:767: input := make([]byte, 8096)

On 2012/09/24 15:34:09, rsc wrote:

> Interesting constant. Did you mean 8<<10 (8192)?

Ugh, thats embarrassing. Fixed.

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode771
src/pkg/bufio/bufio_test.go:771: inputRdr := bytes.NewBuffer(input)

On 2012/09/24 15:34:09, rsc wrote:

> You never use this variable again. Eliminate it:

> r := NewReader(bytes.NewReader(input))

Done

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode775
src/pkg/bufio/bufio_test.go:775:

On 2012/09/24 15:34:09, rsc wrote:

> Can collapse the next 8 lines into

> if n != int64(len(input)) || err != nil {
> t.Fatalf("r.WriteTo(w) = %d, %v, want %d, nil", n, err,
len(input))
> }

Moved n,err := into the initializer block of the if as well

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode786
src/pkg/bufio/bufio_test.go:786: t.Errorf("Invalid byte written at index
%d. Expected %d, was %d.", i, input[i], val)

On 2012/09/24 15:34:09, rsc wrote:

> Echo the values back in the same order as the comparison, and use Go
syntax when
> possible, like in the new Fatalf in the last comment.
> t.Errorf("after write: out[%#x] = %#x, want %#x", i, val, input[i])

Done

http://codereview.appspot.com/6548047/diff/3/src/pkg/bufio/bufio_test.go#newcode791
src/pkg/bufio/bufio_test.go:791: //Indirect reader masks any other
interface a reader might implement, like io.WriterTo

On 2012/09/24 15:34:09, rsc wrote:

> Please call this 'onlyReader'. The type name should appear verbatim,
without
> additional spaces or case changes:

> // An onlyReader only implements io.Reader, no matter what other
methods
> // the underlying implementation may have.

Done and ditto for the writer.

http://codereview.appspot.com/6548047/

[mch...@gmail.com]

Hello da...@cheney.net, nige...@golang.org, r...@golang.org,
brad...@golang.org (cc: golan...@googlegroups.com),

Please take another look.


http://codereview.appspot.com/6548047/

[mch...@gmail.com]

[rogp...@gmail.com]

http://codereview.appspot.com/6548047/diff/16001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/6548047/diff/16001/src/pkg/bufio/bufio.go#newcode379
src/pkg/bufio/bufio.go:379:
d

http://codereview.appspot.com/6548047/diff/16001/src/pkg/bufio/bufio.go#newcode384
src/pkg/bufio/bufio.go:384: }
i wonder if it might be good to be robust in the case where the
underlying writer returns n<len(data) but does not return an error. if
that happens here and the writer implements WriteTo, we'll lose data in
a hard-to-diagnose way. a check in writeTo would be sufficient.

http://codereview.appspot.com/6548047/diff/16001/src/pkg/bufio/bufio.go#newcode408
src/pkg/bufio/bufio.go:408: func (b *Reader) writeTo(w io.Writer)
(int64, error) {
bike shedding i know, but...
s/writeTo/writeBuf/

it really isn't anything like the other WriteTo method.

http://codereview.appspot.com/6548047/

[mch...@gmail.com]

http://codereview.appspot.com/6548047/diff/16001/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

http://codereview.appspot.com/6548047/diff/16001/src/pkg/bufio/bufio.go#newcode384
src/pkg/bufio/bufio.go:384: }

On 2012/09/25 11:08:38, rog wrote:
> i wonder if it might be good to be robust in the case where the
underlying
> writer returns n<len(data) but does not return an error. if that
happens here
> and the writer implements WriteTo, we'll lose data in a
hard-to-diagnose way. a
> check in writeTo would be sufficient.

Hmm, I don't know the correct solution here for writers that are buggy.
Do you know of any examples elsewhere in the stdlib that work around
something like this?

One possible solution would be to keep writing until everything is
written, an error is returned or no data is written.

Another one would be to return io.ErrShortWrite if n != (w-r).

Or I could just panic.

Interested to hear your thoughts on this, I'll update the existing CR
with your other feedback in the meantime.

http://codereview.appspot.com/6548047/

[mch...@gmail.com]

Hello da...@cheney.net, nige...@golang.org, r...@golang.org,
brad...@golang.org, rogp...@gmail.com (cc:

[r...@golang.org]

looks fine to me. nigel or rog?


http://codereview.appspot.com/6548047/

[nige...@golang.org]

LGTM.

Please change the changelist description: s/Issue/issue/ in "Update
Issue 4028".

It'd also be nice if the description gave the benchmark numbers.


https://codereview.appspot.com/6548047/diff/11002/src/pkg/bufio/bufio_test.go
File src/pkg/bufio/bufio_test.go (right):

https://codereview.appspot.com/6548047/diff/11002/src/pkg/bufio/bufio_test.go#newcode768
src/pkg/bufio/bufio_test.go:768: for i, _ := range input {
for i := range input {

https://codereview.appspot.com/6548047/diff/11002/src/pkg/bufio/bufio_test.go#newcode814
src/pkg/bufio/bufio_test.go:814: // An onlyReader only implements
io.Reader, no matter what other methods the underlying implementation
may have
Add a full stop to complete the sentence.

Similarly below.

https://codereview.appspot.com/6548047/

[nige...@golang.org]

https://codereview.appspot.com/6548047/diff/11002/src/pkg/bufio/bufio.go
File src/pkg/bufio/bufio.go (right):

https://codereview.appspot.com/6548047/diff/11002/src/pkg/bufio/bufio.go#newcode378
src/pkg/bufio/bufio.go:378: // WriteTo implements io.WriterTo
Add a full stop.

https://codereview.appspot.com/6548047/diff/11002/src/pkg/bufio/bufio.go#newcode406
src/pkg/bufio/bufio.go:406: // writeBuf writes the Reader's buffer to
the writer
Add a full stop.

https://codereview.appspot.com/6548047/

[mch...@gmail.com]

[nige...@golang.org]

I'll make a couple of fixes and submit...


https://codereview.appspot.com/6548047/diff/16002/src/pkg/bufio/bufio_test.go
File src/pkg/bufio/bufio_test.go (right):

https://codereview.appspot.com/6548047/diff/16002/src/pkg/bufio/bufio_test.go#newcode769
src/pkg/bufio/bufio_test.go:769: input[i] = byte(len(input) % 256)
The RHS is always zero. Even after you fix that with s/len(input)/i/,
the input repeats itself every 256 bytes, which might mask a subtle bug
if the WriteTo buffers are a multiple of 256 bytes. I'd do something
like

for i := range input {

// 101 and 251 are arbitrary prime numbers.
// The idea is to create an input sequence
// which doesn't repeat itself too frequently.
input[i] = byte(i % 251)
if i%101 == 0 {
input[i] ^= byte(i/101)
}
}

https://codereview.appspot.com/6548047/diff/16002/src/pkg/bufio/bufio_test.go#newcode779
src/pkg/bufio/bufio_test.go:779: t.Errorf("after write: out[%#x] = %#x,

want %#x", i, val, input[i])

I'd change the first "%#x" to "%d".

https://codereview.appspot.com/6548047/

[nige...@golang.org]

*** Submitted as
http://code.google.com/p/go/source/detail?r=2c0e435aa879 ***

bufio: Implement io.WriterTo for (*Reader)

This is part 1 of 2 for issue 4028

benchmark old ns/op new ns/op delta
BenchmarkReaderCopyOptimal 33495 9849 -70.60%
BenchmarkReaderCopyUnoptimal 70631 27041 -61.72%
BenchmarkReaderCopyOldImpl 51407 52970 +3.04%

Update issue 4028

R=dave, nigeltao, rsc, bradfitz, rogpeppe
CC=golang-dev
http://codereview.appspot.com/6548047

Committer: Nigel Tao <nige...@golang.org>


http://codereview.appspot.com/6548047/