Reimplement vsync latency UMA to be based on BeginImplFrame rather than BeginMainFrame

Reimplement vsync latency UMA to be based on BeginImplFrame rather than BeginMainFrame

The previous fix that I submitted a couple of days ago mistakenly
hooked to a wrong Scheduler method which resulted in recording
latency for throttled BeginMainFrame call. BeginImplFrame is a much
better method to hook to.
I verified this locally by looking at chrome://histograms.
The browser side histogram now looks way cleaner than in the first
implementation.

BUG=701181
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2754943002
Cr-Commit-Position: refs/heads/master@{#457676}
Committed: https://chromium.googlesource.com/chromium/src/+/0828f5b83f4835d9772dab5bd0bea6d8d9704737

[stanisc, 2017-03-16 18:32:34]

Description was changed from

==========
Reimplement vsync latency UMA to be based on BeginImplFrame rather than
BeginMainFrame

BUG=701181
==========

to

==========
Reimplement vsync latency UMA to be based on BeginImplFrame rather than
BeginMainFrame

BUG=701181
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[stanisc, 2017-03-16 18:38:17]

Description was changed from

==========
Reimplement vsync latency UMA to be based on BeginImplFrame rather than
BeginMainFrame

BUG=701181
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Reimplement vsync latency UMA to be based on BeginImplFrame rather than
BeginMainFrame

The previous fix that I submitted a couple of days ago mistakenly
hooked to a wrong Scheduler method which resulted in recording
latency for throttled BeginMainFrame call. BeginImplFrame is a much
better method to hook to.
I verified this locally by looked at chrome://histograms.
The browser side histogram now looks way cleaner than in the first
implementation.

BUG=701181
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[stanisc, 2017-03-16 18:41:20]

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

[stanisc, 2017-03-16 18:41:21]

brianderson@, please review cc/scheduler changes.
isherman@, please review histograms.xml

[stanisc, 2017-03-16 18:42:01]

Description was changed from

==========
Reimplement vsync latency UMA to be based on BeginImplFrame rather than
BeginMainFrame

The previous fix that I submitted a couple of days ago mistakenly
hooked to a wrong Scheduler method which resulted in recording
latency for throttled BeginMainFrame call. BeginImplFrame is a much
better method to hook to.
I verified this locally by looked at chrome://histograms.
The browser side histogram now looks way cleaner than in the first
implementation.

BUG=701181
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Reimplement vsync latency UMA to be based on BeginImplFrame rather than
BeginMainFrame

The previous fix that I submitted a couple of days ago mistakenly
hooked to a wrong Scheduler method which resulted in recording
latency for throttled BeginMainFrame call. BeginImplFrame is a much
better method to hook to.
I verified this locally by looking at chrome://histograms.
The browser side histogram now looks way cleaner than in the first
implementation.

BUG=701181
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[brianderson, 2017-03-16 19:11:57]

https://codereview.chromium.org/2754943002/diff/1/cc/scheduler/compositor_tim...
File cc/scheduler/compositor_timing_history.cc (right):

https://codereview.chromium.org/2754943002/diff/1/cc/scheduler/compositor_tim...
cc/scheduler/compositor_timing_history.cc:202: void
AddBeginImplFrameLatency(base::TimeDelta delta) override {
Nit: Here and below, can you put the method in the same order as the UMARporter
base class?

https://codereview.chromium.org/2754943002/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2754943002/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:329: base::TimeTicks now = Now();
You could re-use this Now() and pass it to BeginImplFrame, which would remove a
syscall per frame.

[Ilya Sherman, 2017-03-16 20:35:56]

Metrics LGTM

[stanisc, 2017-03-16 20:45:49]

https://codereview.chromium.org/2754943002/diff/1/cc/scheduler/compositor_tim...
File cc/scheduler/compositor_timing_history.cc (right):

https://codereview.chromium.org/2754943002/diff/1/cc/scheduler/compositor_tim...
cc/scheduler/compositor_timing_history.cc:202: void
AddBeginImplFrameLatency(base::TimeDelta delta) override {
On 2017/03/16 19:11:56, brianderson wrote:
> Nit: Here and below, can you put the method in the same order as the
UMARporter
> base class?

Done.

https://codereview.chromium.org/2754943002/diff/1/cc/scheduler/scheduler.cc
File cc/scheduler/scheduler.cc (right):

https://codereview.chromium.org/2754943002/diff/1/cc/scheduler/scheduler.cc#n...
cc/scheduler/scheduler.cc:329: base::TimeTicks now = Now();
On 2017/03/16 19:11:56, brianderson wrote:
> You could re-use this Now() and pass it to BeginImplFrame, which would remove
a
> syscall per frame.

Done.

[brianderson, 2017-03-16 20:53:16]

lgtm

https://codereview.chromium.org/2754943002/diff/20001/cc/scheduler/compositor...
File cc/scheduler/compositor_timing_history.cc (right):

https://codereview.chromium.org/2754943002/diff/20001/cc/scheduler/compositor...
cc/scheduler/compositor_timing_history.cc:152:
"Scheduling.Renderer.VsyncToBeginImplFrameLatency", delta);
Using the "Vsync" is true on Windows, but is not quite right on Android. Maybe
just call it BeginImplFrameLatency?

[stanisc, 2017-03-16 21:01:12]

https://codereview.chromium.org/2754943002/diff/20001/cc/scheduler/compositor...
File cc/scheduler/compositor_timing_history.cc (right):

https://codereview.chromium.org/2754943002/diff/20001/cc/scheduler/compositor...
cc/scheduler/compositor_timing_history.cc:152:
"Scheduling.Renderer.VsyncToBeginImplFrameLatency", delta);
On 2017/03/16 20:53:16, brianderson wrote:
> Using the "Vsync" is true on Windows, but is not quite right on Android. Maybe
> just call it BeginImplFrameLatency?

ajuma@ suggested adding VSync in the previous review but I am OK with taking it
off. I'll change it before submitting.

isherman@, this will require histograms.xml to be changed again to reflect the
new name.

[Ilya Sherman, 2017-03-16 21:28:04]

https://codereview.chromium.org/2754943002/diff/20001/cc/scheduler/compositor...
File cc/scheduler/compositor_timing_history.cc (right):

https://codereview.chromium.org/2754943002/diff/20001/cc/scheduler/compositor...
cc/scheduler/compositor_timing_history.cc:152:
"Scheduling.Renderer.VsyncToBeginImplFrameLatency", delta);
On 2017/03/16 21:01:12, stanisc wrote:
> On 2017/03/16 20:53:16, brianderson wrote:
> > Using the "Vsync" is true on Windows, but is not quite right on Android.
Maybe
> > just call it BeginImplFrameLatency?
> 
> ajuma@ suggested adding VSync in the previous review but I am OK with taking
it
> off. I'll change it before submitting.
> 
> isherman@, this will require histograms.xml to be changed again to reflect the
> new name.

