Re-land: cc: Remove WorkerPool class and instead use TaskGraphRunner directly.

cc/resources/raster_worker_pool.h

Index: cc/resources/raster_worker_pool.h
diff --git a/cc/resources/raster_worker_pool.h b/cc/resources/raster_worker_pool.h
index 7a821fd1a82f75fb224fc30eb2891868cae8c3dd..b4a266a8fc0c3dd1bf8116e2a2bc98b7fee246ad 100644
--- a/cc/resources/raster_worker_pool.h
+++ b/cc/resources/raster_worker_pool.h
@@ -12,22 +12,39 @@
 #include "cc/debug/rendering_stats_instrumentation.h"
 #include "cc/resources/picture_pile_impl.h"
 #include "cc/resources/raster_mode.h"
-#include "cc/resources/resource.h"
-#include "cc/resources/resource_provider.h"
+#include "cc/resources/resource_format.h"
+#include "cc/resources/task_graph_runner.h"
 #include "cc/resources/tile_priority.h"
-#include "cc/resources/worker_pool.h"
-#include "third_party/khronos/GLES2/gl2.h"
 
 class SkPixelRef;
 
 namespace cc {
+
+class ContextProvider;
+class Resource;
+class ResourceProvider;
+
 namespace internal {
 
+class CC_EXPORT WorkerPoolTask : public Task {

[vmpstr, 2014/01/21 18:14:04]

Do you think it's worth it to just merge the *Complete stuff into
internal::Task? I don't really think there will be a case where we want to run a
task and not have some sort of completion indication. If you know of a real case
where that would be (or rather would not be) needed, then this is fine. Other
than that, I would prefer to limit the number of task objects that we have. This
would also eliminate the need for static_casts that convert a task into a
workerpooltaks.

One issue of course is that the task runner doesn't actually complete tasks, but
maybe we can do that as well? That is, instead of collect in this class and then
call complete, it could just complete by itself? (we can swap/copy to a temp
vector and release the internal lock so that performance will be unchanged)

[alokp, 2014/01/21 18:42:50]

I agree. If Complete is the only extra, it seems worth moving it to
internal::Task.

[reveman, 2014/01/21 19:51:32]

The raster tasks passed to the TaskGraphRunner by PBRWP are a great example of
when we like to control completion processing outside TaskGraphRunner.

What I'd like to do is remove the need to create internal::RasterWorkerPoolTask
instances that are not internal::Task instances. The reason we have to do that
right now is that RasterWorkerPool instances are not in control over how
completed tasks are processed. By moving all this completion logic into the
RasterWorkerPool class instead of TaskGraphRunner, we're not only getting a
better separation of responsibilities but can also make our RasterWorkerPool
instances cleaner and more efficient. ie. no need to use anonymous functions to
process completion of raster tasks inside RasterWorkerPool instances.

This WorkerPoolTask class is temporary. Follow-up patch will merge this with the
RasterWorkerPoolTask. This was just a nice way for me to limit the size of this
patch while still getting the separation of logic that I'd like to be part of
it.

[vmpstr, 2014/01/21 20:30:43]

Right... I forgot about the uploads in PBRWP.

+ public:
+  virtual void CompleteOnOriginThread() = 0;
+
+  void WillComplete();
+  void DidComplete();
+  bool HasCompleted() const;
+
+ protected:
+  WorkerPoolTask();
+  virtual ~WorkerPoolTask();
+
+ private:
+  bool did_complete_;
+};
+
 class CC_EXPORT RasterWorkerPoolTask
     : public base::RefCounted<RasterWorkerPoolTask> {
  public:
-  typedef std::vector<scoped_refptr<WorkerPoolTask> > TaskVector;
-
   // Returns true if |buffer| was written to. False indicate that
   // the content of |buffer| is undefined and the resource doesn't
   // need to be initialized.
@@ -47,14 +64,14 @@ class CC_EXPORT RasterWorkerPoolTask
   bool HasCompleted() const;
 
   const Resource* resource() const { return resource_; }
-  const TaskVector& dependencies() const { return dependencies_; }
+  const internal::Task::Vector& dependencies() const { return dependencies_; }

[sohanjg, 2014/01/21 09:41:08]

How is it helpful to change WorkerPoolTask to Task ?

[reveman, 2014/01/21 15:37:48]

First, there's no base WorkerPool class anymore so I don't think "WorkerPool"
should be part of the name for the tasks that TaskGraphRunner process. Second,
this makes the tasks known to TaskGraphRunner simpler as it no longer has to be
aware of completion callbacks. That's now something that RasterWorkerPool can
implement by creating a internal::Task subclass. See the WorkerPoolTask class
above.

   bool use_gpu_rasterization() const { return use_gpu_rasterization_; }
 
  protected:
   friend class base::RefCounted<RasterWorkerPoolTask>;
 
   RasterWorkerPoolTask(const Resource* resource,
-                       TaskVector* dependencies,
+                       internal::Task::Vector* dependencies,
                        bool use_gpu_rasterization);
   virtual ~RasterWorkerPoolTask();
 
@@ -63,7 +80,7 @@ class CC_EXPORT RasterWorkerPoolTask
   bool did_complete_;
   bool was_canceled_;
   const Resource* resource_;
-  TaskVector dependencies_;
+  Task::Vector dependencies_;
   bool use_gpu_rasterization_;
 };
 
@@ -93,7 +110,7 @@ class CC_EXPORT RasterWorkerPoolClient {
 };
 
 // A worker thread pool that runs raster tasks.
-class CC_EXPORT RasterWorkerPool : public WorkerPool {
+class CC_EXPORT RasterWorkerPool {
  public:
   class CC_EXPORT Task {
    public:
@@ -110,8 +127,7 @@ class CC_EXPORT RasterWorkerPool : public WorkerPool {
       friend class RasterWorkerPool;
       friend class RasterWorkerPoolTest;
 
-      typedef internal::RasterWorkerPoolTask::TaskVector TaskVector;
-      TaskVector tasks_;
+      internal::Task::Vector tasks_;

[sohanjg, 2014/01/21 09:41:08]

And here from RasterWorkerPoolTask to Task ?

[reveman, 2014/01/21 15:37:48]

Type didn't change. Just removed confusing typedef now that we have
internal::Task::Vector.

     };
 
     Task();
@@ -174,27 +190,9 @@ class CC_EXPORT RasterWorkerPool : public WorkerPool {
 
   virtual ~RasterWorkerPool();
 
-  void SetClient(RasterWorkerPoolClient* client);
-
-  // Overidden from WorkerPool:
-  virtual void Shutdown() OVERRIDE;
-
-  // Schedule running of raster tasks in |queue| and all dependencies.
-  // Previously scheduled tasks that are no longer needed to run
-  // raster tasks in |queue| will be canceled unless already running.
-  // Once scheduled, reply callbacks are guaranteed to run for all tasks
-  // even if they later get canceled by another call to ScheduleTasks().
-  virtual void ScheduleTasks(RasterTask::Queue* queue) = 0;
-
-  // Returns the target that needs to be used for raster task resources.
-  virtual GLenum GetResourceTarget() const = 0;
+  static void SetNumRasterThreads(int num_threads);
 
-  // Returns the format that needs to be used for raster task resources.
-  virtual ResourceFormat GetResourceFormat() const = 0;
-
-  // Force a check for completed raster tasks.
-  // Calls completion callbacks on completed tasks.
-  virtual void CheckForCompletedTasks();
+  static int GetNumRasterThreads();
 
   // TODO(vmpstr): Figure out an elegant way to not pass this many parameters.
   static RasterTask CreateRasterTask(
@@ -218,7 +216,30 @@ class CC_EXPORT RasterWorkerPool : public WorkerPool {
       RenderingStatsInstrumentation* stats_instrumentation,
       const Task::Reply& reply);
 
+  void SetClient(RasterWorkerPoolClient* client);
+
+  // Tells the worker pool to shutdown after canceling all previously
+  // scheduled tasks. Reply callbacks are still guaranteed to run.
+  virtual void Shutdown();
+
+  // Schedule running of raster tasks in |queue| and all dependencies.
+  // Previously scheduled tasks that are no longer needed to run
+  // raster tasks in |queue| will be canceled unless already running.
+  // Once scheduled, reply callbacks are guaranteed to run for all tasks
+  // even if they later get canceled by another call to ScheduleTasks().
+  virtual void ScheduleTasks(RasterTask::Queue* queue) = 0;
+
+  // Force a check for completed tasks.
+  virtual void CheckForCompletedTasks();
+
+  // Returns the target that needs to be used for raster task resources.
+  virtual unsigned GetResourceTarget() const = 0;
+
+  // Returns the format that needs to be used for raster task resources.
+  virtual ResourceFormat GetResourceFormat() const = 0;
+
  protected:
+  typedef internal::TaskGraphRunner::TaskGraph TaskGraph;
   typedef std::vector<scoped_refptr<internal::WorkerPoolTask> > TaskVector;
   typedef std::vector<scoped_refptr<internal::RasterWorkerPoolTask> >
       RasterTaskVector;
@@ -226,8 +247,8 @@ class CC_EXPORT RasterWorkerPool : public WorkerPool {
       RasterTaskDeque;
   typedef base::hash_set<internal::RasterWorkerPoolTask*> RasterTaskSet;
   typedef internal::RasterWorkerPoolTask* TaskMapKey;
-  typedef base::hash_map<TaskMapKey,
-                         scoped_refptr<internal::WorkerPoolTask> > TaskMap;
+  typedef base::hash_map<TaskMapKey, scoped_refptr<internal::WorkerPoolTask> >
+      TaskMap;
 
   RasterWorkerPool(ResourceProvider* resource_provider,
                    ContextProvider* context_provider);
@@ -235,6 +256,9 @@ class CC_EXPORT RasterWorkerPool : public WorkerPool {
   virtual void OnRasterTasksFinished() = 0;
   virtual void OnRasterTasksRequiredForActivationFinished() = 0;
 
+  void CheckForCompletedWorkerPoolTasks();
+  void SetTaskGraph(TaskGraph* graph);
+
   void SetRasterTasks(RasterTask::Queue* queue);
   bool IsRasterTaskRequiredForActivation(
       internal::RasterWorkerPoolTask* task) const;
@@ -249,12 +273,11 @@ class CC_EXPORT RasterWorkerPool : public WorkerPool {
     return raster_tasks_required_for_activation_;
   }
   void set_raster_finished_task(
-      scoped_refptr<internal::WorkerPoolTask> raster_finished_task) {
+      internal::WorkerPoolTask* raster_finished_task) {
     raster_finished_task_ = raster_finished_task;
   }
   void set_raster_required_for_activation_finished_task(
-      scoped_refptr<internal::WorkerPoolTask>
-          raster_required_for_activation_finished_task) {
+      internal::WorkerPoolTask* raster_required_for_activation_finished_task) {
     raster_required_for_activation_finished_task_ =
         raster_required_for_activation_finished_task;
   }
@@ -273,7 +296,7 @@ class CC_EXPORT RasterWorkerPool : public WorkerPool {
 
   static internal::GraphNode* CreateGraphNodeForRasterTask(
       internal::WorkerPoolTask* raster_task,
-      const TaskVector& decode_tasks,
+      const internal::Task::Vector& decode_tasks,
       unsigned priority,
       TaskGraph* graph);
 
@@ -282,6 +305,7 @@ class CC_EXPORT RasterWorkerPool : public WorkerPool {
   void OnRasterRequiredForActivationFinished(
       const internal::WorkerPoolTask* source);
 
+  internal::NamespaceToken namespace_token_;
   RasterWorkerPoolClient* client_;
   ResourceProvider* resource_provider_;
   ContextProvider* context_provider_;