Oilpan: disable conservative GCs during initial GC mixin construction.

Oilpan: disable conservative GCs during initial GC mixin construction.

Another, more effective, attempt at disallowing conservative GCs from
striking while constructing a GC mixin object and it is not in a
well-formed state to allow a GC to strike.

GC mixins now override 'operator new', which along with taking care of
the allocation request, disables conservative GCs. GCs are kept disabled
until the constructors of the mixin's inherited classes have run and
the mixin itself is in a well-formed state to re-enable GCs.

Arrange for that re-enabling by way of a private field to an empty type
with a constructor that takes care of the underlying details. All of this
is hidden from direct view to the Blink code, still only requiring that
GC mixin instances declare themselves as such by using the
USING_GARBAGE_COLLECTED_MIXIN(Type) macro.

R=haraken
BUG=456823
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=191501

[sof, 2015-03-04 22:27:38]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2015-03-04 22:27:39]

Please take a look.

I reckon this is ready, but I wish there was a less involved way to achieve
this. But I can't see how.

(As this relies on GC plugin wrt 'operator new' overrides, I expect the bots to
be rather red in places.)

[haraken, 2015-03-05 02:20:18]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2015-03-05 02:20:19]

> Another, more effective, attempt at disallowing conservative GCs from
> striking while constructing a GC mixin object and it is not in a
> well-formed state to allow a GC to strike.

Just help me understand, how is the new approach more effective than the old
approach? Can the new approach catch more problematic cases?

https://codereview.chromium.org/980653002/diff/1/Source/core/frame/DeviceSing...
File Source/core/frame/DeviceSingleWindowEventController.h (left):

https://codereview.chromium.org/980653002/diff/1/Source/core/frame/DeviceSing...
Source/core/frame/DeviceSingleWindowEventController.h:18:
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(DeviceSingleWindowEventController);

Why can we remove this?

https://codereview.chromium.org/980653002/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/980653002/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.h:650: if (m_gcForbiddenCount)

Hmm, it is concerning that m_gcForbiddenCount can be unbalanced. It would be
safe as far as we decrease m_gcForbiddenCount too much, but it is dangerous if
we increase m_gcForbiddenCount too much because it will end up with suppressing
GCs for a long time. I don't think this will happen with this CL, but the fact
that the counter is not balanced looks not nice...

[sof, 2015-03-05 06:28:42]

On 2015/03/05 02:20:19, haraken wrote:
> > Another, more effective, attempt at disallowing conservative GCs from
> > striking while constructing a GC mixin object and it is not in a
> > well-formed state to allow a GC to strike.
> 
> Just help me understand, how is the new approach more effective than the old
> approach? Can the new approach catch more problematic cases?
> 

The approach one trunk leaves the forbidden GC scope first thing after having
allocated, i.e., GarbageCollected would most likely be the leftmost class and
its ctor body is run next. Which will leave the forbidden GC scope.. D'oh. The
reasoning that it would run after all the other inherited bases ctors had was
just wrong.

So, instead, we enter the GC scope upon allocation (*), and leave it by way of a
custom ctor for a field declared first in the mixins class.

* - the use of an overriding 'operator new' for Node makes this a bit tricky for
GC mixins that derive from Node, as mixins have to also arrange for the
forbidden GC scope to be entered. I wish this trickery wasn't needed..but work
around that by overriding 'operator new' for mixins instead here.

Re: nice/not nice comment on unbalanced accounting -- this comes about from
nested GC mixins, which will now have classes with nested declarations of these
fields whose ctor leave the GC-forbidden scope. I failed to see a way to avoid
those by way of metaprogramming (not for lack of trying), so had to open up for
the possibility.

[sof, 2015-03-05 07:14:59]

https://codereview.chromium.org/980653002/diff/1/Source/core/frame/DeviceSing...
File Source/core/frame/DeviceSingleWindowEventController.h (left):

https://codereview.chromium.org/980653002/diff/1/Source/core/frame/DeviceSing...
Source/core/frame/DeviceSingleWindowEventController.h:18:
WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(DeviceSingleWindowEventController);
On 2015/03/05 02:20:19, haraken wrote:
> 
> Why can we remove this?

It's an abstract class with all objects that implement it are also declared as
GC mixins. The declaration here serves no purpose?

(I can split the change out, its presence initially created some extra problems
for this CL.)

[sof, 2015-03-05 15:24:58]

Nested mixins are now dealt with in a manner I'm reasonably happy with.

[haraken, 2015-03-05 15:31:52]

> > > Another, more effective, attempt at disallowing conservative GCs from
> > > striking while constructing a GC mixin object and it is not in a
> > > well-formed state to allow a GC to strike.
> > 
> > Just help me understand, how is the new approach more effective than the old
> > approach? Can the new approach catch more problematic cases?
> > 
> 
> The approach one trunk leaves the forbidden GC scope first thing after having
> allocated, i.e., GarbageCollected would most likely be the leftmost class and
> its ctor body is run next. Which will leave the forbidden GC scope.. D'oh. The
> reasoning that it would run after all the other inherited bases ctors had was
> just wrong.

Thanks for the clarification! Now I understand what this CL is doing.

> So, instead, we enter the GC scope upon allocation (*), and leave it by way of
a
> custom ctor for a field declared first in the mixins class.

How is it guaranteed that we leave the GC scope when the custom constructor of
the right-most mixin class runs? I mean,

