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

cc/resources/pixel_buffer_raster_worker_pool.cc

Index: cc/resources/pixel_buffer_raster_worker_pool.cc
diff --git a/cc/resources/pixel_buffer_raster_worker_pool.cc b/cc/resources/pixel_buffer_raster_worker_pool.cc
index 488b5ac912a1600a6e29eb9a90c66eae82e3e536..cea6d062ffb4609cc42dd9285b22297e6e2ae67c 100644
--- a/cc/resources/pixel_buffer_raster_worker_pool.cc
+++ b/cc/resources/pixel_buffer_raster_worker_pool.cc
@@ -84,11 +84,16 @@ const size_t kMaxPendingRasterBytes =
 
 PixelBufferRasterWorkerPool::PixelBufferRasterWorkerPool(
     ResourceProvider* resource_provider,
-    size_t num_threads) : RasterWorkerPool(resource_provider, num_threads),
-                          shutdown_(false),
-                          bytes_pending_upload_(0),
-                          has_performed_uploads_since_last_flush_(false),
-                          check_for_completed_raster_tasks_pending_(false) {
+    size_t num_threads)
+    : RasterWorkerPool(resource_provider, num_threads),
+      shutdown_(false),
+      bytes_pending_upload_(0),
+      has_performed_uploads_since_last_flush_(false),
+      check_for_completed_raster_tasks_pending_(false),
+      num_scheduled_raster_tasks_(0),
+      num_scheduled_raster_tasks_required_for_activation_(0),
+      finished_running_tasks_pending_(false),
+      finished_running_tasks_required_for_activation_pending_(false) {
 }
 
 PixelBufferRasterWorkerPool::~PixelBufferRasterWorkerPool() {
@@ -102,7 +107,10 @@ PixelBufferRasterWorkerPool::~PixelBufferRasterWorkerPool() {
 void PixelBufferRasterWorkerPool::Shutdown() {
   shutdown_ = true;
   RasterWorkerPool::Shutdown();
-  CheckForCompletedRasterTasks();
+  RasterWorkerPool::CheckForCompletedTasks();
+  CheckForCompletedUploads();
+  check_for_completed_raster_tasks_callback_.Cancel();
+  check_for_completed_raster_tasks_pending_ = false;
   for (TaskMap::iterator it = pixel_buffer_tasks_.begin();
        it != pixel_buffer_tasks_.end(); ++it) {
     internal::RasterWorkerPoolTask* task = it->first;
@@ -114,6 +122,7 @@ void PixelBufferRasterWorkerPool::Shutdown() {
       completed_tasks_.push_back(task);
     }
   }
+  DCHECK_EQ(completed_tasks_.size(), pixel_buffer_tasks_.size());
 }
 
 void PixelBufferRasterWorkerPool::ScheduleTasks(RasterTask::Queue* queue) {
@@ -121,6 +130,9 @@ void PixelBufferRasterWorkerPool::ScheduleTasks(RasterTask::Queue* queue) {
 
   RasterWorkerPool::SetRasterTasks(queue);
 
+  finished_running_tasks_pending_ = true;
+  finished_running_tasks_required_for_activation_pending_ = true;
+
   // Build new pixel buffer task set.
   TaskMap new_pixel_buffer_tasks;
   for (RasterTaskVector::const_iterator it = raster_tasks().begin();
@@ -160,13 +172,37 @@ void PixelBufferRasterWorkerPool::ScheduleTasks(RasterTask::Queue* queue) {
     }
   }
 
+  tasks_required_for_activation_.clear();
+  raster_tasks_required_for_activation_.clear();
+  for (TaskMap::iterator it = new_pixel_buffer_tasks.begin();
+       it != new_pixel_buffer_tasks.end(); ++it) {
+    internal::RasterWorkerPoolTask* task = it->first;
+    if (IsRasterTaskRequiredForActivation(task)) {
+      internal::WorkerPoolTask* pixel_buffer_task = it->second.get();
+
+      tasks_required_for_activation_.insert(task);
+      if (!pixel_buffer_task || !pixel_buffer_task->HasCompleted())
+        raster_tasks_required_for_activation_.insert(task);
+    }
+  }
+
   pixel_buffer_tasks_.swap(new_pixel_buffer_tasks);
 
-  // This will schedule more tasks after checking for completed raster
-  // tasks. It's worth checking for completed tasks when ScheduleTasks()
-  // is called as priorities might have changed and this allows us to
-  // schedule as many new top priority tasks as possible.
-  CheckForCompletedRasterTasks();
+  // Check for completed tasks when ScheduleTasks() is called as
+  // priorities might have changed and this maximizes the number
+  // of top priority tasks that are scheduled.
+  RasterWorkerPool::CheckForCompletedTasks();
+  CheckForCompletedUploads();
+  FlushUploads();
+
+  // Schedule new tasks.
+  ScheduleMoreTasks();
+
+  // Cancel any pending check for completed raster tasks and schedule
+  // another check.
+  check_for_completed_raster_tasks_callback_.Cancel();
+  check_for_completed_raster_tasks_pending_ = false;
+  ScheduleCheckForCompletedRasterTasks();
 }
 
 void PixelBufferRasterWorkerPool::CheckForCompletedTasks() {
@@ -176,8 +212,11 @@ void PixelBufferRasterWorkerPool::CheckForCompletedTasks() {
   CheckForCompletedUploads();
   FlushUploads();
 
-  while (!completed_tasks_.empty()) {
-    internal::RasterWorkerPoolTask* task = completed_tasks_.front().get();
+  TaskDeque completed_tasks;
+  completed_tasks_.swap(completed_tasks);
+
+  while (!completed_tasks.empty()) {
+    internal::RasterWorkerPoolTask* task = completed_tasks.front().get();
     DCHECK(pixel_buffer_tasks_.find(task) != pixel_buffer_tasks_.end());
 
     pixel_buffer_tasks_.erase(task);
@@ -185,17 +224,33 @@ void PixelBufferRasterWorkerPool::CheckForCompletedTasks() {
     task->DidComplete();
     task->DispatchCompletionCallback();
 
-    completed_tasks_.pop_front();
+    completed_tasks.pop_front();
   }
 }
 
 void PixelBufferRasterWorkerPool::OnRasterTasksFinished() {
+  // Early out when more raster tasks are pending.
+  if (num_scheduled_raster_tasks_ != PendingRasterTaskCount())

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

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?

[reveman, 2013/06/27 09:06:58]

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.
It is as far as the WorkerPool and base RasterWorkerPool class knows. The
concept of throttling is applied at the PixelBufferRasterWorkerPool level.

+    return;
+
   // Call CheckForCompletedTasks() when we've finished running all raster
-  // tasks needed since last time ScheduleMoreTasks() was called. This
+  // tasks needed since last time ScheduleTasks() was called. This
   // reduces latency when processing only a small number of raster tasks.
   CheckForCompletedRasterTasks();
 }
 
+void PixelBufferRasterWorkerPool::OnRasterTasksRequiredForActivationFinished() {
+  if (raster_tasks_required_for_activation_.empty())
+    return;
+
+  // Early out when more raster tasks required for activation are pending.
+  if (raster_tasks_required_for_activation_.size() !=

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

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:58]

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]

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]

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.

+      num_scheduled_raster_tasks_required_for_activation_)
+    return;
+
+  OnRasterTasksFinished();
+}
+
 void PixelBufferRasterWorkerPool::FlushUploads() {
   if (!has_performed_uploads_since_last_flush_)
     return;
@@ -271,6 +326,8 @@ void PixelBufferRasterWorkerPool::CheckForCompletedUploads() {
                      task) == completed_tasks_.end());
     completed_tasks_.push_back(task);
 
+    tasks_required_for_activation_.erase(task);
+
     tasks_with_completed_uploads.pop_front();
   }
 }
@@ -300,12 +357,16 @@ void PixelBufferRasterWorkerPool::CheckForCompletedRasterTasks() {
   CheckForCompletedUploads();
   FlushUploads();
 
-  ScheduleMoreTasks();
+  if (PendingRasterTaskCount())
+    ScheduleMoreTasks();
 
-  // Make sure another check for completed uploads is scheduled
-  // while there is still pending uploads left.
-  if (!tasks_with_pending_upload_.empty())
+  // Schedule another check for completed raster tasks while there are
+  // pending raster tasks or pending uploads.
+  if (PendingRasterTaskCount() || !tasks_with_pending_upload_.empty())
     ScheduleCheckForCompletedRasterTasks();
+
+  NotifyClientIfFinishedRunningTasksRequiredForActivation();
+  NotifyClientIfFinishedRunningTasks();
 }
 
 void PixelBufferRasterWorkerPool::ScheduleMoreTasks() {
@@ -314,6 +375,9 @@ void PixelBufferRasterWorkerPool::ScheduleMoreTasks() {
   size_t bytes_pending_upload = bytes_pending_upload_;
   size_t bytes_pending_raster = 0;
 
+  num_scheduled_raster_tasks_ = 0;
+  num_scheduled_raster_tasks_required_for_activation_ = 0;
+
   RasterTaskGraph graph;
   for (RasterTaskVector::const_iterator it = raster_tasks().begin();
        it != raster_tasks().end(); ++it) {
@@ -358,9 +422,15 @@ void PixelBufferRasterWorkerPool::ScheduleMoreTasks() {
     bytes_pending_raster = new_bytes_pending_raster;
     bytes_pending_upload = new_bytes_pending_upload;
 
+    ++num_scheduled_raster_tasks_;
+    if (IsRasterTaskRequiredForActivation(task))
+      ++num_scheduled_raster_tasks_required_for_activation_;
+
     // Use existing pixel buffer task if available.
     if (pixel_buffer_task) {
-      graph.InsertRasterTask(pixel_buffer_task, task->dependencies());
+      graph.InsertRasterTask(pixel_buffer_task,
+                             task->dependencies(),
+                             IsRasterTaskRequiredForActivation(task));
       continue;
     }
 
@@ -381,16 +451,16 @@ void PixelBufferRasterWorkerPool::ScheduleMoreTasks() {
                        base::Unretained(this),
                        make_scoped_refptr(task))));
     pixel_buffer_tasks_[task] = new_pixel_buffer_task;
-    graph.InsertRasterTask(new_pixel_buffer_task.get(), task->dependencies());
+    graph.InsertRasterTask(new_pixel_buffer_task.get(),
+                           task->dependencies(),
+                           IsRasterTaskRequiredForActivation(task));
   }
 
-  SetRasterTaskGraph(&graph);
+  // We might fail to schedule pending raster tasks when upload bound.
+  if (!bytes_pending_raster)
+    return;
 
-  // At least one task that could need an upload is now pending, schedule
-  // a check for completed raster tasks to ensure this upload is dispatched
-  // without too much latency.
-  if (bytes_pending_raster)
-    ScheduleCheckForCompletedRasterTasks();
+  SetRasterTaskGraph(&graph);
 }
 
 void PixelBufferRasterWorkerPool::OnRasterTaskCompleted(
@@ -406,6 +476,8 @@ void PixelBufferRasterWorkerPool::OnRasterTaskCompleted(
   // Balanced with MapPixelBuffer() call in ScheduleMoreTasks().
   resource_provider()->UnmapPixelBuffer(task->resource()->id());
 
+  raster_tasks_required_for_activation_.erase(task);
+
   if (!needs_upload) {
     resource_provider()->ReleasePixelBuffer(task->resource()->id());
     task->DidRun(was_canceled);
@@ -413,6 +485,7 @@ void PixelBufferRasterWorkerPool::OnRasterTaskCompleted(
                      completed_tasks_.end(),
                      task) == completed_tasks_.end());
     completed_tasks_.push_back(task);
+    tasks_required_for_activation_.erase(task);
     return;
   }
 
@@ -423,4 +496,37 @@ void PixelBufferRasterWorkerPool::OnRasterTaskCompleted(
   tasks_with_pending_upload_.push_back(task);
 }
 
+unsigned PixelBufferRasterWorkerPool::PendingRasterTaskCount() const {
+  unsigned num_completed_raster_tasks =
+      tasks_with_pending_upload_.size() + completed_tasks_.size();
+  DCHECK_GE(pixel_buffer_tasks_.size(), num_completed_raster_tasks);
+  return pixel_buffer_tasks_.size() - num_completed_raster_tasks;
+}
+
+void PixelBufferRasterWorkerPool::
+    NotifyClientIfFinishedRunningTasksRequiredForActivation() {
+  if (!finished_running_tasks_required_for_activation_pending_)
+    return;
+
+  if (!tasks_required_for_activation_.empty())
+    return;
+
+  client()->DidFinishedRunningTasksRequiredForActivation();
+  finished_running_tasks_required_for_activation_pending_ = false;
+}
+
+void PixelBufferRasterWorkerPool::NotifyClientIfFinishedRunningTasks() {
+  if (!finished_running_tasks_pending_)
+    return;
+
+  if (PendingRasterTaskCount())
+    return;
+
+  if (!tasks_with_pending_upload_.empty())
+    return;
+
+  client()->DidFinishedRunningTasks();
+  finished_running_tasks_pending_ = false;
+}
+
 }  // namespace cc