Add CSSTiming to collect aggregate PLT-level stats about CSS.

Add CSSTiming to collect aggregate PLT-level stats about CSS.

We have histograms on micro-metrics like per stylesheet parse time. This
patch adds histograms of how much aggregate time was spent parsing CSS
before the first paint.

This gives us a much better metric for tracking improvements to CSS
related to loading, as improvements directly map to improvements to
first contentful paint.

BUG=642722, 666030
Committed: https://crrev.com/37627d6f7cb982de2c40de899bd18b17c6d6943d
Cr-Commit-Position: refs/heads/master@{#432868}

[Charlie Harrison, 2016-11-01 15:04:03]

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

[commit-bot: I haz the power, 2016-11-01 15:04:20]

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

[Charlie Harrison, 2016-11-01 15:29:41]

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

[commit-bot: I haz the power, 2016-11-01 15:29:59]

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

[Charlie Harrison, 2016-11-01 15:34:33]

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

[commit-bot: I haz the power, 2016-11-01 15:34:54]

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

[Charlie Harrison, 2016-11-01 15:36:25]

csharrison@chromium.org changed reviewers:
+ bmcquade@chromium.org

[Charlie Harrison, 2016-11-01 15:36:26]

Bryan, could you take a first pass at this?

I plan on adding a PageLoad.CSSTiming.Update.BeforeFirstContentfulPaint variant
in a follow up.

These two metrics should allow us to properly the evaluate lazy parsing
experiment.

[commit-bot: I haz the power, 2016-11-01 15:43:37]

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

[commit-bot: I haz the power, 2016-11-01 15:43:38]

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

[Bryan McQuade, 2016-11-04 14:11:30]

Thanks! structurally this looks good. i have some questions about time
resolution & consistency with existing code.

https://codereview.chromium.org/2468913002/diff/60001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_load_timing.h (right):

https://codereview.chromium.org/2468913002/diff/60001/chrome/common/page_load...
chrome/common/page_load_metrics/page_load_timing.h:102:
base::Optional<base::TimeDelta> author_style_sheet_parse_duration_before_fcp;
I think we're overdue for providing some amount of hierarchical structure here.
I should have defined a sub-struct that contained the parse-related timings, for
example. I'll go back and do that cleanup, but to start the process of providing
hierarchy and organization now, can you add a 'struct StyleSheetTiming' which
has one field for your new data point?

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSTiming.h (right):

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSTiming.h:28: // In microseconds.
iirc most of our other fields operate in seconds, rather than micros. if you are
using double, let's keep it consistent and also use seconds here. it'd be really
easy for someone who's used to perf timing code operating on seconds to
mistakenly misinterpret this as being in seconds, if they happen to miss the
comment.

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleSheetContents.cpp:331: double startTime
= monotonicallyIncreasingTimeMS();
can we call this startTimeMs? any time we're dealing with double rather than a
well typed class like TimeDelta we should declare the units in the name to
minimize chance for bugs. i'm also not sure we need to convert to millis here.
though i'm not an expert when it comes to doubles, my sense is that they can
meet our needs here with second resolution.

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceTiming.cpp (right):

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceTiming.cpp:413:
timing->authorStyleSheetParseDurationBeforeFCP());
re: returning micros here, for the purpose of page load perf tracking, if you
have a value that's below 1ms, is it useful to know its microsecond resolution,
as opposed to just rounding it up to 1ms, or down to 0ms? my sense is that it's
not really interesting at page load performance scale to track anything beyond
millisecond resolution, and i think there's a lot of value to being consistent
with our existing code, so i'm only supportive of doing this if you really think
we're going to learn something interesting here by having greater than
millisecond resolution.

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebPerformance.h (right):

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebPerformance.h:101: // In microseconds (seconds
are too coarse).
are seconds really too coarse if you are using double precision? reading
https://en.wikipedia.org/wiki/Double-precision_floating-point_format it seems
like you have plenty of precision in a double.

[Charlie Harrison, 2016-11-04 16:05:19]