class A : public B, public Mixin1, public Mixin2 { };

how is it guaranteed that the GC scope is left in the constructor of Mixin2 (not
Mixin1)?

> * - the use of an overriding 'operator new' for Node makes this a bit tricky
for
> GC mixins that derive from Node, as mixins have to also arrange for the
> forbidden GC scope to be entered. I wish this trickery wasn't needed..but work
> around that by overriding 'operator new' for mixins instead here.
> 
> Re: nice/not nice comment on unbalanced accounting -- this comes about from
> nested GC mixins, which will now have classes with nested declarations of
these
> fields whose ctor leave the GC-forbidden scope. I failed to see a way to avoid
> those by way of metaprogramming (not for lack of trying), so had to open up
for
> the possibility.

Thanks, I'm still a bit concerned about the complexity of this CL (especially
for the new NESTED rule introduced to the code base). If we cannot come up with
a better idea than this, I'm OK with this though.

Another idea would be to let V8Interrupter run a conservative GC. That wouldn't
introduce any complexity to the code base, but that is hacky...

[sof, 2015-03-05 15:56:44]

On 2015/03/05 15:31:52, haraken wrote:
> > > > Another, more effective, attempt at disallowing conservative GCs from
> > > > striking while constructing a GC mixin object and it is not in a
> > > > well-formed state to allow a GC to strike.
> > > 
> > > Just help me understand, how is the new approach more effective than the
old
> > > approach? Can the new approach catch more problematic cases?
> > > 
> > 
> > The approach one trunk leaves the forbidden GC scope first thing after
having
> > allocated, i.e., GarbageCollected would most likely be the leftmost class
and
> > its ctor body is run next. Which will leave the forbidden GC scope.. D'oh.
The
> > reasoning that it would run after all the other inherited bases ctors had
was
> > just wrong.
> 
> Thanks for the clarification! Now I understand what this CL is doing.
> 

Really bad of me not to have thought through the previous one; just broken
entirely.

> > So, instead, we enter the GC scope upon allocation (*), and leave it by way
of
> a
> > custom ctor for a field declared first in the mixins class.
> 
> How is it guaranteed that we leave the GC scope when the custom constructor of
> the right-most mixin class runs? I mean,
> 
> class A : public B, public Mixin1, public Mixin2 { };
> 
> how is it guaranteed that the GC scope is left in the constructor of Mixin2
(not
> Mixin1)?
> 

I'm not certain what B, Mixin1 and Mixin2 refers to, but the rule for A is that
the GC scope will (and cannot) be left until its constructor (which encompasses
its field initialization list) starts executing. That's the first time A's vptr
has been initialized to hold the GC-mixin relevant vtable.

> > * - the use of an overriding 'operator new' for Node makes this a bit tricky
> for
> > GC mixins that derive from Node, as mixins have to also arrange for the
> > forbidden GC scope to be entered. I wish this trickery wasn't needed..but
work
> > around that by overriding 'operator new' for mixins instead here.
> > 
> > Re: nice/not nice comment on unbalanced accounting -- this comes about from
> > nested GC mixins, which will now have classes with nested declarations of
> these
> > fields whose ctor leave the GC-forbidden scope. I failed to see a way to
avoid
> > those by way of metaprogramming (not for lack of trying), so had to open up
> for
> > the possibility.
> 
> Thanks, I'm still a bit concerned about the complexity of this CL (especially
> for the new NESTED rule introduced to the code base). If we cannot come up
with
> a better idea than this, I'm OK with this though.
> 

It's a tight space to work in; right now, I have no other workable
alternatives/approaches.

There are enough static checks added, I hope, such that if you end up declaring
a mixin that nests another, you'll get a static compilation error.

[haraken, 2015-03-05 16:34:29]

On 2015/03/05 15:56:44, sof wrote:
> On 2015/03/05 15:31:52, haraken wrote:
> > > > > Another, more effective, attempt at disallowing conservative GCs from
> > > > > striking while constructing a GC mixin object and it is not in a
> > > > > well-formed state to allow a GC to strike.
> > > > 
> > > > Just help me understand, how is the new approach more effective than the
> old
> > > > approach? Can the new approach catch more problematic cases?
> > > > 
> > > 
> > > The approach one trunk leaves the forbidden GC scope first thing after
> having
> > > allocated, i.e., GarbageCollected would most likely be the leftmost class
> and
> > > its ctor body is run next. Which will leave the forbidden GC scope.. D'oh.
> The
> > > reasoning that it would run after all the other inherited bases ctors had
> was
> > > just wrong.
> > 
> > Thanks for the clarification! Now I understand what this CL is doing.
> > 
> 
> Really bad of me not to have thought through the previous one; just broken
> entirely.
> 
> > > So, instead, we enter the GC scope upon allocation (*), and leave it by
way
> of
> > a
> > > custom ctor for a field declared first in the mixins class.
> > 
> > How is it guaranteed that we leave the GC scope when the custom constructor
of
> > the right-most mixin class runs? I mean,
> > 
> > class A : public B, public Mixin1, public Mixin2 { };
> > 
> > how is it guaranteed that the GC scope is left in the constructor of Mixin2
> (not
> > Mixin1)?
> > 
> 
> I'm not certain what B, Mixin1 and Mixin2 refers to, but the rule for A is
that
> the GC scope will (and cannot) be left until its constructor (which
encompasses
> its field initialization list) starts executing. That's the first time A's
vptr
> has been initialized to hold the GC-mixin relevant vtable.

