cc: Move internal::Task::did_schedule_ to internal::WorkerPoolTask.

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"
@@ skipping 438 matching lines @@
     LAZY_INSTANCE_INITIALIZER;
 
 const int kDefaultNumRasterThreads = 1;
 
 int g_num_raster_threads = 0;
 
 }  // namespace
 
 namespace internal {
 
-WorkerPoolTask::WorkerPoolTask() : did_complete_(false) {}
+WorkerPoolTask::WorkerPoolTask() : did_schedule_(false), did_complete_(false) {}
 
 WorkerPoolTask::~WorkerPoolTask() {
   DCHECK_EQ(did_schedule_, did_complete_);
   DCHECK(!did_run_ || did_complete_);
 }
 
+void WorkerPoolTask::WillSchedule() {}
+
+void WorkerPoolTask::DidSchedule() {
+  did_schedule_ = true;
+  did_complete_ = false;
+}
+
+bool WorkerPoolTask::HasBeenScheduled() const { return did_schedule_; }
+
 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_; }
 
@@ skipping 137 matching lines @@
 bool RasterWorkerPool::IsRasterTaskRequiredForActivation(
     internal::RasterWorkerPoolTask* task) const {
   return raster_tasks_required_for_activation_.find(task) !=
          raster_tasks_required_for_activation_.end();
 }
 
 void RasterWorkerPool::SetTaskGraph(TaskGraph* graph) {
   TRACE_EVENT1(
       "cc", "RasterWorkerPool::SetTaskGraph", "num_tasks", graph->size());
 
+  for (internal::GraphNode::Map::iterator it = graph->begin();
+       it != graph->end();
+       ++it) {
+    internal::WorkerPoolTask* task =

[vmpstr, 2014/02/05 16:56:23]

Can you do a DCHECK here to ensure that this task isn't done yet? Something
along the lines of !HasFinishedRunning or !HasBeenScheduled.

My concern is that will/did schedule indicates that this task will eventually
run, is it possible for the task graph runner to not do that for whatever
reason?

[reveman, 2014/02/05 18:00:00]

The task might have finished running. We can't know that here. Schedule can be
called multiple times before CompleteOnOriginThread is called. Maybe we should
reset did_schedule_ in DidComplete() and only call Schedule here if
!HasBeenScheduled. That would make life a bit easier for the task
implementation. Wdyt?
It could be canceled if the next call to SetTaskGraph doesn't include this task.

[vmpstr, 2014/02/05 18:07:42]

I think that makes sense. From the task perspective then it's scheduled once
and it will just keep running until it completes. The fact that it is scheduled
multiple times should be opaque to the task. I mean if it is scheduled once
and run multiple times should be the same as if it goes through multiple
schedule/run cycles from the task perspective.

[reveman, 2014/02/05 19:52:42]

Take a look at latest patch.

+        static_cast<internal::WorkerPoolTask*>(it->first);
+
+    task->WillSchedule();
+    task->ScheduleOnOriginThread(this);
+    task->DidSchedule();
+  }
+
   g_task_graph_runner.Pointer()->SetTaskGraph(namespace_token_, graph);
 }
 
 void RasterWorkerPool::CollectCompletedWorkerPoolTasks(
     internal::Task::Vector* completed_tasks) {
   g_task_graph_runner.Pointer()->CollectCompletedTasks(namespace_token_,
                                                        completed_tasks);
 }
 
 void RasterWorkerPool::RunGpuRasterTasks(const RasterTaskVector& tasks) {
   if (tasks.empty())
     return;
 
   GrContext* gr_context = context_provider_->GrContext();
   // TODO(alokp): Implement TestContextProvider::GrContext().
   if (gr_context)
     gr_context->resetContext();
 
   for (RasterTaskVector::const_iterator it = tasks.begin(); it != tasks.end();
        ++it) {
     internal::RasterWorkerPoolTask* task = it->get();
     DCHECK(task->use_gpu_rasterization());
 
+    task->WillSchedule();

[vmpstr, 2014/02/05 16:56:23]

Does this also need a ScheduleOnOriginThread? There's a DCHECK in the task that
it's not a gpu rasterization task, but I'm wondering whether that DCHECK should
be an early out instead? I mean it kind of makes sense to me that all tasks do a
WillSchedule/Schedule/DidSchedule cycle instead of special casing GPU tasks to
not Schedule. Wdyt?

I was also thinking of having two ScheduleOnOriginThreadForWorkerThread and
ScheduleOnOriginThreadForOriginThread kind of thing, but I think that's
overkill.

[reveman, 2014/02/05 18:00:00]

I agree. Did the same for CompleteOnOriginThread().

[vmpstr, 2014/02/05 18:07:42]

Thanks! I think it looks nice now (ie symmetrical with the way other tasks are
run).

     task->DidSchedule();
     task->WillRun();
     task->RunOnOriginThread(resource_provider_, context_provider_);
     task->DidRun();
     task->WillComplete();
     task->DidComplete();
 
     completed_gpu_raster_tasks_.push_back(task);
   }
 
@@ skipping 70 matching lines @@
   graph->set(task, make_scoped_ptr(node));
   return node;
 }
 
 // static
 internal::GraphNode* RasterWorkerPool::CreateGraphNodeForRasterTask(
     internal::WorkerPoolTask* raster_task,
     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 (internal::Task::Vector::const_iterator it = decode_tasks.begin();
        it != decode_tasks.end();
        ++it) {
     internal::WorkerPoolTask* decode_task =
         static_cast<internal::WorkerPoolTask*>(it->get());
 
@@ skipping 13 matching lines @@
 
     internal::GraphNode* decode_node =
         CreateGraphNodeForTask(decode_task, priority, graph);
     decode_node->add_dependent(raster_node);
   }
 
   return raster_node;
 }
 
 }  // namespace cc