Oilpan: support eager finalization/sweeping.

Oilpan: support eager finalization/sweeping.

To accommodate objects that must be finalized promptly, add a designated
heap for these. If lazy sweeping is enabled, that heap will be swept
eagerly and not delayed until (say) an out-of-line allocation trigger
the sweep. If lazy sweeping is disabled, the eagerly swept heap will be
processed just like any other.

Prompt finalization is needed for objects that can be accessed through
"off-heap" references, like direct access from the embedder or through
a timer, or some other non-Oilpan object in Blink(*). References that have
been considered safe by carefully working through finalization order and
other lifetime dependencies. If such 'safe' lifetime dependencies rely on
the finalization of other heap objects, they cannot be assumed to hold
iff lazy sweeping is enabled. That's because all dead objects are then
not finalized synchronously, as observed by the rest of Blink, leading
to the possibility that 'off-heap' access of a heap object might
access an object that has yet to be finalized (but is known to be
dead), or that some of the heap objects it refers to may have
finalized by now. Regardless of exact state, an object that shouldn't
be accessed in this near-dead state by another party. Hence the
introduction of eagerly swept heaps -- all objects residing there
that have been deemed unreachable by the marking phase are eagerly
swept & finalized before the GC returns. Class hierarchies
are marked as eagerly swept by using the WILL_BE_EAGERLY_SWEPT(Class)
macro.

A very relevant follow-on question here is if we can reliably identify
objects that must be eagerly swept.

* - while the goal is to remove or otherwise bring all such off-heap
references under control within Blink, they currently exist. But
not profusely many, but more than we would like to while we're in
in a transition state where Oilpan isn't always enabled.

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

[haraken, 2015-05-11 01:20:26]

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

[haraken, 2015-05-11 01:20:27]

Thanks for the patch! The implementation looks correct.

Here are a couple of comments.

Currently we have three similar mechanisms to avoid problems related to the lazy
sweeping.

(a) eager sweep
(b) pre-finalizer
(c) TimerIsObjectAliveTrait

Once (a) is implemented, (c) will be gone. In other words, we'll have (a) and
(b) in the code base.

The question is whether we need (a) or not. Can we probably use the
pre-finalizer for all scenarios (which means that we add pre-finalizers to all
Timers and LifecycleObservers)?

The only concern of using pre-finalizers would be the overhead of scanning the
pre-finalizers in each GC. However, I guess that the overhead would be
acceptable given that the number of pre-finalizers would be limited. (Remember
that once we ship Oilpan for everything, those objects will anyway need to have
weak processings -- i.e., if we argue that the overhead of the pre-finalizer is
a problem, we also have to argue that the weak processing that will be added to
the object would be a problem as well.)

> A very relevant follow-on question here is if we can reliably identify
> objects that must be eagerly swept.

I think that the ASan poisoning would be helpful. The poisoning poisons all
will-be-swept objects before the lazy sweeping starts. Thus if someone accesses
the will-be-swept object before the lazy sweeping actually sweeps the object,
then the poisoning system will detect the error.

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

https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h...
Source/platform/heap/Heap.h:1127: // We use sized heaps for most 'normal' objcts
to improve memory locality.

objects

https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h...
Source/platform/heap/Heap.h:1149: return NormalPage1HeapIndex;

Indent

https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h...
Source/platform/heap/Heap.h:1153: return NormalPage3HeapIndex;

Indent

https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h...
Source/platform/heap/Heap.h:1158: #define WILL_BE_EAGERLY_SWEPT(TYPE)         \

WILL_BE_EAGERLY_SWEPT => EAGERLY_SWEPT ?

[sof, 2015-05-11 05:20:13]

On 2015/05/11 01:20:27, haraken wrote:
> Thanks for the patch! The implementation looks correct.
> 
> Here are a couple of comments.
> 
> Currently we have three similar mechanisms to avoid problems related to the
lazy
> sweeping.
> 
> (a) eager sweep
> (b) pre-finalizer
> (c) TimerIsObjectAliveTrait
> 
> Once (a) is implemented, (c) will be gone. In other words, we'll have (a) and
> (b) in the code base.
> 
> The question is whether we need (a) or not. Can we probably use the
> pre-finalizer for all scenarios (which means that we add pre-finalizers to all
> Timers and LifecycleObservers)?
> 
> The only concern of using pre-finalizers would be the overhead of scanning the
> pre-finalizers in each GC. However, I guess that the overhead would be
> acceptable given that the number of pre-finalizers would be limited. (Remember
> that once we ship Oilpan for everything, those objects will anyway need to
have
> weak processings -- i.e., if we argue that the overhead of the pre-finalizer
is
> a problem, we also have to argue that the weak processing that will be added
to
> the object would be a problem as well.)
> 

Prefinalizers require code modifications, e.g., for PlatformSpeechSynthesizer in
the above it would be a dispose method that manually clears an OwnPtr<>.

> > A very relevant follow-on question here is if we can reliably identify
> > objects that must be eagerly swept.
> 
> I think that the ASan poisoning would be helpful. The poisoning poisons all
> will-be-swept objects before the lazy sweeping starts. Thus if someone
accesses
> the will-be-swept object before the lazy sweeping actually sweeps the object,
> then the poisoning system will detect the error.
> 

It would preferable not to have to rely on such, but a helpful way to root out
problems.

> https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h
> File Source/platform/heap/Heap.h (right):
> 
>
https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h...
> Source/platform/heap/Heap.h:1127: // We use sized heaps for most 'normal'
objcts
> to improve memory locality.
> 
> objects
> 
>
https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h...
> Source/platform/heap/Heap.h:1149: return NormalPage1HeapIndex;
> 
> Indent
> 
>
https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h...
> Source/platform/heap/Heap.h:1153: return NormalPage3HeapIndex;
> 
> Indent
> 
>
https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h...
> Source/platform/heap/Heap.h:1158: #define WILL_BE_EAGERLY_SWEPT(TYPE)        
\
> 
> WILL_BE_EAGERLY_SWEPT => EAGERLY_SWEPT ?

[sof, 2015-05-11 05:20:22]

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

https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h...
Source/platform/heap/Heap.h:1158: #define WILL_BE_EAGERLY_SWEPT(TYPE)         \
On 2015/05/11 01:20:27, haraken wrote:
> 
> WILL_BE_EAGERLY_SWEPT => EAGERLY_SWEPT ?

That doesn't give enough context about what this is about next to a class
declaration, i think. Could you motivate why that is a preferred naming?

[haraken, 2015-05-11 06:44:08]

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

