Update FirstMeaningfulPaint to use Blink's implementation

Update FirstMeaningfulPaint to use Blink's implementation

Note to perf sheriffs: this is going to affect timeToFirstMeaningfulPaint
values in page_cycler_v2 benchmarks.

This updates FirstMeaningfulPaint metric to use trace events logged by
Blink's FirstMeaningfulPaintDetector, instead of computing layout
significance from FrameView::performLayout trace event.

Since Blink implementation counts more layout objects / uses the end
time of the paint phase, this patch will change TTFMP value, but it
increases accuracy (see https://crbug.com/638124#c8 for details).

This requires the new trace event added in https://crrev.com/2276573003/.

BUG=chromium:638124
Committed: https://chromium.googlesource.com/external/github.com/catapult-project/catapult/+/7bf2ae5c26055537f6c07d88d15d29485bca65b1

[Kunihiko Sakamoto, 2016-08-29 06:39:00]

Patchset #1 (id:1) has been deleted

[Kunihiko Sakamoto, 2016-08-29 06:41:39]

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

[commit-bot: I haz the power, 2016-08-29 06:41:43]

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, 2016-08-29 07:03:36]

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

[commit-bot: I haz the power, 2016-08-29 07:03:37]

Dry run: This issue passed the CQ dry run.

[Kunihiko Sakamoto, 2016-08-30 05:43:20]

Description was changed from

==========
WIP
==========

to

==========
Update FirstMeaningfulPaint to use Blink's implementation

Note to perf sheriffs: this is going to affect timeToFirstMeaningfulPaint
values in page_cycler_v2 benchmarks.

This updates FirstMeaningfulPaint metric to use trace events logged by
Blink's FirstMeaningfulPaintDetector, instead of computing layout
significance from FrameView::performLayout trace event.

