cc: Add UMA to measure the raster duration for pending tree activation.

cc: Add UMA to measure the raster duration for pending tree activation.

The PendingTreeDuration currently captures the time the pending tree
has to wait after notifying that it was activated, while the raster
work required for activation has finished. Add a seperate UMA to
capture only the raster duration for required for activation work.

R=ericrk@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2946223003
Cr-Commit-Position: refs/heads/master@{#481685}
Committed: https://chromium.googlesource.com/chromium/src/+/9fa7e15d5754fff17661c9e34e5fc67567ee3f89

[Khushal, 2017-06-21 18:27:16]

Description was changed from

==========
cc: Add UMA to measure the raster duration for pending tree activation.

The PendingTreeDuration currently captures the time the pending tree
has to wait after notifying that it was activated, while the raster
work required for activation has finished. Add a seperate UMA to
capture only the raster duration for required for activation work.

R=ericrk@chromium.org
==========

to

==========
cc: Add UMA to measure the raster duration for pending tree activation.

The PendingTreeDuration currently captures the time the pending tree
has to wait after notifying that it was activated, while the raster
work required for activation has finished. Add a seperate UMA to
capture only the raster duration for required for activation work.

R=ericrk@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Khushal, 2017-06-21 18:35:41]

I'm seeing a regression in PendingTreeDuration with checker-imaging on. I just
want to see which part of the pipeline is getting affected. Whether its the
scheduling of raster vs decode work slowing down raster for activation tiles or
if we are creating a pending tree too with impl-side invalidations too late in
the impl-frame phase so the tree is ready but has to wait until the next frame.

[ericrk, 2017-06-21 19:43:55]

lgtm

[Khushal, 2017-06-21 19:44:19]

Description was changed from

==========
cc: Add UMA to measure the raster duration for pending tree activation.

The PendingTreeDuration currently captures the time the pending tree
has to wait after notifying that it was activated, while the raster
work required for activation has finished. Add a seperate UMA to
capture only the raster duration for required for activation work.

R=ericrk@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Add UMA to measure the raster duration for pending tree activation.

The PendingTreeDuration currently captures the time the pending tree
has to wait after notifying that it was activated, while the raster
work required for activation has finished. Add a seperate UMA to
capture only the raster duration for required for activation work.

R=ericrk@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

[Khushal, 2017-06-21 19:44:20]

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

[Ilya Sherman, 2017-06-22 16:31:34]

On 2017/06/21 19:44:20, Khushal wrote:
> mailto:khushalsagar@chromium.org changed reviewers:
> + mailto:isherman@chromium.org

FYI, you need to "Publish+Mail Comments" in order for a reviewer to get an email
alerting them that you've added them.  (I don't think this is true in the new
Gerrit UI, so this will soon stop being an issue.)

[Khushal, 2017-06-22 16:33:58]

On 2017/06/22 16:31:34, Ilya Sherman wrote:
> On 2017/06/21 19:44:20, Khushal wrote:
> > mailto:khushalsagar@chromium.org changed reviewers:
> > + mailto:isherman@chromium.org
> 
> FYI, you need to "Publish+Mail Comments" in order for a reviewer to get an
email
> alerting them that you've added them.  (I don't think this is true in the new
> Gerrit UI, so this will soon stop being an issue.)

Woops. I thought I did that. :l

[Khushal, 2017-06-22 16:34:14]

+isherman, PTAL.

[Ilya Sherman, 2017-06-22 16:34:42]

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65617: +<histogram
name="Scheduling.PendingTreeRasterDuration"
Did you mean to add this to a histogram_suffixes list as well?

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65618: +  units="microseconds">
nit: Please run "git cl format" to fix up the formatting in this file.  In fact,
it should be automatically run as a presubmit hook, and give a warning if it
fails.  Did you see that warning when uploading this change?

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65622: +    to activate. Unlike
PendingTreeDuration which includes the time to commit
nit: I'd add a dash here between "PendingTreeDuration" and "which" – it makes it
easier to see where the PendingTreeDuration description ends (if you also add
another dash below, between "activated" and "this").

[Khushal, 2017-06-22 16:53:04]

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65617: +<histogram
name="Scheduling.PendingTreeRasterDuration"
On 2017/06/22 16:34:42, Ilya Sherman wrote:
> Did you mean to add this to a histogram_suffixes list as well?

It will only be measured in the renderer, since the pending tree is created in
the renderer only.

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65618: +  units="microseconds">
On 2017/06/22 16:34:42, Ilya Sherman wrote:
> nit: Please run "git cl format" to fix up the formatting in this file.  In
fact,
> it should be automatically run as a presubmit hook, and give a warning if it
> fails.  Did you see that warning when uploading this change?

