Add LazyMultiError.

Add LazyMultiError.

This is essentially a MultiError, except that it knows how big it should get
if it's ever assigned an actual error, and it only does its allocation lazily.
This is useful for implementing e.g. PutMulti and other Multi-style methods.

R=dnj@chromium.org, estaab@chromium.org, martiniss@chromium.org, vadimsh@chromium.org
BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/562466310abab467e6f2a9c597902847e43fade2

[iannucci, 2015-07-17 01:07:14]

PTAL, this one is short

[Vadim Sh., 2015-07-17 01:09:03]

https://codereview.chromium.org/1234493005/diff/1/go/src/infra/gae/libs/gae/u...
File go/src/infra/gae/libs/gae/upstream_errors.go (right):

https://codereview.chromium.org/1234493005/diff/1/go/src/infra/gae/libs/gae/u...
go/src/infra/gae/libs/gae/upstream_errors.go:131: // the allocated MultiError,
or nil if no error was encountered.
mention that it's not thread safe or make it thread safe. I'd be tempted to use
it as

me := LazyMultiError{}
for i := .... {
 go func(indx int) {
   err := ...
   me.Assign(i, err) 
}(i)
}

and will be sad to realize it's not safe.

[iannucci, 2015-07-17 01:16:54]

https://chromiumcodereview.appspot.com/1234493005/diff/1/go/src/infra/gae/lib...
File go/src/infra/gae/libs/gae/upstream_errors.go (right):

https://chromiumcodereview.appspot.com/1234493005/diff/1/go/src/infra/gae/lib...
go/src/infra/gae/libs/gae/upstream_errors.go:131: // the allocated MultiError,
or nil if no error was encountered.
Good catch, done :)

[Vadim Sh., 2015-07-17 01:26:28]

https://codereview.chromium.org/1234493005/diff/20001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/upstream_errors.go (right):

https://codereview.chromium.org/1234493005/diff/20001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/upstream_errors.go:150: e.me[i] = err
strictly speaking this needs memory barrier (e.g. lock) to be safely read from
another core. I don't know what go memory model is though, maybe it's fine.

[iannucci, 2015-07-17 01:30:05]

On 2015/07/17 01:26:28, Vadim Sh. wrote:
>
https://codereview.chromium.org/1234493005/diff/20001/go/src/infra/gae/libs/g...
> File go/src/infra/gae/libs/gae/upstream_errors.go (right):
> 
>
https://codereview.chromium.org/1234493005/diff/20001/go/src/infra/gae/libs/g...
> go/src/infra/gae/libs/gae/upstream_errors.go:150: e.me[i] = err
> strictly speaking this needs memory barrier (e.g. lock) to be safely read from
> another core. I don't know what go memory model is though, maybe it's fine.

I'm pretty sure the once is the barrier here (http://golang.org/pkg/sync/#Once):
   "no call to Do returns until the one call to f returns"

Which means that by line 150, e.me will always be available. Or are you meaning
that there can be cacheline conflicts between adjacent members in the slice or
something like that? I'll do some more research.

[Vadim Sh., 2015-07-17 01:32:02]

