cc: Simplify raster task completion notification logic

cc: Simplify raster task completion notification logic

(Relanding after missing activation bug fixed in https://codereview.chromium.org/131763003/)

Previously the pixel buffer raster worker pool used a combination of
polling and explicit notifications from the raster worker pool to decide
when to tell the client about the completion of 1) all tasks or 2) the
subset of tasks required for activation. This patch simplifies the logic
by only triggering the notification based on the OnRasterTasksFinished
and OnRasterTasksRequiredForActivationFinished calls from the worker
pool.

BUG=307841, 331534
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243991

[Sami, 2013-12-13 21:17:11]

PTAL. As a follow-up I was thinking we could completely eliminate the raster
thread round trip if the new graph has zero tasks or no tasks needed for
activation.

[reveman, 2013-12-14 04:31:01]

https://codereview.chromium.org/99873007/diff/1/cc/resources/pixel_buffer_ras...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/99873007/diff/1/cc/resources/pixel_buffer_ras...
cc/resources/pixel_buffer_raster_worker_pool.cc:258:
client()->DidFinishRunningTasks();
Hm, what happens if CheckForCompletedRasterTasks() returns TASKS_PENDING because
some uploads are pending? Seems like this client callback is never going to
happen in that case or am I missing something?

What happens in ToT right now is that a periodic check can sometimes result in
us detecting completion and making the client callback before the signal has
been received, right?  I think we can keep the existing code but just make sure
will_notify_client_that_no_tasks_are_pending is not true unless we've received
the ::OnRasterTasksFinished() signal. Wdyt?

[Sami, 2013-12-17 16:27:38]

https://codereview.chromium.org/99873007/diff/1/cc/resources/pixel_buffer_ras...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/99873007/diff/1/cc/resources/pixel_buffer_ras...
cc/resources/pixel_buffer_raster_worker_pool.cc:258:
client()->DidFinishRunningTasks();
On 2013/12/14 04:31:01, David Reveman wrote:
> Hm, what happens if CheckForCompletedRasterTasks() returns TASKS_PENDING
because
> some uploads are pending? Seems like this client callback is never going to
> happen in that case or am I missing something?

This shouldn't be any different than before, i.e., DidFinishRunningTasks() will
never get signaled while HasPendingTasks() is true. AFAICT pending uploads don't
factor into that, since they don't get their own worker pool tasks.

> What happens in ToT right now is that a periodic check can sometimes result in
> us detecting completion and making the client callback before the signal has
> been received, right?  I think we can keep the existing code but just make
sure
> will_notify_client_that_no_tasks_are_pending is not true unless we've received
> the ::OnRasterTasksFinished() signal. Wdyt?

Yes, that's exactly the problem I was trying to fix, but perhaps tried to change
things around a little too much. I've now gone for the simpler approach you
suggested.

[reveman, 2013-12-17 20:46:24]

lgtm, thanks for making this a bit more consistent.

https://codereview.chromium.org/99873007/diff/1/cc/resources/pixel_buffer_ras...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/99873007/diff/1/cc/resources/pixel_buffer_ras...
cc/resources/pixel_buffer_raster_worker_pool.cc:258:
client()->DidFinishRunningTasks();
On 2013/12/17 16:27:38, Sami wrote:
> On 2013/12/14 04:31:01, David Reveman wrote:
> > Hm, what happens if CheckForCompletedRasterTasks() returns TASKS_PENDING
> because
> > some uploads are pending? Seems like this client callback is never going to
> > happen in that case or am I missing something?
> 
> This shouldn't be any different than before, i.e., DidFinishRunningTasks()
will
> never get signaled while HasPendingTasks() is true. AFAICT pending uploads
don't
> factor into that, since they don't get their own worker pool tasks.

The problem was that you removed the client callback in
CheckForCompletedRasterTasks() which will happend when the uploads complete. Not
a problem in latest patch though.

> 
> > What happens in ToT right now is that a periodic check can sometimes result
in
> > us detecting completion and making the client callback before the signal has
> > been received, right?  I think we can keep the existing code but just make
> sure
> > will_notify_client_that_no_tasks_are_pending is not true unless we've
received
> > the ::OnRasterTasksFinished() signal. Wdyt?
> 
> Yes, that's exactly the problem I was trying to fix, but perhaps tried to
change
> things around a little too much. I've now gone for the simpler approach you
> suggested.

[Sami, 2013-12-18 12:05:05]

https://codereview.chromium.org/99873007/diff/1/cc/resources/pixel_buffer_ras...
File cc/resources/pixel_buffer_raster_worker_pool.cc (right):

https://codereview.chromium.org/99873007/diff/1/cc/resources/pixel_buffer_ras...
cc/resources/pixel_buffer_raster_worker_pool.cc:258:
client()->DidFinishRunningTasks();
On 2013/12/17 20:46:25, David Reveman wrote:
> The problem was that you removed the client callback in
> CheckForCompletedRasterTasks() which will happend when the uploads complete.
Not
> a problem in latest patch though.

Ah, now I see. I didn't realize pending uploads were also keeping the
CheckForCompletedRasterTasks() tick going.

[commit-bot: I haz the power, 2013-12-18 12:05:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/99873007/40001

[commit-bot: I haz the power, 2013-12-18 14:45:00]

Change committed as 241569

[Sami, 2014-01-09 13:15:21]

Here's the original patch with a fix and a test for the freezing bug
(crbug.com/331534). Here's what was happening:

1. Schedule a set T1 of raster tasks.
2. Later while some of the tasks were still pending, we schedule another set of
tasks T2. (In this case we assume there's no overlap between the sets.)
3. Because some of the tasks in T1 were still pending, the new active set
(PixelBufferRasterWorkerPool::pixel_buffer_tasks_) now contains tasks from both
sets.
4. Because of the newly scheduled tasks only come from T2, the resulting raster
task graph will always be smaller than pixel_buffer_tasks_.
5. This causes the logic at [1] to falsely think some of the tasks have been
throttled, so the RasterFinishedTask does not get scheduled.
6. Next time we check for finished raster tasks, the tasks from T1 are reported
as cancelled and the tasks from T2 eventually finish.
7. client()->DidFinishRunningTasks never ends up getting called because we never
scheduled RasterFinishedTask for T2. However,
client()->DidFinishRunningTasksRequiredForActivation is called, but if we are
low on memory[2] it becomes a no-op, causing the hang.

The fix is to make sure [1] only considers actually throttled tasks from the new
set.

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/pixel...
[2]
https://code.google.com/p/chromium/codesearch#chromium/src/cc/resources/tile_...

[Sami, 2014-01-09 18:00:48]

Split off the bugfix to https://codereview.chromium.org/131763003/

[reveman, 2014-01-09 18:02:47]

lgtm

[commit-bot: I haz the power, 2014-01-09 20:32:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/99873007/150001

[commit-bot: I haz the power, 2014-01-09 22:11:08]

Change committed as 243991