Reland - Refactor signaling in RWP

content/renderer/raster_worker_pool.h

Index: content/renderer/raster_worker_pool.h
diff --git a/content/renderer/raster_worker_pool.h b/content/renderer/raster_worker_pool.h
index 1d6fc04ca6719ed8fd22f34093b7a35d3c8b4814..69a2d676beace551ae82da0e2ebe6ce73d5ec40b 100644
--- a/content/renderer/raster_worker_pool.h
+++ b/content/renderer/raster_worker_pool.h
@@ -46,9 +46,8 @@ class CONTENT_EXPORT RasterWorkerPool : public base::TaskRunner,
   void CollectCompletedTasks(cc::NamespaceToken token,
                              cc::Task::Vector* completed_tasks) override;
 
-  // Runs a task from one of the provided categories. Categories listed first
-  // have higher priority.
-  void Run(const std::vector<cc::TaskCategory>& categories);
+  // Runs a task from the provided category.
+  void Run(cc::TaskCategory category);
 
   void FlushForTesting();
 
@@ -75,14 +74,6 @@ class CONTENT_EXPORT RasterWorkerPool : public base::TaskRunner,
   class RasterWorkerPoolSequencedTaskRunner;
   friend class RasterWorkerPoolSequencedTaskRunner;
 
-  // Runs a task from one of the provided categories. Categories listed first
-  // have higher priority. Returns false if there were no tasks to run.
-  bool RunTaskWithLockAcquired(const std::vector<cc::TaskCategory>& categories);
-
-  // Run next task for the given category. Caller must acquire |lock_| prior to
-  // calling this function and make sure at least one task is ready to run.
-  void RunTaskInCategoryWithLockAcquired(cc::TaskCategory category);
-
   // Simple Task for the TaskGraphRunner that wraps a closure.
   // This class is used to schedule TaskRunner tasks on the
   // |task_graph_runner_|.
@@ -102,6 +93,46 @@ class CONTENT_EXPORT RasterWorkerPool : public base::TaskRunner,
     DISALLOW_COPY_AND_ASSIGN(ClosureTask);
   };
 
+  // Helper class used to increment the running task count while a task is
+  // running. Automatically decrements when this class goes out of scope.
+  class ScopedTaskCount {
+   public:
+    ScopedTaskCount(int* running_task_count)
+        : running_task_count_(running_task_count) {
+      (*running_task_count_)++;
+    }
+    ~ScopedTaskCount() {
+      (*running_task_count_)--;
+      DCHECK_GE(*running_task_count_, 0);
+    }
+
+   private:
+    int* const running_task_count_;
+  };
+
+  // Run next task for the given category. Caller must acquire |lock_| prior to
+  // calling this function and make sure at least one task is ready to run.
+  void RunTaskWithLockAcquired(cc::TaskCategory category);
+
+  // Helper function which signals CVs for each category where tasks are ready
+  // to be run.
+  void SignalTaskReadyConditionVariables();
+
+  // Helper which returns a ScopedTaskCount for the given category. This
+  // increments the count immediately, and decrements it when the returned
+  // ScopedTaskCount is deleted.
+  ScopedTaskCount ScopedTaskCountForCategory(cc::TaskCategory category);
+
+  // Determines if we should run a new task for the given category. This factors
+  // in whether a task is available and whether the count of running tasks is
+  // low enough to start a new one.
+  bool ShouldRunTaskForCategory(cc::TaskCategory category);
+
+  // Helper which returns the condition variable being used to signal work
+  // available for the given category.
+  base::ConditionVariable& ConditionVariableForCategory(
+      cc::TaskCategory category);
+
   void ScheduleTasksWithLockAcquired(cc::NamespaceToken token,
                                      cc::TaskGraph* graph);
   void CollectCompletedTasksWithLockAcquired(cc::NamespaceToken token,
@@ -124,9 +155,17 @@ class CONTENT_EXPORT RasterWorkerPool : public base::TaskRunner,
   // Cached vector to avoid allocation when getting the list of complete
   // tasks.
   cc::Task::Vector completed_tasks_;
-  // Condition variable that is waited on by Run() until new tasks are ready to
-  // run or shutdown starts.
-  base::ConditionVariable has_ready_to_run_tasks_cv_;
+  // Condition variable for each cc::TaskCategory. Waited on by Run() until new
+  // tasks are ready to run or shutdown starts.
+  base::ConditionVariable nonconcurrent_foreground_has_ready_to_run_tasks_cv_;
+  base::ConditionVariable foreground_has_ready_to_run_tasks_cv_;
+  base::ConditionVariable background_has_ready_to_run_tasks_cv_;
+  // Counters which track the number of running tasks for each category.

[reveman, 2016/02/05 17:15:02]

Can we use the existing run count in work_queue instead of duplicating the state
here?

+  int nonconcurrent_foreground_running_task_count_;
+  int foreground_running_task_count_;
+  int background_running_task_count_;
+  // The target number of threads that should be active at any given time.
+  int target_running_task_count_;

[reveman, 2016/02/05 17:15:02]

I find this a bit awkward. Now that we're using the "run count" to determine
what to run, could we just have "num_threads" of foreground priority threads
that can all run nonconcurrent and normal foreground tasks but if a
nonconcurrent task can run or not depends on 0 nonconcurrent tasks currently
running?

In this case only two CVs would be needed, one for background-tasks-can-run and
one for foreground-tasks-can-run. When any foreground thread wakes up, it would
run a nonconcurrent task if possible, otherwise a normal foreground task.

   // Condition variable that is waited on by origin threads until a namespace
   // has finished running all associated tasks.
   base::ConditionVariable has_namespaces_with_finished_running_tasks_cv_;