code review 94380043: io: copy slice argument in MultiReader and MultiWriter

io: copy slice argument in MultiReader and MultiWriter

Replaces CL 91240045.
Fixes issue 7809.

[rsc, 2014-05-13 02:25:33]

Hello golang-codereviews@googlegroups.com (cc: adg, bradfitz, iant, r),

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

[bradfitz, 2014-05-13 02:26:29]

LGTM

Sad for how rarely this will matter but sure.
 On May 12, 2014 7:25 PM, <rsc@golang.org> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com (cc: adg, bradfitz, iant, r),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> io: copy slice argument in MultiReader and MultiWriter
>
> Replaces CL 91240045.
> Fixes issue 7809.
>
> Please review this at https://codereview.appspot.com/94380043/
>
> Affected files (+33, -2 lines):
>   M src/pkg/io/multi.go
>   M src/pkg/io/multi_test.go
>
>
> Index: src/pkg/io/multi.go
> ===================================================================
> --- a/src/pkg/io/multi.go
> +++ b/src/pkg/io/multi.go
> @@ -29,7 +29,9 @@
>  // inputs have returned EOF, Read will return EOF.  If any of the readers
>  // return a non-nil, non-EOF error, Read will return that error.
>  func MultiReader(readers ...Reader) Reader {
> -       return &multiReader{readers}
> +       r := make([]Reader, len(readers))
> +       copy(r, readers)
> +       return &multiReader{r}
>  }
>
>  type multiWriter struct {
> @@ -53,5 +55,7 @@
>  // MultiWriter creates a writer that duplicates its writes to all the
>  // provided writers, similar to the Unix tee(1) command.
>  func MultiWriter(writers ...Writer) Writer {
> -       return &multiWriter{writers}
> +       w := make([]Writer, len(writers))
> +       copy(w, writers)
> +       return &multiWriter{w}
>  }
> Index: src/pkg/io/multi_test.go
> ===================================================================
> --- a/src/pkg/io/multi_test.go
> +++ b/src/pkg/io/multi_test.go
> @@ -9,6 +9,7 @@
>         "crypto/sha1"
>         "fmt"
>         . "io"
> +       "io/ioutil"
>         "strings"
>         "testing"
>  )
> @@ -86,3 +87,29 @@
>                 t.Errorf("expected %q; got %q", sourceString,
> sink.String())
>         }
>  }
> +
> +// Test that MultiReader copies the input slice and is insulated from
> future modification.
> +func TestMultiReaderCopy(t *testing.T) {
> +       slice := []Reader{strings.NewReader("hello world")}
> +       r := MultiReader(slice...)
> +       slice[0] = nil
> +       data, err := ioutil.ReadAll(r)
> +       if err != nil || string(data) != "hello world" {
> +               t.Errorf("ReadAll() = %q, %v, want %q, nil", data, err,
> "hello world")
> +       }
> +}
> +
> +// Test that MultiWriter copies the input slice and is insulated from
> future modification.
> +func TestMultiWriterCopy(t *testing.T) {
> +       var buf bytes.Buffer
> +       slice := []Writer{&buf}
> +       w := MultiWriter(slice...)
> +       slice[0] = nil
> +       n, err := w.Write([]byte("hello world"))
> +       if err != nil || n != 11 {
> +               t.Errorf("Write(`hello world`) = %d, %v, want 11, nil", n,
> err)
> +       }
> +       if buf.String() != "hello world" {
> +               t.Errorf("buf.String() = %q, want %q", buf.String(),
> "hello world")
> +       }
> +}
>
>
>

[rsc, 2014-05-13 02:51:26]

On Mon, May 12, 2014 at 10:26 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> LGTM
>
> Sad for how rarely this will matter but sure.
>
Why sad? The only added cost here is on the MultiReader(x...) or
MultiWriter(x...) calls. The calls like MultiReader(r1, r2, r3) are
unaffected. (Before, the allocation of the slice at the call site; now the
call site passes a pointer to a stack array and the heap allocation is in
the function, but there is still just one allocation.)

Russ

[adg, 2014-05-13 02:56:44]

On 13 May 2014 12:51, Russ Cox <rsc@golang.org> wrote:

> Why sad? The only added cost here is on the MultiReader(x...) or
> MultiWriter(x...) calls. The calls like MultiReader(r1, r2, r3) are
> unaffected. (Before, the allocation of the slice at the call site; now the
> call site passes a pointer to a stack array and the heap allocation is in
> the function, but there is still just one allocation.)
>

Theoretically: we could avoid the slice allocation entirely in the common
case by putting a fixed size array in the struct and using that if it's big
enough.

Doesn't seem worth it though. I don't know of a any programs that
bottleneck on collecting multiwriters.

[minux1, 2014-05-13 02:57:50]

https://codereview.appspot.com/94380043/diff/40001/src/pkg/io/multi.go
File src/pkg/io/multi.go (right):

