code review 6842078: bufio: Optimize (*Writer).ReadFrom when the buffer is n...

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

[chaten, 2012-11-21 01:42:54]

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

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

[dfc, 2012-11-21 01:47:54]

your additions to bufio_test do no fail against an unmodified bufio.go. Should
they >

[chaten, 2012-11-21 01:59:21]

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.

[dfc, 2012-11-21 02:01:09]

> 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, 2012-11-21 03:34:43]

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#newco...
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#newco...
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#newco...
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#newco...
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#newco...
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#...
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#...
src/pkg/bufio/bufio_test.go:873: //If we are returning an err this time, all
future writes should fail completely
space after //

[chaten, 2012-11-22 02:06:45]

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#newco...
> 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#newco...
> 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?

[chaten, 2012-11-22 02:40:56]

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?

[dfc, 2012-11-22 03:12:20]

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/

[chaten, 2012-11-22 03:32:39]

Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org (cc:
golang-dev@googlegroups.com, nigeltao@golang.org),

Please take another look.

[chaten, 2012-11-22 04:00:50]

Hello golang-dev@googlegroups.com, dave@cheney.net, bradfitz@golang.org (cc:
golang-dev@googlegroups.com, nigeltao@golang.org),

Please take another look.

[bradfitz, 2012-11-26 02:58:22]

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#newco...
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".

[bradfitz, 2013-06-04 22:15:08]

R=close

Please reply with a PTAL if there's activity on this again.