Just help me understand:

class GCMixin1 {
  GCMixin1() : ...  // (3)
  {
    // (4)
  } 
};

class GCMixin2 {
  GCMixin2() : ...  // (6)
  {
    // (7)
  } 
};

class A : public GarbageCollected<A>, public GCMixin1, public GCMixin2 {
  A()  // (1)
  : GCMixin1()  // (2)
  , GCMixin2()  // (5)
  {
    // (8)
  }
};

- At what point the vtable is guaranteed to be correct?
- At what point the GC forbidden scope is left?

[sof, 2015-03-05 16:46:47]

On 2015/03/05 16:34:29, haraken wrote:
> On 2015/03/05 15:56:44, sof wrote:
> > On 2015/03/05 15:31:52, haraken wrote:
> > > > > > Another, more effective, attempt at disallowing conservative GCs
from
> > > > > > striking while constructing a GC mixin object and it is not in a
> > > > > > well-formed state to allow a GC to strike.
> > > > > 
> > > > > Just help me understand, how is the new approach more effective than
the
> > old
> > > > > approach? Can the new approach catch more problematic cases?
> > > > > 
> > > > 
> > > > The approach one trunk leaves the forbidden GC scope first thing after
> > having
> > > > allocated, i.e., GarbageCollected would most likely be the leftmost
class
> > and
> > > > its ctor body is run next. Which will leave the forbidden GC scope..
D'oh.
> > The
> > > > reasoning that it would run after all the other inherited bases ctors
had
> > was
> > > > just wrong.
> > > 
> > > Thanks for the clarification! Now I understand what this CL is doing.
> > > 
> > 
> > Really bad of me not to have thought through the previous one; just broken
> > entirely.
> > 
> > > > So, instead, we enter the GC scope upon allocation (*), and leave it by
> way
> > of
> > > a
> > > > custom ctor for a field declared first in the mixins class.
> > > 
> > > How is it guaranteed that we leave the GC scope when the custom
constructor
> of
> > > the right-most mixin class runs? I mean,
> > > 
> > > class A : public B, public Mixin1, public Mixin2 { };
> > > 
> > > how is it guaranteed that the GC scope is left in the constructor of
Mixin2
> > (not
> > > Mixin1)?
> > > 
> > 
> > I'm not certain what B, Mixin1 and Mixin2 refers to, but the rule for A is
> that
> > the GC scope will (and cannot) be left until its constructor (which
> encompasses
> > its field initialization list) starts executing. That's the first time A's
> vptr
> > has been initialized to hold the GC-mixin relevant vtable.
> 
> Just help me understand:
> 
> class GCMixin1 {
>   GCMixin1() : ...  // (3)
>   {
>     // (4)
>   } 
> };
> 
> class GCMixin2 {
>   GCMixin2() : ...  // (6)
>   {
>     // (7)
>   } 
> };
> 
> class A : public GarbageCollected<A>, public GCMixin1, public GCMixin2 {
>   A()  // (1)
>   : GCMixin1()  // (2)
>   , GCMixin2()  // (5)
>   {
>     // (8)
>   }
> };
> 
> - At what point the vtable is guaranteed to be correct?
> - At what point the GC forbidden scope is left?

8 & 8 -- if the ctor had been

    A() : GCMixin1(), GCMixin2(), /*X*/ m_field1(SomeObject::create()),
m_field2(SomeObject::create())
    {
       // (8)
    }

then it would have been at point X, as the rewritten ctor will be something like

   void constructorForA(A* objA)
   {
     ...
     objA->vptr = vtable-for-A;
     objA->m_mixinConstructorMarker = GarbageCollectedMixinConstructorMarker();
     // X - GC scope has now been exited.
     objA->m_field1 = SomeObject::create();
     objA->m_field2 = SomeObject::create();
     // (8)
     ...
   }

[sof, 2015-03-05 16:55:26]

On 2015/03/05 16:46:47, sof wrote:
...
> > > > 
> > > 
> > > I'm not certain what B, Mixin1 and Mixin2 refers to, but the rule for A is
> > that
> > > the GC scope will (and cannot) be left until its constructor (which
> > encompasses
> > > its field initialization list) starts executing. That's the first time A's
> > vptr
> > > has been initialized to hold the GC-mixin relevant vtable.
> > 
> > Just help me understand:
> > 
> > class GCMixin1 {
> >   GCMixin1() : ...  // (3)
> >   {
> >     // (4)
> >   } 
> > };
> > 
> > class GCMixin2 {
> >   GCMixin2() : ...  // (6)
> >   {
> >     // (7)
> >   } 
> > };
> > 
> > class A : public GarbageCollected<A>, public GCMixin1, public GCMixin2 {
> >   A()  // (1)
> >   : GCMixin1()  // (2)
> >   , GCMixin2()  // (5)
> >   {
> >     // (8)
> >   }
> > };
> > 
> > - At what point the vtable is guaranteed to be correct?
> > - At what point the GC forbidden scope is left?
> 
> 8 & 8 -- if the ctor had been
> 
>     A() : GCMixin1(), GCMixin2(), /*X*/ m_field1(SomeObject::create()),
> m_field2(SomeObject::create())
>     {
>        // (8)
>     }
> 
> then it would have been at point X, as the rewritten ctor will be something
like
> 
>    void constructorForA(A* objA)
>    {
>      ...
>      objA->vptr = vtable-for-A;
>      objA->m_mixinConstructorMarker =
GarbageCollectedMixinConstructorMarker();
>      // X - GC scope has now been exited.
>      objA->m_field1 = SomeObject::create();
>      objA->m_field2 = SomeObject::create();
>      // (8)
>      ...
>    }

