LeakDetector should run a GC on the compositor thread

LeakDetector should run a GC on the compositor thread

Compositor workers do not own the compositor thread so they do not do a cleanup gc on termination. This means objects will be left uncollected once per thread heap is enabled for compositor workers. To make up for that, the LeakDetector should run a GC on the compositor thread explicitly.

BUG=

Committed: https://crrev.com/717584e9840d3174ea30b712e7a5b6f58beb3488
Cr-Commit-Position: refs/heads/master@{#417574}

[keishi, 2016-09-02 06:48:40]

keishi@chromium.org changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org

[keishi, 2016-09-02 06:48:41]

We run a GC before the layout test leak detector but, it only triggers a gc on
the main thread heap. We need to run GCs on the per thread heaps too.

haraken@ Is there a better way, perhaps by extending GCTaskRunner?

[haraken, 2016-09-02 09:15:48]

On 2016/09/02 06:48:41, keishi wrote:
> We run a GC before the layout test leak detector but, it only triggers a gc on
> the main thread heap. We need to run GCs on the per thread heaps too.

Is this needed only in the leak detector? If yes, would it be an option to fix
the leak detector to shut down all worker threads (where the thread-termination
GC runs) before calling collectAllGarbage()?

It's anyway a good thing to run termination GCs in the leak detector since it's
useful to detect persistent handle leaks etc.

> haraken@ Is there a better way, perhaps by extending GCTaskRunner?

Yeah, maybe. On the other hand, we want to remove GCTaskRunner at least from
production builds since it's adding overhead to running tasks.

[haraken, 2016-09-02 09:21:29]

On 2016/09/02 09:15:48, haraken wrote:
> On 2016/09/02 06:48:41, keishi wrote:
> > We run a GC before the layout test leak detector but, it only triggers a gc
on
> > the main thread heap. We need to run GCs on the per thread heaps too.
> 
> Is this needed only in the leak detector? If yes, would it be an option to fix
> the leak detector to shut down all worker threads (where the
thread-termination
> GC runs) before calling collectAllGarbage()?
> 
> It's anyway a good thing to run termination GCs in the leak detector since
it's
> useful to detect persistent handle leaks etc.

Ah, I now understand this doesn't work because a termination GC is not called
for CompositorWorker's backing thread.

Maybe would it be helpful call
CompositorWorkerThread::clearSharedBackingThread() in the leak detector?

> 
> > haraken@ Is there a better way, perhaps by extending GCTaskRunner?
> 
> Yeah, maybe. On the other hand, we want to remove GCTaskRunner at least from
> production builds since it's adding overhead to running tasks.

[haraken, 2016-09-02 09:26:20]

On 2016/09/02 09:21:29, haraken wrote:
> On 2016/09/02 09:15:48, haraken wrote:
> > On 2016/09/02 06:48:41, keishi wrote:
> > > We run a GC before the layout test leak detector but, it only triggers a
gc
> on
> > > the main thread heap. We need to run GCs on the per thread heaps too.
> > 
> > Is this needed only in the leak detector? If yes, would it be an option to
fix
> > the leak detector to shut down all worker threads (where the
> thread-termination
> > GC runs) before calling collectAllGarbage()?
> > 
> > It's anyway a good thing to run termination GCs in the leak detector since
> it's
> > useful to detect persistent handle leaks etc.
> 
> Ah, I now understand this doesn't work because a termination GC is not called
> for CompositorWorker's backing thread.
> 
> Maybe would it be helpful call
> CompositorWorkerThread::clearSharedBackingThread() in the leak detector?
> 
> > 
> > > haraken@ Is there a better way, perhaps by extending GCTaskRunner?
> > 
> > Yeah, maybe. On the other hand, we want to remove GCTaskRunner at least from
> > production builds since it's adding overhead to running tasks.

Here is another idea :)

1) Move collectGarbage() and collectAllGarbage() from Heap to ThreadState. It
will make it clearer that they trigger GCs only on the specified thread.
2) Add AbstractAnimationWorkletThread::collectAllGarbage(), which calls
ThreadState::collectAllGarbage() on the compositor thread.
3) Make the leak detector call
AbstractAnimationWorkletThread::collectAllGarbage().