https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h...
Source/platform/heap/Heap.h:1158: #define WILL_BE_EAGERLY_SWEPT(TYPE)         \
On 2015/05/11 05:20:22, sof wrote:
> On 2015/05/11 01:20:27, haraken wrote:
> > 
> > WILL_BE_EAGERLY_SWEPT => EAGERLY_SWEPT ?
> 
> That doesn't give enough context about what this is about next to a class
> declaration, i think. Could you motivate why that is a preferred naming?

XWillBeY means that it is X on non-oilpan and it is Y on oilpan.

For PlatformSpeechSynthesizer, we want to enable EAGER_SWEPT on both non-oilpan
and oilpan, so it should be EAGER_SWEPT. For LifecycleObservers, we want to
enable EAGER_SWEPT only on non-oilpan, it should be EAGER_SWEPT_WILL_BE_REMOVED.

(But I don't have a strong opinion here :-)

[haraken, 2015-05-11 06:50:11]

> > Currently we have three similar mechanisms to avoid problems related to the
> lazy
> > sweeping.
> > 
> > (a) eager sweep
> > (b) pre-finalizer
> > (c) TimerIsObjectAliveTrait
> > 
> > Once (a) is implemented, (c) will be gone. In other words, we'll have (a)
and
> > (b) in the code base.
> > 
> > The question is whether we need (a) or not. Can we probably use the
> > pre-finalizer for all scenarios (which means that we add pre-finalizers to
all
> > Timers and LifecycleObservers)?
> > 
> > The only concern of using pre-finalizers would be the overhead of scanning
the
> > pre-finalizers in each GC. However, I guess that the overhead would be
> > acceptable given that the number of pre-finalizers would be limited.
(Remember
> > that once we ship Oilpan for everything, those objects will anyway need to
> have
> > weak processings -- i.e., if we argue that the overhead of the pre-finalizer
> is
> > a problem, we also have to argue that the weak processing that will be added
> to
> > the object would be a problem as well.)
> > 
> 
> Prefinalizers require code modifications, e.g., for PlatformSpeechSynthesizer
in
> the above it would be a dispose method that manually clears an OwnPtr<>.

Yes, we need to move the contents of the destructor into a pre-finalizer. I'm
not sure which is better -- the pre-finalizer would be better in a sense that we
don't need to add another mechanism (i.e., eager sweep) to the GC; on the other
hand, eager sweep would be better if the pre-finalizers need a bunch of
intrusive changes.

[sof, 2015-05-11 06:52:31]

On 2015/05/11 06:50:11, haraken wrote:
> > > Currently we have three similar mechanisms to avoid problems related to
the
> > lazy
> > > sweeping.
> > > 
> > > (a) eager sweep
> > > (b) pre-finalizer
> > > (c) TimerIsObjectAliveTrait
> > > 
> > > Once (a) is implemented, (c) will be gone. In other words, we'll have (a)
> and
> > > (b) in the code base.
> > > 
> > > The question is whether we need (a) or not. Can we probably use the
> > > pre-finalizer for all scenarios (which means that we add pre-finalizers to
> all
> > > Timers and LifecycleObservers)?
> > > 
> > > The only concern of using pre-finalizers would be the overhead of scanning
> the
> > > pre-finalizers in each GC. However, I guess that the overhead would be
> > > acceptable given that the number of pre-finalizers would be limited.
> (Remember
> > > that once we ship Oilpan for everything, those objects will anyway need to
> > have
> > > weak processings -- i.e., if we argue that the overhead of the
pre-finalizer
> > is
> > > a problem, we also have to argue that the weak processing that will be
added
> > to
> > > the object would be a problem as well.)
> > > 
> > 
> > Prefinalizers require code modifications, e.g., for
PlatformSpeechSynthesizer
> in
> > the above it would be a dispose method that manually clears an OwnPtr<>.
> 
> Yes, we need to move the contents of the destructor into a pre-finalizer. I'm
> not sure which is better -- the pre-finalizer would be better in a sense that
we
> don't need to add another mechanism (i.e., eager sweep) to the GC; on the
other
> hand, eager sweep would be better if the pre-finalizers need a bunch of
> intrusive changes.

Another thing to keep in mind is that prefinalizers don't nest. Which would be
an issue given that you want to add prefinalizers to all objects having a Timer
field, right?

[haraken, 2015-05-11 08:10:26]

On 2015/05/11 06:52:31, sof wrote:
> On 2015/05/11 06:50:11, haraken wrote:
> > > > Currently we have three similar mechanisms to avoid problems related to
> the
> > > lazy
> > > > sweeping.
> > > > 
> > > > (a) eager sweep
> > > > (b) pre-finalizer
> > > > (c) TimerIsObjectAliveTrait
> > > > 
> > > > Once (a) is implemented, (c) will be gone. In other words, we'll have
(a)
> > and
> > > > (b) in the code base.
> > > > 
> > > > The question is whether we need (a) or not. Can we probably use the
> > > > pre-finalizer for all scenarios (which means that we add pre-finalizers
to
> > all
> > > > Timers and LifecycleObservers)?
> > > > 
> > > > The only concern of using pre-finalizers would be the overhead of
scanning
> > the
> > > > pre-finalizers in each GC. However, I guess that the overhead would be
> > > > acceptable given that the number of pre-finalizers would be limited.
> > (Remember
> > > > that once we ship Oilpan for everything, those objects will anyway need
to
> > > have
> > > > weak processings -- i.e., if we argue that the overhead of the
> pre-finalizer
> > > is
> > > > a problem, we also have to argue that the weak processing that will be
> added
> > > to
> > > > the object would be a problem as well.)
> > > > 
> > > 
> > > Prefinalizers require code modifications, e.g., for
> PlatformSpeechSynthesizer
> > in
> > > the above it would be a dispose method that manually clears an OwnPtr<>.
> > 
> > Yes, we need to move the contents of the destructor into a pre-finalizer.
I'm
> > not sure which is better -- the pre-finalizer would be better in a sense
that
> we
> > don't need to add another mechanism (i.e., eager sweep) to the GC; on the
> other
> > hand, eager sweep would be better if the pre-finalizers need a bunch of
> > intrusive changes.
> 
> Another thing to keep in mind is that prefinalizers don't nest. Which would be
> an issue given that you want to add prefinalizers to all objects having a
Timer
> field, right?

That makes sense... Also I now understand that we need to insert a EAGER_SWEPT
or pre-finalizer to all objects that hold Timers; i.e., I now realized what you
mean by "A very relevant follow-on question here is if we can reliably identify
objects that must be eagerly swept."

Give me some time for more thinking :)