A's vptr is correct&valid (as an A) at the line prior to X, of course. 

If GCMixin1 refers to another class annotated with
USING_GARBAGE_COLLECTED_MIXIN(), the CL handles the nesting as expected, so that
the "inner" mixin constructor doesn't exit the GC scope prematurely while
constructing A.

[haraken, 2015-03-06 04:28:58]

On 2015/03/05 16:55:26, sof wrote:
> On 2015/03/05 16:46:47, sof wrote:
> ...
> > > > > 
> > > > 
> > > > I'm not certain what B, Mixin1 and Mixin2 refers to, but the rule for A
is
> > > that
> > > > the GC scope will (and cannot) be left until its constructor (which
> > > encompasses
> > > > its field initialization list) starts executing. That's the first time
A's
> > > vptr
> > > > has been initialized to hold the GC-mixin relevant vtable.
> > > 
> > > Just help me understand:
> > > 
> > > class GCMixin1 {
> > >   GCMixin1() : ...  // (3)
> > >   {
> > >     // (4)
> > >   } 
> > > };
> > > 
> > > class GCMixin2 {
> > >   GCMixin2() : ...  // (6)
> > >   {
> > >     // (7)
> > >   } 
> > > };
> > > 
> > > class A : public GarbageCollected<A>, public GCMixin1, public GCMixin2 {
> > >   A()  // (1)
> > >   : GCMixin1()  // (2)
> > >   , GCMixin2()  // (5)
> > >   {
> > >     // (8)
> > >   }
> > > };
> > > 
> > > - At what point the vtable is guaranteed to be correct?
> > > - At what point the GC forbidden scope is left?
> > 
> > 8 & 8 -- if the ctor had been
> > 
> >     A() : GCMixin1(), GCMixin2(), /*X*/ m_field1(SomeObject::create()),
> > m_field2(SomeObject::create())
> >     {
> >        // (8)
> >     }
> > 
> > then it would have been at point X, as the rewritten ctor will be something
> like
> > 
> >    void constructorForA(A* objA)
> >    {
> >      ...
> >      objA->vptr = vtable-for-A;
> >      objA->m_mixinConstructorMarker =
> GarbageCollectedMixinConstructorMarker();
> >      // X - GC scope has now been exited.
> >      objA->m_field1 = SomeObject::create();
> >      objA->m_field2 = SomeObject::create();
> >      // (8)
> >      ...
> >    }
> 
> A's vptr is correct&valid (as an A) at the line prior to X, of course. 
> 
> If GCMixin1 refers to another class annotated with
> USING_GARBAGE_COLLECTED_MIXIN(), the CL handles the nesting as expected, so
that
> the "inner" mixin constructor doesn't exit the GC scope prematurely while
> constructing A.

Thanks a lot; understood.

Can we probably get rid of the NESTED rule by doing something like this?

- operator TYPE::new() enters the GC forbidden scope once. It also records the
TYPE to ThreadState.
- In the GarbageCollectedMixinConstructorMarker<TYPE>'s constructor, leave the
GC forbidden scope only when the TYPE is equal to the TYPE recorded in the
ThreadState. This guarantees the balance of the scope counter.

It would be great if we have a test for this in HeapTest.cpp.

[sof, 2015-03-06 06:14:59]

On 2015/03/06 04:28:58, haraken wrote:
> On 2015/03/05 16:55:26, sof wrote:
> > On 2015/03/05 16:46:47, sof wrote:
> > ...
> > > > > > 
> > > > > 
> > > > > I'm not certain what B, Mixin1 and Mixin2 refers to, but the rule for
A
> is
> > > > that
> > > > > the GC scope will (and cannot) be left until its constructor (which
> > > > encompasses
> > > > > its field initialization list) starts executing. That's the first time
> A's
> > > > vptr
> > > > > has been initialized to hold the GC-mixin relevant vtable.
> > > > 
> > > > Just help me understand:
> > > > 
> > > > class GCMixin1 {
> > > >   GCMixin1() : ...  // (3)
> > > >   {
> > > >     // (4)
> > > >   } 
> > > > };
> > > > 
> > > > class GCMixin2 {
> > > >   GCMixin2() : ...  // (6)
> > > >   {
> > > >     // (7)
> > > >   } 
> > > > };
> > > > 
> > > > class A : public GarbageCollected<A>, public GCMixin1, public GCMixin2 {
> > > >   A()  // (1)
> > > >   : GCMixin1()  // (2)
> > > >   , GCMixin2()  // (5)
> > > >   {
> > > >     // (8)
> > > >   }
> > > > };
> > > > 
> > > > - At what point the vtable is guaranteed to be correct?
> > > > - At what point the GC forbidden scope is left?
> > > 
> > > 8 & 8 -- if the ctor had been
> > > 
> > >     A() : GCMixin1(), GCMixin2(), /*X*/ m_field1(SomeObject::create()),
> > > m_field2(SomeObject::create())
> > >     {
> > >        // (8)
> > >     }
> > > 
> > > then it would have been at point X, as the rewritten ctor will be
something
> > like
> > > 
> > >    void constructorForA(A* objA)
> > >    {
> > >      ...
> > >      objA->vptr = vtable-for-A;
> > >      objA->m_mixinConstructorMarker =
> > GarbageCollectedMixinConstructorMarker();
> > >      // X - GC scope has now been exited.
> > >      objA->m_field1 = SomeObject::create();
> > >      objA->m_field2 = SomeObject::create();
> > >      // (8)
> > >      ...
> > >    }
> > 
> > A's vptr is correct&valid (as an A) at the line prior to X, of course. 
> > 
> > If GCMixin1 refers to another class annotated with
> > USING_GARBAGE_COLLECTED_MIXIN(), the CL handles the nesting as expected, so
> that
> > the "inner" mixin constructor doesn't exit the GC scope prematurely while
> > constructing A.
> 
> Thanks a lot; understood.
> 
> Can we probably get rid of the NESTED rule by doing something like this?
> 
> - operator TYPE::new() enters the GC forbidden scope once. It also records the
> TYPE to ThreadState.
> - In the GarbageCollectedMixinConstructorMarker<TYPE>'s constructor, leave the
> GC forbidden scope only when the TYPE is equal to the TYPE recorded in the
> ThreadState. This guarantees the balance of the scope counter.
> 

