Add UMA stats for time in paint+layout, compositing, paint invalidation and paint.

Add UMA stats for time in paint+layout, compositing, paint invalidation and paint.

BUG=599905
Committed: https://crrev.com/19527e9d324c69604169e40c632a636a180ca58c
Cr-Commit-Position: refs/heads/master@{#384666}

[chrishtr, 2016-03-31 22:58:29]

Description was changed from

==========
none


none


none


none

BUG=
==========

to

==========
Add UMA stats for time in layout, compositing, paint invalidation and paint.
==========

[chrishtr, 2016-03-31 23:47:06]

chrishtr@chromium.org changed reviewers:
+ asvitkine@chromium.org, eae@chromium.org, wangxianzhu@chromium.org

[chrishtr, 2016-03-31 23:47:20]

Description was changed from

==========
Add UMA stats for time in layout, compositing, paint invalidation and paint.
==========

to

==========
Add UMA stats for time in paint+layout, compositing, paint invalidation and
paint.
==========

[Xianzhu, 2016-03-31 23:58:57]

core/ lgtm

[pdr., 2016-04-01 00:03:15]

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

[pdr., 2016-04-01 00:03:16]

(driveby) this looks pretty good to me too.

https://codereview.chromium.org/1852633002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/Histogram.h (right):

https://codereview.chromium.org/1852633002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/Histogram.h:49: :
m_startTime(WTF::monotonicallyIncreasingTime()),
Nit: indentation

[eae, 2016-04-01 00:06:15]

LGTM

[wkorman, 2016-04-01 00:15:29]

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

[wkorman, 2016-04-01 00:15:30]

https://codereview.chromium.org/1852633002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/Histogram.h (right):

https://codereview.chromium.org/1852633002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/Histogram.h:68: // This macro records all
times between 0us and 10 seconds.
In 95th pctile I believe we have seen for some metrics close to 10s, are we ok
seeing this max out in some cases?

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

https://codereview.chromium.org/1852633002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:3389: +    Time spent updating style and
layout in the document lifecycle.
missing 'Blink' before document.

[chrishtr, 2016-04-01 00:29:40]

https://codereview.chromium.org/1852633002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/Histogram.h (right):

https://codereview.chromium.org/1852633002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/Histogram.h:68: // This macro records all
times between 0us and 10 seconds.
On 2016/04/01 at 00:15:30, wkorman wrote:
> In 95th pctile I believe we have seen for some metrics close to 10s, are we ok
seeing this max out in some cases?

I've been cribbing off of the implementation of SCOPED_UMA_HISTOGRAM_TIMER. I
think it bottoms out at UMA_HISTOGRAM_TIMES which uses 10s and 50 buckets. I'll
change to 50 buckets. There is also a _MEDIUM and _LONG variant implemented, but
I figured that is overkill.

[Alexei Svitkine (slow), 2016-04-01 15:26:12]

https://codereview.chromium.org/1852633002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/Histogram.h (right):

https://codereview.chromium.org/1852633002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/Histogram.h:68: // This macro records all
times between 0us and 10 seconds.
On 2016/04/01 00:29:40, chrishtr wrote:
> On 2016/04/01 at 00:15:30, wkorman wrote:
> > In 95th pctile I believe we have seen for some metrics close to 10s, are we
ok
> seeing this max out in some cases?
> 
> I've been cribbing off of the implementation of SCOPED_UMA_HISTOGRAM_TIMER. I
> think it bottoms out at UMA_HISTOGRAM_TIMES which uses 10s and 50 buckets.
I'll
> change to 50 buckets. There is also a _MEDIUM and _LONG variant implemented,
but
> I figured that is overkill.
Yes, please change to 50 buckets. 100 bucket histograms are quite expensive and
often don't provide more insight than 50 bucket ones.

Note: The normal TIMES() macros in base/ are in ms, not μs, so they're not
necessarily a good point of comparison.

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

https://codereview.chromium.org/1852633002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:3285: +<histogram
name="Blink.Compositing.UpdateUs" units="microseconds">
I would just call these UpdateTime rather than UpdateUs. First, UpdateUs might
sound confusing if you think about the second meaning of "Us", but also all our
dashboards show the units already, so it's redundant.

[chrishtr, 2016-04-01 16:33:42]

https://codereview.chromium.org/1852633002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/Histogram.h (right):

https://codereview.chromium.org/1852633002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/Histogram.h:49: :
m_startTime(WTF::monotonicallyIncreasingTime()),
On 2016/04/01 at 00:03:16, pdr wrote:
> Nit: indentation

Done

https://codereview.chromium.org/1852633002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/Histogram.h:68: // This macro records all
times between 0us and 10 seconds.
On 2016/04/01 at 15:26:12, Alexei Svitkine wrote:
> On 2016/04/01 00:29:40, chrishtr wrote:
> > On 2016/04/01 at 00:15:30, wkorman wrote:
> > > In 95th pctile I believe we have seen for some metrics close to 10s, are
we ok
> > seeing this max out in some cases?
> > 
> > I've been cribbing off of the implementation of SCOPED_UMA_HISTOGRAM_TIMER.
I
> > think it bottoms out at UMA_HISTOGRAM_TIMES which uses 10s and 50 buckets.
I'll
> > change to 50 buckets. There is also a _MEDIUM and _LONG variant implemented,
but
> > I figured that is overkill.
> Yes, please change to 50 buckets. 100 bucket histograms are quite expensive
and often don't provide more insight than 50 bucket ones.
> 
> Note: The normal TIMES() macros in base/ are in ms, not μs, so they're not
necessarily a good point of comparison.

Done.

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

https://codereview.chromium.org/1852633002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:3285: +<histogram
name="Blink.Compositing.UpdateUs" units="microseconds">
On 2016/04/01 at 15:26:12, Alexei Svitkine wrote:
> I would just call these UpdateTime rather than UpdateUs. First, UpdateUs might
sound confusing if you think about the second meaning of "Us", but also all our
dashboards show the units already, so it's redundant.

Done

https://codereview.chromium.org/1852633002/diff/140001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:3389: +    Time spent updating style and
layout in the document lifecycle.
On 2016/04/01 at 00:15:30, wkorman wrote:
> missing 'Blink' before document.

Done

[Alexei Svitkine (slow), 2016-04-01 16:39:04]

lgtm % comments

Should this be associated with a BUG= somewhere?

https://codereview.chromium.org/1852633002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/Histogram.h (right):

https://codereview.chromium.org/1852633002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/Histogram.h:50: :
m_startTime(WTF::monotonicallyIncreasingTime() *
base::Time::kMicrosecondsPerSecond),
Nit: Why not just do the multiplication on the delta in the end and keep the
intermediate as the base unit? This way, you only need to multiply once.

https://codereview.chromium.org/1852633002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/Histogram.h:70: // This macro records all
times between 0us and 10 seconds.
Expand comment to mention that this macro shouldn't be changed without renaming
the metrics using these.

[chrishtr, 2016-04-01 16:43:44]

https://codereview.chromium.org/1852633002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/Histogram.h (right):

https://codereview.chromium.org/1852633002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/Histogram.h:50: :
m_startTime(WTF::monotonicallyIncreasingTime() *
base::Time::kMicrosecondsPerSecond),
On 2016/04/01 at 16:39:04, Alexei Svitkine wrote:
> Nit: Why not just do the multiplication on the delta in the end and keep the
intermediate as the base unit? This way, you only need to multiply once.

Done. Good idea.

https://codereview.chromium.org/1852633002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/Histogram.h:70: // This macro records all
times between 0us and 10 seconds.
On 2016/04/01 at 16:39:04, Alexei Svitkine wrote:
> Expand comment to mention that this macro shouldn't be changed without
renaming the metrics using these.

Done.

[chrishtr, 2016-04-01 16:43:51]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-04-01 16:43:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org, eae@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1852633002/#ps240001
(title: " ")

[commit-bot: I haz the power, 2016-04-01 16:44:12]

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

[commit-bot: I haz the power, 2016-04-01 16:55:14]

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

[commit-bot: I haz the power, 2016-04-01 16:55:15]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromeos_x86-generic_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2016-04-01 17:08:30]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-04-01 17:08:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
asvitkine@chromium.org, wangxianzhu@chromium.org, eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1852633002/#ps260001
(title: " ")

