Reland - Refactor signaling in RWP

content/renderer/raster_worker_pool.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/renderer/raster_worker_pool.h"
 
 #include <string>
 #include <utility>
 #include <vector>
 
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_restrictions.h"
 #include "base/trace_event/trace_event.h"
 #include "cc/base/math_util.h"
 #include "cc/raster/task_category.h"
 
 namespace content {
 namespace {
 
 // A thread which forwards to RasterWorkerPool::Run with the runnable
 // 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
 
 // A sequenced task runner which posts tasks to a RasterWorkerPool.
 class RasterWorkerPool::RasterWorkerPoolSequencedTaskRunner
     : public base::SequencedTaskRunner {
  public:
   explicit RasterWorkerPoolSequencedTaskRunner(
       cc::TaskGraphRunner* task_graph_runner)
@@ skipping 62 matching lines @@
   cc::Task::Vector tasks_;
   // Graph object used for scheduling tasks.
   cc::TaskGraph graph_;
   // Cached vector to avoid allocation when getting the list of complete
   // tasks.
   cc::Task::Vector completed_tasks_;
 };
 
 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) {}
 
 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);
-    }
+  // 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);
 
-    // All threads can run foreground tasks.
-    task_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, task_categories));
+        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, background_categories,
+        &background_has_ready_to_run_tasks_cv_));
+    thread->Start();
+    threads_.push_back(std::move(thread));
+  }
 }
 
 void RasterWorkerPool::Shutdown() {
   WaitForTasksToFinishRunning(namespace_token_);
   CollectCompletedTasks(namespace_token_, &completed_tasks_);
   // Shutdown raster threads.
   {
     base::AutoLock lock(lock_);
 
     DCHECK(!work_queue_.HasReadyToRunTasks());
     DCHECK(!work_queue_.HasAnyNamespaces());
 
     DCHECK(!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();
     threads_.pop_back();
   }
 }
 
 // Overridden from base::TaskRunner:
 bool RasterWorkerPool::PostDelayedTask(
     const tracked_objects::Location& from_here,
@@ skipping 25 matching lines @@
 
   ScheduleTasksWithLockAcquired(namespace_token_, &graph_);
   completed_tasks_.clear();
   return true;
 }
 
 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;
     }
   }
 }
 
 void RasterWorkerPool::FlushForTesting() {
   base::AutoLock lock(lock_);
 
   while (!work_queue_.HasFinishedRunningTasksInAllNamespaces()) {
     has_namespaces_with_finished_running_tasks_cv_.Wait();
@@ skipping 24 matching lines @@
 }
 
 void RasterWorkerPool::ScheduleTasksWithLockAcquired(cc::NamespaceToken token,
                                                      cc::TaskGraph* graph) {
   DCHECK(token.IsValid());
   DCHECK(!cc::TaskGraphWorkQueue::DependencyMismatch(graph));
   DCHECK(!shutdown_);
 
   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) {
   TRACE_EVENT0("disabled-by-default-cc.debug",
                "RasterWorkerPool::WaitForTasksToFinishRunning");
 
   DCHECK(token.IsValid());
 
   {
     base::AutoLock lock(lock_);
     base::ThreadRestrictions::ScopedAllowWait allow_wait;
 
     auto* task_namespace = work_queue_.GetNamespaceForToken(token);
 
     if (!task_namespace)
       return;
 
     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();
   }
 }
 
 void RasterWorkerPool::CollectCompletedTasks(
     cc::NamespaceToken token,
     cc::Task::Vector* completed_tasks) {
   TRACE_EVENT0("disabled-by-default-cc.debug",
                "RasterWorkerPool::CollectCompletedTasks");
 
   {
@@ skipping 22 matching lines @@
 
 void RasterWorkerPool::RunTaskInCategoryWithLockAcquired(
     cc::TaskCategory category) {
   TRACE_EVENT0("toplevel", "TaskGraphRunner::RunTask");
 
   lock_.AssertAcquired();
 
   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();
 
   {
     base::AutoUnlock unlock(lock_);
 
     task->RunOnWorkerThread();
   }
 
   // This will mark task as finished running.
   task->DidRun();
 
   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)
     : closure_(closure) {}
 
 // Overridden from cc::Task:
 void RasterWorkerPool::ClosureTask::RunOnWorkerThread() {
   closure_.Run();
   closure_.Reset();
 }
 
 RasterWorkerPool::ClosureTask::~ClosureTask() {}
 
 }  // namespace content