scheduler: Disable expensive timers during main thread user input

scheduler: Disable expensive timers during main thread user input

Previously, if the user was interacting with the device, we
would only allow timers to run during idle periods. Turns
out this strategy wasn't effective with workloads with very
expensive timers. This is because even the occasional idle
period is enough to let a long running timer execute,
potentially causing jank.

This patch changes the heuristic to be based on the
expected run time of each timer task. The estimation is
based on historical run times. If we have seen user input
recently and the main thread is on the critical path, we
now disable timer tasks completely for the duration of the
interaction.

As a follow up we may want to consider doing similar
estimation for loading tasks, as well as estimating
workloads from different domains separately.

BUG=523406
Committed: https://crrev.com/e8b4542c35200e17ea6cadfb1d3f0cab13a563bc
Cr-Commit-Position: refs/heads/master@{#344858}

[Sami, 2015-08-21 16:59:15]

skyostil@chromium.org changed reviewers:
+ alexclarke@chromium.org

[Sami, 2015-08-21 17:00:08]

Tests forthcoming.

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

Needs some tests :)

https://codereview.chromium.org/1303353003/diff/1/components/scheduler/render...
File components/scheduler/renderer/renderer_scheduler_impl.cc (left):

https://codereview.chromium.org/1303353003/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.cc:519: if
(!AnyThread().in_idle_period_ &&
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.

https://codereview.chromium.org/1303353003/diff/1/components/scheduler/render...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1303353003/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.cc:430:
base::TimeDelta::FromMilliseconds(16);
I wonder if it makes more sense to compute how much time is avaliable till the
next compositor deadline rather than hard coding this?

https://codereview.chromium.org/1303353003/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.cc:654:
state->SetBoolean("timer_tasks_seem_expensive_",
nit: remove trailing _

https://codereview.chromium.org/1303353003/diff/1/components/scheduler/render...
File components/scheduler/renderer/task_cost_estimator.cc (right):

https://codereview.chromium.org/1303353003/diff/1/components/scheduler/render...
components/scheduler/renderer/task_cost_estimator.cc:28: // TODO(skyostil):
Should we do this less often?
I doubt it's terribly expensive.

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

https://codereview.chromium.org/1303353003/diff/1/components/scheduler/render...
File components/scheduler/renderer/renderer_scheduler_impl.cc (left):

https://codereview.chromium.org/1303353003/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.cc:519: if
(!AnyThread().in_idle_period_ &&
On 2015/08/21 17:13:30, alexclarke1 wrote:
> 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.

Done.

https://codereview.chromium.org/1303353003/diff/1/components/scheduler/render...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1303353003/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.cc:430:
base::TimeDelta::FromMilliseconds(16);
On 2015/08/21 17:13:30, alexclarke1 wrote:
> I wonder if it makes more sense to compute how much time is avaliable till the
> next compositor deadline rather than hard coding this?

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

https://codereview.chromium.org/1303353003/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.cc:654:
state->SetBoolean("timer_tasks_seem_expensive_",
On 2015/08/21 17:13:30, alexclarke1 wrote:
> nit: remove trailing _

Well spotted, thanks.

https://codereview.chromium.org/1303353003/diff/1/components/scheduler/render...
File components/scheduler/renderer/task_cost_estimator.cc (right):

https://codereview.chromium.org/1303353003/diff/1/components/scheduler/render...
components/scheduler/renderer/task_cost_estimator.cc:28: // TODO(skyostil):
Should we do this less often?
On 2015/08/21 17:13:30, alexclarke1 wrote:
> I doubt it's terribly expensive.

Yeah, let's see if it shows up in any profiles.

[alex clarke (OOO till 29th), 2015-08-21 17:56:46]

https://codereview.chromium.org/1303353003/diff/40001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1303353003/diff/40001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:437: 
Maybe add a todo to trace in the detailed version when these estimates where
wrong?

https://codereview.chromium.org/1303353003/diff/40001/components/scheduler/re...
File components/scheduler/renderer/task_cost_estimator.cc (right):

https://codereview.chromium.org/1303353003/diff/40001/components/scheduler/re...
components/scheduler/renderer/task_cost_estimator.cc:19: if
(++outstanding_task_count_ == 1)
Ack nested loops, we need a test sorry ;)

[Sami, 2015-08-21 18:14:42]

https://codereview.chromium.org/1303353003/diff/40001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1303353003/diff/40001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.cc:437: 
On 2015/08/21 17:56:46, alexclarke1 wrote:
> Maybe add a todo to trace in the detailed version when these estimates where
> wrong?

Done (not really sure about the best way to do that atm.)

https://codereview.chromium.org/1303353003/diff/40001/components/scheduler/re...
File components/scheduler/renderer/task_cost_estimator.cc (right):

https://codereview.chromium.org/1303353003/diff/40001/components/scheduler/re...
components/scheduler/renderer/task_cost_estimator.cc:19: if
(++outstanding_task_count_ == 1)
On 2015/08/21 17:56:46, alexclarke1 wrote:
> Ack nested loops, we need a test sorry ;)

Agreed, bane of my existence, done.

[alex clarke (OOO till 29th), 2015-08-21 18:16:46]

lgtm

[Sami, 2015-08-21 18:18:17]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2015-08-21 18:19:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1303353003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1303353003/100001

[commit-bot: I haz the power, 2015-08-21 18:36:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-21 18:36:55]

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

[Sami, 2015-08-21 18:41:01]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2015-08-21 18:41:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1303353003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1303353003/100001

[commit-bot: I haz the power, 2015-08-21 19:04:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-21 19:04:57]

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

[Sami, 2015-08-21 19:31:05]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2015-08-21 19:32:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1303353003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1303353003/100001

[commit-bot: I haz the power, 2015-08-21 19:56:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-21 19:56:56]

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

[Sami, 2015-08-21 20:50:14]

The CQ bit was checked by skyostil@chromium.org

[commit-bot: I haz the power, 2015-08-21 20:51:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1303353003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1303353003/100001

[commit-bot: I haz the power, 2015-08-21 21:04:49]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-08-21 21:05:43]

Patchset 6 (id:??) landed as
https://crrev.com/e8b4542c35200e17ea6cadfb1d3f0cab13a563bc
Cr-Commit-Position: refs/heads/master@{#344858}