cc: Simplify task and its derived classes.

cc: Simplify task and its derived classes.

This patch merges TileTask, ImageDecodeTask and RasterTask into one
class to create a single class capable of handling dependencies,
scheduling and completing and is moved to its own file. Now this new
task has following functions for doing job by the task.

  ScheduleOnOriginThread - must be called on origin thread
  CompleteOnOriginThread - must be called on origin thread
  RunOnWorkerThread - mab be called origin or worker thread

All the implementation classes like RasterTaskImpl,
ImageDecodeTaskImpl, etc. are derived directly from Task. The
TileTaskClient is renamed to RasterBufferProvider and
TileTaskRunner is inherited from it.

BUG=599863
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

[prashant.n, 2016-04-01 14:13:13]

Description was changed from

==========
cc: Refactor Task.

This patch renames Task to DependencyTask to exhibit what it does.
TileTask is merged into DependencyTask. Now this new task has following
functions for getting job done by the task.

  ScheduleOnOriginThread - must be called on origin thread
  CompleteOnOriginThread - must be called on origin thread
  RunOnWorkerThread - may be called origin or worker thread

The RasterTask and ImageDecodeTask are removed and their subclasses are
derived directly from DependencyTask.

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

to

==========
cc: Refactor Task.

This patch renames Task to DependencyTask to exhibit what it does.
TileTask is merged into DependencyTask. Now this new task has following
functions for getting job done by the task.

  ScheduleOnOriginThread - must be called on origin thread
  CompleteOnOriginThread - must be called on origin thread
  RunOnWorkerThread - may be called origin or worker thread

The RasterTask and ImageDecodeTask are removed and their subclasses are
derived directly from DependencyTask.

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

[prashant.n, 2016-04-01 14:19:25]

Description was changed from

==========
cc: Refactor Task.

This patch renames Task to DependencyTask to exhibit what it does.
TileTask is merged into DependencyTask. Now this new task has following
functions for getting job done by the task.

  ScheduleOnOriginThread - must be called on origin thread
  CompleteOnOriginThread - must be called on origin thread
  RunOnWorkerThread - may be called origin or worker thread

The RasterTask and ImageDecodeTask are removed and their subclasses are
derived directly from DependencyTask.

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

to

==========
cc: Refactor Task.

This patch renames Task to DependencyTask to exhibit what it does.
TileTask is merged into DependencyTask. Now this new task has following
functions for getting job done by the task.

  ScheduleOnOriginThread - must be called on origin thread
  CompleteOnOriginThread - must be called on origin thread
  RunOnWorkerThread - may be called origin or worker thread

The RasterTask and ImageDecodeTask are removed and their subclasses are
derived directly from DependencyTask.

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

[prashant.n, 2016-04-01 14:21:31]

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

[prashant.n, 2016-04-01 14:21:31]

ptal.