Either name is ok with me.  Thanks.

[stanisc, 2017-03-16 23:17:00]

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

[stanisc, 2017-03-16 23:17:41]

https://codereview.chromium.org/2754943002/diff/20001/cc/scheduler/compositor...
File cc/scheduler/compositor_timing_history.cc (right):

https://codereview.chromium.org/2754943002/diff/20001/cc/scheduler/compositor...
cc/scheduler/compositor_timing_history.cc:152:
"Scheduling.Renderer.VsyncToBeginImplFrameLatency", delta);
On 2017/03/16 21:28:04, Ilya Sherman wrote:
> On 2017/03/16 21:01:12, stanisc wrote:
> > On 2017/03/16 20:53:16, brianderson wrote:
> > > Using the "Vsync" is true on Windows, but is not quite right on Android.
> Maybe
> > > just call it BeginImplFrameLatency?
> > 
> > ajuma@ suggested adding VSync in the previous review but I am OK with taking
> it
> > off. I'll change it before submitting.
> > 
> > isherman@, this will require histograms.xml to be changed again to reflect
the
> > new name.
> 
> Either name is ok with me.  Thanks.

Done.

[commit-bot: I haz the power, 2017-03-16 23:18:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 02:02:56]

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

[commit-bot: I haz the power, 2017-03-17 02:02:57]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[stanisc, 2017-03-17 02:46:33]

The CQ bit was checked by stanisc@chromium.org

[stanisc, 2017-03-17 02:46:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, brianderson@chromium.org
Link to the patchset: https://codereview.chromium.org/2754943002/#ps40001
(title: "Changed UMA metric name, removed VsyncTo.")

[commit-bot: I haz the power, 2017-03-17 02:47:06]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-17 03:38:44]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1489718793427520,
"parent_rev": "28fbcba6865441b3c83be87bbadc71c726a3d190", "commit_rev":
"0828f5b83f4835d9772dab5bd0bea6d8d9704737"}

[commit-bot: I haz the power, 2017-03-17 03:39:24]

Description was changed from

==========
Reimplement vsync latency UMA to be based on BeginImplFrame rather than
BeginMainFrame

The previous fix that I submitted a couple of days ago mistakenly
hooked to a wrong Scheduler method which resulted in recording
latency for throttled BeginMainFrame call. BeginImplFrame is a much
better method to hook to.
I verified this locally by looking at chrome://histograms.
The browser side histogram now looks way cleaner than in the first
implementation.

BUG=701181
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
Reimplement vsync latency UMA to be based on BeginImplFrame rather than
BeginMainFrame

The previous fix that I submitted a couple of days ago mistakenly
hooked to a wrong Scheduler method which resulted in recording
latency for throttled BeginMainFrame call. BeginImplFrame is a much
better method to hook to.
I verified this locally by looking at chrome://histograms.
The browser side histogram now looks way cleaner than in the first
implementation.

BUG=701181
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2754943002
Cr-Commit-Position: refs/heads/master@{#457676}
Committed:
https://chromium.googlesource.com/chromium/src/+/0828f5b83f4835d9772dab5bd0be...
==========

[commit-bot: I haz the power, 2017-03-17 03:39:25]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/0828f5b83f4835d9772dab5bd0be...