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..774e48d7ce423403779622e4718e4522869b1e84 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* work_ready_cv)
       : SimpleThread(name_prefix, options),
         pool_(pool),
-        categories_(categories) {}
+        categories_(categories),
+        work_ready_cv_(work_ready_cv) {}
 
-  void Run() override { pool_->Run(categories_); }
+  void Run() override { pool_->Run(categories_, work_ready_cv_); }
 
  private:
   RasterWorkerPool* const pool_;
   const std::vector<cc::TaskCategory> categories_;
+  base::ConditionVariable* const work_ready_cv_;

[reveman, 2016/02/08 19:10:21]

nit: ready_to_run_tasks_cv_ for consistency

[ericrk, 2016/02/10 18:44:49]

Done.

 };
 
 }  // namespace
@@ -116,7 +119,9 @@ 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_),
+      target_running_task_count_(0),
       has_namespaces_with_finished_running_tasks_cv_(&lock_),
       shutdown_(false) {}
 
@@ -124,28 +129,36 @@ 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);
-    }
+  target_running_task_count_ = num_threads;
 
-    // All threads can run foreground tasks.
-    task_categories.push_back(cc::TASK_CATEGORY_FOREGROUND);
+  // start |num_threads| threads for foreground work, including nonconcurrent

[reveman, 2016/02/08 19:10:21]

nit: s/start/Start/

[ericrk, 2016/02/10 18:44:49]

Done.

+  // 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 +178,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 +225,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* work_ready_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.
+      SignalTaskReadyConditionVariables();
+
       // 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();
+      work_ready_cv->Wait();
       continue;
     }
   }
@@ -266,9 +285,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.
+  SignalTaskReadyConditionVariables();
 }
 
 void RasterWorkerPool::WaitForTasksToFinishRunning(cc::NamespaceToken token) {
@@ -288,6 +306,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();
   }
 }
 
@@ -320,7 +342,6 @@ bool RasterWorkerPool::RunTaskWithLockAcquired(
   }
   return false;
 }
-
 void RasterWorkerPool::RunTaskInCategoryWithLockAcquired(
     cc::TaskCategory category) {
   TRACE_EVENT0("toplevel", "TaskGraphRunner::RunTask");
@@ -330,6 +351,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.
+  SignalTaskReadyConditionVariables();
+
   // Call WillRun() before releasing |lock_| and running task.
   task->WillRun();
 
@@ -344,14 +368,46 @@ 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();
-
   // 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::ShouldRunTaskForCategory(cc::TaskCategory category) {

[reveman, 2016/02/08 19:10:21]

nit: lock_.AssertAcquired() and add "WithLockAcquired" suffix to function name

[ericrk, 2016/02/10 18:44:49]

Done.

+  // During shutdown we want to flush threads as fast as possible - don't bother
+  // enforcing running task limits.

[reveman, 2016/02/08 19:10:21]

Is this special case worthwhile? Seems like it makes sense to keep the same
restrictions during shutdown to avoid corner cases and potentially hard to
diagnose bugs.

[ericrk, 2016/02/10 18:44:49]

makes sense - will remove this.

+  if (!shutdown_) {
+    // Enforce running task limits.
+    int running_task_count = work_queue_.NumRunningTasks();
+    if (running_task_count >= target_running_task_count_) {
+      return false;
+    }
+
+    if (category == cc::TASK_CATEGORY_BACKGROUND && running_task_count > 0) {
+      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);
+}
+
+void RasterWorkerPool::SignalTaskReadyConditionVariables() {

[reveman, 2016/02/08 19:10:21]

nit: add "WithLockAcquired" suffix to function name

[ericrk, 2016/02/10 18:44:49]

Done.

+  lock_.AssertAcquired();

[reveman, 2016/02/08 19:10:21]

nit: blank line after this to separate it from the implementation of the
function

[ericrk, 2016/02/10 18:44:49]

Done.

+  if (ShouldRunTaskForCategory(cc::TASK_CATEGORY_FOREGROUND) ||
+      ShouldRunTaskForCategory(cc::TASK_CATEGORY_NONCONCURRENT_FOREGROUND)) {
+    foreground_has_ready_to_run_tasks_cv_.Signal();
+  } else if (ShouldRunTaskForCategory(cc::TASK_CATEGORY_BACKGROUND)) {
+    // Only run background tasks if there are no forground tasks left.

[reveman, 2016/02/08 19:10:21]

hm, it looks like this logic is both here and inside the
ShouldRunTaskForCategory function. Assuming we need ShouldRunTaskForCategory,
can all this logic such as only running background tasks when no foreground
tasks are running be constrained to that function? ie. can we remove the "else"
and move this comment to ShouldRunTaskForCategory.

[ericrk, 2016/02/10 18:44:49]

We can do this - results in a little extra work being done, but does clean up
the logic.

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

sorry- I was assuming that a separate ShouldRunTaskForCategory was necessary but
it looks like that is not the case so maybe the best is to just merge all this
into this function to avoid extra work and make it as clean as possible. Latest
patch is fine though so if you prefer that then no need to change it except
maybe remove the category argument. See my comment on the latest patch.

+    background_has_ready_to_run_tasks_cv_.Signal();
+  }
 }
 
 RasterWorkerPool::ClosureTask::ClosureTask(const base::Closure& closure)