Since Blink implementation counts more layout objects / uses the end
time of the paint phase, this patch will change TTFMP value, but it
increases accuracy (see https://crbug.com/638124#c8 for details).

BUG=chromium:638124
==========

[Kunihiko Sakamoto, 2016-08-30 05:45:50]

ksakamoto@chromium.org changed reviewers:
+ kouhei@chromium.org, nednguyen@google.com

[Kunihiko Sakamoto, 2016-08-30 05:45:51]

PTAL

[kouhei (in TOK), 2016-08-31 00:43:05]

lgtm

Ned: We probably need the ref builds update to land this.

[kouhei (in TOK), 2016-08-31 00:45:04]

> Ned: We probably need the ref builds update to land this.
ksakamoto-san: Would you note the required Chromium revision num in the CL
description and request the ref builds update (to aiolos@)?

[nednguyen, 2016-08-31 00:46:48]

On 2016/08/31 00:45:04, kouhei wrote:
> > Ned: We probably need the ref builds update to land this.
> ksakamoto-san: Would you note the required Chromium revision num in the CL
> description and request the ref builds update (to aiolos@)?

I recall that we disabled running loading metrics on the reference build?

[nednguyen, 2016-08-31 00:52:02]

https://codereview.chromium.org/2259723002/diff/20001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/loading_metric.html (right):

https://codereview.chromium.org/2259723002/diff/20001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric.html:328: var
timeToFirstMeaningfulPaint = paintEvent.start - navigationStart.start;
paintEvent.end - navigationStart.start?

https://codereview.chromium.org/2259723002/diff/20001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric.html:336: return
{firstMeaningfulPaint: paintEvent.start, url: url};
paintEvent.end?

https://codereview.chromium.org/2259723002/diff/20001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/loading_metric_test.html (right):

https://codereview.chromium.org/2259723002/diff/20001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric_test.html:135: duration:
0.0,
Should we use a non zero duration since we want our test to cover the fact that
we pick the end time?

[Kunihiko Sakamoto, 2016-08-31 00:54:36]

Description was changed from

==========
Update FirstMeaningfulPaint to use Blink's implementation

Note to perf sheriffs: this is going to affect timeToFirstMeaningfulPaint
values in page_cycler_v2 benchmarks.

This updates FirstMeaningfulPaint metric to use trace events logged by
Blink's FirstMeaningfulPaintDetector, instead of computing layout
significance from FrameView::performLayout trace event.

Since Blink implementation counts more layout objects / uses the end
time of the paint phase, this patch will change TTFMP value, but it
increases accuracy (see https://crbug.com/638124#c8 for details).

BUG=chromium:638124
==========

to

==========
Update FirstMeaningfulPaint to use Blink's implementation

Note to perf sheriffs: this is going to affect timeToFirstMeaningfulPaint
values in page_cycler_v2 benchmarks.

This updates FirstMeaningfulPaint metric to use trace events logged by
Blink's FirstMeaningfulPaintDetector, instead of computing layout
significance from FrameView::performLayout trace event.

Since Blink implementation counts more layout objects / uses the end
time of the paint phase, this patch will change TTFMP value, but it
increases accuracy (see https://crbug.com/638124#c8 for details).

This requires a new trace event added in https://crrev.com/2276573003/.

BUG=chromium:638124
==========

[kouhei (in TOK), 2016-08-31 01:03:55]

On 2016/08/31 00:46:48, nednguyen wrote:
> On 2016/08/31 00:45:04, kouhei wrote:
> > > Ned: We probably need the ref builds update to land this.
> > ksakamoto-san: Would you note the required Chromium revision num in the CL
> > description and request the ref builds update (to aiolos@)?
> 
> I recall that we disabled running loading metrics on the reference build?

That's right, let me retract this. crbug.com/619254

[Kunihiko Sakamoto, 2016-08-31 01:04:40]

https://codereview.chromium.org/2259723002/diff/20001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/loading_metric.html (right):

https://codereview.chromium.org/2259723002/diff/20001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric.html:328: var
timeToFirstMeaningfulPaint = paintEvent.start - navigationStart.start;
On 2016/08/31 00:52:02, nednguyen wrote:
> paintEvent.end - navigationStart.start?

The new event is not a duration event but a mark event logged at the end of
paint phase, so start == end.

https://codereview.chromium.org/2259723002/diff/20001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/loading_metric_test.html (right):

https://codereview.chromium.org/2259723002/diff/20001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric_test.html:135: duration:
0.0,
On 2016/08/31 00:52:02, nednguyen wrote:
> Should we use a non zero duration since we want our test to cover the fact
that
> we pick the end time?

Since this is a non-duration event marked at the end of Blink paint.

[nednguyen, 2016-08-31 01:12:40]

lgtm

https://codereview.chromium.org/2259723002/diff/20001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/loading_metric.html (right):

https://codereview.chromium.org/2259723002/diff/20001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric.html:328: var
timeToFirstMeaningfulPaint = paintEvent.start - navigationStart.start;
On 2016/08/31 01:04:39, Kunihiko Sakamoto wrote:
> On 2016/08/31 00:52:02, nednguyen wrote:
> > paintEvent.end - navigationStart.start?
> 
> The new event is not a duration event but a mark event logged at the end of
> paint phase, so start == end.

I see. Can you rename this to s.t like fmpMarkerEvent?

[Kunihiko Sakamoto, 2016-08-31 01:20:58]

Description was changed from

==========
Update FirstMeaningfulPaint to use Blink's implementation

Note to perf sheriffs: this is going to affect timeToFirstMeaningfulPaint
values in page_cycler_v2 benchmarks.

This updates FirstMeaningfulPaint metric to use trace events logged by
Blink's FirstMeaningfulPaintDetector, instead of computing layout
significance from FrameView::performLayout trace event.

Since Blink implementation counts more layout objects / uses the end
time of the paint phase, this patch will change TTFMP value, but it
increases accuracy (see https://crbug.com/638124#c8 for details).

This requires a new trace event added in https://crrev.com/2276573003/.

BUG=chromium:638124
==========

to

==========
Update FirstMeaningfulPaint to use Blink's implementation

Note to perf sheriffs: this is going to affect timeToFirstMeaningfulPaint
values in page_cycler_v2 benchmarks.

This updates FirstMeaningfulPaint metric to use trace events logged by
Blink's FirstMeaningfulPaintDetector, instead of computing layout
significance from FrameView::performLayout trace event.

Since Blink implementation counts more layout objects / uses the end
time of the paint phase, this patch will change TTFMP value, but it
increases accuracy (see https://crbug.com/638124#c8 for details).

This requires the new trace event added in https://crrev.com/2276573003/.

BUG=chromium:638124
==========

[Kunihiko Sakamoto, 2016-08-31 01:21:59]

Thanks!

https://codereview.chromium.org/2259723002/diff/20001/tracing/tracing/metrics...
File tracing/tracing/metrics/system_health/loading_metric.html (right):

https://codereview.chromium.org/2259723002/diff/20001/tracing/tracing/metrics...
tracing/tracing/metrics/system_health/loading_metric.html:328: var
timeToFirstMeaningfulPaint = paintEvent.start - navigationStart.start;
On 2016/08/31 01:12:40, nednguyen wrote:
> On 2016/08/31 01:04:39, Kunihiko Sakamoto wrote:
> > On 2016/08/31 00:52:02, nednguyen wrote:
> > > paintEvent.end - navigationStart.start?
> > 
> > The new event is not a duration event but a mark event logged at the end of
> > paint phase, so start == end.
> 
> I see. Can you rename this to s.t like fmpMarkerEvent?

Done.

[Kunihiko Sakamoto, 2016-08-31 01:22:05]

The CQ bit was checked by ksakamoto@chromium.org

[Kunihiko Sakamoto, 2016-08-31 01:22:05]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2259723002/#ps40001
(title: "paintEvent -> fmpMarkerEvent")

[commit-bot: I haz the power, 2016-08-31 01:22:09]

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, 2016-08-31 01:23:17]

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

[commit-bot: I haz the power, 2016-08-31 01:23:18]

Try jobs failed on following builders:
  Catapult Android Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20An...)
  Catapult Linux Tryserver on master.tryserver.client.catapult (JOB_FAILED,
https://build.chromium.org/p/tryserver.client.catapult/builders/Catapult%20Li...)

[Kunihiko Sakamoto, 2016-08-31 01:27:56]

The CQ bit was checked by ksakamoto@chromium.org

[Kunihiko Sakamoto, 2016-08-31 01:27:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
nednguyen@google.com, kouhei@chromium.org
Link to the patchset: https://codereview.chromium.org/2259723002/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-08-31 01:28:08]

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

[nednguyen, 2016-08-31 01:35:20]

On 2016/08/31 01:28:08, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

Also does this allow us remove some of existing tracing categories for
page_cycler_v2 benchmark, e.g: 'disabled-by-default-blink.debug.layout'? If so,
can you make a follow up CL to remove them to reduce the overhead further? :-)

[Kunihiko Sakamoto, 2016-08-31 01:37:14]

On 2016/08/31 01:35:20, nednguyen wrote:
> On 2016/08/31 01:28:08, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> Also does this allow us remove some of existing tracing categories for
> page_cycler_v2 benchmark, e.g: 'disabled-by-default-blink.debug.layout'? If
so,
> can you make a follow up CL to remove them to reduce the overhead further? :-)

Yes! I'll remove them in a follow up CL.

[commit-bot: I haz the power, 2016-08-31 01:50:44]

Description was changed from

==========
Update FirstMeaningfulPaint to use Blink's implementation

Note to perf sheriffs: this is going to affect timeToFirstMeaningfulPaint
values in page_cycler_v2 benchmarks.

This updates FirstMeaningfulPaint metric to use trace events logged by
Blink's FirstMeaningfulPaintDetector, instead of computing layout
significance from FrameView::performLayout trace event.

Since Blink implementation counts more layout objects / uses the end
time of the paint phase, this patch will change TTFMP value, but it
increases accuracy (see https://crbug.com/638124#c8 for details).

This requires the new trace event added in https://crrev.com/2276573003/.

BUG=chromium:638124
==========

to

==========
Update FirstMeaningfulPaint to use Blink's implementation

Note to perf sheriffs: this is going to affect timeToFirstMeaningfulPaint
values in page_cycler_v2 benchmarks.

This updates FirstMeaningfulPaint metric to use trace events logged by
Blink's FirstMeaningfulPaintDetector, instead of computing layout
significance from FrameView::performLayout trace event.

Since Blink implementation counts more layout objects / uses the end
time of the paint phase, this patch will change TTFMP value, but it
increases accuracy (see https://crbug.com/638124#c8 for details).

This requires the new trace event added in https://crrev.com/2276573003/.

BUG=chromium:638124

Committed:
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...
==========

[commit-bot: I haz the power, 2016-08-31 01:50:46]

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/external/github.com/catapult-project/catapu...