cc: Track BeginMainFrame more precisely in CompositorTimingHistory.

cc: Track BeginMainFrame more precisely CompositorTimingHistory.

This tracks and estimates the following:
BeginMainFrameQueueDurationCritical
BeginMainFrameQueueDurationNotCritical
BeginMainFrameStartToCommitDuration

The above is a breakdown of the older
BeginMainFrameToCommitDurationEstimate
and is intended to replace it.

Tracking the above separately will help
make better decisions regarding whether we
should wait for the main thread or not
and also give us better UMA data regarding
main thread responsiveness when the
on_critical_path flag is set vs. not.

BUG=469953
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel

Committed: https://crrev.com/21aef16e0d6f1aae6fe9632be90206211b9a3bc5
Cr-Commit-Position: refs/heads/master@{#359055}

[brianderson, 2015-11-05 02:47:14]

Description was changed from

==========
cc: Track BeginMainFrame more precisely CompositorTimingHistory.

This tracks and estimates the following:
BeginMainFrameQueueDurationCritical
BeginMainFrameQueueDurationNotCritical
BeginMainFrameStartToCommitDuration

The above is a breakdown of the older
BeginMainFrameToCommitDurationEstimate
and is intended to replace it.

BUG=469953
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
cc: Track BeginMainFrame more precisely CompositorTimingHistory.

This tracks and estimates the following:
BeginMainFrameQueueDurationCritical
BeginMainFrameQueueDurationNotCritical
BeginMainFrameStartToCommitDuration

The above is a breakdown of the older
BeginMainFrameToCommitDurationEstimate
and is intended to replace it.

Tracking the above separately will help
make better decisions regarding whether we
should wait for the main thread or not
and also give us better UMA data regarding
main thread responsiveness when the
on_critical_path flag is set vs. not.

BUG=469953
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[brianderson, 2015-11-05 02:47:14]

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

[brianderson, 2015-11-05 02:49:40]

Ptal. I still need to add tests for the new methods, but all existing tests are
passing now.

I also need to update histograms.xml for UMA tracking - but before I do that,
I'm thinking of removing the Over/Underestimate UMAs since I never use them and
can't think of a case where I would. Wdy'allt?

[brianderson, 2015-11-05 02:52:53]

+tdresser as fyi

[tdresser, 2015-11-05 13:51:56]

On 2015/11/05 02:52:53, brianderson wrote:
> +tdresser as fyi

I'd consider leaving the existing UMAs in place and filing a bug to remove them
in a milestone or two, to enable comparison with the new UMA metric as a sanity
check.

[Sami, 2015-11-05 14:42:36]

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

[Sami, 2015-11-05 14:42:36]

If we truly never care about the existing over/underestimated UMA then IMO we
shouldn't bother making them work with this patch.

https://codereview.chromium.org/1432463002/diff/20001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/1432463002/diff/20001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:131: // and statistics puposes.
nit: I'm not sure if the remote machine example is more mystifying or helpful :)
Would the single thread proxy case be better?

[brianderson, 2015-11-05 21:27:57]

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

[brianderson, 2015-11-05 21:27:57]

+isherman for owners review of histograms.xml

CompositorTimingHistory tests have been modified to test the new functionality.

I opened a follow up bug
(https://code.google.com/p/chromium/issues/detail?id=552004) to remove the now
deprecated BeginMainFrameToCommitDurationEstimate in the future.

The newly added UMA's do not have over/under estimates, but I kept the old ones
around. We can remove the over/under estimates for the old UMA's separately.

[brianderson, 2015-11-05 21:33:26]

https://codereview.chromium.org/1432463002/diff/20001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/1432463002/diff/20001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:131: // and statistics puposes.
On 2015/11/05 14:42:36, Sami wrote:
> nit: I'm not sure if the remote machine example is more mystifying or helpful
:)
> Would the single thread proxy case be better?

With STP, we can still get valid timestamps. I checked with dtrainor and he said
I could change this to "(e.g. blink is running on a remote channel)" which
aligns more with where the code for "channel_main.h" is heading.

[Ilya Sherman, 2015-11-06 01:05:56]

histograms.xml lgtm % nit:

https://codereview.chromium.org/1432463002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1432463002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:40677: +    set.
nit: It seems like this histogram should be when the flag is not set, and the
one above should be when it is set -- no?

[brianderson, 2015-11-06 02:07:58]

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

[brianderson, 2015-11-06 02:07:59]

+fsaumuel for mus.

https://codereview.chromium.org/1432463002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1432463002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:40677: +    set.
On 2015/11/06 01:05:56, Ilya Sherman wrote:
> nit: It seems like this histogram should be when the flag is not set, and the
> one above should be when it is set -- no?

Good eye. Done.

[Fady Samuel, 2015-11-06 03:04:35]

MUS lgtm

[Sami, 2015-11-06 11:27:50]

cc/ lgtm. I was trying to think of a way to avoid having to ferry the
BeginMainFrame start time around in ThreadProxy, but couldn't come up with
anything simpler.

https://codereview.chromium.org/1432463002/diff/20001/cc/scheduler/scheduler.h
File cc/scheduler/scheduler.h (right):

https://codereview.chromium.org/1432463002/diff/20001/cc/scheduler/scheduler....
cc/scheduler/scheduler.h:131: // and statistics puposes.
On 2015/11/05 21:33:26, brianderson wrote:
> On 2015/11/05 14:42:36, Sami wrote:
> > nit: I'm not sure if the remote machine example is more mystifying or
helpful
> :)
> > Would the single thread proxy case be better?
> 
> With STP, we can still get valid timestamps. I checked with dtrainor and he
said
> I could change this to "(e.g. blink is running on a remote channel)" which
> aligns more with where the code for "channel_main.h" is heading.

Ah right, looks like I misread STP as passing in a zero timeticks but that's
actually not the case. Speaking about channels sgtm.

https://codereview.chromium.org/1432463002/diff/100001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1432463002/diff/100001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:40685: +    The time from when the main
side acually starts the BeginMainFrame to when
typo: actually

[brianderson, 2015-11-11 03:31:38]

The CQ bit was checked by brianderson@chromium.org

[brianderson, 2015-11-11 03:31:39]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, fsamuel@chromium.org, skyostil@chromium.org
Link to the patchset: https://codereview.chromium.org/1432463002/#ps120001
(title: "rebase; fix typo")

[commit-bot: I haz the power, 2015-11-11 03:33:07]

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

[commit-bot: I haz the power, 2015-11-11 05:12:31]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-11-11 05:13:37]

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