Reland - Refactor signaling in RWP

content/renderer/raster_worker_pool.cc

Index: content/renderer/raster_worker_pool.cc
diff --git a/content/renderer/raster_worker_pool.cc b/content/renderer/raster_worker_pool.cc
index 0924c0a5118b24e3a97717b8e842d3045fe55286..1558e8015b87340501c8aa9600b1624ed7f61d4d 100644
--- a/content/renderer/raster_worker_pool.cc
+++ b/content/renderer/raster_worker_pool.cc
@@ -21,19 +21,22 @@ namespace {
 // categories.
 class RasterWorkerPoolThread : public base::SimpleThread {
  public:
-  explicit RasterWorkerPoolThread(const std::string& name_prefix,
-                                  const Options& options,
-                                  RasterWorkerPool* pool,
-                                  std::vector<cc::TaskCategory> categories)
+  RasterWorkerPoolThread(const std::string& name_prefix,
+                         const Options& options,
+                         RasterWorkerPool* pool,
+                         std::vector<cc::TaskCategory> categories,
+                         base::ConditionVariable* ready_to_run_tasks_cv)
       : SimpleThread(name_prefix, options),
         pool_(pool),
-        categories_(categories) {}
+        categories_(categories),
+        ready_to_run_tasks_cv_(ready_to_run_tasks_cv) {}
 
-  void Run() override { pool_->Run(categories_); }
+  void Run() override { pool_->Run(categories_, ready_to_run_tasks_cv_); }
 
  private:
   RasterWorkerPool* const pool_;
   const std::vector<cc::TaskCategory> categories_;
+  base::ConditionVariable* const ready_to_run_tasks_cv_;
 };
 
 }  // namespace
@@ -116,7 +119,8 @@ class RasterWorkerPool::RasterWorkerPoolSequencedTaskRunner
 
 RasterWorkerPool::RasterWorkerPool()
     : namespace_token_(GetNamespaceToken()),
-      has_ready_to_run_tasks_cv_(&lock_),
+      foreground_has_ready_to_run_tasks_cv_(&lock_),
+      background_has_ready_to_run_tasks_cv_(&lock_),
       has_namespaces_with_finished_running_tasks_cv_(&lock_),
       shutdown_(false) {}
 
@@ -124,28 +128,34 @@ void RasterWorkerPool::Start(
     int num_threads,
     const base::SimpleThread::Options& thread_options) {
   DCHECK(threads_.empty());
-  while (threads_.size() < static_cast<size_t>(num_threads)) {
-    // Determine the categories that each thread can run.
-    std::vector<cc::TaskCategory> task_categories;
 
-    // The first thread can run nonconcurrent tasks.
-    if (threads_.size() == 0) {
-      task_categories.push_back(cc::TASK_CATEGORY_NONCONCURRENT_FOREGROUND);
-    }
-
-    // All threads can run foreground tasks.
-    task_categories.push_back(cc::TASK_CATEGORY_FOREGROUND);
+  // Start |num_threads| threads for foreground work, including nonconcurrent
+  // foreground work.
+  std::vector<cc::TaskCategory> foreground_categories;
+  foreground_categories.push_back(cc::TASK_CATEGORY_NONCONCURRENT_FOREGROUND);
+  foreground_categories.push_back(cc::TASK_CATEGORY_FOREGROUND);
 
-    // The last thread can run background tasks.
-    if (threads_.size() == (static_cast<size_t>(num_threads) - 1)) {
-      task_categories.push_back(cc::TASK_CATEGORY_BACKGROUND);
-    }
+  for (int i = 0; i < num_threads; i++) {
+    scoped_ptr<base::SimpleThread> thread(new RasterWorkerPoolThread(
+        base::StringPrintf("CompositorTileWorker%u",
+                           static_cast<unsigned>(threads_.size() + 1))
+            .c_str(),
+        thread_options, this, foreground_categories,
+        &foreground_has_ready_to_run_tasks_cv_));
+    thread->Start();
+    threads_.push_back(std::move(thread));
+  }
 
+  // Start a single thread for background work.
+  {
+    std::vector<cc::TaskCategory> background_categories;
+    background_categories.push_back(cc::TASK_CATEGORY_BACKGROUND);
     scoped_ptr<base::SimpleThread> thread(new RasterWorkerPoolThread(
         base::StringPrintf("CompositorTileWorker%u",
                            static_cast<unsigned>(threads_.size() + 1))
             .c_str(),
-        thread_options, this, task_categories));
+        thread_options, this, background_categories,
+        &background_has_ready_to_run_tasks_cv_));
     thread->Start();
     threads_.push_back(std::move(thread));
   }