Thanks for the review. I think reporting MS makes sense, though I expect in
practice we won't get many reports with > 1s of parse time before FCP.

https://codereview.chromium.org/2468913002/diff/60001/chrome/common/page_load...
File chrome/common/page_load_metrics/page_load_timing.h (right):

https://codereview.chromium.org/2468913002/diff/60001/chrome/common/page_load...
chrome/common/page_load_metrics/page_load_timing.h:102:
base::Optional<base::TimeDelta> author_style_sheet_parse_duration_before_fcp;
On 2016/11/04 14:11:29, Bryan McQuade wrote:
> I think we're overdue for providing some amount of hierarchical structure
here.
> I should have defined a sub-struct that contained the parse-related timings,
for
> example. I'll go back and do that cleanup, but to start the process of
providing
> hierarchy and organization now, can you add a 'struct StyleSheetTiming' which
> has one field for your new data point?

Done.

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/CSSTiming.h (right):

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/CSSTiming.h:28: // In microseconds.
On 2016/11/04 14:11:30, Bryan McQuade wrote:
> iirc most of our other fields operate in seconds, rather than micros. if you
are
> using double, let's keep it consistent and also use seconds here. it'd be
really
> easy for someone who's used to perf timing code operating on seconds to
> mistakenly misinterpret this as being in seconds, if they happen to miss the
> comment.

Done.

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/css/StyleSheetContents.cpp (right):

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/css/StyleSheetContents.cpp:331: double startTime
= monotonicallyIncreasingTimeMS();
On 2016/11/04 14:11:30, Bryan McQuade wrote:
> can we call this startTimeMs? any time we're dealing with double rather than a
> well typed class like TimeDelta we should declare the units in the name to
> minimize chance for bugs. i'm also not sure we need to convert to millis here.
> though i'm not an expert when it comes to doubles, my sense is that they can
> meet our needs here with second resolution.

Changed the name. Note that our local histogram here expects microseconds.

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/timing/PerformanceTiming.cpp (right):

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/timing/PerformanceTiming.cpp:413:
timing->authorStyleSheetParseDurationBeforeFCP());
On 2016/11/04 14:11:30, Bryan McQuade wrote:
> re: returning micros here, for the purpose of page load perf tracking, if you
> have a value that's below 1ms, is it useful to know its microsecond
resolution,
> as opposed to just rounding it up to 1ms, or down to 0ms? my sense is that
it's
> not really interesting at page load performance scale to track anything beyond
> millisecond resolution, and i think there's a lot of value to being consistent
> with our existing code, so i'm only supportive of doing this if you really
think
> we're going to learn something interesting here by having greater than
> millisecond resolution.

Let me try to return second resolution.

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/publ...
File third_party/WebKit/public/web/WebPerformance.h (right):

https://codereview.chromium.org/2468913002/diff/60001/third_party/WebKit/publ...
third_party/WebKit/public/web/WebPerformance.h:101: // In microseconds (seconds
are too coarse).
On 2016/11/04 14:11:30, Bryan McQuade wrote:
> are seconds really too coarse if you are using double precision? reading
> https://en.wikipedia.org/wiki/Double-precision_floating-point_format it seems
> like you have plenty of precision in a double.

As discussed offline, I think this really has to do with microsecond clamping
done in PerformanceTiming. I'll try to fix it up.

[Charlie Harrison, 2016-11-08 02:05:13]

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

[commit-bot: I haz the power, 2016-11-08 02:05:44]

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-11-08 04:18:53]

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

[commit-bot: I haz the power, 2016-11-08 04:18:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Charlie Harrison, 2016-11-08 19:18:16]

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

[commit-bot: I haz the power, 2016-11-08 19:19:05]

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-11-08 22:06:12]

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

[commit-bot: I haz the power, 2016-11-08 22:06:13]

Dry run: This issue passed the CQ dry run.

[Bryan McQuade, 2016-11-09 18:56:50]

LGTM, thanks! Sorry for the delayed review.

https://codereview.chromium.org/2468913002/diff/100001/chrome/test/data/page_...
File chrome/test/data/page_load_metrics/page_with_css.html (right):