I didn't see a warning. In fact running it is touching an unrelated histogram in
this file.

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65622: +    to activate. Unlike
PendingTreeDuration which includes the time to commit
On 2017/06/22 16:34:42, Ilya Sherman wrote:
> nit: I'd add a dash here between "PendingTreeDuration" and "which" – it makes
it
> easier to see where the PendingTreeDuration description ends (if you also add
> another dash below, between "activated" and "this").

Done.

[Ilya Sherman, 2017-06-22 17:00:10]

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65617: +<histogram
name="Scheduling.PendingTreeRasterDuration"
On 2017/06/22 16:53:04, Khushal wrote:
> On 2017/06/22 16:34:42, Ilya Sherman wrote:
> > Did you mean to add this to a histogram_suffixes list as well?
> 
> It will only be measured in the renderer, since the pending tree is created in
> the renderer only.

So, should this histogram name include "Renderer" in it?

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65618: +  units="microseconds">
On 2017/06/22 16:53:04, Khushal wrote:
> On 2017/06/22 16:34:42, Ilya Sherman wrote:
> > nit: Please run "git cl format" to fix up the formatting in this file.  In
> fact,
> > it should be automatically run as a presubmit hook, and give a warning if it
> > fails.  Did you see that warning when uploading this change?
> 
> I didn't see a warning. In fact running it is touching an unrelated histogram
in
> this file.

Hmm, that's odd :/  I wonder why the presubmit isn't triggering correctly...

[Khushal, 2017-06-22 17:31:38]

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65617: +<histogram
name="Scheduling.PendingTreeRasterDuration"
On 2017/06/22 17:00:10, Ilya Sherman wrote:
> On 2017/06/22 16:53:04, Khushal wrote:
> > On 2017/06/22 16:34:42, Ilya Sherman wrote:
> > > Did you mean to add this to a histogram_suffixes list as well?
> > 
> > It will only be measured in the renderer, since the pending tree is created
in
> > the renderer only.
> 
> So, should this histogram name include "Renderer" in it?

Sure. Then I'll just add it with the suffix for Scheduling.PendingTreeDuration
to stay consistent. But both of these would have data from the renderer only.

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65618: +  units="microseconds">
On 2017/06/22 17:00:10, Ilya Sherman wrote:
> On 2017/06/22 16:53:04, Khushal wrote:
> > On 2017/06/22 16:34:42, Ilya Sherman wrote:
> > > nit: Please run "git cl format" to fix up the formatting in this file.  In
> > fact,
> > > it should be automatically run as a presubmit hook, and give a warning if
it
> > > fails.  Did you see that warning when uploading this change?
> > 
> > I didn't see a warning. In fact running it is touching an unrelated
histogram
> in
> > this file.
> 
> Hmm, that's odd :/  I wonder why the presubmit isn't triggering correctly...

And its failing on some weird error as well:

Error: Unrecognized attributes "type" in tag "enum"

[Khushal, 2017-06-22 17:37:37]

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

[commit-bot: I haz the power, 2017-06-22 17:37:52]

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-06-22 18:42:34]

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

[commit-bot: I haz the power, 2017-06-22 18:42:35]

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

[Khushal, 2017-06-22 19:23:31]

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

[commit-bot: I haz the power, 2017-06-22 19:23:48]

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-06-22 20:54:30]

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

[commit-bot: I haz the power, 2017-06-22 20:54:31]

Dry run: This issue passed the CQ dry run.

[Ilya Sherman, 2017-06-22 21:20:46]

Metrics LGTM

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65617: +<histogram
name="Scheduling.PendingTreeRasterDuration"
On 2017/06/22 17:31:38, Khushal wrote:
> On 2017/06/22 17:00:10, Ilya Sherman wrote:
> > On 2017/06/22 16:53:04, Khushal wrote:
> > > On 2017/06/22 16:34:42, Ilya Sherman wrote:
> > > > Did you mean to add this to a histogram_suffixes list as well?
> > > 
> > > It will only be measured in the renderer, since the pending tree is
created
> in
> > > the renderer only.
> > 
> > So, should this histogram name include "Renderer" in it?
> 
> Sure. Then I'll just add it with the suffix for Scheduling.PendingTreeDuration
> to stay consistent. But both of these would have data from the renderer only.

