core(lantern): FCP improvements #7513

patrickhulce · 2019-03-14T17:40:53Z

Summary

While doing some investigation into resilient FCP, I noticed a few cases popping up where we could be more accurate.

Specifically:

Previously, if a network request was for a script, it finished before FCP, and it had a render blocking priority, we would include it in the FCP graph even if the EvaluateScript task for that script happened after FCP. This doesn't make sense so we now exclude such scripts. This in particular was a source of variance as sometimes FCP would happen to be before the script finished downloading and sometimes it wouldn't. Now such scripts will always be excluded.
Previously, we were only including EvaluateScript tasks for blocking scripts, no other CPU nodes were included. That frequently misses some pretty big/important tasks like parsing the HTML, the first layout, etc. We now include the first layout/paint/parse HTML task if they happened before FCP.

Impact

Huge boosts to our FCP accuracy!! 🎉 FMP takes a slight hit to be consistent here, but it doesn't move any sites to worse buckets and FCP has a much higher priority/visibility anyway.

This also shrunk the observed variance for the Hulu case from +/- 2000ms to +/-200ms, which is far more tolerable.

brendankenny

a few clarifying questions, a few actually important :)

brendankenny · 2019-03-20T23:11:30Z

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

  }

  /**
   * @param {Node} dependencyGraph
-   * @param {LH.Artifacts.TraceOfTab} traceOfTab
+   * @param {number} fcpTs


some docs would be good. Not at clear from the outside why this is called fcpTs but lantern-first-meaningful-paint.js is passing in fmp, for instance :)

brendankenny · 2019-03-20T23:15:48Z

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

+   * @param {number} fcpTs
+   * @param {function(NetworkNode):boolean} networkFilter
+   * @param {function(CPUNode):boolean=} cpuFilter
+   * @return {{possiblyRenderBlockingScriptUrls: Set<string>, actuallyRenderBlockingScriptUrls: Set<string>, blockingCpuNodeIds: Set<string>}}


actually reads like possibly blocking nodes really weren't. definitelyRenderBlockingScriptUrls? :)

haha good point sounds good 👍

well, on further reflection definitely feels a tad too strong :)

we don't know with 100% certainty, but it's the delta between the two is definitelyNotRenderBlocking, maybe I just return that directly

brendankenny · 2019-03-20T23:17:09Z

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

+   * @param {Node} graph
+   * @param {number} fcpTs
+   * @param {function(NetworkNode):boolean} networkFilter
+   * @param {function(CPUNode):boolean=} cpuFilter


does {(function(CPUNode):boolean)=} work? = isn't supposed to be applicable to return types, but with tsc you never know, so it ends up hard to read

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

brendankenny · 2019-03-20T23:45:18Z

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

-   * @param {LH.Artifacts.TraceOfTab} traceOfTab
+   * @param {number} fcpTs
+   * @param {function(NetworkNode):boolean} blockingScriptFilter
+   * @param {function(CPUNode):boolean=} cpuNodeFilter


cpuNodeFilter does the opposite of what I expected...maybe incorporate block/blocking into its name as well?

yeah this was a bad name, renamed both of them to extraBlockingCpuNodesToIncludeFilter

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

paulirish · 2019-03-21T01:23:32Z

Previously, if a network request was for a script, it finished before FCP, and it had a render blocking priority, we would include it in the FCP graph even if the EvaluateScript task for that script happened after FCP. This doesn't make sense so we now exclude such scripts

Yeah this doesn't make sense at all. :) Like.... the script is render blocking so why would chrome do a real contentful paint before the evaluateScript??? That's not really supposed to happen. Can we look closer at what's going on here?

We now include the first layout/paint/parse HTML task if they happened before FCP.

that sounds awesome.

patrickhulce · 2019-03-21T01:56:04Z

Like.... the script is render blocking so why would chrome do a real contentful paint before the evaluateScript??? That's not really supposed to happen. Can we look closer at what's going on here?

Oh the thing is it wasn't really render-blocking. It just had a render-blocking priority. Scripts added to the head by other scripts for example aren't render-blocking but will be requested by Chrome with the same priority as render-blocking scripts. We exclude these types of scripts from the optimistic estimate but still include them in the pessimistic one since they're usually important and could have been the script to actually render the content, but aren't necessarily required.

This PR now uses the extra bit of information about when the EvaluateScript finished to eliminate the ones that definitely weren't required for paint.

brendankenny

nice, I like the cleanup!

One big question and a few documentation comments

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

brendankenny · 2019-03-25T20:33:04Z

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

+    const firstPaint = cpuNodes.find(node => node.childEvents.some(e => e.name === 'Paint'));
+    if (firstPaint) blockingCpuNodeIds.add(firstPaint.id);
+    const firstParse = cpuNodes.find(node => node.childEvents.some(e => e.name === 'ParseHTML'));
+    if (firstParse) blockingCpuNodeIds.add(firstParse.id);


are the FCP/FMP results weird (and not worth keeping) if any of these are missing?

theoretically yes, but in practice I've seen some of these be shifted slightly so I wouldn't want to throw it all away

maybe another candidate for #7041 ?

maybe another candidate for #7041 ?

yeah, probably the best thing before we start throwing on it

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

brendankenny · 2019-03-25T20:37:14Z

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

-      return node.hasRenderBlockingPriority();
+        const url = node.record.url;
+        // If the URL definitely wasn't render-blocking then we filter it out.
+        if (definitelyNotRenderBlockingScriptUrls.has(url)) {


definitelyNotRenderBlockingScriptUrls is a little confusing inside of getBlockingCpuData, but really nice here!

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

brendankenny

LGTM!

brendankenny · 2019-03-25T21:23:47Z

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

+
+    // A script is *definitely not* render blocking if its EvaluateScript task started after filterTimestamp.
+    // We'll check `cpuNodes` to see if we can find its EvaluateScript.
+    // If we can't, that means it started after filterTimestamp, and we add it to our set.


ah, this is great

brendankenny · 2019-03-25T21:24:31Z

@paulirish not sure if you want to take another look before we land

paulirish

the comments are crazy valuable, especially for this kind of thing.
thanks.

lg here.

brendankenny · 2019-03-25T22:04:51Z

looks like need a new golden lhr and render-blocking-resources expectations need adjusting

patrickhulce · 2019-03-26T01:15:51Z

sorry y'all the fix for rendering blocking smoke is slightly more involved but I think it makes the definitelyNotRenderBlocking computation a little clearer too in the process.

just one more quick look? :D

the good news is we keep goin' up accuracy-wise! 😃

this is the incremental progress on what we already had 💣 💥 🥇

brendankenny

still LGTM :)

I think it makes the definitelyNotRenderBlocking computation a little clearer too in the process.

agreed!

lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

brendankenny · 2019-03-27T19:55:15Z

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

+      extraBlockingCpuNodesToIncludeFilter
+  ) {
+    /** @type {Map<string, CPUNode>} */
+    const scriptUrlToNodeMap = new Map();


