Oilpan: insist on eager sweeping for some objects.

Oilpan: insist on eager sweeping for some objects.

To prepare for enabling lazy sweeping, non-Oilpan - add EAGERLY_SWEEP()
annotations for LifecycleObserver and DOMWindowProperty.

The former is a template, so extend range of EAGERLY_SWEEP() macros
to handle that also, along with ways to handle ambiguity.

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

[sof, 2015-05-21 10:10:08]

please take a look.

[sof, 2015-05-21 10:41:53]

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

[haraken, 2015-05-21 11:45:38]

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

[haraken, 2015-05-21 11:45:39]

https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h...
Source/platform/heap/Heap.h:1145: // for Blink's DOMWindowProperty -- provide a
convenience

Just to confirm: If we write:

  EAGERLY_SWEEP_TEMPLATE(LifecycleObserver);
  EAGERLY_SWEEP(DOMWindowProperty);

then we hit the disambiguation error when encountering a class that is both
LifecycleObserver and DOMWindowProperty?

[sof, 2015-05-21 11:47:50]

https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h...
Source/platform/heap/Heap.h:1145: // for Blink's DOMWindowProperty -- provide a
convenience
On 2015/05/21 11:45:39, haraken wrote:
> 
> Just to confirm: If we write:
> 
>   EAGERLY_SWEEP_TEMPLATE(LifecycleObserver);
>   EAGERLY_SWEEP(DOMWindowProperty);
> 
> then we hit the disambiguation error when encountering a class that is both
> LifecycleObserver and DOMWindowProperty?

Correct.

[haraken, 2015-05-21 13:23:24]

https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h...
Source/platform/heap/Heap.h:1145: // for Blink's DOMWindowProperty -- provide a
convenience
On 2015/05/21 11:47:50, sof wrote:
> On 2015/05/21 11:45:39, haraken wrote:
> > 
> > Just to confirm: If we write:
> > 
> >   EAGERLY_SWEEP_TEMPLATE(LifecycleObserver);
> >   EAGERLY_SWEEP(DOMWindowProperty);
> > 
> > then we hit the disambiguation error when encountering a class that is both
> > LifecycleObserver and DOMWindowProperty?
> 
> Correct.

Makes sense.

I'm OK with this but would slightly prefer hard-coding the things instead of
introducing new macros, for clarity.

  // TODO: These are layering violations. Remove this once
  // the DOMWindowProperty and LifecycleObserver are moved to
  // Oilpan by default.
  class DOMWindowProperty;
  class LifecycleObserver;
  template<typename T>
  class HeapIndexTrait<T, typename WTF::EnableIf<WTF::IsSubclassOfTemplate<T,
LifecycleObserver>::value || WTF::IsSubclass<T,
DOMWindowProperty>::value>::Type> {
    ...;
  };

What do you think?

[sof, 2015-05-21 13:28:11]

On 2015/05/21 13:23:24, haraken wrote:
> https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h
> File Source/platform/heap/Heap.h (right):
> 
>
https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h...
> Source/platform/heap/Heap.h:1145: // for Blink's DOMWindowProperty -- provide
a
> convenience
> On 2015/05/21 11:47:50, sof wrote:
> > On 2015/05/21 11:45:39, haraken wrote:
> > > 
> > > Just to confirm: If we write:
> > > 
> > >   EAGERLY_SWEEP_TEMPLATE(LifecycleObserver);
> > >   EAGERLY_SWEEP(DOMWindowProperty);
> > > 
> > > then we hit the disambiguation error when encountering a class that is
both
> > > LifecycleObserver and DOMWindowProperty?
> > 
> > Correct.
> 
> Makes sense.
> 
> I'm OK with this but would slightly prefer hard-coding the things instead of
> introducing new macros, for clarity.
> 
>   // TODO: These are layering violations. Remove this once
>   // the DOMWindowProperty and LifecycleObserver are moved to
>   // Oilpan by default.
>   class DOMWindowProperty;
>   class LifecycleObserver;
>   template<typename T>
>   class HeapIndexTrait<T, typename WTF::EnableIf<WTF::IsSubclassOfTemplate<T,
> LifecycleObserver>::value || WTF::IsSubclass<T,
> DOMWindowProperty>::value>::Type> {
>     ...;
>   };
> 
> What do you think?

I would not prefer hardcoding something like that.

An alternative is to define the trait directly in DOMWindowProperty.h, at the
cost of exposing some Oilpan near-internal details there.

