Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1211153002:
Expose a SequencedTaskRunner that runs tasks on raster threads. (Closed)
|
Created:
5 years, 6 months ago by Daniele Castagna Modified:
5 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionExpose a SequencedTaskRunner that runs tasks on raster threads.
This is the first step to make the raster worker threads reusable
within the render.
The raster threads and the task graph runner have been moved in
a RasterWorkerPool class that implements the SequencedTaskRunner
interface.
The long term plan is to have a better defined TaskGraphRunner
interface and expose that one instead.
BUG=
Committed: https://crrev.com/b880e8fd28fa5ff83ceceeeeccba993e1c159929
Cr-Commit-Position: refs/heads/master@{#336862}
Patch Set 1 #
Total comments: 29
Patch Set 2 : Move TaskGraphRunner and Threads in RasterWorkerPool. #
Total comments: 20
Patch Set 3 : Address reveman's comments. #
Total comments: 8
Patch Set 4 : Move RasterWorkerPool away from the anonymous namespace. #Patch Set 5 : Address reveman's nits. #Messages
Total messages: 15 (4 generated)
dcastagna@chromium.org changed reviewers: + reveman@chromium.org
https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:764: DCHECK_IMPLIES(worker_task_runner_, worker_task_runner_->HasOneRef()); 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. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1796: namespace { please move this to anonymous namespace at the top of the file instead https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1799: class TaskGraphRunnerClosureTask : public cc::Task { Can we make this a private subclass of the class that implements SequencedTaskRunner? https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1803: void RunOnWorkerThread() override { closure_.Run(); }; 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. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1816: TaskGraphRunnerSequencedTaskRunner(cc::TaskGraphRunner* task_graph_runner) explicit https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1819: bool PostDelayedTask(const tracked_objects::Location& from_here, please add a "// Overridden from base::TaskRunner:" comment here and put RunsTasksOnCurrentThread just below this function without a blank line https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1827: bool PostNonNestableDelayedTask(const tracked_objects::Location& from_here, // Overridden from base::SequencedTaskRunner: https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1829: base::TimeDelta delay) override { This function is currently not thread-safe but it needs to be. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1832: TaskGraphRunnerClosureTask::Vector completed_tasks; Maybe make this a member variable to avoid unnecessary heap allocations each time PostTask is called https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1835: auto new_end = std::remove_if( 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? https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1844: // Rebuild graph_. not sure how useful this comment is https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1846: for (const auto& t : tasks_) { nit: s/t/task/ https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1847: const auto& node = cc::TaskGraph::Node(t.get(), 0, graph_.nodes.size()); nit: "cc::TaskGraph::Node node(t.get(), 0, graph_.nodes.size())" https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1862: ~TaskGraphRunnerSequencedTaskRunner() override{}; s/override{};/override {}/
PTAL. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:764: DCHECK_IMPLIES(worker_task_runner_, worker_task_runner_->HasOneRef()); On 2015/06/26 at 21:13:56, reveman wrote: > 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. Done. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1796: namespace { On 2015/06/26 at 21:13:56, reveman wrote: > please move this to anonymous namespace at the top of the file instead Done. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1799: class TaskGraphRunnerClosureTask : public cc::Task { On 2015/06/26 at 21:13:56, reveman wrote: > Can we make this a private subclass of the class that implements SequencedTaskRunner? Sure. Also made CompositorRasterThread private since it's used only by RasterWorkerPool now. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1803: void RunOnWorkerThread() override { closure_.Run(); }; On 2015/06/26 at 21:13:56, reveman wrote: > 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. For the SequencedTaskRunner returned by the SequencedWorkerPool it seems tasks are deleted in the worker thread (SequencedWorkerPool::Inner::ThreadLoop). Added closure_.Reset() here. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1816: TaskGraphRunnerSequencedTaskRunner(cc::TaskGraphRunner* task_graph_runner) On 2015/06/26 at 21:13:56, reveman wrote: > explicit This class owns TaskGraphRunner now, so the parameter is gone. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1819: bool PostDelayedTask(const tracked_objects::Location& from_here, On 2015/06/26 at 21:13:57, reveman wrote: > please add a "// Overridden from base::TaskRunner:" comment here and put RunsTasksOnCurrentThread just below this function without a blank line Done. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1827: bool PostNonNestableDelayedTask(const tracked_objects::Location& from_here, On 2015/06/26 at 21:13:56, reveman wrote: > // Overridden from base::SequencedTaskRunner: Done. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1829: base::TimeDelta delay) override { On 2015/06/26 at 21:13:56, reveman wrote: > This function is currently not thread-safe but it needs to be. Done. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1832: TaskGraphRunnerClosureTask::Vector completed_tasks; On 2015/06/26 at 21:13:56, reveman wrote: > Maybe make this a member variable to avoid unnecessary heap allocations each time PostTask is called Done. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1835: auto new_end = std::remove_if( On 2015/06/26 at 21:13:57, reveman wrote: > 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? Added the DCHECKs. Using erase to avoid the quadratic behavior. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1844: // Rebuild graph_. On 2015/06/26 at 21:13:56, reveman wrote: > not sure how useful this comment is Removed. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1846: for (const auto& t : tasks_) { On 2015/06/26 at 21:13:57, reveman wrote: > nit: s/t/task/ Done. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1847: const auto& node = cc::TaskGraph::Node(t.get(), 0, graph_.nodes.size()); On 2015/06/26 at 21:13:57, reveman wrote: > nit: "cc::TaskGraph::Node node(t.get(), 0, graph_.nodes.size())" Done. https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1862: ~TaskGraphRunnerSequencedTaskRunner() override{}; On 2015/06/26 at 21:13:56, reveman wrote: > s/override{};/override {}/ "git cl format" seems to disagree. :/
https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1211153002/diff/1/content/renderer/render_thr... content/renderer/render_thread_impl.cc:1862: ~TaskGraphRunnerSequencedTaskRunner() override{}; On 2015/06/29 at 23:30:49, Daniele Castagna wrote: > On 2015/06/26 at 21:13:56, reveman wrote: > > s/override{};/override {}/ > > "git cl format" seems to disagree. :/ because of the ";"? https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:382: RasterWorkerPool() : namespace_token_(){}; ; after function definition? https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:385: const base::SimpleThread::Options& thread_options) { Can we create whatever is possible in ctor and just start the threads when Start is called? So that in ctor; we create stuff, Start; starts processing tasks, Shutdown stops processing tasks, dtor destroys everything. https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:463: class TaskGraphRunnerClosureTask : public cc::Task { "ClosureTask" sufficient name? https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:482: class RasterThread : public base::SimpleThread { can we use DelegateSimpleThread instead and have RasterWorkerPool implement Run? https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:501: scoped_ptr<cc::TaskGraphRunner> task_graph_runner_; Can this just be "cc::TaskGraphRunner task_graph_runner_"? https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:503: // Lock for the SequencedTaskRunner implementation state. what is that state? everything below? please make that more clear. https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:509: // Current graph set for scheduling. not really true, is it? this is used to avoid unnecessary heap allocations, right? https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:792: raster_worker_pool_ = make_scoped_refptr(new RasterWorkerPool()); move this to ctor? https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:873: raster_worker_pool_ = nullptr; We need Shutdown here but maybe just let the dtor take care of dropping ref now that this is ref-counted https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.h:326: scoped_refptr<base::SequencedTaskRunner> GetWorkerSequencedTaskRunner(); s/scoped_refptr<base::SequencedTaskRunner>/base::SequencedTaskRunner*/
https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:382: RasterWorkerPool() : namespace_token_(){}; On 2015/06/30 at 04:47:52, reveman wrote: > ; after function definition? Done. https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:385: const base::SimpleThread::Options& thread_options) { On 2015/06/30 at 04:47:51, reveman wrote: > Can we create whatever is possible in ctor and just start the threads when Start is called? So that in ctor; we create stuff, Start; starts processing tasks, Shutdown stops processing tasks, dtor destroys everything. Done. https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:463: class TaskGraphRunnerClosureTask : public cc::Task { On 2015/06/30 at 04:47:52, reveman wrote: > "ClosureTask" sufficient name? Done. https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:482: class RasterThread : public base::SimpleThread { On 2015/06/30 at 04:47:51, reveman wrote: > can we use DelegateSimpleThread instead and have RasterWorkerPool implement Run? Done. It'd be nicer if DelegateSimpleThread took a closure. I think having RasterWorkerPool implement DelegateSimpleThread::Delegate is misleading, but convenient. https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:501: scoped_ptr<cc::TaskGraphRunner> task_graph_runner_; On 2015/06/30 at 04:47:52, reveman wrote: > Can this just be "cc::TaskGraphRunner task_graph_runner_"? Done. https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:503: // Lock for the SequencedTaskRunner implementation state. On 2015/06/30 at 04:47:52, reveman wrote: > what is that state? everything below? please make that more clear. Done. https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:509: // Current graph set for scheduling. On 2015/06/30 at 04:47:52, reveman wrote: > not really true, is it? this is used to avoid unnecessary heap allocations, right? This is the last graph scheduled with ::ScheduleTasks, changing the comment since I didn't notice that the object is modified after that call. https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:792: raster_worker_pool_ = make_scoped_refptr(new RasterWorkerPool()); On 2015/06/30 at 04:47:51, reveman wrote: > move this to ctor? Done. https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.cc:873: raster_worker_pool_ = nullptr; On 2015/06/30 at 04:47:52, reveman wrote: > We need Shutdown here but maybe just let the dtor take care of dropping ref now that this is ref-counted Done. https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... File content/renderer/render_thread_impl.h (right): https://codereview.chromium.org/1211153002/diff/20001/content/renderer/render... content/renderer/render_thread_impl.h:326: scoped_refptr<base::SequencedTaskRunner> GetWorkerSequencedTaskRunner(); On 2015/06/30 at 04:47:52, reveman wrote: > s/scoped_refptr<base::SequencedTaskRunner>/base::SequencedTaskRunner*/ Done.
lgtm with nits we also need a TODO somewhere with a crbug that explains the long term plan and links to the design doc. https://codereview.chromium.org/1211153002/diff/40001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1211153002/diff/40001/content/renderer/render... content/renderer/render_thread_impl.cc:417: // Overridden from base::TaskRunner: nit: remove line 417 and 416 so these functions become a group of functions affected by the comment on line 410 https://codereview.chromium.org/1211153002/diff/40001/content/renderer/render... content/renderer/render_thread_impl.cc:466: void RunOnWorkerThread() override { nit: blank line after ctor and add: // Overridden from cc:Task: https://codereview.chromium.org/1211153002/diff/40001/content/renderer/render... content/renderer/render_thread_impl.cc:481: ScopedVector<base::DelegateSimpleThread> raster_threads_; nit: just "threads_" as "raster" is implicit from the class name and it would make a future rasterworkerpool->workerpool transition cleaner. https://codereview.chromium.org/1211153002/diff/40001/content/renderer/render... content/renderer/render_thread_impl.cc:494: cc::NamespaceToken namespace_token_; nit: can you move this above the lock_ and make it const?
dcastagna@chromium.org changed reviewers: + avi@chromium.org
On 2015/06/30 at 18:03:53, reveman wrote: > lgtm with nits > > we also need a TODO somewhere with a crbug that explains the long term plan and links to the design doc. > Added a TODO before RasterWorkerPool declaration. Please note that the class is now in the 'content' namespace since it wouldn't compile in the unnamed one on android. Adding avi@ for OWNERS review. https://codereview.chromium.org/1211153002/diff/40001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/1211153002/diff/40001/content/renderer/render... content/renderer/render_thread_impl.cc:417: // Overridden from base::TaskRunner: On 2015/06/30 at 18:03:52, reveman wrote: > nit: remove line 417 and 416 so these functions become a group of functions affected by the comment on line 410 Done. https://codereview.chromium.org/1211153002/diff/40001/content/renderer/render... content/renderer/render_thread_impl.cc:466: void RunOnWorkerThread() override { On 2015/06/30 at 18:03:52, reveman wrote: > nit: blank line after ctor and add: // Overridden from cc:Task: Done. https://codereview.chromium.org/1211153002/diff/40001/content/renderer/render... content/renderer/render_thread_impl.cc:481: ScopedVector<base::DelegateSimpleThread> raster_threads_; On 2015/06/30 at 18:03:52, reveman wrote: > nit: just "threads_" as "raster" is implicit from the class name and it would make a future rasterworkerpool->workerpool transition cleaner. Done. https://codereview.chromium.org/1211153002/diff/40001/content/renderer/render... content/renderer/render_thread_impl.cc:494: cc::NamespaceToken namespace_token_; On 2015/06/30 at 18:03:52, reveman wrote: > nit: can you move this above the lock_ and make it const? Done.
lgtm
The CQ bit was checked by dcastagna@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from reveman@chromium.org Link to the patchset: https://codereview.chromium.org/1211153002/#ps80001 (title: "Address reveman's nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1211153002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/b880e8fd28fa5ff83ceceeeeccba993e1c159929 Cr-Commit-Position: refs/heads/master@{#336862} |