The reason I want to avoid calling
CompositorWorkerThread::clearSharedBackingThread() is that the shutdown logic is
way too complicated so I want to entirely remove it once I remove the graceful
shutdown sequence from the renderer.

What do you think?

[keishi, 2016-09-07 10:04:33]

On 2016/09/02 09:26:20, haraken wrote:
> On 2016/09/02 09:21:29, haraken wrote:
> > On 2016/09/02 09:15:48, haraken wrote:
> > > On 2016/09/02 06:48:41, keishi wrote:
> > > > We run a GC before the layout test leak detector but, it only triggers a
> gc
> > on
> > > > the main thread heap. We need to run GCs on the per thread heaps too.
> > > 
> > > Is this needed only in the leak detector? If yes, would it be an option to
> fix
> > > the leak detector to shut down all worker threads (where the
> > thread-termination
> > > GC runs) before calling collectAllGarbage()?
> > > 
> > > It's anyway a good thing to run termination GCs in the leak detector since
> > it's
> > > useful to detect persistent handle leaks etc.
> > 
> > Ah, I now understand this doesn't work because a termination GC is not
called
> > for CompositorWorker's backing thread.
> > 
> > Maybe would it be helpful call
> > CompositorWorkerThread::clearSharedBackingThread() in the leak detector?
> > 
> > > 
> > > > haraken@ Is there a better way, perhaps by extending GCTaskRunner?
> > > 
> > > Yeah, maybe. On the other hand, we want to remove GCTaskRunner at least
from
> > > production builds since it's adding overhead to running tasks.
> 
> Here is another idea :)
> 
> 1) Move collectGarbage() and collectAllGarbage() from Heap to ThreadState. It
> will make it clearer that they trigger GCs only on the specified thread.
> 2) Add AbstractAnimationWorkletThread::collectAllGarbage(), which calls
> ThreadState::collectAllGarbage() on the compositor thread.
> 3) Make the leak detector call
> AbstractAnimationWorkletThread::collectAllGarbage().
> 
> The reason I want to avoid calling
> CompositorWorkerThread::clearSharedBackingThread() is that the shutdown logic
is
> way too complicated so I want to entirely remove it once I remove the graceful
> shutdown sequence from the renderer.
> 
> What do you think?

CL for 1) is at https://codereview.chromium.org/2307003002
I've uploaded a new PS here for 2), 3)

If we have complex reference cycles between threads it could require
interleaving GCs, but for the tests we have now this seems to work.

[haraken, 2016-09-07 14:51:59]

On 2016/09/07 10:04:33, keishi wrote:
> On 2016/09/02 09:26:20, haraken wrote:
> > On 2016/09/02 09:21:29, haraken wrote:
> > > On 2016/09/02 09:15:48, haraken wrote:
> > > > On 2016/09/02 06:48:41, keishi wrote:
> > > > > We run a GC before the layout test leak detector but, it only triggers
a
> > gc
> > > on
> > > > > the main thread heap. We need to run GCs on the per thread heaps too.
> > > > 
> > > > Is this needed only in the leak detector? If yes, would it be an option
to
> > fix
> > > > the leak detector to shut down all worker threads (where the
> > > thread-termination
> > > > GC runs) before calling collectAllGarbage()?
> > > > 
> > > > It's anyway a good thing to run termination GCs in the leak detector
since
> > > it's
> > > > useful to detect persistent handle leaks etc.
> > > 
> > > Ah, I now understand this doesn't work because a termination GC is not
> called
> > > for CompositorWorker's backing thread.
> > > 
> > > Maybe would it be helpful call
> > > CompositorWorkerThread::clearSharedBackingThread() in the leak detector?
> > > 
> > > > 
> > > > > haraken@ Is there a better way, perhaps by extending GCTaskRunner?
> > > > 
> > > > Yeah, maybe. On the other hand, we want to remove GCTaskRunner at least
> from
> > > > production builds since it's adding overhead to running tasks.
> > 
> > Here is another idea :)
> > 
> > 1) Move collectGarbage() and collectAllGarbage() from Heap to ThreadState.
It
> > will make it clearer that they trigger GCs only on the specified thread.
> > 2) Add AbstractAnimationWorkletThread::collectAllGarbage(), which calls
> > ThreadState::collectAllGarbage() on the compositor thread.
> > 3) Make the leak detector call
> > AbstractAnimationWorkletThread::collectAllGarbage().
> > 
> > The reason I want to avoid calling
> > CompositorWorkerThread::clearSharedBackingThread() is that the shutdown
logic
> is
> > way too complicated so I want to entirely remove it once I remove the
graceful
> > shutdown sequence from the renderer.
> > 
> > What do you think?
> 
> CL for 1) is at https://codereview.chromium.org/2307003002
> I've uploaded a new PS here for 2), 3)
> 
> If we have complex reference cycles between threads it could require
> interleaving GCs, but for the tests we have now this seems to work.

