cc: Remove ScheduleOnOriginThread() and CompleteOnOriginThread().

cc: Remove ScheduleOnOriginThread() and CompleteOnOriginThread().

The task's job is performed in RunOnWorkerThread() and schedule or
complete are not needed as a part of task's job. Those are the
responsibilities of task's owner (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload. Now raster buffer is provided to task at the time of ctor
and functionality of CompleteOnOriginThread() is moved to task's owner.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

New OnTaskCompleted() function calls corresponding function of task
owner. This patch implements following life cycle for the task.

  1. Task's owner creates task with all needed info on origin thread.
  2. Task is scheduled and run on worker thread.
  3. Completed task is processed on origin thread by task owner.
  4. Task is destroyed.

This patch also fixes few task related failing cc_perftests (607818).

BUG=499372, 599863, 607818, 613529
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/06e1561a832083330a193743a427b1354ab3cd39
Cr-Commit-Position: refs/heads/master@{#396218}

[prashant.n, 2016-04-08 16:26:21]

Description was changed from

==========
cc: Move handling of completed tasks to TileManager.

This patch removes the schedule/complete which was needed for async
upload. Now TileManager handles checking completion of completed tasks.

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

to

==========
cc: Move handling of completed tasks to TileManager.

This patch removes the schedule/complete which was needed for async
upload. Now TileManager handles checking completion of completed tasks.

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

[prashant.n, 2016-04-08 16:38:38]

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

[prashant.n, 2016-04-08 16:38:39]

https://codereview.chromium.org/1866043006/diff/1/cc/tiles/software_image_dec...
File cc/tiles/software_image_decode_controller.cc (right):

https://codereview.chromium.org/1866043006/diff/1/cc/tiles/software_image_dec...
cc/tiles/software_image_decode_controller.cc:76:
controller_->RemovePendingTask(image_key_);
By moving this code from CompleteOnOriginThread() hurts current logic of
GetTaskForImageAndRef().

browser runs well, but unittests crashes.

ericrk@, vmpstr@,

Is it okay to move this functionality to dtor? If yes I'll try to modify the
logic to take care of this change.

[vmpstr, 2016-04-08 18:59:16]

Overall, I like this change if we can fix the issues below

https://codereview.chromium.org/1866043006/diff/1/cc/tiles/software_image_dec...
File cc/tiles/software_image_decode_controller.cc (right):

https://codereview.chromium.org/1866043006/diff/1/cc/tiles/software_image_dec...
cc/tiles/software_image_decode_controller.cc:76:
controller_->RemovePendingTask(image_key_);
On 2016/04/08 16:38:39, prashant.n wrote:
> By moving this code from CompleteOnOriginThread() hurts current logic of
> GetTaskForImageAndRef().
> 
> browser runs well, but unittests crashes.
> 
> ericrk@, vmpstr@,
> 
> Is it okay to move this functionality to dtor? If yes I'll try to modify the
> logic to take care of this change.

No, this doesn't work. The task is kept alive by the pending tasks on the
controller, so one of two things is true:

1. Either this dtor will never be called, or
2. This call will not do anything since there are no references.

I think you might end up leaking things.

Basically, if the task decoded the image then it will have removed itself from
the pending set. However, if the task was cancelled then there is nothing to
remove it from the pending set and this ctor will never be called.

https://codereview.chromium.org/1866043006/diff/1/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1866043006/diff/1/cc/tiles/tile_manager.cc#ne...
cc/tiles/tile_manager.cc:882: for (auto task : completed_tasks) {
auto& task? or auto* task? depending on what completed_tasks is.

https://codereview.chromium.org/1866043006/diff/1/cc/tiles/tile_manager.cc#ne...
cc/tiles/tile_manager.cc:884: std::find(raster_tasks_.begin(),
raster_tasks_.end(), task);
This is a n^2 loop... how's the performance here if we have a lot of tasks?

[prashant.n, 2016-04-08 19:41:27]

On 2016/04/08 18:59:16, vmpstr wrote:
> Overall, I like this change if we can fix the issues below
> 
>
https://codereview.chromium.org/1866043006/diff/1/cc/tiles/software_image_dec...
> File cc/tiles/software_image_decode_controller.cc (right):
> 
>
https://codereview.chromium.org/1866043006/diff/1/cc/tiles/software_image_dec...
> cc/tiles/software_image_decode_controller.cc:76:
> controller_->RemovePendingTask(image_key_);
> On 2016/04/08 16:38:39, prashant.n wrote:
> > By moving this code from CompleteOnOriginThread() hurts current logic of
> > GetTaskForImageAndRef().
> > 
> > browser runs well, but unittests crashes.
> > 
> > ericrk@, vmpstr@,
> > 
> > Is it okay to move this functionality to dtor? If yes I'll try to modify the
> > logic to take care of this change.
> 
> No, this doesn't work. The task is kept alive by the pending tasks on the
> controller, so one of two things is true:
> 
> 1. Either this dtor will never be called, or
> 2. This call will not do anything since there are no references.
> 
> I think you might end up leaking things.
> 
> Basically, if the task decoded the image then it will have removed itself from
> the pending set. However, if the task was cancelled then there is nothing to
> remove it from the pending set and this ctor will never be called.
> 



TileManager has image decode controller, we can call RemovePendingTask() when
image decode task is finished.
For this we will need to know if task is image decode task or raster task. This
will also improve n^2 loop to n cost loop.



> https://codereview.chromium.org/1866043006/diff/1/cc/tiles/tile_manager.cc
> File cc/tiles/tile_manager.cc (right):
> 
>
https://codereview.chromium.org/1866043006/diff/1/cc/tiles/tile_manager.cc#ne...
> cc/tiles/tile_manager.cc:882: for (auto task : completed_tasks) {
> auto& task? or auto* task? depending on what completed_tasks is.
> 
>
https://codereview.chromium.org/1866043006/diff/1/cc/tiles/tile_manager.cc#ne...
> cc/tiles/tile_manager.cc:884: std::find(raster_tasks_.begin(),
> raster_tasks_.end(), task);
> This is a n^2 loop... how's the performance here if we have a lot of tasks?

For this I'll check some better way.

[prashant.n, 2016-04-09 04:03:02]

I'll add CompleteImageTask function to image decode controller.

[prashant.n, 2016-04-11 14:17:48]

Kindly comment on approach.

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

https://codereview.chromium.org/1866043006/diff/40001/cc/raster/task_graph_ru...
cc/raster/task_graph_runner.h:34: TaskTypeId GetTaskTypeId();
This looks little wierd. But I don't see any other approach. Pls. comment on
this if this is not okay?

Basically what CompleteOnOriginThread was doing is job of Task owner (viz.
TileManager, [Software|Gpu]ImageDecodeController). So we need to move those out
of Task interfaces making Task leaner.

[prashant.n, 2016-04-11 14:51:52]

https://codereview.chromium.org/1866043006/diff/40001/cc/raster/tile_task_wor...
File cc/raster/tile_task_worker_pool_unittest.cc (right):

https://codereview.chromium.org/1866043006/diff/40001/cc/raster/tile_task_wor...
cc/raster/tile_task_worker_pool_unittest.cc:187: for (auto task :
completed_tasks) {
Add comment or processing completed imagedecodetasks.

[prashant.n, 2016-04-12 15:56:55]

Description was changed from

==========
cc: Move handling of completed tasks to TileManager.

This patch removes the schedule/complete which was needed for async
upload. Now TileManager handles checking completion of completed tasks.

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

to

==========
cc: Move the processing of completed tasks to TileManager.

The task's job is to be performed in RunOnWorkerThread(). This patch
removes following functions which were needed for async upload.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

Now TileManager handles processing the completed tasks. The completed
image decode tasks are processed in ImageDecodeController.

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

[prashant.n, 2016-04-12 15:57:45]

ptal

https://codereview.chromium.org/1866043006/diff/100001/cc/raster/one_copy_til...
File cc/raster/one_copy_tile_task_worker_pool.cc (right):

https://codereview.chromium.org/1866043006/diff/100001/cc/raster/one_copy_til...
cc/raster/one_copy_tile_task_worker_pool.cc:265: DCHECK(resource->size().width()
&& resource->size().height());
With width = 0, it will cause divide by zero crash (bytes_per_row).

https://codereview.chromium.org/1866043006/diff/100001/cc/raster/task_graph_r...
File cc/raster/task_graph_runner.h (right):

https://codereview.chromium.org/1866043006/diff/100001/cc/raster/task_graph_r...
cc/raster/task_graph_runner.h:25: const TaskTypeId kDefaultTaskTypeId = 0;
We can have either this (which is generic) OR

downcasting routines like

virtual Task* Task::AsRasterTask() { return nullptr; }

Task* RasterTask::AsRasterTask() override { return this; }

[prashant.n, 2016-04-12 16:05:06]

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

[commit-bot: I haz the power, 2016-04-12 16:05:23]

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

[commit-bot: I haz the power, 2016-04-12 16:07:56]

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

[commit-bot: I haz the power, 2016-04-12 16:07:57]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[prashant.n, 2016-04-12 16:47:33]

I'll take care of gpu_image_decode_controller latest changes soon.

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

Description was changed from

==========
cc: Move the processing of completed tasks to TileManager.

The task's job is to be performed in RunOnWorkerThread(). This patch
removes following functions which were needed for async upload.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

Now TileManager handles processing the completed tasks. The completed
image decode tasks are processed in ImageDecodeController.

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

to

==========
cc: Move the processing of completed tasks to TileManager.

The task's job is performed in RunOnWorkerThread() and Schedule() or
Complete() are not needed as a part of Task's job. Those are the
responsibilities of origin thread (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

Now TileManager handles processing the completed tasks. The completed
image decode tasks are processed in ImageDecodeController.

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

[prashant.n, 2016-04-13 10:20:41]

Modified gpu_image_decode_controller_unittest.cc tests accordingly.
PTAL.

[prashant.n, 2016-04-13 10:20:59]

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

[commit-bot: I haz the power, 2016-04-13 10:21:11]

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

[prashant.n, 2016-04-13 10:51:37]

Description was changed from

==========
cc: Move the processing of completed tasks to TileManager.

The task's job is performed in RunOnWorkerThread() and Schedule() or
Complete() are not needed as a part of Task's job. Those are the
responsibilities of origin thread (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

Now TileManager handles processing the completed tasks. The completed
image decode tasks are processed in ImageDecodeController.

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

to

==========
cc: Move the processing of completed tasks to TileManager.

The task's job is performed in RunOnWorkerThread() and Schedule() or
Complete() are not needed as a part of Task's job. Those are the
responsibilities of origin thread (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

Now TileManager handles processing the completed tasks. The completed
image decode/upload tasks are processed in ImageDecodeController.

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

[commit-bot: I haz the power, 2016-04-13 10:57:20]

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

[commit-bot: I haz the power, 2016-04-13 10:57:21]

Dry run: Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/8...)

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

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

https://codereview.chromium.org/1866043006/diff/160001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:926: DCHECK(tiles_.find(raster_task->tile_->id()) ==
tiles_.end());
The check should be !=

[prashant.n, 2016-04-13 12:10:44]

ptal

[prashant.n, 2016-04-13 12:10:54]

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

[commit-bot: I haz the power, 2016-04-13 12:11:05]

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

[commit-bot: I haz the power, 2016-04-13 12:52:29]

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

[commit-bot: I haz the power, 2016-04-13 12:52:30]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

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

Description was changed from

==========
cc: Move the processing of completed tasks to TileManager.

The task's job is performed in RunOnWorkerThread() and Schedule() or
Complete() are not needed as a part of Task's job. Those are the
responsibilities of origin thread (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

Now TileManager handles processing the completed tasks. The completed
image decode/upload tasks are processed in ImageDecodeController.

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

to

==========
cc: Move the processing of completed tasks to TileManager.

The task's job is performed in RunOnWorkerThread() and Schedule() or
Complete() are not needed as a part of Task's job. Those are the
responsibilities of origin thread (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

Now TileManager processes the completed tasks and completed image
decode/upload tasks are processed in ImageDecodeController. The raster
task gets created with acquiring raster buffer and releasing of raster
buffer is handled while processing completed tasks in TileManager.

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

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

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

[commit-bot: I haz the power, 2016-04-13 15:14:22]

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

[commit-bot: I haz the power, 2016-04-13 16:08:51]

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

[commit-bot: I haz the power, 2016-04-13 16:08:52]

Dry run: This issue passed the CQ dry run.

[prashant.n, 2016-04-15 07:03:27]

ptal - patch #1.

[prashant.n, 2016-04-17 18:30:48]

ping

[reveman, 2016-04-17 20:10:07]

On 2016/04/17 at 18:30:48, prashant.n wrote:
> ping

As this depends on another patch. I'm going to wait with this review until that
patch has landed.

[prashant.n, 2016-04-18 02:48:07]

On 2016/04/17 20:10:07, reveman wrote:
> On 2016/04/17 at 18:30:48, prashant.n wrote:
> > ping
> 
> As this depends on another patch. I'm going to wait with this review until
that
> patch has landed.

Actually this is first patch which removes schedule and complete.

[prashant.n, 2016-04-18 05:13:15]

> Actually this is first patch which removes schedule and complete.

I'll change the dependency of this patch. First try to land "Simplify Task"
patch.

[ericrk, 2016-05-02 20:17:38]

On 2016/04/18 05:13:15, prashant.n wrote:
> > Actually this is first patch which removes schedule and complete.
> 
> I'll change the dependency of this patch. First try to land "Simplify Task"
> patch.

Is this patch still needed? If so, can you rebase now that the "Simplify Task"
patch has landed. Thanks!

[prashant.n, 2016-05-03 02:09:55]

> Is this patch still needed? If so, can you rebase now that the "Simplify Task"
> patch has landed. Thanks!

I'll rebase this. OOO for 2 weeks from tomorrow.

[prashant.n, 2016-05-16 15:06:39]

Description was changed from

==========
cc: Move the processing of completed tasks to TileManager.

The task's job is performed in RunOnWorkerThread() and Schedule() or
Complete() are not needed as a part of Task's job. Those are the
responsibilities of origin thread (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

Now TileManager processes the completed tasks and completed image
decode/upload tasks are processed in ImageDecodeController. The raster
task gets created with acquiring raster buffer and releasing of raster
buffer is handled while processing completed tasks in TileManager.

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

to

==========
cc: Remove TileTask::ScheduleOnOriginThread().

This patch removes ScheduleOnOriginThread() which was basically calling
AcquireBufferForRaster(). Now Tasks which need raster buffer are
created with raster buffer acquired.

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

[prashant.n, 2016-05-16 15:12:20]

PTAL.

In this patch, I've removed only ScheduleOnOriginThread(). As
CompleteOnOriginThread() has different definitions for different tasks, we can
decide on whether to remove and implement something like patch set 13 OR not. If
to be removed, I'll submit separate patch for it.

[prashant.n, 2016-05-16 15:19:36]

Description was changed from

==========
cc: Remove TileTask::ScheduleOnOriginThread().

This patch removes ScheduleOnOriginThread() which was basically calling
AcquireBufferForRaster(). Now Tasks which need raster buffer are
created with raster buffer acquired.

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

to

==========
cc: Remove TileTask::ScheduleOnOriginThread().

This patch removes ScheduleOnOriginThread() which was basically calling
AcquireBufferForRaster(). Now raster buffer is provided to task at the
time of ctor as scheduling task asynchronously is not needed.

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

[reveman, 2016-05-17 13:45:03]

lgtm % ericrk's review

Removing CompleteOnOriginThread would be nice as a next step. Maybe just move
that code to the dtor as a first step?

[prashant.n, 2016-05-17 15:57:49]

I'm fixing few perf tests which are failing in debug mode.

[prashant.n, 2016-05-17 16:09:08]

> I'm fixing few perf tests which are failing in debug mode.

Looks for this we need to remove the CompleteOnOriginThread in this patch only
as it fails at DCHECK(!did_complete_);

[prashant.n, 2016-05-18 14:53:58]

Description was changed from

==========
cc: Remove TileTask::ScheduleOnOriginThread().

This patch removes ScheduleOnOriginThread() which was basically calling
AcquireBufferForRaster(). Now raster buffer is provided to task at the
time of ctor as scheduling task asynchronously is not needed.

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

to

==========
cc: Move the processing of completed task to its owner.

The task's job is performed in RunOnWorkerThread() and schedule or
complete are not needed as a part of task's job. Those are the
responsibilities of task's owner (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload. Now raster buffer is provided to task at the time of ctor
and functionality of CompleteOnOriginThread() is moved to task's owner.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

This patch implements following life cycle for the task.

  1. Task's owner creates task with all needed info on origin thread.
  2. Task is scheduled and run on worker thread.
  3. Completed task is processed on origin thread by task owner.
  4. Task is destroyed.

This patch also fixes few task related failing cc_perftests (607818).

BUG=499372, 599863, 607818
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[prashant.n, 2016-05-18 14:54:26]

PTAL.

[reveman, 2016-05-18 16:20:31]

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/tile_task.h
File cc/raster/tile_task.h (right):

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/tile_task.h#...
cc/raster/tile_task.h:19: enum class Type : uint16_t { DEFAULT, RASTER,
IMAGE_DECODE, IMAGE_UPLOAD };
why are we not adding the virtual functions we need instead of this?

[prashant.n, 2016-05-18 16:45:38]

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/tile_task.h
File cc/raster/tile_task.h (right):

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/tile_task.h#...
cc/raster/tile_task.h:19: enum class Type : uint16_t { DEFAULT, RASTER,
IMAGE_DECODE, IMAGE_UPLOAD };
On 2016/05/18 16:20:31, reveman wrote:
> why are we not adding the virtual functions we need instead of this?

Do you mean to say - something like following?
IsRasterTask()
IsImageDecodeTask()
IsImageUploadTask()

Either way is fine, but
1. thought of having tasks know the type at the time of ctor.
2. same types can be used for perf, unit tests tasks.

[reveman, 2016-05-18 17:16:48]

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/tile_task.h
File cc/raster/tile_task.h (right):

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/tile_task.h#...
cc/raster/tile_task.h:19: enum class Type : uint16_t { DEFAULT, RASTER,
IMAGE_DECODE, IMAGE_UPLOAD };
On 2016/05/18 at 16:45:38, prashant.n wrote:
> On 2016/05/18 16:20:31, reveman wrote:
> > why are we not adding the virtual functions we need instead of this?
> 
> Do you mean to say - something like following?
> IsRasterTask()
> IsImageDecodeTask()
> IsImageUploadTask()

No, this is not what I meant. Something seems off when we have a class with
virtual functions and we have to identify the type of instance. Why is there not
a virtual function for what we like to do differently depending on the TileTask
class? virtual void OnTaskCompleted()?

> 
> Either way is fine, but
> 1. thought of having tasks know the type at the time of ctor.
> 2. same types can be used for perf, unit tests tasks.

[prashant.n, 2016-05-18 18:25:50]

> 
> No, this is not what I meant. Something seems off when we have a class with
> virtual functions and we have to identify the type of instance. Why is there
not
> a virtual function for what we like to do differently depending on the
TileTask
> class? virtual void OnTaskCompleted()?

Earlier we had CompleteOnOriginThread() which was vitrual and doing same. To
remove same I moved handling to task owner, but it has become clumsy. May be we
can keep function and rename it to OnTaskCompleted().

Next step I'm thinking is adding closures for Run and Complete methods. WDYT?

[reveman, 2016-05-18 18:46:39]

On 2016/05/18 at 18:25:50, prashant.n wrote:
> > 
> > No, this is not what I meant. Something seems off when we have a class with
> > virtual functions and we have to identify the type of instance. Why is there
not
> > a virtual function for what we like to do differently depending on the
TileTask
> > class? virtual void OnTaskCompleted()?
> 
> Earlier we had CompleteOnOriginThread() which was vitrual and doing same. To
remove same I moved handling to task owner, but it has become clumsy. May be we
can keep function and rename it to OnTaskCompleted().

A virtual OnTaskCompleted() makes sense and that can just call some task owner
function directly without any closure involved.


> 
> Next step I'm thinking is adding closures for Run and Complete methods. WDYT?

That's the opposite direction I'd like to see us going. I'd us to remove the
existing use of closures in this code and just make function calls directly.
Closures have some overhead that it's nice to avoid here.

[vmpstr, 2016-05-18 19:08:34]

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

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task.h#newco...
cc/raster/task.h:19: // if scheduled concludes in COMPLETED state. So possible
life cycle paths for
you say here "if scheduled concludes in completed" but below you have an example
of NEW -> CANCELED -> COMPLETED. That seems contradictory.

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task.h#newco...
cc/raster/task.h:21: //   NEW
I don't think you need this one?

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task.h#newco...
cc/raster/task.h:44: // completed and no more it should be processed.
s/no more it should be processed/it doesn't need to be processed anymore/

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task.h#newco...
cc/raster/task.h:60: COMPLETED
In my mind the difference between "FINISHED" and "COMPLETED" is very small... I
think we either need to come up with a different name or... somethign

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task_graph_r...
File cc/raster/task_graph_runner_perftest.cc (right):

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task_graph_r...
cc/raster/task_graph_runner_perftest.cc:275: for (auto task : *completed_tasks)
{
nit: auto* task

https://codereview.chromium.org/1866043006/diff/330001/cc/tiles/software_imag...
File cc/tiles/software_image_decode_controller.cc (right):

https://codereview.chromium.org/1866043006/diff/330001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:84: public:
Why are these public? This doesn't look correct.

https://codereview.chromium.org/1866043006/diff/330001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:722: ImageDecodeTaskImpl*
decode_task = static_cast<ImageDecodeTaskImpl*>(task);
I think we should rework this so we don't need a static cast somehow. Maybe
double dispatch

[prashant.n, 2016-05-19 00:59:52]

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

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task.h#newco...
cc/raster/task.h:19: // if scheduled concludes in COMPLETED state. So possible
life cycle paths for
On 2016/05/18 19:08:33, vmpstr wrote:
> you say here "if scheduled concludes in completed" but below you have an
example
> of NEW -> CANCELED -> COMPLETED. That seems contradictory. 

This above path is for task which is not scheduled (has dependecies), but
cancelled due to reschedule. Actually task scheduled means added to
|ready_to_run_tasks|.

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task.h#newco...
cc/raster/task.h:21: //   NEW
We will omit this.

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task.h#newco...
cc/raster/task.h:44: // completed and no more it should be processed.
Ok, I'll modify this.

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task.h#newco...
cc/raster/task.h:60: COMPLETED
Yes. Finished and Completed are 2 sides of same coin. Finished is by task graph
runner and Completed is by task owner (e.g. tile manager). It just for
semantics.

Ideally finished tasks get skipped while sheduling. And as we ensure checking
completed tasks before TileManager::ScheduleTask(), Completed looks redundant,
we can use finished state there.

I'll revisit this and remove Completed if redundant.

https://codereview.chromium.org/1866043006/diff/330001/cc/tiles/software_imag...
File cc/tiles/software_image_decode_controller.cc (right):

https://codereview.chromium.org/1866043006/diff/330001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:84: public:
On 2016/05/18 19:08:34, vmpstr wrote:
> Why are these public? This doesn't look correct.

Yes. This looks odd and not good as this class is under unamed nmaespace, we
cannot add friend for it.

I'll check another way suggested by reveman@, having completed function in task
itself.

https://codereview.chromium.org/1866043006/diff/330001/cc/tiles/software_imag...
cc/tiles/software_image_decode_controller.cc:722: ImageDecodeTaskImpl*
decode_task = static_cast<ImageDecodeTaskImpl*>(task);
I'll check this. static cast was added for downcasting.

[prashant.n, 2016-05-19 11:59:21]

ptal. addressed review comments.

[prashant.n, 2016-05-19 14:32:30]

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task_graph_r...
File cc/raster/task_graph_runner_perftest.cc (right):

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task_graph_r...
cc/raster/task_graph_runner_perftest.cc:275: for (auto task : *completed_tasks)
{
It is scoped_refptr class object, so it would not work with auto*.

[prashant.n, 2016-05-19 16:24:29]

Description was changed from

==========
cc: Move the processing of completed task to its owner.

The task's job is performed in RunOnWorkerThread() and schedule or
complete are not needed as a part of task's job. Those are the
responsibilities of task's owner (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload. Now raster buffer is provided to task at the time of ctor
and functionality of CompleteOnOriginThread() is moved to task's owner.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

This patch implements following life cycle for the task.

  1. Task's owner creates task with all needed info on origin thread.
  2. Task is scheduled and run on worker thread.
  3. Completed task is processed on origin thread by task owner.
  4. Task is destroyed.

This patch also fixes few task related failing cc_perftests (607818).

BUG=499372, 599863, 607818
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Remove ScheduleOnOriginThread() and CompleteOnOriginThread(). 

The task's job is performed in RunOnWorkerThread() and schedule or
complete are not needed as a part of task's job. Those are the
responsibilities of task's owner (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload. Now raster buffer is provided to task at the time of ctor
and functionality of CompleteOnOriginThread() is moved to task's owner.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

New OnTaskCompleted() function calls appropriate function of task
owner. This patch implements following life cycle for the task.

  1. Task's owner creates task with all needed info on origin thread.
  2. Task is scheduled and run on worker thread.
  3. Completed task is processed on origin thread by task owner.
  4. Task is destroyed.

This patch also fixes few task related failing cc_perftests (607818).

BUG=499372, 599863, 607818
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[ericrk, 2016-05-19 23:04:08]

https://codereview.chromium.org/1866043006/diff/390001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1866043006/diff/390001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:793: // DCHECK(!task->state().IsFinished());
Does this mean that this check was failing without being commented out? Should
this check both !IsFinished and !IsCancelled to more closely match previous
behavior?

https://codereview.chromium.org/1866043006/diff/390001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:913: std::unique_ptr<RasterBuffer> raster_buffer =
vmpstr@ - I remember there being talk of allocating buffers on the worker thread
- would this change actually move us farther from that? May be fine, but wanted
to check.

[ericrk, 2016-05-19 23:19:37]

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task_graph_r...
File cc/raster/task_graph_runner_perftest.cc (right):

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task_graph_r...
cc/raster/task_graph_runner_perftest.cc:275: for (auto task : *completed_tasks)
{
On 2016/05/19 14:32:30, prashant.n wrote:
> It is scoped_refptr class object, so it would not work with auto*.
> 

In that case, would auto& be more appropriate - the current version will
addref/release with each iteration.

https://codereview.chromium.org/1866043006/diff/390001/cc/raster/raster_buffe...
File cc/raster/raster_buffer_provider_perftest.cc (right):

https://codereview.chromium.org/1866043006/diff/390001/cc/raster/raster_buffe...
cc/raster/raster_buffer_provider_perftest.cc:141: class
PerfRasterBufferProviderHelper {
Why not just use RasterBufferProvider directly, as the implementation of this
class just wraps two of its functions?

[prashant.n, 2016-05-20 01:57:17]

https://codereview.chromium.org/1866043006/diff/390001/cc/raster/raster_buffe...
File cc/raster/raster_buffer_provider_perftest.cc (right):

https://codereview.chromium.org/1866043006/diff/390001/cc/raster/raster_buffe...
cc/raster/raster_buffer_provider_perftest.cc:141: class
PerfRasterBufferProviderHelper {
On 2016/05/19 23:19:37, ericrk wrote:
> Why not just use RasterBufferProvider directly, as the implementation of this
> class just wraps two of its functions?

This was basically added, because of class definition dependency as done in
patch set 19. We already have RasterBufferProvider from
raster_buffer_provider.h. This class is not actual raster provider, it just gets
it from its derived class.

https://codereview.chromium.org/1866043006/diff/390001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1866043006/diff/390001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:793: // DCHECK(!task->state().IsFinished());
On 2016/05/19 23:04:08, ericrk wrote:
> Does this mean that this check was failing without being commented out?

Yes.

 Should
> this check both !IsFinished and !IsCancelled to more closely match previous
> behavior?

I'm going to check this today. Because earlier we relied on HasCompleted(),
which is now removed.

https://codereview.chromium.org/1866043006/diff/390001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:913: std::unique_ptr<RasterBuffer> raster_buffer =
On 2016/05/19 23:04:08, ericrk wrote:
> vmpstr@ - I remember there being talk of allocating buffers on the worker
thread
> - would this change actually move us farther from that? May be fine, but
wanted
> to check.

Then it could have been lot simpler, as acquire and release would get moved to
Run method in task graph runner, as before/after running task on worker thread,
we do hold the lock.

[prashant.n, 2016-05-20 11:48:52]

ptal.

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task_graph_r...
File cc/raster/task_graph_runner_perftest.cc (right):

https://codereview.chromium.org/1866043006/diff/330001/cc/raster/task_graph_r...
cc/raster/task_graph_runner_perftest.cc:275: for (auto task : *completed_tasks)
{
Now I understood why vmpstr added comment. Thank you for point out this. I fixed
this and some other places.

[prashant.n, 2016-05-20 11:51:04]

Description was changed from

==========
cc: Remove ScheduleOnOriginThread() and CompleteOnOriginThread(). 

The task's job is performed in RunOnWorkerThread() and schedule or
complete are not needed as a part of task's job. Those are the
responsibilities of task's owner (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload. Now raster buffer is provided to task at the time of ctor
and functionality of CompleteOnOriginThread() is moved to task's owner.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

New OnTaskCompleted() function calls appropriate function of task
owner. This patch implements following life cycle for the task.

  1. Task's owner creates task with all needed info on origin thread.
  2. Task is scheduled and run on worker thread.
  3. Completed task is processed on origin thread by task owner.
  4. Task is destroyed.

This patch also fixes few task related failing cc_perftests (607818).

BUG=499372, 599863, 607818
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Remove ScheduleOnOriginThread() and CompleteOnOriginThread(). 

The task's job is performed in RunOnWorkerThread() and schedule or
complete are not needed as a part of task's job. Those are the
responsibilities of task's owner (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload. Now raster buffer is provided to task at the time of ctor
and functionality of CompleteOnOriginThread() is moved to task's owner.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

New OnTaskCompleted() function calls corresponding function of task
owner. This patch implements following life cycle for the task.

  1. Task's owner creates task with all needed info on origin thread.
  2. Task is scheduled and run on worker thread.
  3. Completed task is processed on origin thread by task owner.
  4. Task is destroyed.

This patch also fixes few task related failing cc_perftests (607818).

BUG=499372, 599863, 607818, 613529
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[prashant.n, 2016-05-20 11:51:58]

> Now I understood why vmpstr added comment. Thank you for point out this. I
fixed
> this and some other places.

pointing*

[prashant.n, 2016-05-20 11:52:01]

> Now I understood why vmpstr added comment. Thank you for point out this. I
fixed
> this and some other places.

pointing*

[vmpstr, 2016-05-21 00:02:14]

https://codereview.chromium.org/1866043006/diff/430001/cc/raster/task.cc
File cc/raster/task.cc (right):

https://codereview.chromium.org/1866043006/diff/430001/cc/raster/task.cc#newc...
cc/raster/task.cc:17: // cycle correctly. Few tests still do not take care of
task states.
I'd prefer that we fix the tests instead of adding this TODO. Are there a lot of
cases? How difficult is it to fix them?

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

https://codereview.chromium.org/1866043006/diff/430001/cc/raster/task.h#newco...
cc/raster/task.h:37: //    │ RUNNING │-------> ║ FINISHED ║
awesome comment, thanks

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:44: class RasterTaskImpl : public TileTask {
We might want to restructure this task a bit. Specifically, since we're passing
a tile, we can have a private struct like ThreadSafeState and just pull out all
of the relevant information from the tile into ThreadSafeState at construction
time. Then, it's clear what can and cannot be accessed in RunOnWorkerThread and
we pass far fewer parameters into RasterTaskImpl...

Can you leave a TODO here?

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:122: Tile* tile_;
The reason we had this as a void* is that it would only be used as an id for
tracing. That is, we have to be careful not to be accessing tiles from different
threads. I think the patch you have now is fine, but can you leave a comment
here to say that tile object should not be accessed in any function that runs on
a different thread (it's not a thread safe object)

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:251: bool
CheckDependenciesForFinishedRasterTask(TileTask* raster_task) {
Our pattern is usually:

void CheckFoo(Bar* bar) {
#if DCHECK_IS_ON()
  for (...)
    ...
#endif
}

and then in the calling code:

CheckFoo(bar);

Also can you move this function after the class below?

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:803: if (task->state().IsFinished()) {
In what situation can we be getting a finished task here? Also, is cancelled a
type of finished or does finish imply it ran to completion?

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_manager_...
File cc/tiles/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_manager_...
cc/tiles/tile_manager_unittest.cc:1911: using CancellingTileTaskManager =
FakeTileTaskManagerImpl;
Can you just rename CancellingTileTaskManager  to FakeTileTaskManagerImpl. Also,
are the tests testing the same thing now? I guess tasks are implicitly cancelled
unless run?

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_task_man...
File cc/tiles/tile_task_manager.cc (right):

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_task_man...
cc/tiles/tile_task_manager.cc:51: completed_tasks.clear();
you don't need this.

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_task_man...
File cc/tiles/tile_task_manager.h (right):

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_task_man...
cc/tiles/tile_task_manager.h:31: // Check for completed tasks.
you should keep "and call OnTaskCompleted on them"

[prashant.n, 2016-05-21 00:33:53]

https://codereview.chromium.org/1866043006/diff/430001/cc/raster/task.cc
File cc/raster/task.cc (right):

https://codereview.chromium.org/1866043006/diff/430001/cc/raster/task.cc#newc...
cc/raster/task.cc:17: // cycle correctly. Few tests still do not take care of
task states.
On 2016/05/21 00:02:14, vmpstr wrote:
> I'd prefer that we fix the tests instead of adding this TODO. Are there a lot
of
> cases? How difficult is it to fix them?

Few testcases I've fixed in https://codereview.chromium.org/1997743002/. Now
only few have remained  related to RasterBufferProvider tests.

I'll submit separate patch for this.

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:44: class RasterTaskImpl : public TileTask {
Yes. I'll create two structures one for running task and one for processing
completion in next patch. For this I'll leave the comment in current patch.

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:122: Tile* tile_;
Yes.

I was thinking to add clousres here one for running and one for completing. But
passing members is better than having closures.

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:251: bool
CheckDependenciesForFinishedRasterTask(TileTask* raster_task) {
Yes.

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:803: if (task->state().IsFinished()) {
On 2016/05/21 00:02:14, vmpstr wrote:
> In what situation can we be getting a finished task here? Also, is cancelled a

See crbug.com/613529 for more details. I'll modify the comment to depict the
situation.

> type of finished or does finish imply it ran to completion?

Only finished state implies to it ran to completion. The canceled state implies
it completed as canceled. The task task is modified to canceled state in
TaskGraphWorkQueue::ScheduleTasks(). So this won't arise but I'll check further
- canceled task must be collected before next scheduling happens, so that if
tile which had task canceled should get new raster task.

[prashant.n, 2016-05-21 00:51:46]

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_manager_...
File cc/tiles/tile_manager_unittest.cc (right):

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_manager_...
cc/tiles/tile_manager_unittest.cc:1911: using CancellingTileTaskManager =
FakeTileTaskManagerImpl;
On 2016/05/21 00:02:14, vmpstr wrote:
> Can you just rename CancellingTileTaskManager  to FakeTileTaskManagerImpl.
Also,
> are the tests testing the same thing now? I guess tasks are implicitly
cancelled
> unless run?

Now if task has be to canceled, it's explicit. So base class
FakeTileTaskManagerImpl does the same stuff.

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_task_man...
File cc/tiles/tile_task_manager.cc (right):

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_task_man...
cc/tiles/tile_task_manager.cc:51: completed_tasks.clear();
On 2016/05/21 00:02:14, vmpstr wrote:
> you don't need this. 

Hmm.

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_task_man...
File cc/tiles/tile_task_manager.h (right):

https://codereview.chromium.org/1866043006/diff/430001/cc/tiles/tile_task_man...
cc/tiles/tile_task_manager.h:31: // Check for completed tasks.
On 2016/05/21 00:02:14, vmpstr wrote:
> you should keep "and call OnTaskCompleted on them"

Ok. I'll modify.

[prashant.n, 2016-05-21 16:29:33]

ptal

[prashant.n, 2016-05-23 09:58:32]

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

[commit-bot: I haz the power, 2016-05-23 09:58:48]

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

[commit-bot: I haz the power, 2016-05-23 12:24:26]

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

[commit-bot: I haz the power, 2016-05-23 12:24:27]

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-05-24 02:56:18]

ping.

[vmpstr, 2016-05-24 23:44:58]

https://codereview.chromium.org/1866043006/diff/470001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1866043006/diff/470001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:806: // TileManager::Schedule(), happens. So if tile
which had task canceled
nit: TileManager::ScheduleTasks()

https://codereview.chromium.org/1866043006/diff/470001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:810: // finished. If task gets finished after it is
checked, then it gets skipped
Why can't we just rely on TaskGraphWorkQueue::ScheduleTasks to skip all of
these?

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

https://codereview.chromium.org/1866043006/diff/470001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1866043006/diff/470001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:810: // finished. If task gets finished after it is
checked, then it gets skipped
On 2016/05/24 23:44:57, vmpstr wrote:
> Why can't we just rely on TaskGraphWorkQueue::ScheduleTasks to skip all of
> these?

To avoid processing related to task done, i.e. adding node, edge in next graph.

[reveman, 2016-05-25 03:47:31]

still lg-tm once vmpstr and ericrk are happy

[prashant.n, 2016-05-25 13:12:49]

On 2016/05/25 03:47:31, reveman wrote:
> still lg-tm once vmpstr and ericrk are happy

ptal, pending vmpstr@ to review.

[prashant.n, 2016-05-25 14:35:36]

https://codereview.chromium.org/1866043006/diff/490001/cc/tiles/tile_manager.cc
File cc/tiles/tile_manager.cc (right):

https://codereview.chromium.org/1866043006/diff/490001/cc/tiles/tile_manager....
cc/tiles/tile_manager.cc:268: void OnTaskCompleted() override{};
I'll correct this - before final submission -

void OnTaskCompleted() override {}

[vmpstr, 2016-05-25 19:53:43]

lgtm if ericrk is ok with it.

[prashant.n, 2016-05-26 00:53:17]

On 2016/05/25 19:53:43, vmpstr wrote:
> lgtm if ericrk is ok with it.

ericrk@, pls. review it.

[prashant.n, 2016-05-26 14:23:30]

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

[commit-bot: I haz the power, 2016-05-26 14:23:43]

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

[commit-bot: I haz the power, 2016-05-26 14:58:20]

Dry run: None

[prashant.n, 2016-05-26 15:07:23]

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

[commit-bot: I haz the power, 2016-05-26 15:07:44]

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

[commit-bot: I haz the power, 2016-05-26 16:20:15]

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

[commit-bot: I haz the power, 2016-05-26 16:20:17]

Dry run: This issue passed the CQ dry run.

[commit-bot: I haz the power, 2016-05-26 16:20:29]

Dry run: None

[ericrk, 2016-05-26 17:28:46]

lgtm

[prashant.n, 2016-05-26 17:29:50]

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

[prashant.n, 2016-05-26 17:29:50]

The patchset sent to the CQ was uploaded after l-g-t-m from
reveman@chromium.org, vmpstr@chromium.org
Link to the patchset: https://codereview.chromium.org/1866043006/#ps530001
(title: "nits")

[commit-bot: I haz the power, 2016-05-26 17:30:27]

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

[commit-bot: I haz the power, 2016-05-26 17:35:32]

Description was changed from

==========
cc: Remove ScheduleOnOriginThread() and CompleteOnOriginThread(). 

The task's job is performed in RunOnWorkerThread() and schedule or
complete are not needed as a part of task's job. Those are the
responsibilities of task's owner (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload. Now raster buffer is provided to task at the time of ctor
and functionality of CompleteOnOriginThread() is moved to task's owner.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

New OnTaskCompleted() function calls corresponding function of task
owner. This patch implements following life cycle for the task.

  1. Task's owner creates task with all needed info on origin thread.
  2. Task is scheduled and run on worker thread.
  3. Completed task is processed on origin thread by task owner.
  4. Task is destroyed.

This patch also fixes few task related failing cc_perftests (607818).

BUG=499372, 599863, 607818, 613529
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Remove ScheduleOnOriginThread() and CompleteOnOriginThread(). 

The task's job is performed in RunOnWorkerThread() and schedule or
complete are not needed as a part of task's job. Those are the
responsibilities of task's owner (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload. Now raster buffer is provided to task at the time of ctor
and functionality of CompleteOnOriginThread() is moved to task's owner.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

New OnTaskCompleted() function calls corresponding function of task
owner. This patch implements following life cycle for the task.

  1. Task's owner creates task with all needed info on origin thread.
  2. Task is scheduled and run on worker thread.
  3. Completed task is processed on origin thread by task owner.
  4. Task is destroyed.

This patch also fixes few task related failing cc_perftests (607818).

BUG=499372, 599863, 607818, 613529
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[commit-bot: I haz the power, 2016-05-26 17:35:34]

Committed patchset #28 (id:530001)

[commit-bot: I haz the power, 2016-05-26 17:38:02]

Description was changed from

==========
cc: Remove ScheduleOnOriginThread() and CompleteOnOriginThread(). 

The task's job is performed in RunOnWorkerThread() and schedule or
complete are not needed as a part of task's job. Those are the
responsibilities of task's owner (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload. Now raster buffer is provided to task at the time of ctor
and functionality of CompleteOnOriginThread() is moved to task's owner.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

New OnTaskCompleted() function calls corresponding function of task
owner. This patch implements following life cycle for the task.

  1. Task's owner creates task with all needed info on origin thread.
  2. Task is scheduled and run on worker thread.
  3. Completed task is processed on origin thread by task owner.
  4. Task is destroyed.

This patch also fixes few task related failing cc_perftests (607818).

BUG=499372, 599863, 607818, 613529
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Remove ScheduleOnOriginThread() and CompleteOnOriginThread().

The task's job is performed in RunOnWorkerThread() and schedule or
complete are not needed as a part of task's job. Those are the
responsibilities of task's owner (TileManager - compositor thread).
This patch removes following functions which were basically needed for
async upload. Now raster buffer is provided to task at the time of ctor
and functionality of CompleteOnOriginThread() is moved to task's owner.

  ScheduleOnOriginThread()
  CompleteOnOriginThread()

New OnTaskCompleted() function calls corresponding function of task
owner. This patch implements following life cycle for the task.

  1. Task's owner creates task with all needed info on origin thread.
  2. Task is scheduled and run on worker thread.
  3. Completed task is processed on origin thread by task owner.
  4. Task is destroyed.

This patch also fixes few task related failing cc_perftests (607818).

BUG=499372, 599863, 607818, 613529
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/06e1561a832083330a193743a427b1354ab3cd39
Cr-Commit-Position: refs/heads/master@{#396218}
==========

[commit-bot: I haz the power, 2016-05-26 17:38:04]

Patchset 28 (id:??) landed as
https://crrev.com/06e1561a832083330a193743a427b1354ab3cd39
Cr-Commit-Position: refs/heads/master@{#396218}