So you would potentially have an unbounded stack of them? What is TYPE? What
problem are you solving by doing it this way?

[haraken, 2015-03-06 06:20:37]

On 2015/03/06 06:14:59, sof wrote:
> On 2015/03/06 04:28:58, haraken wrote:
> > On 2015/03/05 16:55:26, sof wrote:
> > > On 2015/03/05 16:46:47, sof wrote:
> > > ...
> > > > > > > 
> > > > > > 
> > > > > > I'm not certain what B, Mixin1 and Mixin2 refers to, but the rule
for
> A
> > is
> > > > > that
> > > > > > the GC scope will (and cannot) be left until its constructor (which
> > > > > encompasses
> > > > > > its field initialization list) starts executing. That's the first
time
> > A's
> > > > > vptr
> > > > > > has been initialized to hold the GC-mixin relevant vtable.
> > > > > 
> > > > > Just help me understand:
> > > > > 
> > > > > class GCMixin1 {
> > > > >   GCMixin1() : ...  // (3)
> > > > >   {
> > > > >     // (4)
> > > > >   } 
> > > > > };
> > > > > 
> > > > > class GCMixin2 {
> > > > >   GCMixin2() : ...  // (6)
> > > > >   {
> > > > >     // (7)
> > > > >   } 
> > > > > };
> > > > > 
> > > > > class A : public GarbageCollected<A>, public GCMixin1, public GCMixin2
{
> > > > >   A()  // (1)
> > > > >   : GCMixin1()  // (2)
> > > > >   , GCMixin2()  // (5)
> > > > >   {
> > > > >     // (8)
> > > > >   }
> > > > > };
> > > > > 
> > > > > - At what point the vtable is guaranteed to be correct?
> > > > > - At what point the GC forbidden scope is left?
> > > > 
> > > > 8 & 8 -- if the ctor had been
> > > > 
> > > >     A() : GCMixin1(), GCMixin2(), /*X*/ m_field1(SomeObject::create()),
> > > > m_field2(SomeObject::create())
> > > >     {
> > > >        // (8)
> > > >     }
> > > > 
> > > > then it would have been at point X, as the rewritten ctor will be
> something
> > > like
> > > > 
> > > >    void constructorForA(A* objA)
> > > >    {
> > > >      ...
> > > >      objA->vptr = vtable-for-A;
> > > >      objA->m_mixinConstructorMarker =
> > > GarbageCollectedMixinConstructorMarker();
> > > >      // X - GC scope has now been exited.
> > > >      objA->m_field1 = SomeObject::create();
> > > >      objA->m_field2 = SomeObject::create();
> > > >      // (8)
> > > >      ...
> > > >    }
> > > 
> > > A's vptr is correct&valid (as an A) at the line prior to X, of course. 
> > > 
> > > If GCMixin1 refers to another class annotated with
> > > USING_GARBAGE_COLLECTED_MIXIN(), the CL handles the nesting as expected,
so
> > that
> > > the "inner" mixin constructor doesn't exit the GC scope prematurely while
> > > constructing A.
> > 
> > Thanks a lot; understood.
> > 
> > Can we probably get rid of the NESTED rule by doing something like this?
> > 
> > - operator TYPE::new() enters the GC forbidden scope once. It also records
the
> > TYPE to ThreadState.
> > - In the GarbageCollectedMixinConstructorMarker<TYPE>'s constructor, leave
the
> > GC forbidden scope only when the TYPE is equal to the TYPE recorded in the
> > ThreadState. This guarantees the balance of the scope counter.
> > 
> 
> So you would potentially have an unbounded stack of them?

What do you mean by an unbounded stack?

> What is TYPE?

For example, you can use a pointer value of
GarbageCollectedMixinConstructorMarker.

class A : public GarbageCollected<A>, public Mixin1, public Mixin2 {
operator new() {
  state->enterGCForbiddenScope(&m_mixinConstructorMarker);
}
GarbageCollectedMixinConstructorMarker m_mixinConstructorMarker;  // This
constructor leaves the forbidden scope if state has m_mixinConstructorMarker.
Otherwise, do nothing.
};

