Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
664963002:
content: Add RendererScheduler. (Closed)
|
Created:
6 years, 2 months ago by rmcilroy Modified:
6 years, 1 month ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, jdduke+watch_chromium.org, jam, petrcermak, picksi1, eseidel Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
Descriptioncontent: Add RendererScheduler.
This CL introduces a RendererScheduler which makes use of a
TaskQueueManager to schedule rendering tasks based upon
incoming events.
A RendererSchedulerSelector chooses the next queue to
service in the available TasksQueues by priority. The
RendererSchedulerSelector first looks at all queues with
the highest priority level, choosing the queue with the
oldest pending task. If these queues are empty, it then
performs the same process one level of priority lower.
Queues can be enabled / disabled and changed priority.
The RendererScheduler has three user accessible queues -
a default task queue, a compositor task queue and an
idle task queue. By default the default and compositor
task queues have normal priority, with the idle task queue
being disabled. The scheduler will enter scheduler priority
policy for a short time (100ms) after an input event,
during which the compositor tasks will have high priority.
Between frame times the scheduler will enable idle periods,
during which it enables scheduling of tasks in the idle
queue at a best effort priority (i.e., lower priority than
any other task).
The RendererScheduler also creates a non-user accessible
task queue called the control queue which is used to enable
posting of scheduling control operations on the main
thread. This queue has the highest priority, with tasks
always run before any user task. This queue is used to
enable thread-safe transitioning to the compositor priority
policy.
Design doc: https://docs.google.com/a/chromium.org/document/d/16f_RIhZa47uEK_OdtTgzWdRU0RFMTQWMpEWyWXIpXUo/edit
BUG=391005
Committed: https://crrev.com/321f924d52091bd19cbdf7397fb0764a574741ee
Cr-Commit-Position: refs/heads/master@{#302929}
Patch Set 1 : #
Total comments: 24
Patch Set 2 : #
Total comments: 39
Patch Set 3 : Update for review and add starvation prevention #Patch Set 4 : Refine the starvation checking logic #
Total comments: 30
Patch Set 5 : Add IdleTaskRunner, address review comments and Moar Tests! #Patch Set 6 : Fix bug in calculating remaining_compositor_priority_time #
Total comments: 28
Patch Set 7 : Even moar tests, add a null scheduler and follow review. #
Total comments: 10
Patch Set 8 : Remove RendererSchedulerSelector which is now in https://codereview.chromium.org/657953004/ #
Total comments: 28
Patch Set 9 : #
Total comments: 15
Patch Set 10 : Address Alex's comments. #
Total comments: 4
Patch Set 11 : RendererSchedulerSelector -> RendererTaskQueueSelector #
Total comments: 8
Patch Set 12 : Fix nits #
Total comments: 45
Patch Set 13 : #Patch Set 14 : Address Daniel's comments. #Patch Set 15 : Add TODO and CONTENT_EXPORT for tests. #
Total comments: 6
Patch Set 16 : Address Daniel and Jared's comments and move renderer_scheduler ownership to render_thread_impl #
Total comments: 27
Patch Set 17 : Address Comments #
Total comments: 2
Patch Set 18 : Update comments #Messages
Total messages: 69 (15 generated)
rmcilroy@chromium.org changed reviewers: + alexclarke@chromium.org, petrcermak@chromium.org, picksi@chromium.org, skyostil@chromium.org
This isn't ready for review yet (it's still not well tested and I still need to hook up the input handling to the DidReceiveInputEvent) but I thought I would send it out to see if you have any comments on the general approach. I initially started with some tricky lock-free CAS style bitmap manipulation for the RendererSchedulerSelector::SetQueuePriority to enable moving between compositor/normal priority policy from any thread, however this ended up being pretty complicated (and difficult to ensure it was race-free) so I went with a simpler approach where the scheduler can post tasks to the control queue (which has the highest priority level) which can perform these priority updates on the main thread before running any of the user-posted tasks. Let me know what you think.
Patchset #1 (id:1) has been deleted
alexclarke@google.com changed reviewers: + alexclarke@google.com
https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:49: control_task_runner_->PostTask( On one hand I like the idea of posting tasks to avoid locks, but on the other hand this regresses the usefulness of ShouldYieldForHighPriorityWork which is suupposed to be able to transition from false -> true mid task.
https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:49: control_task_runner_->PostTask( On 2014/10/20 10:10:10, alexclarke wrote: > On one hand I like the idea of posting tasks to avoid locks, but on the other > hand this regresses the usefulness of ShouldYieldForHighPriorityWork which is > suupposed to be able to transition from false -> true mid task. Good point. We could potentially update the current_policy_ atomic word directly from the input thread and only do the updates of the renderer_scheduler_selector priorities on the control task, this would enable the transition mid-task, but would be a bit more complex. WDYT?
https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:72: if (SchedulerPolicy() != policy) { Should we early out if SchedulerPolicy() == policy rather than making the entire function conditional? https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:83: break; When we set Scheduler Policy to Compositor Priority don't we need to 'clean' any existing delayed tasks that drop us back into normal priority?... Or at least a ref count that tracks how many enters/exits of highpriority mode we've had? https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:96: task_queue_manager_.PumpQueue(kIdleTaskQueue); as above, do we need to clean any other 'return to normal priority' tasks from the delayed task queue? Or is the delayed task queue not going to be processed in idle mode? https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.h (right): https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.h:65: Why are these not part of the enum? https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:14: } Does this exist elsewhere as a constant already? https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:28: queue_priorities_[kNormalPriority] = (1 << work_queues.size()) - 1; Is it worth doing setting the initial bits here to all zero's and then having a for loop that calls EnableQueue() setting all queues as kNormalPriority? I know it would be (much) slower but it would make it clearer than the base 2-voodoo(!), and helps 'encapsulate' the implementation details. https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:52: } Does each queue only exist in a single 'priority' list? If so we could track where each queue was enabled and then this loop could change into something like: // Remove old flag queue_priorities_[ owner[queue_id] ] &= ~queue_id_mask_; // Change where this flag is set owner[queue_id] = set_priority; // Set the flag queue_priorities_[ owner[queue_id] ] |= queue_id_mask_; ... or ... is it worth simplifying this function to: // Clear from all priorities DisableQueue(queue_id); // Set for the priority requested queue_priorities_[priority] |= queue_id_mask_; https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:76: } There's a lot of (1<<queue_id)'s in the code. Should we wrap them as a function in case we (go mad and) decide to change how we allocate bits to queues? https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:86: } For clarity it might be worth breaking this 'if' statement into 2 instead of or'ing ~unrelated tests together. If the empty check was done first it would be clearer too.
https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:49: control_task_runner_->PostTask( On 2014/10/20 11:34:03, rmcilroy wrote: > On 2014/10/20 10:10:10, alexclarke wrote: > > On one hand I like the idea of posting tasks to avoid locks, but on the other > > hand this regresses the usefulness of ShouldYieldForHighPriorityWork which is > > suupposed to be able to transition from false -> true mid task. > > Good point. We could potentially update the current_policy_ atomic word directly > from the input thread and only do the updates of the renderer_scheduler_selector > priorities on the control task, this would enable the transition mid-task, but > would be a bit more complex. WDYT? I think it may be necessary to change policy here. I'd prefer to keep EnterSchedulerPolicy the only place where current_policy_ is assigned. Unfortunately that implies EnterSchedulerPolicy must be thread safe. https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:77: control_task_runner_->PostDelayedTask( Suppose you keep your finger down longer than kCompositoryPriorityAfterTouchTime, won't that result in the policy flapping between normal and compositor priority? Also what happens if we shut down while this is pending? https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:70: void RendererScheduler::EnterSchedulerPolicy(Policy policy) { With an eye to the future where we may have a loading priority policy, I think it would make sense to write a function that based on various signals decides what policy the scheduler should be in and calls EnterSchedulerPolicy accordingly. It needs to obey the flag I added too, which disables the scheduler.
I really like the division between the scheduler and the selector. Maybe we'd want to pull out the policy too but we can always do that later. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:20: task_queue_manager_.TaskRunnerForQueue(kControlQueue)) { Is this intended with git cl format? Looks a bit weird. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:22: kControlQueue, RendererSchedulerSelector::kControlPriority);; nit: extra ; https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:43: if (base::TimeTicks::Now() < estimated_next_frame_begin_) { Please use gfx::FrameTime::Now() when comparing BeginFrame time stamps. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:49: control_task_runner_->PostTask( I guess Alex already brought this up but somehow we need to make this affect the output of ShouldYieldForHighPriorityWork immediately so that DOM timers can yield right away without going back to the message loop. It would be good if we can avoid posting a new task for every input event we get. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:79: base::Bind(&RendererScheduler::EnterSchedulerPolicy, This is nice way to get back to normal mode. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.h (right): https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.h:27: kTaskQueueCount, Strange to have the count here since we have +1 queue internally. Do we need this for anything? Alternatively we could add separate accessors for the different task queues. It's not like anyone needs to iterate them outside this class. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.h:33: // Returns the task runner which targets the queue selected by |queue_index|. |queue|. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.h:67: base::TimeDelta kCompositoryPriorityAfterTouchTime = static const? Needs to be a POD number in that case. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:13: int kBitsInByte = 8; nit: const int. Generally things in namespaces aren't indented. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:23: DCHECK(work_queues.size() < queue_priorities_[kNormalPriority] * kBitsInByte); What are we checking for here? It seems like we'd need to check we have enough bits to represent every queue. Is there a sizeof missing here? https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:43: int64_t queue_id_mask_ = 1 << queue_id; size_t? Also the RHS has only 32 bits. Also no underscore at the end. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:104: for (QueuePriority priority = kControlPriority; It seems like this prioritizes the high priority set a little too much because it as long as there is work in the high priority queue, it will always get serviced first. I think we need to round-robin the queues instead. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:30: // Enable the |queue_id| with a priority of |priority|. Are all queues enabled or disabled by default? Probably worth mentioning. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:50: bool ControlQueueId(size_t* out_queue_index); Unused? https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:53: bool QueueEnabledWithPriority(size_t queue_id, QueuePriority priority); nit: const https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:55: QueuePriority NextPriorty(QueuePriority priority); typo: Priority
https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:57: return (SchedulerPolicy() == kCompositorPriorityPolicy) && nit - is there any reason for the brackets? https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:13: int kBitsInByte = 8; On 2014/10/20 13:31:08, Sami wrote: > nit: const int. Generally things in namespaces aren't indented. Is there any reason why we don't use CHAR_BIT from <limits.h>? Also, the number should probably be unsigned. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:23: DCHECK(work_queues.size() < queue_priorities_[kNormalPriority] * kBitsInByte); On 2014/10/20 13:31:09, Sami wrote: > What are we checking for here? It seems like we'd need to check we have enough > bits to represent every queue. Is there a sizeof missing here? I think that it should be "sizeof(queue_priorities_[kNormalPriority])". Moreover, is there any reason to exclude equality (i.e. shouldn't it be "<=")? https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:41: DCHECK(queue_id < work_queues_.size()); DCHECK_LT (twice) https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:63: int64_t queue_id_mask_ = 1 << queue_id; Why don't you calculate the inverse mask straightaway? https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:30: // Enable the |queue_id| with a priority of |priority|. On 2014/10/20 13:31:09, Sami wrote: > Are all queues enabled or disabled by default? Probably worth mentioning. This is explained on renderer_scheduler_selector.cc:25. Nevertheless, I agree that it should be mentioned. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:39: virtual bool SelectWorkQueueToService(size_t* out_queue_index) override; Why is the queue index called "queue_id" when it's an input argument and "...queue_index" when it's an output argument? This suggests they are two different types of indices.
eseidel@chromium.org changed reviewers: + eseidel@chromium.org
https://codereview.chromium.org/664963002/diff/40001/content/renderer/rendere... File content/renderer/renderer_blink_platform_impl.h (right): https://codereview.chromium.org/664963002/diff/40001/content/renderer/rendere... content/renderer/renderer_blink_platform_impl.h:63: virtual void callOnMainThread(void (*func)(void*), void* context); I guess we don't have a Closure typedef (for the void function ptr) in the Web API?
On 2014/10/21 16:13:04, eseidel wrote: > https://codereview.chromium.org/664963002/diff/40001/content/renderer/rendere... > File content/renderer/renderer_blink_platform_impl.h (right): > > https://codereview.chromium.org/664963002/diff/40001/content/renderer/rendere... > content/renderer/renderer_blink_platform_impl.h:63: virtual void > callOnMainThread(void (*func)(void*), void* context); > I guess we don't have a Closure typedef (for the void function ptr) in the Web > API? That's right. I suspect we can't easily have one before the repo merge. Maybe if we get to use lambdas we could figure something out before that.
Updated for review feedback and added starvation-prevention in renderer_scheduler_selector as discussed offline. PTAL, thanks. https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:49: control_task_runner_->PostTask( On 2014/10/20 12:48:35, alexclarke wrote: > On 2014/10/20 11:34:03, rmcilroy wrote: > > On 2014/10/20 10:10:10, alexclarke wrote: > > > On one hand I like the idea of posting tasks to avoid locks, but on the > other > > > hand this regresses the usefulness of ShouldYieldForHighPriorityWork which > is > > > suupposed to be able to transition from false -> true mid task. > > > > Good point. We could potentially update the current_policy_ atomic word > directly > > from the input thread and only do the updates of the > renderer_scheduler_selector > > priorities on the control task, this would enable the transition mid-task, but > > would be a bit more complex. WDYT? > > I think it may be necessary to change policy here. I'd prefer to keep > EnterSchedulerPolicy the only place where current_policy_ is assigned. > Unfortunately that implies EnterSchedulerPolicy must be thread safe. > I've updated this to have a policy_may_need_update_ dirty bit which is set when the policy should be updated. SchedulerPolicy() will check for this and recalculate the policy if it is set. I removed EnterSchedulerPolicy and replaced it with UpdatePolicy, which is still only run on the main thread. WDYT? https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:72: if (SchedulerPolicy() != policy) { On 2014/10/20 11:42:04, picksi1 wrote: > Should we early out if SchedulerPolicy() == policy rather than making the entire > function conditional? Done. https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:77: control_task_runner_->PostDelayedTask( On 2014/10/20 12:48:35, alexclarke wrote: > Suppose you keep your finger down longer than > kCompositoryPriorityAfterTouchTime, won't that result in the policy flapping > between normal and compositor priority? Yes I guess you would go to normal prio, then immediately hit compositor prio again when the next InputEvent happened. The new patchset doesn't have this issue. > Also what happens if we shut down while this is pending? All the pending tasks get deleted in ~MessageLoop which calls DeletePendingTasks() - this should be fine, right? https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:83: break; On 2014/10/20 11:42:04, picksi1 wrote: > When we set Scheduler Policy to Compositor Priority don't we need to 'clean' any > existing delayed tasks that drop us back into normal priority?... Or at least a > ref count that tracks how many enters/exits of highpriority mode we've had? The updated patchset changes this around and should hopefully address your concerns, let me know what you think? https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:96: task_queue_manager_.PumpQueue(kIdleTaskQueue); On 2014/10/20 11:42:04, picksi1 wrote: > as above, do we need to clean any other 'return to normal priority' tasks from > the delayed task queue? Or is the delayed task queue not going to be processed > in idle mode? There is no delayed task which sets between idle time enabled / disabled. The idle time settings are orthogonal to compositor / normal priority policy and can happen independently from them. https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.h (right): https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.h:65: On 2014/10/20 11:42:04, picksi1 wrote: > Why are these not part of the enum? Because they are not public (I don't want someone calling TaskRunnerForQueue(kControlQueue) and being able to run control level tasks since these should only be posted by the scheduler. https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:14: } On 2014/10/20 11:42:04, picksi1 wrote: > Does this exist elsewhere as a constant already? Changed to CHAR_BITS as suggested by Petr. https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:28: queue_priorities_[kNormalPriority] = (1 << work_queues.size()) - 1; On 2014/10/20 11:42:04, picksi1 wrote: > Is it worth doing setting the initial bits here to all zero's and then having a > for loop that calls EnableQueue() setting all queues as kNormalPriority? I know > it would be (much) slower but it would make it clearer than the base > 2-voodoo(!), and helps 'encapsulate' the implementation details. I could do this but I'm not sure it's worth it. How about encapsolating the Voodoo in an EnableAllQueues(Priority) function? https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:52: } On 2014/10/20 11:42:04, picksi1 wrote: > Does each queue only exist in a single 'priority' list? If so we could track > where each queue was enabled and then this loop could change into something > like: > > // Remove old flag > queue_priorities_[ owner[queue_id] ] &= ~queue_id_mask_; > // Change where this flag is set > owner[queue_id] = set_priority; > // Set the flag > queue_priorities_[ owner[queue_id] ] |= queue_id_mask_; > > > ... or ... > > is it worth simplifying this function to: > > // Clear from all priorities > DisableQueue(queue_id); > // Set for the priority requested > queue_priorities_[priority] |= queue_id_mask_; Good point, I went with the second option. Done. https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:76: } On 2014/10/20 11:42:04, picksi1 wrote: > There's a lot of (1<<queue_id)'s in the code. Should we wrap them as a function > in case we (go mad and) decide to change how we allocate bits to queues? Done. https://codereview.chromium.org/664963002/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:86: } On 2014/10/20 11:42:04, picksi1 wrote: > For clarity it might be worth breaking this 'if' statement into 2 instead of > or'ing ~unrelated tests together. If the empty check was done first it would be > clearer too. I changed the order, but I'm not sure I agree that breaking the if into 2 would be clearer. Would you be are you OK with this now? https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:20: task_queue_manager_.TaskRunnerForQueue(kControlQueue)) { On 2014/10/20 13:31:08, Sami wrote: > Is this intended with git cl format? Looks a bit weird. Done. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:22: kControlQueue, RendererSchedulerSelector::kControlPriority);; On 2014/10/20 13:31:08, Sami wrote: > nit: extra ; Done. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:43: if (base::TimeTicks::Now() < estimated_next_frame_begin_) { On 2014/10/20 13:31:08, Sami wrote: > Please use gfx::FrameTime::Now() when comparing BeginFrame time stamps. Done. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:49: control_task_runner_->PostTask( On 2014/10/20 13:31:08, Sami wrote: > I guess Alex already brought this up but somehow we need to make this affect the > output of ShouldYieldForHighPriorityWork immediately so that DOM timers can > yield right away without going back to the message loop. > > It would be good if we can avoid posting a new task for every input event we > get. Done. Using policy_may_need_update_ bit to ensure policy update is immediate for ShouldYieldForHighPriorityWork and only posting a new task if last_input_time_ is null. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:57: return (SchedulerPolicy() == kCompositorPriorityPolicy) && On 2014/10/20 17:43:35, petrcermak wrote: > nit - is there any reason for the brackets? Nope, removed. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:70: void RendererScheduler::EnterSchedulerPolicy(Policy policy) { On 2014/10/20 12:48:35, alexclarke wrote: > With an eye to the future where we may have a loading priority policy, I think > it would make sense to write a function that based on various signals decides > what policy the scheduler should be in and calls EnterSchedulerPolicy > accordingly. Agreed - I've updated this to be simply UpdatePolicy which modifies the policy based on the signals, WDYT? > It needs to obey the flag I added too, which disables the scheduler. Done. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:79: base::Bind(&RendererScheduler::EnterSchedulerPolicy, On 2014/10/20 13:31:08, Sami wrote: > This is nice way to get back to normal mode. Acknowledged. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.h (right): https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.h:27: kTaskQueueCount, On 2014/10/20 13:31:08, Sami wrote: > Strange to have the count here since we have +1 queue internally. Do we need > this for anything? > > Alternatively we could add separate accessors for the different task queues. > It's not like anyone needs to iterate them outside this class. Good point, went with accessors. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.h:33: // Returns the task runner which targets the queue selected by |queue_index|. On 2014/10/20 13:31:08, Sami wrote: > |queue|. Done. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.h:67: base::TimeDelta kCompositoryPriorityAfterTouchTime = On 2014/10/20 13:31:08, Sami wrote: > static const? Needs to be a POD number in that case. Done. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:30: // Enable the |queue_id| with a priority of |priority|. On 2014/10/20 17:43:36, petrcermak wrote: > On 2014/10/20 13:31:09, Sami wrote: > > Are all queues enabled or disabled by default? Probably worth mentioning. > > This is explained on renderer_scheduler_selector.cc:25. Nevertheless, I agree > that it should be mentioned. Done. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:39: virtual bool SelectWorkQueueToService(size_t* out_queue_index) override; On 2014/10/20 17:43:36, petrcermak wrote: > Why is the queue index called "queue_id" when it's an input argument and > "...queue_index" when it's an output argument? This suggests they are two > different types of indices. Good catch. Changed all to queue_index https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:50: bool ControlQueueId(size_t* out_queue_index); On 2014/10/20 13:31:09, Sami wrote: > Unused? Removed, thanks. https://codereview.chromium.org/664963002/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:53: bool QueueEnabledWithPriority(size_t queue_id, QueuePriority priority); On 2014/10/20 13:31:09, Sami wrote: > nit: const Done.
https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:114: return; Shouldn't we clear the dirty flag here too? https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.h (right): https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.h:37: void WillBeginFrame(const cc::BeginFrameArgs& args); Perhaps this could post itself onto the kControlTaskQueue if it's called from a different thread? https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.h:41: void DidCommitFrameToCompositor(); Likewise perhaps this could post itself onto the kControlTaskQueue if it's called from a different thread? https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.h:86: base::ThreadChecker main_thread_checker_; It might make sense to add a struct to gather together all variables that must only be accessed on a particular thread or inside a the lock.
Thanks, added some initial comments. I'll still think through the starvation logic. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.h (right): https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.h:27: scoped_refptr<base::SingleThreadTaskRunner> DefaultTaskRunner(); These guys could be const, right (now that I fixed the corresponding function in TQM)? https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.h:37: void WillBeginFrame(const cc::BeginFrameArgs& args); On 2014/10/22 10:06:37, alexclarke wrote: > Perhaps this could post itself onto the kControlTaskQueue if it's called from a > different thread? It should never be called from a different thread so I think we can just assert that. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.h:41: void DidCommitFrameToCompositor(); On 2014/10/22 10:06:37, alexclarke wrote: > Likewise perhaps this could post itself onto the kControlTaskQueue if it's > called from a different thread? Likewise. I think we should try to limit things to the use cases we'll actually encounter. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:21: DCHECK(work_queues.size() < queue_priorities_[kNormalPriority] * CHAR_BIT); Still missing the sizeof here? Also, DCHECK_LT. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:32: DCHECK(priority < kQueuePriorityCount); DCHECK_LT https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:44: DCHECK(queue_index < work_queues_.size()); DCHECK_LT again (2x). https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:57: DCHECK(queue_index < work_queues_.size()); DCHECK_LT https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:61: queue_priorities_[priority] &= ~(PrioritiesMaskFromQueueIndex(queue_index)); nit: No parens needed around the function call. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:16: kControlPriority, Maybe mention that this must be the first entry. Alternatively you could add a kFirstQueuePriority = kControlPriority at the end. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:58: static const int kMaxStarvationTasks = 5; Any science behind this number? :) We should check with tough_scheduling_cases whether this works well in the corner cases in there.
I think the starvation prevention should work, but probably the next patch we want to write after this one adds tracing for all the internal bits so we know why the scheduler did what it did. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:128: base::subtle::NoBarrier_Store(&policy_may_need_update_, 0); Thinking out loud: these can be barrier-less safely because they aren't guarding any critical data; it doesn't matter if the policy is wrong for a few iterations of the message loop. Right? Of course, the situation where it does matter when we're going from normal to compositor priority and have critical work to do. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:62: size_t queue_priorities_[kQueuePriorityCount]; Could you add a comment here that explains the data layout (each element has N bits for the N different queues. only one bit in the same column can be set across all the elements. etc.) Edit: thinking about this more, since we're iterating across every queue every time we select a new task for a given priority, the bit mask isn't really buying us much. If we made it into a data structure where we can easily traverse the queues for a given priority (e.g., each priority has a list of queue indices), then that would make more sense performance-wise. Otherwise we could just have an array of N (N = number of queues) enums that holds the priority of each queue.
https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:74: task_queue_manager_.PollQueue(kCompositorTaskQueue); I think this should be: !task_queue_manager_.IsQueueEmpty(kCompositorTaskQueue);
https://codereview.chromium.org/664963002/diff/80001/content/renderer/input/i... File content/renderer/input/input_handler_wrapper.cc (right): https://codereview.chromium.org/664963002/diff/80001/content/renderer/input/i... content/renderer/input/input_handler_wrapper.cc:61: // TODO(rmcilroy) I tried this locally and I had to route render_thread_impl_ from render_thread_impl.cc in via InputHandlerManager if (render_thread_impl_ && render_thread_impl_->blink_platform_impl() && render_thread_impl_->blink_platform_impl()->renderer_scheduler()) { render_thread_impl_->blink_platform_impl()->renderer_scheduler()-> DidReceiveInputEvent(); }
Still working on some issues, but uploading now for Alex to do some testing on. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:74: task_queue_manager_.PollQueue(kCompositorTaskQueue); On 2014/10/22 15:59:42, alexclarke wrote: > I think this should be: > !task_queue_manager_.IsQueueEmpty(kCompositorTaskQueue); Hadn't synced Sami's new changes yet - done now. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:114: return; On 2014/10/22 10:06:37, alexclarke wrote: > Shouldn't we clear the dirty flag here too? Yes good point - moved this to the top of the function (since the dirty flag is write-protected by the incoming_signals_lock). https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler.cc:128: base::subtle::NoBarrier_Store(&policy_may_need_update_, 0); On 2014/10/22 14:49:24, Sami wrote: > Thinking out loud: these can be barrier-less safely because they aren't guarding > any critical data; it doesn't matter if the policy is wrong for a few iterations > of the message loop. Right? Of course, the situation where it does matter when > we're going from normal to compositor priority and have critical work to do. Yes, they aren't guarding anything, and actually write access is protected by the incoming_signals_lock_ mutex. The lock operations are already a barrier, however there is no implicit barrier for the SchedulerPolicy() function, so it could possibly miss a switch to compositor priority. I've updated to Acquire/Release which should ensure we are always as up-to-date as possible, but I'm not sure if the cost of the memory barriers will hurt us. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:21: DCHECK(work_queues.size() < queue_priorities_[kNormalPriority] * CHAR_BIT); On 2014/10/22 13:26:06, Sami wrote: > Still missing the sizeof here? Also, DCHECK_LT. Sorry missed your comment here last time - done. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:32: DCHECK(priority < kQueuePriorityCount); On 2014/10/22 13:26:06, Sami wrote: > DCHECK_LT Done. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:44: DCHECK(queue_index < work_queues_.size()); On 2014/10/22 13:26:06, Sami wrote: > DCHECK_LT again (2x). Done. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:57: DCHECK(queue_index < work_queues_.size()); On 2014/10/22 13:26:06, Sami wrote: > DCHECK_LT Done. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.cc:61: queue_priorities_[priority] &= ~(PrioritiesMaskFromQueueIndex(queue_index)); On 2014/10/22 13:26:06, Sami wrote: > nit: No parens needed around the function call. Done. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:16: kControlPriority, On 2014/10/22 13:26:06, Sami wrote: > Maybe mention that this must be the first entry. Alternatively you could add a > kFirstQueuePriority = kControlPriority at the end. Done. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:58: static const int kMaxStarvationTasks = 5; On 2014/10/22 13:26:06, Sami wrote: > Any science behind this number? :) We should check with tough_scheduling_cases > whether this works well in the corner cases in there. Yes, no science :). I will run against tough_scheduler_cases to check what might work best here. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:62: size_t queue_priorities_[kQueuePriorityCount]; On 2014/10/22 14:49:24, Sami wrote: > Could you add a comment here that explains the data layout (each element has N > bits for the N different queues. only one bit in the same column can be set > across all the elements. etc.) > > Edit: thinking about this more, since we're iterating across every queue every > time we select a new task for a given priority, the bit mask isn't really buying > us much. If we made it into a data structure where we can easily traverse the > queues for a given priority (e.g., each priority has a list of queue indices), > then that would make more sense performance-wise. Otherwise we could just have > an array of N (N = number of queues) enums that holds the priority of each > queue. Yes makes sense. I'll update this on my next patch-set.
The tests look great! https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.cc:76: base::Bind(&RendererScheduler::UpdatePolicy, base::Unretained(this))); |This| should be a weak pointer, right? Is this worth making into a helper function, PostUpdatePolicyOnControlRunner? https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.cc:107: base::TimeDelta compositor_priority_time = nit: I'd call this compositor_priority_duration to avoid mixing times and durations. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.cc:109: base::TimeDelta remaining_compositor_priority_time = Ditto. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.cc:115: base::Bind(&RendererScheduler::UpdatePolicy, base::Unretained(this)), weak_ptr here too. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler.h (right): https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.h:57: scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_); Extra underscore at the end. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_unittest.cc (right): https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_unittest.cc:16: class DelaySupportingTestSimpleTaskRunner : public base::TestSimpleTaskRunner { I wonder if we could just reuse cc/test/ordered_simple_task_runner.h here? It looks very similar. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.cc (right): https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.cc:26: task_runner_->PostTask(FROM_HERE, base::Bind( Please pass the original |from_here| so we don't lose track of which task this is. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.cc:27: &SingleThreadIdleTaskRunner::RunTask, base::Unretained(this), idle_task)); I guess Unretained shouldn't be used here since the class is refcounted and we check the weak pointer in the callback. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.h (right): https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:27: explicit SingleThreadIdleTaskRunner( nit: No need for explicit. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:37: friend class base::RefCountedThreadSafe<SingleThreadIdleTaskRunner>; One extra space of indent here. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:47: public: Indentation is a little off here.
https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_selector.cc:21: DCHECK_LT(work_queues.size(), Maybe add a comment, I had to read this several times to understand what it was guarding against. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_selector.cc:27: queue_priorities_[kNormalPriority] = (1 << work_queues.size()) - 1; Is there a reason to use work_queues.size() instead of kQueuePriorityCount? Alternatively should we assert that work_queues.size() == kQueuePriorityCount? Or should that be >= ? https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_selector.cc:54: for (auto priority: { kControlPriority, fyi: gcc doesn't like this, although I do (apart from the auto). I had to change this to the less elegant: for (QueuePriority priority = kControlPriority; priority < kQueuePriorityCount; priority = static_cast<QueuePriority>(static_cast<int>(priority) + 1)) {
Updated following review. Note, unfortunately I renamed some files which will make patch-set diff unhappy :( - sorry. https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/664963002/diff/80001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_selector.h:62: size_t queue_priorities_[kQueuePriorityCount]; On 2014/10/23 14:54:50, rmcilroy wrote: > On 2014/10/22 14:49:24, Sami wrote: > > Could you add a comment here that explains the data layout (each element has N > > bits for the N different queues. only one bit in the same column can be set > > across all the elements. etc.) > > > > Edit: thinking about this more, since we're iterating across every queue every > > time we select a new task for a given priority, the bit mask isn't really > buying > > us much. If we made it into a data structure where we can easily traverse the > > queues for a given priority (e.g., each priority has a list of queue indices), > > then that would make more sense performance-wise. Otherwise we could just have > > an array of N (N = number of queues) enums that holds the priority of each > > queue. > > Yes makes sense. I'll update this on my next patch-set. Done. Changed it to use a set. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.cc:76: base::Bind(&RendererScheduler::UpdatePolicy, base::Unretained(this))); On 2014/10/24 10:46:13, Sami wrote: > |This| should be a weak pointer, right? > > Is this worth making into a helper function, PostUpdatePolicyOnControlRunner? Done. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.cc:109: base::TimeDelta remaining_compositor_priority_time = On 2014/10/24 10:46:13, Sami wrote: > Ditto. Done. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.cc:115: base::Bind(&RendererScheduler::UpdatePolicy, base::Unretained(this)), On 2014/10/24 10:46:13, Sami wrote: > weak_ptr here too. Done. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler.h (right): https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.h:57: scoped_refptr<base::SingleThreadTaskRunner> main_task_runner_); On 2014/10/24 10:46:13, Sami wrote: > Extra underscore at the end. Done. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_selector.cc:21: DCHECK_LT(work_queues.size(), On 2014/10/24 14:37:26, alexclarke wrote: > Maybe add a comment, I had to read this several times to understand what it was > guarding against. No longer applicable. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_selector.cc:27: queue_priorities_[kNormalPriority] = (1 << work_queues.size()) - 1; On 2014/10/24 14:37:26, alexclarke wrote: > Is there a reason to use work_queues.size() instead of kQueuePriorityCount? > > Alternatively should we assert that work_queues.size() == kQueuePriorityCount? > Or should that be >= ? No longer applicable (plus work_queues.size() is never equal to kQueuePriorityCount - there can be many more queues than there are priority levels). https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_selector.cc:54: for (auto priority: { kControlPriority, On 2014/10/24 14:37:26, alexclarke wrote: > fyi: gcc doesn't like this, although I do (apart from the auto). > > I had to change this to the less elegant: > > for (QueuePriority priority = kControlPriority; > priority < kQueuePriorityCount; > priority = static_cast<QueuePriority>(static_cast<int>(priority) + 1)) { I changed it to a static array. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_selector.cc:54: for (auto priority: { kControlPriority, On 2014/10/24 14:37:26, alexclarke wrote: > fyi: gcc doesn't like this, although I do (apart from the auto). > > I had to change this to the less elegant: > > for (QueuePriority priority = kControlPriority; > priority < kQueuePriorityCount; > priority = static_cast<QueuePriority>(static_cast<int>(priority) + 1)) { Modified this to be a ranged for on a static const array instead. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_unittest.cc (right): https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_unittest.cc:16: class DelaySupportingTestSimpleTaskRunner : public base::TestSimpleTaskRunner { On 2014/10/24 10:46:13, Sami wrote: > I wonder if we could just reuse cc/test/ordered_simple_task_runner.h here? It > looks very similar. I'll look into whether this is possible. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.cc (right): https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.cc:26: task_runner_->PostTask(FROM_HERE, base::Bind( On 2014/10/24 10:46:13, Sami wrote: > Please pass the original |from_here| so we don't lose track of which task this > is. Argh, that's what I meant to do - good spot! Done. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.cc:27: &SingleThreadIdleTaskRunner::RunTask, base::Unretained(this), idle_task)); On 2014/10/24 10:46:13, Sami wrote: > I guess Unretained shouldn't be used here since the class is refcounted and we > check the weak pointer in the callback. Done. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.h (right): https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:27: explicit SingleThreadIdleTaskRunner( On 2014/10/24 10:46:13, Sami wrote: > nit: No need for explicit. Your right, not now that I added an extra param :). Done https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:37: friend class base::RefCountedThreadSafe<SingleThreadIdleTaskRunner>; On 2014/10/24 10:46:13, Sami wrote: > One extra space of indent here. Done. https://codereview.chromium.org/664963002/diff/120001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:47: public: On 2014/10/24 10:46:13, Sami wrote: > Indentation is a little off here. Done.
https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.cc:17: class NullIdleTaskRunner : public SingleThreadIdleTaskRunner { Shouldn't the null scheduler be in it's own file? https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:132: if (new_policy == current_policy_) { Consider adding: TRACE_COUNTER1(TRACE_DISABLED_BY_DEFAULT("blink.scheduler"), "policy", current_policy_); And maybe TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("blink.scheduler"), "UpdatePolicy"); https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:100: base::ThreadChecker main_thread_checker_; I think it might be useful to group these by which thread they're allowed to be accessed from. https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_selector.cc:60: bool RendererSchedulerSelector::ChooseOldestWithPriority( nit add a blank line between the two methods https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_selector.h:21: kBestEffortPriority, Please document what kBestEffortPriority does (i.e. tasks from a kBestEffortPriority queue are only run when the other queues are empty)
https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:132: if (new_policy == current_policy_) { On 2014/10/24 15:56:29, alexclarke wrote: > Consider adding: > > TRACE_COUNTER1(TRACE_DISABLED_BY_DEFAULT("blink.scheduler"), "policy", > current_policy_); > > And maybe > > TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("blink.scheduler"), "UpdatePolicy"); I've got a follow-up that adds all sorts of tracing so we can leave that out of this one.
Looks excellent. Added some minor gripes. https://codereview.chromium.org/664963002/diff/160001/content/content_rendere... File content/content_renderer.gypi (right): https://codereview.chromium.org/664963002/diff/160001/content/content_rendere... content/content_renderer.gypi:371: 'renderer/scheduler/renderer_scheduler_selector.cc', Looks like the selector got left in here. https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.cc:109: if (command_line->HasSwitch(switches::kDisableBlinkScheduler)) { Could you hardcode this to use the null scheduler for now with a TODO so we can switch everything on with a separate patch? https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:25: // Returns the default task runner. Probably best not to repeat the same comments here since they will get out of sync. Just say "// RendererScheduler implementation:". https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:113: // and write access to policy_may_need_update_. Seems odd to say this just protects writes to |policy_may_need_update_|. Just protecting writes doesn't really buy you anything in terms of data correctness :) Since |policy_may_need_update_| is always read and written atomically with barriers I don't think it needs any lock protection. https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:48: bool HaveRunnablePendingTasks() { nit: const https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:49: for (base::TestPendingTask pending_task : pending_tasks_) { nit: const ref (or const auto&). https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:62: for (base::TestPendingTask pending_task : tasks_to_run) { nit: const ref (or const auto&). https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:66: pending_tasks_.push_front(pending_task); I think this is slightly wrong since delayed tasks should run after all pending non-delayed tasks regardless of the current time (e.g. base::MessageLoop first does DoWork() and then DoDelayedWork()). If a non-pending task here is reentrant (on line 64), it will interleave rather than jump ahead of the delayed tasks. To fix this you could push these tasks a another deque (delayed_tasks) and then append that to pending_tasks_ after exiting the run loop. https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:252: EXPECT_FALSE(task_run); // Shouldn't run as no didCommitFrameToCompositor. s/did/Did/ https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:266: EXPECT_TRUE(task_run); Could you also check that the deadline was given to the task correctly? Could also be an independent test that just checks CurrentIdleTaskDeadline(). https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.h (right): https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:51: virtual base::TimeTicks CurrentIdleTaskDeadline() const = 0; protected: virtual ~IdleTaskDeadlineSupplier(); https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... File content/renderer/scheduler/web_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/web_scheduler_impl.h:19: virtual bool shouldYieldForHighPriorityWork(); We need to add |shutdown| here too.
Thanks for the reviews. https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.cc:17: class NullIdleTaskRunner : public SingleThreadIdleTaskRunner { On 2014/10/24 15:56:29, alexclarke wrote: > Shouldn't the null scheduler be in it's own file? I didn't think it was worth it, but OK, done. https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:100: base::ThreadChecker main_thread_checker_; On 2014/10/24 15:56:30, alexclarke wrote: > I think it might be useful to group these by which thread they're allowed to be > accessed from. It's not easy to do this and still maintain constructor initialization order (e.g., the weak_factory needs to be at the end in order to work correctly). Done as best as I can https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_selector.cc (right): https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_selector.cc:60: bool RendererSchedulerSelector::ChooseOldestWithPriority( On 2014/10/24 15:56:30, alexclarke wrote: > nit add a blank line between the two methods Done. https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/664963002/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_selector.h:21: kBestEffortPriority, On 2014/10/24 15:56:30, alexclarke wrote: > Please document what kBestEffortPriority does (i.e. tasks from a > kBestEffortPriority queue are only run when the other queues are empty) Added documentation for all priorities (in other CL). https://codereview.chromium.org/664963002/diff/160001/content/content_rendere... File content/content_renderer.gypi (right): https://codereview.chromium.org/664963002/diff/160001/content/content_rendere... content/content_renderer.gypi:371: 'renderer/scheduler/renderer_scheduler_selector.cc', On 2014/10/27 12:02:38, Sami wrote: > Looks like the selector got left in here. Done. https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.cc:109: if (command_line->HasSwitch(switches::kDisableBlinkScheduler)) { On 2014/10/27 12:02:38, Sami wrote: > Could you hardcode this to use the null scheduler for now with a TODO so we can > switch everything on with a separate patch? Done. https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:25: // Returns the default task runner. On 2014/10/27 12:02:38, Sami wrote: > Probably best not to repeat the same comments here since they will get out of > sync. Just say "// RendererScheduler implementation:". Done. https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:113: // and write access to policy_may_need_update_. On 2014/10/27 12:02:38, Sami wrote: > Seems odd to say this just protects writes to |policy_may_need_update_|. Just > protecting writes doesn't really buy you anything in terms of data correctness > :) Since |policy_may_need_update_| is always read and written atomically with > barriers I don't think it needs any lock protection. It does actually buy you something because we want want to avoid the following situation: - Thread 1 updates signals to X and sets policy_may_need_update_ to 1 - Thread 0 runs updatePolicy, and reads signals X, then releases signals lock and gets unscheduled - Thread 1 updates signals to Y and sets policy_may_need_update_ to 1 - Thread 0 gets rescheduled and updates policy based on signals X, then sets policy_may_need_update_ to 0 - Now the policy update based on signals Y has been lost. Subtle, but I think we should avoid this, and can do so if we protect write access to policy_may_need_update_ with the lock and ensure we only update it at the same time as we read the signals variables. https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:48: bool HaveRunnablePendingTasks() { On 2014/10/27 12:02:38, Sami wrote: > nit: const Moved to using cc::OrderedSimpleTaskRunner (thanks for the hint), so no longer applicable. https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:49: for (base::TestPendingTask pending_task : pending_tasks_) { On 2014/10/27 12:02:38, Sami wrote: > nit: const ref (or const auto&). ditto https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:62: for (base::TestPendingTask pending_task : tasks_to_run) { On 2014/10/27 12:02:38, Sami wrote: > nit: const ref (or const auto&). ditto https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:66: pending_tasks_.push_front(pending_task); On 2014/10/27 12:02:38, Sami wrote: > I think this is slightly wrong since delayed tasks should run after all pending > non-delayed tasks regardless of the current time (e.g. base::MessageLoop first > does DoWork() and then DoDelayedWork()). If a non-pending task here is reentrant > (on line 64), it will interleave rather than jump ahead of the delayed tasks. > > To fix this you could push these tasks a another deque (delayed_tasks) and then > append that to pending_tasks_ after exiting the run loop. ditto https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:252: EXPECT_FALSE(task_run); // Shouldn't run as no didCommitFrameToCompositor. On 2014/10/27 12:02:38, Sami wrote: > s/did/Did/ Done. https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:266: EXPECT_TRUE(task_run); On 2014/10/27 12:02:38, Sami wrote: > Could you also check that the deadline was given to the task correctly? Could > also be an independent test that just checks CurrentIdleTaskDeadline(). The deadline was checked in IdleTestTask. Moved the checking to here for clarity. https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.h (right): https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:51: virtual base::TimeTicks CurrentIdleTaskDeadline() const = 0; On 2014/10/27 12:02:38, Sami wrote: > protected: > virtual ~IdleTaskDeadlineSupplier(); Done. https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... File content/renderer/scheduler/web_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/web_scheduler_impl.h:19: virtual bool shouldYieldForHighPriorityWork(); On 2014/10/27 12:02:38, Sami wrote: > We need to add |shutdown| here too. Done.
https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... File content/renderer/scheduler/null_renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/null_renderer_scheduler.cc:63: void NullRendererScheduler::WillBeginFrame(const cc::BeginFrameArgs& args) { Do you get any compile warnings here? I wonder if you need to remove args. https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:54: return default_task_runner_; Maybe add DCHECK(task_queue_manager_). It would be unfortunate if something tried to post to the queues after shutdown has been called, but before this class is deleted. https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:123: weak_renderer_scheduler_ptr_); Hmm the weak pointer results in a check that the scheduler has not been deleted, but I think it's possible for Shutdown() to get called while one of these is pending. So I think UpdatePolicy should bail out if task_queue_manager_ is null. https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:172: task_queue_manager_->PumpQueue(kIdleTaskQueue); Is it worth mentioning that this function assumes task_queue_manager_ is not null? https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:62: static const int kCompositorPriorityAfterTouchMillis = 100; Could this be a base::TimeDelta or is there a problem with static initialization of non POD classes? https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.cc (right): https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.cc:17: : task_runner_(task_runner), deadline_supplier_(deadline_supplier) { Maybe DCHECK that deadline_supplier is not null? https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... File content/renderer/scheduler/web_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/web_scheduler_impl.cc:35: idle_task_runner_->PostIdleTask( From a defensive programming PoV I think we should track if shutdown has been called and make this function exit if so. Otherwise if this function gets called, we risk accessing deleted memory.
https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... File content/renderer/scheduler/null_renderer_scheduler.cc (right): https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/null_renderer_scheduler.cc:63: void NullRendererScheduler::WillBeginFrame(const cc::BeginFrameArgs& args) { On 2014/10/27 17:35:29, alexclarke wrote: > Do you get any compile warnings here? I wonder if you need to remove args. Nope, no warnings. Unused arguments aren't don't get warnings like unused locals AFAIK. https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:54: return default_task_runner_; On 2014/10/27 17:35:29, alexclarke wrote: > Maybe add DCHECK(task_queue_manager_). It would be unfortunate if something > tried to post to the queues after shutdown has been called, but before this > class is deleted. This doesn't really help though - whether or not the call to this function is before or after shutdown, the caller will still have a reference to the task_runner, so they can continue posting tasks to it after shutdown no matter what DCHECKS we add here. Sami's TaskQueueManager implementation deals with this by dropping the tasks, which is fine I think - to catch what you are suggesting we would need to add the DCHECK into TaskQueueManager::TaskQueue. https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:123: weak_renderer_scheduler_ptr_); On 2014/10/27 17:35:29, alexclarke wrote: > Hmm the weak pointer results in a check that the scheduler has not been deleted, > but I think it's possible for Shutdown() to get called while one of these is > pending. So I think UpdatePolicy should bail out if task_queue_manager_ is > null. Done (although UpdatePolicy doesn't touch task_queue_manager_ so would be safe without the bailout anyway, but agreed, this is probably better). https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:172: task_queue_manager_->PumpQueue(kIdleTaskQueue); On 2014/10/27 17:35:29, alexclarke wrote: > Is it worth mentioning that this function assumes task_queue_manager_ is not > null? I don't think it's necessary to mention it for every private function which doesn't have the bailout since I think it it is fairly self-explanitory that no private functions should be called if we have shutdown - all public functions which call this or any other private function should check task_queue_manager_ and bailout if the schedule has shutdown. https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:62: static const int kCompositorPriorityAfterTouchMillis = 100; On 2014/10/27 17:35:29, alexclarke wrote: > Could this be a base::TimeDelta or is there a problem with static initialization > of non POD classes? It needs to be a POD https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.cc (right): https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.cc:17: : task_runner_(task_runner), deadline_supplier_(deadline_supplier) { On 2014/10/27 17:35:29, alexclarke wrote: > Maybe DCHECK that deadline_supplier is not null? Then it would fail with the NullRendererScheduler where I explicitly pass null :). https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... File content/renderer/scheduler/web_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/web_scheduler_impl.cc:35: idle_task_runner_->PostIdleTask( On 2014/10/27 17:35:29, alexclarke wrote: > From a defensive programming PoV I think we should track if shutdown has been > called and make this function exit if so. Otherwise if this function gets > called, we risk accessing deleted memory. I disagree - it is OK to post to the task runners when the scheduler is shutdown, the tasks just get silently dropped and deleted so there shouldn't be a risk of memory corruption. I think tracking shutdown in two places is overly complex - if we track it here, should we also track it where we are posting to the compositor or default task runners? What happens if we call shutdown from a different entry point (e.g., within chrome)? WDYT?
https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... File content/renderer/scheduler/web_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/180001/content/renderer/schedu... content/renderer/scheduler/web_scheduler_impl.cc:35: idle_task_runner_->PostIdleTask( On 2014/10/27 18:18:02, rmcilroy wrote: > On 2014/10/27 17:35:29, alexclarke wrote: > > From a defensive programming PoV I think we should track if shutdown has been > > called and make this function exit if so. Otherwise if this function gets > > called, we risk accessing deleted memory. > > I disagree - it is OK to post to the task runners when the scheduler is > shutdown, the tasks just get silently dropped and deleted so there shouldn't be > a risk of memory corruption. I think tracking shutdown in two places is overly > complex - if we track it here, should we also track it where we are posting to > the compositor or default task runners? What happens if we call shutdown from a > different entry point (e.g., within chrome)? WDYT? Ah TaskRunner is refcounted. My concern is moot.
https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:113: // and write access to policy_may_need_update_. On 2014/10/27 16:25:01, rmcilroy wrote: > On 2014/10/27 12:02:38, Sami wrote: > > Seems odd to say this just protects writes to |policy_may_need_update_|. Just > > protecting writes doesn't really buy you anything in terms of data correctness > > :) Since |policy_may_need_update_| is always read and written atomically with > > barriers I don't think it needs any lock protection. > > It does actually buy you something because we want want to avoid the following > situation: > - Thread 1 updates signals to X and sets policy_may_need_update_ to 1 > - Thread 0 runs updatePolicy, and reads signals X, then releases signals lock > and gets unscheduled > - Thread 1 updates signals to Y and sets policy_may_need_update_ to 1 > - Thread 0 gets rescheduled and updates policy based on signals X, then sets > policy_may_need_update_ to 0 > - Now the policy update based on signals Y has been lost. > > Subtle, but I think we should avoid this, and can do so if we protect write > access to policy_may_need_update_ with the lock and ensure we only update it at > the same time as we read the signals variables. Isn't the bug there that the may_need_update flag is cleared only after we read the new signals? If we did it the other way around the race you described shouldn't happen, right? https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:266: EXPECT_TRUE(task_run); On 2014/10/27 16:25:01, rmcilroy wrote: > On 2014/10/27 12:02:38, Sami wrote: > > Could you also check that the deadline was given to the task correctly? Could > > also be an independent test that just checks CurrentIdleTaskDeadline(). > > The deadline was checked in IdleTestTask. Moved the checking to here for > clarity. Ah, missed it. Probably good that you moved it here then. https://codereview.chromium.org/664963002/diff/200001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/664963002/diff/200001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_selector.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. Looks like the selector files got left in this patch. https://codereview.chromium.org/664963002/diff/200001/content/renderer/schedu... File content/renderer/scheduler/web_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/200001/content/renderer/schedu... content/renderer/scheduler/web_scheduler_impl.cc:27: task->run(deadline.ToInternalValue() / nit: These types of timestamps are usually converted like this, which is a tiny bit clearer: (deadline - base::TimeTicks()).InSecondsF();
https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:113: // and write access to policy_may_need_update_. On 2014/10/27 18:56:29, Sami wrote: > On 2014/10/27 16:25:01, rmcilroy wrote: > > On 2014/10/27 12:02:38, Sami wrote: > > > Seems odd to say this just protects writes to |policy_may_need_update_|. > Just > > > protecting writes doesn't really buy you anything in terms of data > correctness > > > :) Since |policy_may_need_update_| is always read and written atomically > with > > > barriers I don't think it needs any lock protection. > > > > It does actually buy you something because we want want to avoid the following > > situation: > > - Thread 1 updates signals to X and sets policy_may_need_update_ to 1 > > - Thread 0 runs updatePolicy, and reads signals X, then releases signals lock > > and gets unscheduled > > - Thread 1 updates signals to Y and sets policy_may_need_update_ to 1 > > - Thread 0 gets rescheduled and updates policy based on signals X, then sets > > policy_may_need_update_ to 0 > > - Now the policy update based on signals Y has been lost. > > > > Subtle, but I think we should avoid this, and can do so if we protect write > > access to policy_may_need_update_ with the lock and ensure we only update it > at > > the same time as we read the signals variables. > > Isn't the bug there that the may_need_update flag is cleared only after we read > the new signals? If we did it the other way around the race you described > shouldn't happen, right? The opposite can happen when the policy_may_need_update is set to 1 before the input is updated (as happens now in DidReceiveInputEvent - e.g.: - Thread 1 sets policy_may_need_update_ to 1 and gets descheduled - Thread 0 runs updatePolicy and sets policy_may_need_update_ to 0 and updates policy based on old signals X - Thread 1 gets rescheduled and updates signals to Y The update of the signals to Y now get lost due to this ordering. I think relying on the ordering guarantees of when may_need_update is set in relation to the input signals is really subtle and would lead to broken assumptions down the road as we add more signals. I would much prefer that we just protect the writes to this flag with the lock, which we need to hold in order to update the input signals in any case. WDYT? https://codereview.chromium.org/664963002/diff/200001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_selector.h (right): https://codereview.chromium.org/664963002/diff/200001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_selector.h:1: // Copyright 2014 The Chromium Authors. All rights reserved. On 2014/10/27 18:56:29, Sami wrote: > Looks like the selector files got left in this patch. Opps, bad upload - Done. https://codereview.chromium.org/664963002/diff/200001/content/renderer/schedu... File content/renderer/scheduler/web_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/200001/content/renderer/schedu... content/renderer/scheduler/web_scheduler_impl.cc:27: task->run(deadline.ToInternalValue() / On 2014/10/27 18:56:29, Sami wrote: > nit: These types of timestamps are usually converted like this, which is a tiny > bit clearer: > > (deadline - base::TimeTicks()).InSecondsF(); Done.
https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:113: // and write access to policy_may_need_update_. On 2014/10/28 18:26:53, rmcilroy wrote: > On 2014/10/27 18:56:29, Sami wrote: > > On 2014/10/27 16:25:01, rmcilroy wrote: > > > On 2014/10/27 12:02:38, Sami wrote: > > > > Seems odd to say this just protects writes to |policy_may_need_update_|. > > Just > > > > protecting writes doesn't really buy you anything in terms of data > > correctness > > > > :) Since |policy_may_need_update_| is always read and written atomically > > with > > > > barriers I don't think it needs any lock protection. > > > > > > It does actually buy you something because we want want to avoid the > following > > > situation: > > > - Thread 1 updates signals to X and sets policy_may_need_update_ to 1 > > > - Thread 0 runs updatePolicy, and reads signals X, then releases signals > lock > > > and gets unscheduled > > > - Thread 1 updates signals to Y and sets policy_may_need_update_ to 1 > > > - Thread 0 gets rescheduled and updates policy based on signals X, then > sets > > > policy_may_need_update_ to 0 > > > - Now the policy update based on signals Y has been lost. > > > > > > Subtle, but I think we should avoid this, and can do so if we protect write > > > access to policy_may_need_update_ with the lock and ensure we only update it > > at > > > the same time as we read the signals variables. > > > > Isn't the bug there that the may_need_update flag is cleared only after we > read > > the new signals? If we did it the other way around the race you described > > shouldn't happen, right? > > The opposite can happen when the policy_may_need_update is set to 1 before the > input is updated (as happens now in DidReceiveInputEvent - e.g.: > - Thread 1 sets policy_may_need_update_ to 1 and gets descheduled > - Thread 0 runs updatePolicy and sets policy_may_need_update_ to 0 and updates > policy based on old signals X > - Thread 1 gets rescheduled and updates signals to Y > The update of the signals to Y now get lost due to this ordering. > > I think relying on the ordering guarantees of when may_need_update is set in > relation to the input signals is really subtle and would lead to broken > assumptions down the road as we add more signals. I would much prefer that we > just protect the writes to this flag with the lock, which we need to hold in > order to update the input signals in any case. WDYT? Ah, good point. It's better to be explicit about these things, so let's use the lock for writes. I guess the unlocked access to current_policy_ is still safe because it only happens on the main thread, which is the only thread that writes to that member. If you haven't already taken this for a spin with TSAN, let's do that :)
On 2014/10/28 18:46:53, Sami wrote: > https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... > File content/renderer/scheduler/renderer_scheduler_impl.h (right): > > https://codereview.chromium.org/664963002/diff/160001/content/renderer/schedu... > content/renderer/scheduler/renderer_scheduler_impl.h:113: // and write access to > policy_may_need_update_. > On 2014/10/28 18:26:53, rmcilroy wrote: > > On 2014/10/27 18:56:29, Sami wrote: > > > On 2014/10/27 16:25:01, rmcilroy wrote: > > > > On 2014/10/27 12:02:38, Sami wrote: > > > > > Seems odd to say this just protects writes to |policy_may_need_update_|. > > > Just > > > > > protecting writes doesn't really buy you anything in terms of data > > > correctness > > > > > :) Since |policy_may_need_update_| is always read and written atomically > > > with > > > > > barriers I don't think it needs any lock protection. > > > > > > > > It does actually buy you something because we want want to avoid the > > following > > > > situation: > > > > - Thread 1 updates signals to X and sets policy_may_need_update_ to 1 > > > > - Thread 0 runs updatePolicy, and reads signals X, then releases signals > > lock > > > > and gets unscheduled > > > > - Thread 1 updates signals to Y and sets policy_may_need_update_ to 1 > > > > - Thread 0 gets rescheduled and updates policy based on signals X, then > > sets > > > > policy_may_need_update_ to 0 > > > > - Now the policy update based on signals Y has been lost. > > > > > > > > Subtle, but I think we should avoid this, and can do so if we protect > write > > > > access to policy_may_need_update_ with the lock and ensure we only update > it > > > at > > > > the same time as we read the signals variables. > > > > > > Isn't the bug there that the may_need_update flag is cleared only after we > > read > > > the new signals? If we did it the other way around the race you described > > > shouldn't happen, right? > > > > The opposite can happen when the policy_may_need_update is set to 1 before the > > input is updated (as happens now in DidReceiveInputEvent - e.g.: > > - Thread 1 sets policy_may_need_update_ to 1 and gets descheduled > > - Thread 0 runs updatePolicy and sets policy_may_need_update_ to 0 and > updates > > policy based on old signals X > > - Thread 1 gets rescheduled and updates signals to Y > > The update of the signals to Y now get lost due to this ordering. > > > > I think relying on the ordering guarantees of when may_need_update is set in > > relation to the input signals is really subtle and would lead to broken > > assumptions down the road as we add more signals. I would much prefer that we > > just protect the writes to this flag with the lock, which we need to hold in > > order to update the input signals in any case. WDYT? > > Ah, good point. It's better to be explicit about these things, so let's use the > lock for writes. I guess the unlocked access to current_policy_ is still safe > because it only happens on the main thread, which is the only thread that writes > to that member. > > If you haven't already taken this for a spin with TSAN, let's do that :) Done, I didn't see any warnings about races or errors when running with TSAN.
Thanks Ross. lgtm % a handful of nits. https://codereview.chromium.org/664963002/diff/220001/content/content_rendere... File content/content_renderer.gypi (right): https://codereview.chromium.org/664963002/diff/220001/content/content_rendere... content/content_renderer.gypi:380: 'renderer/scheduler_proxy_task_runner.h', Left in by mistake? This file doesn't exist anymore. https://codereview.chromium.org/664963002/diff/220001/content/renderer/render... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/664963002/diff/220001/content/renderer/render... content/renderer/renderer_blink_platform_impl.cc:262: renderer_scheduler_->DefaultTaskRunner()->PostTask(FROM_HERE, Note to self: we should make this Blink pass in a WebTraceLocation here. https://codereview.chromium.org/664963002/diff/220001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/220001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:127: control_task_runner_->PostTask(FROM_HERE, closure); nit: PostTask just calls PostDelayedTask with a zero delay, so we may as well do the same here. https://codereview.chromium.org/664963002/diff/220001/content/renderer/schedu... File content/renderer/scheduler/web_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/220001/content/renderer/schedu... content/renderer/scheduler/web_scheduler_impl.h:22: bool shouldYieldForHighPriorityWork() override; No overrides here -- just virtuals -- because WebScheduler is defined in Blink.
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
https://codereview.chromium.org/664963002/diff/240001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/240001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:86: void RendererSchedulerImpl::DidReceiveInputEvent() { I'm still not sure if all input events should be treated the same here, e.g., do we really want mousemoves to kick us into a different policy? I guess we can restrict it later on if we find it to be problematic, but at the least we should have a TODO and/or file a bug to monitor this.
Patchset #12 (id:240001) has been deleted
Patchset #12 (id:260001) has been deleted
rmcilroy@chromium.org changed reviewers: - jdduke@chromium.org
Thanks Sami! https://codereview.chromium.org/664963002/diff/220001/content/content_rendere... File content/content_renderer.gypi (right): https://codereview.chromium.org/664963002/diff/220001/content/content_rendere... content/content_renderer.gypi:380: 'renderer/scheduler_proxy_task_runner.h', On 2014/10/29 13:51:27, Sami wrote: > Left in by mistake? This file doesn't exist anymore. Opps yes, done. Thanks. https://codereview.chromium.org/664963002/diff/220001/content/renderer/render... File content/renderer/renderer_blink_platform_impl.cc (right): https://codereview.chromium.org/664963002/diff/220001/content/renderer/render... content/renderer/renderer_blink_platform_impl.cc:262: renderer_scheduler_->DefaultTaskRunner()->PostTask(FROM_HERE, On 2014/10/29 13:51:27, Sami wrote: > Note to self: we should make this Blink pass in a WebTraceLocation here. Acknowledged. https://codereview.chromium.org/664963002/diff/220001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/220001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:127: control_task_runner_->PostTask(FROM_HERE, closure); On 2014/10/29 13:51:27, Sami wrote: > nit: PostTask just calls PostDelayedTask with a zero delay, so we may as well do > the same here. Done. https://codereview.chromium.org/664963002/diff/220001/content/renderer/schedu... File content/renderer/scheduler/web_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/220001/content/renderer/schedu... content/renderer/scheduler/web_scheduler_impl.h:22: bool shouldYieldForHighPriorityWork() override; On 2014/10/29 13:51:27, Sami wrote: > No overrides here -- just virtuals -- because WebScheduler is defined in Blink. Done.
rmcilroy@chromium.org changed reviewers: + sievers@chromium.org - alexclarke@google.com, eseidel@chromium.org, petrcermak@chromium.org, picksi@chromium.org
Daniel, could you please take a look at this CL. If follows on from the RendererTaskQueueSelector CL, implementing the RendererScheduler. Thanks.
On 2014/10/29 14:54:29, jdduke wrote: > I'm still not sure if all input events should be treated the same here, e.g., do > we really want mousemoves to kick us into a different policy? I guess we can > restrict it later on if we find it to be problematic, but at the least we should > have a TODO and/or file a bug to monitor this. Right, this is probably a good first approximation but we could probably make it smarter. I'm not sure what the right heuristic would be -- even mousemoves are pretty important for FPSs and drawing apps.
On 2014/10/29 15:23:05, Sami wrote: > On 2014/10/29 14:54:29, jdduke wrote: > > I'm still not sure if all input events should be treated the same here, e.g., > do > > we really want mousemoves to kick us into a different policy? I guess we can > > restrict it later on if we find it to be problematic, but at the least we > should > > have a TODO and/or file a bug to monitor this. > > Right, this is probably a good first approximation but we could probably make it > smarter. I'm not sure what the right heuristic would be -- even mousemoves are > pretty important for FPSs and drawing apps. Yeah, differentiating is a tricky task, but idle, passive mouse movement is probably a far more common scenario in desktop environments.
sievers@chromium.org changed reviewers: + brianderson@chromium.org
+brianderson
https://codereview.chromium.org/664963002/diff/280001/content/content_rendere... File content/content_renderer.gypi (right): https://codereview.chromium.org/664963002/diff/280001/content/content_rendere... content/content_renderer.gypi:374: 'renderer/scheduler/single_thread_idle_task_runner.cc', ultra-nit: .cc before .h https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:53: RendererSchedulerImpl::DefaultTaskRunner() { What about these task runner getter methods being called from another thread while we are shutting down on the main thread? https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:69: if (!task_queue_manager_) Do we really expect this after shutdown? Should it be DCHECK()? Same for other calls below. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:87: if (!task_queue_manager_) This is accessing task_queue_manager_ without a mutex. But more importantly how will we make sure that this doesn't get called from the IO thread when the scheduler is about to or has already shut down? https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:115: UpdatePolicy(); Why do we need to do this here? We know that if it needed an update we already scheduled a task to update it. Also, you could use base::CancellationFlag to hide the 'subtle' stuff here. But am wondering why we need it at all. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:137: base::TimeDelta compositor_priority_duration = nit: indent https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:141: if (remaining_compositor_priority_duration > base::TimeDelta()) { nit: can you write instead: base::TimeTicks priority_end(last_input_time_ + compositor_priority_duration); base::TimeTicks now(Now()); if (priority_end > now) { new_policy = ... Post(priority_end - now); } https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:22: RendererSchedulerImpl(); is this constructor needed? https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:23: ~RendererSchedulerImpl() override; nit: can this be protected? https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:43: DISALLOW_COPY_AND_ASSIGN(RendererSchedulerImpl); nit: make this the last thing in the class (style-guide) https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:62: static const int kCompositorPriorityAfterTouchMillis = 100; So this covers multiple frames. So we are basically always in compositor prio mode while there is some sort of touch interaction going on? https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:113: if (*run_count == 0) nit: braces https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:202: EXPECT_EQ(1, run_count); How many idle tasks is it expected to run? So it doesn't run the nested task (posted from when we were idle) but what if we initially posted two tasks? https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.cc (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.cc:36: if (deadline_supplier_) { Do we really want to cancel the task when the deadline supplier goes away? Or should it not happen that a task is run when that was the case and we DCHECK() instead? https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.h (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:49: class IdleTaskDeadlineSupplier { Can this be a CancelableCallback? https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/web_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/web_scheduler_impl.cc:35: location, base::Bind(&WebSchedulerImpl::runIdleTask, base::Owned(task))); would it work to be paranoid and use base::Passed(task) here and receive a scoped_ptr in runIdleTask so that really (as long as the task is run) nobody can hold another ref to the bound state? https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/web_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/web_scheduler_impl.h:22: virtual bool shouldYieldForHighPriorityWork(); 'override' or 'final' for these methods
https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:53: RendererSchedulerImpl::DefaultTaskRunner() { On 2014/10/30 23:40:51, sievers wrote: > What about these task runner getter methods being called from another thread > while we are shutting down on the main thread? If a task gets posted during shut down, then it will go onto the queue but will never get executed. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:115: UpdatePolicy(); On 2014/10/30 23:40:51, sievers wrote: > Why do we need to do this here? We know that if it needed an update we already > scheduled a task to update it. > Also, you could use base::CancellationFlag to hide the 'subtle' stuff here. But > am wondering why we need it at all. The motivation for this is to let ShouldYieldForHighPriorityWork() return true as fast as possible in the following scenario: The scheduler is in NORMAL_PRIORITY_POLICY and while the Blink platform timers are executing, the user touches the screen. In that situation we want ShouldYieldForHighPriorityWork (which is polled by the blink platform timer loop) to return true, hence we update the policy here if the policy_may_need_update_ dirty flag is set. The use of subtle is an optimization to try and avoid locking because this function can get called a lot. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:62: static const int kCompositorPriorityAfterTouchMillis = 100; On 2014/10/30 23:40:51, sievers wrote: > So this covers multiple frames. So we are basically always in compositor prio > mode while there is some sort of touch interaction going on? Yes the idea is we want smooth and responsive scrolling while the user is touching or during a fling. The 100ms duration is a number I pulled out of thin air, but it seems to behave reasonably.
Thanks for the review comment's Daniel. Updated based on your comments, PTAL, thanks. https://codereview.chromium.org/664963002/diff/280001/content/content_rendere... File content/content_renderer.gypi (right): https://codereview.chromium.org/664963002/diff/280001/content/content_rendere... content/content_renderer.gypi:374: 'renderer/scheduler/single_thread_idle_task_runner.cc', On 2014/10/30 23:40:51, sievers wrote: > ultra-nit: .cc before .h Good catch! Done. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:53: RendererSchedulerImpl::DefaultTaskRunner() { On 2014/10/30 23:40:51, sievers wrote: > What about these task runner getter methods being called from another thread > while we are shutting down on the main thread? This should be fine - the task_runners are ref_counted so will stay alive even as we are shutting down, and internally contain a weak pointer to the TaskQueueManager (see task_queue_manager.cc), which is checked during posting, so if we have shutdown the task_runner will just drop the posted tasks. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:69: if (!task_queue_manager_) On 2014/10/30 23:40:51, sievers wrote: > Do we really expect this after shutdown? Should it be DCHECK()? Same for other > calls below. Unfortunately we can still get called by the compositor while Blink is shutting down because it doesn't seem to be guaranteed that the compositor thread is dead by the point Blink shuts down, so I don't think we can just DCHECK() here. http://crbug.com/415758 describes an issue due to this. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:87: if (!task_queue_manager_) On 2014/10/30 23:40:51, sievers wrote: > This is accessing task_queue_manager_ without a mutex. > But more importantly how will we make sure that this doesn't get called from the > IO thread when the scheduler is about to or has already shut down? Very good point. I've removed the check entirely because we never actually access the task_queue_manager_ from this function, and if we have shutdown then the control_task_runner_ will just silently drop the UpdatePolicy task we post here. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:115: UpdatePolicy(); On 2014/10/30 23:40:51, sievers wrote: > Why do we need to do this here? We know that if it needed an update we already > scheduled a task to update it. > Also, you could use base::CancellationFlag to hide the 'subtle' stuff here. But > am wondering why we need it at all. We need this due to the ShouldYieldForHighPriorityWork() - this is called by long running tasks to check whether they should yield. We want a policy change to cause this to return 'true' as soon as possible if we need to move to compositor priority. The UpdatePolicy task can only be run once the current task has completed, so we wouldn't be able to signal to the long-running task to yield if we didn't use this flag to enable short-circuiting. I've added a test (TestShouldYield) to check for this. base::CancellationFlag almost works, however we need to be able to set it from multiple threads (with a lock) and read it from one thread (CancellationFlag is the opposite). I created a PolicyNeedsUpdateFlag class which encapsulates the behavior we need - WDYT? https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:137: base::TimeDelta compositor_priority_duration = On 2014/10/30 23:40:51, sievers wrote: > nit: indent I'm not sure what's wrongly indented here? https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:141: if (remaining_compositor_priority_duration > base::TimeDelta()) { On 2014/10/30 23:40:51, sievers wrote: > nit: can you write instead: > > base::TimeTicks priority_end(last_input_time_ + compositor_priority_duration); > base::TimeTicks now(Now()); > if (priority_end > now) { > new_policy = ... > Post(priority_end - now); > } Done. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:22: RendererSchedulerImpl(); On 2014/10/30 23:40:51, sievers wrote: > is this constructor needed? Good point, replaced it with the previously protected one. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:23: ~RendererSchedulerImpl() override; On 2014/10/30 23:40:51, sievers wrote: > nit: can this be protected? Unfortunately not since we wrap it in a scoped_ptr https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:43: DISALLOW_COPY_AND_ASSIGN(RendererSchedulerImpl); On 2014/10/30 23:40:51, sievers wrote: > nit: make this the last thing in the class (style-guide) Done. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:62: static const int kCompositorPriorityAfterTouchMillis = 100; On 2014/10/30 23:40:51, sievers wrote: > So this covers multiple frames. So we are basically always in compositor prio > mode while there is some sort of touch interaction going on? Yes that is the idea, we try to stay in compositor mode while touch events are going on. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl_unittest.cc (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:113: if (*run_count == 0) On 2014/10/30 23:40:51, sievers wrote: > nit: braces Done. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl_unittest.cc:202: EXPECT_EQ(1, run_count); On 2014/10/30 23:40:51, sievers wrote: > How many idle tasks is it expected to run? So it doesn't run the nested task > (posted from when we were idle) but what if we initially posted two tasks? If we posted two tasks then we would expect both to be run, unless the first task used up the whole deadline. I've added a test for this below. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.cc (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.cc:36: if (deadline_supplier_) { On 2014/10/30 23:40:51, sievers wrote: > Do we really want to cancel the task when the deadline supplier goes away? > Or should it not happen that a task is run when that was the case and we > DCHECK() instead? Yes we should probably DCHECK - done. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.h (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:49: class IdleTaskDeadlineSupplier { On 2014/10/30 23:40:51, sievers wrote: > Can this be a CancelableCallback? I don't thinks so. We need to be able to pass the task an actual deadline which it should work until (e.g., the V8 GC idle task will use this deadline to estimate how much marking it can do before the deadline), not just be able to cancel it. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/web_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/web_scheduler_impl.h:22: virtual bool shouldYieldForHighPriorityWork(); On 2014/10/30 23:40:51, sievers wrote: > 'override' or 'final' for these methods I had this originally however Sami commented that the interface between Blink and Chromium should only have 'virtual' to avoid breakage when changing the APIs in different repos.
On 2014/10/29 15:28:23, jdduke wrote: > On 2014/10/29 15:23:05, Sami wrote: > > On 2014/10/29 14:54:29, jdduke wrote: > > > I'm still not sure if all input events should be treated the same here, > e.g., > > do > > > we really want mousemoves to kick us into a different policy? I guess we can > > > restrict it later on if we find it to be problematic, but at the least we > > should > > > have a TODO and/or file a bug to monitor this. > > > > Right, this is probably a good first approximation but we could probably make > it > > smarter. I'm not sure what the right heuristic would be -- even mousemoves are > > pretty important for FPSs and drawing apps. > > Yeah, differentiating is a tricky task, but idle, passive mouse movement is > probably a far more common scenario in desktop environments. Sorry, meant to reply to this earlier - I've added a TODO and crbug.com/429814 https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/web_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/web_scheduler_impl.cc:35: location, base::Bind(&WebSchedulerImpl::runIdleTask, base::Owned(task))); On 2014/10/30 23:40:51, sievers wrote: > would it work to be paranoid and use base::Passed(task) here and receive a > scoped_ptr in runIdleTask so that really (as long as the task is run) nobody can > hold another ref to the bound state? Done (missed replying to this comment last time).
jdduke@chromium.org changed reviewers: + jdduke@chromium.org
Thanks for the TODO (and sorry for the final drive-by...). https://codereview.chromium.org/664963002/diff/340001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/340001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:82: weak_renderer_scheduler_ptr_), See below (wondering if caching the closure provides any benefit here). https://codereview.chromium.org/664963002/diff/340001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:123: base::Closure closure = base::Bind(&RendererSchedulerImpl::UpdatePolicy, It looks like |weak_renderer_scheduler_ptr_| never changes. I don't know enough about the details of our Callback implementation, but would we benefit from caching the closure and not regenerating for every update (i.e., can we avoid a heap alloc here?)? https://codereview.chromium.org/664963002/diff/340001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:153: if (new_policy == current_policy_) { Nit: No braces necessary.
https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:53: RendererSchedulerImpl::DefaultTaskRunner() { But these getters should call main_thread_checker_.CalledOnValidThread(). You can't access |this| from another thread because |this| might get deleted while you are calling into it which will crash. Everybody needs to acquire a reference to the task runners from the main thread and then hold on to it. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:115: UpdatePolicy(); On 2014/11/03 19:02:52, rmcilroy wrote: > On 2014/10/30 23:40:51, sievers wrote: > > Why do we need to do this here? We know that if it needed an update we already > > scheduled a task to update it. > > Also, you could use base::CancellationFlag to hide the 'subtle' stuff here. > But > > am wondering why we need it at all. > > We need this due to the ShouldYieldForHighPriorityWork() - this is called by > long running tasks to check whether they should yield. We want a policy change > to cause this to return 'true' as soon as possible if we need to move to > compositor priority. The UpdatePolicy task can only be run once the current > task has completed, so we wouldn't be able to signal to the long-running task to > yield if we didn't use this flag to enable short-circuiting. I've added a test > (TestShouldYield) to check for this. > > base::CancellationFlag almost works, however we need to be able to set it from > multiple threads (with a lock) and read it from one thread (CancellationFlag is > the opposite). I created a PolicyNeedsUpdateFlag class which encapsulates the > behavior we need - WDYT? Shouldn't it also yield then for any control task in general? Could we just do this: bool RendererSchedulerImpl::ShouldYieldForHighPriorityWork() { ... return !task_queue_manager_->IsQueueEmpty(CONTROL_TASK_QUEUE) || (SchedulerPolicy() == COMPOSITOR_PRIORITY_POLICY && !task_queue_manager_->IsQueueEmpty(COMPOSITOR_TASK_QUEUE)); } https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.cc (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.cc:36: if (deadline_supplier_) { On 2014/11/03 19:02:53, rmcilroy wrote: > On 2014/10/30 23:40:51, sievers wrote: > > Do we really want to cancel the task when the deadline supplier goes away? > > Or should it not happen that a task is run when that was the case and we > > DCHECK() instead? > > Yes we should probably DCHECK - done. Actually based on other comments it seems like RendererSchedulerImpl (the deadline supplier) might go away, while another thread is still holding a ref to this task runner. So no DCHECK() then? What about the task? Run it without a deadline, or drop it? https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.h (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:49: class IdleTaskDeadlineSupplier { On 2014/11/03 19:02:53, rmcilroy wrote: > On 2014/10/30 23:40:51, sievers wrote: > > Can this be a CancelableCallback? > > I don't thinks so. We need to be able to pass the task an actual deadline which > it should work until (e.g., the V8 GC idle task will use this deadline to > estimate how much marking it can do before the deadline), not just be able to > cancel it. I meant just to avoid needing the interface class. The callback itself is of type Callback<base::TimeTicks()> and returns the current deadline. It's just cancelable (from RendererSchedulerImpl) to handle the scheduler impl going away.
Also moved ownership of renderer_scheduler from renderer_blink_platform_impl to render_thread_impl. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:53: RendererSchedulerImpl::DefaultTaskRunner() { On 2014/11/03 22:19:01, sievers wrote: > But these getters should call main_thread_checker_.CalledOnValidThread(). > > You can't access |this| from another thread because |this| might get deleted > while you are calling into it which will crash. > Everybody needs to acquire a reference to the task runners from the main thread > and then hold on to it. Yes, these getters should call main_thread_checker_ - the clients all do currently get the task runners on the main thread and hold onto it, I had just forgotton to add the checks here. Added main_thread_checker_.CalledOnValidThread() (and also in Shutdown which should also only be called from the main thread). https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:115: UpdatePolicy(); On 2014/11/03 22:19:01, sievers wrote: > On 2014/11/03 19:02:52, rmcilroy wrote: > > On 2014/10/30 23:40:51, sievers wrote: > > > Why do we need to do this here? We know that if it needed an update we > already > > > scheduled a task to update it. > > > Also, you could use base::CancellationFlag to hide the 'subtle' stuff here. > > But > > > am wondering why we need it at all. > > > > We need this due to the ShouldYieldForHighPriorityWork() - this is called by > > long running tasks to check whether they should yield. We want a policy change > > to cause this to return 'true' as soon as possible if we need to move to > > compositor priority. The UpdatePolicy task can only be run once the current > > task has completed, so we wouldn't be able to signal to the long-running task > to > > yield if we didn't use this flag to enable short-circuiting. I've added a > test > > (TestShouldYield) to check for this. > > > > base::CancellationFlag almost works, however we need to be able to set it from > > multiple threads (with a lock) and read it from one thread (CancellationFlag > is > > the opposite). I created a PolicyNeedsUpdateFlag class which encapsulates the > > behavior we need - WDYT? > > Shouldn't it also yield then for any control task in general? > Could we just do this: > > bool RendererSchedulerImpl::ShouldYieldForHighPriorityWork() { > ... > return !task_queue_manager_->IsQueueEmpty(CONTROL_TASK_QUEUE) > || (SchedulerPolicy() == COMPOSITOR_PRIORITY_POLICY && > !task_queue_manager_->IsQueueEmpty(COMPOSITOR_TASK_QUEUE)); > } > As discussed offline, this doesn't quite give us what we want because: in the future we might end up posting a low-prio policy update on the control task queue which we don't want to force tasks to yield for; and IsQueueEmpty requires locking if the queue is empty so we don't want to do it in the normal case. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.cc (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.cc:36: if (deadline_supplier_) { On 2014/11/03 22:19:01, sievers wrote: > On 2014/11/03 19:02:53, rmcilroy wrote: > > On 2014/10/30 23:40:51, sievers wrote: > > > Do we really want to cancel the task when the deadline supplier goes away? > > > Or should it not happen that a task is run when that was the case and we > > > DCHECK() instead? > > > > Yes we should probably DCHECK - done. > > Actually based on other comments it seems like RendererSchedulerImpl (the > deadline supplier) might go away, while another thread is still holding a ref to > this task runner. So no DCHECK() then? What about the task? Run it without a > deadline, or drop it? The RendererSchedulerImpl could go away while another thread is holding a ref to this task runner - we need to make PostIdleTask able to deal with RendererSchedulerImpl having gone away, however RunTask should never be called if RendererSchedulerImpl has gone away, so it should be fine to DCHECK here. https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.h (right): https://codereview.chromium.org/664963002/diff/280001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:49: class IdleTaskDeadlineSupplier { On 2014/11/03 22:19:01, sievers wrote: > On 2014/11/03 19:02:53, rmcilroy wrote: > > On 2014/10/30 23:40:51, sievers wrote: > > > Can this be a CancelableCallback? > > > > I don't thinks so. We need to be able to pass the task an actual deadline > which > > it should work until (e.g., the V8 GC idle task will use this deadline to > > estimate how much marking it can do before the deadline), not just be able to > > cancel it. > > I meant just to avoid needing the interface class. > The callback itself is of type Callback<base::TimeTicks()> and returns the > current deadline. > It's just cancelable (from RendererSchedulerImpl) to handle the scheduler impl > going away. Ahh I see what you mean, good idea. Done, but using a non cancelable callback with a weakref 'this' parameter since this will implicitly cancel the callback if the weakptr becomes invalid (see https://code.google.com/p/chromium/codesearch#chromium/src/base/bind_internal...). https://codereview.chromium.org/664963002/diff/340001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/340001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:82: weak_renderer_scheduler_ptr_), On 2014/11/03 21:33:48, jdduke wrote: > See below (wondering if caching the closure provides any benefit here). Done. https://codereview.chromium.org/664963002/diff/340001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:123: base::Closure closure = base::Bind(&RendererSchedulerImpl::UpdatePolicy, On 2014/11/03 21:33:48, jdduke wrote: > It looks like |weak_renderer_scheduler_ptr_| never changes. I don't know enough > about the details of our Callback implementation, but would we benefit from > caching the closure and not regenerating for every update (i.e., can we avoid a > heap alloc here?)? Yes, it looks like caching the closure would avoid at least one heap alloc here - done. https://codereview.chromium.org/664963002/diff/340001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:153: if (new_policy == current_policy_) { On 2014/11/03 21:33:48, jdduke wrote: > Nit: No braces necessary. Done.
Patchset #16 (id:360001) has been deleted
https://codereview.chromium.org/664963002/diff/380001/content/public/test/ren... File content/public/test/render_view_test.cc (right): https://codereview.chromium.org/664963002/diff/380001/content/public/test/ren... content/public/test/render_view_test.cc:77: new RendererBlinkPlatformImplNoSandboxImpl(renderer_scheduler_.get())); indent += 2 (or git cl format :) https://codereview.chromium.org/664963002/diff/380001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/664963002/diff/380001/content/renderer/render... content/renderer/render_thread_impl.cc:858: renderer_scheduler()->CompositorTaskRunner(); indent += 2 https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:176: renderer_task_queue_selector_->EnableQueue( nit: add a main thread check here. https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:181: void RendererSchedulerImpl::EndIdlePeriod() { nit: add a main thread check here. https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:19: namespace { It seems a little weird to have an anonymous namespace in a header like this. I think that means that each .cc that includes this will have their own copy of this class definition. It might be better to call the namespace "internal" instead. https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:21: class PolicyNeedsUpdateFlag { Is it worth calling this something like NeedsUpdateFlag (or maybe PollableNeedsUpdateFlag) since it's not really policy related?
https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler.h (right): https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.h:10: #include "base/threading/thread_checker.h" nit: are the top 3 includes not needed? https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.h:47: virtual void DidReceiveInputEvent() = 0; Should this maybe just be called DidReceiveInputEventOnCompositorThread() to make things really clear? Especially since it's the only member function that's called on another thread. https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.h:54: // Shuts down the scheduler by draining any remaining pending work in the work Is the 'draining' part true? Doesn't look like that would happen during a call to ShutDown() if at all. https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:121: main_thread_checker_.CalledOnValidThread(); How about inlining this code into ShouldYieldForHighPriorityWork(), since the optimization to update the policy is very specific to that one and only call site anyways? https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:19: namespace { On 2014/11/04 17:46:35, Sami wrote: > It seems a little weird to have an anonymous namespace in a header like this. I > think that means that each .cc that includes this will have their own copy of > this class definition. It might be better to call the namespace "internal" > instead. Yea, can this just be a nested private class in RendererSchedulerImpl? https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.h (right): https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:11: #include "base/memory/weak_ptr.h" nit: unneeded https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:44: base::Callback<void(base::TimeTicks*)> deadline_supplier_; nit: DISALLOW_COPY_AND_ASSIGN
https://codereview.chromium.org/664963002/diff/380001/content/public/test/ren... File content/public/test/render_view_test.cc (right): https://codereview.chromium.org/664963002/diff/380001/content/public/test/ren... content/public/test/render_view_test.cc:77: new RendererBlinkPlatformImplNoSandboxImpl(renderer_scheduler_.get())); On 2014/11/04 17:46:35, Sami wrote: > indent += 2 (or git cl format :) Done. https://codereview.chromium.org/664963002/diff/380001/content/renderer/render... File content/renderer/render_thread_impl.cc (right): https://codereview.chromium.org/664963002/diff/380001/content/renderer/render... content/renderer/render_thread_impl.cc:858: renderer_scheduler()->CompositorTaskRunner(); On 2014/11/04 17:46:35, Sami wrote: > indent += 2 Done. https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler.h (right): https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.h:10: #include "base/threading/thread_checker.h" On 2014/11/04 20:41:41, sievers wrote: > nit: are the top 3 includes not needed? Done https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.h:47: virtual void DidReceiveInputEvent() = 0; On 2014/11/04 20:41:41, sievers wrote: > Should this maybe just be called DidReceiveInputEventOnCompositorThread() to > make things really clear? > Especially since it's the only member function that's called on another thread. Done. https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.h:54: // Shuts down the scheduler by draining any remaining pending work in the work On 2014/11/04 20:41:41, sievers wrote: > Is the 'draining' part true? Doesn't look like that would happen during a call > to ShutDown() if at all. By deleting the task_queue_manager this means that the remaining tasks in the task queues are also deleted and won't be run after this point. This is what I mean by draining the queue (i.e., they get dropped) - is this clear or would you like me to reword the comment to make it clearer what exactly happens here? https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:121: main_thread_checker_.CalledOnValidThread(); On 2014/11/04 20:41:41, sievers wrote: > How about inlining this code into ShouldYieldForHighPriorityWork(), since the > optimization to update the policy is very specific to that one and only call > site anyways? OK I guess this would work, I moved it to MaybeUpdateSchedulerPolicy and call that from ShouldYield rather than inlining directly. https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:176: renderer_task_queue_selector_->EnableQueue( On 2014/11/04 17:46:35, Sami wrote: > nit: add a main thread check here. Done. https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:181: void RendererSchedulerImpl::EndIdlePeriod() { On 2014/11/04 17:46:35, Sami wrote: > nit: add a main thread check here. Done. https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:19: namespace { On 2014/11/04 20:41:41, sievers wrote: > On 2014/11/04 17:46:35, Sami wrote: > > It seems a little weird to have an anonymous namespace in a header like this. > I > > think that means that each .cc that includes this will have their own copy of > > this class definition. It might be better to call the namespace "internal" > > instead. > > Yea, can this just be a nested private class in RendererSchedulerImpl? Good point - went with private class. https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:21: class PolicyNeedsUpdateFlag { On 2014/11/04 17:46:35, Sami wrote: > Is it worth calling this something like NeedsUpdateFlag (or maybe > PollableNeedsUpdateFlag) since it's not really policy related? Done. https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... File content/renderer/scheduler/single_thread_idle_task_runner.h (right): https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:11: #include "base/memory/weak_ptr.h" On 2014/11/04 20:41:41, sievers wrote: > nit: unneeded Done. https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/single_thread_idle_task_runner.h:44: base::Callback<void(base::TimeTicks*)> deadline_supplier_; On 2014/11/04 20:41:41, sievers wrote: > nit: DISALLOW_COPY_AND_ASSIGN Done.
lgtm https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler.h (right): https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.h:54: // Shuts down the scheduler by draining any remaining pending work in the work On 2014/11/05 00:34:00, rmcilroy wrote: > On 2014/11/04 20:41:41, sievers wrote: > > Is the 'draining' part true? Doesn't look like that would happen during a call > > to ShutDown() if at all. > > By deleting the task_queue_manager this means that the remaining tasks in the > task queues are also deleted and won't be run after this point. This is what I > mean by draining the queue (i.e., they get dropped) - is this clear or would you > like me to reword the comment to make it clearer what exactly happens here? I'd change it to say that it's dropping/discarding any pending tasks then. Draining sounds a bit like they get run synchronously here. https://codereview.chromium.org/664963002/diff/400001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/400001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:110: MaybeUpdatePolicy(); nit: Put a comment here explaining that we do want to yield for compositor tasks (if we are transitioning to compositor priority mode), but not for control tasks. I find it a bit confusing because usually control has higher prio than compositor.
Thanks Daniel. https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler.h (right): https://codereview.chromium.org/664963002/diff/380001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler.h:54: // Shuts down the scheduler by draining any remaining pending work in the work On 2014/11/05 21:18:57, sievers wrote: > On 2014/11/05 00:34:00, rmcilroy wrote: > > On 2014/11/04 20:41:41, sievers wrote: > > > Is the 'draining' part true? Doesn't look like that would happen during a > call > > > to ShutDown() if at all. > > > > By deleting the task_queue_manager this means that the remaining tasks in the > > task queues are also deleted and won't be run after this point. This is what > I > > mean by draining the queue (i.e., they get dropped) - is this clear or would > you > > like me to reword the comment to make it clearer what exactly happens here? > > I'd change it to say that it's dropping/discarding any pending tasks then. > Draining sounds a bit like they get run synchronously here. Done. https://codereview.chromium.org/664963002/diff/400001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/664963002/diff/400001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:110: MaybeUpdatePolicy(); On 2014/11/05 21:18:57, sievers wrote: > nit: Put a comment here explaining that we do want to yield for compositor tasks > (if we are transitioning to compositor priority mode), but not for control > tasks. I find it a bit confusing because usually control has higher prio than > compositor. Done.
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/664963002/420001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/664963002/420001
Message was sent while issue was closed.
Committed patchset #18 (id:420001)
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/321f924d52091bd19cbdf7397fb0764a574741ee Cr-Commit-Position: refs/heads/master@{#302929} |
