Re-land: cc: Cancel and re-prioritize worker pool tasks.

cc/resources/raster_worker_pool.cc

Index: cc/resources/raster_worker_pool.cc
diff --git a/cc/resources/raster_worker_pool.cc b/cc/resources/raster_worker_pool.cc
index 0cb913d4e91ecf4447cc3fd164d50fc6595e8970..6cbfd8f6212d74c95d6fe584d6b966bfaaf1a7c8 100644
--- a/cc/resources/raster_worker_pool.cc
+++ b/cc/resources/raster_worker_pool.cc
@@ -10,24 +10,88 @@ namespace cc {
 
 namespace {
 
-class RasterWorkerPoolTaskImpl : public internal::WorkerPoolTask {
+class RasterWorkerPoolTaskImpl : public internal::RasterWorkerPoolTask {
  public:
-  RasterWorkerPoolTaskImpl(PicturePileImpl* picture_pile,
-                           const RasterWorkerPool::RasterCallback& task,
-                           const base::Closure& reply)
-      : internal::WorkerPoolTask(reply),
+  RasterWorkerPoolTaskImpl(const base::Closure& callback,
+                           const internal::RasterWorkerPoolTask::Reply& reply)
+      : internal::RasterWorkerPoolTask(reply),
+        callback_(callback) {
+  }
+
+  // Overridden from internal::WorkerPoolTask:
+  virtual void RunOnThread(unsigned thread_index) OVERRIDE {
+    WillRun();
+    callback_.Run();
+    DidRun();
+  }
+
+ private:
+  virtual ~RasterWorkerPoolTaskImpl() {}
+
+  const base::Closure callback_;
+};
+
+class RasterWorkerPoolPictureTaskImpl : public internal::RasterWorkerPoolTask {
+ public:
+  RasterWorkerPoolPictureTaskImpl(
+      PicturePileImpl* picture_pile,
+      const RasterWorkerPool::PictureTask::Callback& callback,
+      const internal::RasterWorkerPoolTask::Reply& reply)
+      : internal::RasterWorkerPoolTask(reply),
         picture_pile_(picture_pile),
-        task_(task) {
+        callback_(callback) {
     DCHECK(picture_pile_);
   }
 
+  // Overridden from internal::WorkerPoolTask:
   virtual void RunOnThread(unsigned thread_index) OVERRIDE {
-    task_.Run(picture_pile_->GetCloneForDrawingOnThread(thread_index));
+    WillRun();
+    callback_.Run(picture_pile_->GetCloneForDrawingOnThread(thread_index));

[vmpstr, 2013/05/06 23:08:48]

It's a bit unfortunate that we have to specialize RasterWorkerPoolTaskImpl just
because we need this line :\

Is it maybe possible to do something like the following:
Bind(&SelectCloneAndRun, 
     picture_pile,
     Bind(&ThingToRun, ...));

SelectCloneAndRun(pile, callback) would simply do
callback->Run(pile->GetCloneForDrawingOnThread(thread_id));

The only problem is getting the running thread id in SelectCloneAndRun. Is that
available somehow from the callback? I mean one option is to just always pass a
thread_index to the callback, but I think that might clutter all the callbacks.

[reveman, 2013/05/07 01:33:54]

I'd rather not expose the thread id. Also there's a bit of overhead from every
base::Bind so I'd avoid adding another one unless it's a significant cleanup. I
think it's ok to maintain a few lines of duplicate code here for now. We can
remove it once https://code.google.com/p/skia/issues/detail?id=1143 has been
fixed.

+    DidRun();
   }
 
  private:
+  virtual ~RasterWorkerPoolPictureTaskImpl() {}
+
   scoped_refptr<PicturePileImpl> picture_pile_;
-  RasterWorkerPool::RasterCallback task_;
+  RasterWorkerPool::PictureTask::Callback callback_;
+};
+
+class RasterWorkerPoolTaskQueueImpl : public internal::WorkerPoolTaskGraph {
+ public:
+  typedef std::deque<scoped_refptr<internal::RasterWorkerPoolTask> > TaskDeque;
+
+  explicit RasterWorkerPoolTaskQueueImpl(TaskDeque* tasks) {
+    tasks->swap(tasks_);
+  }
+  virtual ~RasterWorkerPoolTaskQueueImpl() {}
+
+  // Overridden from internal::WorkerPoolTaskGraph:
+  virtual bool HasMoreTasks() OVERRIDE {
+    return !tasks_.empty();
+  }
+  virtual bool HasTask(internal::WorkerPoolTask* task) OVERRIDE {
+    // TODO(reveman): Use hash if size of |tasks_| gets large.
+    return std::find(tasks_.begin(), tasks_.end(), task) != tasks_.end();
+  }
+  virtual internal::WorkerPoolTask* TopTask() OVERRIDE {
+    return tasks_.front();
+  }
+  virtual scoped_refptr<internal::WorkerPoolTask> TakeTask(
+      internal::WorkerPoolTask* task) OVERRIDE {
+    scoped_refptr<internal::WorkerPoolTask> reference = task;
+
+    // Note: This is efficient when removing the top task, which is
+    // the most common use case of TakeTask().
+    TaskDeque::iterator it = std::find(tasks_.begin(), tasks_.end(), task);
+    DCHECK(it != tasks_.end());
+    tasks_.erase(it);
+
+    return reference;
+  }
+
+ private:
+  TaskDeque tasks_;
 };
 
 const char* kWorkerThreadNamePrefix = "CompositorRaster";
@@ -36,6 +100,86 @@ const int kCheckForCompletedTasksDelayMs = 6;
 
 }  // namespace
 
+namespace internal {
+
+RasterWorkerPoolTask::RasterWorkerPoolTask(const Reply& reply)
+    : reply_(reply),
+      did_schedule_(false),
+      did_run_(false),
+      did_complete_(false) {
+}
+
+RasterWorkerPoolTask::~RasterWorkerPoolTask() {
+  DCHECK_EQ(did_schedule_, did_complete_);
+  DCHECK(!did_run_ || did_schedule_);
+  DCHECK(!did_run_ || did_complete_);
+}
+
+void RasterWorkerPoolTask::DispatchCompletionCallback() {
+  DCHECK(did_schedule_);
+  DCHECK(!did_complete_);
+  did_complete_ = true;
+  reply_.Run(!did_run_);
+}
+
+void RasterWorkerPoolTask::DidSchedule() {
+  DCHECK(!did_complete_);
+  did_schedule_ = true;
+}
+
+void RasterWorkerPoolTask::WillRun() {
+  DCHECK(!did_complete_);
+  DCHECK(!did_run_);
+}
+
+void RasterWorkerPoolTask::DidRun() {
+  did_run_ = true;
+}
+
+}  // namespace internal
+
+RasterWorkerPool::Task::Queue::Queue() {
+}
+
+RasterWorkerPool::Task::Queue::~Queue() {
+}
+
+void RasterWorkerPool::Task::Queue::Append(const Task& task) {
+  DCHECK(!task.is_null());
+  DCHECK(std::find(tasks_.begin(),
+                   tasks_.end(),
+                   task.internal_) == tasks_.end());
+  tasks_.push_back(task.internal_);
+}
+
+RasterWorkerPool::Task::Task() {
+}
+
+RasterWorkerPool::Task::Task(const base::Closure& callback,
+                             const Reply& reply)
+    : internal_(new RasterWorkerPoolTaskImpl(callback, reply)) {
+}
+
+RasterWorkerPool::Task::Task(
+    scoped_refptr<internal::RasterWorkerPoolTask> internal)
+    : internal_(internal) {
+}
+
+RasterWorkerPool::Task::~Task() {
+}
+
+void RasterWorkerPool::Task::Reset() {
+  internal_ = NULL;
+}
+
+RasterWorkerPool::PictureTask::PictureTask(PicturePileImpl* picture_pile,
+                                           const Callback& callback,
+                                           const Reply& reply)
+    : RasterWorkerPool::Task(new RasterWorkerPoolPictureTaskImpl(picture_pile,
+                                                                 callback,
+                                                                 reply)) {
+}
+
 RasterWorkerPool::RasterWorkerPool(
     WorkerPoolClient* client, size_t num_threads) : WorkerPool(
         client,
@@ -47,13 +191,19 @@ RasterWorkerPool::RasterWorkerPool(
 RasterWorkerPool::~RasterWorkerPool() {
 }
 
-void RasterWorkerPool::PostRasterTaskAndReply(PicturePileImpl* picture_pile,
-                                              const RasterCallback& task,
-                                              const base::Closure& reply) {
-  PostTask(make_scoped_ptr(new RasterWorkerPoolTaskImpl(
-                               picture_pile,
-                               task,
-                               reply)).PassAs<internal::WorkerPoolTask>());
+void RasterWorkerPool::ScheduleTasks(Task::Queue* queue) {
+  // Note: DidSchedule() is only used for DCHECKs.

[vmpstr, 2013/05/06 23:08:48]

If we're only doing these for DCHECKs, is it worth to put a TODO to remove this
extra loop eventually?

[reveman, 2013/05/07 01:33:54]

we could #if !defined(NDEBUG) all this stuff but at this point I don't think
it's even worth a TODO. If we at a later point need to do some micro
optimizations of this code the current comment should be enough to make it clear
that this can easily be improved.

+  for (Task::Queue::Deque::iterator it = queue->tasks_.begin();
+       it != queue->tasks_.end();
+       ++it) {
+    internal::RasterWorkerPoolTask* task = *it;
+    task->DidSchedule();
+  }
+
+  WorkerPool::ScheduleTasks(
+      make_scoped_ptr(
+          new RasterWorkerPoolTaskQueueImpl(
+              &queue->tasks_)).PassAs<internal::WorkerPoolTaskGraph>());
 }
 
 }  // namespace cc