TaskGraphRunner refactor

Interface-izes TaskGraphRunner and moves implementation helpers to TaskGraphWorkQueue.

There are two new classes:
SynchronousTaskGraphRunner - used in some tests and by LayerTreeHostImpl
SingleThreadTaskGraphRunner - used in a number of tests (wrapped by TestTaskGraphRunner) and by GpuProcessTransportFactory and CompositorImplAndroid.

Additionally RasterWorkerPool has been re-worked to use the new helper.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4e3aa5a539e3774b51b998dc60eac42a97c2cfb8
Cr-Commit-Position: refs/heads/master@{#362338}

[ericrk, 2015-11-18 19:18:15]

ericrk@chromium.org changed reviewers:
+ reveman@chromium.org

[ericrk, 2015-11-18 19:18:16]

Here's a version of TaskGraphRunner which is interface-ized and has
implementation helpers moved to TaskGraphWorkQueue.

There are two new classes:
SynchronousTaskGraphRunner - used in some tests and by LayerTreeHostImpl
SingleThreadTaskGraphRunner - used in a number of tests (wrapped by
TestTaskGraphRunner) and by GpuProcessTransportFactory and
CompositorImplAndroid.

Additionally RasterWorkerPool has been re-worked to use the new helper.

Let me know how this looks so far. Thanks!

[ericrk, 2015-11-18 19:18:50]

Description was changed from

==========
wip - task graph runner refactor

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Interface-izes TaskGraphRunner and moves implementation helpers to
TaskGraphWorkQueue.

There are two new classes:
SynchronousTaskGraphRunner - used in some tests and by LayerTreeHostImpl
SingleThreadTaskGraphRunner - used in a number of tests (wrapped by
TestTaskGraphRunner) and by GpuProcessTransportFactory and
CompositorImplAndroid.

Additionally RasterWorkerPool has been re-worked to use the new helper.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[reveman, 2015-11-19 16:05:46]

nice refactor! just some nits and comments below

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/single_thread...
File cc/raster/single_thread_task_graph_runner.cc (right):

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/single_thread...
cc/raster/single_thread_task_graph_runner.cc:29: // Shutdown raster threads.
nit: "Shutdown threads" or maybe just remove this as it doesn't add much

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/single_thread...
File cc/raster/single_thread_task_graph_runner.h (right):

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/single_thread...
cc/raster/single_thread_task_graph_runner.h:42: scoped_ptr<base::SimpleThread>
raster_thread_;
nit: s/raster_thread_/thread_/ as this is not strictly for raster anymore

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/synchronous_t...
File cc/raster/synchronous_task_graph_runner.cc (right):

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/synchronous_t...
cc/raster/synchronous_task_graph_runner.cc:41:
!TaskGraphWorkQueue::HasFinishedRunningTasksInNamespace(task_namespace))
heh, cl format is not always making the best choices. nit: add {} as this is now
a multi-line while statement

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/task_graph_ru...
File cc/raster/task_graph_runner.h (right):

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/task_graph_ru...
cc/raster/task_graph_runner.h:133: virtual ~TaskGraphRunner() {}
nit: please make this protected if possible. code that access this interface
doesn't need the ability to delete it afaik

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/task_graph_ru...
cc/raster/task_graph_runner.h:137: static NamespaceToken GetNamespaceToken();
Could this be non-static? No reason for namespace tokens to be globally unique

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/task_graph_ru...
cc/raster/task_graph_runner.h:156: static bool DependencyMismatch(const
TaskGraph* graph);
can this be part of TaskGraphWorkQueue instead?

[ericrk, 2015-11-21 00:10:47]

Patchset #6 (id:100001) has been deleted

[ericrk, 2015-11-23 18:43:37]

Addressed the comments - needed to do a rebase afterwards, so might be easiest
to diff with second to last CL - not sure.

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/single_thread...
File cc/raster/single_thread_task_graph_runner.cc (right):

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/single_thread...
cc/raster/single_thread_task_graph_runner.cc:29: // Shutdown raster threads.
On 2015/11/19 16:05:46, reveman wrote:
> nit: "Shutdown threads" or maybe just remove this as it doesn't add much

Done.

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/single_thread...
File cc/raster/single_thread_task_graph_runner.h (right):

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/single_thread...
cc/raster/single_thread_task_graph_runner.h:42: scoped_ptr<base::SimpleThread>
raster_thread_;
On 2015/11/19 16:05:46, reveman wrote:
> nit: s/raster_thread_/thread_/ as this is not strictly for raster anymore