https://codereview.chromium.org/2468913002/diff/100001/chrome/test/data/page_...
chrome/test/data/page_load_metrics/page_with_css.html:3: <link rel="stylesheet"
href="/dromaeo/application.css">
maybe include an HTML comment to explain why there are so many duplicate
entries?

[Charlie Harrison, 2016-11-10 19:39:12]

csharrison@chromium.org changed reviewers:
+ timloh@chromium.org

[Charlie Harrison, 2016-11-10 19:39:12]

Thanks! timloh: does this look okay to you? I'm hoping this infrastructure will
help assess the loading impact of lazy parsing and other CSS optimizations.

https://codereview.chromium.org/2468913002/diff/100001/chrome/test/data/page_...
File chrome/test/data/page_load_metrics/page_with_css.html (right):

https://codereview.chromium.org/2468913002/diff/100001/chrome/test/data/page_...
chrome/test/data/page_load_metrics/page_with_css.html:3: <link rel="stylesheet"
href="/dromaeo/application.css">
On 2016/11/09 18:56:50, Bryan McQuade wrote:
> maybe include an HTML comment to explain why there are so many duplicate
> entries?

Done.

[Timothy Loh, 2016-11-11 04:19:22]

I skimmed it and it looks reasonable.

https://codereview.chromium.org/2468913002/diff/120001/chrome/test/data/page_...
File chrome/test/data/page_load_metrics/page_with_css.html (right):

https://codereview.chromium.org/2468913002/diff/120001/chrome/test/data/page_...
chrome/test/data/page_load_metrics/page_with_css.html:8: <link rel="stylesheet"
href="/dromaeo/application.css">
I'd expect that we only parse it once though...

[Charlie Harrison, 2016-11-11 16:27:11]

https://codereview.chromium.org/2468913002/diff/120001/chrome/test/data/page_...
File chrome/test/data/page_load_metrics/page_with_css.html (right):

https://codereview.chromium.org/2468913002/diff/120001/chrome/test/data/page_...
chrome/test/data/page_load_metrics/page_with_css.html:8: <link rel="stylesheet"
href="/dromaeo/application.css">
On 2016/11/11 04:19:22, Timothy Loh wrote:
> I'd expect that we only parse it once though...

It is odd. The reason is that application.css is uncacheable (in parsed form)
because it has a syntactically invalid first rule. This is fragile so I'll
change the URL here.

I'm not quite sure why the first rule is deemed invalid, even when I change it
to @charset "utf-8", so I'm investigating that separately.

[Charlie Harrison, 2016-11-11 19:11:33]

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

[commit-bot: I haz the power, 2016-11-11 19:11:58]

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-11-11 19:22:59]

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

[commit-bot: I haz the power, 2016-11-11 19:23:00]

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

[Charlie Harrison, 2016-11-14 21:28:16]

Thanks. The problem was we were erroring out on parsing the css because it was
being transferred with bad mime type. I've fixed that here.

Additionally, we need the parsing to take > 1ms, or else it gets clamped in the
render_frame_observer.

PTAL.

[Timothy Loh, 2016-11-16 02:25:56]

On 2016/11/14 21:28:16, Charlie Harrison wrote:
> Thanks. The problem was we were erroring out on parsing the css because it was
> being transferred with bad mime type. I've fixed that here.
> 
> Additionally, we need the parsing to take > 1ms, or else it gets clamped in
the
> render_frame_observer.
> 
> PTAL.

lgtm

[Charlie Harrison, 2016-11-16 02:31:41]

csharrison@chromium.org changed reviewers:
+ rkaplow@chromium.org

[Charlie Harrison, 2016-11-16 02:31:42]

Thanks!
+rkaplow for histograms.xml

[rkaplow, 2016-11-16 06:09:34]

lgtm

[Charlie Harrison, 2016-11-16 12:57:35]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-11-16 12:57:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org
Link to the patchset: https://codereview.chromium.org/2468913002/#ps160001
(title: "add license to css file (trybots prev)")

[commit-bot: I haz the power, 2016-11-16 12:57:59]

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-11-16 13:06:35]

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

