Re-land: cc: Add raster finished signals to RasterWorkerPool.

Re-land: cc: Add raster finished signals to RasterWorkerPool.
    
This adds to important signals to the RasterWorkerPoolClient
interface. Each signal is generated as a result of calling
ScheduleTasks() and the signals always reflect the state of the
last call to ScheduleTasks().
    
BUG=253508, 254135, 252787
TEST=cc_unittests --gtest_filter=TileManagerTest.*

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

[reveman, 2013-06-21 01:58:34]

WIP but early feedback is still welcome.

[vmpstr, 2013-06-21 18:38:33]

https://codereview.chromium.org/17351017/diff/3001/cc/resources/pixel_buffer_...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/3001/cc/resources/pixel_buffer_...
cc/resources/pixel_buffer_raster_worker_pool.cc:174:
DCHECK(std::find(tasks_required_for_activation_.begin(),
I don't think we need this dcheck. We already dcheck to ensure
new_pixel_buffer_tasks contains unique tasks above, and we clear the activation
list right above the loop.

https://codereview.chromium.org/17351017/diff/3001/cc/resources/pixel_buffer_...
cc/resources/pixel_buffer_raster_worker_pool.cc:446: unsigned
PixelBufferRasterWorkerPool::NumPendingRasterTasks() const {
nit: I prefer PendingRasterTaskCount

https://codereview.chromium.org/17351017/diff/3001/cc/resources/pixel_buffer_...
cc/resources/pixel_buffer_raster_worker_pool.cc:451: void
PixelBufferRasterWorkerPool::CheckIfFinishedRunningTasks() {
Maybe something like NotifyClientIfFinishedRunningTasks? I think that would
better reflect what the function is doing

https://codereview.chromium.org/17351017/diff/3001/cc/resources/pixel_buffer_...
cc/resources/pixel_buffer_raster_worker_pool.cc:466:
CheckIfFinishedRunningTasksRequiredForActivation() {
NotifyClient... The function name gets to be a bit too long though.

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

https://codereview.chromium.org/17351017/diff/3001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:386:
raster_required_for_activation_finished_node_->add_dependency();
Could you maybe add required_for_activation_task as a dependency of
raster_finished task? I mean this works as well, it would just seem nicer I
think. That is, any raster task would only have one dependent, and the dependent
of required_for_activation is always raster_finished.

https://codereview.chromium.org/17351017/diff/3001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:490: base::Bind(&RasterWorkerPool::
Would it be better if we just explicitly call this from OnRasterFinished? This
is kind of messy: a bind within a bind but both binds are for the same class

https://codereview.chromium.org/17351017/diff/3001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/17351017/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:185: AssignGpuMemoryToTiles();
Can we make this return true if reassign oom is required?

https://codereview.chromium.org/17351017/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:207: if
(!oom_tiles_that_need_to_be_initialized_for_activation_.empty())
Can you make a comment that explains why this is not a dcheck. That is, the fact
that we could have already scheduled new activation required tasks

https://codereview.chromium.org/17351017/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:561: for (TileVector::iterator it = tiles_.begin();
it != tiles_.end(); ++it) {
Isn't this just a loop over
oom_tiles_that_need_to_be_initialized_for_activation_?

https://codereview.chromium.org/17351017/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:574: for (TileVector::iterator it = tiles_.begin();
it != tiles_.end(); ++it) {
You should early out before this if nothing is OOM.

[reveman, 2013-06-24 16:04:03]

Latest patch seem to be working OK. I've done some testing on my N10 and
on-demand raster seem to be gone completely. PTAL.

https://codereview.chromium.org/17351017/diff/3001/cc/resources/pixel_buffer_...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/3001/cc/resources/pixel_buffer_...
cc/resources/pixel_buffer_raster_worker_pool.cc:174:
DCHECK(std::find(tasks_required_for_activation_.begin(),
On 2013/06/21 18:38:33, vmpstr wrote:
> I don't think we need this dcheck. We already dcheck to ensure
> new_pixel_buffer_tasks contains unique tasks above, and we clear the
activation
> list right above the loop.

Done.

https://codereview.chromium.org/17351017/diff/3001/cc/resources/pixel_buffer_...
cc/resources/pixel_buffer_raster_worker_pool.cc:446: unsigned
PixelBufferRasterWorkerPool::NumPendingRasterTasks() const {
On 2013/06/21 18:38:33, vmpstr wrote:
> nit: I prefer PendingRasterTaskCount

Done.

https://codereview.chromium.org/17351017/diff/3001/cc/resources/pixel_buffer_...
cc/resources/pixel_buffer_raster_worker_pool.cc:451: void
PixelBufferRasterWorkerPool::CheckIfFinishedRunningTasks() {
On 2013/06/21 18:38:33, vmpstr wrote:
> Maybe something like NotifyClientIfFinishedRunningTasks? I think that would
> better reflect what the function is doing

Done.

https://codereview.chromium.org/17351017/diff/3001/cc/resources/pixel_buffer_...
cc/resources/pixel_buffer_raster_worker_pool.cc:466:
CheckIfFinishedRunningTasksRequiredForActivation() {
On 2013/06/21 18:38:33, vmpstr wrote:
> NotifyClient... The function name gets to be a bit too long though.

Done.

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

https://codereview.chromium.org/17351017/diff/3001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:386:
raster_required_for_activation_finished_node_->add_dependency();
On 2013/06/21 18:38:33, vmpstr wrote:
> Could you maybe add required_for_activation_task as a dependency of
> raster_finished task? I mean this works as well, it would just seem nicer I
> think. That is, any raster task would only have one dependent, and the
dependent
> of required_for_activation is always raster_finished.

Done.

https://codereview.chromium.org/17351017/diff/3001/cc/resources/raster_worker...
cc/resources/raster_worker_pool.cc:490: base::Bind(&RasterWorkerPool::
On 2013/06/21 18:38:33, vmpstr wrote:
> Would it be better if we just explicitly call this from OnRasterFinished? This
> is kind of messy: a bind within a bind but both binds are for the same class

Done. I used two levels of bind here to avoid OnRasterFinished code duplication
but that's probably not worth it.

https://codereview.chromium.org/17351017/diff/3001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/17351017/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:185: AssignGpuMemoryToTiles();
On 2013/06/21 18:38:33, vmpstr wrote:
> Can we make this return true if reassign oom is required?

I'd rather call reassign unconditionally to keep things as simple as possible
for now. This is a bit different in latest patch. PTAL.

https://codereview.chromium.org/17351017/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:207: if
(!oom_tiles_that_need_to_be_initialized_for_activation_.empty())
On 2013/06/21 18:38:33, vmpstr wrote:
> Can you make a comment that explains why this is not a dcheck. That is, the
fact
> that we could have already scheduled new activation required tasks

Added a comment that explains why we might need to early out here.

https://codereview.chromium.org/17351017/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:561: for (TileVector::iterator it = tiles_.begin();
it != tiles_.end(); ++it) {
On 2013/06/21 18:38:33, vmpstr wrote:
> Isn't this just a loop over
> oom_tiles_that_need_to_be_initialized_for_activation_?

Yes, but it adds tiles to |tiles_requiring_memory_but_oomed| in order of
priority. We could instead make
|oom_tiles_that_need_to_be_initialized_for_activation_| a vector ordered by
priority but that would instead make erase less efficient. I'm not too worried
about performance here as we're already in a relatively bad spot when trying to
re-assign memory from active tree.

https://codereview.chromium.org/17351017/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:574: for (TileVector::iterator it = tiles_.begin();
it != tiles_.end(); ++it) {
On 2013/06/21 18:38:33, vmpstr wrote:
> You should early out before this if nothing is OOM.

Done.

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:527: // then it means that we have assigned memory
for it.
I think there's a bug here. A tile will still be in the to-rasterize list after
completing and raster_task will then be null. We might want to land a fix for
this separately.

[vmpstr, 2013-06-24 16:34:02]

https://codereview.chromium.org/17351017/diff/3001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/17351017/diff/3001/cc/resources/tile_manager....
cc/resources/tile_manager.cc:561: for (TileVector::iterator it = tiles_.begin();
it != tiles_.end(); ++it) {
On 2013/06/24 16:04:03, David Reveman wrote:
> On 2013/06/21 18:38:33, vmpstr wrote:
> > Isn't this just a loop over
> > oom_tiles_that_need_to_be_initialized_for_activation_?
> 
> Yes, but it adds tiles to |tiles_requiring_memory_but_oomed| in order of
> priority. We could instead make
> |oom_tiles_that_need_to_be_initialized_for_activation_| a vector ordered by
> priority but that would instead make erase less efficient. I'm not too worried
> about performance here as we're already in a relatively bad spot when trying
to
> re-assign memory from active tree.

Oh I didn't realize that it was a set, my bad.

https://codereview.chromium.org/17351017/diff/10001/cc/resources/pixel_buffer...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/10001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:231:
OnRasterTasksRequiredForActivationFinished() {
nit: I _think_ this might just fit on one line?

https://codereview.chromium.org/17351017/diff/10001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/10001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:483:
new_raster_required_for_activation_finished_task(
Should this one be marked as a dependency of the raster_finished_node?

https://codereview.chromium.org/17351017/diff/10001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:492:
raster_required_for_activation_finished_node->set_priority(1u);
As a follow-up, we might want to do something like set_task_with_priority, since
any time we call set_task, we immediately call set_priority.

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:527: // then it means that we have assigned memory
for it.
On 2013/06/24 16:04:04, David Reveman wrote:
> I think there's a bug here. A tile will still be in the to-rasterize list
after
> completing and raster_task will then be null. We might want to land a fix for
> this separately.

I don't think that's possible with the current code. to-rasterize list is
cleared at the top of the function, and the only items put into this are tasks
that actually need a raster (ie not completed). Unless you mean that it was
completed but we were not notified of that yet? I'm not sure what we can do
about that, short of forcing completion check

I think with the refactor that you have done here, it might be more possible? :P
I'm not sure.

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:672: TRACE_EVENT0("cc",
"TileManager::CreateImageDecodeTask");
Can we replace these with INSTANT traces that just log the number of image
decodes and raster tasks that were used?

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:613: if (mode == mts.raster_mode) {
It's a bit awkward to have this in the inner loop. Is it possible to move this
out? Maybe have something like "bool current_raster_mode_had_resource" temporary
variable or something? This is not a strong opinion though, so I'm fine if you
like it this way.

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:651: AddRequiredTileForActivation(tile);
DCHECK that it's required for activation, please.

[reveman, 2013-06-24 19:05:17]

https://codereview.chromium.org/17351017/diff/10001/cc/resources/pixel_buffer...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/10001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:231:
OnRasterTasksRequiredForActivationFinished() {
On 2013/06/24 16:34:03, vmpstr wrote:
> nit: I _think_ this might just fit on one line?

Done.

https://codereview.chromium.org/17351017/diff/10001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/10001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:483:
new_raster_required_for_activation_finished_task(
On 2013/06/24 16:34:03, vmpstr wrote:
> Should this one be marked as a dependency of the raster_finished_node?

That's currently done in RasterTaskGraph ctor.

https://codereview.chromium.org/17351017/diff/10001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:492:
raster_required_for_activation_finished_node->set_priority(1u);
On 2013/06/24 16:34:03, vmpstr wrote:
> As a follow-up, we might want to do something like set_task_with_priority,
since
> any time we call set_task, we immediately call set_priority.

Sounds good. This will likely be done as part of the switch to using a vector
which I think will require similar changes.

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:517: for (TileVector::iterator it = tiles_.begin();
it != tiles_.end(); ++it) {
I changed this to a reverse iterator so we try to steal memory from low priority
tiles in the active tree before higher priority tiles.

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:527: // then it means that we have assigned memory
for it.
On 2013/06/24 16:34:03, vmpstr wrote:
> On 2013/06/24 16:04:04, David Reveman wrote:
> > I think there's a bug here. A tile will still be in the to-rasterize list
> after
> > completing and raster_task will then be null. We might want to land a fix
for
> > this separately.
> 
> I don't think that's possible with the current code. to-rasterize list is
> cleared at the top of the function, and the only items put into this are tasks
> that actually need a raster (ie not completed). Unless you mean that it was
> completed but we were not notified of that yet? I'm not sure what we can do
> about that, short of forcing completion check
> 
> I think with the refactor that you have done here, it might be more possible?
:P
> I'm not sure.

You're right. It's not possible as tiles_that_need_to_be_rasterized_ is a
temporary variable in practice. We should make that a real temporary variable to
avoid confusion.

I changed back to your original code but simplified it a bit. computing
bytes_that_can_be_freed before actually freeing things doesn't seem to serve a
purpose. Latest patch just frees and adjusts bytes_freed directly. PTAL.

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:672: TRACE_EVENT0("cc",
"TileManager::CreateImageDecodeTask");
On 2013/06/24 16:34:03, vmpstr wrote:
> Can we replace these with INSTANT traces that just log the number of image
> decodes and raster tasks that were used?

I added a count argument to the ScheduleTasks trace for this purpose. A trace
event for each task allocation seems a bit excessive but I can add that if you
insist?

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (right):

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:651: AddRequiredTileForActivation(tile);
On 2013/06/24 16:34:03, vmpstr wrote:
> DCHECK that it's required for activation, please.

Done.

[vmpstr, 2013-06-24 19:16:46]

lgtm with a nit.

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager.cc
File cc/resources/tile_manager.cc (left):

https://codereview.chromium.org/17351017/diff/10001/cc/resources/tile_manager...
cc/resources/tile_manager.cc:672: TRACE_EVENT0("cc",
"TileManager::CreateImageDecodeTask");
On 2013/06/24 19:05:17, David Reveman wrote:
> On 2013/06/24 16:34:03, vmpstr wrote:
> > Can we replace these with INSTANT traces that just log the number of image
> > decodes and raster tasks that were used?
> 
> I added a count argument to the ScheduleTasks trace for this purpose. A trace
> event for each task allocation seems a bit excessive but I can add that if you
> insist?

No, I just meant to have the count somewhere. I'm happy with what you have now.

https://codereview.chromium.org/17351017/diff/17001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/17001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:338: raster_finished_node_->add_dependency();
Ah, here it is :) I missed it last time.

https://codereview.chromium.org/17351017/diff/17001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool_unittest.cc (right):

https://codereview.chromium.org/17351017/diff/17001/cc/resources/raster_worke...
cc/resources/raster_worker_pool_unittest.cc:81: virtual void
DidFinishedRunningTasks() OVERRIDE { }
nit: No space between {}, or a space between them on the next line.

[reveman, 2013-06-24 21:08:33]

https://codereview.chromium.org/17351017/diff/17001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool_unittest.cc (right):

https://codereview.chromium.org/17351017/diff/17001/cc/resources/raster_worke...
cc/resources/raster_worker_pool_unittest.cc:81: virtual void
DidFinishedRunningTasks() OVERRIDE { }
On 2013/06/24 19:16:46, vmpstr wrote:
> nit: No space between {}, or a space between them on the next line.

Done.

[commit-bot: I haz the power, 2013-06-24 21:09:21]

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

[commit-bot: I haz the power, 2013-06-25 11:07:44]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[reveman, 2013-06-26 13:45:07]

Latest patch fixes some task scheduling issues in PixeBufferRWP that could cause
it to busy loop when upload bound.

This latest patch also hooks up the "ready to activate" signal to the LTHI and
we're now activating asap. I kept the CheckForTileUploads/AttemptTreeActivation
system in place as I'm not sure yet if it's a good idea to remove that.

PTAL.

[vmpstr, 2013-06-26 18:05:07]

I suspect that I'm missing something... See comments:

https://codereview.chromium.org/17351017/diff/57001/cc/resources/pixel_buffer...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/57001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:233: if
(num_scheduled_raster_tasks_ != PendingRasterTaskCount())
I don't understand this check. Shouldn't this be if (PendingRasterTaskCount() !=
0)?

PendingRasterTasks returns the total number of tasks pending raster in the
worker pool. num_scheduled_raster_tasks is the number we last scheduled. They
should only be the same if none of the tasks completed yet, no?

In either case, can you expand the comment a bit to explain the situation?

Also, OnRasterTasksFinished should only be called when the batch of raster tasks
is done. Why isn't that the case?

https://codereview.chromium.org/17351017/diff/57001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:247: if
(raster_tasks_required_for_activation_.size() !=
I guess I'm missing something, since I have the same confusion about this check.
With no other scheduletasks being called, we remove things from
raster_tasks_required_for_activation_, but we never decrease num_..., so we
early out all but the first time?

[reveman, 2013-06-27 09:06:57]

I think the mistake we're doing is trying too hard to reuse code between
ImageRasterWorkerPool and PixelBufferRasterWorkerPool. I'll try to refactor this
with less of a concern for code duplication between RWP implementations to see
if that is not a lot cleaner. ie. only schedule "raster finished" tasks in pixel
buffer RWP when needed.

https://codereview.chromium.org/17351017/diff/57001/cc/resources/pixel_buffer...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/57001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:233: if
(num_scheduled_raster_tasks_ != PendingRasterTaskCount())
On 2013/06/26 18:05:07, vmpstr wrote:
> I don't understand this check. Shouldn't this be if (PendingRasterTaskCount()
!=
> 0)?
> 
> PendingRasterTasks returns the total number of tasks pending raster in the
> worker pool. num_scheduled_raster_tasks is the number we last scheduled. They
> should only be the same if none of the tasks completed yet, no?

They will be the same if we're not throttled in any way. When throttled, only a
subset of our pending raster tasks have been scheduled.

> 
> In either case, can you expand the comment a bit to explain the situation?
> 
> Also, OnRasterTasksFinished should only be called when the batch of raster
tasks
> is done. Why isn't that the case?

It is as far as the WorkerPool and base RasterWorkerPool class knows. The
concept of throttling is applied at the PixelBufferRasterWorkerPool level.

https://codereview.chromium.org/17351017/diff/57001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:247: if
(raster_tasks_required_for_activation_.size() !=
On 2013/06/26 18:05:07, vmpstr wrote:
> I guess I'm missing something, since I have the same confusion about this
check.
> With no other scheduletasks being called, we remove things from
> raster_tasks_required_for_activation_, but we never decrease num_..., so we
> early out all but the first time?

Not sure what you mean by "first time". These functions are only called once for
each SetRasterTaskGraph call. And the reason we early out is that we only want
to handle these calls when they truly mean "raster finished" or "raster required
for activation finished".

It's not a disaster if we have an extra CheckForCompletedRasterTasks call but if
we don't take throttling into account we easily end up in a
SetRasterTaskGraph/CheckForCompletedRasterTask busy loop or keep calling
CheckForCompletedRasterTask after each task has finished.

[vmpstr, 2013-06-27 16:32:12]

I agree with your comment on trying to share too much. Conceptually, it's a nice
abstraction, but I think we should just make it clear first (with regards to
what throttles what), and then see if we can refactor common code.

https://chromiumcodereview.appspot.com/17351017/diff/57001/cc/resources/pixel...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://chromiumcodereview.appspot.com/17351017/diff/57001/cc/resources/pixel...
cc/resources/pixel_buffer_raster_worker_pool.cc:247: if
(raster_tasks_required_for_activation_.size() !=
On 2013/06/27 09:06:58, David Reveman wrote:
> On 2013/06/26 18:05:07, vmpstr wrote:
> > I guess I'm missing something, since I have the same confusion about this
> check.
> > With no other scheduletasks being called, we remove things from
> > raster_tasks_required_for_activation_, but we never decrease num_..., so we
> > early out all but the first time?
> 
> Not sure what you mean by "first time". These functions are only called once
for
> each SetRasterTaskGraph call. And the reason we early out is that we only want
> to handle these calls when they truly mean "raster finished" or "raster
required
> for activation finished".
> 
> It's not a disaster if we have an extra CheckForCompletedRasterTasks call but
if
> we don't take throttling into account we easily end up in a
> SetRasterTaskGraph/CheckForCompletedRasterTask busy loop or keep calling
> CheckForCompletedRasterTask after each task has finished.

What I mean is that, in the case where we schedule everything (we're not bound
by anything), it would be the case that
|raster_tasks_required_for_activation_.size()| ==
|num_scheduled_raster_tasks_required_for_activation_|. But, when a single task
finishes, we remove it from  raster_tasks_required_for_activation_ (via
OnRasterTaskCompleted). In that case we don't update num_scheduled... (or at
least I don't see where). From then on, all calls to this function will early
out, no?

[reveman, 2013-06-27 22:33:18]

Latest patch takes a new approach. Let me know what you think and maybe we can
iterate on this quickly to reduce some of the code duplication.

https://chromiumcodereview.appspot.com/17351017/diff/57001/cc/resources/pixel...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://chromiumcodereview.appspot.com/17351017/diff/57001/cc/resources/pixel...
cc/resources/pixel_buffer_raster_worker_pool.cc:247: if
(raster_tasks_required_for_activation_.size() !=
On 2013/06/27 16:32:12, vmpstr wrote:
> On 2013/06/27 09:06:58, David Reveman wrote:
> > On 2013/06/26 18:05:07, vmpstr wrote:
> > > I guess I'm missing something, since I have the same confusion about this
> > check.
> > > With no other scheduletasks being called, we remove things from
> > > raster_tasks_required_for_activation_, but we never decrease num_..., so
we
> > > early out all but the first time?
> > 
> > Not sure what you mean by "first time". These functions are only called once
> for
> > each SetRasterTaskGraph call. And the reason we early out is that we only
want
> > to handle these calls when they truly mean "raster finished" or "raster
> required
> > for activation finished".
> > 
> > It's not a disaster if we have an extra CheckForCompletedRasterTasks call
but
> if
> > we don't take throttling into account we easily end up in a
> > SetRasterTaskGraph/CheckForCompletedRasterTask busy loop or keep calling
> > CheckForCompletedRasterTask after each task has finished.
> 
> What I mean is that, in the case where we schedule everything (we're not bound
> by anything), it would be the case that
> |raster_tasks_required_for_activation_.size()| ==
> |num_scheduled_raster_tasks_required_for_activation_|. But, when a single task
> finishes, we remove it from  raster_tasks_required_for_activation_ (via
> OnRasterTaskCompleted). In that case we don't update num_scheduled... (or at
> least I don't see where). From then on, all calls to this function will early
> out, no?

hm, OnRasterTaskCompleted is usually not called before OnRasterFinished* unless
the tile manager calls RasterWorkerPool::CheckForCompletedTasks(), in which case
you're right that we might delay this notification up to 6ms.

[vmpstr, 2013-06-27 23:31:20]

I'm not sure that I like this approach over the previous one (I find it more
difficult to follow the logic). I know we want to save some refactoring for
follow-ups, but I think there are some things that should be done in this patch
as well.

https://codereview.chromium.org/17351017/diff/69001/cc/resources/image_raster...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/69001/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:160:
scoped_refptr<internal::WorkerPoolTask> new_raster_finished_task(
Can you either add some comments in the code from here to the end of the
function, or possibly move this to a different function since it kind of seems
like wrap-up code (we scheduled everything.. now we just need to tie everything
together into a single graph).

https://codereview.chromium.org/17351017/diff/69001/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:169: for (unsigned i = 0; i <
NUM_TYPES; ++i) {
nit: type (to be consistent with the loop above)

https://codereview.chromium.org/17351017/diff/69001/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:170: for (unsigned j = 0; j <
tasks[i].size(); ++j) {
This can be a small anonymous namespace function like AddDependency(tasks[i],
raster_finished_node). You can reuse that in the loop below for
AddDependency(tasks[REQUIRED_FOR_ACTIVATION_TYPE],
raster_required_for_activation_node).

What I'm really trying to say is that there's quite a bit of replication of code
in this block. I would prefer that we try to reduce that a bit in this patch,
but we can also save some of the work for a follow-up.

https://codereview.chromium.org/17351017/diff/69001/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:198: for (unsigned i = 0; i <
NUM_TYPES; ++i) {
I wonder if it's better to have tasks[i][j] as a raw pointer and insert into the
graph as we go, and just save tasks for easy updates; this loop looks like it's
begging to be removed, but I might be missing some corner case

https://codereview.chromium.org/17351017/diff/69001/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:218: // Call DidFinishedRunningTasks()
if this callback is
nit: blank line above this, please

https://codereview.chromium.org/17351017/diff/69001/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:230: // Call
DidFinishedRunningTasksRequiredForActivation() if this callback is
nit: blank line above this, please

https://codereview.chromium.org/17351017/diff/69001/cc/resources/pixel_buffer...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/69001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:362:
NotifyClientThatFinishedRunningTasksRequiredForActivation();
ShouldNotify... I think we have to rethink the names here, they are getting a
bit too long.

https://codereview.chromium.org/17351017/diff/69001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:480:
DCHECK_LE(tasks[PREPAINT_TYPE].size() +
The comments from irwp apply in in this area as well.

https://codereview.chromium.org/17351017/diff/69001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/69001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:410: scoped_ptr<WorkerPool::GraphNode>
RasterWorkerPool::CreateRasterTaskGraphNode(
Can this be non-static on TaskGraph?

Something like
scoped_ptr<WorkerPool::GraphNode> TaskGraph::InsertRasterTask(...) {
  ...
  return raster_node.Pass();
}

https://codereview.chromium.org/17351017/diff/69001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool_perftest.cc (right):

https://codereview.chromium.org/17351017/diff/69001/cc/resources/raster_worke...
cc/resources/raster_worker_pool_perftest.cc:60: enum RasterTaskType {
This enum is in quite a few places, I think it's worth it to just pull it out to
RasterWorkerPool level. I know that this is just a test, but it also is defined
a couple of times in non-test code

[reveman, 2013-06-28 17:26:07]

Please take a look at the latest patch. It's hybrid of the approach taken in
previous patch and what I was doing before that. Gives us the flexibility we
need to do things correctly in the pixel buffer rwp class without a bunch of
code duplication.

https://codereview.chromium.org/17351017/diff/69001/cc/resources/image_raster...
File cc/resources/image_raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/69001/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:169: for (unsigned i = 0; i <
NUM_TYPES; ++i) {
On 2013/06/27 23:31:21, vmpstr wrote:
> nit: type (to be consistent with the loop above)

Done.

https://codereview.chromium.org/17351017/diff/69001/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:170: for (unsigned j = 0; j <
tasks[i].size(); ++j) {
On 2013/06/27 23:31:21, vmpstr wrote:
> This can be a small anonymous namespace function like AddDependency(tasks[i],
> raster_finished_node). You can reuse that in the loop below for
> AddDependency(tasks[REQUIRED_FOR_ACTIVATION_TYPE],
> raster_required_for_activation_node).
> 
> What I'm really trying to say is that there's quite a bit of replication of
code
> in this block. I would prefer that we try to reduce that a bit in this patch,
> but we can also save some of the work for a follow-up.

Latest patch reduces code duplication significantly. PTAL.

https://codereview.chromium.org/17351017/diff/69001/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:198: for (unsigned i = 0; i <
NUM_TYPES; ++i) {
On 2013/06/27 23:31:21, vmpstr wrote:
> I wonder if it's better to have tasks[i][j] as a raw pointer and insert into
the
> graph as we go, and just save tasks for easy updates; this loop looks like
it's
> begging to be removed, but I might be missing some corner case

Done.

https://codereview.chromium.org/17351017/diff/69001/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:218: // Call DidFinishedRunningTasks()
if this callback is
On 2013/06/27 23:31:21, vmpstr wrote:
> nit: blank line above this, please

code doesn't exist in latest patch.

https://codereview.chromium.org/17351017/diff/69001/cc/resources/image_raster...
cc/resources/image_raster_worker_pool.cc:230: // Call
DidFinishedRunningTasksRequiredForActivation() if this callback is
On 2013/06/27 23:31:21, vmpstr wrote:
> nit: blank line above this, please

code doesn't exist in latest patch.

https://codereview.chromium.org/17351017/diff/69001/cc/resources/pixel_buffer...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/69001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:362:
NotifyClientThatFinishedRunningTasksRequiredForActivation();
On 2013/06/27 23:31:21, vmpstr wrote:
> ShouldNotify... I think we have to rethink the names here, they are getting a
> bit too long.

I agree. I'll come up with a better name asap. Let me know if you have
suggestions.

https://codereview.chromium.org/17351017/diff/69001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:480:
DCHECK_LE(tasks[PREPAINT_TYPE].size() +
On 2013/06/27 23:31:21, vmpstr wrote:
> The comments from irwp apply in in this area as well.

PTAL.

https://codereview.chromium.org/17351017/diff/69001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/69001/cc/resources/raster_worke...
cc/resources/raster_worker_pool.cc:410: scoped_ptr<WorkerPool::GraphNode>
RasterWorkerPool::CreateRasterTaskGraphNode(
On 2013/06/27 23:31:21, vmpstr wrote:
> Can this be non-static on TaskGraph?
> 
> Something like
> scoped_ptr<WorkerPool::GraphNode> TaskGraph::InsertRasterTask(...) {
>   ...
>   return raster_node.Pass();
> }

TaskGraph is just a ScopedPtrHashMap at the moment which I don't think we should
subclass. We'll probably add a TaskGraph class in the future when switching to
an array for storage but I prefer to keep this a static function until then.

https://codereview.chromium.org/17351017/diff/69001/cc/resources/raster_worke...
File cc/resources/raster_worker_pool_perftest.cc (right):

https://codereview.chromium.org/17351017/diff/69001/cc/resources/raster_worke...
cc/resources/raster_worker_pool_perftest.cc:60: enum RasterTaskType {
On 2013/06/27 23:31:21, vmpstr wrote:
> This enum is in quite a few places, I think it's worth it to just pull it out
to
> RasterWorkerPool level. I know that this is just a test, but it also is
defined
> a couple of times in non-test code

Only used by pixel buffer RWP in latest patch.

[vmpstr, 2013-06-28 18:12:34]

lgtm with some nits. Can you file a bug for a follow-up to clean this up /
refactor?

https://codereview.chromium.org/17351017/diff/81001/cc/resources/pixel_buffer...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/81001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:347:
NotifyClientThatFinishedRunningTasksRequiredForActivation();
Maybe something like AreTaskRequiredForActivationFinished? Since you moved the
actual client notification out of there, the name doesn't have to include
"NotifyClient" in it.

https://codereview.chromium.org/17351017/diff/81001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:467: // Schedule
OnRasterTasksRequiredForActivationFinished call when
nit: Schedule OnRaster.. call only when

https://codereview.chromium.org/17351017/diff/81001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:480: for (unsigned j = 0;
I still think this loop, and the inner loop around line 507 can be a small
separate function. But we can leave that for a clean up followup

https://codereview.chromium.org/17351017/diff/81001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:494: // Schedule
OnRasterTasksFinished call when notification is pending
nit: ... only when ...

[reveman, 2013-06-28 20:18:41]

https://codereview.chromium.org/17351017/diff/81001/cc/resources/pixel_buffer...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/17351017/diff/81001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:347:
NotifyClientThatFinishedRunningTasksRequiredForActivation();
On 2013/06/28 18:12:35, vmpstr wrote:
> Maybe something like AreTaskRequiredForActivationFinished? Since you moved the
> actual client notification out of there, the name doesn't have to include
> "NotifyClient" in it.

Cleaned this up a bit in latest patch. Let me know what you think.

https://codereview.chromium.org/17351017/diff/81001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:467: // Schedule
OnRasterTasksRequiredForActivationFinished call when
On 2013/06/28 18:12:35, vmpstr wrote:
> nit: Schedule OnRaster.. call only when

Done.

https://codereview.chromium.org/17351017/diff/81001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:480: for (unsigned j = 0;
On 2013/06/28 18:12:35, vmpstr wrote:
> I still think this loop, and the inner loop around line 507 can be a small
> separate function. But we can leave that for a clean up followup

Move inner loop to a separate function.

https://codereview.chromium.org/17351017/diff/81001/cc/resources/pixel_buffer...
cc/resources/pixel_buffer_raster_worker_pool.cc:494: // Schedule
OnRasterTasksFinished call when notification is pending
On 2013/06/28 18:12:35, vmpstr wrote:
> nit: ... only when ...

Done.

[vmpstr, 2013-06-28 20:21:46]

On 2013/06/28 20:18:41, David Reveman wrote:
>
https://codereview.chromium.org/17351017/diff/81001/cc/resources/pixel_buffer...
> File cc/resources/pixel_buffer_raster_worker_pool.cc (right):
> 
>
https://codereview.chromium.org/17351017/diff/81001/cc/resources/pixel_buffer...
> cc/resources/pixel_buffer_raster_worker_pool.cc:347:
> NotifyClientThatFinishedRunningTasksRequiredForActivation();
> On 2013/06/28 18:12:35, vmpstr wrote:
> > Maybe something like AreTaskRequiredForActivationFinished? Since you moved
the
> > actual client notification out of there, the name doesn't have to include
> > "NotifyClient" in it.
> 
> Cleaned this up a bit in latest patch. Let me know what you think.
> 

Looks good. Thanks

[commit-bot: I haz the power, 2013-06-28 20:26:08]

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

[commit-bot: I haz the power, 2013-06-29 17:23:30]

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

[commit-bot: I haz the power, 2013-06-30 02:56:52]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[commit-bot: I haz the power, 2013-06-30 04:23:11]

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

[commit-bot: I haz the power, 2013-06-30 14:48:32]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[boliu, 2013-07-01 20:40:23]

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

https://codereview.chromium.org/17351017/diff/144001/cc/resources/tile_manage...
cc/resources/tile_manager.h:92:
oom_tiles_that_need_to_be_initialized_for_activation_.empty();
The android_webivew tests run in software mode with a gpu memory budget of 0. I
think this is causing oom_tiles_that_need_to_be_initialized_for_activation_ to
never be empty. I think this is blocking activations.

Also webview sets the root layer damage to the viewport before each begin frame
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/an...)

I'm not sure how these two interacts, but some tests are getting into a live
lock situation that begin frame continuously called expecting to activate tree,
but is blocked due to oom_tiles_that_need_to_be_initialized_for_activation_ not
being empty.

[commit-bot: I haz the power, 2013-07-03 04:03:21]

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

[commit-bot: I haz the power, 2013-07-03 08:16:22]

Change committed as 209912

[commit-bot: I haz the power, 2013-07-03 14:45:31]

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

[commit-bot: I haz the power, 2013-07-03 17:09:56]

Change committed as 209995