Prevent GCs when constructing GC mixin objects.

Prevent GCs when constructing GC mixin objects.

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

[sof, 2015-02-23 06:03:41]

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

[sof, 2015-02-23 06:03:41]

Please take a look.

I suggest we do this.

[haraken, 2015-02-23 08:24:22]

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

[haraken, 2015-02-23 08:24:23]

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

https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:1072: // vtabke after the constructors of all its
subclasses have run,

vtable

https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:1092:
ThreadState::current()->enterNoGCAllowedScope();

Are compilers smart enough to remove the two ThreadState::current() lookups (one
in Heap::allocateOnHeapIndex and the other here)?

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

https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:239: , m_noGCAllowedCount(0)

Can we add ASSERT(!m_noGCAllowedCount) to somewhere to check that the enter and
exit are balanced (e.g., when entering SafePointScope)?

https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:673: if (UNLIKELY(m_noGCAllowedCount))

I'm wondering how unlikely this happens. What happens if we create a ton of
Nodes?

  for (...)
    document.createElement("div");

Given Node is a GCMixin, we might end up not triggering any conservative GC?

https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.h:647: void enterNoGCAllowedScope() {
m_noGCAllowedCount++; }

enterGCForbiddenScope ? (for consistency with SweepForbiddenScope)

[sof, 2015-02-23 08:30:20]

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

https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:673: if (UNLIKELY(m_noGCAllowedCount))
On 2015/02/23 08:24:23, haraken wrote:
> 
> I'm wondering how unlikely this happens. What happens if we create a ton of
> Nodes?
> 
>   for (...)
>     document.createElement("div");
> 
> Given Node is a GCMixin, we might end up not triggering any conservative GC?

Not sure I understand the concern - Node is an EventTarget, which is not a
mixin.

[haraken, 2015-02-23 08:47:18]

On 2015/02/23 08:30:20, sof wrote:
>
https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
> File Source/platform/heap/ThreadState.cpp (right):
> 
>
https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
> Source/platform/heap/ThreadState.cpp:673: if (UNLIKELY(m_noGCAllowedCount))
> On 2015/02/23 08:24:23, haraken wrote:
> > 
> > I'm wondering how unlikely this happens. What happens if we create a ton of
> > Nodes?
> > 
> >   for (...)
> >     document.createElement("div");
> > 
> > Given Node is a GCMixin, we might end up not triggering any conservative GC?
> 
> Not sure I understand the concern - Node is an EventTarget, which is not a
> mixin.

Sorry, div was not a good example. I meant something like:

  for (...)
    document.createElement("media");

where HTMLMediaElement is a mixin. Can we have a conservative GC during the for
loop?

[sof, 2015-02-23 08:50:39]

On 2015/02/23 08:47:18, haraken wrote:
> On 2015/02/23 08:30:20, sof wrote:
> >
>
https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
> > File Source/platform/heap/ThreadState.cpp (right):
> > 
> >
>
https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
> > Source/platform/heap/ThreadState.cpp:673: if (UNLIKELY(m_noGCAllowedCount))
> > On 2015/02/23 08:24:23, haraken wrote:
> > > 
> > > I'm wondering how unlikely this happens. What happens if we create a ton
of
> > > Nodes?
> > > 
> > >   for (...)
> > >     document.createElement("div");
> > > 
> > > Given Node is a GCMixin, we might end up not triggering any conservative
GC?
> > 
> > Not sure I understand the concern - Node is an EventTarget, which is not a
> > mixin.
> 
> Sorry, div was not a good example. I meant something like:
> 
>   for (...)
>     document.createElement("media");
> 
> where HTMLMediaElement is a mixin. Can we have a conservative GC during the
for
> loop?

A heavy object.

The allocations made by the HTMLMediaElement ctor may trigger conservative GCs;
we leave the no-GC scope once the GarbageCollectedFinalized<> ctor runs.

[haraken, 2015-02-23 08:57:27]

On 2015/02/23 08:50:39, sof wrote:
> On 2015/02/23 08:47:18, haraken wrote:
> > On 2015/02/23 08:30:20, sof wrote:
> > >
> >
>
https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
> > > File Source/platform/heap/ThreadState.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
> > > Source/platform/heap/ThreadState.cpp:673: if
(UNLIKELY(m_noGCAllowedCount))
> > > On 2015/02/23 08:24:23, haraken wrote:
> > > > 
> > > > I'm wondering how unlikely this happens. What happens if we create a ton
> of
> > > > Nodes?
> > > > 
> > > >   for (...)
> > > >     document.createElement("div");
> > > > 
> > > > Given Node is a GCMixin, we might end up not triggering any conservative
> GC?
> > > 
> > > Not sure I understand the concern - Node is an EventTarget, which is not a
> > > mixin.
> > 
> > Sorry, div was not a good example. I meant something like:
> > 
> >   for (...)
> >     document.createElement("media");
> > 
> > where HTMLMediaElement is a mixin. Can we have a conservative GC during the
> for
> > loop?
> 
> A heavy object.
> 
> The allocations made by the HTMLMediaElement ctor may trigger conservative
GCs;
> we leave the no-GC scope once the GarbageCollectedFinalized<> ctor runs.

