Oilpan: We need to collect all garbage before shutting down the worker thread

Oilpan: We need to collect all garbage before shutting down the worker thread

- Before checking ASSERT(m_workerGlobalScope->hasOneRef()) at the end of worker thread execution, we need to collect all garbage. Specifically, we need to call Heap::collectGarbage() before the ASSERT. Note that we don't need to call V8's GC because wrappers of the worker thread are stored in DOMWrapperMap and they are already cleared in ~WorkerScriptController().

- The essential part of this CL is to add Heap::collectGarbage() before the ASSERT.

- This CL moves ThreadState::cleanup() into ThreadState::detach(), because cleanup() should be always coupled with detach().

- This CL adds Heap::collectAllGarbage() that calls Heap::collectGarabge() multiple times. Until we remove all persistent handles, we need to call multiple times to break persistent chains and collect all garbage.

BUG=340522
NOTRY=true

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168717

[haraken, 2014-03-05 05:08:35]

PTAL

I think this CL will fix worker-related crashes that happen when moving Events
to the heap.

https://codereview.chromium.org/187483002/diff/1/Source/core/workers/WorkerTh...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/187483002/diff/1/Source/core/workers/WorkerTh...
Source/core/workers/WorkerThread.cpp:145:
Heap::collectAllGarbage(ThreadState::NoHeapPointersOnStack);

Actually, I'll remove this code soon because the following ASSERT will be
removed by sof's CL that moves WorkerGlobalScope to the heap.

After removing the ASSERT, we just need to make sure that the thread heap is
empty at the end of ThreadState::cleanup(). In other words, we just need to make
sure that all garbage is collected before checking that the thread heap is
empty. However, this is already done in ThreadState::cleanup().

In conclusion, once we remove the ASSERT, we can just remove
Heap::collectAllGarbage().

[kouhei (in TOK), 2014-03-05 05:58:14]

https://codereview.chromium.org/187483002/diff/1/Source/heap/ThreadState.cpp
File Source/heap/ThreadState.cpp (right):

https://codereview.chromium.org/187483002/diff/1/Source/heap/ThreadState.cpp#...
Source/heap/ThreadState.cpp:333: delete state;
Doesn't this mean we fail if we couldn't collect all objects within 5 gc cycles?

[haraken, 2014-03-05 06:00:57]

https://codereview.chromium.org/187483002/diff/1/Source/heap/ThreadState.cpp
File Source/heap/ThreadState.cpp (right):

https://codereview.chromium.org/187483002/diff/1/Source/heap/ThreadState.cpp#...
Source/heap/ThreadState.cpp:333: delete state;
On 2014/03/05 05:58:15, kouhei wrote:
> Doesn't this mean we fail if we couldn't collect all objects within 5 gc
cycles?

Yes, it means. However, there is no way to know the number of GCs we really need
to trigger. I guess 5 would be enough. If we have a persistent chain longer than
5, it will be something we should fix.

[kouhei (in TOK), 2014-03-05 06:16:55]

On 2014/03/05 06:00:57, haraken wrote:
> https://codereview.chromium.org/187483002/diff/1/Source/heap/ThreadState.cpp
> File Source/heap/ThreadState.cpp (right):
> 
>
https://codereview.chromium.org/187483002/diff/1/Source/heap/ThreadState.cpp#...
> Source/heap/ThreadState.cpp:333: delete state;
> On 2014/03/05 05:58:15, kouhei wrote:
> > Doesn't this mean we fail if we couldn't collect all objects within 5 gc
> cycles?
> 
> Yes, it means. However, there is no way to know the number of GCs we really
need
> to trigger. I guess 5 would be enough. If we have a persistent chain longer
than
> 5, it will be something we should fix.

I agree that if chain is longer than 5, that's something we should fix, but
after this CL, we will crash when we have chain longer than 5. My worry is that
it would affect stability.

[haraken, 2014-03-05 06:19:11]

On 2014/03/05 06:16:55, kouhei wrote:
> On 2014/03/05 06:00:57, haraken wrote:
> > https://codereview.chromium.org/187483002/diff/1/Source/heap/ThreadState.cpp
> > File Source/heap/ThreadState.cpp (right):
> > 
> >
>
https://codereview.chromium.org/187483002/diff/1/Source/heap/ThreadState.cpp#...
> > Source/heap/ThreadState.cpp:333: delete state;
> > On 2014/03/05 05:58:15, kouhei wrote:
> > > Doesn't this mean we fail if we couldn't collect all objects within 5 gc
> > cycles?
> > 
> > Yes, it means. However, there is no way to know the number of GCs we really
> need
> > to trigger. I guess 5 would be enough. If we have a persistent chain longer
> than
> > 5, it will be something we should fix.
> 
> I agree that if chain is longer than 5, that's something we should fix, but
> after this CL, we will crash when we have chain longer than 5. My worry is
that
> it would affect stability.

Before this CL, we were crashing if we have a chain longer than 2. In that
sense, this CL improves our situation.

[Mads Ager (chromium), 2014-03-05 07:40:34]

LGTM, but please just #ifdef out the assert since this will get caught by the
empty check. Either you or sof will then have to merge anyway when one of these
changes land.

