[content]: Add support for long idle times in the Blink Scheduler.

content/renderer/scheduler/renderer_scheduler_impl.cc

Index: content/renderer/scheduler/renderer_scheduler_impl.cc
diff --git a/content/renderer/scheduler/renderer_scheduler_impl.cc b/content/renderer/scheduler/renderer_scheduler_impl.cc
index 6f7821690393a3b8c61b9956f28deb523e766a6b..8580e893fb1ea7a2a78da8721d557284f5e2707f 100644
--- a/content/renderer/scheduler/renderer_scheduler_impl.cc
+++ b/content/renderer/scheduler/renderer_scheduler_impl.cc
@@ -41,6 +41,13 @@ RendererSchedulerImpl::RendererSchedulerImpl(
                                       weak_renderer_scheduler_ptr_);
   end_idle_period_closure_.Reset(base::Bind(
       &RendererSchedulerImpl::EndIdlePeriod, weak_renderer_scheduler_ptr_));
+  initiate_next_long_idle_period_closure_.Reset(base::Bind(
+      &RendererSchedulerImpl::InitiateLongIdlePeriod,
+      weak_renderer_scheduler_ptr_));
+  after_wakeup_initiate_next_long_idle_period_closure_.Reset(base::Bind(
+      &RendererSchedulerImpl::AfterWakeupInitiateLongIdlePeriod,
+      weak_renderer_scheduler_ptr_));
+
   idle_task_runner_ = make_scoped_refptr(new SingleThreadIdleTaskRunner(
       task_queue_manager_->TaskRunnerForQueue(IDLE_TASK_QUEUE),
       control_task_after_wakeup_runner_,
@@ -142,10 +149,14 @@ void RendererSchedulerImpl::BeginFrameNotExpectedSoon() {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("renderer.scheduler"),
                "RendererSchedulerImpl::BeginFrameNotExpectedSoon");
   DCHECK(main_thread_checker_.CalledOnValidThread());
+  if (!task_queue_manager_)
+    return;
+
   // TODO(skyostil): Wire up real notification of input events processing
   // instead of this approximation.
   DidProcessInputEvent(base::TimeTicks());
-  // TODO(rmcilroy): Implement long idle times.
+
+  InitiateLongIdlePeriod();
 }
 
 void RendererSchedulerImpl::DidReceiveInputEventOnCompositorThread(
@@ -325,6 +336,29 @@ void RendererSchedulerImpl::UpdatePolicy() {
                  "RendererScheduler.policy", current_policy_);
 }
 
+base::TimeDelta RendererSchedulerImpl::TimeLeftInInputEscalatedPolicy() {
+  DCHECK(main_thread_checker_.CalledOnValidThread());

[Sami, 2015/03/03 16:55:35]

DCHECK_NE(input_stream_state_, InputStreamState::INACTIVE);

[rmcilroy, 2015/03/04 14:23:11]

Done.

+  incoming_signals_lock_.AssertAcquired();
+
+  base::TimeDelta escalate_priority_duration =

[Sami, 2015/03/03 16:55:36]

nit: escalated_priority_duration

[rmcilroy, 2015/03/04 14:23:11]

Done.

+      base::TimeDelta::FromMilliseconds(kPriorityEscalationAfterInputMillis);
+  base::TimeDelta time_left_in_policy;
+  if (last_input_process_time_on_main_.is_null() &&

[picksi, 2015/03/04 10:43:22]

In the 'else' section you also check last_input_receipt_time_on_compositor_, do
you need to check this here too?

[rmcilroy, 2015/03/04 14:23:11]

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).

+      !task_queue_manager_->IsQueueEmpty(COMPOSITOR_TASK_QUEUE)) {
+    // If the input event is still pending, go into input prioritized policy
+    // and check again later.
+    time_left_in_policy = escalate_priority_duration;

[picksi, 2015/03/04 10:43:22]

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.

[rmcilroy, 2015/03/04 14:23:11]

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? 

+  } else {
+    // Otherwise make sure the input prioritization policy ends on time.
+    base::TimeTicks new_priority_end(
+        std::max(last_input_receipt_time_on_compositor_,
+                 last_input_process_time_on_main_) +

[picksi, 2015/03/04 10:43:22]

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?

[rmcilroy, 2015/03/04 14:23:11]

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?

[Sami, 2015/03/04 18:26:40]

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.

+        escalate_priority_duration);
+    time_left_in_policy = new_priority_end - Now();
+  }
+  return time_left_in_policy;
+}
+
 RendererSchedulerImpl::Policy RendererSchedulerImpl::ComputeNewPolicy(
     base::TimeDelta* new_policy_duration) {
   DCHECK(main_thread_checker_.CalledOnValidThread());
@@ -336,41 +370,110 @@ RendererSchedulerImpl::Policy RendererSchedulerImpl::ComputeNewPolicy(
   if (input_stream_state_ == InputStreamState::INACTIVE)
     return new_policy;
 
-  base::TimeDelta new_priority_duration =
-      base::TimeDelta::FromMilliseconds(kPriorityEscalationAfterInputMillis);
   Policy input_priority_policy =
       input_stream_state_ ==
               InputStreamState::ACTIVE_AND_AWAITING_TOUCHSTART_RESPONSE
           ? Policy::TOUCHSTART_PRIORITY
           : Policy::COMPOSITOR_PRIORITY;
-
-  // If the input event is still pending, go into input prioritized policy
-  // and check again later.
-  if (last_input_process_time_on_main_.is_null() &&
-      !task_queue_manager_->IsQueueEmpty(COMPOSITOR_TASK_QUEUE)) {
+  base::TimeDelta time_left_in_policy = TimeLeftInInputEscalatedPolicy();
+  if (time_left_in_policy > base::TimeDelta()) {
     new_policy = input_priority_policy;
-    *new_policy_duration = new_priority_duration;
+    *new_policy_duration = time_left_in_policy;
   } else {
-    // Otherwise make sure the input prioritization policy ends on time.
-    base::TimeTicks new_priority_end(
-        std::max(last_input_receipt_time_on_compositor_,
-                 last_input_process_time_on_main_) +
-        new_priority_duration);
-    base::TimeDelta time_left_in_policy = new_priority_end - Now();
-
-    if (time_left_in_policy > base::TimeDelta()) {
-      new_policy = input_priority_policy;
-      *new_policy_duration = time_left_in_policy;
-    } else {
-      // Reset |input_stream_state_| to ensure
-      // DidReceiveInputEventOnCompositorThread will post an UpdatePolicy task
-      // when it's next called.
-      input_stream_state_ = InputStreamState::INACTIVE;
-    }
+    // Reset |input_stream_state_| to ensure
+    // DidReceiveInputEventOnCompositorThread will post an UpdatePolicy task
+    // when it's next called.
+    input_stream_state_ = InputStreamState::INACTIVE;
   }
   return new_policy;
 }
 
+bool RendererSchedulerImpl::ShouldStartLongIdlePeriod(
+    const base::TimeTicks& now,
+    base::TimeDelta* next_long_idle_period_delay_out) {
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+
+  MaybeUpdatePolicy();
+  if (current_policy_ == Policy::TOUCHSTART_PRIORITY) {

[Sami, 2015/03/03 16:55:36]

We have a comment in the .h saying we should use SchedulerPolicy() instead of
accessing this directly but I can't remember why :)

[rmcilroy, 2015/03/04 14:23:11]

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.

[Sami, 2015/03/04 18:26:40]

Right, makes sense.

+    // Don't start a long idle task in touch start priority, try again when
+    // the policy is scheduled to end.
+    base::AutoLock lock(incoming_signals_lock_);
+    *next_long_idle_period_delay_out = TimeLeftInInputEscalatedPolicy();

[Sami, 2015/03/03 16:55:35]

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.

[rmcilroy, 2015/03/04 14:23:11]

sgtm, done.

+    return false;
+  }
+
+  base::TimeTicks next_pending_delayed_task =
+      task_queue_manager_->NextPendingDelayedTask();
+
+  base::TimeDelta long_idle_period_duration =
+      base::TimeDelta::FromMilliseconds(kMaximumIdlePeriodMillis);
+  if (!next_pending_delayed_task.is_null()) {
+     // Limit the idle period duration to be before the next pending task.
+     long_idle_period_duration = std::min(

[Sami, 2015/03/03 16:55:35]

Dumb question: why aren't we doing this adjustment for short idle tasks?

[rmcilroy, 2015/03/04 14:23:11]

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?

[Sami, 2015/03/04 18:26:40]

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.

[rmcilroy, 2015/03/05 11:48:52]

Done in DidCommitFrameToCompositor.
 

+        next_pending_delayed_task - now, long_idle_period_duration);
+  }
+
+  if (long_idle_period_duration > base::TimeDelta()) {
+    *next_long_idle_period_delay_out = long_idle_period_duration;
+    return true;
+  } else {
+    // If we can't start the idle period yet then try again after a short delay.
+    *next_long_idle_period_delay_out =
+        base::TimeDelta::FromMilliseconds(1);

[Sami, 2015/03/03 16:55:36]

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?

[picksi, 2015/03/04 10:43:22]

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.

[rmcilroy, 2015/03/04 14:23:11]

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.

[rmcilroy, 2015/03/04 14:23:11]

Done.

[Sami, 2015/03/04 18:26:40]

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.

[rmcilroy, 2015/03/05 11:48:52]

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).

+    return false;
+  }
+}
+
+void RendererSchedulerImpl::InitiateLongIdlePeriod() {
+  TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("renderer.scheduler"),
+               "AfterWakeupInitiateLongIdlePeriod");

[Sami, 2015/03/03 16:55:35]

Trace event doesn't match the function name.

[rmcilroy, 2015/03/04 14:23:11]

Done.

+  DCHECK(main_thread_checker_.CalledOnValidThread());
+
+  // End any previous idle period.
+  EndIdlePeriod();
+
+  base::TimeTicks now(Now());
+  base::TimeDelta next_long_idle_period_delay_out;

[Sami, 2015/03/03 16:55:36]

nit: It feels weird to call a stack variable "_out".

[rmcilroy, 2015/03/04 14:23:11]

Yes this was wrong - done.

+  if (ShouldStartLongIdlePeriod(now, &next_long_idle_period_delay_out)) {
+    estimated_next_frame_begin_ = now + next_long_idle_period_delay_out;
+    StartIdlePeriod();
+  }
+
+  if (task_queue_manager_->IsQueueEmpty(IDLE_TASK_QUEUE)) {
+    // If there are no current idle tasks then post the call to initiate the
+    // next idle for execution after wakeup (at which point after-wakeup idle
+    // tasks might be eligible to run or more idle tasks posted).
+    control_task_after_wakeup_runner_->PostDelayedTask(
+        FROM_HERE,
+        after_wakeup_initiate_next_long_idle_period_closure_.callback(),
+        next_long_idle_period_delay_out);
+  } else {
+    // Otherwise post on the normal control task queue.
+    control_task_runner_->PostDelayedTask(
+        FROM_HERE,
+        initiate_next_long_idle_period_closure_.callback(),
+        next_long_idle_period_delay_out);
+  }
+}
+
+void RendererSchedulerImpl::AfterWakeupInitiateLongIdlePeriod() {
+  TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("renderer.scheduler"),
+               "AfterWakeupInitiateLongIdlePeriod");
+  DCHECK(main_thread_checker_.CalledOnValidThread());
+  // Since we were asleep until now, end the async idle period trace event at
+  // the time when it would have ended were we awake.
+  TRACE_EVENT_ASYNC_END_WITH_TIMESTAMP0(

[Sami, 2015/03/03 16:55:35]

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?

[rmcilroy, 2015/03/04 14:23:11]

We don't (at least in about::tracing) but agreed we should keep track so that we
avoid spamming tracing with extra events. Done.

+      "renderer.scheduler", "RendererSchedulerIdlePeriod", this,
+      std::min(estimated_next_frame_begin_, Now()).ToInternalValue());
+  // Post a task to initiate the next long idle period rather than calling it
+  // directly to allow all pending PostIdleTaskAfterWakeup tasks to get enqueued
+  // on the idle task queue before the next idle period starts so they are
+  // eligible to be run during the new idle period.
+  control_task_runner_->PostTask(
+      FROM_HERE,
+      initiate_next_long_idle_period_closure_.callback());
+}
+
 void RendererSchedulerImpl::StartIdlePeriod() {
   TRACE_EVENT_ASYNC_BEGIN0("renderer.scheduler",
                            "RendererSchedulerIdlePeriod", this);
@@ -396,6 +499,8 @@ void RendererSchedulerImpl::EndIdlePeriod() {
                          "RendererSchedulerIdlePeriod", this);
   DCHECK(main_thread_checker_.CalledOnValidThread());
   end_idle_period_closure_.Cancel();
+  initiate_next_long_idle_period_closure_.Cancel();
+  after_wakeup_initiate_next_long_idle_period_closure_.Cancel();
   renderer_task_queue_selector_->DisableQueue(IDLE_TASK_QUEUE);
 }