FAQ
Reviewers: golang-dev_googlegroups.com,

Message:
Hello golang-dev@googlegroups.com (cc: nigeltao@golang.org),

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


Description:
bufio: Optimize (*Writer).ReadFrom when the buffer is not empty

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

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


Index: src/pkg/bufio/bufio.go
===================================================================
--- a/src/pkg/bufio/bufio.go
+++ b/src/pkg/bufio/bufio.go
@@ -569,32 +569,50 @@

// ReadFrom implements io.ReaderFrom.
func (b *Writer) ReadFrom(r io.Reader) (n int64, err error) {
- if b.Buffered() == 0 {
- if w, ok := b.wr.(io.ReaderFrom); ok {
- return w.ReadFrom(r)
+ //Want to call Write with the most amt of data possible, so
+ // if the buf has data in it, fill it then use ReadFrom instead
+ // of flushing immediately
+ if w, ok := b.wr.(io.ReaderFrom); ok {
+ if b.Buffered() != 0 {
+ //Have data, fill it up & then flush
+ m, err1 := io.ReadFull(r, b.buf[b.n:])
+ b.n += m
+ n += int64(m)
+ if err = b.Flush(); err1 != nil {
+ return
+ }
+ err = err1
+ if err != nil {
+ //Don't care that we didn't read the entire buf, so
+ // unexpected EOF should be treated the same as EOF
+ if err == io.EOF || err == io.ErrUnexpectedEOF {
+ err = nil
+ }
+ return
+ }
}
+ var m int64
+ m, err = w.ReadFrom(r)
+ return n + m, err
}
- var m int
for {
- m, err = r.Read(b.buf[b.n:])
- if m == 0 {
- break
- }
+ m, err1 := io.ReadFull(r, b.buf[b.n:])
b.n += m
n += int64(m)
if b.Available() == 0 {
- if err1 := b.Flush(); err1 != nil {
- return n, err1
+ if err = b.Flush(); err != nil {
+ return
}
}
+ err = err1
if err != nil {
break
}
}
- if err == io.EOF {
+ if err == io.EOF || err == io.ErrUnexpectedEOF {
err = nil
}
- return n, err
+ return
}

// buffered input and output
Index: src/pkg/bufio/bufio_test.go
===================================================================
--- a/src/pkg/bufio/bufio_test.go
+++ b/src/pkg/bufio/bufio_test.go
@@ -856,12 +856,24 @@
expected error
}

-func (r errorReaderFromTest) Read(p []byte) (int, error) {
- return len(p) * r.rn, r.rerr
+func (r *errorReaderFromTest) Read(p []byte) (n int, err error) {
+ n = len(p) * r.rn
+ err = r.rerr
+ if r.rerr != nil {
+ //If we are returning an err this time, all future reads should fail
completely
+ r.rn = 0
+ }
+ return
}

-func (w errorReaderFromTest) Write(p []byte) (int, error) {
- return len(p) * w.wn, w.werr
+func (w *errorReaderFromTest) Write(p []byte) (n int, err error) {
+ n = len(p) * w.wn
+ err = w.werr
+ if w.werr != nil {
+ //If we are returning an err this time, all future writes should fail
completely
+ w.wn = 0
+ }
+ return
}

var errorReaderFromTests = []errorReaderFromTest{
@@ -874,8 +886,8 @@

func TestWriterReadFromErrors(t *testing.T) {
for i, rw := range errorReaderFromTests {
- w := NewWriter(rw)
- if _, err := w.ReadFrom(rw); err != rw.expected {
+ w := NewWriter(&rw)
+ if _, err := w.ReadFrom(&rw); err != rw.expected {
t.Errorf("w.ReadFrom(errorReaderFromTests[%d]) = _, %v, want _,%v", i,
err, rw.expected)
}
}

Search Discussions

  • Dave at Nov 21, 2012 at 1:47 am
    your additions to bufio_test do no fail against an unmodified bufio.go.
    Should they >

    https://codereview.appspot.com/6842078/
  • Mchaten at Nov 21, 2012 at 1:59 am

    On 2012/11/21 01:47:54, dfc wrote:
    your additions to bufio_test do no fail against an unmodified
    bufio.go. Should
    they >
    They shouldn't fail on an unmodified version. The modifications make it
    so the test readers and writers behave properly after returning an err.
    Without those changes io.ReadAll never fails, causing an infinite loop.


    https://codereview.appspot.com/6842078/
  • Dave Cheney at Nov 21, 2012 at 2:01 am

    They shouldn't fail on an unmodified version. The modifications make it
    so the test readers and writers behave properly after returning an err.
    Without those changes io.ReadAll never fails, causing an infinite loop.
    Is it possible to add a test for that condition ?
  • Bradfitz at Nov 21, 2012 at 3:34 am
    https://codereview.appspot.com/6842078/diff/5001/src/pkg/bufio/bufio.go
    File src/pkg/bufio/bufio.go (right):

    https://codereview.appspot.com/6842078/diff/5001/src/pkg/bufio/bufio.go#newcode572
    src/pkg/bufio/bufio.go:572: //Want to call Write with the most amt of
    data possible, so
    Space before "Want", write "amount" instead of "amt", etc.

    But why is this comment up here? It should be after the b.Buffered() !=
    0 check, right?

    https://codereview.appspot.com/6842078/diff/5001/src/pkg/bufio/bufio.go#newcode577
    src/pkg/bufio/bufio.go:577: //Have data, fill it up & then flush
    space after //

    https://codereview.appspot.com/6842078/diff/5001/src/pkg/bufio/bufio.go#newcode581
    src/pkg/bufio/bufio.go:581: if err = b.Flush(); err1 != nil {
    this is misleading. if you really want this behavior, you'd put "err =
    b.Flush()" inside the if body, not in the initializer. the idiom is you
    check the variable you declared in the condition, but here you don't.
    you check err1 instead of err.

    https://codereview.appspot.com/6842078/diff/5001/src/pkg/bufio/bufio.go#newcode586
    src/pkg/bufio/bufio.go:586: //Don't care that we didn't read the entire
    buf, so
    space after //

    https://codereview.appspot.com/6842078/diff/5001/src/pkg/bufio/bufio.go#newcode612
    src/pkg/bufio/bufio.go:612: if err == io.EOF || err ==
    io.ErrUnexpectedEOF {
    why?

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

    https://codereview.appspot.com/6842078/diff/5001/src/pkg/bufio/bufio_test.go#newcode863
    src/pkg/bufio/bufio_test.go:863: //If we are returning an err this time,
    all future reads should fail completely
    space after //

    https://codereview.appspot.com/6842078/diff/5001/src/pkg/bufio/bufio_test.go#newcode873
    src/pkg/bufio/bufio_test.go:873: //If we are returning an err this time,
    all future writes should fail completely
    space after //

    https://codereview.appspot.com/6842078/
  • Mchaten at Nov 22, 2012 at 2:06 am

    On 2012/11/21 03:34:43, bradfitz wrote:
    But why is this comment up here? It should be after the b.Buffered() != 0
    check, right?
    Depends on how you look it. I view this comment as explaining the entire
    if block, not just the != 0 block, but I can easily see the opposite.


    https://codereview.appspot.com/6842078/diff/5001/src/pkg/bufio/bufio.go#newcode581
    src/pkg/bufio/bufio.go:581: if err = b.Flush(); err1 != nil {
    this is misleading. if you really want this behavior, you'd put "err =
    b.Flush()" inside the if body, not in the initializer. the idiom is you check
    the variable you declared in the condition, but here you don't. you
    check err1
    instead of err.
    This is a bug that the tests are not catching. I'm adding tests for this
    section now. Thank you for catching this.



    https://codereview.appspot.com/6842078/diff/5001/src/pkg/bufio/bufio.go#newcode612
    src/pkg/bufio/bufio.go:612: if err == io.EOF || err ==
    io.ErrUnexpectedEOF {
    why?
    I touch on this earlier in the code, should I repeat the comment?




    https://codereview.appspot.com/6842078/
  • Mchaten at Nov 22, 2012 at 2:40 am

    On 2012/11/21 02:01:09, dfc wrote:
    They shouldn't fail on an unmodified version. The modifications make
    it
    so the test readers and writers behave properly after returning an
    err.
    Without those changes io.ReadAll never fails, causing an infinite
    loop.
    Is it possible to add a test for that condition ?
    Can you be more specific on the exact condition to test?

    https://codereview.appspot.com/6842078/
  • Dave Cheney at Nov 22, 2012 at 3:12 am
    Can you add a test that demonstrates your change prevents the infinite loop in ReadAll ?
    On 22/11/2012, at 13:40, mchaten@gmail.com wrote:
    On 2012/11/21 02:01:09, dfc wrote:
    They shouldn't fail on an unmodified version. The modifications make
    it
    so the test readers and writers behave properly after returning an
    err.
    Without those changes io.ReadAll never fails, causing an infinite
    loop.
    Is it possible to add a test for that condition ?
    Can you be more specific on the exact condition to test?

    https://codereview.appspot.com/6842078/
  • Mchaten at Nov 22, 2012 at 3:32 am
    Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org
    (cc: golang-dev@googlegroups.com, nigeltao@golang.org),

    Please take another look.


    http://codereview.appspot.com/6842078/
  • Mchaten at Nov 22, 2012 at 4:00 am
    Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org
    (cc: golang-dev@googlegroups.com, nigeltao@golang.org),

    Please take another look.


    http://codereview.appspot.com/6842078/
  • Bradfitz at Nov 26, 2012 at 2:58 am
    For a CL about optimizing, you should probably include a BenchmarkFoo
    function.



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

    https://codereview.appspot.com/6842078/diff/7006/src/pkg/bufio/bufio.go#newcode572
    src/pkg/bufio/bufio.go:572: fillFlush := func() error {
    I'd make this be a (private) method on *Writer instead, even if it has
    to take an explicit (rather than closed-over) io.Reader argument.

    Then you can also document its function and return values more, and also
    test it stand-alone easier.

    Maybe call it "fillFrom".

    https://codereview.appspot.com/6842078/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedNov 21, '12 at 1:42a
activeNov 26, '12 at 2:58a
posts11
users3
websitegolang.org

3 users in discussion

Mchaten: 6 posts Dave Cheney: 3 posts Bradfitz: 2 posts

People

Translate

site design / logo © 2022 Grokbase