LGTM

[keishi, 2016-09-09 01:25:35]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-09-09 01:25:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 01:34:14]

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

[commit-bot: I haz the power, 2016-09-09 01:34:15]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[keishi, 2016-09-09 01:42:10]

Description was changed from

==========
collectAllGarbage should run GC for all ThreadHeaps

BUG=
==========

to

==========
LeakDetector should run a GC on the compositor thread

Compositor workers do not own the compositor thread so they do not do a cleanup
gc on termination. To make up for that the LeakDetector should run a GC on the
compositor thread explicitly.

BUG=
==========

[keishi, 2016-09-09 01:43:33]

Description was changed from

==========
LeakDetector should run a GC on the compositor thread

Compositor workers do not own the compositor thread so they do not do a cleanup
gc on termination. To make up for that the LeakDetector should run a GC on the
compositor thread explicitly.

BUG=
==========

to

==========
LeakDetector should run a GC on the compositor thread

Compositor workers do not own the compositor thread so they do not do a cleanup
gc on termination. This means objects will be left uncollected once per thread
heap is enabled for compositor workers. To make up for that, the LeakDetector
should run a GC on the compositor thread explicitly.

BUG=
==========

[keishi, 2016-09-09 09:59:17]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-09-09 09:59:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-09 13:45:48]

Description was changed from

==========
LeakDetector should run a GC on the compositor thread

Compositor workers do not own the compositor thread so they do not do a cleanup
gc on termination. This means objects will be left uncollected once per thread
heap is enabled for compositor workers. To make up for that, the LeakDetector
should run a GC on the compositor thread explicitly.

BUG=
==========

to

==========
LeakDetector should run a GC on the compositor thread

Compositor workers do not own the compositor thread so they do not do a cleanup
gc on termination. This means objects will be left uncollected once per thread
heap is enabled for compositor workers. To make up for that, the LeakDetector
should run a GC on the compositor thread explicitly.

BUG=
==========

[commit-bot: I haz the power, 2016-09-09 13:45:49]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-09 13:47:37]

Description was changed from

==========
LeakDetector should run a GC on the compositor thread

Compositor workers do not own the compositor thread so they do not do a cleanup
gc on termination. This means objects will be left uncollected once per thread
heap is enabled for compositor workers. To make up for that, the LeakDetector
should run a GC on the compositor thread explicitly.

BUG=
==========

to

==========
LeakDetector should run a GC on the compositor thread

Compositor workers do not own the compositor thread so they do not do a cleanup
gc on termination. This means objects will be left uncollected once per thread
heap is enabled for compositor workers. To make up for that, the LeakDetector
should run a GC on the compositor thread explicitly.

BUG=

Committed: https://crrev.com/717584e9840d3174ea30b712e7a5b6f58beb3488
Cr-Commit-Position: refs/heads/master@{#417574}
==========

[commit-bot: I haz the power, 2016-09-09 13:47:38]

Patchset 2 (id:??) landed as
https://crrev.com/717584e9840d3174ea30b712e7a5b6f58beb3488
Cr-Commit-Position: refs/heads/master@{#417574}