[commit-bot: I haz the power, 2016-04-01 17:09:03]

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

[chrishtr, 2016-04-01 17:27:22]

Description was changed from

==========
Add UMA stats for time in paint+layout, compositing, paint invalidation and
paint.
==========

to

==========
Add UMA stats for time in paint+layout, compositing, paint invalidation and
paint.

BUG=599905
==========

[commit-bot: I haz the power, 2016-04-01 18:31:20]

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

[commit-bot: I haz the power, 2016-04-01 18:31:21]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chrishtr, 2016-04-01 18:32:28]

The CQ bit was checked by chrishtr@chromium.org

[commit-bot: I haz the power, 2016-04-01 18:32:43]

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

[commit-bot: I haz the power, 2016-04-01 19:54:49]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-04-01 19:56:08]

Description was changed from

==========
Add UMA stats for time in paint+layout, compositing, paint invalidation and
paint.

BUG=599905
==========

to

==========
Add UMA stats for time in paint+layout, compositing, paint invalidation and
paint.

BUG=599905

Committed: https://crrev.com/19527e9d324c69604169e40c632a636a180ca58c
Cr-Commit-Position: refs/heads/master@{#384666}
==========

[commit-bot: I haz the power, 2016-04-01 19:56:09]

Patchset 14 (id:??) landed as
https://crrev.com/19527e9d324c69604169e40c632a636a180ca58c
Cr-Commit-Position: refs/heads/master@{#384666}