Re-land: cc: Remove WorkerPool class and instead use TaskGraphRunner directly.

Re-land: cc: Remove WorkerPool class and instead use TaskGraphRunner directly.

Removes the unnecessary WorkerPool layer. This makes the job
of the TaskGraphRunner more clear and will allow us to remove
some complexity and unnecessary heap allocations from the
RasterWorkerPool code in follow up patches.

Also moves the kNumRasterTasks switch to content/.

Refactor only, no changes in behavior.

BUG=331844
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=246554

[reveman, 2014-01-18 15:18:32]

I'm planning to make RasterWorkerPoolTask an internal::Task as a follow up and
in that way reduce heap allocations and complexity of
RasterWorkerPool::ScheduleTasks().

Please take a look.

[sohanjg, 2014-01-21 09:41:08]

lgtm

Looks good to remove unnecessary WorkerPool abstraction.

https://codereview.chromium.org/141163019/diff/160001/cc/resources/pixel_buff...
File cc/resources/pixel_buffer_raster_worker_pool.h (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/pixel_buff...
cc/resources/pixel_buffer_raster_worker_pool.h:12: #include
"base/cancelable_callback.h"
nit: is this needed ?

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:94: reply_(reply) {
nit: do we need this line here ?

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:318: reply_(reply) {
nit: do we need this line here ?

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:650: internal::Task::Vector completed_tasks;
nit: can we typedef internal::Task::Vector ?

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:794: internal::GraphNode::Map::iterator
decode_it = graph->find(decode_task);
nit: And typedef internal::GraphNode::Map too ?

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:794: internal::GraphNode::Map::iterator
decode_it = graph->find(decode_task);
nit: And typedef internal::GraphNode::Map too ?

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.h:67: const internal::Task::Vector&
dependencies() const { return dependencies_; }
How is it helpful to change WorkerPoolTask to Task ?

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.h:130: internal::Task::Vector tasks_;
And here from RasterWorkerPoolTask to Task ?

[reveman, 2014-01-21 15:37:46]

https://codereview.chromium.org/141163019/diff/160001/cc/resources/pixel_buff...
File cc/resources/pixel_buffer_raster_worker_pool.h (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/pixel_buff...
cc/resources/pixel_buffer_raster_worker_pool.h:12: #include
"base/cancelable_callback.h"
On 2014/01/21 09:41:08, sohanjg wrote:
> nit: is this needed ?

Needed for base::CancelableClosure below.

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:94: reply_(reply) {
On 2014/01/21 09:41:08, sohanjg wrote:
> nit: do we need this line here ?

I'm trying to make the code consistent in that only functions that completely
fit on one line get to use "{}". A function that has a line break somewhere (in
parameter list, initialization list...) gets a line break between "{" and "}".
This is not in the style guide afaik and my choice here is somewhat arbitrary
but it made the most sense to me. The important part is making it consistent.

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:650: internal::Task::Vector completed_tasks;
On 2014/01/21 09:41:08, sohanjg wrote:
> nit: can we typedef internal::Task::Vector ?

To what? I think it's good to not remove the internal:: namespace and it doesn't
get much simpler than Task::Vector.

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:794: internal::GraphNode::Map::iterator
decode_it = graph->find(decode_task);
On 2014/01/21 09:41:08, sohanjg wrote:
> nit: And typedef internal::GraphNode::Map too ?

what are you suggesting we typedef to? I like to keep internal:: as it makes it
clear that this should not leak into the public API.

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.h:67: const internal::Task::Vector&
dependencies() const { return dependencies_; }
On 2014/01/21 09:41:08, sohanjg wrote:
> How is it helpful to change WorkerPoolTask to Task ?

First, there's no base WorkerPool class anymore so I don't think "WorkerPool"
should be part of the name for the tasks that TaskGraphRunner process. Second,
this makes the tasks known to TaskGraphRunner simpler as it no longer has to be
aware of completion callbacks. That's now something that RasterWorkerPool can
implement by creating a internal::Task subclass. See the WorkerPoolTask class
above.

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.h:130: internal::Task::Vector tasks_;
On 2014/01/21 09:41:08, sohanjg wrote:
> And here from RasterWorkerPoolTask to Task ?

Type didn't change. Just removed confusing typedef now that we have
internal::Task::Vector.

[danakj, 2014-01-21 15:47:04]

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:94: reply_(reply) {
On 2014/01/21 15:37:48, David Reveman wrote:
> On 2014/01/21 09:41:08, sohanjg wrote:
> > nit: do we need this line here ?
> 
> I'm trying to make the code consistent in that only functions that completely
> fit on one line get to use "{}". A function that has a line break somewhere
(in
> parameter list, initialization list...) gets a line break between "{" and "}".
> This is not in the style guide afaik and my choice here is somewhat arbitrary
> but it made the most sense to me. The important part is making it consistent.

FWIW the easiest way to be consistent is to just run git cl format and let it
figure this stuff out. Soon a presubmit warning will be added to cc if code
doesn't match what Git cl format would do.

[reveman, 2014-01-21 16:10:44]

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:94: reply_(reply) {
On 2014/01/21 15:47:05, danakj wrote:
> On 2014/01/21 15:37:48, David Reveman wrote:
> > On 2014/01/21 09:41:08, sohanjg wrote:
> > > nit: do we need this line here ?
> > 
> > I'm trying to make the code consistent in that only functions that
completely
> > fit on one line get to use "{}". A function that has a line break somewhere
> (in
> > parameter list, initialization list...) gets a line break between "{" and
"}".
> > This is not in the style guide afaik and my choice here is somewhat
arbitrary
> > but it made the most sense to me. The important part is making it
consistent.
> 
> FWIW the easiest way to be consistent is to just run git cl format and let it
> figure this stuff out. Soon a presubmit warning will be added to cc if code
> doesn't match what Git cl format would do.  

Good point. I'm not going to "cl format" this patch as it results in huge
changes to chrome/ that makes this patch really hard to read. What I'll do is
remove any style changes from this patch that are not what "cl format" prefer
and then do a style-only change where I "cl format" all worker pool code in a
follow up.

[vmpstr, 2014-01-21 18:14:03]

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.h:29: class CC_EXPORT WorkerPoolTask : public
Task {
Do you think it's worth it to just merge the *Complete stuff into
internal::Task? I don't really think there will be a case where we want to run a
task and not have some sort of completion indication. If you know of a real case
where that would be (or rather would not be) needed, then this is fine. Other
than that, I would prefer to limit the number of task objects that we have. This
would also eliminate the need for static_casts that convert a task into a
workerpooltaks.

One issue of course is that the task runner doesn't actually complete tasks, but
maybe we can do that as well? That is, instead of collect in this class and then
call complete, it could just complete by itself? (we can swap/copy to a temp
vector and release the internal lock so that performance will be unchanged)

[alokp, 2014-01-21 18:42:48]

lgtm. Minor comments.

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:431: int g_num_raster_threads = 0;
make these static members of RasterTaskGraphRunner or RasterWorkerPool?

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:561: DCHECK_EQ(0, g_num_raster_threads);
Are you trying to make sure that SetNumRasterThreads is called only once? Isn't
it better to do that by having a local static variable here. The you do not need
kDefaultNumRasterThreads. You could simply initialize g_num_raster_threads with
whatever the default calue should be.

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.h:29: class CC_EXPORT WorkerPoolTask : public
Task {
I agree. If Complete is the only extra, it seems worth moving it to
internal::Task.

https://codereview.chromium.org/141163019/diff/160001/cc/resources/task_graph...
File cc/resources/task_graph_runner.h (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/task_graph...
cc/resources/task_graph_runner.h:188: TaskNamespace* task_namespace) {
nit: this may fit in 80 chars.

[reveman, 2014-01-21 19:51:32]

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:318: reply_(reply) {
On 2014/01/21 09:41:08, sohanjg wrote:
> nit: do we need this line here ?

sohanjg, you're right. clang-format prefer it the old way so I removed this
change.

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:431: int g_num_raster_threads = 0;
On 2014/01/21 18:42:50, Alok Priyadarshi wrote:
> make these static members of RasterTaskGraphRunner or RasterWorkerPool?

I prefer anonymous namespace and hiding these from the .h when possible. What
would the benefit of moving them to RasterTaskGraphRunner be? It doesn't feel
like they belong there, imo.

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:561: DCHECK_EQ(0, g_num_raster_threads);
On 2014/01/21 18:42:50, Alok Priyadarshi wrote:
> Are you trying to make sure that SetNumRasterThreads is called only once?
Isn't
> it better to do that by having a local static variable here. The you do not
need
> kDefaultNumRasterThreads. You could simply initialize g_num_raster_threads
with
> whatever the default calue should be.

I also want to make sure it's not called after GetNumRasterThreads().

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.h:29: class CC_EXPORT WorkerPoolTask : public
Task {
On 2014/01/21 18:14:04, vmpstr wrote:
> Do you think it's worth it to just merge the *Complete stuff into
> internal::Task? I don't really think there will be a case where we want to run
a
> task and not have some sort of completion indication. If you know of a real
case
> where that would be (or rather would not be) needed, then this is fine. Other
> than that, I would prefer to limit the number of task objects that we have.
This
> would also eliminate the need for static_casts that convert a task into a
> workerpooltaks.
> 
> One issue of course is that the task runner doesn't actually complete tasks,
but
> maybe we can do that as well? That is, instead of collect in this class and
then
> call complete, it could just complete by itself? (we can swap/copy to a temp
> vector and release the internal lock so that performance will be unchanged)

The raster tasks passed to the TaskGraphRunner by PBRWP are a great example of
when we like to control completion processing outside TaskGraphRunner.

What I'd like to do is remove the need to create internal::RasterWorkerPoolTask
instances that are not internal::Task instances. The reason we have to do that
right now is that RasterWorkerPool instances are not in control over how
completed tasks are processed. By moving all this completion logic into the
RasterWorkerPool class instead of TaskGraphRunner, we're not only getting a
better separation of responsibilities but can also make our RasterWorkerPool
instances cleaner and more efficient. ie. no need to use anonymous functions to
process completion of raster tasks inside RasterWorkerPool instances.

This WorkerPoolTask class is temporary. Follow-up patch will merge this with the
RasterWorkerPoolTask. This was just a nice way for me to limit the size of this
patch while still getting the separation of logic that I'd like to be part of
it.

https://codereview.chromium.org/141163019/diff/160001/cc/resources/task_graph...
File cc/resources/task_graph_runner.h (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/task_graph...
cc/resources/task_graph_runner.h:188: TaskNamespace* task_namespace) {
On 2014/01/21 18:42:50, Alok Priyadarshi wrote:
> nit: this may fit in 80 chars.

It does seem like that would fit but clang-format doesn't like it that way so
I'll keep it like this as I'm planning to clang-format all these files in a
follow up.

[vmpstr, 2014-01-21 20:30:42]

lgtm

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/141163019/diff/160001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.h:29: class CC_EXPORT WorkerPoolTask : public
Task {
On 2014/01/21 19:51:32, David Reveman wrote:
> On 2014/01/21 18:14:04, vmpstr wrote:
> > Do you think it's worth it to just merge the *Complete stuff into
> > internal::Task? I don't really think there will be a case where we want to
run
> a
> > task and not have some sort of completion indication. If you know of a real
> case
> > where that would be (or rather would not be) needed, then this is fine.
Other
> > than that, I would prefer to limit the number of task objects that we have.
> This
> > would also eliminate the need for static_casts that convert a task into a
> > workerpooltaks.
> > 
> > One issue of course is that the task runner doesn't actually complete tasks,
> but
> > maybe we can do that as well? That is, instead of collect in this class and
> then
> > call complete, it could just complete by itself? (we can swap/copy to a temp
> > vector and release the internal lock so that performance will be unchanged)
> 
> The raster tasks passed to the TaskGraphRunner by PBRWP are a great example of
> when we like to control completion processing outside TaskGraphRunner.
> 
> What I'd like to do is remove the need to create
internal::RasterWorkerPoolTask
> instances that are not internal::Task instances. The reason we have to do that
> right now is that RasterWorkerPool instances are not in control over how
> completed tasks are processed. By moving all this completion logic into the
> RasterWorkerPool class instead of TaskGraphRunner, we're not only getting a
> better separation of responsibilities but can also make our RasterWorkerPool
> instances cleaner and more efficient. ie. no need to use anonymous functions
to
> process completion of raster tasks inside RasterWorkerPool instances.
> 
> This WorkerPoolTask class is temporary. Follow-up patch will merge this with
the
> RasterWorkerPoolTask. This was just a nice way for me to limit the size of
this
> patch while still getting the separation of logic that I'd like to be part of
> it.

Right... I forgot about the uploads in PBRWP.

[reveman, 2014-01-21 22:47:47]

+jamesr for content/renderer
+piman for chrome/

[piman, 2014-01-21 22:54:20]

lgtm

[commit-bot: I haz the power, 2014-01-21 22:56:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/141163019/440001

[jamesr, 2014-01-21 22:58:21]

lgtm2

[commit-bot: I haz the power, 2014-01-22 01:08:13]

Retried try job too often on win_rel for step(s) ash_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-22 05:12:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/141163019/440001

[commit-bot: I haz the power, 2014-01-22 10:50:46]

Change committed as 246284

[Mattias Nissler (ping if slow), 2014-01-22 11:39:58]

A revert of this CL has been created in
https://codereview.chromium.org/143003012/ by mnissler@chromium.org.

The reason for reverting is: Bad file mode for task_graph_runner.h:

/b/build/slave/Linux_ChromiumOS/build/src/cc/resources/task_graph_runner.h: Must
not have executable bit set


http://build.chromium.org/p/chromium.chromiumos/builders/Linux%20ChromiumOS%2....

[commit-bot: I haz the power, 2014-01-22 14:43:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/141163019/220002

[commit-bot: I haz the power, 2014-01-22 15:18:10]

Retried try job too often on linux_rel for step(s) content_browsertests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-22 15:19:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/141163019/220002

[commit-bot: I haz the power, 2014-01-23 06:02:03]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2014-01-23 06:04:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/reveman@chromium.org/141163019/220002

[commit-bot: I haz the power, 2014-01-23 09:46:52]

Change committed as 246554