[sof, 2015-05-11 08:46:13]

On 2015/05/11 08:10:26, haraken wrote:
> On 2015/05/11 06:52:31, sof wrote:
> > On 2015/05/11 06:50:11, haraken wrote:
> > > > > Currently we have three similar mechanisms to avoid problems related
to
> > the
> > > > lazy
> > > > > sweeping.
> > > > > 
> > > > > (a) eager sweep
> > > > > (b) pre-finalizer
> > > > > (c) TimerIsObjectAliveTrait
> > > > > 
> > > > > Once (a) is implemented, (c) will be gone. In other words, we'll have
> (a)
> > > and
> > > > > (b) in the code base.
> > > > > 
> > > > > The question is whether we need (a) or not. Can we probably use the
> > > > > pre-finalizer for all scenarios (which means that we add
pre-finalizers
> to
> > > all
> > > > > Timers and LifecycleObservers)?
> > > > > 
> > > > > The only concern of using pre-finalizers would be the overhead of
> scanning
> > > the
> > > > > pre-finalizers in each GC. However, I guess that the overhead would be
> > > > > acceptable given that the number of pre-finalizers would be limited.
> > > (Remember
> > > > > that once we ship Oilpan for everything, those objects will anyway
need
> to
> > > > have
> > > > > weak processings -- i.e., if we argue that the overhead of the
> > pre-finalizer
> > > > is
> > > > > a problem, we also have to argue that the weak processing that will be
> > added
> > > > to
> > > > > the object would be a problem as well.)
> > > > > 
> > > > 
> > > > Prefinalizers require code modifications, e.g., for
> > PlatformSpeechSynthesizer
> > > in
> > > > the above it would be a dispose method that manually clears an OwnPtr<>.
> > > 
> > > Yes, we need to move the contents of the destructor into a pre-finalizer.
> I'm
> > > not sure which is better -- the pre-finalizer would be better in a sense
> that
> > we
> > > don't need to add another mechanism (i.e., eager sweep) to the GC; on the
> > other
> > > hand, eager sweep would be better if the pre-finalizers need a bunch of
> > > intrusive changes.
> > 
> > Another thing to keep in mind is that prefinalizers don't nest. Which would
be
> > an issue given that you want to add prefinalizers to all objects having a
> Timer
> > field, right?
> 
> That makes sense... Also I now understand that we need to insert a EAGER_SWEPT
> or pre-finalizer to all objects that hold Timers; i.e., I now realized what
you
> mean by "A very relevant follow-on question here is if we can reliably
identify
> objects that must be eagerly swept."
> 
> Give me some time for more thinking :)

ok, will try to do some of that too :) But I think it makes sense to leave
Timers alone for now & let them manually check liveness before invoking -- it
solves a large class of headaches already.

Perhaps a next step here would be to fuzz with
https://codereview.chromium.org/1110853002/ ?

[haraken, 2015-05-11 09:14:52]

On 2015/05/11 08:46:13, sof wrote:
> On 2015/05/11 08:10:26, haraken wrote:
> > On 2015/05/11 06:52:31, sof wrote:
> > > On 2015/05/11 06:50:11, haraken wrote:
> > > > > > Currently we have three similar mechanisms to avoid problems related
> to
> > > the
> > > > > lazy
> > > > > > sweeping.
> > > > > > 
> > > > > > (a) eager sweep
> > > > > > (b) pre-finalizer
> > > > > > (c) TimerIsObjectAliveTrait
> > > > > > 
> > > > > > Once (a) is implemented, (c) will be gone. In other words, we'll
have
> > (a)
> > > > and
> > > > > > (b) in the code base.
> > > > > > 
> > > > > > The question is whether we need (a) or not. Can we probably use the
> > > > > > pre-finalizer for all scenarios (which means that we add
> pre-finalizers
> > to
> > > > all
> > > > > > Timers and LifecycleObservers)?
> > > > > > 
> > > > > > The only concern of using pre-finalizers would be the overhead of
> > scanning
> > > > the
> > > > > > pre-finalizers in each GC. However, I guess that the overhead would
be
> > > > > > acceptable given that the number of pre-finalizers would be limited.
> > > > (Remember
> > > > > > that once we ship Oilpan for everything, those objects will anyway
> need
> > to
> > > > > have
> > > > > > weak processings -- i.e., if we argue that the overhead of the
> > > pre-finalizer
> > > > > is
> > > > > > a problem, we also have to argue that the weak processing that will
be
> > > added
> > > > > to
> > > > > > the object would be a problem as well.)
> > > > > > 
> > > > > 
> > > > > Prefinalizers require code modifications, e.g., for
> > > PlatformSpeechSynthesizer
> > > > in
> > > > > the above it would be a dispose method that manually clears an
OwnPtr<>.
> > > > 
> > > > Yes, we need to move the contents of the destructor into a
pre-finalizer.
> > I'm
> > > > not sure which is better -- the pre-finalizer would be better in a sense
> > that
> > > we
> > > > don't need to add another mechanism (i.e., eager sweep) to the GC; on
the
> > > other
> > > > hand, eager sweep would be better if the pre-finalizers need a bunch of
> > > > intrusive changes.
> > > 
> > > Another thing to keep in mind is that prefinalizers don't nest. Which
would
> be
> > > an issue given that you want to add prefinalizers to all objects having a
> > Timer
> > > field, right?
> > 
> > That makes sense... Also I now understand that we need to insert a
EAGER_SWEPT
> > or pre-finalizer to all objects that hold Timers; i.e., I now realized what
> you
> > mean by "A very relevant follow-on question here is if we can reliably
> identify
> > objects that must be eagerly swept."
> > 
> > Give me some time for more thinking :)
> 
> ok, will try to do some of that too :) But I think it makes sense to leave
> Timers alone for now & let them manually check liveness before invoking -- it
> solves a large class of headaches already.
> 
> Perhaps a next step here would be to fuzz with
> https://codereview.chromium.org/1110853002/ ?

Except for Timers and LifecycleObservers, how many objects would cause the
problem in lazy sweeping?

If the number is limited, I might want to suggest:

1) Keep the TimerIsObjectAliveTrait as is.
2) Land https://codereview.chromium.org/1132053002/ for LifecycleObservers.
3) Use pre-finalizers for other objects.

I'm OK with landing eager sweeping instead of 2) and 3), but I'm a bit afraid
that it will add complexity to the GC infrastructure. For example, some Nodes
are ActiveDOMObjects. Those Nodes will go to not the NodeHeap but the
EagerSweepHeap. It won't cause any problem but is a bit nasty.

