|
|
Created:
11 years, 6 months ago by chaten Modified:
11 years, 6 months ago Reviewers:
CC:
dfc, nigeltao, rsc, bradfitz, rog, golang-dev Visibility:
Public. |
Descriptionbufio: 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
Patch Set 1 #Patch Set 2 : diff -r 90c9121e26c3 https://code.google.com/p/go/ #Patch Set 3 : diff -r 90c9121e26c3 https://code.google.com/p/go/ #
Total comments: 22
Patch Set 4 : diff -r 84b4a0bb424a https://code.google.com/p/go/ #Patch Set 5 : diff -r 84b4a0bb424a https://code.google.com/p/go/ #
Total comments: 4
Patch Set 6 : diff -r 84b4a0bb424a https://code.google.com/p/go/ #
Total comments: 4
Patch Set 7 : diff -r bb1ad0db059a https://code.google.com/p/go/ #
Total comments: 2
MessagesTotal messages: 19
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
On 2012/09/21 08:51:30, chaten wrote: > Hello mailto:golang-dev@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%
Sign in to reply to this message.
Updates issue 4028. On Fri, Sep 21, 2012 at 6:52 PM, <mchaten@gmail.com> wrote: > On 2012/09/21 08:51:30, chaten wrote: >> >> Hello mailto:golang-dev@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/
Sign in to reply to this message.
On 21 September 2012 18:55, Dave Cheney <dave@cheney.net> wrote: > Updates issue 4028. Drop the s: "Update issue 4028". http://code.google.com/p/support/wiki/IssueTracker#Integration_with_version_c...
Sign in to reply to this message.
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#newc... 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#newc... 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#newc... 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#newc... 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#newc... 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#newc... 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.
Sign in to reply to this message.
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.
Sign in to reply to this message.
Sure.
Sign in to reply to this message.
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#newc... 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#newc... 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#newc... 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#newc... 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#newc... 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.
Sign in to reply to this message.
Hello dave@cheney.net, nigeltao@golang.org, rsc@golang.org, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
Hello dave@cheney.net, nigeltao@golang.org, rsc@golang.org, bradfitz@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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#newco... src/pkg/bufio/bufio.go:379: d http://codereview.appspot.com/6548047/diff/16001/src/pkg/bufio/bufio.go#newco... 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#newco... 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.
Sign in to reply to this message.
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#newco... 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.
Sign in to reply to this message.
Hello dave@cheney.net, nigeltao@golang.org, rsc@golang.org, bradfitz@golang.org, rogpeppe@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
looks fine to me. nigel or rog?
Sign in to reply to this message.
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... 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... 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.
Sign in to reply to this message.
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#newc... 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#newc... src/pkg/bufio/bufio.go:406: // writeBuf writes the Reader's buffer to the writer Add a full stop.
Sign in to reply to this message.
Hello dave@cheney.net, nigeltao@golang.org, rsc@golang.org, bradfitz@golang.org, rogpeppe@gmail.com (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
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... 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... 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".
Sign in to reply to this message.
*** 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 <nigeltao@golang.org>
Sign in to reply to this message.
|