Oilpan: introduce eager finalization.

Oilpan: introduce eager finalization.

Extend the capabilities of eager finalization and the eagerly swept heap.
Objects allocated on that heap are now allowed to have destructors that
can (but are not encouraged) to access other heap objects, provided the
objects are not other eagerly finalized objects. (Few objects are thought
to require eager finalization, so this is assumed not to limit their
use and usefulness.)

The clang Oilpan/GC plugin needs to be extended to allow such destructors,
and catch out any/some attempts to access other eagerly finalized objects
from destructors (https://codereview.chromium.org/1158623002/)

To additionally enforce finalizer correctness, ASan heap poisoning during
sweeps is also strengthened: all payloads on the eagerly swept heap are
poisoned so as to catch out illegal inter-heap references while the sweep
is underway. Once the eager sweep has completed, access to the live objects
is allowed, while all unmarked objects in other heaps are poisoned.

With the above in place, existing "prefinalizer" usage can be recast to
be eagerly finalized instead. This will be handled in a followup CL.

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

[sof, 2015-05-28 07:13:15]

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

[sof, 2015-05-28 07:13:16]

Please take a look.

As the prefinalizer switchover is dependent on GC plugin support, will handle
that separately (when rolled.)

[haraken, 2015-05-28 12:30:03]

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

[haraken, 2015-05-28 12:30:05]

Thanks for working on this!

https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
File Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
Source/core/events/EventTarget.h:207: // EventTarget is not eagerly finalized.

Oh, does this mean that if we have:

class A : public GarbageCollected<A> { };

class B : public A {
  EAGERLY_FINALIZED();
  static B* create() {
    return new B;  // This won't be allocated on the eager heap unless we
override the operator 'new' in A?
  }
};

If that is the case, what happens if we have:

class X : public RefCountedGarbageCollectedEventTargetWithInlineData<X> { };

class Y : public X {
  EAGERLY_FINALIZED();
  static Y* create() {
    return new Y;  // This won't be allocated on the eager heap unless we
override the operator 'new' in X?
  }
};

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:341: void BaseHeap::poisonHeap(bool setPoison)

poisonHeap => poisonEagerHeap

I'd use SetPoison/ClearPoison instead of bool.

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:348: for (BasePage* page = m_firstUnsweptPage;
page; page = page->next()) {

Add ASSERT(!m_firstPage).

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:349:
page->poisonObjects(BasePage::UnmarkedOrMarked, BasePage::SetPoison);

Why do we need to poison marked objects? I'm ok with this, but I'm wondering why
we need it because we're not poisoning marked objects for other heaps.

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:354: page->poisonObjects(BasePage::UnmarkedOnly,
BasePage::ClearPoison);

Who unpoisons the marked objects that have been poisoned at line 349?

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1298: int heapIndex = page->heap()->heapIndex();

I'd add ASSERT(heapIndex != EagerSweepHeapIndex) // Don't yet support.

reallocate is used by a very limited number of classes, so we won't need to
support it.

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:925: poisonHeapsForCompleteSweep();

poisonEagerHeaps => poisonEagerHeap
poisonHeapsForCompleteSweep => poisonAllHeaps

?

[sof, 2015-05-28 12:48:54]

https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
File Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
Source/core/events/EventTarget.h:207: // EventTarget is not eagerly finalized.
On 2015/05/28 12:30:04, haraken wrote:
> 
> Oh, does this mean that if we have:
> 
> class A : public GarbageCollected<A> { };
> 
> class B : public A {
>   EAGERLY_FINALIZED();
>   static B* create() {
>     return new B;  // This won't be allocated on the eager heap unless we
> override the operator 'new' in A?
>   }
> };
> 
> If that is the case, what happens if we have:
> 
> class X : public RefCountedGarbageCollectedEventTargetWithInlineData<X> { };
> 
> class Y : public X {
>   EAGERLY_FINALIZED();
>   static Y* create() {
>     return new Y;  // This won't be allocated on the eager heap unless we
> override the operator 'new' in X?
>   }
> };

Yes, the eagerly finalized types have to have their "GC base" as immediates &
cannot inherit that (mixins excepted.)

[sof, 2015-05-28 12:51:39]

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1298: int heapIndex = page->heap()->heapIndex();
On 2015/05/28 12:30:04, haraken wrote:
> 
> I'd add ASSERT(heapIndex != EagerSweepHeapIndex) // Don't yet support.
> 
> reallocate is used by a very limited number of classes, so we won't need to
> support it.

That seems like a random restriction to make; what if you embed one in a
HeapTerminatedArray at some point?

(This also fixes e.g., Node/CSSValue reallocation.)

[haraken, 2015-05-28 14:20:24]

On 2015/05/28 12:48:54, sof wrote:
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> File Source/core/events/EventTarget.h (right):
> 
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> Source/core/events/EventTarget.h:207: // EventTarget is not eagerly finalized.
> On 2015/05/28 12:30:04, haraken wrote:
> > 
> > Oh, does this mean that if we have:
> > 
> > class A : public GarbageCollected<A> { };
> > 
> > class B : public A {
> >   EAGERLY_FINALIZED();
> >   static B* create() {
> >     return new B;  // This won't be allocated on the eager heap unless we
> > override the operator 'new' in A?
> >   }
> > };
> > 
> > If that is the case, what happens if we have:
> > 
> > class X : public RefCountedGarbageCollectedEventTargetWithInlineData<X> { };
> > 
> > class Y : public X {
> >   EAGERLY_FINALIZED();
> >   static Y* create() {
> >     return new Y;  // This won't be allocated on the eager heap unless we
> > override the operator 'new' in X?
> >   }
> > };
> 
> Yes, the eagerly finalized types have to have their "GC base" as immediates &
> cannot inherit that (mixins excepted.)

Hmm, it seems we're hitting a problem (again) :)

What happens if we have:

class ActiveDOMObject {
  EAGERLY_FINALIZE();
};
class X : public GarbageCollected<X> { };
class Y : public X, public ActiveDOMObject<Y> { };  // How can this be allocated
on the eager heap?

This can happen in HTMLMdiaElement and so on. Hmm.

One (bad) idea I came up with is to write ActiveDOMObject as:

class ActiveDOMObject {
  ActiveDOMObject() {
    registerEagerSweep(this);  // Register the pointer at runtime
  }
};

Then Oilpan keeps track of the registered pointers and call destructors of those
pointers first. However, I don't like this idea since it will introduce a lot of
complexity to the GC infrastructure. Pre-finalizers would be much simpler than
this...

[sof, 2015-05-28 14:32:01]

On 2015/05/28 14:20:24, haraken wrote:
> On 2015/05/28 12:48:54, sof wrote:
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > File Source/core/events/EventTarget.h (right):
> > 
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > Source/core/events/EventTarget.h:207: // EventTarget is not eagerly
finalized.
> > On 2015/05/28 12:30:04, haraken wrote:
> > > 
> > > Oh, does this mean that if we have:
> > > 
> > > class A : public GarbageCollected<A> { };
> > > 
> > > class B : public A {
> > >   EAGERLY_FINALIZED();
> > >   static B* create() {
> > >     return new B;  // This won't be allocated on the eager heap unless we
> > > override the operator 'new' in A?
> > >   }
> > > };
> > > 
> > > If that is the case, what happens if we have:
> > > 
> > > class X : public RefCountedGarbageCollectedEventTargetWithInlineData<X> {
};
> > > 
> > > class Y : public X {
> > >   EAGERLY_FINALIZED();
> > >   static Y* create() {
> > >     return new Y;  // This won't be allocated on the eager heap unless we
> > > override the operator 'new' in X?
> > >   }
> > > };
> > 
> > Yes, the eagerly finalized types have to have their "GC base" as immediates
&
> > cannot inherit that (mixins excepted.)
> 
> Hmm, it seems we're hitting a problem (again) :)
> 
> What happens if we have:
> 
> class ActiveDOMObject {
>   EAGERLY_FINALIZE();
> };
> class X : public GarbageCollected<X> { };
> class Y : public X, public ActiveDOMObject<Y> { };  // How can this be
allocated
> on the eager heap?
> 
> This can happen in HTMLMdiaElement and so on. Hmm.
> 
> One (bad) idea I came up with is to write ActiveDOMObject as:
> 
> class ActiveDOMObject {
>   ActiveDOMObject() {
>     registerEagerSweep(this);  // Register the pointer at runtime
>   }
> };
> 
> Then Oilpan keeps track of the registered pointers and call destructors of
those
> pointers first. However, I don't like this idea since it will introduce a lot
of
> complexity to the GC infrastructure. Pre-finalizers would be much simpler than
> this...

For mixins, this is fine. I'm not convinced (yet) you're describing a real
problem -- the current set of prefinalizers & other eagerly finalizable objects
don't run into this.

[haraken, 2015-05-28 15:04:25]

On 2015/05/28 14:32:01, sof wrote:
> On 2015/05/28 14:20:24, haraken wrote:
> > On 2015/05/28 12:48:54, sof wrote:
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > File Source/core/events/EventTarget.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > Source/core/events/EventTarget.h:207: // EventTarget is not eagerly
> finalized.
> > > On 2015/05/28 12:30:04, haraken wrote:
> > > > 
> > > > Oh, does this mean that if we have:
> > > > 
> > > > class A : public GarbageCollected<A> { };
> > > > 
> > > > class B : public A {
> > > >   EAGERLY_FINALIZED();
> > > >   static B* create() {
> > > >     return new B;  // This won't be allocated on the eager heap unless
we
> > > > override the operator 'new' in A?
> > > >   }
> > > > };
> > > > 
> > > > If that is the case, what happens if we have:
> > > > 
> > > > class X : public RefCountedGarbageCollectedEventTargetWithInlineData<X>
{
> };
> > > > 
> > > > class Y : public X {
> > > >   EAGERLY_FINALIZED();
> > > >   static Y* create() {
> > > >     return new Y;  // This won't be allocated on the eager heap unless
we
> > > > override the operator 'new' in X?
> > > >   }
> > > > };
> > > 
> > > Yes, the eagerly finalized types have to have their "GC base" as
immediates
> &
> > > cannot inherit that (mixins excepted.)
> > 
> > Hmm, it seems we're hitting a problem (again) :)
> > 
> > What happens if we have:
> > 
> > class ActiveDOMObject {
> >   EAGERLY_FINALIZE();
> > };
> > class X : public GarbageCollected<X> { };
> > class Y : public X, public ActiveDOMObject<Y> { };  // How can this be
> allocated
> > on the eager heap?
> > 
> > This can happen in HTMLMdiaElement and so on. Hmm.
> > 
> > One (bad) idea I came up with is to write ActiveDOMObject as:
> > 
> > class ActiveDOMObject {
> >   ActiveDOMObject() {
> >     registerEagerSweep(this);  // Register the pointer at runtime
> >   }
> > };
> > 
> > Then Oilpan keeps track of the registered pointers and call destructors of
> those
> > pointers first. However, I don't like this idea since it will introduce a
lot
> of
> > complexity to the GC infrastructure. Pre-finalizers would be much simpler
than
> > this...
> 
> For mixins, this is fine. I'm not convinced (yet) you're describing a real
> problem -- the current set of prefinalizers & other eagerly finalizable
objects
> don't run into this.

Just help me understand, how can Oilpan understand that it needs to allocate X
on the eager heap if it is actually an instance of Y?

[sof, 2015-05-28 15:07:38]

On 2015/05/28 15:04:25, haraken wrote:
> On 2015/05/28 14:32:01, sof wrote:
> > On 2015/05/28 14:20:24, haraken wrote:
> > > On 2015/05/28 12:48:54, sof wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > > File Source/core/events/EventTarget.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > > Source/core/events/EventTarget.h:207: // EventTarget is not eagerly
> > finalized.
> > > > On 2015/05/28 12:30:04, haraken wrote:
> > > > > 
> > > > > Oh, does this mean that if we have:
> > > > > 
> > > > > class A : public GarbageCollected<A> { };
> > > > > 
> > > > > class B : public A {
> > > > >   EAGERLY_FINALIZED();
> > > > >   static B* create() {
> > > > >     return new B;  // This won't be allocated on the eager heap unless
> we
> > > > > override the operator 'new' in A?
> > > > >   }
> > > > > };
> > > > > 
> > > > > If that is the case, what happens if we have:
> > > > > 
> > > > > class X : public
RefCountedGarbageCollectedEventTargetWithInlineData<X>
> {
> > };
> > > > > 
> > > > > class Y : public X {
> > > > >   EAGERLY_FINALIZED();
> > > > >   static Y* create() {
> > > > >     return new Y;  // This won't be allocated on the eager heap unless
> we
> > > > > override the operator 'new' in X?
> > > > >   }
> > > > > };
> > > > 
> > > > Yes, the eagerly finalized types have to have their "GC base" as
> immediates
> > &
> > > > cannot inherit that (mixins excepted.)
> > > 
> > > Hmm, it seems we're hitting a problem (again) :)
> > > 
> > > What happens if we have:
> > > 
> > > class ActiveDOMObject {
> > >   EAGERLY_FINALIZE();
> > > };
> > > class X : public GarbageCollected<X> { };
> > > class Y : public X, public ActiveDOMObject<Y> { };  // How can this be
> > allocated
> > > on the eager heap?
> > > 
> > > This can happen in HTMLMdiaElement and so on. Hmm.
> > > 
> > > One (bad) idea I came up with is to write ActiveDOMObject as:
> > > 
> > > class ActiveDOMObject {
> > >   ActiveDOMObject() {
> > >     registerEagerSweep(this);  // Register the pointer at runtime
> > >   }
> > > };
> > > 
> > > Then Oilpan keeps track of the registered pointers and call destructors of
> > those
> > > pointers first. However, I don't like this idea since it will introduce a
> lot
> > of
> > > complexity to the GC infrastructure. Pre-finalizers would be much simpler
> than
> > > this...
> > 
> > For mixins, this is fine. I'm not convinced (yet) you're describing a real
> > problem -- the current set of prefinalizers & other eagerly finalizable
> objects
> > don't run into this.
> 
> Just help me understand, how can Oilpan understand that it needs to allocate X
> on the eager heap if it is actually an instance of Y?

What? I'm not saying it does. Just that your hierarchy isn't something we
typically have (without involving mixins.)

[haraken, 2015-05-28 15:26:10]

On 2015/05/28 15:07:38, sof wrote:
> On 2015/05/28 15:04:25, haraken wrote:
> > On 2015/05/28 14:32:01, sof wrote:
> > > On 2015/05/28 14:20:24, haraken wrote:
> > > > On 2015/05/28 12:48:54, sof wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > > > File Source/core/events/EventTarget.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > > > Source/core/events/EventTarget.h:207: // EventTarget is not eagerly
> > > finalized.
> > > > > On 2015/05/28 12:30:04, haraken wrote:
> > > > > > 
> > > > > > Oh, does this mean that if we have:
> > > > > > 
> > > > > > class A : public GarbageCollected<A> { };
> > > > > > 
> > > > > > class B : public A {
> > > > > >   EAGERLY_FINALIZED();
> > > > > >   static B* create() {
> > > > > >     return new B;  // This won't be allocated on the eager heap
unless
> > we
> > > > > > override the operator 'new' in A?
> > > > > >   }
> > > > > > };
> > > > > > 
> > > > > > If that is the case, what happens if we have:
> > > > > > 
> > > > > > class X : public
> RefCountedGarbageCollectedEventTargetWithInlineData<X>
> > {
> > > };
> > > > > > 
> > > > > > class Y : public X {
> > > > > >   EAGERLY_FINALIZED();
> > > > > >   static Y* create() {
> > > > > >     return new Y;  // This won't be allocated on the eager heap
unless
> > we
> > > > > > override the operator 'new' in X?
> > > > > >   }
> > > > > > };
> > > > > 
> > > > > Yes, the eagerly finalized types have to have their "GC base" as
> > immediates
> > > &
> > > > > cannot inherit that (mixins excepted.)
> > > > 
> > > > Hmm, it seems we're hitting a problem (again) :)
> > > > 
> > > > What happens if we have:
> > > > 
> > > > class ActiveDOMObject {
> > > >   EAGERLY_FINALIZE();
> > > > };
> > > > class X : public GarbageCollected<X> { };
> > > > class Y : public X, public ActiveDOMObject<Y> { };  // How can this be
> > > allocated
> > > > on the eager heap?
> > > > 
> > > > This can happen in HTMLMdiaElement and so on. Hmm.
> > > > 
> > > > One (bad) idea I came up with is to write ActiveDOMObject as:
> > > > 
> > > > class ActiveDOMObject {
> > > >   ActiveDOMObject() {
> > > >     registerEagerSweep(this);  // Register the pointer at runtime
> > > >   }
> > > > };
> > > > 
> > > > Then Oilpan keeps track of the registered pointers and call destructors
of
> > > those
> > > > pointers first. However, I don't like this idea since it will introduce
a
> > lot
> > > of
> > > > complexity to the GC infrastructure. Pre-finalizers would be much
simpler
> > than
> > > > this...
> > > 
> > > For mixins, this is fine. I'm not convinced (yet) you're describing a real
> > > problem -- the current set of prefinalizers & other eagerly finalizable
> > objects
> > > don't run into this.
> > 
> > Just help me understand, how can Oilpan understand that it needs to allocate
X
> > on the eager heap if it is actually an instance of Y?
> 
> What? I'm not saying it does. Just that your hierarchy isn't something we
> typically have (without involving mixins.)

Yes, my question is how it works when mixins are involved.

> > > > class ActiveDOMObject {
> > > >   EAGERLY_FINALIZE();
> > > > };
> > > > class X : public GarbageCollected<X> { };
> > > > class Y : public X, public ActiveDOMObject<Y> { };  // How can this be
> > > allocated
> > > > on the eager heap?

For example, Y is HTMLMediaElement.

[sof, 2015-05-28 15:35:30]

On 2015/05/28 15:26:10, haraken wrote:
> On 2015/05/28 15:07:38, sof wrote:
> > On 2015/05/28 15:04:25, haraken wrote:
> > > On 2015/05/28 14:32:01, sof wrote:
> > > > On 2015/05/28 14:20:24, haraken wrote:
> > > > > On 2015/05/28 12:48:54, sof wrote:
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > > > > File Source/core/events/EventTarget.h (right):
> > > > > > 
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > > > > Source/core/events/EventTarget.h:207: // EventTarget is not eagerly
> > > > finalized.
> > > > > > On 2015/05/28 12:30:04, haraken wrote:
> > > > > > > 
> > > > > > > Oh, does this mean that if we have:
> > > > > > > 
> > > > > > > class A : public GarbageCollected<A> { };
> > > > > > > 
> > > > > > > class B : public A {
> > > > > > >   EAGERLY_FINALIZED();
> > > > > > >   static B* create() {
> > > > > > >     return new B;  // This won't be allocated on the eager heap
> unless
> > > we
> > > > > > > override the operator 'new' in A?
> > > > > > >   }
> > > > > > > };
> > > > > > > 
> > > > > > > If that is the case, what happens if we have:
> > > > > > > 
> > > > > > > class X : public
> > RefCountedGarbageCollectedEventTargetWithInlineData<X>
> > > {
> > > > };
> > > > > > > 
> > > > > > > class Y : public X {
> > > > > > >   EAGERLY_FINALIZED();
> > > > > > >   static Y* create() {
> > > > > > >     return new Y;  // This won't be allocated on the eager heap
> unless
> > > we
> > > > > > > override the operator 'new' in X?
> > > > > > >   }
> > > > > > > };
> > > > > > 
> > > > > > Yes, the eagerly finalized types have to have their "GC base" as
> > > immediates
> > > > &
> > > > > > cannot inherit that (mixins excepted.)
> > > > > 
> > > > > Hmm, it seems we're hitting a problem (again) :)
> > > > > 
> > > > > What happens if we have:
> > > > > 
> > > > > class ActiveDOMObject {
> > > > >   EAGERLY_FINALIZE();
> > > > > };
> > > > > class X : public GarbageCollected<X> { };
> > > > > class Y : public X, public ActiveDOMObject<Y> { };  // How can this be
> > > > allocated
> > > > > on the eager heap?
> > > > > 
> > > > > This can happen in HTMLMdiaElement and so on. Hmm.
> > > > > 
> > > > > One (bad) idea I came up with is to write ActiveDOMObject as:
> > > > > 
> > > > > class ActiveDOMObject {
> > > > >   ActiveDOMObject() {
> > > > >     registerEagerSweep(this);  // Register the pointer at runtime
> > > > >   }
> > > > > };
> > > > > 
> > > > > Then Oilpan keeps track of the registered pointers and call
destructors
> of
> > > > those
> > > > > pointers first. However, I don't like this idea since it will
introduce
> a
> > > lot
> > > > of
> > > > > complexity to the GC infrastructure. Pre-finalizers would be much
> simpler
> > > than
> > > > > this...
> > > > 
> > > > For mixins, this is fine. I'm not convinced (yet) you're describing a
real
> > > > problem -- the current set of prefinalizers & other eagerly finalizable
> > > objects
> > > > don't run into this.
> > > 
> > > Just help me understand, how can Oilpan understand that it needs to
allocate
> X
> > > on the eager heap if it is actually an instance of Y?
> > 
> > What? I'm not saying it does. Just that your hierarchy isn't something we
> > typically have (without involving mixins.)
> 
> Yes, my question is how it works when mixins are involved.
> 
> > > > > class ActiveDOMObject {
> > > > >   EAGERLY_FINALIZE();
> > > > > };
> > > > > class X : public GarbageCollected<X> { };
> > > > > class Y : public X, public ActiveDOMObject<Y> { };  // How can this be
> > > > allocated
> > > > > on the eager heap?
> 
> For example, Y is HTMLMediaElement.

I still don't understand the practical problem you outline. If you want to
annotate some object as eagerly finalizable, it'll have to be on the heap,
right? Then if you want to have it in that position for Y, wouldn't it have to
be a mixin? Where this mechanism works.

[sof, 2015-05-28 15:35:53]

On 2015/05/28 15:35:30, sof wrote:
> On 2015/05/28 15:26:10, haraken wrote:
> > On 2015/05/28 15:07:38, sof wrote:
> > > On 2015/05/28 15:04:25, haraken wrote:
> > > > On 2015/05/28 14:32:01, sof wrote:
> > > > > On 2015/05/28 14:20:24, haraken wrote:
> > > > > > On 2015/05/28 12:48:54, sof wrote:
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > > > > > File Source/core/events/EventTarget.h (right):
> > > > > > > 
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > > > > > Source/core/events/EventTarget.h:207: // EventTarget is not
eagerly
> > > > > finalized.
> > > > > > > On 2015/05/28 12:30:04, haraken wrote:
> > > > > > > > 
> > > > > > > > Oh, does this mean that if we have:
> > > > > > > > 
> > > > > > > > class A : public GarbageCollected<A> { };
> > > > > > > > 
> > > > > > > > class B : public A {
> > > > > > > >   EAGERLY_FINALIZED();
> > > > > > > >   static B* create() {
> > > > > > > >     return new B;  // This won't be allocated on the eager heap
> > unless
> > > > we
> > > > > > > > override the operator 'new' in A?
> > > > > > > >   }
> > > > > > > > };
> > > > > > > > 
> > > > > > > > If that is the case, what happens if we have:
> > > > > > > > 
> > > > > > > > class X : public
> > > RefCountedGarbageCollectedEventTargetWithInlineData<X>
> > > > {
> > > > > };
> > > > > > > > 
> > > > > > > > class Y : public X {
> > > > > > > >   EAGERLY_FINALIZED();
> > > > > > > >   static Y* create() {
> > > > > > > >     return new Y;  // This won't be allocated on the eager heap
> > unless
> > > > we
> > > > > > > > override the operator 'new' in X?
> > > > > > > >   }
> > > > > > > > };
> > > > > > > 
> > > > > > > Yes, the eagerly finalized types have to have their "GC base" as
> > > > immediates
> > > > > &
> > > > > > > cannot inherit that (mixins excepted.)
> > > > > > 
> > > > > > Hmm, it seems we're hitting a problem (again) :)
> > > > > > 
> > > > > > What happens if we have:
> > > > > > 
> > > > > > class ActiveDOMObject {
> > > > > >   EAGERLY_FINALIZE();
> > > > > > };
> > > > > > class X : public GarbageCollected<X> { };
> > > > > > class Y : public X, public ActiveDOMObject<Y> { };  // How can this
be
> > > > > allocated
> > > > > > on the eager heap?
> > > > > > 
> > > > > > This can happen in HTMLMdiaElement and so on. Hmm.
> > > > > > 
> > > > > > One (bad) idea I came up with is to write ActiveDOMObject as:
> > > > > > 
> > > > > > class ActiveDOMObject {
> > > > > >   ActiveDOMObject() {
> > > > > >     registerEagerSweep(this);  // Register the pointer at runtime
> > > > > >   }
> > > > > > };
> > > > > > 
> > > > > > Then Oilpan keeps track of the registered pointers and call
> destructors
> > of
> > > > > those
> > > > > > pointers first. However, I don't like this idea since it will
> introduce
> > a
> > > > lot
> > > > > of
> > > > > > complexity to the GC infrastructure. Pre-finalizers would be much
> > simpler
> > > > than
> > > > > > this...
> > > > > 
> > > > > For mixins, this is fine. I'm not convinced (yet) you're describing a
> real
> > > > > problem -- the current set of prefinalizers & other eagerly
finalizable
> > > > objects
> > > > > don't run into this.
> > > > 
> > > > Just help me understand, how can Oilpan understand that it needs to
> allocate
> > X
> > > > on the eager heap if it is actually an instance of Y?
> > > 
> > > What? I'm not saying it does. Just that your hierarchy isn't something we
> > > typically have (without involving mixins.)
> > 
> > Yes, my question is how it works when mixins are involved.
> > 
> > > > > > class ActiveDOMObject {
> > > > > >   EAGERLY_FINALIZE();
> > > > > > };
> > > > > > class X : public GarbageCollected<X> { };
> > > > > > class Y : public X, public ActiveDOMObject<Y> { };  // How can this
be
> > > > > allocated
> > > > > > on the eager heap?
> > 
> > For example, Y is HTMLMediaElement.
> 
> I still don't understand the practical problem you outline. If you want to
> annotate some object as eagerly finalizable, it'll have to be on the heap,
> right? Then if you want to have it in that position for Y, wouldn't it have to
> be a mixin? Where this mechanism works.

s/some object/some object class type like ActiveDOMObject/

[haraken, 2015-05-28 16:05:50]

On 2015/05/28 15:35:53, sof wrote:
> On 2015/05/28 15:35:30, sof wrote:
> > On 2015/05/28 15:26:10, haraken wrote:
> > > On 2015/05/28 15:07:38, sof wrote:
> > > > On 2015/05/28 15:04:25, haraken wrote:
> > > > > On 2015/05/28 14:32:01, sof wrote:
> > > > > > On 2015/05/28 14:20:24, haraken wrote:
> > > > > > > On 2015/05/28 12:48:54, sof wrote:
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > > > > > > File Source/core/events/EventTarget.h (right):
> > > > > > > > 
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > > > > > > Source/core/events/EventTarget.h:207: // EventTarget is not
> eagerly
> > > > > > finalized.
> > > > > > > > On 2015/05/28 12:30:04, haraken wrote:
> > > > > > > > > 
> > > > > > > > > Oh, does this mean that if we have:
> > > > > > > > > 
> > > > > > > > > class A : public GarbageCollected<A> { };
> > > > > > > > > 
> > > > > > > > > class B : public A {
> > > > > > > > >   EAGERLY_FINALIZED();
> > > > > > > > >   static B* create() {
> > > > > > > > >     return new B;  // This won't be allocated on the eager
heap
> > > unless
> > > > > we
> > > > > > > > > override the operator 'new' in A?
> > > > > > > > >   }
> > > > > > > > > };
> > > > > > > > > 
> > > > > > > > > If that is the case, what happens if we have:
> > > > > > > > > 
> > > > > > > > > class X : public
> > > > RefCountedGarbageCollectedEventTargetWithInlineData<X>
> > > > > {
> > > > > > };
> > > > > > > > > 
> > > > > > > > > class Y : public X {
> > > > > > > > >   EAGERLY_FINALIZED();
> > > > > > > > >   static Y* create() {
> > > > > > > > >     return new Y;  // This won't be allocated on the eager
heap
> > > unless
> > > > > we
> > > > > > > > > override the operator 'new' in X?
> > > > > > > > >   }
> > > > > > > > > };
> > > > > > > > 
> > > > > > > > Yes, the eagerly finalized types have to have their "GC base" as
> > > > > immediates
> > > > > > &
> > > > > > > > cannot inherit that (mixins excepted.)
> > > > > > > 
> > > > > > > Hmm, it seems we're hitting a problem (again) :)
> > > > > > > 
> > > > > > > What happens if we have:
> > > > > > > 
> > > > > > > class ActiveDOMObject {
> > > > > > >   EAGERLY_FINALIZE();
> > > > > > > };
> > > > > > > class X : public GarbageCollected<X> { };
> > > > > > > class Y : public X, public ActiveDOMObject<Y> { };  // How can
this
> be
> > > > > > allocated
> > > > > > > on the eager heap?
> > > > > > > 
> > > > > > > This can happen in HTMLMdiaElement and so on. Hmm.
> > > > > > > 
> > > > > > > One (bad) idea I came up with is to write ActiveDOMObject as:
> > > > > > > 
> > > > > > > class ActiveDOMObject {
> > > > > > >   ActiveDOMObject() {
> > > > > > >     registerEagerSweep(this);  // Register the pointer at runtime
> > > > > > >   }
> > > > > > > };
> > > > > > > 
> > > > > > > Then Oilpan keeps track of the registered pointers and call
> > destructors
> > > of
> > > > > > those
> > > > > > > pointers first. However, I don't like this idea since it will
> > introduce
> > > a
> > > > > lot
> > > > > > of
> > > > > > > complexity to the GC infrastructure. Pre-finalizers would be much
> > > simpler
> > > > > than
> > > > > > > this...
> > > > > > 
> > > > > > For mixins, this is fine. I'm not convinced (yet) you're describing
a
> > real
> > > > > > problem -- the current set of prefinalizers & other eagerly
> finalizable
> > > > > objects
> > > > > > don't run into this.
> > > > > 
> > > > > Just help me understand, how can Oilpan understand that it needs to
> > allocate
> > > X
> > > > > on the eager heap if it is actually an instance of Y?
> > > > 
> > > > What? I'm not saying it does. Just that your hierarchy isn't something
we
> > > > typically have (without involving mixins.)
> > > 
> > > Yes, my question is how it works when mixins are involved.
> > > 
> > > > > > > class ActiveDOMObject {
> > > > > > >   EAGERLY_FINALIZE();
> > > > > > > };
> > > > > > > class X : public GarbageCollected<X> { };
> > > > > > > class Y : public X, public ActiveDOMObject<Y> { };  // How can
this
> be
> > > > > > allocated
> > > > > > > on the eager heap?
> > > 
> > > For example, Y is HTMLMediaElement.
> > 
> > I still don't understand the practical problem you outline. If you want to
> > annotate some object as eagerly finalizable, it'll have to be on the heap,
> > right? Then if you want to have it in that position for Y, wouldn't it have
to
> > be a mixin? Where this mechanism works.
> 
> s/some object/some object class type like ActiveDOMObject/

Ah, I was missing the fact that ActiveDOMObject is still off-heap. Makes sense.

So maybe I'm describing a problem that doesn't exist but it will cause a problem
if we have:

class Z : public GarbageCollectedMixin {
  EAGERLY_FINALIZE();  // I want to put this mixin object on the eager heap.
};
class X : public GarbageCollected<X> { };
class Y : public X, public Z { };  // Y won't be allocated on the eager heap,
right?

[sof, 2015-05-28 16:16:13]

On 2015/05/28 16:05:50, haraken wrote:
> On 2015/05/28 15:35:53, sof wrote:
> > On 2015/05/28 15:35:30, sof wrote:
> > > On 2015/05/28 15:26:10, haraken wrote:
> > > > On 2015/05/28 15:07:38, sof wrote:
> > > > > On 2015/05/28 15:04:25, haraken wrote:
> > > > > > On 2015/05/28 14:32:01, sof wrote:
> > > > > > > On 2015/05/28 14:20:24, haraken wrote:
> > > > > > > > On 2015/05/28 12:48:54, sof wrote:
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > > > > > > > File Source/core/events/EventTarget.h (right):
> > > > > > > > > 
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > > > > > > > Source/core/events/EventTarget.h:207: // EventTarget is not
> > eagerly
> > > > > > > finalized.
> > > > > > > > > On 2015/05/28 12:30:04, haraken wrote:
> > > > > > > > > > 
> > > > > > > > > > Oh, does this mean that if we have:
> > > > > > > > > > 
> > > > > > > > > > class A : public GarbageCollected<A> { };
> > > > > > > > > > 
> > > > > > > > > > class B : public A {
> > > > > > > > > >   EAGERLY_FINALIZED();
> > > > > > > > > >   static B* create() {
> > > > > > > > > >     return new B;  // This won't be allocated on the eager
> heap
> > > > unless
> > > > > > we
> > > > > > > > > > override the operator 'new' in A?
> > > > > > > > > >   }
> > > > > > > > > > };
> > > > > > > > > > 
> > > > > > > > > > If that is the case, what happens if we have:
> > > > > > > > > > 
> > > > > > > > > > class X : public
> > > > > RefCountedGarbageCollectedEventTargetWithInlineData<X>
> > > > > > {
> > > > > > > };
> > > > > > > > > > 
> > > > > > > > > > class Y : public X {
> > > > > > > > > >   EAGERLY_FINALIZED();
> > > > > > > > > >   static Y* create() {
> > > > > > > > > >     return new Y;  // This won't be allocated on the eager
> heap
> > > > unless
> > > > > > we
> > > > > > > > > > override the operator 'new' in X?
> > > > > > > > > >   }
> > > > > > > > > > };
> > > > > > > > > 
> > > > > > > > > Yes, the eagerly finalized types have to have their "GC base"
as
> > > > > > immediates
> > > > > > > &
> > > > > > > > > cannot inherit that (mixins excepted.)
> > > > > > > > 
> > > > > > > > Hmm, it seems we're hitting a problem (again) :)
> > > > > > > > 
> > > > > > > > What happens if we have:
> > > > > > > > 
> > > > > > > > class ActiveDOMObject {
> > > > > > > >   EAGERLY_FINALIZE();
> > > > > > > > };
> > > > > > > > class X : public GarbageCollected<X> { };
> > > > > > > > class Y : public X, public ActiveDOMObject<Y> { };  // How can
> this
> > be
> > > > > > > allocated
> > > > > > > > on the eager heap?
> > > > > > > > 
> > > > > > > > This can happen in HTMLMdiaElement and so on. Hmm.
> > > > > > > > 
> > > > > > > > One (bad) idea I came up with is to write ActiveDOMObject as:
> > > > > > > > 
> > > > > > > > class ActiveDOMObject {
> > > > > > > >   ActiveDOMObject() {
> > > > > > > >     registerEagerSweep(this);  // Register the pointer at
runtime
> > > > > > > >   }
> > > > > > > > };
> > > > > > > > 
> > > > > > > > Then Oilpan keeps track of the registered pointers and call
> > > destructors
> > > > of
> > > > > > > those
> > > > > > > > pointers first. However, I don't like this idea since it will
> > > introduce
> > > > a
> > > > > > lot
> > > > > > > of
> > > > > > > > complexity to the GC infrastructure. Pre-finalizers would be
much
> > > > simpler
> > > > > > than
> > > > > > > > this...
> > > > > > > 
> > > > > > > For mixins, this is fine. I'm not convinced (yet) you're
describing
> a
> > > real
> > > > > > > problem -- the current set of prefinalizers & other eagerly
> > finalizable
> > > > > > objects
> > > > > > > don't run into this.
> > > > > > 
> > > > > > Just help me understand, how can Oilpan understand that it needs to
> > > allocate
> > > > X
> > > > > > on the eager heap if it is actually an instance of Y?
> > > > > 
> > > > > What? I'm not saying it does. Just that your hierarchy isn't something
> we
> > > > > typically have (without involving mixins.)
> > > > 
> > > > Yes, my question is how it works when mixins are involved.
> > > > 
> > > > > > > > class ActiveDOMObject {
> > > > > > > >   EAGERLY_FINALIZE();
> > > > > > > > };
> > > > > > > > class X : public GarbageCollected<X> { };
> > > > > > > > class Y : public X, public ActiveDOMObject<Y> { };  // How can
> this
> > be
> > > > > > > allocated
> > > > > > > > on the eager heap?
> > > > 
> > > > For example, Y is HTMLMediaElement.
> > > 
> > > I still don't understand the practical problem you outline. If you want to
> > > annotate some object as eagerly finalizable, it'll have to be on the heap,
> > > right? Then if you want to have it in that position for Y, wouldn't it
have
> to
> > > be a mixin? Where this mechanism works.
> > 
> > s/some object/some object class type like ActiveDOMObject/
> 
> Ah, I was missing the fact that ActiveDOMObject is still off-heap. Makes
sense.
> 
> So maybe I'm describing a problem that doesn't exist but it will cause a
problem
> if we have:
> 
> class Z : public GarbageCollectedMixin {
>   EAGERLY_FINALIZE();  // I want to put this mixin object on the eager heap.
> };
> class X : public GarbageCollected<X> { };
> class Y : public X, public Z { };  // Y won't be allocated on the eager heap,
> right?

It will be, see the GarbageCollected.h change.

[sof, 2015-05-28 21:36:34]

On 2015/05/28 16:16:13, sof wrote:
> On 2015/05/28 16:05:50, haraken wrote:
> > On 2015/05/28 15:35:53, sof wrote:
> > > On 2015/05/28 15:35:30, sof wrote:
> > > > On 2015/05/28 15:26:10, haraken wrote:
> > > > > On 2015/05/28 15:07:38, sof wrote:
> > > > > > On 2015/05/28 15:04:25, haraken wrote:
> > > > > > > On 2015/05/28 14:32:01, sof wrote:
> > > > > > > > On 2015/05/28 14:20:24, haraken wrote:
> > > > > > > > > On 2015/05/28 12:48:54, sof wrote:
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > > > > > > > > File Source/core/events/EventTarget.h (right):
> > > > > > > > > > 
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1157933002/diff/40001/Source/core/events/Even...
> > > > > > > > > > Source/core/events/EventTarget.h:207: // EventTarget is not
> > > eagerly
> > > > > > > > finalized.
> > > > > > > > > > On 2015/05/28 12:30:04, haraken wrote:
> > > > > > > > > > > 
> > > > > > > > > > > Oh, does this mean that if we have:
> > > > > > > > > > > 
> > > > > > > > > > > class A : public GarbageCollected<A> { };
> > > > > > > > > > > 
> > > > > > > > > > > class B : public A {
> > > > > > > > > > >   EAGERLY_FINALIZED();
> > > > > > > > > > >   static B* create() {
> > > > > > > > > > >     return new B;  // This won't be allocated on the eager
> > heap
> > > > > unless
> > > > > > > we
> > > > > > > > > > > override the operator 'new' in A?
> > > > > > > > > > >   }
> > > > > > > > > > > };
> > > > > > > > > > > 
> > > > > > > > > > > If that is the case, what happens if we have:
> > > > > > > > > > > 
> > > > > > > > > > > class X : public
> > > > > > RefCountedGarbageCollectedEventTargetWithInlineData<X>
> > > > > > > {
> > > > > > > > };
> > > > > > > > > > > 
> > > > > > > > > > > class Y : public X {
> > > > > > > > > > >   EAGERLY_FINALIZED();
> > > > > > > > > > >   static Y* create() {
> > > > > > > > > > >     return new Y;  // This won't be allocated on the eager
> > heap
> > > > > unless
> > > > > > > we
> > > > > > > > > > > override the operator 'new' in X?
> > > > > > > > > > >   }
> > > > > > > > > > > };
> > > > > > > > > > 
> > > > > > > > > > Yes, the eagerly finalized types have to have their "GC
base"
> as
> > > > > > > immediates
> > > > > > > > &
> > > > > > > > > > cannot inherit that (mixins excepted.)
> > > > > > > > > 
> > > > > > > > > Hmm, it seems we're hitting a problem (again) :)
> > > > > > > > > 
> > > > > > > > > What happens if we have:
> > > > > > > > > 
> > > > > > > > > class ActiveDOMObject {
> > > > > > > > >   EAGERLY_FINALIZE();
> > > > > > > > > };
> > > > > > > > > class X : public GarbageCollected<X> { };
> > > > > > > > > class Y : public X, public ActiveDOMObject<Y> { };  // How can
> > this
> > > be
> > > > > > > > allocated
> > > > > > > > > on the eager heap?
> > > > > > > > > 
> > > > > > > > > This can happen in HTMLMdiaElement and so on. Hmm.
> > > > > > > > > 
> > > > > > > > > One (bad) idea I came up with is to write ActiveDOMObject as:
> > > > > > > > > 
> > > > > > > > > class ActiveDOMObject {
> > > > > > > > >   ActiveDOMObject() {
> > > > > > > > >     registerEagerSweep(this);  // Register the pointer at
> runtime
> > > > > > > > >   }
> > > > > > > > > };
> > > > > > > > > 
> > > > > > > > > Then Oilpan keeps track of the registered pointers and call
> > > > destructors
> > > > > of
> > > > > > > > those
> > > > > > > > > pointers first. However, I don't like this idea since it will
> > > > introduce
> > > > > a
> > > > > > > lot
> > > > > > > > of
> > > > > > > > > complexity to the GC infrastructure. Pre-finalizers would be
> much
> > > > > simpler
> > > > > > > than
> > > > > > > > > this...
> > > > > > > > 
> > > > > > > > For mixins, this is fine. I'm not convinced (yet) you're
> describing
> > a
> > > > real
> > > > > > > > problem -- the current set of prefinalizers & other eagerly
> > > finalizable
> > > > > > > objects
> > > > > > > > don't run into this.
> > > > > > > 
> > > > > > > Just help me understand, how can Oilpan understand that it needs
to
> > > > allocate
> > > > > X
> > > > > > > on the eager heap if it is actually an instance of Y?
> > > > > > 
> > > > > > What? I'm not saying it does. Just that your hierarchy isn't
something
> > we
> > > > > > typically have (without involving mixins.)
> > > > > 
> > > > > Yes, my question is how it works when mixins are involved.
> > > > > 
> > > > > > > > > class ActiveDOMObject {
> > > > > > > > >   EAGERLY_FINALIZE();
> > > > > > > > > };
> > > > > > > > > class X : public GarbageCollected<X> { };
> > > > > > > > > class Y : public X, public ActiveDOMObject<Y> { };  // How can
> > this
> > > be
> > > > > > > > allocated
> > > > > > > > > on the eager heap?
> > > > > 
> > > > > For example, Y is HTMLMediaElement.
> > > > 
> > > > I still don't understand the practical problem you outline. If you want
to
> > > > annotate some object as eagerly finalizable, it'll have to be on the
heap,
> > > > right? Then if you want to have it in that position for Y, wouldn't it
> have
> > to
> > > > be a mixin? Where this mechanism works.
> > > 
> > > s/some object/some object class type like ActiveDOMObject/
> > 
> > Ah, I was missing the fact that ActiveDOMObject is still off-heap. Makes
> sense.
> > 
> > So maybe I'm describing a problem that doesn't exist but it will cause a
> problem
> > if we have:
> > 
> > class Z : public GarbageCollectedMixin {
> >   EAGERLY_FINALIZE();  // I want to put this mixin object on the eager heap.
> > };
> > class X : public GarbageCollected<X> { };
> > class Y : public X, public Z { };  // Y won't be allocated on the eager
heap,
> > right?
> 
> It will be, see the GarbageCollected.h change.

For an example that doesn't behave, if you add EAGERLY_FINALIZE(); to
HTMLElement, that will not extend to subclasses (unless they happen to be
mixins).

I'd prefer it if we could statically catch such attempts, the rule would have to
be something like (for a class T):

 - if its leftmost base is an eagerly finalized class, then the derived class T
would have to be declared eagerly finalizable also (or be a mixin.)

That ought to be checkable by the GC plugin, but I don't see how to encode it
via declarations & static asserts on the class.

[sof, 2015-05-29 21:25:09]

sorry about the delay in following up.

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:341: void BaseHeap::poisonHeap(bool setPoison)
On 2015/05/28 12:30:04, haraken wrote:
> 
> poisonHeap => poisonEagerHeap
> 
> I'd use SetPoison/ClearPoison instead of bool.

Moved enums around to make that possible.

Keeping it as poisonHeap(), for reasons mentioned below.

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:348: for (BasePage* page = m_firstUnsweptPage;
page; page = page->next()) {
On 2015/05/28 12:30:04, haraken wrote:
> 
> Add ASSERT(!m_firstPage).

Done.

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:349:
page->poisonObjects(BasePage::UnmarkedOrMarked, BasePage::SetPoison);
On 2015/05/28 12:30:04, haraken wrote:
> 
> Why do we need to poison marked objects? I'm ok with this, but I'm wondering
why
> we need it because we're not poisoning marked objects for other heaps.

You're not allowed to touch other eagerly finalized objects. By poisoning the
live objects also, we catch not only accesses to other eagerly finalized objects
that happen to have been finalized already, but all.

This wouldn't work for lazy sweeping, as the mutator gains access to the live
objects earlier, but I believe we can do something similar for 'normal' complete
sweeps of the other heaps also. That's why I would prefer to keep this named
poisonHeap().

(This follows up the suggestion made in
https://codereview.chromium.org/1159583002/#msg14 )

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:354: page->poisonObjects(BasePage::UnmarkedOnly,
BasePage::ClearPoison);
On 2015/05/28 12:30:04, haraken wrote:
> 
> Who unpoisons the marked objects that have been poisoned at line 349?

This very loop when poisonHeap() is called on the eager heap afterwards to clear
the poisoning. The live objects have all been unmarked at this stage.

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/1157933002/diff/40001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:925: poisonHeapsForCompleteSweep();
On 2015/05/28 12:30:04, haraken wrote:
> 
> poisonEagerHeaps => poisonEagerHeap
> poisonHeapsForCompleteSweep => poisonAllHeaps
> 
> ?

Done.

[sof, 2015-05-31 12:25:46]

friendly ping.

need this one to move ahead on a couple of other contributions.

[haraken, 2015-06-01 05:08:07]

On 2015/05/31 12:25:46, sof wrote:
> friendly ping.
> 
> need this one to move ahead on a couple of other contributions.

(Sorry for the delay. Will take a look in hours.)

[haraken, 2015-06-01 08:42:52]

LGTM

https://codereview.chromium.org/1157933002/diff/60001/Source/core/events/Even...
File Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1157933002/diff/60001/Source/core/events/Even...
Source/core/events/EventTarget.h:202: void* operator new(size_t size)

I'm OK with this approach, but it's error-prone that we sometimes have to
override 'new' to allocate objects on the eager sweep heap as intended. We
should have a verification system for this.

- One way to do this would be to use a GC plugin, as you suggested.

- Another way would be to do the verification at runtime, like this:

#define EAGERLY_FINALIZE() \
  EagerlySweptDestructor m_destructor;

class EagerlySweptDestructor {
  ~EagerlySweptDestructor() {
    ASSERT(isSweepingEagerHeap());
  }
};

- Having both verifications sounds much better :)

https://codereview.chromium.org/1157933002/diff/60001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1157933002/diff/60001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:353: void BaseHeap::poisonHeap(Poisoning
poisoning)

I'd call this function poisonUnmarkedAndMarkedObjects, in consistency with
poisonUnmarkedObjects().

https://codereview.chromium.org/1157933002/diff/60001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:369: page->poisonObjects(UnmarkedOnly,
ClearPoison);

Nit: If we call the function poisonUnmarkedAndMarkedObjects, I'd use
UnmarkedOrMarked instead of UnmarkedOnly here. There would be no behavioral
difference though.

https://codereview.chromium.org/1157933002/diff/60001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:805: ASSERT(heapIndex() != EagerSweepHeapIndex);

I'd use RELEASE_ASSERT, since if we hit this, we'll have a real bug.

[haraken, 2015-06-01 09:12:58]

https://codereview.chromium.org/1157933002/diff/60001/Source/core/events/Even...
File Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1157933002/diff/60001/Source/core/events/Even...
Source/core/events/EventTarget.h:202: void* operator new(size_t size)
On 2015/06/01 08:42:52, haraken wrote:
> 
> I'm OK with this approach, but it's error-prone that we sometimes have to
> override 'new' to allocate objects on the eager sweep heap as intended. We
> should have a verification system for this.
> 
> - One way to do this would be to use a GC plugin, as you suggested.
> 
> - Another way would be to do the verification at runtime, like this:
> 
> #define EAGERLY_FINALIZE() \
>   EagerlySweptDestructor m_destructor;
> 
> class EagerlySweptDestructor {
>   ~EagerlySweptDestructor() {
>     ASSERT(isSweepingEagerHeap());
>   }
> };
> 
> - Having both verifications sounds much better :)

