cc: Move timing history to the Scheduler.

cc: Move timing history to the Scheduler.

BUG=406158
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/c9f50357f74e12c26370c42cc54f904de3033b26
Cr-Commit-Position: refs/heads/master@{#335866}

[brianderson, 2015-06-13 01:36:28]

brianderson@chromium.org changed reviewers:
+ enne@chromium.org, mithro@mithis.com, sunnyps@chromium.org

[brianderson, 2015-06-13 01:36:29]

This kind of came out of a comment Tim made in
https://codereview.chromium.org/1133673004. I've always wanted the timing
history more under the scheduler's control too, so I ran with it.

The code review accounting is off, but it still seems to be a net negative
amount of code.

It was also a good exercise since I realized the measurements were slightly off
for scheduling purposes. For example, the scheduler cares more about commit to
NotifyReadyToActivate time than actual activation time. It is probably also
better to measure the BeginMainFrame time before (rather than after) we send the
message.

I got it to pass tests - but would still like to fix some of the naming. Wdyt?

[enne (OOO), 2015-06-17 17:15:46]

This looks good in general to me.  What would you change the naming to?

[brianderson, 2015-06-18 19:22:18]

On 2015/06/17 17:15:46, enne wrote:
> This looks good in general to me.  What would you change the naming to?

It's hard to come up with good names since the meanings are somewhat contextual.

For this patch, I will change DidBeginMainFrame to WillBeginMainFrame and keep
the DidActivateSyncTree name and avoid changing what it means for now.

I put together a followup patch that fixes the naming and is a bit more aware of
other actions/context:
https://codereview.chromium.org/1192663005

[brianderson, 2015-06-18 23:29:24]

Should be ready now. ptal!

[mithro-old, 2015-06-23 09:05:35]

The CQ bit was checked by mithro@mithis.com

[mithro-old, 2015-06-23 09:05:35]

lgtm

[commit-bot: I haz the power, 2015-06-23 09:06:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1184863004/180001

[commit-bot: I haz the power, 2015-06-23 09:09:29]

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

[commit-bot: I haz the power, 2015-06-23 09:09:30]

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...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mithro-old, 2015-06-23 10:25:22]

Looks like this change needs a rebase before it can be landed.

Tim

[sunnyps, 2015-06-23 18:11:03]

https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/composito...
File cc/scheduler/compositor_timing_history.h (right):

https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/composito...
cc/scheduler/compositor_timing_history.h:42: RenderingStatsInstrumentation*
rendering_stats_instrumentation_;
DISALLOW_COPY_AND_ASSIGN?

https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:67: RenderingStatsInstrumentation*
rendering_stats_instrumentation,
The Scheduler could take in a CompositorTimingHistory scoped_ptr (that's created
in Scheduler::Create or TestScheduler::Create) instead of a
RenderingStatsInstrumentation. That leads to cleaner testing code IMO.

https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler...
cc/scheduler/scheduler.h:184: // virtual for testing - Don't call these in the
constructor or
nit: Do we really need a comment asking not to use virtual methods in
ctor/dtors?

[danakj, 2015-06-23 18:13:00]

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

[danakj, 2015-06-23 18:13:02]

https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler...
cc/scheduler/scheduler.h:184: // virtual for testing - Don't call these in the
constructor or
On 2015/06/23 18:11:03, sunnyps wrote:
> nit: Do we really need a comment asking not to use virtual methods in
> ctor/dtors?

The style guide forbids doing so.

[sunnyps, 2015-06-23 18:19:04]

On 2015/06/23 18:13:02, danakj wrote:
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler.h
> File cc/scheduler/scheduler.h (right):
> 
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler...
> cc/scheduler/scheduler.h:184: // virtual for testing - Don't call these in the
> constructor or
> On 2015/06/23 18:11:03, sunnyps wrote:
> > nit: Do we really need a comment asking not to use virtual methods in
> > ctor/dtors?
> 
> The style guide forbids doing so.

Yeah, I meant the comment is stating something that should be known by all
chromium contributors so it's not very useful.

[danakj, 2015-06-23 18:25:08]

On Tue, Jun 23, 2015 at 11:19 AM, <sunnyps@chromium.org> wrote:

> On 2015/06/23 18:13:02, danakj wrote:
>
>
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler.h
>
>> File cc/scheduler/scheduler.h (right):
>>
>
>
>
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler...
>
>> cc/scheduler/scheduler.h:184: // virtual for testing - Don't call these
>> in the
>> constructor or
>> On 2015/06/23 18:11:03, sunnyps wrote:
>> > nit: Do we really need a comment asking not to use virtual methods in
>> > ctor/dtors?
>>
>
>  The style guide forbids doing so.
>>
>
> Yeah, I meant the comment is stating something that should be known by all
> chromium contributors so it's not very useful.
>

Yep, agreeing :)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[brianderson, 2015-06-23 20:23:26]