Done.

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/single_thread...
cc/raster/single_thread_task_graph_runner.h:42: scoped_ptr<base::SimpleThread>
raster_thread_;
On 2015/11/19 16:05:46, reveman wrote:
> nit: s/raster_thread_/thread_/ as this is not strictly for raster anymore

Done.

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/synchronous_t...
File cc/raster/synchronous_task_graph_runner.cc (right):

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/synchronous_t...
cc/raster/synchronous_task_graph_runner.cc:41:
!TaskGraphWorkQueue::HasFinishedRunningTasksInNamespace(task_namespace))
On 2015/11/19 16:05:46, reveman wrote:
> heh, cl format is not always making the best choices. nit: add {} as this is
now
> a multi-line while statement

yup... heh

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/task_graph_ru...
File cc/raster/task_graph_runner.h (right):

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/task_graph_ru...
cc/raster/task_graph_runner.h:133: virtual ~TaskGraphRunner() {}
On 2015/11/19 16:05:46, reveman wrote:
> nit: please make this protected if possible. code that access this interface
> doesn't need the ability to delete it afaik

Done.

https://codereview.chromium.org/1449133002/diff/80001/cc/raster/task_graph_ru...
cc/raster/task_graph_runner.h:137: static NamespaceToken GetNamespaceToken();
On 2015/11/19 16:05:46, reveman wrote:
> Could this be non-static? No reason for namespace tokens to be globally unique

Moved to TaskGraphWorkQueue as non-static

[reveman, 2015-11-23 19:47:06]

lgtm

https://codereview.chromium.org/1449133002/diff/160001/cc/test/test_task_grap...
File cc/test/test_task_graph_runner.cc (right):

https://codereview.chromium.org/1449133002/diff/160001/cc/test/test_task_grap...
cc/test/test_task_graph_runner.cc:10: Start("TestTaskGraphRunner1",
base::SimpleThread::Options());
nit: TestTaskGraphRunner, no need for the "1" suffix

Fyi, some telemetry tests depend on the name of these threads having the
"CompositorTileWorker" prefix. Pretty sure TestTaskGraphRunner is not used by
any telemetry tests so this should be fine.

[ericrk, 2015-11-23 21:45:49]

ericrk@chromium.org changed reviewers:
+ sievers@chromium.org

[ericrk, 2015-11-23 21:46:32]

ericrk@chromium.org changed reviewers:
+ dtrainor@chromium.org

[ericrk, 2015-11-23 21:47:22]

ericrk@chromium.org changed reviewers:
+ fsamuel@google.com

[ericrk, 2015-11-23 21:47:45]

ericrk@chromium.org changed reviewers:
+ fsamuel@chromium.org
- fsamuel@google.com

[ericrk, 2015-11-23 21:49:50]

The remaining code should be fairly straightforward - Adding more reviewers for
owners:

sievers for content/
fsamuel for mus/
dtrainor for blimp/

https://codereview.chromium.org/1449133002/diff/160001/cc/test/test_task_grap...
File cc/test/test_task_graph_runner.cc (right):

https://codereview.chromium.org/1449133002/diff/160001/cc/test/test_task_grap...
cc/test/test_task_graph_runner.cc:10: Start("TestTaskGraphRunner1",
base::SimpleThread::Options());
On 2015/11/23 19:47:06, reveman wrote:
> nit: TestTaskGraphRunner, no need for the "1" suffix
> 
> Fyi, some telemetry tests depend on the name of these threads having the
> "CompositorTileWorker" prefix. Pretty sure TestTaskGraphRunner is not used by
> any telemetry tests so this should be fine.

Done.

[David Trainor- moved to gerrit, 2015-11-23 21:51:56]

blimp/ lgtm

[Fady Samuel, 2015-11-23 21:53:50]

mus lgtm

[ericrk, 2015-11-24 19:04:10]

ericrk@chromium.org changed reviewers:
- sievers@chromium.org

[ericrk, 2015-11-24 19:04:56]

ericrk@chromium.org changed reviewers:
+ sievers@chromium.org

[no sievers, 2015-11-25 00:03:34]

lgtm

