Re-land: cc: Cancel and re-prioritize worker pool tasks.

Re-land: cc: Cancel and re-prioritize worker pool tasks.

This adds a task graph interface to the worker pool and
implements a simple queue instance of this interface for
use by the tile manager.

The task graph interface can be used describe more
complicated task dependencies in the future and
provides the immediate benefit of seamlessly being
able to cancel and re-prioritize tasks.

BUG=178974, 244642
TEST=cc_unittests --gtest_filter=WorkerPoolTest.Dependencies

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

[vmpstr, 2013-05-06 18:05:51]

Just a first run through. I'm sorry if some comments are a bit silly, I'm just
trying to understand the code at this point.

https://codereview.chromium.org/14689004/diff/1/cc/resources/raster_worker_po...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/14689004/diff/1/cc/resources/raster_worker_po...
cc/resources/raster_worker_pool.cc:76: }
nit: I kinda prefer blank lines between these functions for readability

https://codereview.chromium.org/14689004/diff/1/cc/resources/raster_worker_po...
cc/resources/raster_worker_pool.cc:86: TaskDeque::iterator it =
std::find(tasks_.begin(), tasks_.end(), task);
can you DCHECK that it != tasks_.end. It would seem weird if you call TakeTask
with a task that is not in tasks_

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

https://codereview.chromium.org/14689004/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:627: bool
TileManager::DispatchImageDecodeTasksForTile(Tile* tile) {
I guess my earlier comment is asking to just rename this function, instead of
inlining it

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

https://codereview.chromium.org/14689004/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:512: // Tile shouldn't be rasterized if we've
failed to assign
If a large tile is OOM, followed by a smaller tile that has enough memory, this
would assign memory to it (via set_use_resource) and subtract the memory from
the budget, but then not enqueue it for raster. Is this intentional? I'm not
sure I understand the intent

https://codereview.chromium.org/14689004/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:578: DCHECK_NE(USING_UNRELEASABLE_MEMORY,
tile->drawing_info().memory_state_);
nit: I kinda prefer the constant to be the second parameter, up to you

https://codereview.chromium.org/14689004/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:616: for (PicturePileImpl::PixelRefIterator
iter(tile->content_rect(),
Is it possible to refactor this loop into a separate function? Something like
UpdateDecodeTasksIfNeeded(tile, &decode_tasks, &task_queue) -> true iff it has
pending decode tasks.

I'm having a bit of trouble keeping decode_tasks, task_queue, and
pending_decode_tasks_ clear in my mind. Maybe this would help

https://codereview.chromium.org/14689004/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:689: // previously scheduled tasks and effectively
cancels any tasks
How do we cancel an upload? (or ensure upload continues?)

https://codereview.chromium.org/14689004/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:749: CHECK(!(reinterpret_cast<intptr_t>(buffer) &
3));
I would vote that this moves into MapPixelBuffer, but that's unrelated to this
change.

https://codereview.chromium.org/14689004/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:805: tile->drawing_info().memory_state_ =
CAN_USE_MEMORY;
Why can use memory? I would think that if it was cancelled, then it would go
into NOT_ALLOWED_TO_USE_MEMORY, unless AssignGpuMemory decides to allocate
memory to it again

[reveman, 2013-05-06 21:36:27]

https://codereview.chromium.org/14689004/diff/1/cc/resources/raster_worker_po...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/14689004/diff/1/cc/resources/raster_worker_po...
cc/resources/raster_worker_pool.cc:76: }
On 2013/05/06 18:05:51, vmpstr wrote:
> nit: I kinda prefer blank lines between these functions for readability

I avoid the use of a blank line here to make it clear that all these functions
are part of the "Overridden from internal::WorkerPoolTaskGraph" group. Typically
that's done in a header and not using a blank line there is cleaner. I prefer to
keep it as is but could avoid inlining of these functions completely if you
think it's too hard to read.

https://codereview.chromium.org/14689004/diff/1/cc/resources/raster_worker_po...
cc/resources/raster_worker_pool.cc:86: TaskDeque::iterator it =
std::find(tasks_.begin(), tasks_.end(), task);
On 2013/05/06 18:05:51, vmpstr wrote:
> can you DCHECK that it != tasks_.end. It would seem weird if you call TakeTask
> with a task that is not in tasks_