On 2015/07/17 01:30:05, iannucci wrote:
> On 2015/07/17 01:26:28, Vadim Sh. wrote:
> >
>
https://codereview.chromium.org/1234493005/diff/20001/go/src/infra/gae/libs/g...
> > File go/src/infra/gae/libs/gae/upstream_errors.go (right):
> > 
> >
>
https://codereview.chromium.org/1234493005/diff/20001/go/src/infra/gae/libs/g...
> > go/src/infra/gae/libs/gae/upstream_errors.go:150: e.me[i] = err
> > strictly speaking this needs memory barrier (e.g. lock) to be safely read
from
> > another core. I don't know what go memory model is though, maybe it's fine.
> 
> I'm pretty sure the once is the barrier here
(http://golang.org/pkg/sync/#Once):
>    "no call to Do returns until the one call to f returns"
> 
> Which means that by line 150, e.me will always be available. Or are you
meaning
> that there can be cacheline conflicts between adjacent members in the slice or
> something like that? I'll do some more research.

I mean write to e.me[i] may stay "invisible" to other cores.

[Vadim Sh., 2015-07-17 01:34:33]

Just use lock around the whole struct, it's the surest way :) Fancier sync is
easy to screw up in a bad way (e.g. it will fail once a month in production
driving you nuts).

Also, write concurrent tests (similar to for loop I've suggested) and run it
with go test -race. I think it should detect this as a race.

[iannucci, 2015-07-17 01:57:30]

On 2015/07/17 01:34:33, Vadim Sh. wrote:
> Just use lock around the whole struct, it's the surest way :) Fancier sync is
> easy to screw up in a bad way (e.g. it will fail once a month in production
> driving you nuts).
> 
> Also, write concurrent tests (similar to for loop I've suggested) and run it
> with go test -race. I think it should detect this as a race.

I actually don't think so. The Once /will/ 'happen before' line 150 according to
the memory model. e.me will be there (Once is basically an int and a Mutex).

I also think that parallel manipulation of e.me is safe, too, as long as the
user of LazyMulitError synchronizes the goroutines writing to it (which... it's
their fault if they don't).

I think it's fine :)

[iannucci, 2015-07-17 01:57:45]

On 2015/07/17 01:57:30, iannucci wrote:
> On 2015/07/17 01:34:33, Vadim Sh. wrote:
> > Just use lock around the whole struct, it's the surest way :) Fancier sync
is
> > easy to screw up in a bad way (e.g. it will fail once a month in production
> > driving you nuts).
> > 
> > Also, write concurrent tests (similar to for loop I've suggested) and run it
> > with go test -race. I think it should detect this as a race.
> 
> I actually don't think so. The Once /will/ 'happen before' line 150 according
to
> the memory model. e.me will be there (Once is basically an int and a Mutex).
> 
> I also think that parallel manipulation of e.me is safe, too, as long as the
> user of LazyMulitError synchronizes the goroutines writing to it (which...
it's
> their fault if they don't).
> 
> I think it's fine :)

(I'll add a parallel benchmark and run it through race)

[Vadim Sh., 2015-07-17 02:03:11]

On 2015/07/17 01:57:45, iannucci wrote:
> On 2015/07/17 01:57:30, iannucci wrote:
> > On 2015/07/17 01:34:33, Vadim Sh. wrote:
> > > Just use lock around the whole struct, it's the surest way :) Fancier sync
> is
> > > easy to screw up in a bad way (e.g. it will fail once a month in
production
> > > driving you nuts).
> > > 
> > > Also, write concurrent tests (similar to for loop I've suggested) and run
it
> > > with go test -race. I think it should detect this as a race.
> > 
> > I actually don't think so. The Once /will/ 'happen before' line 150
according
> to
> > the memory model. e.me will be there (Once is basically an int and a Mutex).
> > 
> > I also think that parallel manipulation of e.me is safe, too, as long as the
> > user of LazyMulitError synchronizes the goroutines writing to it (which...
> it's
> > their fault if they don't).
> > 
> > I think it's fine :)
> 
> (I'll add a parallel benchmark and run it through race)

e.me is there, yes. I'm talking about &e.me[i] memory location:

core 1: writes 'abc' to e.me[0] (via e.Assign(0, 'abc')).
core 2: reads e.me[0] (via e.Get()).

Without some sort of a barrier, core 2 may read stale data in e.me[0].

In practice there are lots of "accidental" barriers though (e.g. waiting on a
wait group). Dunno. I'm not an expert on this stuff. When in doubt, and code is
not performance critical, I just use locks and sleep well at night.

[iannucci, 2015-07-17 02:05:21]

On 2015/07/17 01:57:45, iannucci wrote:
> On 2015/07/17 01:57:30, iannucci wrote:
> > On 2015/07/17 01:34:33, Vadim Sh. wrote:
> > > Just use lock around the whole struct, it's the surest way :) Fancier sync
> is
> > > easy to screw up in a bad way (e.g. it will fail once a month in
production
> > > driving you nuts).
> > > 
> > > Also, write concurrent tests (similar to for loop I've suggested) and run
it
> > > with go test -race. I think it should detect this as a race.
> > 
> > I actually don't think so. The Once /will/ 'happen before' line 150
according
> to
> > the memory model. e.me will be there (Once is basically an int and a Mutex).
> > 
> > I also think that parallel manipulation of e.me is safe, too, as long as the
> > user of LazyMulitError synchronizes the goroutines writing to it (which...
> it's
> > their fault if they don't).
> > 
> > I think it's fine :)
> 
> (I'll add a parallel benchmark and run it through race)

Added, PTAL

[Vadim Sh., 2015-07-17 02:06:14]

lgtm

[iannucci, 2015-07-17 02:10:03]

Finnneeee....

[iannucci, 2015-07-17 02:14:30]

On 2015/07/17 02:10:03, iannucci wrote:
> Finnneeee....

Ok, it'a mutex now.

[Vadim Sh., 2015-07-17 02:16:46]

https://codereview.chromium.org/1234493005/diff/60001/go/src/infra/gae/libs/g...
File go/src/infra/gae/libs/gae/upstream_errors.go (right):

https://codereview.chromium.org/1234493005/diff/60001/go/src/infra/gae/libs/g...
go/src/infra/gae/libs/gae/upstream_errors.go:157: if e.me == nil {
it's needed here too :)

[iannucci, 2015-07-17 02:23:48]

https://chromiumcodereview.appspot.com/1234493005/diff/60001/go/src/infra/gae...
File go/src/infra/gae/libs/gae/upstream_errors.go (right):

https://chromiumcodereview.appspot.com/1234493005/diff/60001/go/src/infra/gae...
go/src/infra/gae/libs/gae/upstream_errors.go:157: if e.me == nil {
On 2015/07/17 02:16:46, Vadim Sh. wrote:
> it's needed here too :)

Done.

[Vadim Sh., 2015-07-17 02:35:17]

lgtm

[iannucci, 2015-07-17 02:36:08]

The CQ bit was checked by iannucci@chromium.org

[commit-bot: I haz the power, 2015-07-17 02:36:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1234493005/80001

[commit-bot: I haz the power, 2015-07-17 02:39:16]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/infra/infra/+/562466310abab467e6f2a9c597902...