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

cc/resources/raster_worker_pool.cc

 // Copyright 2013 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 "cc/resources/raster_worker_pool.h"
 
 #include "base/debug/trace_event_synthetic_delay.h"
 #include "base/json/json_writer.h"
+#include "base/lazy_instance.h"
 #include "base/metrics/histogram.h"
 #include "base/values.h"
 #include "cc/debug/devtools_instrumentation.h"
 #include "cc/debug/traced_value.h"
 #include "cc/resources/picture_pile_impl.h"
+#include "cc/resources/resource.h"
+#include "cc/resources/resource_provider.h"
 #include "skia/ext/paint_simplifier.h"
 #include "third_party/skia/include/core/SkBitmap.h"
 #include "third_party/skia/include/core/SkPixelRef.h"
 #include "third_party/skia/include/gpu/GrContext.h"
 #include "third_party/skia/include/gpu/SkGpuDevice.h"
 
 namespace cc {
 
 namespace {
 
@@ skipping 44 matching lines @@
                            const gfx::Rect& content_rect,
                            float contents_scale,
                            RasterMode raster_mode,
                            TileResolution tile_resolution,
                            int layer_id,
                            const void* tile_id,
                            int source_frame_number,
                            bool use_gpu_rasterization,
                            RenderingStatsInstrumentation* rendering_stats,
                            const RasterWorkerPool::RasterTask::Reply& reply,
-                           TaskVector* dependencies)
+                           internal::Task::Vector* dependencies)
       : internal::RasterWorkerPoolTask(resource,
                                        dependencies,
                                        use_gpu_rasterization),
         picture_pile_(picture_pile),
         content_rect_(content_rect),
         contents_scale_(contents_scale),
         raster_mode_(raster_mode),
         tile_resolution_(tile_resolution),
         layer_id_(layer_id),
         tile_id_(tile_id),
         source_frame_number_(source_frame_number),
         rendering_stats_(rendering_stats),