[sof, 2015-05-11 09:31:31]

On 2015/05/11 09:14:52, haraken wrote:
>
...
> 
> Except for Timers and LifecycleObservers, how many objects would cause the
> problem in lazy sweeping?
> 

(LifecycleObservers held onto by tasks is another category non-Oilpan.)

That is the key question -- how can we determine what heap references we are
handing over to the embedder, or to other non-Oilpan objects within Blink.
Objects that might be accessed after they've been deemed garbage, but not yet
swept.

It is uncomfortable not to know, or have a process for statically determining
exactly those problematic references reside. Some candidates could be found via
Source/platform/exported/ and public/platform/

[haraken, 2015-05-11 09:36:11]

On 2015/05/11 09:31:31, sof wrote:
> On 2015/05/11 09:14:52, haraken wrote:
> >
> ...
> > 
> > Except for Timers and LifecycleObservers, how many objects would cause the
> > problem in lazy sweeping?
> > 
> 
> (LifecycleObservers held onto by tasks is another category non-Oilpan.)
> 
> That is the key question -- how can we determine what heap references we are
> handing over to the embedder, or to other non-Oilpan objects within Blink.

If these handed-over references are held by Persistent or Member, it won't be an
issue. If the references are held by a raw pointer, it will be an issue, but
they are already marked with GC_PLUGIN_IGNORE, aren't they?

[haraken, 2015-05-11 09:47:12]

On 2015/05/11 09:36:11, haraken wrote:
> On 2015/05/11 09:31:31, sof wrote:
> > On 2015/05/11 09:14:52, haraken wrote:
> > >
> > ...
> > > 
> > > Except for Timers and LifecycleObservers, how many objects would cause the
> > > problem in lazy sweeping?
> > > 
> > 
> > (LifecycleObservers held onto by tasks is another category non-Oilpan.)
> > 
> > That is the key question -- how can we determine what heap references we are
> > handing over to the embedder, or to other non-Oilpan objects within Blink.
> 
> If these handed-over references are held by Persistent or Member, it won't be
an
> issue. If the references are held by a raw pointer, it will be an issue, but
> they are already marked with GC_PLUGIN_IGNORE, aren't they?

Sorry, the above comment might not make much sense.

I think the problem happens only in cases where we're intentionally doing
something nasty.

For example, the following pattern causes the problem in lazy sweeping:

  class Y : WillBeGarbageCollectedMixin { };
  class X : public GarbageCollected<X>, public Y { };

In non-oilpan, X is on-heap but Y is off-heap. As discussed previously, this
pattern works correctly as long as 1) no one is tracing Y strongly and 2) a raw
pointer to Y is explicitly cleared out when the Y dies.

LifecycleObserver and DOMWindowProperty have this pattern, so it will cause the
problem.

Timer has GC_PLUGIN_IGNORE, so it will cause the problem.

[haraken, 2015-05-11 09:50:22]

On 2015/05/11 09:47:12, haraken wrote:
> On 2015/05/11 09:36:11, haraken wrote:
> > On 2015/05/11 09:31:31, sof wrote:
> > > On 2015/05/11 09:14:52, haraken wrote:
> > > >
> > > ...
> > > > 
> > > > Except for Timers and LifecycleObservers, how many objects would cause
the
> > > > problem in lazy sweeping?
> > > > 
> > > 
> > > (LifecycleObservers held onto by tasks is another category non-Oilpan.)
> > > 
> > > That is the key question -- how can we determine what heap references we
are
> > > handing over to the embedder, or to other non-Oilpan objects within Blink.
> > 
> > If these handed-over references are held by Persistent or Member, it won't
be
> an
> > issue. If the references are held by a raw pointer, it will be an issue, but
> > they are already marked with GC_PLUGIN_IGNORE, aren't they?
> 
> Sorry, the above comment might not make much sense.
> 
> I think the problem happens only in cases where we're intentionally doing
> something nasty.
> 
> For example, the following pattern causes the problem in lazy sweeping:
> 
>   class Y : WillBeGarbageCollectedMixin { };
>   class X : public GarbageCollected<X>, public Y { };
> 
> In non-oilpan, X is on-heap but Y is off-heap. As discussed previously, this
> pattern works correctly as long as 1) no one is tracing Y strongly and 2) a
raw
> pointer to Y is explicitly cleared out when the Y dies.
> 
> LifecycleObserver and DOMWindowProperty have this pattern, so it will cause
the
> problem.
> 
> Timer has GC_PLUGIN_IGNORE, so it will cause the problem.

Basically, the problem happens only when we have raw pointers to on-heap
objects. It can happen if the raw pointer is marked as GC_PLUGIN_IGNORE or we're
intentionally doing something nasty (like LifecycleObserver and
DOMWindowProperty).

The next question: Why does the PlatformSpeechSynthesizer cause the problem?

[sof, 2015-05-11 10:16:09]

On 2015/05/11 09:50:22, haraken wrote:
>
...
> 
> The next question: Why does the PlatformSpeechSynthesizer cause the problem?

It doesn't directly, its WebSpeechSynthesizerClientImpl does, which we hand out
to the embedder.

[haraken, 2015-05-11 13:09:41]

On 2015/05/11 10:16:09, sof wrote:
> On 2015/05/11 09:50:22, haraken wrote:
> >
> ...
> > 
> > The next question: Why does the PlatformSpeechSynthesizer cause the problem?
> 
> It doesn't directly, its WebSpeechSynthesizerClientImpl does, which we hand
out
> to the embedder.

Can/Should we make those handed-out references be wrapped with WebPrivatePtr?

[sof, 2015-05-11 13:22:51]

On 2015/05/11 13:09:41, haraken wrote:
> On 2015/05/11 10:16:09, sof wrote:
> > On 2015/05/11 09:50:22, haraken wrote:
> > >
> > ...
> > > 
> > > The next question: Why does the PlatformSpeechSynthesizer cause the
problem?
> > 
> > It doesn't directly, its WebSpeechSynthesizerClientImpl does, which we hand
> out
> > to the embedder.
> 
> Can/Should we make those handed-out references be wrapped with WebPrivatePtr?

Keeping a strong reference from the embedder instead, you mean? Maybe such a
shift in lifetime responsibilities could work, but the speech implementation
would have to change some. i don't understand the current arrangement in detail.

I worry there are other client impl interfaces like this; someone should look
into that.

[haraken, 2015-05-11 13:32:16]

