cc: Implement states for Task for stricter control.

cc: Implement states for Task for stricter control.

Implement states for task which gives stricter control for managing
states of a task in single variable vs. multiple variables. The states
are as given below.

  NEW, SCHEDULED, RUNNING, FINISHED, CANCELED

As TaskGraphWorkQueue manages life cycle of task, this patch also moves
state management to TaskGraphWorkQueue from all TaskGraphRunners, i.e.
WillRun() and DidRun() are moved to places where |work_queue_| inserts
task in |running_tasks| and |completed_tasks_| respectively. This
also ensures that task if gets scheduled must end in either FINISHED or
CANCELED state and will help catch any bugs which go out of expected
behavior for task state.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/d67b4b01e6b969df93348eaff249f1d190a72afd
Cr-Commit-Position: refs/heads/master@{#389538}

[prashant.n, 2016-04-20 13:25:12]

Description was changed from

==========
cc: Implement states for Task for stricter control.

Implement states for task which gives stricter control for managing
states of a task in single variable vs. multiple variables. As
TaskGraphWorkQueue manages life cycle of task, this patch also moves
state management to TaskGraphWorkQueue.

BUG=599863
==========

to

==========
cc: Implement states for Task for stricter control.

Implement states for task which gives stricter control for managing
states of a task in single variable vs. multiple variables. As
TaskGraphWorkQueue manages life cycle of task, this patch also moves
state management to TaskGraphWorkQueue.

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

[prashant.n, 2016-04-20 13:28:42]

Description was changed from

==========
cc: Implement states for Task for stricter control.

Implement states for task which gives stricter control for managing
states of a task in single variable vs. multiple variables. As
TaskGraphWorkQueue manages life cycle of task, this patch also moves
state management to TaskGraphWorkQueue.

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

to

==========
cc: Implement states for Task for stricter control.

Implement states for task which gives stricter control for managing
states of a task in single variable vs. multiple variables. As
TaskGraphWorkQueue manages life cycle of task, this patch also moves
state management to TaskGraphWorkQueue. This improves semantics of
usage.

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

[prashant.n, 2016-04-20 13:32:53]

prashant.n@samsung.com changed reviewers:
+ ericrk@chromium.org, reveman@chromium.org, vmpstr@chromium.org

[prashant.n, 2016-04-20 13:32:54]

Wip patch for implementing states for task.

[prashant.n, 2016-04-21 09:40:08]

The CQ bit was checked by prashant.n@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 09:40:15]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903733003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903733003/60001

[commit-bot: I haz the power, 2016-04-21 09:44:27]

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

[commit-bot: I haz the power, 2016-04-21 09:44:28]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[prashant.n, 2016-04-21 09:53:18]

Description was changed from

==========
cc: Implement states for Task for stricter control.

Implement states for task which gives stricter control for managing
states of a task in single variable vs. multiple variables. As
TaskGraphWorkQueue manages life cycle of task, this patch also moves
state management to TaskGraphWorkQueue. This improves semantics of
usage.

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

to

==========
cc: Implement states for Task for stricter control.

Implement states for task which gives stricter control for managing
states of a task in single variable vs. multiple variables. The states
are as given below.

  NEW, SCHEDULED, RUNNING, FINISHED, CANCELED

As TaskGraphWorkQueue manages life cycle of task, this patch also moves
state management to TaskGraphWorkQueue from all TaskGraphRunners, i.e.
WillRun() and DidRun() are moved to places where |work_queue_| inserts
task in |running_tasks| and |completed_tasks_| respectively. This
also ensures that task if gets scheduled must end in either FINISHED or
CANCELED state and will help catch any bugs which go out of expected
behavior for task state.

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

[prashant.n, 2016-04-21 11:25:28]

The CQ bit was checked by prashant.n@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-04-21 11:25:42]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1903733003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1903733003/60001

[commit-bot: I haz the power, 2016-04-21 12:26:46]

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

[commit-bot: I haz the power, 2016-04-21 12:26:46]

Dry run: This issue passed the CQ dry run.