-        reply_(reply) {}
+        reply_(reply) {

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

nit: do we need this line here ?

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

I'm trying to make the code consistent in that only functions that completely
fit on one line get to use "{}". A function that has a line break somewhere (in
parameter list, initialization list...) gets a line break between "{" and "}".
This is not in the style guide afaik and my choice here is somewhat arbitrary
but it made the most sense to me. The important part is making it consistent.

[danakj, 2014/01/21 15:47:05]

FWIW the easiest way to be consistent is to just run git cl format and let it
figure this stuff out. Soon a presubmit warning will be added to cc if code
doesn't match what Git cl format would do.  

[reveman, 2014/01/21 16:10:44]

Good point. I'm not going to "cl format" this patch as it results in huge
changes to chrome/ that makes this patch really hard to read. What I'll do is
remove any style changes from this patch that are not what "cl format" prefer
and then do a style-only change where I "cl format" all worker pool code in a
follow up.

+  }
 
   void RunAnalysisOnThread(unsigned thread_index) {
     TRACE_EVENT1("cc",
                  "RasterWorkerPoolTaskImpl::RunAnalysisOnThread",
                  "data",
                  TracedValue::FromValue(DataAsValue().release()));
 
     DCHECK(picture_pile_.get());
     DCHECK(rendering_stats_);
 
@@ skipping 202 matching lines @@
 
 class ImageDecodeWorkerPoolTaskImpl : public internal::WorkerPoolTask {
  public:
   ImageDecodeWorkerPoolTaskImpl(SkPixelRef* pixel_ref,
                                 int layer_id,
                                 RenderingStatsInstrumentation* rendering_stats,
                                 const RasterWorkerPool::Task::Reply& reply)
       : pixel_ref_(skia::SharePtr(pixel_ref)),
         layer_id_(layer_id),
         rendering_stats_(rendering_stats),
-        reply_(reply) {}
+        reply_(reply) {

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

nit: do we need this line here ?

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

sohanjg, you're right. clang-format prefer it the old way so I removed this
change.

+  }
 
-  // Overridden from internal::WorkerPoolTask:
+  // Overridden from internal::Task:
   virtual void RunOnWorkerThread(unsigned thread_index) OVERRIDE {
     TRACE_EVENT0("cc", "ImageDecodeWorkerPoolTaskImpl::RunOnWorkerThread");
     devtools_instrumentation::ScopedImageDecodeTask image_decode_task(
         pixel_ref_.get());
     // This will cause the image referred to by pixel ref to be decoded.
     pixel_ref_->lockPixels();
     pixel_ref_->unlockPixels();
   }
+
+  // Overridden from internal::WorkerPoolTask:
   virtual void CompleteOnOriginThread() OVERRIDE {
     reply_.Run(!HasFinishedRunning());
   }
 
  protected:
   virtual ~ImageDecodeWorkerPoolTaskImpl() {}
 
  private:
   skia::RefPtr<SkPixelRef> pixel_ref_;
   int layer_id_;
   RenderingStatsInstrumentation* rendering_stats_;
   const RasterWorkerPool::Task::Reply reply_;
 
   DISALLOW_COPY_AND_ASSIGN(ImageDecodeWorkerPoolTaskImpl);
 };
 
 class RasterFinishedWorkerPoolTaskImpl : public internal::WorkerPoolTask {
  public:
-  typedef base::Callback<void(const internal::WorkerPoolTask* source)>
-      Callback;
+  typedef base::Callback<void(const internal::WorkerPoolTask* source)> Callback;
 
   RasterFinishedWorkerPoolTaskImpl(
       const Callback& on_raster_finished_callback)
       : origin_loop_(base::MessageLoopProxy::current().get()),
         on_raster_finished_callback_(on_raster_finished_callback) {
   }
 
   // Overridden from internal::WorkerPoolTask:
   virtual void RunOnWorkerThread(unsigned thread_index) OVERRIDE {
     TRACE_EVENT0("cc", "RasterFinishedWorkerPoolTaskImpl::RunOnWorkerThread");
@@ skipping 48 matching lines @@
  private:
   virtual ~RasterRequiredForActivationFinishedWorkerPoolTaskImpl() {}
 
   base::TimeTicks activation_delay_end_time_;
   const size_t tasks_required_for_activation_count_;
 
   DISALLOW_COPY_AND_ASSIGN(
       RasterRequiredForActivationFinishedWorkerPoolTaskImpl);
 };
 
+class RasterTaskGraphRunner : public internal::TaskGraphRunner {
+ public:
+  RasterTaskGraphRunner()
+      : internal::TaskGraphRunner(RasterWorkerPool::GetNumRasterThreads(),
+                                  "CompositorRaster") {
+  }
+};
+base::LazyInstance<RasterTaskGraphRunner>::Leaky g_task_graph_runner =
+    LAZY_INSTANCE_INITIALIZER;
+
+const int kDefaultNumRasterThreads = 1;
+
+int g_num_raster_threads = 0;

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

make these static members of RasterTaskGraphRunner or RasterWorkerPool?

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

I prefer anonymous namespace and hiding these from the .h when possible. What
would the benefit of moving them to RasterTaskGraphRunner be? It doesn't feel
like they belong there, imo.

 
 }  // namespace
 
 namespace internal {
 
-RasterWorkerPoolTask::RasterWorkerPoolTask(const Resource* resource,
-                                           TaskVector* dependencies,
-                                           bool use_gpu_rasterization)
+WorkerPoolTask::WorkerPoolTask() : did_complete_(false) {}
+
+WorkerPoolTask::~WorkerPoolTask() {
+  DCHECK_EQ(did_schedule_, did_complete_);
+  DCHECK(!did_run_ || did_complete_);
+}
+
+void WorkerPoolTask::WillComplete() {
+  DCHECK(!did_complete_);
+}
+
+void WorkerPoolTask::DidComplete() {
+  DCHECK(did_schedule_);
+  DCHECK(!did_complete_);
+  did_complete_ = true;
+}
+
+bool WorkerPoolTask::HasCompleted() const {
+  return did_complete_;
+}
+
+RasterWorkerPoolTask::RasterWorkerPoolTask(
+    const Resource* resource,
+    internal::Task::Vector* dependencies,
+    bool use_gpu_rasterization)
     : did_run_(false),
       did_complete_(false),
       was_canceled_(false),
       resource_(resource),
       use_gpu_rasterization_(use_gpu_rasterization) {
   dependencies_.swap(*dependencies);
 }
 
-RasterWorkerPoolTask::~RasterWorkerPoolTask() {
-}
+RasterWorkerPoolTask::~RasterWorkerPoolTask() {}
 
 void RasterWorkerPoolTask::DidRun(bool was_canceled) {
   DCHECK(!did_run_);
   did_run_ = true;
   was_canceled_ = was_canceled;
 }
 
 bool RasterWorkerPoolTask::HasFinishedRunning() const {
   return did_run_;
 }
@@ skipping 10 matching lines @@
   DCHECK(!did_complete_);
   did_complete_ = true;
 }
 
 bool RasterWorkerPoolTask::HasCompleted() const {
   return did_complete_;
 }
 
 }  // namespace internal
 