OK, LGTM.

My concern is if we will end up suppressing conservative GCs too aggressively by
this change. If that is the case, we should add a way to trigger a conservative
GC in leaveGCForbiddenScope (if a conservative GC has tried to run in the
GCForbiddenScope we're about to leave).

[sof, 2015-02-23 10:21:20]

New patchsets have been uploaded after l-g-t-m from haraken@chromium.org

[sof, 2015-02-23 10:29:02]

Thanks.

If Oilpan alumni have feedback&comments on this one, I'd be most interested to
hear (on or off review), as this involves issues that have received considerable
attention & thought previously.

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

https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:1072: // vtabke after the constructors of all its
subclasses have run,
On 2015/02/23 08:24:23, haraken wrote:
> 
> vtable

Done.

https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:1092:
ThreadState::current()->enterNoGCAllowedScope();
On 2015/02/23 08:24:22, haraken wrote:
> 
> Are compilers smart enough to remove the two ThreadState::current() lookups
(one
> in Heap::allocateOnHeapIndex and the other here)?

Reasonable to assume so, but we might as well manually CSE to remove the
possibility that it won't be. Now done.

I left in a templated version of allocateOnHeapIndex<T>; please check if you
find that agreeable.

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

https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:239: , m_noGCAllowedCount(0)
On 2015/02/23 08:24:23, haraken wrote:
> 
> Can we add ASSERT(!m_noGCAllowedCount) to somewhere to check that the enter
and
> exit are balanced (e.g., when entering SafePointScope)?

It is currently asserted for on leaving; is that sufficient?

https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.h:647: void enterNoGCAllowedScope() {
m_noGCAllowedCount++; }
On 2015/02/23 08:24:23, haraken wrote:
> 
> enterGCForbiddenScope ? (for consistency with SweepForbiddenScope)

Switched naming to align with that.

[sof, 2015-02-23 10:32:04]

On 2015/02/23 08:57:27, haraken wrote:
> On 2015/02/23 08:50:39, sof wrote:
> > On 2015/02/23 08:47:18, haraken wrote:
> > > On 2015/02/23 08:30:20, sof wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
> > > > File Source/platform/heap/ThreadState.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/946163003/diff/40001/Source/platform/heap/Thr...
> > > > Source/platform/heap/ThreadState.cpp:673: if
> (UNLIKELY(m_noGCAllowedCount))
> > > > On 2015/02/23 08:24:23, haraken wrote:
> > > > > 
> > > > > I'm wondering how unlikely this happens. What happens if we create a
ton
> > of
> > > > > Nodes?
> > > > > 
> > > > >   for (...)
> > > > >     document.createElement("div");
> > > > > 
> > > > > Given Node is a GCMixin, we might end up not triggering any
conservative
> > GC?
> > > > 
> > > > Not sure I understand the concern - Node is an EventTarget, which is not
a
> > > > mixin.
> > > 
> > > Sorry, div was not a good example. I meant something like:
> > > 
> > >   for (...)
> > >     document.createElement("media");
> > > 
> > > where HTMLMediaElement is a mixin. Can we have a conservative GC during
the
> > for
> > > loop?
> > 
> > A heavy object.
> > 
> > The allocations made by the HTMLMediaElement ctor may trigger conservative
> GCs;
> > we leave the no-GC scope once the GarbageCollectedFinalized<> ctor runs.
> 
> OK, LGTM.
> 
> My concern is if we will end up suppressing conservative GCs too aggressively
by
> this change. If that is the case, we should add a way to trigger a
conservative
> GC in leaveGCForbiddenScope (if a conservative GC has tried to run in the
> GCForbiddenScope we're about to leave).

I added a comment/FIXME capturing this in leaveGCForbiddenScope().

[haraken, 2015-02-23 10:41:10]

LGTM

[sof, 2015-02-23 12:40:25]

Last Oilpan bot run appears fine also, let's try this.

[sof, 2015-02-23 12:40:33]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2015-02-23 12:41:20]

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

[commit-bot: I haz the power, 2015-02-23 12:44:18]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190650