> What problem are you solving by doing it this way?

The problem I'm solving here is not to introduce another Oilpan programming rule
to the code base (i.e., the NESTED rule).

[sof, 2015-03-06 06:25:31]

On 2015/03/06 06:20:37, haraken wrote:
> On 2015/03/06 06:14:59, sof wrote:
> > On 2015/03/06 04:28:58, haraken wrote:
> > > On 2015/03/05 16:55:26, sof wrote:
> > > > On 2015/03/05 16:46:47, sof wrote:
> > > > ...
> > > > > > > > 
> > > > > > > 
> > > > > > > I'm not certain what B, Mixin1 and Mixin2 refers to, but the rule
> for
> > A
> > > is
> > > > > > that
> > > > > > > the GC scope will (and cannot) be left until its constructor
(which
> > > > > > encompasses
> > > > > > > its field initialization list) starts executing. That's the first
> time
> > > A's
> > > > > > vptr
> > > > > > > has been initialized to hold the GC-mixin relevant vtable.
> > > > > > 
> > > > > > Just help me understand:
> > > > > > 
> > > > > > class GCMixin1 {
> > > > > >   GCMixin1() : ...  // (3)
> > > > > >   {
> > > > > >     // (4)
> > > > > >   } 
> > > > > > };
> > > > > > 
> > > > > > class GCMixin2 {
> > > > > >   GCMixin2() : ...  // (6)
> > > > > >   {
> > > > > >     // (7)
> > > > > >   } 
> > > > > > };
> > > > > > 
> > > > > > class A : public GarbageCollected<A>, public GCMixin1, public
GCMixin2
> {
> > > > > >   A()  // (1)
> > > > > >   : GCMixin1()  // (2)
> > > > > >   , GCMixin2()  // (5)
> > > > > >   {
> > > > > >     // (8)
> > > > > >   }
> > > > > > };
> > > > > > 
> > > > > > - At what point the vtable is guaranteed to be correct?
> > > > > > - At what point the GC forbidden scope is left?
> > > > > 
> > > > > 8 & 8 -- if the ctor had been
> > > > > 
> > > > >     A() : GCMixin1(), GCMixin2(), /*X*/
m_field1(SomeObject::create()),
> > > > > m_field2(SomeObject::create())
> > > > >     {
> > > > >        // (8)
> > > > >     }
> > > > > 
> > > > > then it would have been at point X, as the rewritten ctor will be
> > something
> > > > like
> > > > > 
> > > > >    void constructorForA(A* objA)
> > > > >    {
> > > > >      ...
> > > > >      objA->vptr = vtable-for-A;
> > > > >      objA->m_mixinConstructorMarker =
> > > > GarbageCollectedMixinConstructorMarker();
> > > > >      // X - GC scope has now been exited.
> > > > >      objA->m_field1 = SomeObject::create();
> > > > >      objA->m_field2 = SomeObject::create();
> > > > >      // (8)
> > > > >      ...
> > > > >    }
> > > > 
> > > > A's vptr is correct&valid (as an A) at the line prior to X, of course. 
> > > > 
> > > > If GCMixin1 refers to another class annotated with
> > > > USING_GARBAGE_COLLECTED_MIXIN(), the CL handles the nesting as expected,
> so
> > > that
> > > > the "inner" mixin constructor doesn't exit the GC scope prematurely
while
> > > > constructing A.
> > > 
> > > Thanks a lot; understood.
> > > 
> > > Can we probably get rid of the NESTED rule by doing something like this?
> > > 
> > > - operator TYPE::new() enters the GC forbidden scope once. It also records
> the
> > > TYPE to ThreadState.
> > > - In the GarbageCollectedMixinConstructorMarker<TYPE>'s constructor, leave
> the
> > > GC forbidden scope only when the TYPE is equal to the TYPE recorded in the
> > > ThreadState. This guarantees the balance of the scope counter.
> > > 
> > 
> > So you would potentially have an unbounded stack of them?
> 
> What do you mean by an unbounded stack?
> 

Mixin allocations can be nested, right?

> > What is TYPE?
> 
> For example, you can use a pointer value of
> GarbageCollectedMixinConstructorMarker.
> 
> class A : public GarbageCollected<A>, public Mixin1, public Mixin2 {
> operator new() {
>   state->enterGCForbiddenScope(&m_mixinConstructorMarker);
> }
> GarbageCollectedMixinConstructorMarker m_mixinConstructorMarker;  // This
> constructor leaves the forbidden scope if state has m_mixinConstructorMarker.
> Otherwise, do nothing.
> };
> 
> > What problem are you solving by doing it this way?
> 
> The problem I'm solving here is not to introduce another Oilpan programming
rule
> to the code base (i.e., the NESTED rule).

I don't agree.

[haraken, 2015-03-06 06:34:31]

> Mixin allocations can be nested, right?

Doesn't this work?

class A : public GarbageCollected<A>, public Mixin1, public Mixin2 {
  operator new() {
    state->enterGCForbiddenScopeIfNecessary(&m_marker);
  }
  ConstructorMarker m_marker;
};

ConstructorMarker::ConstructorMarker() {
  state->leaveGCForbiddenScopeIfNecessary(this);
}

ThreadState::enterGCForbiddenScopeIfNecessary(ConstructorMarker* marker) {
  m_markers.push(marker);
  m_gcForbiddenScope++;
}

