cc: Move GPU raster to DirectRasterWorkerPool.

cc: Move GPU raster to DirectRasterWorkerPool.

GPU raster belong in its own RasterWorkerPool implementation as
it requires different throttling logic. This moves all GPU raster
to DirectRasterWorkerPool.

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

[reveman, 2014-02-14 16:11:53]

+vangelis

The more I'm thinking about it the more having a DirectRasterWorkerPool
implementation makes sense to me. The only downside I know of is that image
decode tasks can't be shared between gpu and sw raster. That should be fine
though as long as we're not considering mixing gpu and sw raster for tiles in
the same layer.

https://codereview.chromium.org/165603002/diff/1/cc/resources/direct_raster_w...
File cc/resources/direct_raster_worker_pool.cc (right):

https://codereview.chromium.org/165603002/diff/1/cc/resources/direct_raster_w...
cc/resources/direct_raster_worker_pool.cc:37: void
DirectRasterWorkerPool::ScheduleTasks(RasterTaskQueue* queue) {
Technically this needs to cancel tasks that have not yet run. I'll fix this.

FYI, the need to do this will be removed in the follow up patch that makes
TileVersion a Task as that allows resources to only be pinned to tiles between
::ScheduleTileOnOriginThread/::CompleteTileOnOriginThread.

https://codereview.chromium.org/165603002/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/165603002/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool.h:67: class CC_EXPORT RasterWorkerPoolTask :
public WorkerPoolTask {
should now be relatively easy to remove this class as a follow up.

https://codereview.chromium.org/165603002/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool.h:137: static
scoped_refptr<internal::RasterWorkerPoolTask> CreateRasterTask(
Fyi, I'll remove this in follow up patch that makes TileVersion a
WorkerPoolTask.

[vmpstr, 2014-02-14 17:16:51]

I think this is great.

https://codereview.chromium.org/165603002/diff/1/cc/resources/raster_worker_p...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/165603002/diff/1/cc/resources/raster_worker_p...
cc/resources/raster_worker_pool.h:137: static
scoped_refptr<internal::RasterWorkerPoolTask> CreateRasterTask(
On 2014/02/14 16:11:53, David Reveman wrote:
> Fyi, I'll remove this in follow up patch that makes TileVersion a
> WorkerPoolTask.

That would be awesome.

https://codereview.chromium.org/165603002/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/165603002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:134: class RasterWorkerPoolDelegate : public
RasterWorkerPoolClient {
Can you please make this a separate file?

I was thinking of making this more generic where you can pass a vector of raster
worker pools, and then pass a vector of tasks to schedule of the same size and
iterate over each and schedule. That way, if say gpu raster is disabled, we only
have a vector of 1 element and avoid unnecessary schedules or whatnot. But I
think this is fine as well. No need to over-engineer.

[alokp, 2014-02-14 18:50:58]

Dave: Thanks for working on it. I have some minor comments.

https://codereview.chromium.org/165603002/diff/1/cc/resources/direct_raster_w...
File cc/resources/direct_raster_worker_pool.cc (right):

https://codereview.chromium.org/165603002/diff/1/cc/resources/direct_raster_w...
cc/resources/direct_raster_worker_pool.cc:149:
RunTaskOnOriginThread(raster_required_for_activation_finished_task());
shouldn't activation-finished be run before raster-finished?

https://codereview.chromium.org/165603002/diff/1/cc/resources/direct_raster_w...
File cc/resources/direct_raster_worker_pool.h (right):

https://codereview.chromium.org/165603002/diff/1/cc/resources/direct_raster_w...
cc/resources/direct_raster_worker_pool.h:21: virtual void
ScheduleTasks(RasterTaskQueue* queue) OVERRIDE;
Is it necessary to make this change in this patch? Can it be done separately?

https://codereview.chromium.org/165603002/diff/1/cc/resources/direct_raster_w...
cc/resources/direct_raster_worker_pool.h:49: bool
run_tasks_on_origin_thread_pending_;
nit: can it be just tasks_pending_ for now?

https://codereview.chromium.org/165603002/diff/1/cc/resources/image_raster_wo...
File cc/resources/image_raster_worker_pool.cc (left):

https://codereview.chromium.org/165603002/diff/1/cc/resources/image_raster_wo...
cc/resources/image_raster_worker_pool.cc:59: size_t
raster_required_for_activation_finished_dependency_count = 0u;
Are these changes necessary in this patch?

https://codereview.chromium.org/165603002/diff/1/cc/resources/managed_tile_st...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/165603002/diff/1/cc/resources/managed_tile_st...
cc/resources/managed_tile_state.h:98:
scoped_refptr<internal::RasterWorkerPoolTask> raster_task_;
can it be done in a separate patch?

https://codereview.chromium.org/165603002/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/165603002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:226:
DirectRasterWorkerPool::Create(resource_provider, context_provider),
I was under the impression that we would create a HybridRasterWorkerPool and
push all the complexity of managing two raster worker pools from the tile
manager to the new class.

[reveman, 2014-02-14 23:30:52]

PTAL

https://codereview.chromium.org/165603002/diff/1/cc/resources/direct_raster_w...
File cc/resources/direct_raster_worker_pool.cc (right):

https://codereview.chromium.org/165603002/diff/1/cc/resources/direct_raster_w...
cc/resources/direct_raster_worker_pool.cc:37: void
DirectRasterWorkerPool::ScheduleTasks(RasterTaskQueue* queue) {
On 2014/02/14 16:11:53, David Reveman wrote:
> Technically this needs to cancel tasks that have not yet run. I'll fix this.

Done.

> 
> FYI, the need to do this will be removed in the follow up patch that makes
> TileVersion a Task as that allows resources to only be pinned to tiles between
> ::ScheduleTileOnOriginThread/::CompleteTileOnOriginThread.

https://codereview.chromium.org/165603002/diff/1/cc/resources/direct_raster_w...
cc/resources/direct_raster_worker_pool.cc:136: DCHECK_EQ(0u,
task->dependencies().size());
Turned out this needs to be implement for unit and perf tests to work. I think
that's the correct thing to do though. Makes everything a bit more consistent.

https://codereview.chromium.org/165603002/diff/1/cc/resources/direct_raster_w...
cc/resources/direct_raster_worker_pool.cc:149:
RunTaskOnOriginThread(raster_required_for_activation_finished_task());
On 2014/02/14 18:50:59, Alok Priyadarshi wrote:
> shouldn't activation-finished be run before raster-finished?

Good point. Done. This is not currently required but generally a good idea.
Maybe we should have it be required. Just need to make raster-finished a
dependent of raster-required-for-activation-finished in the multi-threaded
case...

https://codereview.chromium.org/165603002/diff/1/cc/resources/direct_raster_w...
File cc/resources/direct_raster_worker_pool.h (right):

https://codereview.chromium.org/165603002/diff/1/cc/resources/direct_raster_w...
cc/resources/direct_raster_worker_pool.h:21: virtual void
ScheduleTasks(RasterTaskQueue* queue) OVERRIDE;
On 2014/02/14 18:50:59, Alok Priyadarshi wrote:
> Is it necessary to make this change in this patch? Can it be done separately?

Done.

https://codereview.chromium.org/165603002/diff/1/cc/resources/direct_raster_w...
cc/resources/direct_raster_worker_pool.h:49: bool
run_tasks_on_origin_thread_pending_;
On 2014/02/14 18:50:59, Alok Priyadarshi wrote:
> nit: can it be just tasks_pending_ for now?

I like having this name match
ScheduleRunTasksOnOriginThread()/RunTasksOnOriginThread(). This can be true even
if there are no raster tasks pending in which case tasks_pending_ might be a bit
confusing.

https://codereview.chromium.org/165603002/diff/1/cc/resources/image_raster_wo...
File cc/resources/image_raster_worker_pool.cc (left):

https://codereview.chromium.org/165603002/diff/1/cc/resources/image_raster_wo...
cc/resources/image_raster_worker_pool.cc:59: size_t
raster_required_for_activation_finished_dependency_count = 0u;
On 2014/02/14 18:50:59, Alok Priyadarshi wrote:
> Are these changes necessary in this patch?

No but I think they should be included as the only reason we needed these
variables before was because some tasks could be gpu raster tasks. I think they
should be removed in this patch now that they are no longer needed.

https://codereview.chromium.org/165603002/diff/1/cc/resources/managed_tile_st...
File cc/resources/managed_tile_state.h (right):

https://codereview.chromium.org/165603002/diff/1/cc/resources/managed_tile_st...
cc/resources/managed_tile_state.h:98:
scoped_refptr<internal::RasterWorkerPoolTask> raster_task_;
On 2014/02/14 18:50:59, Alok Priyadarshi wrote:
> can it be done in a separate patch?

Done.

https://codereview.chromium.org/165603002/diff/1/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/165603002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:134: class RasterWorkerPoolDelegate : public
RasterWorkerPoolClient {
On 2014/02/14 17:16:51, vmpstr wrote:
> Can you please make this a separate file?

Done.

> 
> I was thinking of making this more generic where you can pass a vector of
raster
> worker pools, and then pass a vector of tasks to schedule of the same size and
> iterate over each and schedule. That way, if say gpu raster is disabled, we
only
> have a vector of 1 element and avoid unnecessary schedules or whatnot. But I
> think this is fine as well. No need to over-engineer.

Have a look at the latest patch. I'm currently not doing anything to avoid
scheduling tasks to all worker pools. I'm hoping we can get away with that for
simplicity.

https://codereview.chromium.org/165603002/diff/1/cc/resources/tile_manager.cc...
cc/resources/tile_manager.cc:226:
DirectRasterWorkerPool::Create(resource_provider, context_provider),
On 2014/02/14 18:50:59, Alok Priyadarshi wrote:
> I was under the impression that we would create a HybridRasterWorkerPool and
> push all the complexity of managing two raster worker pools from the tile
> manager to the new class.

Yes, that might be what we want to do long term but I prefer to wait until we're
done with all cleanup to the RasterWorkerPool interface first.

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

https://codereview.chromium.org/165603002/diff/170001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:116: virtual void RunOnOriginThread()
OVERRIDE {
This shouldn't behave any different from RunOnWorkerThread except that there's
no need to use a picture clone.

FYI, this now calls Analyze as it should. Maybe we should add a flag to disable
that if it's not something we like to do for gpu raster. The flag should apply
to both RunOnWorkerThread and RunOnOriginThread. It might be easiest to wait
until TileVersion is a Task before we do this..

[vmpstr, 2014-02-14 23:47:24]

Cool, this is exactly what I meant by more generic comment. lgtm % alokp's
review.

https://codereview.chromium.org/165603002/diff/170001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/165603002/diff/170001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:201: raster_worker_pool_.get(),
direct_raster_worker_pool_.get(), };
nit: No need for the trailing comma.

https://codereview.chromium.org/165603002/diff/170001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:205: raster_worker_pools +
arraysize(raster_worker_pools));
nit: Maybe pass NUM_RASTER_WORKER_POOL_TYPES instead as a count and do rwps,
rwps + count for the vector constructor? Either one is fine I guess.

[reveman, 2014-02-15 00:02:44]

https://codereview.chromium.org/165603002/diff/170001/cc/resources/tile_manag...
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/165603002/diff/170001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:201: raster_worker_pool_.get(),
direct_raster_worker_pool_.get(), };
On 2014/02/14 23:47:24, vmpstr wrote:
> nit: No need for the trailing comma.

Done.

https://codereview.chromium.org/165603002/diff/170001/cc/resources/tile_manag...
cc/resources/tile_manager.cc:205: raster_worker_pools +
arraysize(raster_worker_pools));
On 2014/02/14 23:47:24, vmpstr wrote:
> nit: Maybe pass NUM_RASTER_WORKER_POOL_TYPES instead as a count and do rwps,
> rwps + count for the vector constructor? Either one is fine I guess.

Done. Passing arraysize.

[alokp, 2014-02-15 00:14:13]

lgtm

[reveman, 2014-02-15 00:24:24]

The CQ bit was checked by reveman@chromium.org

[reveman, 2014-02-15 00:27:05]

The CQ bit was unchecked by reveman@chromium.org

[reveman, 2014-02-15 00:54:23]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-02-15 00:58:47]

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

[commit-bot: I haz the power, 2014-02-15 02:59:01]

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

[commit-bot: I haz the power, 2014-02-15 02:59:01]

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

[reveman, 2014-02-15 03:43:00]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-02-15 03:43:37]

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

[reveman, 2014-02-15 05:00:29]

The CQ bit was unchecked by reveman@chromium.org

[reveman, 2014-02-15 08:27:10]

The CQ bit was checked by reveman@chromium.org

[commit-bot: I haz the power, 2014-02-15 08:27:20]

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

[commit-bot: I haz the power, 2014-02-15 14:22:01]

Change committed as 251569

[vangelis, 2014-02-18 22:25:28]

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

https://codereview.chromium.org/165603002/diff/600001/cc/resources/raster_wor...
cc/resources/raster_worker_pool.cc:119: Analyze(picture_pile_);
I think the Analysis is an overkill for ganesh mode. With the high upload costs
removed, I think it's fine to live with the additional texture mem cost for the
rare cases where a tile is truly solid.

[reveman, 2014-02-18 23:29:34]

On 2014/02/18 22:25:28, vangelis wrote:
>
https://codereview.chromium.org/165603002/diff/600001/cc/resources/raster_wor...
> File cc/resources/raster_worker_pool.cc (right):
> 
>
https://codereview.chromium.org/165603002/diff/600001/cc/resources/raster_wor...
> cc/resources/raster_worker_pool.cc:119: Analyze(picture_pile_);
> I think the Analysis is an overkill for ganesh mode. With the high upload
costs
> removed, I think it's fine to live with the additional texture mem cost for
the
> rare cases where a tile is truly solid.

See my comment on patch set 3. I added this as we shouldn't make the assumption
that gpu raster is used here. Can easily be fixed by adding a flag on the task.

[vangelis, 2014-02-19 03:01:36]

Oh, I see.

Should I go ahead and add the use_gpu_rasterization flag back
to RasterWorkerPool::RasterTask ? The analysis step is really hurting us.



On Tue, Feb 18, 2014 at 3:29 PM, <reveman@chromium.org> wrote:

> On 2014/02/18 22:25:28, vangelis wrote:
>
> https://codereview.chromium.org/165603002/diff/600001/cc/
> resources/raster_worker_pool.cc
>
>> File cc/resources/raster_worker_pool.cc (right):
>>
>
>
> https://codereview.chromium.org/165603002/diff/600001/cc/
> resources/raster_worker_pool.cc#newcode119
>
>> cc/resources/raster_worker_pool.cc:119: Analyze(picture_pile_);
>> I think the Analysis is an overkill for ganesh mode. With the high upload
>>
> costs
>
>> removed, I think it's fine to live with the additional texture mem cost
>> for
>>
> the
>
>> rare cases where a tile is truly solid.
>>
>
> See my comment on patch set 3. I added this as we shouldn't make the
> assumption
> that gpu raster is used here. Can easily be fixed by adding a flag on the
> task.
>
> https://codereview.chromium.org/165603002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[reveman, 2014-02-19 03:14:26]

On 2014/02/19 03:01:36, vangelis wrote:
> Oh, I see.
> 
> Should I go ahead and add the use_gpu_rasterization flag back
> to RasterWorkerPool::RasterTask ? The analysis step is really hurting us.
> 

Yes, or maybe even better: a use_analysis flag. Tile manager can just use
Tile::use_gpu_rasterization() for now.