[ericrk, 2016-04-21 18:24:26]

Nice change. A few comments.

https://codereview.chromium.org/1903733003/diff/60001/cc/raster/task.cc
File cc/raster/task.cc (right):

https://codereview.chromium.org/1903733003/diff/60001/cc/raster/task.cc#newco...
cc/raster/task.cc:21: return (value_ == SCHEDULED);
nit: why the extra ()?

https://codereview.chromium.org/1903733003/diff/60001/cc/raster/task.cc#newco...
cc/raster/task.cc:45: DCHECK(value_ != RUNNING && value_ != FINISHED)
This check is redundant with the above. (they both fail or neither fails).

https://codereview.chromium.org/1903733003/diff/60001/cc/raster/task.cc#newco...
cc/raster/task.cc:57: DCHECK(value_ == NEW || (value_ == SCHEDULED && value_ !=
RUNNING))
Should read DCHECK(value_ == NEW || value_ == SCHEDULED), the last part is
redundant.

https://codereview.chromium.org/1903733003/diff/60001/cc/raster/task.h
File cc/raster/task.h (right):

https://codereview.chromium.org/1903733003/diff/60001/cc/raster/task.h#newcode47
cc/raster/task.h:47: enum { NEW, SCHEDULED, RUNNING, FINISHED, CANCELED }
value_;
please use an "enum class" for extra safety.

[prashant.n, 2016-04-22 03:57:56]

https://codereview.chromium.org/1903733003/diff/60001/cc/raster/task.cc
File cc/raster/task.cc (right):

https://codereview.chromium.org/1903733003/diff/60001/cc/raster/task.cc#newco...
cc/raster/task.cc:21: return (value_ == SCHEDULED);
On 2016/04/21 18:24:25, ericrk wrote:
> nit: why the extra ()?

For readability I added.

I guess chromium does not () at many places. I'll change this.

https://codereview.chromium.org/1903733003/diff/60001/cc/raster/task.h
File cc/raster/task.h (right):

https://codereview.chromium.org/1903733003/diff/60001/cc/raster/task.h#newcode47
cc/raster/task.h:47: enum { NEW, SCHEDULED, RUNNING, FINISHED, CANCELED }
value_;
I thought using simple enum as it is private.

I'll add class.

[prashant.n, 2016-04-22 04:10:02]

ptal

[prashant.n, 2016-04-22 14:37:50]

prashant.n@samsung.com changed reviewers:
+ esprehn@chromium.org

[prashant.n, 2016-04-22 14:37:50]

+ esprehn@ for RasterWorkerPool.

[esprehn, 2016-04-23 02:10:31]

esprehn@chromium.org changed reviewers:
+ jam@chromium.org

[esprehn, 2016-04-23 02:10:31]

Jam@ is maybe a better reviewer. It's strange that the worker pool isn't in some
component or cc itself too...

[prashant.n, 2016-04-23 02:53:37]

On 2016/04/23 02:10:31, esprehn wrote:
> Jam@ is maybe a better reviewer. It's strange that the worker pool isn't in
some
> component or cc itself too...

Currently all of taskgraph related functionality is implemented in cc/, which
ideally should be in base/. But rasterworkerpool as it is used outside cc/ like
media/, so was put in content.

[jam, 2016-04-25 15:45:04]

jam@chromium.org changed reviewers:
+ piman@chromium.org

[jam, 2016-04-25 15:46:04]

On 2016/04/23 02:53:37, prashant.n wrote:
> On 2016/04/23 02:10:31, esprehn wrote:
> > Jam@ is maybe a better reviewer. It's strange that the worker pool isn't in
> some
> > component or cc itself too...
> 
> Currently all of taskgraph related functionality is implemented in cc/, which
> ideally should be in base/. But rasterworkerpool as it is used outside cc/
like
> media/, so was put in content.

+piman who's an owner here and much more familiar

I'm not familiar with this area; reveman and piman were reviewers on change that
split this out into a separate file so they'll be better reviewers.

[ericrk, 2016-04-25 16:21:06]