I mean... the C++ code generates some specific histogram name, and I *think*
that name includes "Renderer" in it, IIUC.  Listing this among
histogram_suffixes even if only one of the suffixes is used seems ok to me.

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65618: +  units="microseconds">
On 2017/06/22 17:31:38, Khushal wrote:
> On 2017/06/22 17:00:10, Ilya Sherman wrote:
> > On 2017/06/22 16:53:04, Khushal wrote:
> > > On 2017/06/22 16:34:42, Ilya Sherman wrote:
> > > > nit: Please run "git cl format" to fix up the formatting in this file. 
In
> > > fact,
> > > > it should be automatically run as a presubmit hook, and give a warning
if
> it
> > > > fails.  Did you see that warning when uploading this change?
> > > 
> > > I didn't see a warning. In fact running it is touching an unrelated
> histogram
> > in
> > > this file.
> > 
> > Hmm, that's odd :/  I wonder why the presubmit isn't triggering correctly...
> 
> And its failing on some weird error as well:
> 
> Error: Unrecognized attributes "type" in tag "enum"

That should go away if you sync your client.  I recently simplified the file
format by dropping the type="int" annotations from all of the entries, and it
looks like some CLs landed after mine without running the presubmit, and hence
broke things a bit.  I believe it's fixed at ToT.

[Khushal, 2017-06-22 21:55:33]

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2946223003/diff/1/tools/metrics/histograms/hi...
tools/metrics/histograms/histograms.xml:65618: +  units="microseconds">
On 2017/06/22 21:20:45, Ilya Sherman wrote:
> On 2017/06/22 17:31:38, Khushal wrote:
> > On 2017/06/22 17:00:10, Ilya Sherman wrote:
> > > On 2017/06/22 16:53:04, Khushal wrote:
> > > > On 2017/06/22 16:34:42, Ilya Sherman wrote:
> > > > > nit: Please run "git cl format" to fix up the formatting in this file.

> In
> > > > fact,
> > > > > it should be automatically run as a presubmit hook, and give a warning
> if
> > it
> > > > > fails.  Did you see that warning when uploading this change?
> > > > 
> > > > I didn't see a warning. In fact running it is touching an unrelated
> > histogram
> > > in
> > > > this file.
> > > 
> > > Hmm, that's odd :/  I wonder why the presubmit isn't triggering
correctly...
> > 
> > And its failing on some weird error as well:
> > 
> > Error: Unrecognized attributes "type" in tag "enum"
> 
> That should go away if you sync your client.  I recently simplified the file
> format by dropping the type="int" annotations from all of the entries, and it
> looks like some CLs landed after mine without running the presubmit, and hence
> broke things a bit.  I believe it's fixed at ToT.

Oh. Thanks for the update.

[Khushal, 2017-06-22 21:55:38]

The CQ bit was checked by khushalsagar@chromium.org

[Khushal, 2017-06-22 21:55:41]

The patchset sent to the CQ was uploaded after l-g-t-m from ericrk@chromium.org
Link to the patchset: https://codereview.chromium.org/2946223003/#ps60001
(title: "tests")

[commit-bot: I haz the power, 2017-06-22 21:56:20]

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-06-22 22:01:48]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498168538541800,
"parent_rev": "eef1400f407d1cbdb1c66469e05b7b7f7ab9bdbd", "commit_rev":
"a1f14e293f500a19bcf00ddb2bb717ea031a1564"}

[commit-bot: I haz the power, 2017-06-22 22:02:04]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498168538541800,
"parent_rev": "591504e50781e1e93e6971b9ecb05786f4e8e309", "commit_rev":
"9fa7e15d5754fff17661c9e34e5fc67567ee3f89"}

[commit-bot: I haz the power, 2017-06-22 22:02:19]

Description was changed from

==========
cc: Add UMA to measure the raster duration for pending tree activation.

The PendingTreeDuration currently captures the time the pending tree
has to wait after notifying that it was activated, while the raster
work required for activation has finished. Add a seperate UMA to
capture only the raster duration for required for activation work.

R=ericrk@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel
==========

to

==========
cc: Add UMA to measure the raster duration for pending tree activation.

The PendingTreeDuration currently captures the time the pending tree
has to wait after notifying that it was activated, while the raster
work required for activation has finished. Add a seperate UMA to
capture only the raster duration for required for activation work.

R=ericrk@chromium.org
CQ_INCLUDE_TRYBOTS=master.tryserver.blink:linux_trusty_blink_rel

Review-Url: https://codereview.chromium.org/2946223003
Cr-Commit-Position: refs/heads/master@{#481685}
Committed:
https://chromium.googlesource.com/chromium/src/+/9fa7e15d5754fff17661c9e34e5f...
==========

[commit-bot: I haz the power, 2017-06-22 22:02:23]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9fa7e15d5754fff17661c9e34e5f...