[haraken, 2015-05-21 13:42:31]

On 2015/05/21 13:28:11, sof wrote:
> On 2015/05/21 13:23:24, haraken wrote:
> >
https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h
> > File Source/platform/heap/Heap.h (right):
> > 
> >
>
https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h...
> > Source/platform/heap/Heap.h:1145: // for Blink's DOMWindowProperty --
provide
> a
> > convenience
> > On 2015/05/21 11:47:50, sof wrote:
> > > On 2015/05/21 11:45:39, haraken wrote:
> > > > 
> > > > Just to confirm: If we write:
> > > > 
> > > >   EAGERLY_SWEEP_TEMPLATE(LifecycleObserver);
> > > >   EAGERLY_SWEEP(DOMWindowProperty);
> > > > 
> > > > then we hit the disambiguation error when encountering a class that is
> both
> > > > LifecycleObserver and DOMWindowProperty?
> > > 
> > > Correct.
> > 
> > Makes sense.
> > 
> > I'm OK with this but would slightly prefer hard-coding the things instead of
> > introducing new macros, for clarity.
> > 
> >   // TODO: These are layering violations. Remove this once
> >   // the DOMWindowProperty and LifecycleObserver are moved to
> >   // Oilpan by default.
> >   class DOMWindowProperty;
> >   class LifecycleObserver;
> >   template<typename T>
> >   class HeapIndexTrait<T, typename
WTF::EnableIf<WTF::IsSubclassOfTemplate<T,
> > LifecycleObserver>::value || WTF::IsSubclass<T,
> > DOMWindowProperty>::value>::Type> {
> >     ...;
> >   };
> > 
> > What do you think?
> 
> I would not prefer hardcoding something like that.
> 
> An alternative is to define the trait directly in DOMWindowProperty.h, at the
> cost of exposing some Oilpan near-internal details there.

Maybe would it be simpler to introduce a EagerlySwept class instead of
introcuding various kinds of macros?

  class LifecycleObserver : public EagerlySwept { };
  class DOMWindowProperty : public EagerlySwept { };

Then we can use IsSubClass<T, EagerlySwept>.

===
BTW, sorry, I'm a bit confused: What's a difference between using eager sweeping
for LifecycleObserver and using a pre-finalizer for LifecycleObserver? I now
remember that we had been using a pre-finalizer for LifecycleObserver to clear
out the raw pointers until we moved LifecycleObserver to the heap...

[sof, 2015-05-21 13:45:41]

On 2015/05/21 13:42:31, haraken wrote:
> On 2015/05/21 13:28:11, sof wrote:
> > On 2015/05/21 13:23:24, haraken wrote:
> > >
> https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h
> > > File Source/platform/heap/Heap.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h...
> > > Source/platform/heap/Heap.h:1145: // for Blink's DOMWindowProperty --
> provide
> > a
> > > convenience
> > > On 2015/05/21 11:47:50, sof wrote:
> > > > On 2015/05/21 11:45:39, haraken wrote:
> > > > > 
> > > > > Just to confirm: If we write:
> > > > > 
> > > > >   EAGERLY_SWEEP_TEMPLATE(LifecycleObserver);
> > > > >   EAGERLY_SWEEP(DOMWindowProperty);
> > > > > 
> > > > > then we hit the disambiguation error when encountering a class that is
> > both
> > > > > LifecycleObserver and DOMWindowProperty?
> > > > 
> > > > Correct.
> > > 
> > > Makes sense.
> > > 
> > > I'm OK with this but would slightly prefer hard-coding the things instead
of
> > > introducing new macros, for clarity.
> > > 
> > >   // TODO: These are layering violations. Remove this once
> > >   // the DOMWindowProperty and LifecycleObserver are moved to
> > >   // Oilpan by default.
> > >   class DOMWindowProperty;
> > >   class LifecycleObserver;
> > >   template<typename T>
> > >   class HeapIndexTrait<T, typename
> WTF::EnableIf<WTF::IsSubclassOfTemplate<T,
> > > LifecycleObserver>::value || WTF::IsSubclass<T,
> > > DOMWindowProperty>::value>::Type> {
> > >     ...;
> > >   };
> > > 
> > > What do you think?
> > 
> > I would not prefer hardcoding something like that.
> > 
> > An alternative is to define the trait directly in DOMWindowProperty.h, at
the
> > cost of exposing some Oilpan near-internal details there.
> 
> Maybe would it be simpler to introduce a EagerlySwept class instead of
> introcuding various kinds of macros?
> 
>   class LifecycleObserver : public EagerlySwept { };
>   class DOMWindowProperty : public EagerlySwept { };
> 
> Then we can use IsSubClass<T, EagerlySwept>.
> 
> ===
> BTW, sorry, I'm a bit confused: What's a difference between using eager
sweeping
> for LifecycleObserver and using a pre-finalizer for LifecycleObserver? I now
> remember that we had been using a pre-finalizer for LifecycleObserver to clear
> out the raw pointers until we moved LifecycleObserver to the heap...