On 2015/05/11 13:22:51, sof wrote:
> On 2015/05/11 13:09:41, haraken wrote:
> > On 2015/05/11 10:16:09, sof wrote:
> > > On 2015/05/11 09:50:22, haraken wrote:
> > > >
> > > ...
> > > > 
> > > > The next question: Why does the PlatformSpeechSynthesizer cause the
> problem?
> > > 
> > > It doesn't directly, its WebSpeechSynthesizerClientImpl does, which we
hand
> > out
> > > to the embedder.
> > 
> > Can/Should we make those handed-out references be wrapped with
WebPrivatePtr?
> 
> Keeping a strong reference from the embedder instead, you mean? Maybe such a
> shift in lifetime responsibilities could work, but the speech implementation
> would have to change some. i don't understand the current arrangement in
detail.

Yes. As far as I understand the design correctly, when we pass Blink pointers to
the embedder, they must be wrapped with WebPrivatePtr. Passing a raw pointer to
the embedder sounds dangerous even without oilpan.

[haraken, 2015-05-18 05:12:42]

On 2015/05/11 09:31:31, sof wrote:
> On 2015/05/11 09:14:52, haraken wrote:
> >
> ...
> > 
> > Except for Timers and LifecycleObservers, how many objects would cause the
> > problem in lazy sweeping?
> > 
> 
> (LifecycleObservers held onto by tasks is another category non-Oilpan.)
> 
> That is the key question -- how can we determine what heap references we are
> handing over to the embedder, or to other non-Oilpan objects within Blink.
> Objects that might be accessed after they've been deemed garbage, but not yet
> swept.
> 
> It is uncomfortable not to know, or have a process for statically determining
> exactly those problematic references reside. Some candidates could be found
via
> Source/platform/exported/ and public/platform/

Trying to catch up...

1) Regarding the WebSpeechSynthesizerClientImpl, it turned out to be an issue
not related to oilpan. Do you know any other crashes caused by lazy sweeping
(except for LifecycleObservers)?

2) Would you tell me Web classes you have in mind that are passing bare on-heap
pointers to the embedder?

[sof, 2015-05-18 05:22:14]

On 2015/05/18 05:12:42, haraken wrote:
> On 2015/05/11 09:31:31, sof wrote:
> > On 2015/05/11 09:14:52, haraken wrote:
> > >
> > ...
> > > 
> > > Except for Timers and LifecycleObservers, how many objects would cause the
> > > problem in lazy sweeping?
> > > 
> > 
> > (LifecycleObservers held onto by tasks is another category non-Oilpan.)
> > 
> > That is the key question -- how can we determine what heap references we are
> > handing over to the embedder, or to other non-Oilpan objects within Blink.
> > Objects that might be accessed after they've been deemed garbage, but not
yet
> > swept.
> > 
> > It is uncomfortable not to know, or have a process for statically
determining
> > exactly those problematic references reside. Some candidates could be found
> via
> > Source/platform/exported/ and public/platform/
> 
> Trying to catch up...
> 
> 1) Regarding the WebSpeechSynthesizerClientImpl, it turned out to be an issue
> not related to oilpan. Do you know any other crashes caused by lazy sweeping
> (except for LifecycleObservers)?
> 

That's news to me, what issue has it found out to be? Not sure I trust
https://code.google.com/p/chromium/issues/detail?id=485843 as repro'ing without
lazy sweeping..

Second part -- tasks calling methods on ActiveDOMObjects (like stop()) is
another. The fuzzer run will identify others with Oilpan enabled, I
hope/imagine.

> 2) Would you tell me Web classes you have in mind that are passing bare
on-heap
> pointers to the embedder?

I think you/we have to work through that -- I don't know there being an
effective boundary or signs that would show what they are. (Looking at the
client implementations in Source/modules/ would be a start..)

[haraken, 2015-05-18 15:12:17]

> > 1) Regarding the WebSpeechSynthesizerClientImpl, it turned out to be an
issue
> > not related to oilpan. Do you know any other crashes caused by lazy sweeping
> > (except for LifecycleObservers)?
> > 
> 
> That's news to me, what issue has it found out to be? Not sure I trust
> https://code.google.com/p/chromium/issues/detail?id=485843 as repro'ing
without
> lazy sweeping..

I thought so since the issue 485843 reproduces without lazy sweeping.

> Second part -- tasks calling methods on ActiveDOMObjects (like stop()) is
> another. The fuzzer run will identify others with Oilpan enabled, I
> hope/imagine.

Agreed.

Another thing we need to address is DOMWindowProperties. Since
LocalDOMWindow::m_properties are unregistered in ~DOMWindowProperty(), it has
the same problem as LifecycleNotifer::m_observerSet.

> > 2) Would you tell me Web classes you have in mind that are passing bare
> on-heap
> > pointers to the embedder?
> 
> I think you/we have to work through that -- I don't know there being an
> effective boundary or signs that would show what they are. (Looking at the
> client implementations in Source/modules/ would be a start..)

Thanks, understood.

My suggestion would be as follows:

1) Observe the ClusterFuzz & fix broken things.
2) Land the ASan poisoning about the destructor. The ASan poisoning will be
helpful to catch the error we're discussing here -- if some code touches X
before the lazy sweeping calls X's destructor, then the ASan poisoning will
detect the error.
3) Land https://codereview.chromium.org/1132053002/ (+ other LifecycleObservers
such as ActiveDOMObjects).
4) Land a similar fix for DOMWindowProperty.
5) Once the result for 1) and 2) looks good, land the lazy sweeping.

What do you think?

[sof, 2015-05-18 20:22:43]

On 2015/05/18 15:12:17, haraken wrote:
> > > 1) Regarding the WebSpeechSynthesizerClientImpl, it turned out to be an
> issue
> > > not related to oilpan. Do you know any other crashes caused by lazy
sweeping
> > > (except for LifecycleObservers)?
> > > 
> > 
> > That's news to me, what issue has it found out to be? Not sure I trust
> > https://code.google.com/p/chromium/issues/detail?id=485843 as repro'ing
> without
> > lazy sweeping..
> 
> I thought so since the issue 485843 reproduces without lazy sweeping.
> 