cc lgtm

Raster worker pool also lgtm, but you'll need to wait for owners there.

[piman, 2016-04-25 18:24:16]

lgtm for content

[prashant.n, 2016-04-25 18:30:37]

The CQ bit was checked by prashant.n@samsung.com

[commit-bot: I haz the power, 2016-04-25 18:31:00]

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

[commit-bot: I haz the power, 2016-04-25 19:36:19]

Description was changed from

==========
cc: Implement states for Task for stricter control.

Implement states for task which gives stricter control for managing
states of a task in single variable vs. multiple variables. The states
are as given below.

  NEW, SCHEDULED, RUNNING, FINISHED, CANCELED

As TaskGraphWorkQueue manages life cycle of task, this patch also moves
state management to TaskGraphWorkQueue from all TaskGraphRunners, i.e.
WillRun() and DidRun() are moved to places where |work_queue_| inserts
task in |running_tasks| and |completed_tasks_| respectively. This
also ensures that task if gets scheduled must end in either FINISHED or
CANCELED state and will help catch any bugs which go out of expected
behavior for task state.

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

to

==========
cc: Implement states for Task for stricter control.

Implement states for task which gives stricter control for managing
states of a task in single variable vs. multiple variables. The states
are as given below.

  NEW, SCHEDULED, RUNNING, FINISHED, CANCELED

As TaskGraphWorkQueue manages life cycle of task, this patch also moves
state management to TaskGraphWorkQueue from all TaskGraphRunners, i.e.
WillRun() and DidRun() are moved to places where |work_queue_| inserts
task in |running_tasks| and |completed_tasks_| respectively. This
also ensures that task if gets scheduled must end in either FINISHED or
CANCELED state and will help catch any bugs which go out of expected
behavior for task state.

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

[commit-bot: I haz the power, 2016-04-25 19:36:21]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-04-25 19:37:16]

Description was changed from

==========
cc: Implement states for Task for stricter control.

Implement states for task which gives stricter control for managing
states of a task in single variable vs. multiple variables. The states
are as given below.

  NEW, SCHEDULED, RUNNING, FINISHED, CANCELED

As TaskGraphWorkQueue manages life cycle of task, this patch also moves
state management to TaskGraphWorkQueue from all TaskGraphRunners, i.e.
WillRun() and DidRun() are moved to places where |work_queue_| inserts
task in |running_tasks| and |completed_tasks_| respectively. This
also ensures that task if gets scheduled must end in either FINISHED or
CANCELED state and will help catch any bugs which go out of expected
behavior for task state.

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

to

==========
cc: Implement states for Task for stricter control.

Implement states for task which gives stricter control for managing
states of a task in single variable vs. multiple variables. The states
are as given below.

  NEW, SCHEDULED, RUNNING, FINISHED, CANCELED

As TaskGraphWorkQueue manages life cycle of task, this patch also moves
state management to TaskGraphWorkQueue from all TaskGraphRunners, i.e.
WillRun() and DidRun() are moved to places where |work_queue_| inserts
task in |running_tasks| and |completed_tasks_| respectively. This
also ensures that task if gets scheduled must end in either FINISHED or
CANCELED state and will help catch any bugs which go out of expected
behavior for task state.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/d67b4b01e6b969df93348eaff249f1d190a72afd
Cr-Commit-Position: refs/heads/master@{#389538}
==========

[commit-bot: I haz the power, 2016-04-25 19:37:18]

Patchset 5 (id:??) landed as
https://crrev.com/d67b4b01e6b969df93348eaff249f1d190a72afd
Cr-Commit-Position: refs/heads/master@{#389538}

[prashant.n, 2016-04-28 07:33:25]

This crashes few perf tests, fixing soon in
https://codereview.chromium.org/1910213005/

[prashant.n, 2016-04-29 08:34:43]

On 2016/04/28 07:33:25, prashant.n wrote:
> This crashes few perf tests, fixing soon in

The crashes are without this CL also. Filed a bug for it
https://bugs.chromium.org/p/chromium/issues/detail?id=607818