code review 11801043: bufio: check buffer availability before reading in ReadFrom

bufio: check buffer availability before reading in ReadFrom

Fixes issue 5947.

[adg, 2013-07-25 00:41:22]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-07-25 00:53:07]

LGTM

but this does change behavior mildly, probably in an unimportant way:

Before, a ReadFrom returning (bufferSize, EOF) would end up causing a
flush.  Now it won't until the next Write.  If we wanted, we could check
Available() == 0 in the err == io.EOF case to mirror that old behavior.



On Wed, Jul 24, 2013 at 5:41 PM, <adg@golang.org> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> bufio: check buffer availability before reading in ReadFrom
>
> Fixes issue 5947.
>
> Please review this at
https://codereview.appspot.**com/11801043/<https://codereview.appspot.com/118...
>
> 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
> @@ -678,17 +678,17 @@
>         }
>         var m int
>         for {
> +               if b.Available() == 0 {
> +                       if err1 := b.flush(); err1 != nil {
> +                               return n, err1
> +                       }
> +               }
>                 m, err = r.Read(b.buf[b.n:])
>                 if m == 0 {
>                         break
>                 }
>                 b.n += m
>                 n += int64(m)
> -               if b.Available() == 0 {
> -                       if err1 := b.flush(); err1 != nil {
> -                               return n, err1
> -                       }
> -               }
>                 if err != nil {
>                         break
>                 }
> Index: src/pkg/bufio/bufio_test.go
> ==============================**==============================**=======
> --- a/src/pkg/bufio/bufio_test.go
> +++ b/src/pkg/bufio/bufio_test.go
> @@ -847,6 +847,10 @@
>                                 t.Errorf("ws[%d],rs[%d]: w.ReadFrom(r) =
> %d, %v, want %d, nil", wi, ri, n, err, len(input))
>                                 continue
>                         }
> +                       if err := w.Flush(); err != nil {
> +                               t.Errorf("Flush returned %v", err)
> +                               continue
> +                       }
>                         if got, want := b.String(), string(input); got !=
> want {
>                                 t.Errorf("ws[%d], rs[%d]:\ngot  %q\nwant
> %q\n", wi, ri, got, want)
>                         }
> @@ -1003,6 +1007,23 @@
>         }
>  }
>
> +func TestReadFromWhileFull(t *testing.T) {
> +       buf := new(bytes.Buffer)
> +       w := NewWriterSize(buf, 10)
> +
> +       // Fill buffer exactly.
> +       n, err := w.Write([]byte("0123456789"))
> +       if n != 10 || err != nil {
> +               t.Fatalf("Write returned (%v, %v), want (10, nil)", n, err)
> +       }
> +
> +       // Use ReadFrom to read in some data.
> +       n2, err := w.ReadFrom(strings.NewReader("**abcdef"))
> +       if n2 != 6 || err != nil {
> +               t.Fatalf("ReadFrom returned (%v, %v), want (6, nil)", n,
> err)
> +       }
> +}
> +
>  // An onlyReader only implements io.Reader, no matter what other methods
> the underlying implementation may have.
>  type onlyReader struct {
>         r io.Reader
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[adg, 2013-07-25 00:58:44]

It's not behavior that should be relied upon (although our tests did), but
it's easy enough to preserve it.

PTAL


On 25 July 2013 10:53, Brad Fitzpatrick <bradfitz@golang.org> wrote:

> LGTM
>
> but this does change behavior mildly, probably in an unimportant way:
>
> Before, a ReadFrom returning (bufferSize, EOF) would end up causing a
> flush.  Now it won't until the next Write.  If we wanted, we could check
> Available() == 0 in the err == io.EOF case to mirror that old behavior.
>
>
>
> On Wed, Jul 24, 2013 at 5:41 PM, <adg@golang.org> wrote:
>
>> Reviewers: golang-dev1,
>>
>> Message:
>> Hello golang-dev@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go
>>
>>
>> Description:
>> bufio: check buffer availability before reading in ReadFrom
>>
>> Fixes issue 5947.
>>
>> Please review this at
https://codereview.appspot.**com/11801043/<https://codereview.appspot.com/118...
>>
>> 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
>> @@ -678,17 +678,17 @@
>>         }
>>         var m int
>>         for {
>> +               if b.Available() == 0 {
>> +                       if err1 := b.flush(); err1 != nil {
>> +                               return n, err1
>> +                       }
>> +               }
>>                 m, err = r.Read(b.buf[b.n:])
>>                 if m == 0 {
>>                         break
>>                 }
>>                 b.n += m
>>                 n += int64(m)
>> -               if b.Available() == 0 {
>> -                       if err1 := b.flush(); err1 != nil {
>> -                               return n, err1
>> -                       }
>> -               }
>>                 if err != nil {
>>                         break
>>                 }
>> Index: src/pkg/bufio/bufio_test.go
>> ==============================**==============================**=======
>> --- a/src/pkg/bufio/bufio_test.go
>> +++ b/src/pkg/bufio/bufio_test.go
>> @@ -847,6 +847,10 @@
>>                                 t.Errorf("ws[%d],rs[%d]: w.ReadFrom(r) =
>> %d, %v, want %d, nil", wi, ri, n, err, len(input))
>>                                 continue
>>                         }
>> +                       if err := w.Flush(); err != nil {
>> +                               t.Errorf("Flush returned %v", err)
>> +                               continue
>> +                       }
>>                         if got, want := b.String(), string(input); got !=
>> want {
>>                                 t.Errorf("ws[%d], rs[%d]:\ngot  %q\nwant
>> %q\n", wi, ri, got, want)
>>                         }
>> @@ -1003,6 +1007,23 @@
>>         }
>>  }
>>
>> +func TestReadFromWhileFull(t *testing.T) {
>> +       buf := new(bytes.Buffer)
>> +       w := NewWriterSize(buf, 10)
>> +
>> +       // Fill buffer exactly.
>> +       n, err := w.Write([]byte("0123456789"))
>> +       if n != 10 || err != nil {
>> +               t.Fatalf("Write returned (%v, %v), want (10, nil)", n,
>> err)
>> +       }
>> +
>> +       // Use ReadFrom to read in some data.
>> +       n2, err := w.ReadFrom(strings.NewReader("**abcdef"))
>> +       if n2 != 6 || err != nil {
>> +               t.Fatalf("ReadFrom returned (%v, %v), want (6, nil)", n,
>> err)
>> +       }
>> +}
>> +
>>  // An onlyReader only implements io.Reader, no matter what other methods
>> the underlying implementation may have.
>>  type onlyReader struct {
>>         r io.Reader
>>
>>
>>
>> --
>>
>> ---You received this message because you are subscribed to the Google
>> Groups "golang-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
>> .
>> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
>> .
>>
>>
>>
>

[bradfitz, 2013-07-25 01:27:30]

LGTM
 On Jul 24, 2013 5:58 PM, "Andrew Gerrand" <adg@golang.org> wrote:

> It's not behavior that should be relied upon (although our tests did), but
> it's easy enough to preserve it.
>
> PTAL
>
>
> On 25 July 2013 10:53, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
>> LGTM
>>
>> but this does change behavior mildly, probably in an unimportant way:
>>
>> Before, a ReadFrom returning (bufferSize, EOF) would end up causing a
>> flush.  Now it won't until the next Write.  If we wanted, we could check
>> Available() == 0 in the err == io.EOF case to mirror that old behavior.
>>
>>
>>
>> On Wed, Jul 24, 2013 at 5:41 PM, <adg@golang.org> wrote:
>>
>>> Reviewers: golang-dev1,
>>>
>>> Message:
>>> Hello golang-dev@googlegroups.com,
>>>
>>> I'd like you to review this change to
>>> https://code.google.com/p/go
>>>
>>>
>>> Description:
>>> bufio: check buffer availability before reading in ReadFrom
>>>
>>> Fixes issue 5947.
>>>
>>> Please review this at
https://codereview.appspot.**com/11801043/<https://codereview.appspot.com/118...
>>>
>>> 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
>>> @@ -678,17 +678,17 @@
>>>         }
>>>         var m int
>>>         for {
>>> +               if b.Available() == 0 {
>>> +                       if err1 := b.flush(); err1 != nil {
>>> +                               return n, err1
>>> +                       }
>>> +               }
>>>                 m, err = r.Read(b.buf[b.n:])
>>>                 if m == 0 {
>>>                         break
>>>                 }
>>>                 b.n += m
>>>                 n += int64(m)
>>> -               if b.Available() == 0 {
>>> -                       if err1 := b.flush(); err1 != nil {
>>> -                               return n, err1
>>> -                       }
>>> -               }
>>>                 if err != nil {
>>>                         break
>>>                 }
>>> Index: src/pkg/bufio/bufio_test.go
>>> ==============================**==============================**=======
>>> --- a/src/pkg/bufio/bufio_test.go
>>> +++ b/src/pkg/bufio/bufio_test.go
>>> @@ -847,6 +847,10 @@
>>>                                 t.Errorf("ws[%d],rs[%d]: w.ReadFrom(r) =
>>> %d, %v, want %d, nil", wi, ri, n, err, len(input))
>>>                                 continue
>>>                         }
>>> +                       if err := w.Flush(); err != nil {
>>> +                               t.Errorf("Flush returned %v", err)
>>> +                               continue
>>> +                       }
>>>                         if got, want := b.String(), string(input); got
>>> != want {
>>>                                 t.Errorf("ws[%d], rs[%d]:\ngot  %q\nwant
>>> %q\n", wi, ri, got, want)
>>>                         }
>>> @@ -1003,6 +1007,23 @@
>>>         }
>>>  }
>>>
>>> +func TestReadFromWhileFull(t *testing.T) {
>>> +       buf := new(bytes.Buffer)
>>> +       w := NewWriterSize(buf, 10)
>>> +
>>> +       // Fill buffer exactly.
>>> +       n, err := w.Write([]byte("0123456789"))
>>> +       if n != 10 || err != nil {
>>> +               t.Fatalf("Write returned (%v, %v), want (10, nil)", n,
>>> err)
>>> +       }
>>> +
>>> +       // Use ReadFrom to read in some data.
>>> +       n2, err := w.ReadFrom(strings.NewReader("**abcdef"))
>>> +       if n2 != 6 || err != nil {
>>> +               t.Fatalf("ReadFrom returned (%v, %v), want (6, nil)", n,
>>> err)
>>> +       }
>>> +}
>>> +
>>>  // An onlyReader only implements io.Reader, no matter what other
>>> methods the underlying implementation may have.
>>>  type onlyReader struct {
>>>         r io.Reader
>>>
>>>
>>>
>>> --
>>>
>>> ---You received this message because you are subscribed to the Google
>>> Groups "golang-dev" group.
>>> To unsubscribe from this group and stop receiving emails from it, send
>>> an email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
>>> .
>>> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
>>> .
>>>
>>>
>>>
>>
>

[adg, 2013-07-25 01:29:19]

*** Submitted as https://code.google.com/p/go/source/detail?r=3ffbc06b4874 ***

bufio: check buffer availability before reading in ReadFrom

Fixes issue 5947.

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/11801043

[dfc, 2013-07-25 01:38:42]

On 2013/07/25 01:29:19, adg wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=3ffbc06b4874 ***
> 
> bufio: check buffer availability before reading in ReadFrom
> 
> Fixes issue 5947.
> 
> R=golang-dev, bradfitz
> CC=golang-dev
> https://codereview.appspot.com/11801043

I am worried that this change may have caused an adverse performance effect

(~/go/src/pkg/bufio) % ~/go/misc/benchcmp {old,new}.txt
benchmark                        old ns/op    new ns/op    delta
BenchmarkReaderCopyOptimal            3642        15417  +323.31%
BenchmarkReaderCopyUnoptimal         24457        18296  -25.19%
BenchmarkReaderCopyNoWriteTo         43859        37217  -15.14%
BenchmarkWriterCopyOptimal           45784        38569  -15.76%
BenchmarkWriterCopyUnoptimal         24205        20826  -13.96%
BenchmarkWriterCopyNoReadFrom        19852        12327  -37.91%
BenchmarkReaderEmpty                 17656        17624   -0.18%
BenchmarkWriterEmpty                  1575         1765  +12.06%
BenchmarkWriterFlush                   277          295   +6.50%

benchmark                       old allocs   new allocs    delta
BenchmarkReaderEmpty                     3            3    0.00%
BenchmarkWriterEmpty                     1            1    0.00%
BenchmarkWriterFlush                     0            0     n/a%

benchmark                        old bytes    new bytes    delta
BenchmarkReaderEmpty                 16512        16512    0.00%
BenchmarkWriterEmpty                    82           82    0.00%
BenchmarkWriterFlush                     0            0     n/a%

My unscientific numbers fluctuate between -15% and +300%, can someone please
confirm.

[dfc, 2013-07-25 01:44:15]

darwin/amd64 is producing completely nonsensical results.

(~/go/src/pkg/bufio) % ./bufio.golden -test.run=X -test.bench=. > old.txt &&
./bufio.test -test.run=X -test.bench=. > new.txt && ~/go/misc/benchcmp
{old,new}.txt
benchmark                        old ns/op    new ns/op    delta
BenchmarkReaderCopyOptimal            3543        14426  +307.17%
BenchmarkReaderCopyUnoptimal         20412        17123  -16.11%
BenchmarkReaderCopyNoWriteTo         35081        34558   -1.49%
BenchmarkWriterCopyOptimal           39856        31322  -21.41%
BenchmarkWriterCopyUnoptimal         20448        16714  -18.26%
BenchmarkWriterCopyNoReadFrom        16490        27384  +66.06%
BenchmarkReaderEmpty                 13790        13671   -0.86%
BenchmarkWriterEmpty                  1291         1281   -0.77%
BenchmarkWriterFlush                   238          230   -3.36%

benchmark                       old allocs   new allocs    delta
BenchmarkReaderEmpty                     3            3    0.00%
BenchmarkWriterEmpty                     1            1    0.00%
BenchmarkWriterFlush                     0            0     n/a%

benchmark                        old bytes    new bytes    delta
BenchmarkReaderEmpty                 16512        16512    0.00%
BenchmarkWriterEmpty                    82           82    0.00%
BenchmarkWriterFlush                     0            0     n/a%
(~/go/src/pkg/bufio) % ./bufio.golden -test.run=X -test.bench=. > old.txt &&
./bufio.test -test.run=X -test.bench=. > new.txt && ~/go/misc/benchcmp
{old,new}.txt
benchmark                        old ns/op    new ns/op    delta
BenchmarkReaderCopyOptimal           14728         8748  -40.60%
BenchmarkReaderCopyUnoptimal         16935        16534   -2.37%
BenchmarkReaderCopyNoWriteTo         34625        34963   +0.98%
BenchmarkWriterCopyOptimal           31549        39385  +24.84%
BenchmarkWriterCopyUnoptimal         16122        16301   +1.11%
BenchmarkWriterCopyNoReadFrom        10047        10054   +0.07%
BenchmarkReaderEmpty                 13628        13819   +1.40%
BenchmarkWriterEmpty                  1278         1276   -0.16%
BenchmarkWriterFlush                   234          231   -1.28%

benchmark                       old allocs   new allocs    delta
BenchmarkReaderEmpty                     3            3    0.00%
BenchmarkWriterEmpty                     1            1    0.00%
BenchmarkWriterFlush                     0            0     n/a%

benchmark                        old bytes    new bytes    delta
BenchmarkReaderEmpty                 16512        16512    0.00%
BenchmarkWriterEmpty                    82           82    0.00%
BenchmarkWriterFlush                     0            0     n/a%
(~/go/src/pkg/bufio) % ./bufio.golden -test.run=X -test.bench=. > old.txt &&
./bufio.test -test.run=X -test.bench=. > new.txt && ~/go/misc/benchcmp
{old,new}.txt
benchmark                        old ns/op    new ns/op    delta
BenchmarkReaderCopyOptimal            3381         3391   +0.30%
BenchmarkReaderCopyUnoptimal         21342        20345   -4.67%
BenchmarkReaderCopyNoWriteTo         35173        35688   +1.46%
BenchmarkWriterCopyOptimal           37465        39083   +4.32%
BenchmarkWriterCopyUnoptimal         20015        20207   +0.96%
BenchmarkWriterCopyNoReadFrom        16825        16412   -2.45%
BenchmarkReaderEmpty                 13777        13845   +0.49%
BenchmarkWriterEmpty                  1311         1403   +7.02%
BenchmarkWriterFlush                   233          236   +1.29%

benchmark                       old allocs   new allocs    delta
BenchmarkReaderEmpty                     3            3    0.00%
BenchmarkWriterEmpty                     1            1    0.00%
BenchmarkWriterFlush                     0            0     n/a%

benchmark                        old bytes    new bytes    delta
BenchmarkReaderEmpty                 16512        16512    0.00%
BenchmarkWriterEmpty                    82           82    0.00%
BenchmarkWriterFlush                     0            0     n/a%

Can someone please confirm or deny if there is a regression.

[adg, 2013-07-25 01:47:16]

They're just really jittery benchmarks.

The wildest fluctuations you're seeing are in Reader benchmarks anyway, and
this change only affects the Writer.

[dfc, 2013-07-25 02:03:00]

ok, thanks for confirming, i'll stand down from DEFCON 1

On Thu, Jul 25, 2013 at 11:46 AM, Andrew Gerrand <adg@golang.org> wrote:
> They're just really jittery benchmarks.
>
> The wildest fluctuations you're seeing are in Reader benchmarks anyway, and
> this change only affects the Writer.