scheduler: Disable expensive timers during main thread user input

components/scheduler/renderer/renderer_scheduler_impl.cc

Index: components/scheduler/renderer/renderer_scheduler_impl.cc
diff --git a/components/scheduler/renderer/renderer_scheduler_impl.cc b/components/scheduler/renderer/renderer_scheduler_impl.cc
index 6506e8b027cff98f22ac3767ad1439c1d7258bf8..82299e0fce0bdf2589980cb6d8ef7d97aa81c081 100644
--- a/components/scheduler/renderer/renderer_scheduler_impl.cc
+++ b/components/scheduler/renderer/renderer_scheduler_impl.cc
@@ -14,6 +14,10 @@
 #include "components/scheduler/child/task_queue_selector.h"
 
 namespace scheduler {
+namespace {
+const int kTimerTaskEstimationSampleCount = 4 * 60;
+const double kTimerTaskEstimationPercentile = 80;
+}
 
 RendererSchedulerImpl::RendererSchedulerImpl(
     scoped_refptr<SchedulerTaskRunnerDelegate> main_task_runner)
@@ -48,6 +52,9 @@ RendererSchedulerImpl::RendererSchedulerImpl(
   end_renderer_hidden_idle_period_closure_.Reset(base::Bind(
       &RendererSchedulerImpl::EndIdlePeriod, weak_factory_.GetWeakPtr()));
 
+  timer_task_runner_->AddTaskObserver(
+      &MainThreadOnly().timer_task_cost_estimator_);
+
   TRACE_EVENT_OBJECT_CREATED_WITH_ID(
       TRACE_DISABLED_BY_DEFAULT("renderer.scheduler"), "RendererScheduler",
       this);
@@ -57,6 +64,8 @@ RendererSchedulerImpl::~RendererSchedulerImpl() {
   TRACE_EVENT_OBJECT_DELETED_WITH_ID(
       TRACE_DISABLED_BY_DEFAULT("renderer.scheduler"), "RendererScheduler",
       this);
+  timer_task_runner_->RemoveTaskObserver(
+      &MainThreadOnly().timer_task_cost_estimator_);
   // Ensure the renderer scheduler was shut down explicitly, because otherwise
   // we could end up having stale pointers to the Blink heap which has been
   // terminated by this point.
@@ -64,18 +73,19 @@ RendererSchedulerImpl::~RendererSchedulerImpl() {
 }
 
 RendererSchedulerImpl::MainThreadOnly::MainThreadOnly()
-    : current_policy_(Policy::NORMAL),
+    : timer_task_cost_estimator_(kTimerTaskEstimationSampleCount,
+                                 kTimerTaskEstimationPercentile),
+      current_policy_(Policy::NORMAL),
       timer_queue_suspend_count_(0),
       renderer_hidden_(false),
-      was_shutdown_(false) {
-}
+      was_shutdown_(false) {}
 
 RendererSchedulerImpl::AnyThread::AnyThread()
     : pending_main_thread_input_event_count_(0),
       awaiting_touch_start_response_(false),
       in_idle_period_(false),
-      begin_main_frame_on_critical_path_(false) {
-}
+      begin_main_frame_on_critical_path_(false),
+      timer_tasks_seem_expensive_(false) {}
 
 RendererSchedulerImpl::CompositorThreadOnly::CompositorThreadOnly()
     : last_input_type_(blink::WebInputEvent::Undefined) {
@@ -415,6 +425,10 @@ void RendererSchedulerImpl::UpdatePolicyLocked(UpdateType update_type) {
   base::TimeTicks now = helper_.Now();
   policy_may_need_update_.SetWhileLocked(false);
 
+  AnyThread().timer_tasks_seem_expensive_ =
+      MainThreadOnly().timer_task_cost_estimator_.expected_task_duration() >
+      base::TimeDelta::FromMilliseconds(16);

[alex clarke (OOO till 29th), 2015/08/21 17:13:30]

I wonder if it makes more sense to compute how much time is avaliable till the
next compositor deadline rather than hard coding this?

[Sami, 2015/08/21 17:42:22]

Good point, I've now implemented this in terms of the expected idle period
duration.

+
   base::TimeDelta new_policy_duration;
   Policy new_policy = ComputeNewPolicy(now, &new_policy_duration);
   if (new_policy_duration > base::TimeDelta()) {
@@ -508,17 +522,10 @@ RendererSchedulerImpl::Policy RendererSchedulerImpl::ComputeNewPolicy(
     if (AnyThread().awaiting_touch_start_response_)
       return Policy::TOUCHSTART_PRIORITY;
     // If BeginMainFrame is on the critical path, we want to try and prevent
-    // timers and loading tasks from running shortly before BeginMainFrame is
-    // due to be posted from the compositor, because they can delay
-    // BeginMainFrame's execution. We do this by limiting execution of timers to
-    // idle periods, provided there has been at least one idle period recently.
-    //
-    // TODO(alexclarke): It's a shame in_idle_period_,
-    // begin_main_frame_on_critical_path_ and last_idle_period_end_time_ are in
-    // the AnyThread struct.  Find a way to migrate them to MainThreadOnly.
-    if (!AnyThread().in_idle_period_ &&

[alex clarke (OOO till 29th), 2015/08/21 17:13:30]

Can you add a todo to remove in_idle_period_  and HadAnIdlePeriodRecently?

We may still want to use those - that's why I'm not suggesting outright removing
them.

[Sami, 2015/08/21 17:42:22]

Done.

-        AnyThread().begin_main_frame_on_critical_path_ &&
-        HadAnIdlePeriodRecently(now)) {
+    // timers and loading tasks from running if we think they might be
+    // expensive.
+    if (AnyThread().timer_tasks_seem_expensive_ &&
+        AnyThread().begin_main_frame_on_critical_path_) {
       return Policy::COMPOSITOR_CRITICAL_PATH_PRIORITY;
     }
     return Policy::COMPOSITOR_PRIORITY;
@@ -640,6 +647,12 @@ RendererSchedulerImpl::AsValueLocked(base::TimeTicks optional_now) const {
                     AnyThread().awaiting_touch_start_response_);
   state->SetBoolean("begin_main_frame_on_critical_path",
                     AnyThread().begin_main_frame_on_critical_path_);
+  state->SetDouble("expected_timer_task_duration",
+                   MainThreadOnly()
+                       .timer_task_cost_estimator_.expected_task_duration()
+                       .InMillisecondsF());
+  state->SetBoolean("timer_tasks_seem_expensive_",

[alex clarke (OOO till 29th), 2015/08/21 17:13:30]

nit: remove trailing _

[Sami, 2015/08/21 17:42:22]

Well spotted, thanks.

+                    AnyThread().timer_tasks_seem_expensive_);
   state->SetDouble("estimated_next_frame_begin",
                    (MainThreadOnly().estimated_next_frame_begin_ -
                     base::TimeTicks()).InMillisecondsF());