But it only first showed when it was turned on? Given the code involved, I
remain unconvinced without seeing crash reports as to what builds this was with
(and I don't have access to the original crash report atm.) But it could be an
hitherto unknown problem, not ruling that out completely.

> > Second part -- tasks calling methods on ActiveDOMObjects (like stop()) is
> > another. The fuzzer run will identify others with Oilpan enabled, I
> > hope/imagine.
> 
> Agreed.
> 
> Another thing we need to address is DOMWindowProperties. Since
> LocalDOMWindow::m_properties are unregistered in ~DOMWindowProperty(), it has
> the same problem as LifecycleNotifer::m_observerSet.
> 

Yes, unregistration of dead DOMWindowProperty instances may not have happened by
the time the window notifies of detach&destruction.

> > > 2) Would you tell me Web classes you have in mind that are passing bare
> > on-heap
> > > pointers to the embedder?
> > 
> > I think you/we have to work through that -- I don't know there being an
> > effective boundary or signs that would show what they are. (Looking at the
> > client implementations in Source/modules/ would be a start..)
> 
> Thanks, understood.
> 
> My suggestion would be as follows:
> 
> 1) Observe the ClusterFuzz & fix broken things.
> 2) Land the ASan poisoning about the destructor. The ASan poisoning will be
> helpful to catch the error we're discussing here -- if some code touches X
> before the lazy sweeping calls X's destructor, then the ASan poisoning will
> detect the error.
> 3) Land https://codereview.chromium.org/1132053002/ (+ other
LifecycleObservers
> such as ActiveDOMObjects).
> 4) Land a similar fix for DOMWindowProperty.
> 5) Once the result for 1) and 2) looks good, land the lazy sweeping.
> 
> What do you think?


- I suspect 1) will throw up a few issues that needs to be addressed first.
- This might help making 2) easier, as
https://codereview.chromium.org/1134523002/ might safely land in the meantime,
allowing the ugly timer heaps hacks on
https://codereview.chromium.org/1120943002/ to be redundant.
- I would prefer not to do 3&4 for non-Oilpan, but if we make it very clear
they're temporary measures, perhaps they'll be accepted?

[haraken, 2015-05-18 23:18:03]

On 2015/05/18 20:22:43, sof wrote:
> On 2015/05/18 15:12:17, haraken wrote:
> > > > 1) Regarding the WebSpeechSynthesizerClientImpl, it turned out to be an
> > issue
> > > > not related to oilpan. Do you know any other crashes caused by lazy
> sweeping
> > > > (except for LifecycleObservers)?
> > > > 
> > > 
> > > That's news to me, what issue has it found out to be? Not sure I trust
> > > https://code.google.com/p/chromium/issues/detail?id=485843 as repro'ing
> > without
> > > lazy sweeping..
> > 
> > I thought so since the issue 485843 reproduces without lazy sweeping.
> > 
> 
> But it only first showed when it was turned on? Given the code involved, I
> remain unconvinced without seeing crash reports as to what builds this was
with
> (and I don't have access to the original crash report atm.) But it could be an
> hitherto unknown problem, not ruling that out completely.

It seems you're right. I'll look into the issue.

> > > Second part -- tasks calling methods on ActiveDOMObjects (like stop()) is
> > > another. The fuzzer run will identify others with Oilpan enabled, I
> > > hope/imagine.
> > 
> > Agreed.
> > 
> > Another thing we need to address is DOMWindowProperties. Since
> > LocalDOMWindow::m_properties are unregistered in ~DOMWindowProperty(), it
has
> > the same problem as LifecycleNotifer::m_observerSet.
> > 
> 
> Yes, unregistration of dead DOMWindowProperty instances may not have happened
by
> the time the window notifies of detach&destruction.
> 
> > > > 2) Would you tell me Web classes you have in mind that are passing bare
> > > on-heap
> > > > pointers to the embedder?
> > > 
> > > I think you/we have to work through that -- I don't know there being an
> > > effective boundary or signs that would show what they are. (Looking at the
> > > client implementations in Source/modules/ would be a start..)
> > 
> > Thanks, understood.
> > 
> > My suggestion would be as follows:
> > 
> > 1) Observe the ClusterFuzz & fix broken things.
> > 2) Land the ASan poisoning about the destructor. The ASan poisoning will be
> > helpful to catch the error we're discussing here -- if some code touches X
> > before the lazy sweeping calls X's destructor, then the ASan poisoning will
> > detect the error.
> > 3) Land https://codereview.chromium.org/1132053002/ (+ other
> LifecycleObservers
> > such as ActiveDOMObjects).
> > 4) Land a similar fix for DOMWindowProperty.
> > 5) Once the result for 1) and 2) looks good, land the lazy sweeping.
> > 
> > What do you think?
> 
> 
> - I suspect 1) will throw up a few issues that needs to be addressed first.
> - This might help making 2) easier, as
> https://codereview.chromium.org/1134523002/ might safely land in the meantime,

Agreed, let's land the CL. LGed.

> allowing the ugly timer heaps hacks on
> https://codereview.chromium.org/1120943002/ to be redundant.
> - I would prefer not to do 3&4 for non-Oilpan, but if we make it very clear
> they're temporary measures, perhaps they'll be accepted?

I prefer not doing 3&4 either, but as far as I can consider, that is the least
intrusive way to enable lazy sweeping without shipping Oilpan for a ton of
objects in one go. Introducing eager sweeping would be another way, but I guess
it would require more intrusive changes on a broad area of the code base...

[haraken, 2015-05-19 10:35:58]