You can implement the verirication(s) in a follow-up even if you want.

[sof, 2015-06-01 14:51:36]

https://codereview.chromium.org/1157933002/diff/60001/Source/core/events/Even...
File Source/core/events/EventTarget.h (right):

https://codereview.chromium.org/1157933002/diff/60001/Source/core/events/Even...
Source/core/events/EventTarget.h:202: void* operator new(size_t size)
On 2015/06/01 09:12:58, haraken wrote:
> On 2015/06/01 08:42:52, haraken wrote:
> > 
> > I'm OK with this approach, but it's error-prone that we sometimes have to
> > override 'new' to allocate objects on the eager sweep heap as intended. We
> > should have a verification system for this.
> > 
> > - One way to do this would be to use a GC plugin, as you suggested.
> > 
> > - Another way would be to do the verification at runtime, like this:
> > 
> > #define EAGERLY_FINALIZE() \
> >   EagerlySweptDestructor m_destructor;
> > 
> > class EagerlySweptDestructor {
> >   ~EagerlySweptDestructor() {
> >     ASSERT(isSweepingEagerHeap());
> >   }
> > };
> > 
> > - Having both verifications sounds much better :)
> 
> You can implement the verirication(s) in a follow-up even if you want.

Will followup, at least for the latter one.

https://codereview.chromium.org/1157933002/diff/60001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/1157933002/diff/60001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:353: void BaseHeap::poisonHeap(Poisoning
poisoning)
On 2015/06/01 08:42:52, haraken wrote:
> 
> I'd call this function poisonUnmarkedAndMarkedObjects, in consistency with
> poisonUnmarkedObjects().