-RasterWorkerPool::Task::Set::Set() {
-}
+RasterWorkerPool::Task::Set::Set() {}
 
-RasterWorkerPool::Task::Set::~Set() {
-}
+RasterWorkerPool::Task::Set::~Set() {}
 
 void RasterWorkerPool::Task::Set::Insert(const Task& task) {
   DCHECK(!task.is_null());
   tasks_.push_back(task.internal_);
 }
 
-RasterWorkerPool::Task::Task() {
-}
+RasterWorkerPool::Task::Task() {}
 
 RasterWorkerPool::Task::Task(internal::WorkerPoolTask* internal)
     : internal_(internal) {
 }
 
-RasterWorkerPool::Task::~Task() {
-}
+RasterWorkerPool::Task::~Task() {}
 
 void RasterWorkerPool::Task::Reset() {
   internal_ = NULL;
 }
 
-RasterWorkerPool::RasterTask::Queue::Queue() {
-}
+RasterWorkerPool::RasterTask::Queue::Queue() {}
 
-RasterWorkerPool::RasterTask::Queue::~Queue() {
-}
+RasterWorkerPool::RasterTask::Queue::~Queue() {}
 
 void RasterWorkerPool::RasterTask::Queue::Append(
     const RasterTask& task, bool required_for_activation) {
   DCHECK(!task.is_null());
   tasks_.push_back(task.internal_);
   if (required_for_activation)
     tasks_required_for_activation_.insert(task.internal_.get());
 }
 
-RasterWorkerPool::RasterTask::RasterTask() {
-}
+RasterWorkerPool::RasterTask::RasterTask() {}
 
 RasterWorkerPool::RasterTask::RasterTask(
     internal::RasterWorkerPoolTask* internal)
     : internal_(internal) {
 }
 
 void RasterWorkerPool::RasterTask::Reset() {
   internal_ = NULL;
 }
 