ThreadState::leaveGCForbiddenScopeIfNecessary(ConstructorMarker* marker) {
  ConstructorMarker* currentMarker = m_markers.last();
  if (currentmarker == marker) {
    m_markers.pop();
    m_gcForbiddenScope--;
  }
}


> > > What problem are you solving by doing it this way?
> > 
> > The problem I'm solving here is not to introduce another Oilpan programming
> rule
> > to the code base (i.e., the NESTED rule).
> 
> I don't agree.

If there is no easy way to avoid an extra rule, I'm OK with it, but if we can
avoid it, I think it would be better to avoid it. We sometimes receive feedback
that Oilpan's syntax is already too complicated.

[sof, 2015-03-06 06:43:04]

On 2015/03/06 06:34:31, haraken wrote:
> > Mixin allocations can be nested, right?
> 
> Doesn't this work?
> 
> class A : public GarbageCollected<A>, public Mixin1, public Mixin2 {
>   operator new() {
>     state->enterGCForbiddenScopeIfNecessary(&m_marker);
>   }
>   ConstructorMarker m_marker;
> };
> 

At this point we've allocated a block for A, but not constructed it in any way -
could you reliably compute the field offset into its block without having done
any construction of it?

> ConstructorMarker::ConstructorMarker() {
>   state->leaveGCForbiddenScopeIfNecessary(this);
> }
> 
> ThreadState::enterGCForbiddenScopeIfNecessary(ConstructorMarker* marker) {
>   m_markers.push(marker);
>   m_gcForbiddenScope++;
> }
> 
> ThreadState::leaveGCForbiddenScopeIfNecessary(ConstructorMarker* marker) {
>   ConstructorMarker* currentMarker = m_markers.last();
>   if (currentmarker == marker) {
>     m_markers.pop();
>     m_gcForbiddenScope--;
>   }
> }
> 
> 
> > > > What problem are you solving by doing it this way?
> > > 
> > > The problem I'm solving here is not to introduce another Oilpan
programming
> > rule
> > > to the code base (i.e., the NESTED rule).
> > 
> > I don't agree.
> 
> If there is no easy way to avoid an extra rule, I'm OK with it, but if we can
> avoid it, I think it would be better to avoid it. We sometimes receive
feedback
> that Oilpan's syntax is already too complicated.

You have considered the limited places where it is needed & that there are
static checks in place should you try construct a non-nested kind where you
shouldn't?

[haraken, 2015-03-06 06:51:41]

On 2015/03/06 06:43:04, sof wrote:
> On 2015/03/06 06:34:31, haraken wrote:
> > > Mixin allocations can be nested, right?
> > 
> > Doesn't this work?
> > 
> > class A : public GarbageCollected<A>, public Mixin1, public Mixin2 {
> >   operator new() {
> >     state->enterGCForbiddenScopeIfNecessary(&m_marker);
> >   }
> >   ConstructorMarker m_marker;
> > };
> > 
> 
> At this point we've allocated a block for A, but not constructed it in any way
-
> could you reliably compute the field offset into its block without having done
> any construction of it?

I was thinking we can rely on it in practice, but I don't know :) (If we cannot
compute the offset, that means we cannot do something like m_marker->... in
operator new. I'm not sure if it's a supported behavior in the spec, but guess
it would work in practice.)

(Note: It's nasty to rely on non-speced behavior, but we're already relying on
it -- e.g, vptr exists at the head of the object etc)

[sof, 2015-03-06 07:04:08]

On 2015/03/06 06:51:41, haraken wrote:
> On 2015/03/06 06:43:04, sof wrote:
> > On 2015/03/06 06:34:31, haraken wrote:
> > > > Mixin allocations can be nested, right?
> > > 
> > > Doesn't this work?
> > > 
> > > class A : public GarbageCollected<A>, public Mixin1, public Mixin2 {
> > >   operator new() {
> > >     state->enterGCForbiddenScopeIfNecessary(&m_marker);
> > >   }
> > >   ConstructorMarker m_marker;
> > > };
> > > 
> > 
> > At this point we've allocated a block for A, but not constructed it in any
way
> -
> > could you reliably compute the field offset into its block without having
done
> > any construction of it?
> 
> I was thinking we can rely on it in practice, but I don't know :) (If we
cannot
> compute the offset, that means we cannot do something like m_marker->... in
> operator new. I'm not sure if it's a supported behavior in the spec, but guess
> it would work in practice.)
> 
> (Note: It's nasty to rely on non-speced behavior, but we're already relying on
> it -- e.g, vptr exists at the head of the object etc)

I'm not particularly interested in doing these exploratory experiments on your
behalf, hope you understand. What's here is rather good already.

[haraken, 2015-03-06 13:01:01]

LGTM. Although I want to avoid introducing another Oilpan syntax, let's land
this CL since this is definitely improving the infrastructure.


A slightly simpler idea:

class A : public GarbageCollected<A>, public Mixin1, public Mixin2 {
  operator new() {
    state->enterGCForbiddenScopeIfNecessary(&m_marker);
  }
  ConstructorMarker m_marker;
};

ConstructorMarker::ConstructorMarker() {
  state->leaveGCForbiddenScopeIfNecessary(this);
}

ThreadState::enterGCForbiddenScopeIfNecessary(ConstructorMarker* marker) {
  if (!m_marker) {  // This can handle nested mixin constructions. We don't need
to prepare a stack of markers.
    m_marker = marker;
    m_gcForbiddenScope++;
  }
}