@@ -165,7 +175,8 @@ void RasterWorkerPool::Shutdown() {
     shutdown_ = true;
 
     // Wake up all workers so they exit.
-    has_ready_to_run_tasks_cv_.Broadcast();
+    foreground_has_ready_to_run_tasks_cv_.Broadcast();
+    background_has_ready_to_run_tasks_cv_.Broadcast();
   }
   while (!threads_.empty()) {
     threads_.back()->Join();
@@ -211,17 +222,22 @@ bool RasterWorkerPool::RunsTasksOnCurrentThread() const {
   return true;
 }
 
-void RasterWorkerPool::Run(const std::vector<cc::TaskCategory>& categories) {
+void RasterWorkerPool::Run(const std::vector<cc::TaskCategory>& categories,
+                           base::ConditionVariable* ready_to_run_tasks_cv) {
   base::AutoLock lock(lock_);
 
   while (true) {
     if (!RunTaskWithLockAcquired(categories)) {
+      // We are no longer running tasks, which may allow another category to
+      // start running. Signal other worker threads.
+      SignalHasReadyToRunTasksWithLockAcquired();

[reveman, 2016/02/10 20:49:32]

Is this necessary if we keep the signal after work_queue_.CompleteTask? See my
other comment for this to make sense.

[ericrk, 2016/02/10 22:30:04]

So, if we signal after completing a task, signaling here isn't necessary.
However, if we signal after completing a task we can end up with duplicated
signaling.

We can potentially signal at three points:
1. After dequeueing a task, but before running it.
2. After completing a task.
3. When there are no more tasks to run for the current category (this line)

If we signal at location (1) only, we won't wake up the background thread when
foreground work finishes.

If we signal at location (2) only, we will wake up foreground threads slower
than is ideal (we wait until after a task is done to wake up an additional
thread).

If we signal at location (3) only, we only ever wake up 1 thread at a time.

To get correct behavior, we can signal at (1) and (3) or (1) and (2).

If we do (1) and (3) - the current approach in this code - we wake up threads
quickly by signaling at (1), and we correctly wake up background threads when
foreground work finishes (3). There is no duplicate signaling.

If we do (1) and (2), we wake up threads quickly (1), and we wake up the
background thread (2), but we also frequently double-signal. If multiple
foreground tasks run in a row, we may end up signaling at location (1) even
after we have just signaled at location (2), waking up 2 threads instead of the
desired 1 thread.

Does this make sense?

[reveman, 2016/02/10 23:25:08]

Yes, current code makes sense. Thanks for explaining! 

+
       // Exit when shutdown is set and no more tasks are pending.
       if (shutdown_)
         break;
 
       // Wait for more tasks.
-      has_ready_to_run_tasks_cv_.Wait();
+      ready_to_run_tasks_cv->Wait();
       continue;
     }
   }
@@ -266,9 +282,8 @@ void RasterWorkerPool::ScheduleTasksWithLockAcquired(cc::NamespaceToken token,
 
   work_queue_.ScheduleTasks(token, graph);
 
-  // If there is more work available, wake up the other worker threads.
-  if (work_queue_.HasReadyToRunTasks())
-    has_ready_to_run_tasks_cv_.Broadcast();
+  // There may be more work available, so wake up another worker thread.
+  SignalHasReadyToRunTasksWithLockAcquired();
 }
 
 void RasterWorkerPool::WaitForTasksToFinishRunning(cc::NamespaceToken token) {
@@ -288,6 +303,10 @@ void RasterWorkerPool::WaitForTasksToFinishRunning(cc::NamespaceToken token) {
 
     while (!work_queue_.HasFinishedRunningTasksInNamespace(task_namespace))
       has_namespaces_with_finished_running_tasks_cv_.Wait();
+
+    // There may be other namespaces that have finished running tasks, so wake
+    // up another origin thread.
+    has_namespaces_with_finished_running_tasks_cv_.Signal();
   }
 }
 
@@ -330,6 +349,9 @@ void RasterWorkerPool::RunTaskInCategoryWithLockAcquired(
   auto prioritized_task = work_queue_.GetNextTaskToRun(category);
   cc::Task* task = prioritized_task.task;
 
+  // There may be more work available, so wake up another worker thread.
+  SignalHasReadyToRunTasksWithLockAcquired();
+
   // Call WillRun() before releasing |lock_| and running task.
   task->WillRun();
 
@@ -344,14 +366,54 @@ void RasterWorkerPool::RunTaskInCategoryWithLockAcquired(
 
   work_queue_.CompleteTask(prioritized_task);
 
-  // We may have just dequeued more tasks, wake up the other worker threads.
-  if (work_queue_.HasReadyToRunTasks())
-    has_ready_to_run_tasks_cv_.Broadcast();

[reveman, 2016/02/10 20:49:32]

Why is this no longer needed? Doesn't CompleteTask potentially result on more
tasks being available to run?

[ericrk, 2016/02/10 22:30:04]

See my comments above.

-
   // If namespace has finished running all tasks, wake up origin threads.
   if (work_queue_.HasFinishedRunningTasksInNamespace(
           prioritized_task.task_namespace))
-    has_namespaces_with_finished_running_tasks_cv_.Broadcast();
+    has_namespaces_with_finished_running_tasks_cv_.Signal();
+}
+
+bool RasterWorkerPool::ShouldRunTaskForCategoryWithLockAcquired(

[reveman, 2016/02/10 20:49:32]

Do you think merging this into SignalHasReadyToRunTasksWithLockAcquired would
make for the most efficient and easiest to read code? If you'd like to use
helper functions instead of having all code in
SignalHasReadyToRunTasksWithLockAcquired() then can we remove the |category|
parameter which seem to be known at compile time and instead have two separate
helper functions? ie.  ShouldRunForegroundTask + ShouldRunBackgroundTask. Maybe
even move these helpers to anonymous namespace.

[ericrk, 2016/02/10 22:30:04]

Meant to use this code in RunTasksWithLockAcquired - it was a bug that I wasn't
(background tasks would not stop running) - this is why I factored this way.
Should make more sense now.

[reveman, 2016/02/10 23:25:08]

Got it. Looks good now.

+    cc::TaskCategory category) {
+  lock_.AssertAcquired();
+
+  if (category == cc::TASK_CATEGORY_BACKGROUND) {
+    // Only run background tasks if there are no foreground tasks running or
+    // ready to run.
+    size_t num_running_foreground_tasks =
+        work_queue_.NumRunningTasksForCategory(
+            cc::TASK_CATEGORY_NONCONCURRENT_FOREGROUND) +
+        work_queue_.NumRunningTasksForCategory(cc::TASK_CATEGORY_FOREGROUND);
+    bool has_ready_to_run_foreground_tasks =
+        work_queue_.HasReadyToRunTasksForCategory(
+            cc::TASK_CATEGORY_NONCONCURRENT_FOREGROUND) ||
+        work_queue_.HasReadyToRunTasksForCategory(cc::TASK_CATEGORY_FOREGROUND);
+
+    if (num_running_foreground_tasks > 0 || has_ready_to_run_foreground_tasks)
+      return false;
+  }
+
+  // Enforce that only one nonconcurrent task runs at a time.
+  if (category == cc::TASK_CATEGORY_NONCONCURRENT_FOREGROUND &&
+      work_queue_.NumRunningTasksForCategory(
+          cc::TASK_CATEGORY_NONCONCURRENT_FOREGROUND) > 0) {
+    return false;
+  }
+
+  return work_queue_.HasReadyToRunTasksForCategory(category);

[reveman, 2016/02/10 20:49:32]

Maybe early out and avoid all the work above if this is not true?

[ericrk, 2016/02/10 22:30:04]

Done.

+}
+
+void RasterWorkerPool::SignalHasReadyToRunTasksWithLockAcquired() {
+  lock_.AssertAcquired();
+
+  if (ShouldRunTaskForCategoryWithLockAcquired(cc::TASK_CATEGORY_FOREGROUND) ||
+      ShouldRunTaskForCategoryWithLockAcquired(
+          cc::TASK_CATEGORY_NONCONCURRENT_FOREGROUND)) {
+    foreground_has_ready_to_run_tasks_cv_.Signal();
+  }
+
+  if (ShouldRunTaskForCategoryWithLockAcquired(cc::TASK_CATEGORY_BACKGROUND)) {
+    background_has_ready_to_run_tasks_cv_.Signal();
+  }
 }
 
 RasterWorkerPool::ClosureTask::ClosureTask(const base::Closure& closure)