code review 69220046: bufio: in Reader.WriteTo, try to use target's ReaderFrom

bufio: in Reader.WriteTo, try to use target's ReaderFrom

This is the simple half of https://codereview.appspot.com/53560043/ with
a new benchmark. pongad is in the C+A files already.

benchmark                         old ns/op     new ns/op     delta
BenchmarkReaderWriteToOptimal     2054          825           -59.83%

Update Issue 6373

[bradfitz, 2014-02-27 07:53:18]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2014-02-27 07:55:53]

R=iant,gri,rsc?  Who likes bufio?



On Wed, Feb 26, 2014 at 11:53 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> bufio: in Reader.WriteTo, try to use target's ReaderFrom
>
> This is the simple half of https://codereview.appspot.com/53560043/ with
> a new benchmark. pongad is in the C+A files already.
>
> benchmark                         old ns/op     new ns/op     delta
> BenchmarkReaderWriteToOptimal     2054          825           -59.83%
>
> Update Issue 6373
>
> Please review this at https://codereview.appspot.com/69220046/
>
> Affected files (+29, -10 lines):
>   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
> @@ -410,6 +410,12 @@
>                 return n, err
>         }
>
> +       if w, ok := w.(io.ReaderFrom); ok {
> +               m, err := w.ReadFrom(b.rd)
> +               n += m
> +               return n, err
> +       }
> +
>         for b.fill(); b.r < b.w; b.fill() {
>                 m, err := b.writeBuf(w)
>                 n += m
> Index: src/pkg/bufio/bufio_test.go
> ===================================================================
> --- a/src/pkg/bufio/bufio_test.go
> +++ b/src/pkg/bufio/bufio_test.go
> @@ -1094,20 +1094,12 @@
>
>  // An onlyReader only implements io.Reader, no matter what other methods
> the underlying implementation may have.
>  type onlyReader struct {
> -       r io.Reader
> -}
> -
> -func (r onlyReader) Read(b []byte) (int, error) {
> -       return r.r.Read(b)
> +       io.Reader
>  }
>
>  // An onlyWriter only implements io.Writer, no matter what other methods
> the underlying implementation may have.
>  type onlyWriter struct {
> -       w io.Writer
> -}
> -
> -func (w onlyWriter) Write(b []byte) (int, error) {
> -       return w.w.Write(b)
> +       io.Writer
>  }
>
>  func BenchmarkReaderCopyOptimal(b *testing.B) {
> @@ -1152,6 +1144,27 @@
>         }
>  }
>
> +func BenchmarkReaderWriteToOptimal(b *testing.B) {
> +       const bufSize = 16 << 10
> +       buf := make([]byte, bufSize)
> +       r := bytes.NewReader(buf)
> +       srcReader := NewReaderSize(onlyReader{r}, 1<<10)
> +       if _, ok := ioutil.Discard.(io.ReaderFrom); !ok {
> +               b.Fatalf("Discard doesn't support ReaderFrom")
> +       }
> +       for i := 0; i < b.N; i++ {
> +               r.Seek(0, 0)
> +               srcReader.Reset(onlyReader{r})
> +               n, err := srcReader.WriteTo(ioutil.Discard)
> +               if err != nil {
> +                       b.Fatal(err)
> +               }
> +               if n != bufSize {
> +                       b.Fatalf("n = %d; want %d", n, bufSize)
> +               }
> +       }
> +}
> +
>  func BenchmarkWriterCopyOptimal(b *testing.B) {
>         // Optimal case is where the underlying writer implements
> io.ReaderFrom
>         srcBuf := bytes.NewBuffer(make([]byte, 8192))
>
>
>

[iant, 2014-02-27 17:50:14]

LGTM

[gri, 2014-02-27 17:50:37]

I'll take it.
- gri


On Wed, Feb 26, 2014 at 11:55 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> R=iant,gri,rsc?  Who likes bufio?
>
>
>
> On Wed, Feb 26, 2014 at 11:53 PM, <bradfitz@golang.org> wrote:
>
>> Reviewers: golang-codereviews,
>>
>> Message:
>> Hello golang-codereviews@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>>
>> Description:
>> bufio: in Reader.WriteTo, try to use target's ReaderFrom
>>
>> This is the simple half of https://codereview.appspot.com/53560043/ with
>> a new benchmark. pongad is in the C+A files already.
>>
>> benchmark                         old ns/op     new ns/op     delta
>> BenchmarkReaderWriteToOptimal     2054          825           -59.83%
>>
>> Update Issue 6373
>>
>> Please review this at https://codereview.appspot.com/69220046/
>>
>> Affected files (+29, -10 lines):
>>   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
>> @@ -410,6 +410,12 @@
>>                 return n, err
>>         }
>>
>> +       if w, ok := w.(io.ReaderFrom); ok {
>> +               m, err := w.ReadFrom(b.rd)
>> +               n += m
>> +               return n, err
>> +       }
>> +
>>         for b.fill(); b.r < b.w; b.fill() {
>>                 m, err := b.writeBuf(w)
>>                 n += m
>> Index: src/pkg/bufio/bufio_test.go
>> ===================================================================
>> --- a/src/pkg/bufio/bufio_test.go
>> +++ b/src/pkg/bufio/bufio_test.go
>> @@ -1094,20 +1094,12 @@
>>
>>  // An onlyReader only implements io.Reader, no matter what other methods
>> the underlying implementation may have.
>>  type onlyReader struct {
>> -       r io.Reader
>> -}
>> -
>> -func (r onlyReader) Read(b []byte) (int, error) {
>> -       return r.r.Read(b)
>> +       io.Reader
>>  }
>>
>>  // An onlyWriter only implements io.Writer, no matter what other methods
>> the underlying implementation may have.
>>  type onlyWriter struct {
>> -       w io.Writer
>> -}
>> -
>> -func (w onlyWriter) Write(b []byte) (int, error) {
>> -       return w.w.Write(b)
>> +       io.Writer
>>  }
>>
>>  func BenchmarkReaderCopyOptimal(b *testing.B) {
>> @@ -1152,6 +1144,27 @@
>>         }
>>  }
>>
>> +func BenchmarkReaderWriteToOptimal(b *testing.B) {
>> +       const bufSize = 16 << 10
>> +       buf := make([]byte, bufSize)
>> +       r := bytes.NewReader(buf)
>> +       srcReader := NewReaderSize(onlyReader{r}, 1<<10)
>> +       if _, ok := ioutil.Discard.(io.ReaderFrom); !ok {
>> +               b.Fatalf("Discard doesn't support ReaderFrom")
>> +       }
>> +       for i := 0; i < b.N; i++ {
>> +               r.Seek(0, 0)
>> +               srcReader.Reset(onlyReader{r})
>> +               n, err := srcReader.WriteTo(ioutil.Discard)
>> +               if err != nil {
>> +                       b.Fatal(err)
>> +               }
>> +               if n != bufSize {
>> +                       b.Fatalf("n = %d; want %d", n, bufSize)
>> +               }
>> +       }
>> +}
>> +
>>  func BenchmarkWriterCopyOptimal(b *testing.B) {
>>         // Optimal case is where the underlying writer implements
>> io.ReaderFrom
>>         srcBuf := bytes.NewBuffer(make([]byte, 8192))
>>
>>
>>
>

[gri, 2014-02-27 18:32:51]

LGTM

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

https://codereview.appspot.com/69220046/diff/40001/src/pkg/bufio/bufio_test.g...
src/pkg/bufio/bufio_test.go:1153: b.Fatalf("Discard doesn't support ReaderFrom")
this should probably be

ioutil.Discard doesn't support io.ReaderFrom

but it's fine as is (can be cleaned up in another CL)

[bradfitz, 2014-02-27 18:48:40]

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

bufio: in Reader.WriteTo, try to use target's ReaderFrom

This is the simple half of https://codereview.appspot.com/53560043/ with
a new benchmark. pongad is in the C+A files already.

benchmark                         old ns/op     new ns/op     delta
BenchmarkReaderWriteToOptimal     2054          825           -59.83%

Update Issue 6373

LGTM=iant, gri
R=golang-codereviews, iant, gri
CC=golang-codereviews
https://codereview.appspot.com/69220046

[gobot, 2014-02-27 18:55:01]

This CL appears to have broken the solaris-amd64-solaris11 builder.