Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1008693004:
Handle delayed tasks more natively in the scheduler (Closed)
|
Created:
5 years, 9 months ago by alex clarke (OOO till 29th) Modified:
5 years, 9 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, cc-bugs_chromium.org, scheduler-bugs_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThe TaskQueueManager's internal::TaskQueue now has a priority
queue of delayed tasks. When these tasks are ready to
execute, they are enqueued on the normal task queue.
The main motivation for doing this is so that delayed
tasks posted from Blink can be properly flushed when the
scheduler shuts down. This is important because we would
otherwise risk leaving tasks allocated on the Blink Heap
that has gone away.
Benchmarking suggests that the overhead of this approach is
actually a bit lower than posting to the base MessageLoop
when posting lots of delayed tasks.
BUG=463143
Committed: https://crrev.com/546ca22ac2fd1bfddcb674813c7777041508dc27
Cr-Commit-Position: refs/heads/master@{#321183}
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Sami's Suggestions #
Total comments: 24
Patch Set 4 : A second round of feedback. #Patch Set 5 : Work around problem with non-auto pump queues and delayed tasks #
Total comments: 15
Patch Set 6 : Responding to yet more feedback. Also removed some boilerplate calls to AppendQueueToService #
Total comments: 13
Patch Set 7 : Responding to feedback again #
Total comments: 3
Patch Set 8 : Fixed the problem with the lack of std::min and added a test. #Patch Set 9 : fix compile issue #Patch Set 10 : Fix bug with AutomaticSelectorForTest and multiple queues #
Messages
Total messages: 36 (11 generated)
alexclarke@chromium.org changed reviewers: + brianderson@chromium.org
alexclarke@chromium.org changed reviewers: + skyostil@chromium.org
+brianderson@ for the cc/test/ordered_simple_task_runner.h &.cc change +sami for the other bits
lgtm!
Sami I've followed the suggestions from https://codereview.chromium.org/962273002/ in here.
Thanks Alex. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:24: class LazyNow { Neat helper! https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:93: // Delayed task posted to the chromium run loop, which calls nit: Replace "chromium" with "base" or "underlying" since this is all in chromium anyway. Also s/timers/tasks/ (here and below). https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:104: bool EnqueueReadyDelayedTasksLocked(LazyNow* lazy_now); We should come up with a better name for this because it's too easy to confuse with EnqueueTaskLocked which puts stuff on the incoming queue as opposed to the work queue. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:148: // Clear all delayed tasks because we need them to be deleted before the blink TQM shouldn't really know anything about blink so I'd remove this comment. By the way, should we be clearing the incoming and work task queues here too? https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:150: while (!delayed_task_queue_.empty()) nit: would delayed_task_queue_ = base::DelayedTaskQueue() work? Probably no reason to do this in order. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:187: lock_.AssertAcquired(); Could you DCHECK that we're on the main thread here? https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:196: task.delayed_run_time = base::TimeTicks(); Could you add a TODO here about not clearing this so that the selector can estimate delayed task starvation? https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:207: task_enqueued = EnqueueReadyDelayedTasksLocked(lazy_now); We can't call this here because the work queue is owned by the main thread. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:227: bool task_enqueued; DCHECK main thread. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:230: LazyNow lazy_now(task_queue_manager_); task_queue_manager_ might be deleted by now. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:315: TRACE_COUNTER1(TRACE_DISABLED_BY_DEFAULT("renderer.scheduler"), name_, Could you add the delayed task queue here too? https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:375: state->BeginArray("work_queue"); Could you dump the delayed task queue here too? Also might be good to record how many delayed task runners we have posted.
skyostil@chromium.org changed reviewers: + rmcilroy@chromium.org
https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:24: class LazyNow { On 2015/03/17 15:54:45, Sami wrote: > Neat helper! Acknowledged. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:93: // Delayed task posted to the chromium run loop, which calls On 2015/03/17 15:54:44, Sami wrote: > nit: Replace "chromium" with "base" or "underlying" since this is all in > chromium anyway. Also s/timers/tasks/ (here and below). Done. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:104: bool EnqueueReadyDelayedTasksLocked(LazyNow* lazy_now); On 2015/03/17 15:54:45, Sami wrote: > We should come up with a better name for this because it's too easy to confuse > with EnqueueTaskLocked which puts stuff on the incoming queue as opposed to the > work queue. How about MoveReadyDelayedTasksToWorkQueueLocked? https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:148: // Clear all delayed tasks because we need them to be deleted before the blink On 2015/03/17 15:54:44, Sami wrote: > TQM shouldn't really know anything about blink so I'd remove this comment. > > By the way, should we be clearing the incoming and work task queues here too? Probably no since the existing implementations of base::MessageLoop try to run any remaining tasks (up to a limit of 100 tasks iirc). https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:150: while (!delayed_task_queue_.empty()) On 2015/03/17 15:54:45, Sami wrote: > nit: would delayed_task_queue_ = base::DelayedTaskQueue() work? Probably no > reason to do this in order. Done. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:187: lock_.AssertAcquired(); On 2015/03/17 15:54:45, Sami wrote: > Could you DCHECK that we're on the main thread here? Done. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:196: task.delayed_run_time = base::TimeTicks(); On 2015/03/17 15:54:45, Sami wrote: > Could you add a TODO here about not clearing this so that the selector can > estimate delayed task starvation? Done. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:207: task_enqueued = EnqueueReadyDelayedTasksLocked(lazy_now); On 2015/03/17 15:54:45, Sami wrote: > We can't call this here because the work queue is owned by the main thread. Done. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:227: bool task_enqueued; On 2015/03/17 15:54:45, Sami wrote: > DCHECK main thread. Done. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:230: LazyNow lazy_now(task_queue_manager_); On 2015/03/17 15:54:45, Sami wrote: > task_queue_manager_ might be deleted by now. Done. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:315: TRACE_COUNTER1(TRACE_DISABLED_BY_DEFAULT("renderer.scheduler"), name_, On 2015/03/17 15:54:45, Sami wrote: > Could you add the delayed task queue here too? Done. https://codereview.chromium.org/1008693004/diff/40001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:375: state->BeginArray("work_queue"); On 2015/03/17 15:54:45, Sami wrote: > Could you dump the delayed task queue here too? Also might be good to record how > many delayed task runners we have posted. Done.
As discussed offline I've worked around a problem with non-auto pump queues and delayed tasks. Some of my review thread comments are obsolete now.
Thanks, looking pretty good now I think. https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:104: // Enqueues any delayed tasks which should be run now on the incomming_queue_. s/incomming/incoming/ https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:151: delayed_task_queue_ = base::DelayedTaskQueue(); Re: the previous discussion about the MessageLoop, we are purposefully discarding all the tasks and not running them like MessageLoop does to ensure anything with a pointer to Blink goes away. It's probably a better task for another patch, but I think we should be clearing all the queues here. Mind adding a TODO here? https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:192: delayed_task_queue_.top().delayed_run_time); Did we lose the code that clears the delayed_run_time here? https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:200: MoveReadyDelayedTasksToIncomingQueueLocked(lazy_now); Would you mind moving this call to KickDelayedTasks for now? It feels weird to penalize the posting thread to do this housekeeping work for us, and I'm not sure it buys us all that much because we have to go through DoWork to run the tasks anyway -- might as well have the migration happen there. https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:280: MoveReadyDelayedTasksToIncomingQueueLocked(lazy_now); Should this happen after the mode check below? It doesn't seem useful to do it if pumping is disabled. https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... File content/renderer/scheduler/task_queue_manager.h (right): https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.h:135: // Delayed Tasks with run_times <= Now() are enqueued onto their work queues. s/their work queues/the work queue/?
This looks pretty good to me. I was originally a bit worried about delayed tasks on after-wakeup queues but after looking through I don't think they will be affected (feel free to grab me for a chat if you think otherwise). https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:218: void TaskQueue::KickDelayedTasks() { Naming nit - could we just call this ScheduleDelayedWork (or call the one above KickDelayedWorkLocked) for clarity?
This looks pretty good to me. I was originally a bit worried about delayed tasks on after-wakeup queues but after looking through I don't think they will be affected (feel free to grab me for a chat if you think otherwise).
https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:104: // Enqueues any delayed tasks which should be run now on the incomming_queue_. On 2015/03/17 18:40:02, Sami wrote: > s/incomming/incoming/ Done. https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:151: delayed_task_queue_ = base::DelayedTaskQueue(); On 2015/03/17 18:40:03, Sami wrote: > Re: the previous discussion about the MessageLoop, we are purposefully > discarding all the tasks and not running them like MessageLoop does to ensure > anything with a pointer to Blink goes away. > > It's probably a better task for another patch, but I think we should be clearing > all the queues here. Mind adding a TODO here? Done. https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:192: delayed_task_queue_.top().delayed_run_time); On 2015/03/17 18:40:02, Sami wrote: > Did we lose the code that clears the delayed_run_time here? It's not needed. See the implementation of EnqueueTaskLocked. https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:200: MoveReadyDelayedTasksToIncomingQueueLocked(lazy_now); On 2015/03/17 18:40:03, Sami wrote: > Would you mind moving this call to KickDelayedTasks for now? It feels weird to > penalize the posting thread to do this housekeeping work for us, and I'm not > sure it buys us all that much because we have to go through DoWork to run the > tasks anyway -- might as well have the migration happen there. Done. https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:218: void TaskQueue::KickDelayedTasks() { On 2015/03/17 19:06:54, rmcilroy wrote: > Naming nit - could we just call this ScheduleDelayedWork (or call the one above > KickDelayedWorkLocked) for clarity? I agree with the sentiment that KickDelayedTasks isn't the best name. However I think MoveReadyDelayedTasksToIncomingQueue is a better name. https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:280: MoveReadyDelayedTasksToIncomingQueueLocked(lazy_now); On 2015/03/17 18:40:03, Sami wrote: > Should this happen after the mode check below? It doesn't seem useful to do it > if pumping is disabled. Done. https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... File content/renderer/scheduler/task_queue_manager.h (right): https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.h:135: // Delayed Tasks with run_times <= Now() are enqueued onto their work queues. On 2015/03/17 18:40:03, Sami wrote: > s/their work queues/the work queue/? Done.
Awesome, I think the delayed task logic is now sound. Just one more nit to pick about the testing changes. https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... content/renderer/scheduler/task_queue_manager.cc:133: base::ThreadChecker main_thread_checker_; nit: main_thread_checker isn't protected by the lock so please move it a bit further down. https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... content/renderer/scheduler/task_queue_manager.cc:177: delayed_task_queue_.push(pending_task); nit: call TraceQueueSize() here. https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... content/renderer/scheduler/task_queue_manager.cc:191: if (!task_queue_manager_) This should be checked while holding the lock. https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... content/renderer/scheduler/task_queue_manager.cc:208: } nit: call TraceQueueSize() here. https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... content/renderer/scheduler/task_queue_manager.cc:217: DCHECK(next_run_time > lazy_now->Now()); nit: DCHECK_GT https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... File content/renderer/scheduler/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... content/renderer/scheduler/task_queue_manager_unittest.cc:31: // If there's only one queue then queues_to_service_ is redundant. I'm not sure I agree: the selector can always opt not to run anything even with just one queue and we should make it possible to test that. https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... content/renderer/scheduler/task_queue_manager_unittest.cc:157: TEST_F(TaskQueueManagerTest, NowNotCalledWhenThereAreNoDelayedTasks) { Thanks, this is a great thing to test!
https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/1008693004/diff/80001/content/renderer/schedu... content/renderer/scheduler/task_queue_manager.cc:218: void TaskQueue::KickDelayedTasks() { On 2015/03/18 10:49:07, alexclarke1 wrote: > On 2015/03/17 19:06:54, rmcilroy wrote: > > Naming nit - could we just call this ScheduleDelayedWork (or call the one > above > > KickDelayedWorkLocked) for clarity? > > I agree with the sentiment that KickDelayedTasks isn't the best name. However I > think MoveReadyDelayedTasksToIncomingQueue is a better name. Works for me!
https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... content/renderer/scheduler/task_queue_manager.cc:133: base::ThreadChecker main_thread_checker_; On 2015/03/18 11:13:21, Sami wrote: > nit: main_thread_checker isn't protected by the lock so please move it a bit > further down. Done. https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... content/renderer/scheduler/task_queue_manager.cc:177: delayed_task_queue_.push(pending_task); On 2015/03/18 11:13:21, Sami wrote: > nit: call TraceQueueSize() here. Done. https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... content/renderer/scheduler/task_queue_manager.cc:191: if (!task_queue_manager_) On 2015/03/18 11:13:21, Sami wrote: > This should be checked while holding the lock. Done. https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... content/renderer/scheduler/task_queue_manager.cc:208: } On 2015/03/18 11:13:21, Sami wrote: > nit: call TraceQueueSize() here. Done. https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... content/renderer/scheduler/task_queue_manager.cc:217: DCHECK(next_run_time > lazy_now->Now()); On 2015/03/18 11:13:21, Sami wrote: > nit: DCHECK_GT Done. https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... File content/renderer/scheduler/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/1008693004/diff/100001/content/renderer/sched... content/renderer/scheduler/task_queue_manager_unittest.cc:31: // If there's only one queue then queues_to_service_ is redundant. On 2015/03/18 11:13:21, Sami wrote: > I'm not sure I agree: the selector can always opt not to run anything even with > just one queue and we should make it possible to test that. As discussed offline I split the SelectorForTest into two classes.
https://codereview.chromium.org/1008693004/diff/120001/content/renderer/sched... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/1008693004/diff/120001/content/renderer/sched... content/renderer/scheduler/task_queue_manager.cc:289: *next_pending_delayed_task = delayed_task_queue_.top().delayed_run_time; It's not obvious from the function's name, but I think this should be doing a min() with the passed-in time. https://codereview.chromium.org/1008693004/diff/120001/content/renderer/sched... File content/renderer/scheduler/task_queue_manager_unittest.cc (right): https://codereview.chromium.org/1008693004/diff/120001/content/renderer/sched... content/renderer/scheduler/task_queue_manager_unittest.cc:32: // Always selects queue 0. Thanks, this is neater.
https://codereview.chromium.org/1008693004/diff/120001/content/renderer/sched... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/1008693004/diff/120001/content/renderer/sched... content/renderer/scheduler/task_queue_manager.cc:289: *next_pending_delayed_task = delayed_task_queue_.top().delayed_run_time; On 2015/03/18 12:26:43, Sami wrote: > It's not obvious from the function's name, but I think this should be doing a > min() with the passed-in time. Did it in the caller instead since it's kind of surprising to do it here. Added a test too.
lgtm, thank you Alex!
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org Link to the patchset: https://codereview.chromium.org/1008693004/#ps140001 (title: "Fixed the problem with the lack of std::min and added a test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1008693004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_compile...)
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1008693004/#ps160001 (title: "fix compile issue")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1008693004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by alexclarke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from brianderson@chromium.org, skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/1008693004/#ps180001 (title: "Fix bug with AutomaticSelectorForTest and multiple queues")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1008693004/180001
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/546ca22ac2fd1bfddcb674813c7777041508dc27 Cr-Commit-Position: refs/heads/master@{#321183} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
