cc: Add RasterWorkerPool class.

cc: Add RasterWorkerPool class.
    
This moves thread pool related code out of the tile manager and into a
new RasterWorkerPool class. This doesn't change the behavior of the tile
manager in any way it just separates tile management logic from the
thread handling logic, which results in a significant cleanup of the
tile manager.
    
BUG=155209
TEST=manual

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

[reveman, 2012-12-18 17:50:30]

Hey, what do you think about this for separating the thread worker pool related
code from the tile manager?

[nduca, 2012-12-19 04:46:50]

https://codereview.chromium.org/11593030/diff/1/cc/raster_worker.h
File cc/raster_worker.h (right):

https://codereview.chromium.org/11593030/diff/1/cc/raster_worker.h#newcode23
cc/raster_worker.h:23: class RasterWorker {
RasterThreadPool? Worker implies that it is only one thread.

https://codereview.chromium.org/11593030/diff/1/cc/tile_manager.cc
File cc/tile_manager.cc (left):

https://codereview.chromium.org/11593030/diff/1/cc/tile_manager.cc#oldcode646
cc/tile_manager.cc:646: // Release raster resources.
I guess I'm a bit confused about why this is stuff is still here... it seems
like you moved out all the raster thread stuff but you didn't move out the
scheduling and upload management. I think thats fine, but there may be a
followup refactoring to split out stuff that is based on the
tiles_that_need_to_be_rasterized_ and drives the rasterized.

I think the end flow is

TileMgr -> TilePaintQueue -> TileMgr
                |
            RasterThreadPool

Right?

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

https://codereview.chromium.org/11593030/diff/1/cc/tile_manager.cc#newcode365
cc/tile_manager.cc:365: if (raster_worker_->IsBusy())
how about making it bool DispatchOneBlahBlah? In the lockless programming world,
you often need to fuse these.

Its like why a threadsafe queue doesn't have a size() and a pop-that-succeeds().
Its bool pop(T& result) since you need to lock the queue to decide if you can
pop.

[reveman, 2012-12-27 15:26:58]

https://codereview.chromium.org/11593030/diff/1/cc/raster_worker.h
File cc/raster_worker.h (right):

https://codereview.chromium.org/11593030/diff/1/cc/raster_worker.h#newcode23
cc/raster_worker.h:23: class RasterWorker {
On 2012/12/19 04:46:50, nduca wrote:
> RasterThreadPool? Worker implies that it is only one thread.

Right. I made it RasterWorkerPool in latest patch, which is somewhat consistent
with base::SequencedWorkerPool.

https://codereview.chromium.org/11593030/diff/1/cc/tile_manager.cc
File cc/tile_manager.cc (left):

https://codereview.chromium.org/11593030/diff/1/cc/tile_manager.cc#oldcode646
cc/tile_manager.cc:646: // Release raster resources.
On 2012/12/19 04:46:50, nduca wrote:
> I guess I'm a bit confused about why this is stuff is still here... it seems
> like you moved out all the raster thread stuff but you didn't move out the
> scheduling and upload management. I think thats fine, but there may be a
> followup refactoring to split out stuff that is based on the
> tiles_that_need_to_be_rasterized_ and drives the rasterized.
> 
> I think the end flow is
> 
> TileMgr -> TilePaintQueue -> TileMgr
>                 |
>             RasterThreadPool
> 
> Right?

I was planning to do some of that in a second patch. The hard part is that we
want to re-assign gpu memory to tiles when a raster task completes to decide if
tile initialization should be aborted or not and pixel buffer handling is
directly affected by the decision to abort. I need some more time to think about
how that is best done.

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

https://codereview.chromium.org/11593030/diff/1/cc/tile_manager.cc#newcode365
cc/tile_manager.cc:365: if (raster_worker_->IsBusy())
On 2012/12/19 04:46:50, nduca wrote:
> how about making it bool DispatchOneBlahBlah? In the lockless programming
world,
> you often need to fuse these.
> 
> Its like why a threadsafe queue doesn't have a size() and a
pop-that-succeeds().
> Its bool pop(T& result) since you need to lock the queue to decide if you can
> pop.

Makes sense. It will be easy to do this once resource allocation is moved to the
RasterWorkerPool class but it's hard until then unless we stop avoiding to
allocate gpu resources until actually needed.

[nduca, 2013-01-05 07:24:44]

David, did I stall you out here with my wierd feedback, or is this still moving
forward?

[reveman, 2013-01-05 18:26:40]

On 2013/01/05 07:24:44, nduca wrote:
> David, did I stall you out here with my wierd feedback, or is this still
moving
> forward?

Please have a look at my last set of comments and the latest patch. I'm
reluctant to doing more in this first patch.

[nduca, 2013-01-05 21:35:02]

I guess I'm just struggling to see how this makes things better. Its pretty
unclean feeling to me as is.

Maybe we need to split up RasterWorkerPool as is into RasterWorkerPool which
itself has a LoadBalancingWorkerPool, which has the Thread and Task classes.

[reveman, 2013-01-06 19:26:57]

On 2013/01/05 21:35:02, nduca wrote:
> I guess I'm just struggling to see how this makes things better. Its pretty
> unclean feeling to me as is.
> 
> Maybe we need to split up RasterWorkerPool as is into RasterWorkerPool which
> itself has a LoadBalancingWorkerPool, which has the Thread and Task classes.

The main purpose is to separate tile management from thread handling logic and I
think the patch achieves that.

The current patch avoids multiple levels of Bind()/Run() using the Task class. I
think it would be cleaner without that (which I think is similar to what you're
suggesting with a LoadBalancingWorkerPool) but I'm a bit worried about the
overhead from multiple Bind()/Run() pairs for each raster task. Maybe I
shouldn't be?

[nduca, 2013-01-08 05:45:36]

Okay. LGTM but lets keep refining this.

[commit-bot: I haz the power, 2013-01-09 16:46:08]

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

[commit-bot: I haz the power, 2013-01-09 17:42:22]

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

[commit-bot: I haz the power, 2013-01-09 20:42:09]

Change committed as 175877