Expose a SequencedTaskRunner that runs tasks on raster threads.

content/renderer/render_thread_impl.cc

 // Copyright (c) 2012 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 "content/renderer/render_thread_impl.h"
 
 #include <algorithm>
 #include <limits>
 #include <map>
 #include <vector>
@@ skipping 743 matching lines @@
   }
 
   media_thread_.reset();
 
   // AudioMessageFilter may be accessed on |media_thread_|, so shutdown after.
   RemoveFilter(audio_message_filter_.get());
   audio_message_filter_ = NULL;
 
   compositor_thread_.reset();
 
+  DCHECK_IMPLIES(worker_task_runner_, worker_task_runner_->HasOneRef());

[reveman, 2015/06/26 21:13:56]

I think we can remove this if we add a RasterWorkerPool class that implements
the TaskRunner interface and owns the threads and taskgraphrunner instance. That
would also align better with the plan to make TaskGraphRunner an interface like
TaskRunner as the RasterWorkerPool class would then also implement this
interface.

[Daniele Castagna, 2015/06/29 23:30:49]

Done.

+  worker_task_runner_ = nullptr;
+
   // Shutdown raster threads.
   compositor_task_graph_runner_->Shutdown();
   while (!compositor_raster_threads_.empty()) {
     compositor_raster_threads_.back()->Join();
     compositor_raster_threads_.pop_back();
   }
   compositor_task_graph_runner_.reset();
 
   main_input_callback_.Cancel();
   input_handler_manager_.reset();
@@ skipping 1009 matching lines @@
     media_thread_->Start();
 
 #if defined(OS_ANDROID)
     renderer_demuxer_ = new RendererDemuxerAndroid();
     AddFilter(renderer_demuxer_.get());
 #endif
   }
   return media_thread_->task_runner();
 }
 