ThreadState::leaveGCForbiddenScopeIfNecessary(ConstructorMarker* marker) {
  if (m_marker == marker) {
    m_gcForbiddenScope--;
  }
}

https://codereview.chromium.org/980653002/diff/20002/Source/platform/heap/Vis...
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/980653002/diff/20002/Source/platform/heap/Vis...
Source/platform/heap/Visitor.h:952: void* operator new(size_t size)             
                                       \

As far as I grep in the code base, GARBAGE_COLLECTED_MIXIN is not used in
performance-sensitive classes. So the overhead in operator new would be OK, I
hope.

https://codereview.chromium.org/980653002/diff/20002/Source/platform/heap/Vis...
Source/platform/heap/Visitor.h:980: #define
DEFINE_GARBAGE_COLLECTED_MIXIN_NESTED(TYPE, SUBTYPE)      \

Just to confirm: This mechanism can detect the following two kinds of errors,
right?

- Forgot to add NESTED.
- Added NESTED by mistake.

[sof, 2015-03-06 15:48:23]

This CL cannot move forward until the GC plugin change in
http://crbug.com/443854 has been rolled out in a plugin update.

I've been trying to come up with an encoding scheme/sfinae magic that lets us
statically compute the GC mixin nesting level, but have failed. If such a
construction exists, there wouldn't be a need to explicitly state that some of
these mixin types nest others.

[sof, 2015-03-06 16:11:06]

https://codereview.chromium.org/980653002/diff/20002/Source/platform/heap/Vis...
File Source/platform/heap/Visitor.h (right):

https://codereview.chromium.org/980653002/diff/20002/Source/platform/heap/Vis...
Source/platform/heap/Visitor.h:980: #define
DEFINE_GARBAGE_COLLECTED_MIXIN_NESTED(TYPE, SUBTYPE)      \
On 2015/03/06 13:01:01, haraken wrote:
> 
> Just to confirm: This mechanism can detect the following two kinds of errors,
> right?
> 
> - Forgot to add NESTED.

The final mixinsCannotBeImplicitlyNested() empty method catches this.

> - Added NESTED by mistake.

e.g., USING_GARBAGE_COLLECTED_MIXIN_NESTED(A, B);

If B isn't a GC mixin, compilation would fail. If it is, but A does not derive
from it, compilation would fail. If both those properties hold, then A is indeed
a mixin that nests B & this declaration would be the correct thing to supply.

[sof, 2015-03-07 08:33:11]

On 2015/03/06 04:28:58, haraken wrote:
>
...
> 
> It would be great if we have a test for this in HeapTest.cpp.

Added one + expanded the comments in Visitor.h, including a FIXME concerning the
handling of nested mixin types. I think it is worthwhile to do most of this
statically (at compile-time.)

[haraken, 2015-03-07 09:19:15]

On 2015/03/07 08:33:11, sof wrote:
> On 2015/03/06 04:28:58, haraken wrote:
> >
> ...
> > 
> > It would be great if we have a test for this in HeapTest.cpp.
> 
> Added one + expanded the comments in Visitor.h, including a FIXME concerning
the
> handling of nested mixin types. I think it is worthwhile to do most of this
> statically (at compile-time.)

Can we add a test that allocates a lot of objects in mixin constructors and
verify that it triggers a GC after the mixin constructions have run?

[sof, 2015-03-07 21:52:24]

On 2015/03/07 09:19:15, haraken wrote:
> On 2015/03/07 08:33:11, sof wrote:
> > On 2015/03/06 04:28:58, haraken wrote:
> > >
> > ...
> > > 
> > > It would be great if we have a test for this in HeapTest.cpp.
> > 
> > Added one + expanded the comments in Visitor.h, including a FIXME concerning
> the
> > handling of nested mixin types. I think it is worthwhile to do most of this
> > statically (at compile-time.)
> 
> Can we add a test that allocates a lot of objects in mixin constructors and
> verify that it triggers a GC after the mixin constructions have run?

Better have coverage for that; added. I also tweaked an older test that verifies
that stack-reachable mixins are only marked if found to be without a vtable.

[haraken, 2015-03-08 02:13:59]

On 2015/03/07 21:52:24, sof wrote:
> On 2015/03/07 09:19:15, haraken wrote:
> > On 2015/03/07 08:33:11, sof wrote:
> > > On 2015/03/06 04:28:58, haraken wrote:
> > > >
> > > ...
> > > > 
> > > > It would be great if we have a test for this in HeapTest.cpp.
> > > 
> > > Added one + expanded the comments in Visitor.h, including a FIXME
concerning
> > the
> > > handling of nested mixin types. I think it is worthwhile to do most of
this
> > > statically (at compile-time.)
> > 
> > Can we add a test that allocates a lot of objects in mixin constructors and
> > verify that it triggers a GC after the mixin constructions have run?
> 
> Better have coverage for that; added. I also tweaked an older test that
verifies
> that stack-reachable mixins are only marked if found to be without a vtable.

Thanks, looks good.

[sof, 2015-03-08 22:17:38]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-03-08 22:17:38]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/980653002/#ps130001
(title: "Add GC mixin test")

[commit-bot: I haz the power, 2015-03-08 22:18:13]

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

[commit-bot: I haz the power, 2015-03-08 23:33:54]

Committed patchset #8 (id:130001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=191501