brianderson@chromium.org changed reviewers:
+ sky@chromium.org

[brianderson, 2015-06-23 20:23:27]

+sky for owners review of surfaces_scheduler.

https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/composito...
File cc/scheduler/compositor_timing_history.h (right):

https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/composito...
cc/scheduler/compositor_timing_history.h:42: RenderingStatsInstrumentation*
rendering_stats_instrumentation_;
On 2015/06/23 18:11:02, sunnyps wrote:
> DISALLOW_COPY_AND_ASSIGN?

Done.

https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler...
cc/scheduler/scheduler.cc:67: RenderingStatsInstrumentation*
rendering_stats_instrumentation,
On 2015/06/23 18:11:02, sunnyps wrote:
> The Scheduler could take in a CompositorTimingHistory scoped_ptr (that's
created
> in Scheduler::Create or TestScheduler::Create) instead of a
> RenderingStatsInstrumentation. That leads to cleaner testing code IMO.

I kept this constructor and Scheduler::Create the same, but changed
TestScheduler::Create a bit so that 1) tests don't need to worry about
RendererinStatsInstrumentation and 2) the Proxys don't need to worry about
CompositorTimingHistory.

Is that good?

https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler...
cc/scheduler/scheduler.h:184: // virtual for testing - Don't call these in the
constructor or
On 2015/06/23 18:11:03, sunnyps wrote:
> nit: Do we really need a comment asking not to use virtual methods in
> ctor/dtors?

Done.

[sunnyps, 2015-06-23 20:33:54]

On 2015/06/23 20:23:27, brianderson wrote:
> +sky for owners review of surfaces_scheduler.
> 
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/composito...
> File cc/scheduler/compositor_timing_history.h (right):
> 
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/composito...
> cc/scheduler/compositor_timing_history.h:42: RenderingStatsInstrumentation*
> rendering_stats_instrumentation_;
> On 2015/06/23 18:11:02, sunnyps wrote:
> > DISALLOW_COPY_AND_ASSIGN?
> 
> Done.
> 
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler.cc
> File cc/scheduler/scheduler.cc (right):
> 
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler...
> cc/scheduler/scheduler.cc:67: RenderingStatsInstrumentation*
> rendering_stats_instrumentation,
> On 2015/06/23 18:11:02, sunnyps wrote:
> > The Scheduler could take in a CompositorTimingHistory scoped_ptr (that's
> created
> > in Scheduler::Create or TestScheduler::Create) instead of a
> > RenderingStatsInstrumentation. That leads to cleaner testing code IMO.
> 
> I kept this constructor and Scheduler::Create the same, but changed
> TestScheduler::Create a bit so that 1) tests don't need to worry about
> RendererinStatsInstrumentation and 2) the Proxys don't need to worry about
> CompositorTimingHistory.
> 
> Is that good?

If RenderingStatsInstrumentation is only used to initialize
CompositorTimingHistory I would prefer if Scheduler took a CTH instead of a RSI.
That way TestScheduler doesn't even have to care about RSI and only create a
TestCPH which SchedulerTest owns. Overriding the estimates would involve calling
into the TestCPH from the test.

> 
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler.h
> File cc/scheduler/scheduler.h (right):
> 
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler...
> cc/scheduler/scheduler.h:184: // virtual for testing - Don't call these in the
> constructor or
> On 2015/06/23 18:11:03, sunnyps wrote:
> > nit: Do we really need a comment asking not to use virtual methods in
> > ctor/dtors?
> 
> Done.

[sky, 2015-06-23 21:02:55]

LGTM

[brianderson, 2015-06-23 22:58:47]