https://codereview.appspot.com/94380043/diff/40001/src/pkg/io/multi.go#newcode34
src/pkg/io/multi.go:34: return &multiReader{r}
i have a style question, i normally would use something like this:
return &multiReader{append([]Reader(nil), readers...)}

so I'd like to know why do you think the explicit allocate and copy is better?
readability?

[bradfitz, 2014-05-13 03:13:08]

On Mon, May 12, 2014 at 7:51 PM, Russ Cox <rsc@golang.org> wrote:

> On Mon, May 12, 2014 at 10:26 PM, Brad Fitzpatrick
<bradfitz@golang.org>wrote:
>
>> LGTM
>>
>> Sad for how rarely this will matter but sure.
>>
> Why sad? The only added cost here is on the MultiReader(x...) or
> MultiWriter(x...) calls. The calls like MultiReader(r1, r2, r3) are
> unaffected. (Before, the allocation of the slice at the call site; now the
> call site passes a pointer to a stack array and the heap allocation is in
> the function, but there is still just one allocation.)
>

Oh, I never realized the escape analysis modified the vararg call site's
code.

I just did some little go tool 6g -S/-m tests to verify.

But now this confuses me... why does this print 0?

package main

import (
        "fmt"
        "testing"
)

var esc []int

func v(x ...int) {
        esc = x
}

func main() {
        n := testing.AllocsPerRun(100, func() {
                a, b, c := 100, 101, 102
                v(a, b, c)
        })
        fmt.Printf("allocs = %v\n", n)
}

Shouldn't the slice made in the func literal escape? -m suggests it does,
but I'd expect the func literal to allocate the slice's backing array.

Incidentally, I'm not a compiler expert, but this looks non-ideal:

        0x0019 00025 (x.go:16)  MOVQ    $100,DI
        0x0020 00032 (x.go:16)  MOVQ    $101,SI
        0x0027 00039 (x.go:16)  MOVQ    $102,DX
        0x002e 00046 (x.go:17)  MOVQ    $0,"".autotmp_0009+56(SP)
        0x0037 00055 (x.go:17)  MOVQ    $0,"".autotmp_0009+64(SP)
        0x0040 00064 (x.go:17)  MOVQ    $0,"".autotmp_0009+72(SP)
        0x0049 00073 (x.go:17)  LEAQ    "".autotmp_0009+56(SP),BX
        0x004e 00078 (x.go:17)  MOVQ    BX,"".autotmp_0007+80(SP)
        0x0053 00083 (x.go:17)  MOVQ    "".autotmp_0007+80(SP),BX
        0x0058 00088 (x.go:17)  CMPQ    BX,$0

Lot of storing and loading and comparing zero.

Are there bugs for stuff like this?

[iant, 2014-05-13 03:25:08]

On Mon, May 12, 2014 at 8:13 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
> But now this confuses me... why does this print 0?

Good question.  For me, it prints 1.

Ian

[bradfitz, 2014-05-13 03:35:19]

On Mon, May 12, 2014 at 8:25 PM, Ian Lance Taylor <iant@golang.org> wrote:

> On Mon, May 12, 2014 at 8:13 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> >
> > But now this confuses me... why does this print 0?
>
> Good question.  For me, it prints 1.
>

Oh, it does for me too after a sync:

mac:~ bradfitz$ go version
go version devel +a9d4ac7d62b9 Thu May 01 09:37:55 2014 -0400 + darwin/amd64

mac:~ bradfitz$ go run x.go
allocs = 0

mac:~ bradfitz$ go version
go version devel +f0e34845f8ac Mon May 12 20:26:27 2014 -0700 darwin/amd64

mac:~ bradfitz$ go run x.go
allocs = 1

It wasn't the recent escape analysis bug fix, because before that point
(5283a606b755) is also printing 1.

Oh, it was https://code.google.com/p/go/source/detail?r=d6c84b68396

[rsc, 2014-05-13 03:37:33]

https://codereview.appspot.com/94380043/diff/40001/src/pkg/io/multi.go
File src/pkg/io/multi.go (right):

https://codereview.appspot.com/94380043/diff/40001/src/pkg/io/multi.go#newcode34
src/pkg/io/multi.go:34: return &multiReader{r}
On 2014/05/13 02:57:50, minux wrote:
> i have a style question, i normally would use something like this:
> return &multiReader{append([]Reader(nil), readers...)}
> 
> so I'd like to know why do you think the explicit allocate and copy is better?
> readability?

yes. one is readable and one is not. :-)

[rsc, 2014-05-13 03:38:36]

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

io: copy slice argument in MultiReader and MultiWriter

Replaces CL 91240045.
Fixes issue 7809.

LGTM=bradfitz
R=golang-codereviews, minux.ma
CC=adg, bradfitz, golang-codereviews, iant, r
https://codereview.appspot.com/94380043

[gobot, 2014-05-13 04:47:27]

This CL appears to have broken the openbsd-amd64-rootbsd builder.
See http://build.golang.org/log/acca24cd79aaa7c48ddecf451830772c007c3801