On 2015/05/18 23:18:03, haraken wrote:
> On 2015/05/18 20:22:43, sof wrote:
> > On 2015/05/18 15:12:17, haraken wrote:
> > > > > 1) Regarding the WebSpeechSynthesizerClientImpl, it turned out to be
an
> > > issue
> > > > > not related to oilpan. Do you know any other crashes caused by lazy
> > sweeping
> > > > > (except for LifecycleObservers)?
> > > > > 
> > > > 
> > > > That's news to me, what issue has it found out to be? Not sure I trust
> > > > https://code.google.com/p/chromium/issues/detail?id=485843 as repro'ing
> > > without
> > > > lazy sweeping..
> > > 
> > > I thought so since the issue 485843 reproduces without lazy sweeping.
> > > 
> > 
> > But it only first showed when it was turned on? Given the code involved, I
> > remain unconvinced without seeing crash reports as to what builds this was
> with
> > (and I don't have access to the original crash report atm.) But it could be
an
> > hitherto unknown problem, not ruling that out completely.
> 
> It seems you're right. I'll look into the issue.
> 
> > > > Second part -- tasks calling methods on ActiveDOMObjects (like stop())
is
> > > > another. The fuzzer run will identify others with Oilpan enabled, I
> > > > hope/imagine.
> > > 
> > > Agreed.
> > > 
> > > Another thing we need to address is DOMWindowProperties. Since
> > > LocalDOMWindow::m_properties are unregistered in ~DOMWindowProperty(), it
> has
> > > the same problem as LifecycleNotifer::m_observerSet.
> > > 
> > 
> > Yes, unregistration of dead DOMWindowProperty instances may not have
happened
> by
> > the time the window notifies of detach&destruction.
> > 
> > > > > 2) Would you tell me Web classes you have in mind that are passing
bare
> > > > on-heap
> > > > > pointers to the embedder?
> > > > 
> > > > I think you/we have to work through that -- I don't know there being an
> > > > effective boundary or signs that would show what they are. (Looking at
the
> > > > client implementations in Source/modules/ would be a start..)
> > > 
> > > Thanks, understood.
> > > 
> > > My suggestion would be as follows:
> > > 
> > > 1) Observe the ClusterFuzz & fix broken things.
> > > 2) Land the ASan poisoning about the destructor. The ASan poisoning will
be
> > > helpful to catch the error we're discussing here -- if some code touches X
> > > before the lazy sweeping calls X's destructor, then the ASan poisoning
will
> > > detect the error.
> > > 3) Land https://codereview.chromium.org/1132053002/ (+ other
> > LifecycleObservers
> > > such as ActiveDOMObjects).
> > > 4) Land a similar fix for DOMWindowProperty.
> > > 5) Once the result for 1) and 2) looks good, land the lazy sweeping.
> > > 
> > > What do you think?
> > 
> > 
> > - I suspect 1) will throw up a few issues that needs to be addressed first.
> > - This might help making 2) easier, as
> > https://codereview.chromium.org/1134523002/ might safely land in the
meantime,
> 
> Agreed, let's land the CL. LGed.
> 
> > allowing the ugly timer heaps hacks on
> > https://codereview.chromium.org/1120943002/ to be redundant.
> > - I would prefer not to do 3&4 for non-Oilpan, but if we make it very clear
> > they're temporary measures, perhaps they'll be accepted?
> 
> I prefer not doing 3&4 either, but as far as I can consider, that is the least
> intrusive way to enable lazy sweeping without shipping Oilpan for a ton of
> objects in one go. Introducing eager sweeping would be another way, but I
guess
> it would require more intrusive changes on a broad area of the code base...

Another idea:

- Land this CL (i.e., eager sweeping). Use the eager sweeping for
LifecycleObservers.
- Keep the current hack for Timers.
- For other things that cause any finalization issue, use a pre-finalizer.

This would minimize the amount of changes in the core/ side.

[sof, 2015-05-19 14:59:56]

On 2015/05/19 10:35:58, haraken wrote:
> On 2015/05/18 23:18:03, haraken wrote:
> > On 2015/05/18 20:22:43, sof wrote:
> > > On 2015/05/18 15:12:17, haraken wrote:
> > > > > > 1) Regarding the WebSpeechSynthesizerClientImpl, it turned out to be
> an
> > > > issue
> > > > > > not related to oilpan. Do you know any other crashes caused by lazy
> > > sweeping
> > > > > > (except for LifecycleObservers)?
> > > > > > 
> > > > > 
> > > > > That's news to me, what issue has it found out to be? Not sure I trust
> > > > > https://code.google.com/p/chromium/issues/detail?id=485843 as
repro'ing
> > > > without
> > > > > lazy sweeping..
> > > > 
> > > > I thought so since the issue 485843 reproduces without lazy sweeping.
> > > > 
> > > 
> > > But it only first showed when it was turned on? Given the code involved, I
> > > remain unconvinced without seeing crash reports as to what builds this was
> > with
> > > (and I don't have access to the original crash report atm.) But it could
be
> an
> > > hitherto unknown problem, not ruling that out completely.
> > 
> > It seems you're right. I'll look into the issue.
> > 
> > > > > Second part -- tasks calling methods on ActiveDOMObjects (like stop())
> is
> > > > > another. The fuzzer run will identify others with Oilpan enabled, I
> > > > > hope/imagine.
> > > > 
> > > > Agreed.
> > > > 
> > > > Another thing we need to address is DOMWindowProperties. Since
> > > > LocalDOMWindow::m_properties are unregistered in ~DOMWindowProperty(),
it
> > has
> > > > the same problem as LifecycleNotifer::m_observerSet.
> > > > 
> > > 
> > > Yes, unregistration of dead DOMWindowProperty instances may not have
> happened
> > by
> > > the time the window notifies of detach&destruction.
> > > 
> > > > > > 2) Would you tell me Web classes you have in mind that are passing
> bare
> > > > > on-heap
> > > > > > pointers to the embedder?
> > > > > 
> > > > > I think you/we have to work through that -- I don't know there being
an
> > > > > effective boundary or signs that would show what they are. (Looking at
> the
> > > > > client implementations in Source/modules/ would be a start..)
> > > > 
> > > > Thanks, understood.
> > > > 
> > > > My suggestion would be as follows:
> > > > 
> > > > 1) Observe the ClusterFuzz & fix broken things.
> > > > 2) Land the ASan poisoning about the destructor. The ASan poisoning will
> be
> > > > helpful to catch the error we're discussing here -- if some code touches
X
> > > > before the lazy sweeping calls X's destructor, then the ASan poisoning
> will
> > > > detect the error.
> > > > 3) Land https://codereview.chromium.org/1132053002/ (+ other
> > > LifecycleObservers
> > > > such as ActiveDOMObjects).
> > > > 4) Land a similar fix for DOMWindowProperty.
> > > > 5) Once the result for 1) and 2) looks good, land the lazy sweeping.
> > > > 
> > > > What do you think?
> > > 
> > > 
> > > - I suspect 1) will throw up a few issues that needs to be addressed
first.
> > > - This might help making 2) easier, as
> > > https://codereview.chromium.org/1134523002/ might safely land in the
> meantime,
> > 
> > Agreed, let's land the CL. LGed.
> > 
> > > allowing the ugly timer heaps hacks on
> > > https://codereview.chromium.org/1120943002/ to be redundant.
> > > - I would prefer not to do 3&4 for non-Oilpan, but if we make it very
clear
> > > they're temporary measures, perhaps they'll be accepted?
> > 
> > I prefer not doing 3&4 either, but as far as I can consider, that is the
least
> > intrusive way to enable lazy sweeping without shipping Oilpan for a ton of
> > objects in one go. Introducing eager sweeping would be another way, but I
> guess
> > it would require more intrusive changes on a broad area of the code base...
> 
> Another idea:
> 
> - Land this CL (i.e., eager sweeping). Use the eager sweeping for
> LifecycleObservers.
> - Keep the current hack for Timers.
> - For other things that cause any finalization issue, use a pre-finalizer.
> 
> This would minimize the amount of changes in the core/ side.