This all a temporary measure.

Prefinalizers don't nest is one issue.

[haraken, 2015-05-21 14:28:46]

On 2015/05/21 13:45:41, sof wrote:
> On 2015/05/21 13:42:31, haraken wrote:
> > On 2015/05/21 13:28:11, sof wrote:
> > > On 2015/05/21 13:23:24, haraken wrote:
> > > >
> >
https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h
> > > > File Source/platform/heap/Heap.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h...
> > > > Source/platform/heap/Heap.h:1145: // for Blink's DOMWindowProperty --
> > provide
> > > a
> > > > convenience
> > > > On 2015/05/21 11:47:50, sof wrote:
> > > > > On 2015/05/21 11:45:39, haraken wrote:
> > > > > > 
> > > > > > Just to confirm: If we write:
> > > > > > 
> > > > > >   EAGERLY_SWEEP_TEMPLATE(LifecycleObserver);
> > > > > >   EAGERLY_SWEEP(DOMWindowProperty);
> > > > > > 
> > > > > > then we hit the disambiguation error when encountering a class that
is
> > > both
> > > > > > LifecycleObserver and DOMWindowProperty?
> > > > > 
> > > > > Correct.
> > > > 
> > > > Makes sense.
> > > > 
> > > > I'm OK with this but would slightly prefer hard-coding the things
instead
> of
> > > > introducing new macros, for clarity.
> > > > 
> > > >   // TODO: These are layering violations. Remove this once
> > > >   // the DOMWindowProperty and LifecycleObserver are moved to
> > > >   // Oilpan by default.
> > > >   class DOMWindowProperty;
> > > >   class LifecycleObserver;
> > > >   template<typename T>
> > > >   class HeapIndexTrait<T, typename
> > WTF::EnableIf<WTF::IsSubclassOfTemplate<T,
> > > > LifecycleObserver>::value || WTF::IsSubclass<T,
> > > > DOMWindowProperty>::value>::Type> {
> > > >     ...;
> > > >   };
> > > > 
> > > > What do you think?
> > > 
> > > I would not prefer hardcoding something like that.
> > > 
> > > An alternative is to define the trait directly in DOMWindowProperty.h, at
> the
> > > cost of exposing some Oilpan near-internal details there.
> > 
> > Maybe would it be simpler to introduce a EagerlySwept class instead of
> > introcuding various kinds of macros?
> > 
> >   class LifecycleObserver : public EagerlySwept { };
> >   class DOMWindowProperty : public EagerlySwept { };
> > 
> > Then we can use IsSubClass<T, EagerlySwept>.
> > 
> > ===
> > BTW, sorry, I'm a bit confused: What's a difference between using eager
> sweeping
> > for LifecycleObserver and using a pre-finalizer for LifecycleObserver? I now
> > remember that we had been using a pre-finalizer for LifecycleObserver to
clear
> > out the raw pointers until we moved LifecycleObserver to the heap...
> 
> This all a temporary measure.

Then let's remove the EAGERLY_SWEEP_IF_NOT and write the EnabledIf in
DOMWindowProperty.

(But if this is really temporary, I'd prefer just hard-coding the class names
with a TODO. If this is not temporary, I'd prefer having a clearer way to
indicate eager sweeping (e.g., the EagerlySwept class).)

> Prefinalizers don't nest is one issue.

Ah, remembered :)

[sof, 2015-05-21 19:15:37]