On 2015/06/23 20:33:54, sunnyps wrote:
> On 2015/06/23 20:23:27, brianderson wrote:
> > +sky for owners review of surfaces_scheduler.
> > 
> >
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/composito...
> > File cc/scheduler/compositor_timing_history.h (right):
> > 
> >
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/composito...
> > cc/scheduler/compositor_timing_history.h:42: RenderingStatsInstrumentation*
> > rendering_stats_instrumentation_;
> > On 2015/06/23 18:11:02, sunnyps wrote:
> > > DISALLOW_COPY_AND_ASSIGN?
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler.cc
> > File cc/scheduler/scheduler.cc (right):
> > 
> >
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler...
> > cc/scheduler/scheduler.cc:67: RenderingStatsInstrumentation*
> > rendering_stats_instrumentation,
> > On 2015/06/23 18:11:02, sunnyps wrote:
> > > The Scheduler could take in a CompositorTimingHistory scoped_ptr (that's
> > created
> > > in Scheduler::Create or TestScheduler::Create) instead of a
> > > RenderingStatsInstrumentation. That leads to cleaner testing code IMO.
> > 
> > I kept this constructor and Scheduler::Create the same, but changed
> > TestScheduler::Create a bit so that 1) tests don't need to worry about
> > RendererinStatsInstrumentation and 2) the Proxys don't need to worry about
> > CompositorTimingHistory.
> > 
> > Is that good?
> 
> If RenderingStatsInstrumentation is only used to initialize
> CompositorTimingHistory I would prefer if Scheduler took a CTH instead of a
RSI.
> That way TestScheduler doesn't even have to care about RSI and only create a
> TestCPH which SchedulerTest owns. Overriding the estimates would involve
calling
> into the TestCPH from the test.

I like it. Ptal.

> 
> > 
> >
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler.h
> > File cc/scheduler/scheduler.h (right):
> > 
> >
>
https://codereview.chromium.org/1184863004/diff/180001/cc/scheduler/scheduler...
> > cc/scheduler/scheduler.h:184: // virtual for testing - Don't call these in
the
> > constructor or
> > On 2015/06/23 18:11:03, sunnyps wrote:
> > > nit: Do we really need a comment asking not to use virtual methods in
> > > ctor/dtors?
> > 
> > Done.

[sunnyps, 2015-06-23 23:05:52]

LGTM

[brianderson, 2015-06-23 23:15:33]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-06-23 23:15:34]

The patchset sent to the CQ was uploaded after l-g-t-m from mithro@mithis.com,
sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1184863004/#ps240001
(title: "inject CompositorTimingHistory")

[commit-bot: I haz the power, 2015-06-23 23:17:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1184863004/240001

[commit-bot: I haz the power, 2015-06-23 23:33:48]

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

[commit-bot: I haz the power, 2015-06-23 23:33:50]

Exceeded global retry quota

[brianderson, 2015-06-24 01:41:09]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-06-24 01:41:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
sunnyps@chromium.org, sky@chromium.org, mithro@mithis.com
Link to the patchset: https://codereview.chromium.org/1184863004/#ps260001
(title: "fix compile and export")

[commit-bot: I haz the power, 2015-06-24 01:42:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1184863004/260001

[commit-bot: I haz the power, 2015-06-24 01:45:00]

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

[commit-bot: I haz the power, 2015-06-24 01:45:02]

Exceeded global retry quota

[brianderson, 2015-06-24 02:06:07]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-06-24 02:06:07]

The patchset sent to the CQ was uploaded after l-g-t-m from
sunnyps@chromium.org, sky@chromium.org, mithro@mithis.com
Link to the patchset: https://codereview.chromium.org/1184863004/#ps280001
(title: "rebase")

[commit-bot: I haz the power, 2015-06-24 02:07:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1184863004/280001

[commit-bot: I haz the power, 2015-06-24 02:35:14]

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

[commit-bot: I haz the power, 2015-06-24 02:35:15]

Exceeded global retry quota

[brianderson, 2015-06-24 02:38:59]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-06-24 02:38:59]

The patchset sent to the CQ was uploaded after l-g-t-m from
sunnyps@chromium.org, sky@chromium.org, mithro@mithis.com
Link to the patchset: https://codereview.chromium.org/1184863004/#ps300001
(title: "add cc::")

[commit-bot: I haz the power, 2015-06-24 02:39:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1184863004/300001

[commit-bot: I haz the power, 2015-06-24 03:39:07]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-06-24 03:39:54]

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