+namespace {

[reveman, 2015/06/26 21:13:56]

please move this to anonymous namespace at the top of the file instead

[Daniele Castagna, 2015/06/29 23:30:49]

Done.

+
+// Simple Task for the TaskGraphRunner that wraps a closure.
+class TaskGraphRunnerClosureTask : public cc::Task {

[reveman, 2015/06/26 21:13:56]

Can we make this a private subclass of the class that implements
SequencedTaskRunner?

[Daniele Castagna, 2015/06/29 23:30:49]

Sure.

Also made CompositorRasterThread private since it's used only by
RasterWorkerPool now.

+ public:
+  TaskGraphRunnerClosureTask(const base::Closure& closure)
+      : closure_(closure) {}
+  void RunOnWorkerThread() override { closure_.Run(); };

[reveman, 2015/06/26 21:13:56]

should we drop the closure_ reference here on the worker thread or keep it
around and drop it on the thread posting tasks? Let's do what other
SequencedTaskRunner implementations does to avoid any issues related to this.

[Daniele Castagna, 2015/06/29 23:30:49]

For the SequencedTaskRunner returned by the SequencedWorkerPool it seems tasks
are deleted in the worker thread (SequencedWorkerPool::Inner::ThreadLoop).

Added closure_.Reset() here.

+
+ protected:
+  ~TaskGraphRunnerClosureTask() override {}
+
+ private:
+  const base::Closure closure_;
+};
+
+// SequencedTaskRunner implementation that runs tasks on
+// |compositor_raster_threads_| using |task_graph_runner_|.
+class TaskGraphRunnerSequencedTaskRunner : public base::SequencedTaskRunner {
+ public:
+  TaskGraphRunnerSequencedTaskRunner(cc::TaskGraphRunner* task_graph_runner)

[reveman, 2015/06/26 21:13:56]

explicit

[Daniele Castagna, 2015/06/29 23:30:49]

This class owns TaskGraphRunner now, so the parameter is gone.

+      : task_graph_runner_(task_graph_runner),
+        namespace_token_(task_graph_runner->GetNamespaceToken()) {}
+  bool PostDelayedTask(const tracked_objects::Location& from_here,

[reveman, 2015/06/26 21:13:57]

please add a "// Overridden from base::TaskRunner:" comment here and put
RunsTasksOnCurrentThread just below this function without a blank line

[Daniele Castagna, 2015/06/29 23:30:49]

Done.

+                       const base::Closure& task,
+                       base::TimeDelta delay) override {
+    DCHECK(task_graph_runner_);
+    PostNonNestableDelayedTask(from_here, task, delay);
+    return true;
+  }
+
+  bool PostNonNestableDelayedTask(const tracked_objects::Location& from_here,

[reveman, 2015/06/26 21:13:56]

// Overridden from base::SequencedTaskRunner:

[Daniele Castagna, 2015/06/29 23:30:49]

Done.

+                                  const base::Closure& task,
+                                  base::TimeDelta delay) override {

[reveman, 2015/06/26 21:13:56]

This function is currently not thread-safe but it needs to be.

[Daniele Castagna, 2015/06/29 23:30:49]

Done.

+    DCHECK(task_graph_runner_);
+    // Remove completed tasks.
+    TaskGraphRunnerClosureTask::Vector completed_tasks;

[reveman, 2015/06/26 21:13:56]

Maybe make this a member variable to avoid unnecessary heap allocations each
time PostTask is called

[Daniele Castagna, 2015/06/29 23:30:49]

Done.

+    task_graph_runner_->CollectCompletedTasks(namespace_token_,
+                                              &completed_tasks);
+    auto new_end = std::remove_if(

[reveman, 2015/06/26 21:13:57]

as tasks are required to complete in a specific order, can you instead just do
pop_front and add a DCHECK to verify the order?

[Daniele Castagna, 2015/06/29 23:30:49]

Added the DCHECKs.
Using erase to avoid the quadratic behavior.

+        tasks_.begin(), tasks_.end(),
+        [&completed_tasks](const scoped_refptr<cc::Task>& t) {
+          return std::find(completed_tasks.begin(), completed_tasks.end(), t) !=
+                 completed_tasks.end();
+        });
+    tasks_.erase(new_end, tasks_.end());
+    tasks_.push_back(make_scoped_refptr(new TaskGraphRunnerClosureTask(task)));
+
+    // Rebuild graph_.

[reveman, 2015/06/26 21:13:56]

not sure how useful this comment is

[Daniele Castagna, 2015/06/29 23:30:49]

Removed.

+    graph_.Reset();
+    for (const auto& t : tasks_) {

[reveman, 2015/06/26 21:13:57]

nit: s/t/task/

[Daniele Castagna, 2015/06/29 23:30:49]

Done.

+      const auto& node = cc::TaskGraph::Node(t.get(), 0, graph_.nodes.size());

[reveman, 2015/06/26 21:13:57]

nit: "cc::TaskGraph::Node node(t.get(), 0, graph_.nodes.size())"

[Daniele Castagna, 2015/06/29 23:30:49]

Done.

+      if (graph_.nodes.size()) {
+        graph_.edges.push_back(
+            cc::TaskGraph::Edge(graph_.nodes.back().task, node.task));
+      }
+      graph_.nodes.push_back(node);
+    }
+
+    task_graph_runner_->ScheduleTasks(namespace_token_, &graph_);
+    return true;
+  }
+
+  bool RunsTasksOnCurrentThread() const override { return true; }
+
+ protected:
+  ~TaskGraphRunnerSequencedTaskRunner() override{};

[reveman, 2015/06/26 21:13:56]

s/override{};/override {}/

[Daniele Castagna, 2015/06/29 23:30:49]

"git cl format" seems to disagree. :/

[reveman, 2015/06/30 04:47:51]

because of the ";"?

+
+ private:
+  TaskGraphRunnerClosureTask::Vector tasks_;
+  cc::TaskGraph graph_;
+  cc::TaskGraphRunner* task_graph_runner_;
+  const cc::NamespaceToken namespace_token_;
+};
+}
+
+scoped_refptr<base::SequencedTaskRunner>
+RenderThreadImpl::GetWorkerSequencedTaskRunner() {
+  if (!worker_task_runner_) {
+    worker_task_runner_ = make_scoped_refptr(
+        new content::TaskGraphRunnerSequencedTaskRunner(GetTaskGraphRunner()));
+  }
+  return worker_task_runner_;
+}
+
 void RenderThreadImpl::SampleGamepads(blink::WebGamepads* data) {
   blink_platform_impl_->sampleGamepads(*data);
 }
 
 bool RenderThreadImpl::RendererIsHidden() const {
   return widget_count_ > 0 && hidden_widget_count_ == widget_count_;
 }
 
 void RenderThreadImpl::WidgetCreated() {
   bool renderer_was_hidden = RendererIsHidden();
@@ skipping 70 matching lines @@
 }
 
 void RenderThreadImpl::PendingRenderFrameConnect::OnConnectionError() {
   size_t erased =
       RenderThreadImpl::current()->pending_render_frame_connects_.erase(
           routing_id_);
   DCHECK_EQ(1u, erased);
 }
 
 }  // namespace content