I allowed task to not always be in tasks_ for efficient (avoids one extra scan
through all tasks) but it's probably better to take that cost in favor of
non-awkwardness. Changed this in latest patch. Making HasTask() use a hash will
fix the efficiency problem if necessary.

Fyi, WorkerPool::Inner::ScheduleTasks() has to remove tasks from a graph without
knowing in advance that they exist.

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

https://codereview.chromium.org/14689004/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:627: bool
TileManager::DispatchImageDecodeTasksForTile(Tile* tile) {
On 2013/05/06 18:05:51, vmpstr wrote:
> I guess my earlier comment is asking to just rename this function, instead of
> inlining it

Done.

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

https://codereview.chromium.org/14689004/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:512: // Tile shouldn't be rasterized if we've
failed to assign
Yes, being assigned memory is not the same as being scheduled for raster after
this change. We need this to be different to be able to cancel tasks.

tiles_that_need_to_be_rasterized_ is essentially all tiles that have memory
assigned to them AND are high enough priority at the moment that we want them
initialized. Not adding a tile to tiles_that_need_to_be_rasterized_ will cause
any pending raster task for that tile to be cancelled asap.

made some minor changes to this code in latest patch to make it cleaner.

https://codereview.chromium.org/14689004/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:578: DCHECK_NE(USING_UNRELEASABLE_MEMORY,
tile->drawing_info().memory_state_);
On 2013/05/06 18:05:51, vmpstr wrote:
> nit: I kinda prefer the constant to be the second parameter, up to you

DCHECK_NE is used in both ways throughout chromium and I would also prefer the
constant as second if EXPECT_EQ and likes didn't prefer it to be the other way
around.

https://codereview.chromium.org/14689004/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:616: for (PicturePileImpl::PixelRefIterator
iter(tile->content_rect(),
On 2013/05/06 18:05:51, vmpstr wrote:
> Is it possible to refactor this loop into a separate function? Something like
> UpdateDecodeTasksIfNeeded(tile, &decode_tasks, &task_queue) -> true iff it has
> pending decode tasks.
> 
> I'm having a bit of trouble keeping decode_tasks, task_queue, and
> pending_decode_tasks_ clear in my mind. Maybe this would help

Done.

https://codereview.chromium.org/14689004/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:689: // previously scheduled tasks and effectively
cancels any tasks
On 2013/05/06 18:05:51, vmpstr wrote:
> How do we cancel an upload? (or ensure upload continues?)

We can't cancel an upload at the moment but that's a separate issue that I
intend to solve later (requires command buffer API changes).

We already avoid to issue an upload if |was_cancelled| is true in
OnRasterTaskCompleted().

https://codereview.chromium.org/14689004/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:749: CHECK(!(reinterpret_cast<intptr_t>(buffer) &
3));
On 2013/05/06 18:05:51, vmpstr wrote:
> I would vote that this moves into MapPixelBuffer, but that's unrelated to this
> change.

I agree. Moving it RP and closer to the source as part of this CL.

https://codereview.chromium.org/14689004/diff/1/cc/resources/tile_manager.cc#...
cc/resources/tile_manager.cc:805: tile->drawing_info().memory_state_ =
CAN_USE_MEMORY;
The tile will always be allowed to use memory at the time
OnRasterTaskCompleted() is called. AssignGpuMemory needs to be called for that
to change. Setting it to  NOT_ALLOWED_TO_USE_MEMORY can cause incorrect behavior
in ScheduleTasks() when tile was cancelled because of throttling.

I just realized a minor issue in the current implementation. If we're OOM and a
tile previously using unreleasable memory transitions to CAN_USE_MEMORY or
USING_RELEASABLE_MEMORY, AssignGpuMemory needs to be called to ensure that
memory is accurately assigned to tiles. That's not a new issue though. And it
probably doesn't have a significant impact.

[vmpstr, 2013-05-06 23:08:48]

This is looking very good. Some more comments:

