code review 56970043: runtime/race: fix finalizer tests

runtime/race: fix finalizer tests
After "runtime: combine small NoScan allocations" finalizers
for small objects run more non deterministically.
TestRaceFin episodically fails on my darwin/amd64.

[dvyukov, 2014-01-25 12:23:39]

Hello golang-codereviews@googlegroups.com,

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

[khr, 2014-01-29 16:35:01]

On 2014/01/25 12:23:39, dvyukov wrote:
> Hello mailto:golang-codereviews@googlegroups.com,
> 
> I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/

Is it nondeterministic just because a live object might share a chunk with a
finalizable object?

[dvyukov, 2014-01-29 20:54:06]

Yes

sent from phone
On 2014/01/25 12:23:39, dvyukov wrote:

> Hello mailto:golang-codereviews@googlegroups.com,
>

 I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/
>

Is it nondeterministic just because a live object might share a chunk
with a finalizable object?


https://codereview.appspot.com/56970043/

[khr, 2014-02-11 22:40:47]

On 2014/01/29 20:54:06, dvyukov wrote:
> Yes
> 
> sent from phone
> On 2014/01/25 12:23:39, dvyukov wrote:
> 
> > Hello mailto:golang-codereviews@googlegroups.com,
> >
> 
>  I'd like you to review this change to
> > https://dvyukov%40google.com@code.google.com/p/go/
> >
> 
> Is it nondeterministic just because a live object might share a chunk
> with a finalizable object?
> 
> 
> https://codereview.appspot.com/56970043/

LGTM.

I'm ok with this change for now, but the general idea makes me nervous.  If
someone sets a finalizer, we should endeavor to run it, not just ignore it
because it shares a bucket with some other object.  For users, this amounts to
mysterious non-finalizing for no good reason.  Maybe we should just disallow
setting finalizers on small (< 1 word) objects.  We already disallow finalizers
for 0-size objects.

[dave_cheney.net, 2014-02-11 22:47:16]

> Maybe we should just disallow
> setting finalizers on small (< 1 word) objects.  We already disallow
finalizers
> for 0-size objects.

How would that rule work ?

type f uint32 

func (*f) Done() [ ... } // can be a finaliser on i386 and arm, but not on amd64

[khr, 2014-02-11 23:01:27]

On 2014/02/11 22:47:16, dfc wrote:
> > Maybe we should just disallow
> > setting finalizers on small (< 1 word) objects.  We already disallow
> finalizers
> > for 0-size objects.
> 
> How would that rule work ?
> 
> type f uint32 
> 
> func (*f) Done() [ ... } // can be a finaliser on i386 and arm, but not on
amd64

Yep, that does seem yucky.  Maybe a finalizable type must be or contain a
pointer, intptr, uintptr, unsafe.Pointer, or reference type in it?  Ugly also,
I'll admit.

Or we could revert Dmitry's change that combines small allocations.  Note that
we could leave it in for the special case of the backing arrays for strings,
which may capture a large fraction of the improvement it got.

[dvyukov, 2014-02-12 13:55:20]

On 2014/02/11 22:40:47, khr wrote:
> On 2014/01/29 20:54:06, dvyukov wrote:
> > Yes
> > 
> > sent from phone
> > On 2014/01/25 12:23:39, dvyukov wrote:
> > 
> > > Hello mailto:golang-codereviews@googlegroups.com,
> > >
> > 
> >  I'd like you to review this change to
> > > https://dvyukov%40google.com@code.google.com/p/go/
> > >
> > 
> > Is it nondeterministic just because a live object might share a chunk
> > with a finalizable object?
> > 
> > 
> > https://codereview.appspot.com/56970043/
> 
> LGTM.
> 
> I'm ok with this change for now, but the general idea makes me nervous.

I understand what you are talking about.


>   If
> someone sets a finalizer, we should endeavor to run it, not just ignore it
> because it shares a bucket with some other object.  For users, this amounts to
> mysterious non-finalizing for no good reason.  Maybe we should just disallow
> setting finalizers on small (< 1 word) objects.  We already disallow
finalizers
> for 0-size objects.


Don't disallow finalizers for 0-size objects, we silently ignore them.
We can do the same for tiny objects. I.e. instead of unreliably running them, do
not run them reliably; and document the minimum size of the object.

One excuse is that to date any particular finalizer is still not guaranteed to
run because of imprecise GC.

[dvyukov, 2014-02-12 13:58:03]

On 2014/02/11 23:01:27, khr wrote:
> On 2014/02/11 22:47:16, dfc wrote:
> > > Maybe we should just disallow
> > > setting finalizers on small (< 1 word) objects.  We already disallow
> > finalizers
> > > for 0-size objects.
> > 
> > How would that rule work ?
> > 
> > type f uint32 
> > 
> > func (*f) Done() [ ... } // can be a finaliser on i386 and arm, but not on
> amd64
> 
> Yep, that does seem yucky.  Maybe a finalizable type must be or contain a
> pointer, intptr, uintptr, unsafe.Pointer, or reference type in it?  Ugly also,
> I'll admit.
> 
> Or we could revert Dmitry's change that combines small allocations.  Note that
> we could leave it in for the special case of the backing arrays for strings,
> which may capture a large fraction of the improvement it got.


I don't have data, but I expect that significant number of tiny allocations come
from escaping/captured local vars. On this level you don't have control over
layout (except combining groups of local vars into structs, which does not sound
like a good approach).

[dvyukov, 2014-02-24 14:15:57]

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

runtime/race: fix finalizer tests
After "runtime: combine small NoScan allocations" finalizers
for small objects run more non deterministically.
TestRaceFin episodically fails on my darwin/amd64.

LGTM=khr
R=golang-codereviews, khr, dave
CC=golang-codereviews
https://codereview.appspot.com/56970043

[gobot, 2014-02-24 14:37:00]

This CL appears to have broken the plan9-386-cnielsen builder.