Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
968073003:
[content]: Add support for long idle times in the Blink Scheduler. (Closed)
|
Created:
5 years, 9 months ago by rmcilroy Modified:
5 years, 9 months ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, jam, scheduler-bugs_chromium.org, mlamouri+watch-content_chromium.org, jochen (gone - plz use gerrit), Hannes Payer (out of office), kouhei (in TOK), haraken Base URL:
https://chromium.googlesource.com/chromium/src.git@long_idle_4 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[content]: Add support for long idle times in the Blink Scheduler.
Adds support for long idle times in the Blink Scheduler.
A long idle time is initiated
if there are no frames being drawn and we are not in TOUCHSTART policy. Each
long idle period will be a maximum of 50 milliseconds or the time to the next
pending delayed task (whichever is sooner). Long idle periods will continually
be scheduled until a call to EndIdlePeriod is made. If there are no pending
tasks in the idle queue then the callback used to initate the next long idle
task will be posted on the control_task_after_wakeup_runner_ task runner, thus
ensuring that it won't cause the scheduler to wake up until another task
could have posted more idle work.
Currently, the long idle tasks are not enabled due to crbug.com/467655 -
once this is fixed they will be enabled.
Long idle task design doc:
https://docs.google.com/a/chromium.org/document/d/1yBlUdYW8VTIfB-DqhvQqUeP0kf-Ap1W4cao2yQq58Do/edit
BUG=455713, 467655
Committed: https://crrev.com/143bfac301e4306a3c5043f44d92381811030a91
Cr-Commit-Position: refs/heads/master@{#320785}
Patch Set 1 #Patch Set 2 : Minor tweaks #
Total comments: 46
Patch Set 3 : Review comments #
Total comments: 8
Patch Set 4 : Review comments #Patch Set 5 : fix missing comment #
Total comments: 2
Patch Set 6 : Add parameter to EndIdlePeriod. #
Total comments: 3
Patch Set 7 : Include IdlePeriodState changes and ENDING_IDLE_PERIOD state. #
Total comments: 8
Patch Set 8 : Final nits. #Patch Set 9 : Rebased #Patch Set 10 : Disable idle periods to allow landing while http://crbug.com/467655 is fixed #Patch Set 11 : Enable for testing #
Messages
Total messages: 43 (15 generated)
rmcilroy@chromium.org changed reviewers: + skyostil@chromium.org
Sami: PTAL, thanks.
Overall looks like the way to do things. Had a few comments about the organization. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:340: DCHECK(main_thread_checker_.CalledOnValidThread()); DCHECK_NE(input_stream_state_, InputStreamState::INACTIVE); https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:343: base::TimeDelta escalate_priority_duration = nit: escalated_priority_duration https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:397: if (current_policy_ == Policy::TOUCHSTART_PRIORITY) { We have a comment in the .h saying we should use SchedulerPolicy() instead of accessing this directly but I can't remember why :) https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:401: *next_long_idle_period_delay_out = TimeLeftInInputEscalatedPolicy(); Instead of recomputing this here, how about we maintain a current_policy_expiration_time_ in UpdatePolicy()? That we we don't need to know much about how TOUCHSTART_PRIORITY works here other than checking that it's active. Should also cut down on the conversions between deadlines and durations. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:412: long_idle_period_duration = std::min( Dumb question: why aren't we doing this adjustment for short idle tasks? https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:422: base::TimeDelta::FromMilliseconds(1); Instead of this, could we schedule a check after the next wakeup? Presumably there is guaranteed to be one since a delayed task is about to go off? https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:429: "AfterWakeupInitiateLongIdlePeriod"); Trace event doesn't match the function name. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:436: base::TimeDelta next_long_idle_period_delay_out; nit: It feels weird to call a stack variable "_out". https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:465: TRACE_EVENT_ASYNC_END_WITH_TIMESTAMP0( Will we get duplicate trace events from this if InitiateLongIdlePeriod() calls EndIdlePeriod() again. Maybe we should keep track of whether we are in an idle period or not? https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:135: base::TimeDelta TimeLeftInInputEscalatedPolicy(); Can this be const? https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:155: const base::TimeTicks& now, nit: pass TimeTicks by value. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:160: void AfterWakeupInitiateLongIdlePeriod(); InitiateLongIdlePeriodAfterWakeup to make this less yoda? https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.h (right): https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.h:101: base::TimeTicks NextPendingDelayedTask(); NextPendingDelayedTaskRunTime since we're not returning the actual task here?
picksi@google.com changed reviewers: + picksi@google.com
Some nit picks and bike-shedding! https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:346: if (last_input_process_time_on_main_.is_null() && In the 'else' section you also check last_input_receipt_time_on_compositor_, do you need to check this here too? https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:350: time_left_in_policy = escalate_priority_duration; code style question: I assume we prefer a single return at the end of the function, rather than multiple early outs with values? This function would be quite a bit shorter if we returned early. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:355: last_input_process_time_on_main_) + Philosophy: It makes my palms slightly itchy to see last_input_process_time_on_main_ being used as a int (or whatever it is) and also as a flag (in the code above where an is_null() check is used). Is it worth the code bloat that would result in adding a bool (etc) to flag whatever situation is being checked for by the is_null() check? https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:422: base::TimeDelta::FromMilliseconds(1); Is there any logic behind the 1 ms duration that we can clarify here? Also, you could probably have a well named const instead of the comment. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:199: void TaskQueue::GetNextPendingDelayedTaskLocked( Can this deal better with the delayed_task_run_times being empty? It's silent "failure" could lead to trouble. Maybe it should return a success bool? https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:415: if (next_pending_delayed_task == If I understand the code, this check for unchanged-time looks fragile to me (i.e. what would happen if GetNextPendingDelayedTask returned kMaxTimeTicks coincidentally?).
https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:340: DCHECK(main_thread_checker_.CalledOnValidThread()); On 2015/03/03 16:55:35, Sami wrote: > DCHECK_NE(input_stream_state_, InputStreamState::INACTIVE); Done. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:343: base::TimeDelta escalate_priority_duration = On 2015/03/03 16:55:36, Sami wrote: > nit: escalated_priority_duration Done. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:346: if (last_input_process_time_on_main_.is_null() && On 2015/03/04 10:43:22, picksi wrote: > In the 'else' section you also check last_input_receipt_time_on_compositor_, do > you need to check this here too? No - input_stream_state_ would be INACTIVE if last_input_receipt_time_on_compositor_ was not set and we would never have called this function (the code is taken from where it previously was in ComputeNewPolicy). https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:350: time_left_in_policy = escalate_priority_duration; On 2015/03/04 10:43:22, picksi wrote: > code style question: I assume we prefer a single return at the end of the > function, rather than multiple early outs with values? This function would be > quite a bit shorter if we returned early. I've been previously advised to avoid early returns except for early-bailouts. I'm not sure if this is an actual code style rule (can't find it in the style guide), but I generally try to do it since it makes control flow more obvious. I'm fine with changing this here if you'd prefer though? https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:355: last_input_process_time_on_main_) + On 2015/03/04 10:43:22, picksi wrote: > Philosophy: It makes my palms slightly itchy to see > last_input_process_time_on_main_ being used as a int (or whatever it is) and > also as a flag (in the code above where an is_null() check is used). Is it worth > the code bloat that would result in adding a bool (etc) to flag whatever > situation is being checked for by the is_null() check? Personally I would prefer to avoid the extra bool since it would make it possible to forget to set one when setting the other. +Sami: You originally wrote the code, what do you think? https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:397: if (current_policy_ == Policy::TOUCHSTART_PRIORITY) { On 2015/03/03 16:55:36, Sami wrote: > We have a comment in the .h saying we should use SchedulerPolicy() instead of > accessing this directly but I can't remember why :) I think it was because it used to do MaybeUpdatePolicy(), but it doesn't anymore. Seems like a good idea anyway in case we change it in the future. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:401: *next_long_idle_period_delay_out = TimeLeftInInputEscalatedPolicy(); On 2015/03/03 16:55:35, Sami wrote: > Instead of recomputing this here, how about we maintain a > current_policy_expiration_time_ in UpdatePolicy()? That we we don't need to know > much about how TOUCHSTART_PRIORITY works here other than checking that it's > active. Should also cut down on the conversions between deadlines and durations. sgtm, done. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:412: long_idle_period_duration = std::min( On 2015/03/03 16:55:35, Sami wrote: > Dumb question: why aren't we doing this adjustment for short idle tasks? Not a dumb question :). The main reason for doing it here was because the deadlines are much larger and I didn't want to delay timers for that long. Saying that, we should take this into account for short idle times too. There are some subtleties though (e.g., for long idle time we re-start the idle period after the delayed task ran, which wouldn't happen on short idle times - they would only restart on the next frame). I'd like to land this change first then I can circle back to doing this on short idle times as a separate CL if that's OK? https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:422: base::TimeDelta::FromMilliseconds(1); On 2015/03/04 10:43:22, picksi wrote: > Is there any logic behind the 1 ms duration that we can clarify here? Also, you > could probably have a well named const instead of the comment. Done. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:422: base::TimeDelta::FromMilliseconds(1); On 2015/03/03 16:55:36, Sami wrote: > Instead of this, could we schedule a check after the next wakeup? Presumably > there is guaranteed to be one since a delayed task is about to go off? This doesn't quite work due to the fact that the after-wakeup queue only pumps if the task which was run was newer than the post-after-wakeup task. In this case the sequence number for the delayed task has already been set (when it was posted) so the it wouldn't count as newer than the initiate_next_long_idle_period task posted. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:429: "AfterWakeupInitiateLongIdlePeriod"); On 2015/03/03 16:55:35, Sami wrote: > Trace event doesn't match the function name. Done. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:436: base::TimeDelta next_long_idle_period_delay_out; On 2015/03/03 16:55:36, Sami wrote: > nit: It feels weird to call a stack variable "_out". Yes this was wrong - done. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:465: TRACE_EVENT_ASYNC_END_WITH_TIMESTAMP0( On 2015/03/03 16:55:35, Sami wrote: > Will we get duplicate trace events from this if InitiateLongIdlePeriod() calls > EndIdlePeriod() again. Maybe we should keep track of whether we are in an idle > period or not? We don't (at least in about::tracing) but agreed we should keep track so that we avoid spamming tracing with extra events. Done. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:135: base::TimeDelta TimeLeftInInputEscalatedPolicy(); On 2015/03/03 16:55:36, Sami wrote: > Can this be const? Yes - done. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:155: const base::TimeTicks& now, On 2015/03/03 16:55:36, Sami wrote: > nit: pass TimeTicks by value. Done. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:160: void AfterWakeupInitiateLongIdlePeriod(); On 2015/03/03 16:55:36, Sami wrote: > InitiateLongIdlePeriodAfterWakeup to make this less yoda? mmh, I originally had this but thought it sounded like it would initiate the idle period after the next wakeup (not initiate the period now that we have woken up). But yeah, this is still probably better - done. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:160: void AfterWakeupInitiateLongIdlePeriod(); On 2015/03/03 16:55:36, Sami wrote: > InitiateLongIdlePeriodAfterWakeup to make this less yoda? Done. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:199: void TaskQueue::GetNextPendingDelayedTaskLocked( On 2015/03/04 10:43:23, picksi wrote: > Can this deal better with the delayed_task_run_times being empty? It's silent > "failure" could lead to trouble. Maybe it should return a success bool? Done. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:415: if (next_pending_delayed_task == On 2015/03/04 10:43:22, picksi wrote: > If I understand the code, this check for unchanged-time looks fragile to me > (i.e. what would happen if GetNextPendingDelayedTask returned kMaxTimeTicks > coincidentally?). Done (although we don't really need to worry about this ever happening until a task is posted for Sun, 4 December 292,277,026,596 ;)). https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.h (right): https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.h:101: base::TimeTicks NextPendingDelayedTask(); On 2015/03/03 16:55:36, Sami wrote: > NextPendingDelayedTaskRunTime since we're not returning the actual task here? Done.
Thanks, looks good. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.cc (right): https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.cc:415: if (next_pending_delayed_task == I suppose the heat death of the universe will be a more pressing issue than a glitch in some ancient software, but I definitely prefer the way it looks now!
https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:355: last_input_process_time_on_main_) + On 2015/03/04 14:23:11, rmcilroy wrote: > On 2015/03/04 10:43:22, picksi wrote: > > Philosophy: It makes my palms slightly itchy to see > > last_input_process_time_on_main_ being used as a int (or whatever it is) and > > also as a flag (in the code above where an is_null() check is used). Is it > worth > > the code bloat that would result in adding a bool (etc) to flag whatever > > situation is being checked for by the is_null() check? > > Personally I would prefer to avoid the extra bool since it would make it > possible to forget to set one when setting the other. > > +Sami: You originally wrote the code, what do you think? Since TimeTicks is already a nullable type (for better or worse), I think adding a bool would just confuse things further and make it possible for the two to get out of sync like Ross said. My vote is to keep the current code as is. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:397: if (current_policy_ == Policy::TOUCHSTART_PRIORITY) { On 2015/03/04 14:23:11, rmcilroy wrote: > On 2015/03/03 16:55:36, Sami wrote: > > We have a comment in the .h saying we should use SchedulerPolicy() instead of > > accessing this directly but I can't remember why :) > > I think it was because it used to do MaybeUpdatePolicy(), but it doesn't > anymore. Seems like a good idea anyway in case we change it in the future. Right, makes sense. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:412: long_idle_period_duration = std::min( On 2015/03/04 14:23:11, rmcilroy wrote: > On 2015/03/03 16:55:35, Sami wrote: > > Dumb question: why aren't we doing this adjustment for short idle tasks? > > Not a dumb question :). The main reason for doing it here was because the > deadlines are much larger and I didn't want to delay timers for that long. > Saying that, we should take this into account for short idle times too. There > are some subtleties though (e.g., for long idle time we re-start the idle period > after the delayed task ran, which wouldn't happen on short idle times - they > would only restart on the next frame). I'd like to land this change first then > I can circle back to doing this on short idle times as a separate CL if that's > OK? Okay, sounds good to me. Mind adding a TODO here or wherever it makes most sense? I guess we also get a little safety from the fact that timer tasks take priority over idle callbacks in the selector. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:422: base::TimeDelta::FromMilliseconds(1); On 2015/03/04 14:23:11, rmcilroy wrote: > On 2015/03/03 16:55:36, Sami wrote: > > Instead of this, could we schedule a check after the next wakeup? Presumably > > there is guaranteed to be one since a delayed task is about to go off? > > This doesn't quite work due to the fact that the after-wakeup queue only pumps > if the task which was run was newer than the post-after-wakeup task. In this > case the sequence number for the delayed task has already been set (when it was > posted) so the it wouldn't count as newer than the > initiate_next_long_idle_period task posted. Oh right, that's a good point. Since presumable the delayed task is already about to run, we could just as well use a non-delayed task for this, but maybe 1ms is better to avoid some racy corner cases. https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:476: } Forgot to reset in_idle_period_? Also in EndIdlePeriod? https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:184: CancelableClosureHolder after_wakeup_initiate_next_long_idle_period_closure_; nit: swap the names around to match the function name. https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:188: bool in_idle_period_; Looks like this isn't initialized in the constructor. Mind dumping this to the state trace too? https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.h (right): https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.h:101: base::TimeTicks NextPendingDelayedTaskRunTime(); Could you add a TQM unit test for this too?
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:412: long_idle_period_duration = std::min( On 2015/03/04 18:26:40, Sami wrote: > On 2015/03/04 14:23:11, rmcilroy wrote: > > On 2015/03/03 16:55:35, Sami wrote: > > > Dumb question: why aren't we doing this adjustment for short idle tasks? > > > > Not a dumb question :). The main reason for doing it here was because the > > deadlines are much larger and I didn't want to delay timers for that long. > > Saying that, we should take this into account for short idle times too. There > > are some subtleties though (e.g., for long idle time we re-start the idle > period > > after the delayed task ran, which wouldn't happen on short idle times - they > > would only restart on the next frame). I'd like to land this change first > then > > I can circle back to doing this on short idle times as a separate CL if that's > > OK? > > Okay, sounds good to me. Mind adding a TODO here or wherever it makes most > sense? Done in DidCommitFrameToCompositor. > I guess we also get a little safety from the fact that timer tasks take > priority over idle callbacks in the selector. https://codereview.chromium.org/968073003/diff/20001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:422: base::TimeDelta::FromMilliseconds(1); On 2015/03/04 18:26:40, Sami wrote: > On 2015/03/04 14:23:11, rmcilroy wrote: > > On 2015/03/03 16:55:36, Sami wrote: > > > Instead of this, could we schedule a check after the next wakeup? Presumably > > > there is guaranteed to be one since a delayed task is about to go off? > > > > This doesn't quite work due to the fact that the after-wakeup queue only pumps > > if the task which was run was newer than the post-after-wakeup task. In this > > case the sequence number for the delayed task has already been set (when it > was > > posted) so the it wouldn't count as newer than the > > initiate_next_long_idle_period task posted. > > Oh right, that's a good point. Since presumable the delayed task is already > about to run, we could just as well use a non-delayed task for this, but maybe > 1ms is better to avoid some racy corner cases. Yes exactly - I think this is best to avoid racy corner cases (e.g., the storm of initiate_next_long_idle_period_closure_ callbacks on the control queue preventing the delayed task from being run). https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.h:188: bool in_idle_period_; On 2015/03/04 18:26:40, Sami wrote: > Looks like this isn't initialized in the constructor. Mind dumping this to the > state trace too? Done and done. https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/task_queue_manager.h (right): https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... content/renderer/scheduler/task_queue_manager.h:101: base::TimeTicks NextPendingDelayedTaskRunTime(); On 2015/03/04 18:26:41, Sami wrote: > Could you add a TQM unit test for this too? Done.
https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:476: } On 2015/03/04 18:26:40, Sami wrote: > Forgot to reset in_idle_period_? Also in EndIdlePeriod? Missed this comment?
https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/968073003/diff/40001/content/renderer/schedul... content/renderer/scheduler/renderer_scheduler_impl.cc:476: } On 2015/03/05 12:01:51, Sami wrote: > On 2015/03/04 18:26:40, Sami wrote: > > Forgot to reset in_idle_period_? Also in EndIdlePeriod? > > Missed this comment? opps, yes thanks. Done for EndIdlePeriod but I don't think we should do it here (otherwise we would fast-track out of EndIdlePeriod without disabling the idle task queue). I've instead explicitly called EndIdlePeriod here - WDYT?
https://codereview.chromium.org/968073003/diff/100001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/968073003/diff/100001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:478: EndIdlePeriod(); Wait, isn't EndIdlePeriod now always going to add the duplicate trace event? Maybe we just need a parameter to tell it not to.
https://codereview.chromium.org/968073003/diff/100001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/968073003/diff/100001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:478: EndIdlePeriod(); On 2015/03/05 12:38:25, Sami wrote: > Wait, isn't EndIdlePeriod now always going to add the duplicate trace event? > Maybe we just need a parameter to tell it not to. Done.
Thanks, almost there :) https://codereview.chromium.org/968073003/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/968073003/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:167: void EndIdlePeriod(bool inhibit_trace_events = false); No default arguments in Chromium :P Also, channeling Simon, I'd flip the meaning of the flag the other way around.
https://codereview.chromium.org/968073003/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/968073003/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:167: void EndIdlePeriod(bool inhibit_trace_events = false); :)
https://codereview.chromium.org/968073003/diff/120001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/968073003/diff/120001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:167: void EndIdlePeriod(bool inhibit_trace_events = false); On 2015/03/05 14:11:32, picksi wrote: > :) As discussed offline - moved to using an explicit IdlePeriodState to handle this. As such I've moved the addition of the IdlePeriodState to this CL. PTAL, thanks.
lgtm, thanks Ross! https://codereview.chromium.org/968073003/diff/140001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/968073003/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:378: DCHECK(input_stream_state_ != InputStreamState::INACTIVE); nit: could remove this TODO in favor of IsInIdlePeriod :) https://codereview.chromium.org/968073003/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:476: "AfterWakeupInitiateLongIdlePeriod"); nit: Trace event doesn't match function name. https://codereview.chromium.org/968073003/diff/140001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/968073003/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:69: // Keep RendererSchedulerImpl::PolicyToString in sync with this enum. Thanks for adding these reminders. https://codereview.chromium.org/968073003/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:89: IN_LONG_IDLE_WITH_MAX_DEADLINE_PERIOD, nit: IN_LONG_IDLE_PERIOD_WITH_MAX_DEADLINE reads a bit better.
https://codereview.chromium.org/968073003/diff/140001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.cc (right): https://codereview.chromium.org/968073003/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:378: DCHECK(input_stream_state_ != InputStreamState::INACTIVE); On 2015/03/05 16:22:32, Sami wrote: > nit: could remove this TODO in favor of IsInIdlePeriod :) This is checking InputStreamState not IsInIdlePeriod, so I can't use IsInIdlePeriod I'm afraid :). (I already changed the IdlePeriodState DCHECK on line 502). https://codereview.chromium.org/968073003/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.cc:476: "AfterWakeupInitiateLongIdlePeriod"); On 2015/03/05 16:22:32, Sami wrote: > nit: Trace event doesn't match function name. Argh again - sorry, done (we should really make a trace event macro which exploits horrible preprocessor magic to use the current function's name as it's name ;). https://codereview.chromium.org/968073003/diff/140001/content/renderer/schedu... File content/renderer/scheduler/renderer_scheduler_impl.h (right): https://codereview.chromium.org/968073003/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:69: // Keep RendererSchedulerImpl::PolicyToString in sync with this enum. On 2015/03/05 16:22:32, Sami wrote: > Thanks for adding these reminders. Acknowledged. https://codereview.chromium.org/968073003/diff/140001/content/renderer/schedu... content/renderer/scheduler/renderer_scheduler_impl.h:89: IN_LONG_IDLE_WITH_MAX_DEADLINE_PERIOD, On 2015/03/05 16:22:32, Sami wrote: > nit: IN_LONG_IDLE_PERIOD_WITH_MAX_DEADLINE reads a bit better. Done.
+jochen,hpayer,kouhei,haraken FYI - when this change lands the scheduler will schedule long idle time task.
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/968073003/#ps160001 (title: "Final nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968073003/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/968073003/#ps170001 (title: "Rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968073003/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by rmcilroy@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968073003/170001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2015/03/11 14:44:01, I haz the power (commit-bot) wrote: > Try jobs failed on following builders: > linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, > http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) Updated to add the long idle period infrastructure but not actually enable long idle periods due to crbug.com/467655.
The CQ bit was checked by rmcilroy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org Link to the patchset: https://codereview.chromium.org/968073003/#ps180001 (title: "Disable idle periods to allow landing while http://crbug.com/467655 is fixed")
The CQ bit was unchecked by rmcilroy@chromium.org
lgtm, thanks for enabling the tests.
The CQ bit was checked by skyostil@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/968073003/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/143bfac301e4306a3c5043f44d92381811030a91 Cr-Commit-Position: refs/heads/master@{#320785} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