https://codereview.chromium.org/14689004/diff/7001/cc/base/worker_pool.cc
File cc/base/worker_pool.cc (right):

https://codereview.chromium.org/14689004/diff/7001/cc/base/worker_pool.cc#new...
cc/base/worker_pool.cc:288: if (shutdown_)
Now that task_graph_ contains all work that needs to be done, with just one
raster thread if we call Shutdown, it would still finish doing all the work
first (the enclosing if would fail). Is this by design?

https://codereview.chromium.org/14689004/diff/7001/cc/base/worker_pool.cc#new...
cc/base/worker_pool.cc:291: // Schedule an idle callback if not tasks are
running.
nit: not -> no

https://codereview.chromium.org/14689004/diff/7001/cc/base/worker_pool.cc#new...
cc/base/worker_pool.cc:397: while (completed_tasks->size()) {
nit: consider !empty

https://codereview.chromium.org/14689004/diff/7001/cc/resources/raster_worker...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/14689004/diff/7001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:49:
callback_.Run(picture_pile_->GetCloneForDrawingOnThread(thread_index));
It's a bit unfortunate that we have to specialize RasterWorkerPoolTaskImpl just
because we need this line :\

Is it maybe possible to do something like the following:
Bind(&SelectCloneAndRun, 
     picture_pile,
     Bind(&ThingToRun, ...));

SelectCloneAndRun(pile, callback) would simply do
callback->Run(pile->GetCloneForDrawingOnThread(thread_id));

The only problem is getting the running thread id in SelectCloneAndRun. Is that
available somehow from the callback? I mean one option is to just always pass a
thread_index to the callback, but I think that might clutter all the callbacks.

https://codereview.chromium.org/14689004/diff/7001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:195: // Note: DidSchedule() is only used for
DCHECKs.
If we're only doing these for DCHECKs, is it worth to put a TODO to remove this
extra loop eventually?

https://codereview.chromium.org/14689004/diff/7001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/14689004/diff/7001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:620: // Skip tile if image decode is pending.
Why do we do this? I thought this change would allow us to still schedule a
raster after we schedule a decode.

Is the problem that multiple threads can do wrong things? Like if one is doing a
decode, another one isn't allowed to grab a raster task?

[reveman, 2013-05-07 01:33:54]

https://codereview.chromium.org/14689004/diff/7001/cc/base/worker_pool.cc
File cc/base/worker_pool.cc (right):

https://codereview.chromium.org/14689004/diff/7001/cc/base/worker_pool.cc#new...
cc/base/worker_pool.cc:288: if (shutdown_)
Yes, we could cancel all pending tasks at shutdown but the way to do that is by
scheduling an empty task graph as we still need to perform all completion
callbacks.

I'd like to keep the WorkerPool class generic and as it's trivial for the client
to do this I'd rather not have it be automatically done by
WorkerPool::Shutdown().

Cancel of all tasks is the prefered behavior for the tile manager though. Latest
patch takes care of this automatically in RasterWorkerPool::Shutdown().

https://codereview.chromium.org/14689004/diff/7001/cc/base/worker_pool.cc#new...
cc/base/worker_pool.cc:291: // Schedule an idle callback if not tasks are
running.
On 2013/05/06 23:08:48, vmpstr wrote:
> nit: not -> no

Done.

https://codereview.chromium.org/14689004/diff/7001/cc/base/worker_pool.cc#new...
cc/base/worker_pool.cc:397: while (completed_tasks->size()) {
On 2013/05/06 23:08:48, vmpstr wrote:
> nit: consider !empty

Done.

https://codereview.chromium.org/14689004/diff/7001/cc/resources/raster_worker...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/14689004/diff/7001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:49:
callback_.Run(picture_pile_->GetCloneForDrawingOnThread(thread_index));
On 2013/05/06 23:08:48, vmpstr wrote:
> It's a bit unfortunate that we have to specialize RasterWorkerPoolTaskImpl
just
> because we need this line :\
> 
> Is it maybe possible to do something like the following:
> Bind(&SelectCloneAndRun, 
>      picture_pile,
>      Bind(&ThingToRun, ...));
> 
> SelectCloneAndRun(pile, callback) would simply do
> callback->Run(pile->GetCloneForDrawingOnThread(thread_id));
> 
> The only problem is getting the running thread id in SelectCloneAndRun. Is
that
> available somehow from the callback? I mean one option is to just always pass
a
> thread_index to the callback, but I think that might clutter all the
callbacks.

I'd rather not expose the thread id. Also there's a bit of overhead from every
base::Bind so I'd avoid adding another one unless it's a significant cleanup. I
think it's ok to maintain a few lines of duplicate code here for now. We can
remove it once https://code.google.com/p/skia/issues/detail?id=1143 has been
fixed.

https://codereview.chromium.org/14689004/diff/7001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:195: // Note: DidSchedule() is only used for
DCHECKs.
On 2013/05/06 23:08:48, vmpstr wrote:
> If we're only doing these for DCHECKs, is it worth to put a TODO to remove
this
> extra loop eventually?

we could #if !defined(NDEBUG) all this stuff but at this point I don't think
it's even worth a TODO. If we at a later point need to do some micro
optimizations of this code the current comment should be enough to make it clear
that this can easily be improved.

https://codereview.chromium.org/14689004/diff/7001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/14689004/diff/7001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:620: // Skip tile if image decode is pending.
On 2013/05/06 23:08:48, vmpstr wrote:
> Why do we do this? I thought this change would allow us to still schedule a
> raster after we schedule a decode.
> 
> Is the problem that multiple threads can do wrong things? Like if one is doing
a
> decode, another one isn't allowed to grab a raster task?

We need to implement a WorkerPoolTaskGraph with task dependencies to fix this.
The simple queue that is included in this patch doesn't provide this.

Saving it for a follow up as I'm trying to not do too much in this patch.

[vmpstr, 2013-05-09 00:29:52]

lgtm

[nduca, 2013-05-10 21:23:06]

I gave this more thought on the flight, and I really think a model of

Task
  vector<Task> dependencies
  int runCount

Would be sufficient. Once a task is enqueued, you cant add new dependencies to
it. With that restriction, its completely efficient to implement, and you dont
need a big taskgraph object that you can replace.

[reveman, 2013-05-14 00:29:02]

On 2013/05/10 21:23:06, nduca wrote:
> I gave this more thought on the flight, and I really think a model of
> 
> Task
>   vector<Task> dependencies
>   int runCount

I don't mind getting rid of the TaskGraph abstraction and only allowing one
structure if that's preferred. The task structure you describe here is
essentially what I was planning to add as a follow up to this patch. I can
include that in this patch if we feel that's necessary but would prefer it as a
follow up.

However, this doesn't change much afaict. We still need to be able to cancel,
re-prioritize and change dependencies of tasks in a non-blocking way and
building of a new graph the replaces the current one is a simple and efficient
way to accomplish this.

> 
> Would be sufficient. Once a task is enqueued, you cant add new dependencies to
> it. With that restriction, its completely efficient to implement, and you dont
> need a big taskgraph object that you can replace.

I don't follow this. How are you suggesting that change dependencies,
re-prioritize and cancel tasks in this system?

[reveman, 2013-05-20 23:09:26]

Latest patch includes full support for task dependencies. I think it's cleaner
and easier to just do this right away instead of some intermediate state where
we have a TaskGraph abstraction. All the idle callback code can be removed as a
result of this but I prefer to leave that as a follow up.

PTAL.

[nduca, 2013-05-21 00:31:12]

https://codereview.chromium.org/14689004/diff/19001/cc/base/worker_pool.h
File cc/base/worker_pool.h (right):

https://codereview.chromium.org/14689004/diff/19001/cc/base/worker_pool.h#new...
cc/base/worker_pool.h:46: 
I guess i'm still puzzled why we have dependency separate from task. Why not
have WorkerPoolTask have a m_dependencies<WorkerPoolTask*>?

[reveman, 2013-05-21 01:22:36]

https://codereview.chromium.org/14689004/diff/19001/cc/base/worker_pool.h
File cc/base/worker_pool.h (right):

https://codereview.chromium.org/14689004/diff/19001/cc/base/worker_pool.h#new...
cc/base/worker_pool.h:46: 
There's a 1-N relationship between tasks and dependency graphs.

A graph is a description of dependencies and priorities of tasks that is used by
the worker pool to determine what task to run. To change this description, we
build a new dependency graph that in one atomic operation ("atomic" from the
workers point of view) replaces the old dependency graph. This new graph will
often reference existing tasks already present in the currently active graph.
This allows us to efficiently change dependencies and priorities without
stopping the workers (except for the quick graph swap operation).

[nduca, 2013-05-21 02:12:26]

It seems to me that if you had tasks have dependencies, then when you enqueue an
image task, you just store on its pixelref object its task*. Then when you make
a raster task against it, you just ref->decode_task->addDependency(rasterTask).
That seems more modular to me?

[Tom Hudson, 2013-05-21 11:40:05]

I still don't understand why "cancel and reprioritize" means we have to have a
general task graph. Is there a design doc that explains the use cases that
require the extra complexity?

[reveman, 2013-05-21 16:18:26]

On 2013/05/21 02:12:26, nduca wrote:
> It seems to me that if you had tasks have dependencies, then when you enqueue
an
> image task, you just store on its pixelref object its task*. Then when you
make
> a raster task against it, you just
ref->decode_task->addDependency(rasterTask).
> That seems more modular to me?

Not clear to me exactly what you're suggesting. The dependents of a task will
change over time. AssignGpuMem will cause some raster tasks to no longer be
dependents of an image decode and some new ones to become.

The opposite is not true though (a raster task will always need the same images
to be decoded) and I'm somewhat interested in making the set of tasks that one
task can depend on immutable. The current patch doesn't have this limitation but
it might be good to add it. I'm just not sure how to do this cleanly while also
allowing the texture upload cleanup we need in a follow up patch.

[reveman, 2013-05-21 16:21:25]

On 2013/05/21 11:40:05, Tom Hudson wrote:
> I still don't understand why "cancel and reprioritize" means we have to have a
> general task graph. Is there a design doc that explains the use cases that
> require the extra complexity?

I've updated the RasterWorkerPool doc that you've looked at before.

[Tom Hudson, 2013-05-21 16:55:53]

On 2013/05/21 16:21:25, David Reveman wrote:
> On 2013/05/21 11:40:05, Tom Hudson wrote:
> > I still don't understand why "cancel and reprioritize" means we have to have
a
> > general task graph. Is there a design doc that explains the use cases that
> > require the extra complexity?
> 
> I've updated the RasterWorkerPool doc that you've looked at before.

Most of my comments from March or April in the introduction of that document are
still unresolved, and I think the short section of that document that addresses
my question about why we need this complex a model is even less clear than the
intro.

[reveman, 2013-05-23 22:21:32]

I think this is ready for another round of reviews. Here's what changed:

- tasks now have dependencies attached directly to them and they can't change
once the task is created. this allows the worker pool to be more efficient while
still providing what we need in terms of cancellation and re-prioritization.

- worker pool now uses a "ready-to-run" queue and task dispatching performance
is no longer affected by task graph size.

- added more perf tests.

PTAL

https://codereview.chromium.org/14689004/diff/34001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.h (right):

https://codereview.chromium.org/14689004/diff/34001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.h:16: class Task {
FYI, added this class instead of exposing the internal::WorkerPoolTask pointers
directly to the tile manager. It prevents non-thread safe usage of these objects
by the tile manager.

https://codereview.chromium.org/14689004/diff/34001/cc/resources/tile_manager.h
File cc/resources/tile_manager.h (right):

https://codereview.chromium.org/14689004/diff/34001/cc/resources/tile_manager...
cc/resources/tile_manager.h:204: PixelRefMap pending_decode_tasks_;
Doesn't make a difference in behavior but we could move the task into
skia::LazyPixelRef if we like.

[vmpstr, 2013-05-24 17:01:36]

https://codereview.chromium.org/14689004/diff/34001/cc/base/worker_pool.cc
File cc/base/worker_pool.cc (right):

https://codereview.chromium.org/14689004/diff/34001/cc/base/worker_pool.cc#ne...
cc/base/worker_pool.cc:23: #if defined(COMPILER_GCC)
Does this part just work with clang?

https://codereview.chromium.org/14689004/diff/34001/cc/base/worker_pool.cc#ne...
cc/base/worker_pool.cc:314: new_running_tasks.set(task,
new_pending_tasks.take_and_erase(task));
Are currently running tasks guaranteed to be in the new pending tasks list?

https://codereview.chromium.org/14689004/diff/34001/cc/base/worker_pool.cc#ne...
cc/base/worker_pool.cc:475: unsigned
WorkerPool::Inner::BuildScheduledTaskMapRecursive(
Can you add a few comments in this function to just explain what is going on (I
spent a couple of minutes in here with a puzzled looked on my face :P)

And also, can you put a comment somewhere to say what values |priority| can
take? I understand that it has to be unique in some cases, but not in all cases?

Maybe some comment like
//
//    root
//   /    \
// task1 task2
//   |     |
// task3   |
//     \  /
//     task4
//
// pending list becomes: task4 with priority X, ...
// After task4 finishes...

to explain what goes where with what priority and what dependents/dependencies?

[vmpstr, 2013-05-24 17:33:52]

Do you think it would be more clear if you separated this into more files?

Here are the files and the classes they define. Maybe *Task classes should be in
separate files? Or do you feel that this way is better?

worker_pool.h:

cc::internal::WorkerPoolTask
cc::WorkerPoolClient
cc::WorkerPool

worker_pool.cc:

cc::WorkerPool::Inner
cc::WorkerPool::Inner::ScheduledTask

raster_worker_pool.h:

cc::RasterWorkerPool : WorkerPool
cc::RasterWorkerPool::Task
cc::RasterWorkerPool::PictureTask
<anonymous>::RasterWorkerPoolContainerTaskImpl : internal::WorkerPoolTask
<anonymous>::RasterWorkerPoolTaskImpl : internal::WorkerPoolTask

[reveman, 2013-05-24 18:53:48]

I'd like to avoid more files at this time. I think the number of task types will
be reduced with follow up cleanup and *_worker_pool.cc code will be reduced
significantly when idle and "check for completed tasks" mechanisms are removed
from worker pool.

https://codereview.chromium.org/14689004/diff/34001/cc/base/worker_pool.cc
File cc/base/worker_pool.cc (right):

https://codereview.chromium.org/14689004/diff/34001/cc/base/worker_pool.cc#ne...
cc/base/worker_pool.cc:23: #if defined(COMPILER_GCC)
On 2013/05/24 17:01:37, vmpstr wrote:
> Does this part just work with clang?

yes, you'll find the same throughout the chromium code base. ie. layer_sorter.h,
layer_tree_host.h

https://codereview.chromium.org/14689004/diff/34001/cc/base/worker_pool.cc#ne...
cc/base/worker_pool.cc:314: new_running_tasks.set(task,
new_pending_tasks.take_and_erase(task));
On 2013/05/24 17:01:37, vmpstr wrote:
> Are currently running tasks guaranteed to be in the new pending tasks list?

No, but we need to set the ScheduledTask value to NULL in that case so this does
exactly what we want. Added a comment to make this more clear.

https://codereview.chromium.org/14689004/diff/34001/cc/base/worker_pool.cc#ne...
cc/base/worker_pool.cc:475: unsigned
WorkerPool::Inner::BuildScheduledTaskMapRecursive(
Added some comments and an example graph to latest patch. Let me know if that
makes it more clear what this function does.

[nduca, 2013-05-24 21:08:30]

this looks great. lgtm.

[vmpstr, 2013-05-24 21:34:26]

lgtm, thanks for the comments.

[commit-bot: I haz the power, 2013-05-26 16:19:40]

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

[commit-bot: I haz the power, 2013-05-27 00:10:40]

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

[commit-bot: I haz the power, 2013-05-27 03:07:35]

Change committed as 202363

[commit-bot: I haz the power, 2013-05-29 21:11:49]

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

[commit-bot: I haz the power, 2013-05-30 03:34:14]

Change committed as 203041