maybe a comment about it being a map from script URL to the earliest associated EvaluateScript CPUNode?

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

brendankenny · 2019-03-27T20:10:31Z

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

+    const firstPaint = cpuNodes.find(node => node.childEvents.some(e => e.name === 'Paint'));
+    if (firstPaint) blockingCpuNodeIds.add(firstPaint.id);
+    const firstParse = cpuNodes.find(node => node.childEvents.some(e => e.name === 'ParseHTML'));
+    if (firstParse) blockingCpuNodeIds.add(firstParse.id);


maybe another candidate for #7041 ?

yeah, probably the best thing before we start throwing on it

lighthouse-core/computed/metrics/lantern-first-contentful-paint.js

paulirish

works for me. excited about this.

paulirish · 2019-03-27T21:59:01Z

We discussed shipping this in v5 (#7752), so we may wait to land it.

brendankenny · 2019-04-09T17:55:38Z

I think we're good to go here now, right?

patrickhulce · 2019-04-09T18:02:17Z

We are! let's do it!

patrickhulce added 2 commits March 14, 2019 12:30

core(lantern): FCP improvements

eae09ce

update snapshots/fixtures

3110ec7

patrickhulce requested review from brendankenny and paulirish as code owners March 14, 2019 17:40

brendankenny reviewed Mar 20, 2019

View reviewed changes

patrickhulce added 2 commits March 20, 2019 21:26

feedback

cf71333

update sample json

3d225c6

patrickhulce requested a review from exterkamp as a code owner March 21, 2019 02:27

lint

58b33ec

brendankenny reviewed Mar 25, 2019

View reviewed changes

feedback

f5ab804

brendankenny approved these changes Mar 25, 2019

View reviewed changes

paulirish reviewed Mar 25, 2019

View reviewed changes

be more forgiving for scripts we could not find at all

227e683

patrickhulce force-pushed the lantern_fcp_exclude_late_scripts branch from dea2ef5 to 227e683 Compare March 26, 2019 01:14

sample json

add4913

brendankenny approved these changes Mar 27, 2019

View reviewed changes

patrickhulce added 2 commits March 27, 2019 16:05

more feedback

2179207

one more comment

3c6867e

paulirish reviewed Mar 27, 2019

View reviewed changes

brendankenny mentioned this pull request Mar 28, 2019

Investigate mutation testing #7607

Closed

paulirish added 5.0 chillin ❄ labels Mar 29, 2019

brendankenny removed the chillin ❄ label Apr 8, 2019

brendankenny merged commit c71853f into master Apr 9, 2019

brendankenny deleted the lantern_fcp_exclude_late_scripts branch April 9, 2019 18:19

paulirish mentioned this pull request Apr 17, 2019

☂️ 👣 Breaking changes for v5 #7752

Closed

8 tasks

paulirish mentioned this pull request Apr 30, 2019

[meta] 2019 I/O Burndown #8331

Closed

67 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(lantern): FCP improvements #7513

core(lantern): FCP improvements #7513

patrickhulce commented Mar 14, 2019 •

edited

Loading

brendankenny left a comment

brendankenny Mar 20, 2019

patrickhulce Mar 21, 2019

brendankenny Mar 20, 2019

patrickhulce Mar 21, 2019

patrickhulce Mar 21, 2019 •

edited

Loading

brendankenny Mar 20, 2019

patrickhulce Mar 21, 2019

brendankenny Mar 20, 2019

patrickhulce Mar 21, 2019

paulirish commented Mar 21, 2019

patrickhulce commented Mar 21, 2019 •

edited

Loading

brendankenny left a comment

brendankenny Mar 25, 2019

patrickhulce Mar 25, 2019

brendankenny Mar 27, 2019

brendankenny Mar 25, 2019

brendankenny left a comment

brendankenny Mar 25, 2019

brendankenny commented Mar 25, 2019

paulirish left a comment

brendankenny commented Mar 25, 2019

patrickhulce commented Mar 26, 2019

brendankenny left a comment

brendankenny Mar 27, 2019

brendankenny Mar 27, 2019

paulirish left a comment

paulirish commented Mar 27, 2019

brendankenny commented Apr 9, 2019

patrickhulce commented Apr 9, 2019

core(lantern): FCP improvements #7513

core(lantern): FCP improvements #7513

Conversation

patrickhulce commented Mar 14, 2019 • edited Loading

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Mar 21, 2019

patrickhulce commented Mar 21, 2019 • edited Loading

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny commented Mar 25, 2019

paulirish left a comment

Choose a reason for hiding this comment

brendankenny commented Mar 25, 2019

patrickhulce commented Mar 26, 2019

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish left a comment

Choose a reason for hiding this comment

paulirish commented Mar 27, 2019

brendankenny commented Apr 9, 2019

patrickhulce commented Apr 9, 2019

patrickhulce commented Mar 14, 2019 •

edited

Loading

patrickhulce Mar 21, 2019 •

edited

Loading

patrickhulce commented Mar 21, 2019 •

edited

Loading