Shared Raster Worker Threads

cc/resources/worker_pool.cc

Index: cc/resources/worker_pool.cc
diff --git a/cc/resources/worker_pool.cc b/cc/resources/worker_pool.cc
old mode 100644
new mode 100755
index dca0c704f09dbc2559bf3f4adf860724ce9a6240..d82d7c3ac6873b5fe80222d051b5a960ebdeb4c2
--- a/cc/resources/worker_pool.cc
+++ b/cc/resources/worker_pool.cc
@@ -10,12 +10,14 @@
 #include "base/bind.h"
 #include "base/containers/hash_tables.h"
 #include "base/debug/trace_event.h"
+#include "base/lazy_instance.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"
 
+
 namespace cc {
 
 namespace internal {
@@ -81,6 +83,8 @@ GraphNode::~GraphNode() {
 // by |lock_|.
 class WorkerPool::Inner : public base::DelegateSimpleThread::Delegate {
  public:
+    // Required for Lazy Instantiation
+    Inner();
   Inner(size_t num_threads, const std::string& thread_name_prefix);

[reveman, 2013/11/15 17:21:41]

One ctor please.

How about we create a derived class with only a default ctor that calls this
existing Inner ctor with proper parameters to use for a global Inner instance?

The default ctor of the derived class can probably read
cc::switches::kNumRasterThreads directly unless we can think of a better place
to do that, in which case it should be a global variable that can be adjusted
using a static setter.

   virtual ~Inner();
 
@@ -97,6 +101,12 @@ class WorkerPool::Inner : public base::DelegateSimpleThread::Delegate {
   // Collect all completed tasks in |completed_tasks|.
   void CollectCompletedTasks(TaskVector* completed_tasks);
 
+  // Required to be pulled out of Ctor, for lazy instantiation
+  void Start();
+  // For setting number of threads from WorkerPool to WorkerPool::Inner
+  void SetNumThreads(size_t val) {num_threads_ = val;}

[reveman, 2013/11/15 17:21:41]

This is not thread safe. One LTHI might change the value of num_threads_ while
another is running Start(). However, if you follow my suggestion above none of
this is needed.

+
+
  private:
   class PriorityComparator {
    public:
@@ -148,9 +158,23 @@ class WorkerPool::Inner : public base::DelegateSimpleThread::Delegate {
 
   ScopedPtrDeque<base::DelegateSimpleThread> workers_;
 
+  size_t num_threads_;
+
   DISALLOW_COPY_AND_ASSIGN(Inner);
 };
 
+// Lazy Instance of WorkerPool::Inner,
+// This will enable sharing the worker thread across LTHI for same process
+base::LazyInstance<WorkerPool::Inner>  g_workerpool_inner;
+
+// Default Ctor required for Lazy Instantiation
+WorkerPool::Inner::Inner()
+    : lock_(),
+      has_ready_to_run_tasks_cv_(&lock_),
+      next_thread_index_(0),
+      shutdown_(false) {
+      }
+
 WorkerPool::Inner::Inner(
     size_t num_threads, const std::string& thread_name_prefix)
     : lock_(),
@@ -158,7 +182,6 @@ WorkerPool::Inner::Inner(
       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(
@@ -177,7 +200,6 @@ WorkerPool::Inner::Inner(
 
 WorkerPool::Inner::~Inner() {
   base::AutoLock lock(lock_);
-
   DCHECK(shutdown_);
 
   DCHECK_EQ(0u, pending_tasks_.size());
@@ -186,6 +208,31 @@ WorkerPool::Inner::~Inner() {
   DCHECK_EQ(0u, completed_tasks_.size());
 }
 
+// Required to be pulled out of Ctor, for lazy instantiation
+void WorkerPool::Inner::Start() {
+  base::AutoLock lock(lock_);
+
+  std::string thread_name_prefix = "CompositorRaster";
+  size_t num_threads = num_threads_;
+
+
+  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());
+    }
+}
+
 void WorkerPool::Inner::Shutdown() {
   {
     base::AutoLock lock(lock_);
@@ -375,8 +422,9 @@ void WorkerPool::Inner::Run() {
 
 WorkerPool::WorkerPool(size_t num_threads,

[reveman, 2013/11/15 17:21:41]

num_threads can't be a LTHI setting anymore as the threads are now process wide.

                        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()->SetNumThreads(num_threads);
+    g_workerpool_inner.Pointer()->Start();

[reveman, 2013/11/15 17:21:41]

What's stopping Start() from being called multiple times?

 }
 
 WorkerPool::~WorkerPool() {
@@ -386,8 +434,7 @@ void WorkerPool::Shutdown() {
   TRACE_EVENT0("cc", "WorkerPool::Shutdown");
 
   DCHECK(!in_dispatch_completion_callbacks_);
-
-  inner_->Shutdown();
+  g_workerpool_inner.Pointer()->Shutdown();

[reveman, 2013/11/15 17:21:41]

You need to introduce a task namespace for each WorkerPool instance and only
shutdown the tasks related to this namespace.

 }
 
 void WorkerPool::CheckForCompletedTasks() {
@@ -396,7 +443,7 @@ void WorkerPool::CheckForCompletedTasks() {
   DCHECK(!in_dispatch_completion_callbacks_);
 
   TaskVector completed_tasks;
-  inner_->CollectCompletedTasks(&completed_tasks);
+  g_workerpool_inner.Pointer()->CollectCompletedTasks(&completed_tasks);

[reveman, 2013/11/15 17:21:41]

Same here. Task namespace needed for this to work.

   ProcessCompletedTasks(completed_tasks);
 }
 
@@ -426,8 +473,7 @@ void WorkerPool::SetTaskGraph(TaskGraph* graph) {
                "num_tasks", graph->size());
 
   DCHECK(!in_dispatch_completion_callbacks_);
-
-  inner_->SetTaskGraph(graph);
-}
+  g_workerpool_inner.Pointer()->SetTaskGraph(graph);

[reveman, 2013/11/15 17:21:41]

And here. Task namespace needed for this to work.

+  }
 
 }  // namespace cc