https://codereview.chromium.org/1449133002/diff/180001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1449133002/diff/180001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:100:
task_graph_runner_->Start("CompositorTileWorker1",
nit: how about 'BrowserCompositorTileWorker'?

https://codereview.chromium.org/1449133002/diff/180001/content/renderer/raste...
File content/renderer/raster_worker_pool.cc (right):

https://codereview.chromium.org/1449133002/diff/180001/content/renderer/raste...
content/renderer/raster_worker_pool.cc:122: has_ready_to_run_tasks_cv_.Signal();
why not Broadcast()?

https://codereview.chromium.org/1449133002/diff/180001/content/renderer/raste...
content/renderer/raster_worker_pool.cc:278: // There may be more work available,
so wake up another worker thread.
can you check if there is actually more work? seems overkill to wake up all
threads with each task

https://codereview.chromium.org/1449133002/diff/180001/content/renderer/raste...
content/renderer/raster_worker_pool.cc:295: // If namespace has finished running
all tasks, wake up origin thread.
there will only ever be one thread waiting right?

[ericrk, 2015-11-25 01:39:55]

ericrk@chromium.org changed reviewers:
+ danakj@chromium.org

[ericrk, 2015-11-25 02:09:52]

+danakj for base owners

https://codereview.chromium.org/1449133002/diff/180001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1449133002/diff/180001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:100:
task_graph_runner_->Start("CompositorTileWorker1",
On 2015/11/25 00:03:34, sievers wrote:
> nit: how about 'BrowserCompositorTileWorker'?

Done.

https://codereview.chromium.org/1449133002/diff/180001/content/renderer/raste...
File content/renderer/raster_worker_pool.cc (right):

https://codereview.chromium.org/1449133002/diff/180001/content/renderer/raste...
content/renderer/raster_worker_pool.cc:122: has_ready_to_run_tasks_cv_.Signal();
On 2015/11/25 00:03:34, sievers wrote:
> why not Broadcast()?

no reason - that does clean things up a bit.

https://codereview.chromium.org/1449133002/diff/180001/content/renderer/raste...
content/renderer/raster_worker_pool.cc:183: has_ready_to_run_tasks_cv_.Signal();
We no longer need this with the broadcast.

https://codereview.chromium.org/1449133002/diff/180001/content/renderer/raste...
content/renderer/raster_worker_pool.cc:278: // There may be more work available,
so wake up another worker thread.
On 2015/11/25 00:03:34, sievers wrote:
> can you check if there is actually more work? seems overkill to wake up all
> threads with each task

Done.

https://codereview.chromium.org/1449133002/diff/180001/content/renderer/raste...
content/renderer/raster_worker_pool.cc:295: // If namespace has finished running
all tasks, wake up origin thread.
On 2015/11/25 00:03:34, sievers wrote:
> there will only ever be one thread waiting right?

Right now there's nothing that stops multiple threads from adding work to a
single RasterWorkerPool under different namespaces - I know that media uses this
as well - not sure if it calls from its own threads. Reveman may know.

This matches the existing logic in TaskGraphRunner (which was ported here), so I
expect this to be fine.

[reveman, 2015-11-25 02:30:07]

https://codereview.chromium.org/1449133002/diff/180001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1449133002/diff/180001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:100:
task_graph_runner_->Start("CompositorTileWorker1",
On 2015/11/25 at 02:09:52, ericrk wrote:
> On 2015/11/25 00:03:34, sievers wrote:
> > nit: how about 'BrowserCompositorTileWorker'?
> 
> Done.

Did you verify that this doesn't break our telemetry tests that rely on specific
thread names to capture cpu usage?

https://codereview.chromium.org/1449133002/diff/180001/content/renderer/raste...
File content/renderer/raster_worker_pool.cc (right):

https://codereview.chromium.org/1449133002/diff/180001/content/renderer/raste...
content/renderer/raster_worker_pool.cc:295: // If namespace has finished running
all tasks, wake up origin thread.
On 2015/11/25 at 02:09:52, ericrk wrote:
> On 2015/11/25 00:03:34, sievers wrote:
> > there will only ever be one thread waiting right?
> 
> Right now there's nothing that stops multiple threads from adding work to a
single RasterWorkerPool under different namespaces - I know that media uses this
as well - not sure if it calls from its own threads. Reveman may know.
> 
> This matches the existing logic in TaskGraphRunner (which was ported here), so
I expect this to be fine.

There can be multiple threads waiting. I would recommend not changing any of the
logic as part of this patch and keep everything just like before and instead
make improvements to the code in follow up patches.

[ericrk, 2015-11-25 19:17:52]

Rolling back the changes based on siever's feedback  - moving these to a new CL
so they're easier to bisect if they cause regressions in beta. Trying to keep
logic matching original as closely as possible in this CL.

https://codereview.chromium.org/1449133002/diff/180001/content/browser/compos...
File content/browser/compositor/gpu_process_transport_factory.cc (right):

https://codereview.chromium.org/1449133002/diff/180001/content/browser/compos...
content/browser/compositor/gpu_process_transport_factory.cc:100:
task_graph_runner_->Start("CompositorTileWorker1",
On 2015/11/25 02:30:07, reveman wrote:
> On 2015/11/25 at 02:09:52, ericrk wrote:
> > On 2015/11/25 00:03:34, sievers wrote:
> > > nit: how about 'BrowserCompositorTileWorker'?
> > 
> > Done.
> 
> Did you verify that this doesn't break our telemetry tests that rely on
specific
> thread names to capture cpu usage?

This does indeed mess up tracing - I'll switch this back for now.

[danakj, 2015-11-30 23:54:27]

Just moving the use of ScopedAllowWait, LGTM

https://codereview.chromium.org/1449133002/diff/260001/base/threading/thread_...
File base/threading/thread_restrictions.h (right):

https://codereview.chromium.org/1449133002/diff/260001/base/threading/thread_...
base/threading/thread_restrictions.h:26: class TaskGraphRunner;
remove this?

[ericrk, 2015-11-30 23:58:48]

https://codereview.chromium.org/1449133002/diff/260001/base/threading/thread_...
File base/threading/thread_restrictions.h (right):

https://codereview.chromium.org/1449133002/diff/260001/base/threading/thread_...
base/threading/thread_restrictions.h:26: class TaskGraphRunner;
On 2015/11/30 23:54:27, danakj (behind on reviews) wrote:
> remove this?

Done.

[ericrk, 2015-11-30 23:58:52]

The CQ bit was checked by ericrk@chromium.org

[ericrk, 2015-11-30 23:58:53]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, fsamuel@chromium.org, dtrainor@chromium.org,
sievers@chromium.org, danakj@chromium.org
Link to the patchset: https://codereview.chromium.org/1449133002/#ps280001
(title: "feedback")

[commit-bot: I haz the power, 2015-12-01 00:05:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1449133002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1449133002/280001

[commit-bot: I haz the power, 2015-12-01 03:54:03]

Description was changed from

==========
Interface-izes TaskGraphRunner and moves implementation helpers to
TaskGraphWorkQueue.

There are two new classes:
SynchronousTaskGraphRunner - used in some tests and by LayerTreeHostImpl
SingleThreadTaskGraphRunner - used in a number of tests (wrapped by
TestTaskGraphRunner) and by GpuProcessTransportFactory and
CompositorImplAndroid.

Additionally RasterWorkerPool has been re-worked to use the new helper.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Interface-izes TaskGraphRunner and moves implementation helpers to
TaskGraphWorkQueue.

There are two new classes:
SynchronousTaskGraphRunner - used in some tests and by LayerTreeHostImpl
SingleThreadTaskGraphRunner - used in a number of tests (wrapped by
TestTaskGraphRunner) and by GpuProcessTransportFactory and
CompositorImplAndroid.

Additionally RasterWorkerPool has been re-worked to use the new helper.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2015-12-01 03:54:04]

Committed patchset #14 (id:280001)

[commit-bot: I haz the power, 2015-12-01 03:55:03]

Description was changed from

==========
Interface-izes TaskGraphRunner and moves implementation helpers to
TaskGraphWorkQueue.

There are two new classes:
SynchronousTaskGraphRunner - used in some tests and by LayerTreeHostImpl
SingleThreadTaskGraphRunner - used in a number of tests (wrapped by
TestTaskGraphRunner) and by GpuProcessTransportFactory and
CompositorImplAndroid.

Additionally RasterWorkerPool has been re-worked to use the new helper.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Interface-izes TaskGraphRunner and moves implementation helpers to
TaskGraphWorkQueue.

There are two new classes:
SynchronousTaskGraphRunner - used in some tests and by LayerTreeHostImpl
SingleThreadTaskGraphRunner - used in a number of tests (wrapped by
TestTaskGraphRunner) and by GpuProcessTransportFactory and
CompositorImplAndroid.

Additionally RasterWorkerPool has been re-worked to use the new helper.

BUG=
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/4e3aa5a539e3774b51b998dc60eac42a97c2cfb8
Cr-Commit-Position: refs/heads/master@{#362338}
==========

[commit-bot: I haz the power, 2015-12-01 03:55:04]

Patchset 14 (id:??) landed as
https://crrev.com/4e3aa5a539e3774b51b998dc60eac42a97c2cfb8
Cr-Commit-Position: refs/heads/master@{#362338}