Addressed (see below.)

https://codereview.chromium.org/1157933002/diff/60001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:369: page->poisonObjects(UnmarkedOnly,
ClearPoison);
On 2015/06/01 08:42:52, haraken wrote:
> 
> Nit: If we call the function poisonUnmarkedAndMarkedObjects, I'd use
> UnmarkedOrMarked instead of UnmarkedOnly here. There would be no behavioral
> difference though.
> 

There wouldn't, but that observation made me realize that we could simplify the
exposed methods. i.e., we can instead parameterize over ObjectsToPoison and only
provide one method; I've kept that named as poisonHeap().

https://codereview.chromium.org/1157933002/diff/60001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:805: ASSERT(heapIndex() != EagerSweepHeapIndex);
On 2015/06/01 08:42:52, haraken wrote:
> 
> I'd use RELEASE_ASSERT, since if we hit this, we'll have a real bug.

switched; to the extent there's overhead, this isn't on any allocation hot path.

[sof, 2015-06-01 14:53:10]

thanks for the review & suggestions surrounding finalization; let's try to move
ahead & land..

[sof, 2015-06-01 15:17:04]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-06-01 15:17:04]

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/1157933002/#ps80001
(title: "Parameterize Heap::poisonHeap() over ObjectsToPoison")

[commit-bot: I haz the power, 2015-06-01 15:17:19]

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

[commit-bot: I haz the power, 2015-06-01 15:46:04]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196232