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..259ab1a60797e34d99ebdddad03084eafc9afb79 100644
--- a/content/renderer/raster_worker_pool.h
+++ b/content/renderer/raster_worker_pool.h
@@ -48,7 +48,8 @@ class CONTENT_EXPORT RasterWorkerPool : public base::TaskRunner,
 
   // Runs a task from one of the provided categories. Categories listed first
   // have higher priority.
-  void Run(const std::vector<cc::TaskCategory>& categories);
+  void Run(const std::vector<cc::TaskCategory>& categories,
+           base::ConditionVariable* work_ready_cv);
 
   void FlushForTesting();
 
@@ -102,6 +103,14 @@ class CONTENT_EXPORT RasterWorkerPool : public base::TaskRunner,
     DISALLOW_COPY_AND_ASSIGN(ClosureTask);
   };
 
+  // Helper function which signals the CV for the next task to run.
+  void SignalTaskReadyConditionVariables();

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

nit: SignalHasReadyToRunTasks(). I think that's more consistent with the naming
of other existing variables and functions and ConditionVariables in the name
doesn't seem necessary and more of an implementation detail

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

Done.

+
+  // 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);
+
   void ScheduleTasksWithLockAcquired(cc::NamespaceToken token,
                                      cc::TaskGraph* graph);
   void CollectCompletedTasksWithLockAcquired(cc::NamespaceToken token,
@@ -124,9 +133,11 @@ 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 variables for foreground and background tasks.
+  base::ConditionVariable foreground_has_ready_to_run_tasks_cv_;
+  base::ConditionVariable background_has_ready_to_run_tasks_cv_;
+  // The target number of threads that should be active at any given time.
+  int target_running_task_count_;

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

Can we remove target_running_task_count_ now?

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

I guess we don't strictly need this. It gets us the ability to limit the
foreground + background tasks to a fixed number - if a background task is
already running when new work comes in, we won't spin up as many foreground
tasks until that background task finishes, keeping us in the intended limit.

I think it's OK to temporarily exceed the limits in this case, especially as it
simplifies the logic a bit - removing this for now.

WDYT?

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

I think it's best to limit how background work can effect how efficient we run
foreground as much as possible. Not having this limit improves that.

   // 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_;