https://codereview.chromium.org/187483002/diff/1/Source/core/workers/WorkerTh...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/187483002/diff/1/Source/core/workers/WorkerTh...
Source/core/workers/WorkerThread.cpp:145:
Heap::collectAllGarbage(ThreadState::NoHeapPointersOnStack);
On 2014/03/05 05:08:35, haraken wrote:
> 
> Actually, I'll remove this code soon because the following ASSERT will be
> removed by sof's CL that moves WorkerGlobalScope to the heap.
> 
> After removing the ASSERT, we just need to make sure that the thread heap is
> empty at the end of ThreadState::cleanup(). In other words, we just need to
make
> sure that all garbage is collected before checking that the thread heap is
> empty. However, this is already done in ThreadState::cleanup().
> 
> In conclusion, once we remove the ASSERT, we can just remove
> Heap::collectAllGarbage().

We should just #ifdef out the ASSERT now so we don't have to introduce the
comment above. Then either you or sof will merge that. :-)

https://codereview.chromium.org/187483002/diff/1/Source/heap/ThreadState.cpp
File Source/heap/ThreadState.cpp (right):

https://codereview.chromium.org/187483002/diff/1/Source/heap/ThreadState.cpp#...
Source/heap/ThreadState.cpp:315: // When ThreadState is detaching from non-main
thread its
Ah, you moved it here. Have the comment on what cleanup does in the header file
with the other comments?

https://codereview.chromium.org/187483002/diff/1/Source/heap/ThreadState.h
File Source/heap/ThreadState.h (left):

https://codereview.chromium.org/187483002/diff/1/Source/heap/ThreadState.h#ol...
Source/heap/ThreadState.h:241: // When ThreadState is detaching from non-main
thread its
Please keep this comment.

[wibling-chromium, 2014-03-05 08:36:08]

LGTM with Mads' comments addressed.

[haraken, 2014-03-05 08:50:25]

Thanks for review!

https://codereview.chromium.org/187483002/diff/1/Source/core/workers/WorkerTh...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/187483002/diff/1/Source/core/workers/WorkerTh...
Source/core/workers/WorkerThread.cpp:145:
Heap::collectAllGarbage(ThreadState::NoHeapPointersOnStack);
On 2014/03/05 07:40:34, Mads Ager (chromium) wrote:
> On 2014/03/05 05:08:35, haraken wrote:
> > 
> > Actually, I'll remove this code soon because the following ASSERT will be
> > removed by sof's CL that moves WorkerGlobalScope to the heap.
> > 
> > After removing the ASSERT, we just need to make sure that the thread heap is
> > empty at the end of ThreadState::cleanup(). In other words, we just need to
> make
> > sure that all garbage is collected before checking that the thread heap is
> > empty. However, this is already done in ThreadState::cleanup().
> > 
> > In conclusion, once we remove the ASSERT, we can just remove
> > Heap::collectAllGarbage().
> 
> We should just #ifdef out the ASSERT now so we don't have to introduce the
> comment above. Then either you or sof will merge that. :-)

Done.

https://codereview.chromium.org/187483002/diff/1/Source/heap/ThreadState.cpp
File Source/heap/ThreadState.cpp (right):

https://codereview.chromium.org/187483002/diff/1/Source/heap/ThreadState.cpp#...
Source/heap/ThreadState.cpp:315: // When ThreadState is detaching from non-main
thread its
On 2014/03/05 07:40:34, Mads Ager (chromium) wrote:
> Ah, you moved it here. Have the comment on what cleanup does in the header
file
> with the other comments?

Done.

[haraken, 2014-03-05 08:50:54]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-03-05 08:53:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/187483002/20001

[commit-bot: I haz the power, 2014-03-05 09:36:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-05 09:36:08]

Retried try job too often on mac_blink_rel for step(s) blink_heap_unittests,
blink_platform_unittests, webkit_lint, webkit_python_tests, webkit_tests,
webkit_unit_tests, wtf_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_blink_...

[zerny-chromium, 2014-03-05 09:43:22]

lgtm

https://codereview.chromium.org/187483002/diff/20001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/187483002/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:140: #ifdef !ENABLE_OILPAN
#if !ENABLE_OILPAN

As seen in the build failure.

[kouhei (in TOK), 2014-03-05 09:47:40]

https://codereview.chromium.org/187483002/diff/20001/Source/core/workers/Work...
File Source/core/workers/WorkerThread.cpp (right):

https://codereview.chromium.org/187483002/diff/20001/Source/core/workers/Work...
Source/core/workers/WorkerThread.cpp:140: #ifdef !ENABLE_OILPAN
On 2014/03/05 09:43:23, zerny-chromium wrote:
> #if !ENABLE_OILPAN
> 
> As seen in the build failure.
or
#if !ENABLE(OILPAN)
to be correct.

[haraken, 2014-03-06 10:49:44]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-03-06 10:49:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/187483002/40001

[commit-bot: I haz the power, 2014-03-06 12:02:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 12:02:44]

Try jobs failed on following builders: linux_blink

[haraken, 2014-03-07 07:58:22]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-03-07 07:58:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/187483002/40001

[commit-bot: I haz the power, 2014-03-07 07:58:56]

Change committed as 168717