-RasterWorkerPool::RasterTask::~RasterTask() {
+RasterWorkerPool::RasterTask::~RasterTask() {}
+
+RasterWorkerPool::RasterWorkerPool(ResourceProvider* resource_provider,
+                                   ContextProvider* context_provider)
+    : namespace_token_(g_task_graph_runner.Pointer()->GetNamespaceToken()),
+      client_(NULL),
+      resource_provider_(resource_provider),
+      context_provider_(context_provider),
+      weak_ptr_factory_(this) {
+}
+
+RasterWorkerPool::~RasterWorkerPool() {}
+
+// static
+void RasterWorkerPool::SetNumRasterThreads(int num_threads) {
+  DCHECK_LT(0, num_threads);
+  DCHECK_EQ(0, g_num_raster_threads);

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

Are you trying to make sure that SetNumRasterThreads is called only once? Isn't
it better to do that by having a local static variable here. The you do not need
kDefaultNumRasterThreads. You could simply initialize g_num_raster_threads with
whatever the default calue should be.

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

I also want to make sure it's not called after GetNumRasterThreads().

+
+  g_num_raster_threads = num_threads;
 }
 
 // static
+int RasterWorkerPool::GetNumRasterThreads() {
+  if (!g_num_raster_threads)
+    g_num_raster_threads = kDefaultNumRasterThreads;
+
+  return g_num_raster_threads;
+}
+
+// static
 RasterWorkerPool::RasterTask RasterWorkerPool::CreateRasterTask(
     const Resource* resource,
     PicturePileImpl* picture_pile,
     const gfx::Rect& content_rect,
     float contents_scale,
     RasterMode raster_mode,
     TileResolution tile_resolution,
     int layer_id,
     const void* tile_id,
     int source_frame_number,
@@ skipping 22 matching lines @@
     SkPixelRef* pixel_ref,
     int layer_id,
     RenderingStatsInstrumentation* stats_instrumentation,
     const Task::Reply& reply) {
   return Task(new ImageDecodeWorkerPoolTaskImpl(pixel_ref,
                                                 layer_id,
                                                 stats_instrumentation,
                                                 reply));
 }
 
-RasterWorkerPool::RasterWorkerPool(ResourceProvider* resource_provider,
-                                   ContextProvider* context_provider)
-    : client_(NULL),
-      resource_provider_(resource_provider),
-      context_provider_(context_provider),
-      weak_ptr_factory_(this) {
-}
-
-RasterWorkerPool::~RasterWorkerPool() {
-}
-
 void RasterWorkerPool::SetClient(RasterWorkerPoolClient* client) {
   client_ = client;
 }
 
 void RasterWorkerPool::Shutdown() {
+  TRACE_EVENT0("cc", "RasterWorkerPool::Shutdown");
+
   raster_tasks_.clear();
   TaskGraph empty;
   SetTaskGraph(&empty);
-  WorkerPool::Shutdown();
+  g_task_graph_runner.Pointer()->WaitForTasksToFinishRunning(namespace_token_);
   weak_ptr_factory_.InvalidateWeakPtrs();
 }
 
 void RasterWorkerPool::CheckForCompletedTasks() {
   TRACE_EVENT0("cc", "RasterWorkerPool::CheckForCompletedTasks");
 
-  // Check for completed worker-thread tasks.
-  CheckForCompletedWorkerTasks();
+  CheckForCompletedWorkerPoolTasks();
 
   // Complete gpu rasterization tasks.
   while (!completed_gpu_raster_tasks_.empty()) {
     internal::RasterWorkerPoolTask* task =
         completed_gpu_raster_tasks_.front().get();
+
     task->WillComplete();
     task->CompleteOnOriginThread();
     task->DidComplete();
 
     completed_gpu_raster_tasks_.pop_front();
   }
 }
 
+void RasterWorkerPool::CheckForCompletedWorkerPoolTasks() {
+  internal::Task::Vector completed_tasks;

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

nit: can we typedef internal::Task::Vector ?

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

To what? I think it's good to not remove the internal:: namespace and it doesn't
get much simpler than Task::Vector.

+  g_task_graph_runner.Pointer()->CollectCompletedTasks(
+      namespace_token_, &completed_tasks);
+
+  for (internal::Task::Vector::const_iterator it = completed_tasks.begin();
+       it != completed_tasks.end();
+       ++it) {
+    internal::WorkerPoolTask* task = static_cast<internal::WorkerPoolTask*>(
+        it->get());
+
+    task->WillComplete();
+    task->CompleteOnOriginThread();
+    task->DidComplete();
+  }
+}
+
+void RasterWorkerPool::SetTaskGraph(TaskGraph* graph) {
+  TRACE_EVENT1("cc", "RasterWorkerPool::SetTaskGraph",
+               "num_tasks", graph->size());
+
+  g_task_graph_runner.Pointer()->SetTaskGraph(namespace_token_, graph);
+}
+
 void RasterWorkerPool::SetRasterTasks(RasterTask::Queue* queue) {
   raster_tasks_.swap(queue->tasks_);
   raster_tasks_required_for_activation_.swap(
       queue->tasks_required_for_activation_);
 }
 
 bool RasterWorkerPool::IsRasterTaskRequiredForActivation(
     internal::RasterWorkerPoolTask* task) const {
   return
       raster_tasks_required_for_activation_.find(task) !=
@@ skipping 27 matching lines @@
 scoped_refptr<internal::WorkerPoolTask>
     RasterWorkerPool::CreateRasterFinishedTask() {
   return make_scoped_refptr(
       new RasterFinishedWorkerPoolTaskImpl(
           base::Bind(&RasterWorkerPool::OnRasterFinished,
                      weak_ptr_factory_.GetWeakPtr())));
 }
 
 scoped_refptr<internal::WorkerPoolTask>
     RasterWorkerPool::CreateRasterRequiredForActivationFinishedTask(
-       size_t tasks_required_for_activation_count) {
+        size_t tasks_required_for_activation_count) {
   return make_scoped_refptr(
       new RasterRequiredForActivationFinishedWorkerPoolTaskImpl(
           base::Bind(&RasterWorkerPool::OnRasterRequiredForActivationFinished,
                      weak_ptr_factory_.GetWeakPtr()),
           tasks_required_for_activation_count));
 }
 
 void RasterWorkerPool::OnRasterFinished(
     const internal::WorkerPoolTask* source) {
   TRACE_EVENT0("cc", "RasterWorkerPool::OnRasterFinished");
@@ skipping 32 matching lines @@
     TaskGraph* graph) {
   internal::GraphNode* node = new internal::GraphNode(task, priority);
   DCHECK(graph->find(task) == graph->end());
   graph->set(task, make_scoped_ptr(node));
   return node;
 }
 
 // static
 internal::GraphNode* RasterWorkerPool::CreateGraphNodeForRasterTask(
     internal::WorkerPoolTask* raster_task,
-    const TaskVector& decode_tasks,
+    const internal::Task::Vector& decode_tasks,
     unsigned priority,
     TaskGraph* graph) {
   DCHECK(!raster_task->HasCompleted());
 
   internal::GraphNode* raster_node = CreateGraphNodeForTask(
       raster_task, priority, graph);
 
   // Insert image decode tasks.
-  for (TaskVector::const_iterator it = decode_tasks.begin();
+  for (internal::Task::Vector::const_iterator it = decode_tasks.begin();
        it != decode_tasks.end(); ++it) {
-    internal::WorkerPoolTask* decode_task = it->get();
+    internal::WorkerPoolTask* decode_task =
+        static_cast<internal::WorkerPoolTask*>(it->get());
 
     // Skip if already decoded.
     if (decode_task->HasCompleted())
       continue;
 
     raster_node->add_dependency();
 
     // Check if decode task already exists in graph.
-    GraphNodeMap::iterator decode_it = graph->find(decode_task);
+    internal::GraphNode::Map::iterator decode_it = graph->find(decode_task);

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

nit: And typedef internal::GraphNode::Map too ?

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

nit: And typedef internal::GraphNode::Map too ?

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

what are you suggesting we typedef to? I like to keep internal:: as it makes it
clear that this should not leak into the public API.

     if (decode_it != graph->end()) {
       internal::GraphNode* decode_node = decode_it->second;
       decode_node->add_dependent(raster_node);
       continue;
     }
 
     internal::GraphNode* decode_node = CreateGraphNodeForTask(
         decode_task, priority, graph);
     decode_node->add_dependent(raster_node);
   }
 
   return raster_node;
 }
 
 }  // namespace cc