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..7ead735a11254658d4ea77c840947b8ed1b61d43 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));
+    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();
+  }

[vmpstr, 2013/05/06 18:05:51]

nit: I kinda prefer blank lines between these functions for readability

[reveman, 2013/05/06 21:36:27]

I avoid the use of a blank line here to make it clear that all these functions
are part of the "Overridden from internal::WorkerPoolTaskGraph" group. Typically
that's done in a header and not using a blank line there is cleaner. I prefer to
keep it as is but could avoid inlining of these functions completely if you
think it's too hard to read.

+  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);

[vmpstr, 2013/05/06 18:05:51]

can you DCHECK that it != tasks_.end. It would seem weird if you call TakeTask
with a task that is not in tasks_

[reveman, 2013/05/06 21:36:27]

I allowed task to not always be in tasks_ for efficient (avoids one extra scan
through all tasks) but it's probably better to take that cost in favor of
non-awkwardness. Changed this in latest patch. Making HasTask() use a hash will
fix the efficiency problem if necessary.

Fyi, WorkerPool::Inner::ScheduleTasks() has to remove tasks from a graph without
knowing in advance that they exist.

+    if (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.
+  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