A better idle time estimator

A better idle time estimator

The previous estimator only tracked the time between commit and the
compositor deadline.  That gives an unrealistic view of how much idle time
is available.  Consider a state where BeginMainFrame takes 10ms and a
timer takes 4ms.  Previously we'd have estimated there to be 2ms free.
That meant we'd think there wasn't enough free time to run the 4ms timer,
which is incorrect.

The new estimator tracks how much time was spent inside compositor tasks
(the only tasks that are 100% essential) each frame.  It computes a rolling
histogram from this and subtracts a user-selected percentile
from the compositor frame interval to obtain the estimate of how much
idle time there is.

EXPECTED PERF IMPACT:
* Regress smoothness.scrolling_tough_ad_cases/
    first_gesture_scroll_update_latency
* Possibly regress pinch zoom metrics

This is because the cost estimation of loading tasks is
likely wrong (too low).  We will fix that separately.

BUG=529245
Committed: https://crrev.com/8fd4c5bff683fd6d47a2902beb13df3a433a8653
Cr-Commit-Position: refs/heads/master@{#354289}

[alex clarke (OOO till 29th), 2015-10-02 17:34:42]

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

[Sami, 2015-10-03 00:56:15]

I like the simplicity of this. Looks like the estimator itself got left out of
the upload :P

Could you add a short note in the changelog about why we think this is better
than the old scheme (e.g., adapts to the actual BeginMainFrame frame rate and
does not subtract work that happens in the idle period from the idle duration
estimate.)

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

https://codereview.chromium.org/1381273002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.cc:835:
state->SetDouble("idle_time_estimator",
nit: expected_idle_duration instead of idle_time_estimator

[alex clarke (OOO till 29th), 2015-10-05 17:32:08]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[alex clarke (OOO till 29th), 2015-10-05 17:32:20]

PTAL

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

https://codereview.chromium.org/1381273002/diff/1/components/scheduler/render...
components/scheduler/renderer/renderer_scheduler_impl.cc:835:
state->SetDouble("idle_time_estimator",
On 2015/10/03 00:56:15, Sami wrote:
> nit: expected_idle_duration instead of idle_time_estimator

Done.

[commit-bot: I haz the power, 2015-10-05 17:38:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1381273002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1381273002/60001

[commit-bot: I haz the power, 2015-10-05 18:47:35]

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

[commit-bot: I haz the power, 2015-10-05 18:47:35]

Dry run: This issue passed the CQ dry run.

[alex clarke (OOO till 29th), 2015-10-07 16:33:52]

The CQ bit was checked by alexclarke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-07 16:39:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1381273002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1381273002/80001

[commit-bot: I haz the power, 2015-10-07 17:34:48]

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

[commit-bot: I haz the power, 2015-10-07 17:34:49]

Dry run: This issue passed the CQ dry run.

[Sami, 2015-10-09 02:32:19]

Sorry it took me a while to get to this.

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
File components/scheduler/renderer/idle_time_estimator.cc (right):

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
components/scheduler/renderer/idle_time_estimator.cc:27: base::TimeDelta
expected_frame_length = std::min(
Are we getting any information out of this frame length estimation? For the main
thread gesture use case the max is 16.6 ms, and this estimation will basically
never be below that number. In the compositor scrolling use case I'm not sure we
should be even throttling anything since that won't really affect the visible
result anyway. In the idle case (when we're expecting a gesture) maybe just a
constant 50 ms threshold would be enough -- even without subtracting compositing
time since we're not yet compositing anything in that state.

Basically I'm wondering if we should just replace |expected_frame_length| with a
parameter that gets passed in instead of |max_frame_length|? I think the only
time we would end up calling here would be in the main thread gesture case.
WDYT?

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
File components/scheduler/renderer/idle_time_estimator.h (right):

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
components/scheduler/renderer/idle_time_estimator.h:15: // Estimates how much
idle time there is avaliable.
typo: available

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
components/scheduler/renderer/idle_time_estimator.h:19: IdleTimeEstimator(int
sample_count, double estimation_percentile);
I think it would be slightly less error prone to pass in the compositor task
runner to the constructor and have this class automatically register itself as a
task observer instead of relying on the user to do remember to do that.

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
components/scheduler/renderer/idle_time_estimator.h:25: // meximum of
|max_frame_length|.
typo: maximum

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.h:147: // The ammount of
time in milliseconds we have to respond to user input as
typo: amount

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:344:
cc::BeginFrameArgs begin_frame_args1 = cc::BeginFrameArgs::Create(
Why begin_frame_args1?

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:351: void
SimulateMainFrameGestureCompositorTask(
s/Frame/Thread/?

[alex clarke (OOO till 29th), 2015-10-15 13:24:01]

PTAL

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
File components/scheduler/renderer/idle_time_estimator.cc (right):

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
components/scheduler/renderer/idle_time_estimator.cc:27: base::TimeDelta
expected_frame_length = std::min(
On 2015/10/09 02:32:18, Sami wrote:
> Are we getting any information out of this frame length estimation? For the
main
> thread gesture use case the max is 16.6 ms, and this estimation will basically
> never be below that number. In the compositor scrolling use case I'm not sure
we
> should be even throttling anything since that won't really affect the visible
> result anyway. In the idle case (when we're expecting a gesture) maybe just a
> constant 50 ms threshold would be enough -- even without subtracting
compositing
> time since we're not yet compositing anything in that state.
> 
> Basically I'm wondering if we should just replace |expected_frame_length| with
a
> parameter that gets passed in instead of |max_frame_length|? I think the only
> time we would end up calling here would be in the main thread gesture case.
> WDYT?

Done.

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
File components/scheduler/renderer/idle_time_estimator.h (right):

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
components/scheduler/renderer/idle_time_estimator.h:15: // Estimates how much
idle time there is avaliable.
On 2015/10/09 02:32:18, Sami wrote:
> typo: available

Done.

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
components/scheduler/renderer/idle_time_estimator.h:19: IdleTimeEstimator(int
sample_count, double estimation_percentile);
On 2015/10/09 02:32:18, Sami wrote:
> I think it would be slightly less error prone to pass in the compositor task
> runner to the constructor and have this class automatically register itself as
a
> task observer instead of relying on the user to do remember to do that.

Done.

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
components/scheduler/renderer/idle_time_estimator.h:25: // meximum of
|max_frame_length|.
On 2015/10/09 02:32:18, Sami wrote:
> typo: maximum

Done.

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl.h (right):

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl.h:147: // The ammount of
time in milliseconds we have to respond to user input as
On 2015/10/09 02:32:19, Sami wrote:
> typo: amount

Done.

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
File components/scheduler/renderer/renderer_scheduler_impl_unittest.cc (right):

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:344:
cc::BeginFrameArgs begin_frame_args1 = cc::BeginFrameArgs::Create(
On 2015/10/09 02:32:19, Sami wrote:
> Why begin_frame_args1?

Done.

https://codereview.chromium.org/1381273002/diff/80001/components/scheduler/re...
components/scheduler/renderer/renderer_scheduler_impl_unittest.cc:351: void
SimulateMainFrameGestureCompositorTask(
On 2015/10/09 02:32:19, Sami wrote:
> s/Frame/Thread/?

Done.

[Sami, 2015-10-15 14:43:04]

Looks nice and straightforward now. Could you update the commit message to match
the implementation?

https://codereview.chromium.org/1381273002/diff/100001/components/scheduler/r...
File components/scheduler/renderer/idle_time_estimator.cc (right):

https://codereview.chromium.org/1381273002/diff/100001/components/scheduler/r...
components/scheduler/renderer/idle_time_estimator.cc:55: task_start_time_ =
time_source_->NowTicks();
By the way we now have thread time support on all platforms, but perhaps we
still want to keep this wall clock based to account for thread contention?

https://codereview.chromium.org/1381273002/diff/100001/components/scheduler/r...
components/scheduler/renderer/idle_time_estimator.cc:59:
cumulative_compositor_runtime_ += time_source_->NowTicks() - task_start_time_;
Could you add a DCHECK() here that we're not running nested tasks in the
compositor task runner since this will get confused if we are?

https://codereview.chromium.org/1381273002/diff/100001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1381273002/diff/100001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:560: // In
UseCase::MAIN_THREAD_GESTURE we want to make sure the idle time
This comments looks out of date?

[alex clarke (OOO till 29th), 2015-10-15 16:07:52]

PTAL

https://codereview.chromium.org/1381273002/diff/100001/components/scheduler/r...
File components/scheduler/renderer/idle_time_estimator.cc (right):

https://codereview.chromium.org/1381273002/diff/100001/components/scheduler/r...
components/scheduler/renderer/idle_time_estimator.cc:55: task_start_time_ =
time_source_->NowTicks();
On 2015/10/15 14:43:04, Sami wrote:
> By the way we now have thread time support on all platforms, but perhaps we
> still want to keep this wall clock based to account for thread contention?

Yeah I think that would be best.

https://codereview.chromium.org/1381273002/diff/100001/components/scheduler/r...
components/scheduler/renderer/idle_time_estimator.cc:59:
cumulative_compositor_runtime_ += time_source_->NowTicks() - task_start_time_;
On 2015/10/15 14:43:04, Sami wrote:
> Could you add a DCHECK() here that we're not running nested tasks in the
> compositor task runner since this will get confused if we are?

Done.

https://codereview.chromium.org/1381273002/diff/100001/components/scheduler/r...
File components/scheduler/renderer/renderer_scheduler_impl.cc (right):

https://codereview.chromium.org/1381273002/diff/100001/components/scheduler/r...
components/scheduler/renderer/renderer_scheduler_impl.cc:560: // In
UseCase::MAIN_THREAD_GESTURE we want to make sure the idle time
On 2015/10/15 14:43:04, Sami wrote:
> This comments looks out of date?

Done.

[Sami, 2015-10-15 16:18:42]

Thanks, lgtm.

[alex clarke (OOO till 29th), 2015-10-15 16:19:44]

The CQ bit was checked by alexclarke@chromium.org

[commit-bot: I haz the power, 2015-10-15 16:20:01]

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

[commit-bot: I haz the power, 2015-10-15 17:22:44]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-10-15 17:23:42]

Patchset 7 (id:??) landed as
https://crrev.com/8fd4c5bff683fd6d47a2902beb13df3a433a8653
Cr-Commit-Position: refs/heads/master@{#354289}

[yurys, 2015-10-20 21:11:19]

yurys@chromium.org changed reviewers:
+ yurys@chromium.org

[yurys, 2015-10-20 21:11:19]

https://codereview.chromium.org/1381273002/diff/120001/components/scheduler/r...
File components/scheduler/renderer/idle_time_estimator.cc (right):

https://codereview.chromium.org/1381273002/diff/120001/components/scheduler/r...
components/scheduler/renderer/idle_time_estimator.cc:61:
DCHECK_EQ(nesting_level_, 1);
This DCHECK keeps failing when pausing on JS breakpoint: see
https://code.google.com/p/chromium/issues/detail?id=545624