[commit-bot: I haz the power, 2016-11-16 13:06:36]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Charlie Harrison, 2016-11-16 18:38:53]

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

[commit-bot: I haz the power, 2016-11-16 18:39:38]

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

[Charlie Harrison, 2016-11-16 18:41:35]

csharrison@chromium.org changed reviewers:
+ mkwst@chromium.org

[Charlie Harrison, 2016-11-16 18:41:36]

Mike, would you review page_load_metrics_messages?

[Charlie Harrison, 2016-11-16 19:47:49]

Description was changed from

==========
Add CSSTiming to collect aggregate PLT-level stats about CSS.

We have histograms on micro-metrics like per stylesheet parse time. This
patch adds histograms of how much aggregate time was spent parsing CSS
before the first paint.

This gives us a much better metric for tracking improvements to CSS
related to loading, as improvements directly map to improvements to
first contentful paint.

BUG=642722
==========

to

==========
Add CSSTiming to collect aggregate PLT-level stats about CSS.

We have histograms on micro-metrics like per stylesheet parse time. This
patch adds histograms of how much aggregate time was spent parsing CSS
before the first paint.

This gives us a much better metric for tracking improvements to CSS
related to loading, as improvements directly map to improvements to
first contentful paint.

BUG=642722,666030
==========

[commit-bot: I haz the power, 2016-11-16 20:24:23]

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

[commit-bot: I haz the power, 2016-11-16 20:24:24]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-11-17 14:33:00]

IPC LGTM.

[Charlie Harrison, 2016-11-17 14:33:40]

Thanks much!

[Charlie Harrison, 2016-11-17 14:33:52]

The CQ bit was checked by csharrison@chromium.org

[Charlie Harrison, 2016-11-17 14:33:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
bmcquade@chromium.org, timloh@chromium.org, rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/2468913002/#ps180001
(title: "fix browser test")

[commit-bot: I haz the power, 2016-11-17 14:34:17]

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-11-17 14:41:01]

Description was changed from

==========
Add CSSTiming to collect aggregate PLT-level stats about CSS.

We have histograms on micro-metrics like per stylesheet parse time. This
patch adds histograms of how much aggregate time was spent parsing CSS
before the first paint.

This gives us a much better metric for tracking improvements to CSS
related to loading, as improvements directly map to improvements to
first contentful paint.

BUG=642722,666030
==========

to

==========
Add CSSTiming to collect aggregate PLT-level stats about CSS.

We have histograms on micro-metrics like per stylesheet parse time. This
patch adds histograms of how much aggregate time was spent parsing CSS
before the first paint.

This gives us a much better metric for tracking improvements to CSS
related to loading, as improvements directly map to improvements to
first contentful paint.

BUG=642722,666030
==========

[commit-bot: I haz the power, 2016-11-17 14:41:02]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-11-17 14:43:01]

Description was changed from

==========
Add CSSTiming to collect aggregate PLT-level stats about CSS.

We have histograms on micro-metrics like per stylesheet parse time. This
patch adds histograms of how much aggregate time was spent parsing CSS
before the first paint.

This gives us a much better metric for tracking improvements to CSS
related to loading, as improvements directly map to improvements to
first contentful paint.

BUG=642722,666030
==========

to

==========
Add CSSTiming to collect aggregate PLT-level stats about CSS.

We have histograms on micro-metrics like per stylesheet parse time. This
patch adds histograms of how much aggregate time was spent parsing CSS
before the first paint.

This gives us a much better metric for tracking improvements to CSS
related to loading, as improvements directly map to improvements to
first contentful paint.

BUG=642722,666030

Committed: https://crrev.com/37627d6f7cb982de2c40de899bd18b17c6d6943d
Cr-Commit-Position: refs/heads/master@{#432868}
==========

[commit-bot: I haz the power, 2016-11-17 14:43:03]

Patchset 10 (id:??) landed as
https://crrev.com/37627d6f7cb982de2c40de899bd18b17c6d6943d
Cr-Commit-Position: refs/heads/master@{#432868}