[prashant.n, 2016-04-01 14:23:39]

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.h
File cc/raster/dependency_task.h (right):

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.h...
cc/raster/dependency_task.h:27: virtual scoped_ptr<RasterBuffer>
AcquireBufferForRaster(
These methods will be made generic in subsequent patch and will be renamed as
OnScheduled(DependencyTask* task) and OnCompleted(DependencyTask* task)

[ericrk, 2016-04-01 21:34:31]

This seems like a reasonable simplification - I do like making dependencies more
generic. We may still want to keep some functionality RasterTask specific - see
my comments.

Thanks!

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.cc
File cc/raster/dependency_task.cc (right):

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.c...
cc/raster/dependency_task.cc:20: dependencies_.swap(*dependencies);
Rather than having them pass in a non-const * and then us taking memory this
way, let's make the arg by value and then we can just std::move into
dependencies.

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.h
File cc/raster/dependency_task.h (right):

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.h...
cc/raster/dependency_task.h:12: #include <map>
nit: map/algorithm are not used

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.h...
cc/raster/dependency_task.h:15: #include "base/logging.h"
nit: logging is not used.

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.h...
cc/raster/dependency_task.h:25: class CC_EXPORT DependencyTaskClient {
This does a lot of things other than dependencies, and the dependencies are
optional. Why not just call this Task?

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.h...
cc/raster/dependency_task.h:27: virtual scoped_ptr<RasterBuffer>
AcquireBufferForRaster(
On 2016/04/01 14:23:39, prashant.n wrote:
> These methods will be made generic in subsequent patch and will be renamed as
> OnScheduled(DependencyTask* task) and OnCompleted(DependencyTask* task)

While I agree that dependencies are a generic concept - I think this
DependencyTaskClient isn't really generic - only raster tasks actually make use
of this functionality - not sure we want to make this generic.

I think you could still do something like:
using RasterTask = DependencyTask;
class RasterTaskClient { ...

to make the division clear?

Unless you think we need this functionality for other kinds of dependency tasks
in the future... but I don't think so.

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.h...
cc/raster/dependency_task.h:73: explicit DependencyTask(DependencyTask::Vector*
dependencies);
maybe take by value to allow us to std::move the vector in here?

[prashant.n, 2016-04-01 23:48:05]

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.cc
File cc/raster/dependency_task.cc (right):

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.c...
cc/raster/dependency_task.cc:20: dependencies_.swap(*dependencies);
On 2016/04/01 21:34:30, ericrk wrote:
> Rather than having them pass in a non-const * and then us taking memory this
> way, let's make the arg by value and then we can just std::move into
> dependencies.

Okay, I'll check apply this modification.

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.h
File cc/raster/dependency_task.h (right):

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.h...
cc/raster/dependency_task.h:25: class CC_EXPORT DependencyTaskClient {
On 2016/04/01 21:34:30, ericrk wrote:
> This does a lot of things other than dependencies, and the dependencies are
> optional. Why not just call this Task?

I called this as DependencyTask to differetiate from Task that it is run by task
runner who is capable of handling dependecies, e.g. TaskGraphRunner (which is to
be renamed as DependencyTaskRunner.)

Graph is iternal concept to handle dependecies. We can abstract it by may be
having following iterfaces

BeginSchdeduling()
AddTask(task, dependency)
AddTask(task, dependency)
.
.
.
EndScheduling()

DependencyTask means either it can have dependencies or not. Later if possible
we can make this more generic to be moved to base/.

https://codereview.chromium.org/1854723002/diff/1/cc/raster/dependency_task.h...
cc/raster/dependency_task.h:27: virtual scoped_ptr<RasterBuffer>
AcquireBufferForRaster(
On 2016/04/01 21:34:30, ericrk wrote:
> On 2016/04/01 14:23:39, prashant.n wrote:
> > These methods will be made generic in subsequent patch and will be renamed
as
> > OnScheduled(DependencyTask* task) and OnCompleted(DependencyTask* task)
> 
> While I agree that dependencies are a generic concept - I think this
> DependencyTaskClient isn't really generic - only raster tasks actually make
use
> of this functionality - not sure we want to make this generic.
> 
> I think you could still do something like:
> using RasterTask = DependencyTask;
> class RasterTaskClient { ...
> 
> to make the division clear?
> 
> Unless you think we need this functionality for other kinds of dependency
tasks
> in the future... but I don't think so.

Yes, I agree with you. Scheduling and Completing the task is generic concept.
May be we can do something like this -

virtual void ScheduleOnOriginThread() = 0;
virtual void CompleteOnOriginThread() = 0;

Removing DependencyTaskClient as argument.

And subclass RasterTaskImpl : DependencyTask
can take one more argument as RasterTaskClient and do the needed alloc, dealloc
in above methods. And other subclasses can take care their own handling.

WDYT?

[prashant.n, 2016-04-01 23:55:05]

One more thing I'm thinking of is moving task, task graph runner, tile task
runner and their related classes to cc/task.

What is your opinion?

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

Patchset #2 (id:20001) has been deleted

[prashant.n, 2016-04-04 10:03:23]

Patchset #2 (id:40001) has been deleted

[prashant.n, 2016-04-04 10:04:42]

Description was changed from

==========
cc: Refactor Task.

This patch renames Task to DependencyTask to exhibit what it does.
TileTask is merged into DependencyTask. Now this new task has following
functions for getting job done by the task.

  ScheduleOnOriginThread - must be called on origin thread
  CompleteOnOriginThread - must be called on origin thread
  RunOnWorkerThread - may be called origin or worker thread

The RasterTask and ImageDecodeTask are removed and their subclasses are
derived directly from DependencyTask.

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

to

==========
cc: Refactor Task.

This patch renames Task to DependencyTask to exhibit what it does.
TileTask is merged into DependencyTask. Now this new task has following
functions for getting job done by the task.

  ScheduleOnOriginThread - must be called on origin thread
  CompleteOnOriginThread - must be called on origin thread
  RunOnWorkerThread - may be called origin or worker thread

The RasterTask and ImageDecodeTask are removed and their subclasses are
derived directly from DependencyTask.

The TileTaskClient is renamed to RasterBufferProvider and
TileTaskRunner is inherited from it.

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

[prashant.n, 2016-04-04 10:07:46]

ptal.

[prashant.n, 2016-04-05 17:29:44]

Would you pls. take a look at this?

[reveman, 2016-04-05 19:14:05]

One question that I think would make this much easier to review.

https://codereview.chromium.org/1854723002/diff/80001/cc/raster/dependency_ta...
File cc/raster/dependency_task.h (right):

https://codereview.chromium.org/1854723002/diff/80001/cc/raster/dependency_ta...
cc/raster/dependency_task.h:1: // Copyright 2016 The Chromium Authors. All
rights reserved.
can you avoid this change as part of this patch and just keep the "Task" name
for now?

[prashant.n, 2016-04-06 00:28:11]

https://codereview.chromium.org/1854723002/diff/80001/cc/raster/dependency_ta...
File cc/raster/dependency_task.h (right):

https://codereview.chromium.org/1854723002/diff/80001/cc/raster/dependency_ta...
cc/raster/dependency_task.h:1: // Copyright 2016 The Chromium Authors. All
rights reserved.
On 2016/04/05 19:14:05, reveman wrote:
> can you avoid this change as part of this patch and just keep the "Task" name
> for now?

Ok. Should that be subsequent change or should revisit later in one go for Task
= DependencyTask and TaskGraphRunner = DependencyTaskRunner?

[reveman, 2016-04-06 01:17:05]

On 2016/04/06 at 00:28:11, prashant.n wrote:
>
https://codereview.chromium.org/1854723002/diff/80001/cc/raster/dependency_ta...
> File cc/raster/dependency_task.h (right):
> 
>
https://codereview.chromium.org/1854723002/diff/80001/cc/raster/dependency_ta...
> cc/raster/dependency_task.h:1: // Copyright 2016 The Chromium Authors. All
rights reserved.
> On 2016/04/05 19:14:05, reveman wrote:
> > can you avoid this change as part of this patch and just keep the "Task"
name
> > for now?
> 
> Ok. Should that be subsequent change or should revisit later in one go for
Task = DependencyTask and TaskGraphRunner = DependencyTaskRunner?

"Task = DependencyTask and TaskGraphRunner = DependencyTaskRunner", not sure we
want either of those changes but it looks like this patch contains other changes
that I think are good so it would be nice to get those in.

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

Description was changed from

==========
cc: Refactor Task.

This patch renames Task to DependencyTask to exhibit what it does.
TileTask is merged into DependencyTask. Now this new task has following
functions for getting job done by the task.

  ScheduleOnOriginThread - must be called on origin thread
  CompleteOnOriginThread - must be called on origin thread
  RunOnWorkerThread - may be called origin or worker thread

The RasterTask and ImageDecodeTask are removed and their subclasses are
derived directly from DependencyTask.

The TileTaskClient is renamed to RasterBufferProvider and
TileTaskRunner is inherited from it.

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

to

==========
cc: Simplify task and its derived classes.

This patch merges TileTask, ImageDecodeTask and RasterTask into one
class to create a single class capable of handling dependencies,
scheduling and completing and is moved to its own file. Now this new
task has following functions for doing job by the task.

  ScheduleOnOriginThread - must be called on origin thread
  CompleteOnOriginThread - must be called on origin thread
  RunOnWorkerThread - mab be called origin or worker thread

All the implementation classes like RasterTaskImpl,
ImageDecodeTaskImpl, etc. are derived directly from Task. The
TileTaskClient is renamed to RasterBufferProvider and
TileTaskRunner is inherited from it.

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

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

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

[prashant.n, 2016-04-06 02:12:14]

ptal,
esprehn for content/renderer/raster_worker_pool.h and .cc

[prashant.n, 2016-04-06 11:03:31]

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

[commit-bot: I haz the power, 2016-04-06 11:03:38]

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

[commit-bot: I haz the power, 2016-04-06 11:33:00]

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

[commit-bot: I haz the power, 2016-04-06 11:33:01]

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

[reveman, 2016-04-06 12:18:06]

I recommend trying to split this into as small pieces as possible.

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

https://codereview.chromium.org/1854723002/diff/120001/cc/raster/task.h#newco...
cc/raster/task.h:19: class CC_EXPORT Task : public
base::RefCountedThreadSafe<Task> {
dependencies should not be part of the task. it should be contained in the task
graph only.

https://codereview.chromium.org/1854723002/diff/120001/cc/raster/task.h#newco...
cc/raster/task.h:30: virtual void CompleteOnOriginThread() = 0;
Schedule/Complete should not be part of the task class. These need to be removed
from the task classes that you're trying to unify before we can do this.

[prashant.n, 2016-04-06 13:11:50]

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

https://codereview.chromium.org/1854723002/diff/120001/cc/raster/task.h#newco...
cc/raster/task.h:19: class CC_EXPORT Task : public
base::RefCountedThreadSafe<Task> {
On 2016/04/06 12:18:06, reveman wrote:
> dependencies should not be part of the task. it should be contained in the
task
> graph only.

Yes. I just checked, we are already keeping that information in edges. :).
Thanks for this point.

https://codereview.chromium.org/1854723002/diff/120001/cc/raster/task.h#newco...
cc/raster/task.h:30: virtual void CompleteOnOriginThread() = 0;
On 2016/04/06 12:18:06, reveman wrote:
> Schedule/Complete should not be part of the task class. These need to be
removed
> from the task classes that you're trying to unify before we can do this.

Do you mean to say, having this only in dervied classes
RasterTaskImpl/ImageDecodeTaskImpl?

One way I can think of implementing this is Having functionality of schedule and
complete in ctor and dtor.

But as per below line
https://code.google.com/p/chromium/codesearch#chromium/src/cc/raster/tile_tas...

We would need to check how we can avoid
AcquireBufferForRaster/ReleaseBufferForRaster, when task is already scheduled.

Do let me know your opinion?

[prashant.n, 2016-04-06 14:19:17]

> One way I can think of implementing this is Having functionality of schedule
and
> complete in ctor and dtor.
RasterTasks are always scoped created on origin thread, so we can take benefit
of ctor, dtor.

[prashant.n, 2016-04-06 14:29:03]

Once review comment 2 is addressed, I'll modify the description accordingly.

[prashant.n, 2016-04-06 14:59:47]

https://codereview.chromium.org/1854723002/diff/140001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1854723002/diff/140001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:761: tile->raster_task_ =
CreateRasterTask(prioritized_tile, &dependencies);
When same tile is rescheduled, this will pose incorrect dependencies.

[prashant.n, 2016-04-06 22:07:27]

We don't create a new task when same tile is rescheduled again. The
dependencies_ member just holds the other dependency tasks which on sheduling
put in task graph. Finally they go in task graph only. If given tile and raster
task are not going to change, IMO patchset 5 is good. I'll check if tile itself
is being reused (earlier tile ij = 1,1 and new tile ij = 5,1)alongwith raster
task present, in such case we may want to generate dependencies again.


For sheduling/completing we'll need to keep those functions in Task only as we
want to keep acquired resources (e.g. staging buffer) only for time when
rastering is being done, once it is copied we may want to use that for other
raster task.

Do let me know your opinion?

[prashant.n, 2016-04-07 03:31:37]

On 2016/04/06 22:07:27, prashant.n wrote:
> We don't create a new task when same tile is rescheduled again. The
> dependencies_ member just holds the other dependency tasks which on sheduling
> put in task graph. Finally they go in task graph only. If given tile and
raster
> task are not going to change, IMO patchset 5 is good. I'll check if tile
itself
> is being reused (earlier tile ij = 1,1 and new tile ij = 5,1)alongwith raster
> task present, in such case we may want to generate dependencies again.

This does not happen. Raster Task is scoped to tile scope. :) 


> 
> For sheduling/completing we'll need to keep those functions in Task only as we
> want to keep acquired resources (e.g. staging buffer) only for time when
> rastering is being done, once it is copied we may want to use that for other
> raster task.
> 
> Do let me know your opinion?

Ignore this comment, when checked more - RasterBufferImpl is the provision for
creating and destroying actual buffers. I'll modify code accordingly.

[prashant.n, 2016-04-07 09:22:13]

https://codereview.chromium.org/1854723002/diff/160001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1854723002/diff/160001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:92: reply_.Run(!HasFinishedRunning());
Due to this, we need to keep scheduling and completing functions.

Do you mean moving this calling to say CompletetaskOnWorkerThread function and
calling that in task graph work queue CompleteTask function?

This may need to have locks in resource_pool_ at following lines -

    404 bool memory_usage_above_limit = resource_pool_->memory_usage_bytes() >
    572 DCHECK(resource_pool_);
    579 resource_pool_->CheckBusyResources();
    591 MemoryUsage memory_usage(resource_pool_->memory_usage_bytes(),
    592 resource_pool_->resource_count());
    702 resource_pool_->ReleaseResource(draw_info.resource_, tile->id());
    804 resource_pool_->ReduceResourceUsage();
    840 resource_pool_->TryAcquireResourceWithContentId(tile->invalidated_id());
    846 resource = resource_pool_->AcquireResource(tile->desired_texture_size(),
    918 resource_pool_->ReleaseResource(resource, 0 /* content_id */);
   1062 resource_pool_->ReduceResourceUsage();

If there is any other way, do let me know.

[prashant.n, 2016-04-07 13:00:22]

IMO, current implementation is better. For RasterTaskImpl -

ScheduleOnOriginThread()
RunOnWorkerThread()
CompleteOnOriginThread()

then destuctor being called by orphan_tasks.clear(). But as we need to call
clear after scheduling next batch of tasks, it's better to call Complete before
scheduling next batch.

Only AcquireBufferForRaster() and ReleaseBufferForRaster() we can move to ctor
and dtor. Other than that I don't see anything to do atleast now, as task has to
complete before getting destroyed.

[prashant.n, 2016-04-07 13:42:44]

If we really want to move shedule and complete methods from Task, then in
TileTaskRunner we need to add callback, which will get called in
OneCopyTileTaskWorkerPool::CheckForCompletedTasks(), but it will require to
store type of Task to segregate from ImageDecodeTask, which will make Task less
generic.

[reveman, 2016-04-07 13:57:30]

On 2016/04/06 at 22:07:27, prashant.n wrote:
> We don't create a new task when same tile is rescheduled again. The
dependencies_ member just holds the other dependency tasks which on sheduling
put in task graph. Finally they go in task graph only. If given tile and raster
task are not going to change, IMO patchset 5 is good. I'll check if tile itself
is being reused (earlier tile ij = 1,1 and new tile ij = 5,1)alongwith raster
task present, in such case we may want to generate dependencies again.
> 
> 
> For sheduling/completing we'll need to keep those functions in Task only as we
want to keep acquired resources (e.g. staging buffer) only for time when
rastering is being done, once it is copied we may want to use that for other
raster task.
> 
> Do let me know your opinion?

We don't need schedule/complete at all. Those functions are supposed to be
removed and where only needed in the past when we had async uploads. I think it
would be best to remove them first. So land a patch that removes
schedule/complete and then give this patch another try.

[prashant.n, 2016-04-08 00:20:35]

> We don't need schedule/complete at all. Those functions are supposed to be

Do you mean to say remove schedule/complete and add RunReply sort of something
or that too we don't need?

[prashant.n, 2016-04-08 15:20:21]

> We don't need schedule/complete at all. Those functions are supposed to be
> removed and where only needed in the past when we had async uploads. I think
it
> would be best to remove them first. So land a patch that removes
> schedule/complete and then give this patch another try.

I'll be submitting patch for this soon.

[prashant.n, 2016-04-14 06:52:59]

I've been submitting this change in small patches by considering dependencies.
So ignore this patch.