On 2015/05/21 14:28:46, haraken wrote:
> On 2015/05/21 13:45:41, sof wrote:
> > On 2015/05/21 13:42:31, haraken wrote:
> > > On 2015/05/21 13:28:11, sof wrote:
> > > > On 2015/05/21 13:23:24, haraken wrote:
> > > > >
> > >
> https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h
> > > > > File Source/platform/heap/Heap.h (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/1146353002/diff/1/Source/platform/heap/Heap.h...
> > > > > Source/platform/heap/Heap.h:1145: // for Blink's DOMWindowProperty --
> > > provide
> > > > a
> > > > > convenience
> > > > > On 2015/05/21 11:47:50, sof wrote:
> > > > > > On 2015/05/21 11:45:39, haraken wrote:
> > > > > > > 
> > > > > > > Just to confirm: If we write:
> > > > > > > 
> > > > > > >   EAGERLY_SWEEP_TEMPLATE(LifecycleObserver);
> > > > > > >   EAGERLY_SWEEP(DOMWindowProperty);
> > > > > > > 
> > > > > > > then we hit the disambiguation error when encountering a class
that
> is
> > > > both
> > > > > > > LifecycleObserver and DOMWindowProperty?
> > > > > > 
> > > > > > Correct.
> > > > > 
> > > > > Makes sense.
> > > > > 
> > > > > I'm OK with this but would slightly prefer hard-coding the things
> instead
> > of
> > > > > introducing new macros, for clarity.
> > > > > 
> > > > >   // TODO: These are layering violations. Remove this once
> > > > >   // the DOMWindowProperty and LifecycleObserver are moved to
> > > > >   // Oilpan by default.
> > > > >   class DOMWindowProperty;
> > > > >   class LifecycleObserver;
> > > > >   template<typename T>
> > > > >   class HeapIndexTrait<T, typename
> > > WTF::EnableIf<WTF::IsSubclassOfTemplate<T,
> > > > > LifecycleObserver>::value || WTF::IsSubclass<T,
> > > > > DOMWindowProperty>::value>::Type> {
> > > > >     ...;
> > > > >   };
> > > > > 
> > > > > What do you think?
> > > > 
> > > > I would not prefer hardcoding something like that.
> > > > 
> > > > An alternative is to define the trait directly in DOMWindowProperty.h,
at
> > the
> > > > cost of exposing some Oilpan near-internal details there.
> > > 
> > > Maybe would it be simpler to introduce a EagerlySwept class instead of
> > > introcuding various kinds of macros?
> > > 
> > >   class LifecycleObserver : public EagerlySwept { };
> > >   class DOMWindowProperty : public EagerlySwept { };
> > > 
> > > Then we can use IsSubClass<T, EagerlySwept>.
> > > 
> > > ===
> > > BTW, sorry, I'm a bit confused: What's a difference between using eager
> > sweeping
> > > for LifecycleObserver and using a pre-finalizer for LifecycleObserver? I
now
> > > remember that we had been using a pre-finalizer for LifecycleObserver to
> clear
> > > out the raw pointers until we moved LifecycleObserver to the heap...
> > 
> > This all a temporary measure.
> 
> Then let's remove the EAGERLY_SWEEP_IF_NOT and write the EnabledIf in
> DOMWindowProperty.
> 
> (But if this is really temporary, I'd prefer just hard-coding the class names
> with a TODO. If this is not temporary, I'd prefer having a clearer way to
> indicate eager sweeping (e.g., the EagerlySwept class).)
> 

I think you'll find that scheme has ambiguity problems of its own etc.

I've switched to an encoding that avoids introducing multiple macros, have a
look?

[haraken, 2015-05-21 23:24:49]

Thanks for tidying up the macros! LGTM.

https://codereview.chromium.org/1146353002/diff/20001/Source/core/frame/DOMWi...
File Source/core/frame/DOMWindowProperty.h (right):

https://codereview.chromium.org/1146353002/diff/20001/Source/core/frame/DOMWi...
Source/core/frame/DOMWindowProperty.h:46: EAGERLY_SWEEP();

Add a TODO to remove this.

https://codereview.chromium.org/1146353002/diff/20001/Source/platform/Lifecyc...
File Source/platform/LifecycleObserver.h (right):

https://codereview.chromium.org/1146353002/diff/20001/Source/platform/Lifecyc...
Source/platform/LifecycleObserver.h:47: EAGERLY_SWEEP();

Add a TODO to remove this.

[sof, 2015-05-22 05:17:24]

The CQ bit was checked by sigbjornf@opera.com

[sof, 2015-05-22 05:17:24]

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/1146353002/#ps40001
(title: "add some limited-lifetime comments")

[commit-bot: I haz the power, 2015-05-22 05:17:51]

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

[commit-bot: I haz the power, 2015-05-22 07:30:07]

Committed patchset #3 (id:40001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195772