Agreed - let's take that approach when enabling lazy sweeping, non-Oilpan. It is
unclear (to my mind) if we need eager sweeping beyond this transition period, so
prefinalizers might be the sole method for controlling finalization timing
longer term.

I've rebased this CL (and removed the PlatformSpeechSynthesizer use of eager
sweeping, not needed.)

[sof, 2015-05-19 15:00:15]

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

https://codereview.chromium.org/1138663002/diff/1/Source/platform/heap/Heap.h...
Source/platform/heap/Heap.h:1158: #define WILL_BE_EAGERLY_SWEPT(TYPE)         \
On 2015/05/11 06:44:07, haraken wrote:
> On 2015/05/11 05:20:22, sof wrote:
> > On 2015/05/11 01:20:27, haraken wrote:
> > > 
> > > WILL_BE_EAGERLY_SWEPT => EAGERLY_SWEPT ?
> > 
> > That doesn't give enough context about what this is about next to a class
> > declaration, i think. Could you motivate why that is a preferred naming?
> 
> XWillBeY means that it is X on non-oilpan and it is Y on oilpan.
> 
> For PlatformSpeechSynthesizer, we want to enable EAGER_SWEPT on both
non-oilpan
> and oilpan, so it should be EAGER_SWEPT. For LifecycleObservers, we want to
> enable EAGER_SWEPT only on non-oilpan, it should be
EAGER_SWEPT_WILL_BE_REMOVED.
> 
> (But I don't have a strong opinion here :-)

Naming.

I've now prefixed OILPAN_ to give some handle for the code reader as to what
this might be about. i.e., OILPAN_EAGERLY_SWEEP() - wdyt?

[haraken, 2015-05-19 23:23:38]

LGTM

Just to confirm: We're are going to add the EAGERLY_SWEEP macro only on
LifecycleObserver, right? (i.e., we won't need to add the macro to all its
subclasses or holders etc, right?)

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

https://codereview.chromium.org/1138663002/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1103: return NormalPage2HeapIndex;

Fix indentation. Or:

  return size < 32 ? NormalPage1HeapIndex : NormalPage2HeapIndex;

https://codereview.chromium.org/1138663002/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1107: return NormalPage4HeapIndex;

Ditto.

https://codereview.chromium.org/1138663002/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1111: #define OILPAN_EAGERLY_SWEEP(TYPE)          \

Add a TODO to mention that the eager sweeping is a temporary technology and
should be removed once Oilpan is shipped for everything.

https://codereview.chromium.org/1138663002/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1111: #define OILPAN_EAGERLY_SWEEP(TYPE)          \

Hmm, we don't use a OILPAN_ prefix in other macros and don't want to introduce
"Oilpan" in the code base (except for comments) since Oilpan is a project name.

EAGERLY_SWEEP(TYPE) or ENABLE_EAGER_SWEEPING(TYPE) ?

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

https://codereview.chromium.org/1138663002/diff/20001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:936: ASSERT(!sweepForbidden());

I think this should be:

  if (sweepForbidden())
    return;

See the comment in completeSweep() when sweepForbidden() can become true.

https://codereview.chromium.org/1138663002/diff/20001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:944: double timeStamp =
WTF::currentTimeMS();

Remove this (see below).

https://codereview.chromium.org/1138663002/diff/20001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:948:
Platform::current()->histogramCustomCounts("BlinkGC.EagerSweep",
WTF::currentTimeMS() - timeStamp, 0, 10 * 1000, 50);

This UMA doesn't exist (and wouldn't be worth adding). So remove this.

[sof, 2015-05-20 09:43:01]

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

https://codereview.chromium.org/1138663002/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1103: return NormalPage2HeapIndex;
On 2015/05/19 23:23:37, haraken wrote:
> 
> Fix indentation. Or:
> 
>   return size < 32 ? NormalPage1HeapIndex : NormalPage2HeapIndex;

Done; sorry, bad editor configuration.

https://codereview.chromium.org/1138663002/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1111: #define OILPAN_EAGERLY_SWEEP(TYPE)          \
On 2015/05/19 23:23:37, haraken wrote:
> 
> Hmm, we don't use a OILPAN_ prefix in other macros and don't want to introduce
> "Oilpan" in the code base (except for comments) since Oilpan is a project
name.
> 
> EAGERLY_SWEEP(TYPE) or ENABLE_EAGER_SWEEPING(TYPE) ?
 
Not worth the time trying to change your mind about this temporary macro,
EAGERLY_SWEEP() it is.

https://codereview.chromium.org/1138663002/diff/20001/Source/platform/heap/He...
Source/platform/heap/Heap.h:1111: #define OILPAN_EAGERLY_SWEEP(TYPE)          \
On 2015/05/19 23:23:37, haraken wrote:
> 
> Add a TODO to mention that the eager sweeping is a temporary technology and
> should be removed once Oilpan is shipped for everything.

That would the ideal outcome, but it is not clear to me that the (e.g.)
PlatformSpeechSynthesizer change to use a prefinalizer is preferable to Blink
over using an eager sweep annotation.

Added a comment about re-evaluating the facility once the dust has settled.

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

https://codereview.chromium.org/1138663002/diff/20001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:936: ASSERT(!sweepForbidden());
On 2015/05/19 23:23:37, haraken wrote:
> 
> I think this should be:
> 
>   if (sweepForbidden())
>     return;
> 
> See the comment in completeSweep() when sweepForbidden() can become true.

Done + xref'ed the comment.

https://codereview.chromium.org/1138663002/diff/20001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:944: double timeStamp =
WTF::currentTimeMS();
On 2015/05/19 23:23:37, haraken wrote:
> 
> Remove this (see below).

Removed.

[sof, 2015-05-20 09:46:50]

On 2015/05/19 23:23:38, haraken wrote:
> LGTM
> 
> Just to confirm: We're are going to add the EAGERLY_SWEEP macro only on
> LifecycleObserver, right? (i.e., we won't need to add the macro to all its
> subclasses or holders etc, right?)
> 

Thanks for the review.

Yes, not needed on subclasses. The HeapTest.EagerlySweepingPages verifies that
class subsumption works for the trait introduced here.

[haraken, 2015-05-20 12:30:10]

Still LGTM

[sof, 2015-05-20 12:42:53]

Adding EAGERLY_SWEEP() annotations in a follow-up CL.

[sof, 2015-05-20 12:43:03]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-05-20 12:43:16]

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

[commit-bot: I haz the power, 2015-05-20 14:16:42]

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