Shared Raster Worker Threads

cc/resources/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/worker_pool.h"
 
 #include <algorithm>
 #include <queue>
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/containers/hash_tables.h"
 #include "base/debug/trace_event.h"
+#include "base/lazy_instance.h"
+#include "base/memory/linked_ptr.h"
 #include "base/strings/stringprintf.h"
 #include "base/synchronization/condition_variable.h"
 #include "base/threading/simple_thread.h"
 #include "base/threading/thread_restrictions.h"
 #include "cc/base/scoped_ptr_deque.h"
+#include "cc/base/switches.h"
 
 namespace cc {
 
-namespace internal {
+namespace {
 
-WorkerPoolTask::WorkerPoolTask()
-    : did_schedule_(false),
-      did_run_(false),
-      did_complete_(false) {
-}
+// TaskGraphRunner will be able to run task graphs
+// from multiple workerpool instances. All members are guarded
+// by |lock_|.

[reveman, 2013/12/20 01:53:24]

nit: s/will be able to/can process/ and I think you can fit the comment on 2
lines

[sohanjg, 2013/12/20 10:09:29]

Done.

+class TaskGraphRunner : public base::DelegateSimpleThread::Delegate {
+ public:
+  TaskGraphRunner(size_t num_threads, const std::string& thread_name_prefix);
+  virtual ~TaskGraphRunner();

[reveman, 2013/12/20 01:53:24]

nit: blankline between ctor/dtor and other member functions

[sohanjg, 2013/12/20 10:09:29]

Done.

+  void Register(const WorkerPool* worker_pool);
+  void Unregister(const WorkerPool* worker_pool);
+  void Shutdown();
 
-WorkerPoolTask::~WorkerPoolTask() {
-  DCHECK_EQ(did_schedule_, did_complete_);
-  DCHECK(!did_run_ || did_schedule_);
-  DCHECK(!did_run_ || did_complete_);
-}
+  typedef WorkerPool::TaskGraph TaskGraph;
 
-void WorkerPoolTask::DidSchedule() {
-  DCHECK(!did_complete_);
-  did_schedule_ = true;
-}
-
-void WorkerPoolTask::WillRun() {
-  DCHECK(did_schedule_);
-  DCHECK(!did_complete_);
-  DCHECK(!did_run_);
-}
-
-void WorkerPoolTask::DidRun() {
-  did_run_ = true;
-}
-
-void WorkerPoolTask::WillComplete() {
-  DCHECK(!did_complete_);
-}
-
-void WorkerPoolTask::DidComplete() {
-  DCHECK(did_schedule_);
-  DCHECK(!did_complete_);
-  did_complete_ = true;
-}
-
-bool WorkerPoolTask::HasFinishedRunning() const {
-  return did_run_;
-}
-
-bool WorkerPoolTask::HasCompleted() const {
-  return did_complete_;
-}
-
-GraphNode::GraphNode(internal::WorkerPoolTask* task, unsigned priority)
-    : task_(task),
-      priority_(priority),
-      num_dependencies_(0) {
-}
-
-GraphNode::~GraphNode() {
-}
-
-}  // namespace internal
-
-// Internal to the worker pool. Any data or logic that needs to be
-// shared between threads lives in this class. All members are guarded
-// by |lock_|.
-class WorkerPool::Inner : public base::DelegateSimpleThread::Delegate {
- public:
-  Inner(size_t num_threads, const std::string& thread_name_prefix);
-  virtual ~Inner();
-
-  void Shutdown();
+  typedef WorkerPool::TaskVector TaskVector;

[reveman, 2013/12/20 01:53:24]

can these typedefs be private?

[sohanjg, 2013/12/20 10:09:29]

SetTaskGraph, CollectCompletedTasks will need the typedef's. either we make the
2 func's pvt and inherit WorkerPool from TaskGraphRunner, or use some gloabl
scope, whats your opinion ?

[reveman, 2013/12/20 16:16:02]

Current code is fine. Just move the typedefs above the ctor. Types are typically
above ctor unless paired with a member.

 
   // Schedule running of tasks in |graph|. Tasks previously scheduled but
   // no longer needed will be canceled unless already running. Canceled
-  // tasks are moved to |completed_tasks_| without being run. The result
+  // tasks are moved to |completed_tasks| without being run. The result
   // is that once scheduled, a task is guaranteed to end up in the
-  // |completed_tasks_| queue even if they later get canceled by another
+  // |completed_tasks| queue even if they later get canceled by another
   // call to SetTaskGraph().
-  void SetTaskGraph(TaskGraph* graph);
+  void SetTaskGraph(const WorkerPool* worker_pool, TaskGraph* graph);
 
   // Collect all completed tasks in |completed_tasks|.
-  void CollectCompletedTasks(TaskVector* completed_tasks);
+  void CollectCompletedTasks(const WorkerPool* worker_pool,
+                                            TaskVector* completed_tasks);

[reveman, 2013/12/20 01:53:24]

nit: "TaskVector* co.." should vertically align with "const WorkerPo.."

[sohanjg, 2013/12/20 10:09:29]

Done.

 
  private:
-  class PriorityComparator {
+  class TaskPriorityComparator {
    public:
-    bool operator()(const internal::GraphNode* a,
-                    const internal::GraphNode* b) {
+    bool operator()(internal::GraphNode* a,
+                    internal::GraphNode* b) {
       // In this system, numerically lower priority is run first.
       if (a->priority() != b->priority())
         return a->priority() > b->priority();
 
       // Run task with most dependents first when priority is the same.
       return a->dependents().size() < b->dependents().size();
     }
   };
 
+  // Ordered set of tasks that are ready to run.
+  typedef std::priority_queue<internal::GraphNode*,
+                              std::vector<internal::GraphNode*>,
+                              TaskPriorityComparator> TaskQueue;
+
+  struct TaskNamespace {
+    TaskGraph pending_tasks;
+    TaskGraph running_tasks;
+    TaskVector completed_tasks;
+    TaskQueue ready_to_run_tasks;
+  };
+
+  class  TaskNamespacePriorityComparator {

[reveman, 2013/12/19 19:35:49]

nit: remove one space between class and TaskNamespace..

[sohanjg, 2013/12/20 10:09:29]

Done.

+  public:

[reveman, 2013/12/19 19:35:49]

nit: "public:" should be indented one space.

[sohanjg, 2013/12/20 10:09:29]

Done.

+    bool operator()(TaskNamespace* a,
+        TaskNamespace* b) {

[reveman, 2013/12/19 19:35:49]

nit: "TaskNamespace* b" vertically aligned with "TaskNamespace* a"

[sohanjg, 2013/12/20 10:09:29]

Done.

+        return comparator_.operator()(a->ready_to_run_tasks.top(),
+                                       b->ready_to_run_tasks.top());

[reveman, 2013/12/19 19:35:49]

nit: "b->ready_to_run_tasks.top()" veritcally aligned with
"a->ready_to_run_tasks.top()"

[sohanjg, 2013/12/20 10:09:29]

Done.

+    }
+    TaskPriorityComparator comparator_;

[reveman, 2013/12/19 19:35:49]

nit: s/comparator_/task_comparator_/

[sohanjg, 2013/12/20 10:09:29]

Done.

+};
+
+  typedef std::map<const WorkerPool*,
+                              linked_ptr<TaskNamespace> > TaskNamespaceMap;

[reveman, 2013/12/19 19:35:49]

nit: fix style:

typedef std::map<const WorkerPool*, linked_ptr<TaskNamespace> >
    TaskNamespaceMap;

[sohanjg, 2013/12/20 10:09:29]

Done.

+
+  typedef std::priority_queue<TaskNamespace*,

[reveman, 2013/12/20 01:53:24]

please add this comment above this:
// Ordered set of tasks namespaces that have ready to run tasks.

[sohanjg, 2013/12/20 10:09:29]

Done.

+                              std::vector<TaskNamespace*>,
+                              TaskNamespacePriorityComparator> NamespaceQueue;

[reveman, 2013/12/19 19:35:49]

nit: TaskNamespaceQueue

[sohanjg, 2013/12/20 10:09:29]

Done.

+
   // Overridden from base::DelegateSimpleThread:
   virtual void Run() OVERRIDE;
 
   // This lock protects all members of this class except
   // |worker_pool_on_origin_thread_|. Do not read or modify anything
   // without holding this lock. Do not block while holding this lock.
   mutable base::Lock lock_;
 
   // Condition variable that is waited on by worker threads until new
   // tasks are ready to run or shutdown starts.
   base::ConditionVariable has_ready_to_run_tasks_cv_;
 
   // Provides each running thread loop with a unique index. First thread
   // loop index is 0.
   unsigned next_thread_index_;
 
   // Set during shutdown. Tells workers to exit when no more tasks
   // are pending.
   bool shutdown_;
 
-  // This set contains all pending tasks.
-  GraphNodeMap pending_tasks_;
-
-  // Ordered set of tasks that are ready to run.
-  typedef std::priority_queue<internal::GraphNode*,
-                              std::vector<internal::GraphNode*>,
-                              PriorityComparator> TaskQueue;
-  TaskQueue ready_to_run_tasks_;
-
-  // This set contains all currently running tasks.
-  GraphNodeMap running_tasks_;
-
-  // Completed tasks not yet collected by origin thread.
-  TaskVector completed_tasks_;
-
   ScopedPtrDeque<base::DelegateSimpleThread> workers_;
 
-  DISALLOW_COPY_AND_ASSIGN(Inner);
+  TaskNamespaceMap namespaces_;
+
+  NamespaceQueue ready_to_run_namespaces_;
+
+  DISALLOW_COPY_AND_ASSIGN(TaskGraphRunner);
 };
 
-WorkerPool::Inner::Inner(
+class CC_EXPORT DerivedInner : public TaskGraphRunner {

[reveman, 2013/12/20 01:53:24]

This needs a better name. I think CompositorRasterTaskGraphRunner will do for
now.

[sohanjg, 2013/12/20 10:09:29]

Done.

+  public:
+    DerivedInner();

[reveman, 2013/12/20 01:53:24]

the implementation of this can be inlined.

[sohanjg, 2013/12/20 10:09:29]

Done.

+};

[reveman, 2013/12/20 01:53:24]

please move this class below the TaskGraphRunner implementation.

[sohanjg, 2013/12/20 10:09:29]

Done.

+
+base::LazyInstance<DerivedInner>  g_workerpool_inner;

[reveman, 2013/12/20 01:53:24]

I think this should be "g_task_graph_runner = LAZY_INSTANCE_INITIALIZER;" and
this of course needs to be moved too.

[sohanjg, 2013/12/20 10:09:29]

Done.

+
+
+TaskGraphRunner::TaskGraphRunner(
     size_t num_threads, const std::string& thread_name_prefix)
     : lock_(),
       has_ready_to_run_tasks_cv_(&lock_),
       next_thread_index_(0),
       shutdown_(false) {
   base::AutoLock lock(lock_);
 
   while (workers_.size() < num_threads) {
     scoped_ptr<base::DelegateSimpleThread> worker = make_scoped_ptr(
         new base::DelegateSimpleThread(
             this,
             thread_name_prefix +
             base::StringPrintf(
                 "Worker%u",
                 static_cast<unsigned>(workers_.size() + 1)).c_str()));
     worker->Start();
 #if defined(OS_ANDROID) || defined(OS_LINUX)
     worker->SetThreadPriority(base::kThreadPriority_Background);
 #endif
     workers_.push_back(worker.Pass());
   }
 }
 
-WorkerPool::Inner::~Inner() {
+TaskGraphRunner::~TaskGraphRunner() {
   base::AutoLock lock(lock_);
 
   DCHECK(shutdown_);
-
-  DCHECK_EQ(0u, pending_tasks_.size());
-  DCHECK_EQ(0u, ready_to_run_tasks_.size());
-  DCHECK_EQ(0u, running_tasks_.size());
-  DCHECK_EQ(0u, completed_tasks_.size());
+  DCHECK_EQ(0u, ready_to_run_namespaces_.size());
 }
 
-void WorkerPool::Inner::Shutdown() {
+void TaskGraphRunner::Register(const WorkerPool* worker_pool) {
+  base::AutoLock lock(lock_);
+
+  DCHECK(namespaces_.find(worker_pool) == namespaces_.end());
+  linked_ptr<TaskNamespace> task_set = make_linked_ptr(new TaskNamespace());
+  namespaces_[worker_pool] = task_set;
+}

[reveman, 2013/12/20 01:53:24]

nit: add blank line between these functions

[sohanjg, 2013/12/20 10:09:29]

Done.

+void TaskGraphRunner::Unregister(const WorkerPool* worker_pool) {
+  base::AutoLock lock(lock_);
+
+  DCHECK(namespaces_.find(worker_pool) != namespaces_.end());
+  namespaces_.erase(worker_pool);
+}
+
+void TaskGraphRunner::Shutdown() {
   {
     base::AutoLock lock(lock_);
 
     DCHECK(!shutdown_);
     shutdown_ = true;
-
     // Wake up a worker so it knows it should exit. This will cause all workers
     // to exit as each will wake up another worker before exiting.
     has_ready_to_run_tasks_cv_.Signal();
   }
 
   while (workers_.size()) {
     scoped_ptr<base::DelegateSimpleThread> worker = workers_.take_front();
     // http://crbug.com/240453 - Join() is considered IO and will block this
     // thread. See also http://crbug.com/239423 for further ideas.
     base::ThreadRestrictions::ScopedAllowIO allow_io;
     worker->Join();
   }
 }
 
-void WorkerPool::Inner::SetTaskGraph(TaskGraph* graph) {
+void TaskGraphRunner::SetTaskGraph(const WorkerPool* worker_pool,
+                                          TaskGraph* graph) {

[reveman, 2013/12/19 19:35:49]

nit: "TaskGraph* graph" vertically aligned with "const WorkerPool* worker_pool"

[sohanjg, 2013/12/20 10:09:29]

Done.

   // It is OK to call SetTaskGraph() after shutdown if |graph| is empty.
   DCHECK(graph->empty() || !shutdown_);
 
-  GraphNodeMap new_pending_tasks;
-  GraphNodeMap new_running_tasks;
+  TaskGraph new_pending_tasks;
+  TaskGraph new_running_tasks;
   TaskQueue new_ready_to_run_tasks;
+  NamespaceQueue new_ready_to_run_namespaces;
 
   new_pending_tasks.swap(*graph);
 
   {
     base::AutoLock lock(lock_);
 
+    DCHECK(namespaces_.find(worker_pool) != namespaces_.end());
+    linked_ptr<TaskNamespace> task_namespace = namespaces_[worker_pool];

[reveman, 2013/12/19 19:35:49]

This can be a raw pointer:
TaskNamespace* task_namespace = namespaces_[worker_pool].get();

[sohanjg, 2013/12/20 10:09:29]

Done.

+
     // First remove all completed tasks from |new_pending_tasks| and
     // adjust number of dependencies.
-    for (TaskVector::iterator it = completed_tasks_.begin();
-         it != completed_tasks_.end(); ++it) {
+    for (TaskVector::iterator it = task_namespace->completed_tasks.begin();
+         it != task_namespace->completed_tasks.end(); ++it) {
       internal::WorkerPoolTask* task = it->get();
-
       scoped_ptr<internal::GraphNode> node = new_pending_tasks.take_and_erase(
           task);
       if (node) {
         for (internal::GraphNode::Vector::const_iterator it =
                  node->dependents().begin();
              it != node->dependents().end(); ++it) {
           internal::GraphNode* dependent_node = *it;
           dependent_node->remove_dependency();
         }
       }
     }
-
     // Build new running task set.
-    for (GraphNodeMap::iterator it = running_tasks_.begin();
-         it != running_tasks_.end(); ++it) {
+    for (TaskGraph::iterator it =
+          task_namespace->running_tasks.begin();

[reveman, 2013/12/19 19:35:49]

nit: move to previous line if it fits. otherwise 4 space indent relative to
"TaskGraph::".

[sohanjg, 2013/12/20 10:09:29]

Done.

+          it != task_namespace->running_tasks.end(); ++it) {

[reveman, 2013/12/19 19:35:49]

nit: "it != task_n..." vertically aligned with "TaskGraph::iter.."

[sohanjg, 2013/12/20 10:09:29]

Done.

       internal::WorkerPoolTask* task = it->first;
       // Transfer scheduled task value from |new_pending_tasks| to
       // |new_running_tasks| if currently running. Value must be set to
       // NULL if |new_pending_tasks| doesn't contain task. This does
       // the right in both cases.
       new_running_tasks.set(task, new_pending_tasks.take_and_erase(task));
-    }
+     }

[reveman, 2013/12/19 19:35:49]

nit: no need to add this space.

[sohanjg, 2013/12/20 10:09:29]

Done.

 
     // Build new "ready to run" tasks queue.
     // TODO(reveman): Create this queue when building the task graph instead.
-    for (GraphNodeMap::iterator it = new_pending_tasks.begin();
+    for (TaskGraph::iterator it = new_pending_tasks.begin();
          it != new_pending_tasks.end(); ++it) {
       internal::WorkerPoolTask* task = it->first;
       DCHECK(task);
       internal::GraphNode* node = it->second;
 
       // Completed tasks should not exist in |new_pending_tasks|.
       DCHECK(!task->HasFinishedRunning());
 
       // Call DidSchedule() to indicate that this task has been scheduled.
       // Note: This is only for debugging purposes.
       task->DidSchedule();
 
       if (!node->num_dependencies())
         new_ready_to_run_tasks.push(node);
 
       // Erase the task from old pending tasks.
-      pending_tasks_.erase(task);
+      task_namespace->pending_tasks.erase(task);
+
     }
 
-    completed_tasks_.reserve(completed_tasks_.size() + pending_tasks_.size());
+    task_namespace->completed_tasks.reserve(
+        task_namespace->completed_tasks.size() +
+        task_namespace->pending_tasks.size());
 
-    // The items left in |pending_tasks_| need to be canceled.
-    for (GraphNodeMap::const_iterator it = pending_tasks_.begin();
-         it != pending_tasks_.end();
-         ++it) {
-      completed_tasks_.push_back(it->first);
+    // The items left in |pending_tasks| need to be canceled.
+    for (TaskGraph::const_iterator it =
+           task_namespace->pending_tasks.begin();

[reveman, 2013/12/19 19:35:49]

nit: move to previous line if it fits. otherwise 4 space indent relative to
"TaskGraph::".

[sohanjg, 2013/12/20 10:09:29]

Done.

+           it != task_namespace->pending_tasks.end();

[reveman, 2013/12/19 19:35:49]

nit: "it != task_n..." vertically aligned with "TaskGraph::const_iter.."

[sohanjg, 2013/12/20 10:09:29]

Done.

+           ++it) {

[reveman, 2013/12/19 19:35:49]

please move this to previous line to be consistent with other loops in this
file.

[sohanjg, 2013/12/20 10:09:29]

Done.

+        task_namespace->completed_tasks.push_back(it->first);

[reveman, 2013/12/19 19:35:49]

nit: this should be indented 2 spaces, not 4

[sohanjg, 2013/12/20 10:09:29]

Done.

     }
 
     // Swap task sets.
     // Note: old tasks are intentionally destroyed after releasing |lock_|.
-    pending_tasks_.swap(new_pending_tasks);
-    running_tasks_.swap(new_running_tasks);
-    std::swap(ready_to_run_tasks_, new_ready_to_run_tasks);
+    task_namespace->pending_tasks.swap(new_pending_tasks);
+    task_namespace->running_tasks.swap(new_running_tasks);
+    std::swap(task_namespace->ready_to_run_tasks, new_ready_to_run_tasks);
 
-    // If |ready_to_run_tasks_| is empty, it means we either have
+    // Re-create the ready_to_run_namespaces_ and add new TaskNamespace
+    while (!ready_to_run_namespaces_.empty()) {
+      new_ready_to_run_namespaces.push(ready_to_run_namespaces_.top());
+      ready_to_run_namespaces_.pop();
+    }

[reveman, 2013/12/19 19:35:49]

Please ignore ready_to_run_namespaces_ here and simply build
new_ready_to_run_namespaces by iterating over task_namespaces_ instead. Add only
add task namespaces with non-empty ready_to_run_tasks.

Also, please move this above line 280 so all swapping is done in one place.

[sohanjg, 2013/12/20 10:09:29]

Done.

+    std::swap(ready_to_run_namespaces_, new_ready_to_run_namespaces);
+    ready_to_run_namespaces_.push(task_namespace.get());

[reveman, 2013/12/19 19:35:49]

this line should not be here if you do what I suggested above.

[sohanjg, 2013/12/20 10:09:29]

Done.

+
+    // If |ready_to_run_tasks| is empty, it means we either have
     // running tasks, or we have no pending tasks.
-    DCHECK(!ready_to_run_tasks_.empty() ||
-           (pending_tasks_.empty() || !running_tasks_.empty()));
+    DCHECK(!task_namespace->ready_to_run_tasks.empty() ||
+           (task_namespace->pending_tasks.empty() ||
+           !task_namespace->running_tasks.empty()));

[reveman, 2013/12/19 19:35:49]

nit: this last line should be indented one more space as it's part of the (a ||
b) group

[sohanjg, 2013/12/20 10:09:29]

Done.

 
     // If there is more work available, wake up worker thread.
-    if (!ready_to_run_tasks_.empty())
+    if (!task_namespace->ready_to_run_tasks.empty())

[reveman, 2013/12/19 19:35:49]

Please change this to:

if (!ready_to_run_namespaces_.empty())

[sohanjg, 2013/12/20 10:09:29]

Done.

       has_ready_to_run_tasks_cv_.Signal();
   }
 }
 
-void WorkerPool::Inner::CollectCompletedTasks(TaskVector* completed_tasks) {
+void TaskGraphRunner::CollectCompletedTasks
+  (const WorkerPool* worker_pool, TaskVector* completed_tasks) {

[reveman, 2013/12/20 01:53:24]

nit: "(" on previous line. "const WorkerPool* wor.." indented 4 spaces

[sohanjg, 2013/12/20 10:09:29]

Done.

   base::AutoLock lock(lock_);
 
   DCHECK_EQ(0u, completed_tasks->size());
-  completed_tasks->swap(completed_tasks_);
+  if (!ready_to_run_namespaces_.empty())

[reveman, 2013/12/20 01:53:24]

no need for this conditional.

[sohanjg, 2013/12/20 10:09:29]

i am seeing a crash when we try to access top if its empty, please let me know
your opinion. 

+    completed_tasks->swap(ready_to_run_namespaces_.top()->completed_tasks);
 }
 
-void WorkerPool::Inner::Run() {
+void TaskGraphRunner::Run() {
   base::AutoLock lock(lock_);
 
   // Get a unique thread index.
   int thread_index = next_thread_index_++;
+  TaskNamespace* ready_to_run_task_set = NULL;

[reveman, 2013/12/20 01:53:24]

move to "while (true) {" scope

[sohanjg, 2013/12/20 10:09:29]

Done.

 
   while (true) {
-    if (ready_to_run_tasks_.empty()) {
-      // Exit when shutdown is set and no more tasks are pending.
-      if (shutdown_ && pending_tasks_.empty())
-        break;
-
-      // Wait for more tasks.
-      has_ready_to_run_tasks_cv_.Wait();
-      continue;
+    if ((ready_to_run_namespaces_.empty()) ||

[reveman, 2013/12/20 01:53:24]

this check should only be "ready_to_run_namespaces_.empty()"

[sohanjg, 2013/12/20 10:09:29]

Done.

+        (ready_to_run_task_set &&
+        ready_to_run_task_set->ready_to_run_tasks.empty())) {
+        // Exit when shutdown is set and no more tasks are pending.

[reveman, 2013/12/20 01:53:24]

nit: this and following lines are indented incorrectly. should be 2 spaces.

[sohanjg, 2013/12/20 10:09:29]

Done.

+        if (shutdown_ && ready_to_run_task_set &&

[reveman, 2013/12/20 01:53:24]

Let's just make this "if (shutdown_)". We'll make that work properly later when
we rework the shutdown code.

[sohanjg, 2013/12/20 10:09:29]

Done.

+            ready_to_run_task_set->pending_tasks.empty()) {
+            break;

[reveman, 2013/12/20 01:53:24]

nit: wrong indent here too

[sohanjg, 2013/12/20 10:09:29]

Done.

+        }
+        // Wait for more tasks.
+        has_ready_to_run_tasks_cv_.Wait();
+        continue;
     }
 
-    // Take top priority task from |ready_to_run_tasks_|.
+    // Take top priority TaskNamespace from |ready_to_run_namespaces_|.
+    ready_to_run_task_set = ready_to_run_namespaces_.top();

[reveman, 2013/12/20 01:53:24]

You need to pop the namespace too. Think this should be:

TaskNamespace* task_namespace = ready_to_run_namespaces_.top();
ready_to_run_namespaces_.pop();
DCHECK(!task_namespace->ready_to_run_tasks.empty());

[sohanjg, 2013/12/20 10:09:29]

Done.

The pop i had done in the end of the loop, when ready_to_run_tasks was empty, so
the next push you refer below could have been avoided.
essentially you are suggesting, pop every time, and push again if
ready_to_run_tasks is not empty.
and what i did was pop only if ready_to_run_tasks was empty.

+
+    // Take top priority task from |ready_to_run_tasks|.
     scoped_refptr<internal::WorkerPoolTask> task(
-        ready_to_run_tasks_.top()->task());
-    ready_to_run_tasks_.pop();
+        ready_to_run_task_set->ready_to_run_tasks.top()->task());
+    ready_to_run_task_set->ready_to_run_tasks.pop();
 

[reveman, 2013/12/20 01:53:24]

Here after taking the top task off the queue, you need to add the task_namespace
back to ready_to_run_namespaces_ if task_namespace->ready_to_run_tasks is not
empty.

[sohanjg, 2013/12/20 10:09:29]

Done.

-    // Move task from |pending_tasks_| to |running_tasks_|.
-    DCHECK(pending_tasks_.contains(task.get()));
-    DCHECK(!running_tasks_.contains(task.get()));
-    running_tasks_.set(task.get(), pending_tasks_.take_and_erase(task.get()));
+
+    // Move task from |pending_tasks| to |running_tasks|.
+    DCHECK(ready_to_run_task_set->pending_tasks.contains(task.get()));
+    DCHECK(!ready_to_run_task_set->running_tasks.contains(task.get()));
+
+    ready_to_run_task_set->running_tasks.set(
+        task.get(), ready_to_run_task_set->pending_tasks.take_and_erase

[reveman, 2013/12/20 01:53:24]

nit: move "ready_to_run_task_set->pendi" to the next line

[sohanjg, 2013/12/20 10:09:29]

Done.

+        (task.get()));
 
     // There may be more work available, so wake up another worker thread.
     has_ready_to_run_tasks_cv_.Signal();
 
+

[reveman, 2013/12/20 01:53:24]

nit: no need to add this line

[sohanjg, 2013/12/20 10:09:29]

Done.

     // Call WillRun() before releasing |lock_| and running task.
     task->WillRun();
 
     {
       base::AutoUnlock unlock(lock_);
-

[reveman, 2013/12/20 01:53:24]

nit: no need to remove this line

       task->RunOnWorkerThread(thread_index);
     }
 
     // This will mark task as finished running.
     task->DidRun();
 
     // Now iterate over all dependents to remove dependency and check
     // if they are ready to run.
-    scoped_ptr<internal::GraphNode> node = running_tasks_.take_and_erase(
+    scoped_ptr<internal::GraphNode> node =
+        ready_to_run_task_set->running_tasks.take_and_erase(
         task.get());
-    if (node) {
-      for (internal::GraphNode::Vector::const_iterator it =
-               node->dependents().begin();
-           it != node->dependents().end(); ++it) {
-        internal::GraphNode* dependent_node = *it;
+      if (node) {

[reveman, 2013/12/20 01:53:24]

nit: not need to change indention of this and following lines

[sohanjg, 2013/12/20 10:09:29]

Done.

+        for (internal::GraphNode::Vector::const_iterator it =
+                 node->dependents().begin();
+             it != node->dependents().end(); ++it) {
+          internal::GraphNode* dependent_node = *it;
 
-        dependent_node->remove_dependency();
-        // Task is ready if it has no dependencies. Add it to
-        // |ready_to_run_tasks_|.
-        if (!dependent_node->num_dependencies())
-          ready_to_run_tasks_.push(dependent_node);
+          dependent_node->remove_dependency();
+          // Task is ready if it has no dependencies. Add it to
+          // |ready_to_run_tasks|.
+          if (!dependent_node->num_dependencies())
+            ready_to_run_task_set->ready_to_run_tasks.push(dependent_node);

[reveman, 2013/12/20 01:53:24]

you need to add the task namespace to |ready_to_run_namespaces_| if it was empty
before this

[sohanjg, 2013/12/20 10:09:29]

Done.

This could have been taken care of, if we popped at the end of loop based on
ready_to_run_tasks is empty or not.

+        }
       }
+
+    // Pop when ready_to_run_tasks is empty
+    if (ready_to_run_task_set->ready_to_run_tasks.empty()) {
+      ready_to_run_namespaces_.pop();

[reveman, 2013/12/20 01:53:24]

what's the purpose of these lines? I think they should just be removed.

[sohanjg, 2013/12/20 10:09:29]

Done.
purpose i have explained above.

     }
 
-    // Finally add task to |completed_tasks_|.
-    completed_tasks_.push_back(task);
+    // Finally add task to |completed_tasks|.
+    ready_to_run_task_set->completed_tasks.push_back(task);
   }
 
   // We noticed we should exit. Wake up the next worker so it knows it should
   // exit as well (because the Shutdown() code only signals once).
   has_ready_to_run_tasks_cv_.Signal();
 }
 
+// Derived TaskGraphRunner Ctor
+DerivedInner::DerivedInner(): TaskGraphRunner
+    (switches::GetNumRasterThreads(), "CompositorRaster") {
+}
+
+}  // namespace
+
+namespace internal {
+
+WorkerPoolTask::WorkerPoolTask()
+    : did_schedule_(false),
+      did_run_(false),
+      did_complete_(false) {
+}
+
+WorkerPoolTask::~WorkerPoolTask() {
+  DCHECK_EQ(did_schedule_, did_complete_);
+  DCHECK(!did_run_ || did_schedule_);
+  DCHECK(!did_run_ || did_complete_);
+}
+
+void WorkerPoolTask::DidSchedule() {
+  DCHECK(!did_complete_);
+  did_schedule_ = true;
+}
+
+void WorkerPoolTask::WillRun() {
+  DCHECK(did_schedule_);
+  DCHECK(!did_complete_);
+  DCHECK(!did_run_);
+}
+
+void WorkerPoolTask::DidRun() {
+  did_run_ = true;
+}
+
+void WorkerPoolTask::WillComplete() {
+  DCHECK(!did_complete_);
+}
+
+void WorkerPoolTask::DidComplete() {
+  DCHECK(did_schedule_);
+  DCHECK(!did_complete_);
+  did_complete_ = true;
+}
+
+bool WorkerPoolTask::HasFinishedRunning() const {
+  return did_run_;
+}
+
+bool WorkerPoolTask::HasCompleted() const {
+  return did_complete_;
+}
+
+GraphNode::GraphNode(internal::WorkerPoolTask* task, unsigned priority)
+    : task_(task),
+      priority_(priority),
+      num_dependencies_(0) {
+}
+
+GraphNode::~GraphNode() {
+}
+
+}  // namespace internal
+
+
 WorkerPool::WorkerPool(size_t num_threads,
                        const std::string& thread_name_prefix)
-    : in_dispatch_completion_callbacks_(false),
-      inner_(make_scoped_ptr(new Inner(num_threads, thread_name_prefix))) {
+    : in_dispatch_completion_callbacks_(false) {
+  g_workerpool_inner.Pointer()->Register(this);
 }
 
 WorkerPool::~WorkerPool() {
+  g_workerpool_inner.Pointer()->Unregister(this);
 }
 
 void WorkerPool::Shutdown() {
   TRACE_EVENT0("cc", "WorkerPool::Shutdown");
 
   DCHECK(!in_dispatch_completion_callbacks_);
-
-  inner_->Shutdown();
+  g_workerpool_inner.Pointer()->Shutdown();
 }
 
 void WorkerPool::CheckForCompletedTasks() {
   TRACE_EVENT0("cc", "WorkerPool::CheckForCompletedTasks");
 
   DCHECK(!in_dispatch_completion_callbacks_);
 
   TaskVector completed_tasks;
-  inner_->CollectCompletedTasks(&completed_tasks);
+  g_workerpool_inner.Pointer()->CollectCompletedTasks(this, &completed_tasks);
   ProcessCompletedTasks(completed_tasks);
 }
 
 void WorkerPool::ProcessCompletedTasks(
     const TaskVector& completed_tasks) {
   TRACE_EVENT1("cc", "WorkerPool::ProcessCompletedTasks",
                "completed_task_count", completed_tasks.size());
 
   // Worker pool instance is not reentrant while processing completed tasks.
   in_dispatch_completion_callbacks_ = true;
 
   for (TaskVector::const_iterator it = completed_tasks.begin();
        it != completed_tasks.end();
        ++it) {
     internal::WorkerPoolTask* task = it->get();
 
     task->WillComplete();
     task->CompleteOnOriginThread();
     task->DidComplete();
   }
 
   in_dispatch_completion_callbacks_ = false;
 }
 
 void WorkerPool::SetTaskGraph(TaskGraph* graph) {
   TRACE_EVENT1("cc", "WorkerPool::SetTaskGraph",
                "num_tasks", graph->size());
 
   DCHECK(!in_dispatch_completion_callbacks_);
-
-  inner_->SetTaskGraph(graph);
+  g_workerpool_inner.Pointer()->SetTaskGraph(this, graph);
 }
 
 }  // namespace cc