core: merge render blocking audits to lantern #4995

patrickhulce · 2018-04-19T17:53:09Z

So this turned out to be quite a doozy 😩

Changes in this PR:

Fixes a bug in byte-efficiency-audit base where the default lantern settings were always used regardless of settings
Adds HTML imports to Lantern's render blocking definition
Merges render blocking links/scripts into single audit
Changes the savings calculation to be based on lantern
- What's the same? We still pass our smoketests and identify all the same things as render blocking! 🎉

What's different about the savings calculation?

Previously we would say that you save Math.max(...downloadTimes) but this wasn't really accurate since you pretty much have to remove all render blocking resource to see this benefit.
Additionally, this assumed that the code is just deferred and you don't need it at all. The new approach is computing "how much faster would this page reach FCP if we inlined all the used CSS from render blocking stylesheets and deferred all scripts" as a result the savings we report are slightly more conservative but much more accurate.
This also means that H2 sites will basically never be flagged since the simulation treats H2 in essentially this way. Doesn't seem so bad though considering the only savings you're seeing with H2 is ~1 RT which in our settings is just 150ms.
Our fancy amp-style detection is less relevant since we don't actually identify savings from addressing a single resource, just all in aggregate and this should be much more conservative. It does mean though that we might slightly overstate savings if AMP-style mitigation didn't kick in on unthrottled but would on throttled.

ref #4333

paulirish · 2018-04-20T16:56:05Z

Merges render blocking links/scripts into single audit

I think we should keep them separate. :)

Handling RB stylesheets is done via reducing, inlining and the critical CSS dance.Handling RB scripts involved a bunch of different techniques. I think they're quite distinct in how you have to reason about them, even though at a browser level they are similar. So i feel its still valuable to report on them individually. Also getting to avoid the super generic "resources" is great..

paulirish · 2018-04-20T17:51:58Z

Discussed this more. Here are the options that we could do:

Start with what's in master. Use lantern for the download timing calculation.
Use this PR, mostly as is. single RB audit instead of the current 2.
Use this PR, but show it as two (CSS & JS). Savings will be the same number. Docs optionally explain that you need to fix both to see the savings.
Take it to the next level. This audit becomes a "Inline critical render-blocking stuff and defer the rest". We use code coverage to calculate how much of the JS/CSS is needed and then calculate if that was inlined and then the rest are deferred. Most sophisticated, but accurate estimation and the user recommendation is very clear.

patrickhulce · 2018-04-20T20:05:23Z

@paulirish says

let's do 4

on it.

patrickhulce · 2018-04-23T17:17:26Z

gentle reminder this is awaitin' review once again :)

paulirish · 2018-04-24T21:53:18Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+    });
+
+    graphWithoutChildren.record._transferSize += totalChildNetworkBytes;
+    const estimateAfterInlineA = simulator.simulate(graphWithoutChildren);


A?
And why not just pull the property off this in one-line?

estimateAfterInlining

just leftover from debugging, fixed

paulirish · 2018-04-24T21:55:53Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+   * @param {Map<string, number>} wastedBytesMap
+   * @return {number}
+   */
+  static estimateSavingsFromInlining(simulator, fcpGraph, deferredIds, wastedBytesMap) {


can you add some comments? it's taking me a long time to grok what's happening within here.

Estimate savings from deferring render-blocking requests and inlining used CSS into the HTML response.

Also a lot of what you wrote in the awesome PR description can be moved into comments. 😀

like

the new approach is computing "how much faster would this page reach FCP if we inlined all the used CSS from render blocking stylesheets and deferred all scripts" as a result the savings we report are slightly more conservative but much more accurate.

paulirish · 2018-04-24T22:19:42Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+      // i.e. the referenced font asset won't become inlined just because you inline the CSS
+      node.traverse(node => deferredNodeIds.add(node.id));
+
+      const wastedMs = nodeTiming.endTime - nodeTiming.startTime;


The wasted cost of each request is from requestSent to responseReceived

btw do you want to round this?

paulirish · 2018-04-24T22:27:16Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+      const willDefer = deferredIds.has(node.id);
+      if (willDefer && node.type === Node.TYPES.NETWORK &&
+          node.record._resourceType === WebInspector.resourceTypes.Stylesheet) {
+        const wastedBytes = wastedBytesMap.get(node.record.url) || 0;


Assume we defer the network request till later, and mark it non-blocking

right?

ya, comment added

paulirish · 2018-04-24T22:27:49Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+      return !willDefer;
+    });
+
+    graphWithoutChildren.record._transferSize += totalChildNetworkBytes;


Add inlined bytes back to the HTML response

paulirish · 2018-04-24T22:42:58Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+   * @param {Map<string, number>} wastedBytesMap
+   * @return {number}
+   */
+  static estimateSavingsFromInlining(simulator, fcpGraph, deferredIds, wastedBytesMap) {


also since the inlining only applies to CSS but this affects both JS and CSS it should be named something that's more balanced. estimateSavingsFromDeferring sg. wdyt?

since it's doing both, I just renamed to estimateSavingsWithGraphs and added better description 👍

paulirish · 2018-04-24T22:49:27Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+    const originalEstimate = simulator.simulate(fcpGraph).timeInMs;
+
+    let totalChildNetworkBytes = 0;
+    const graphWithoutChildren = fcpGraph.cloneWithRelationships(node => {


why is it called this btw? can't really make sense of it.

it was leftover from when everything was inlined, changed to minimalFCPGraph

paulirish · 2018-04-24T22:50:47Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+   * @param {Map<string, number>} wastedBytesMap
+   * @return {number}
+   */
+  static estimateSavingsFromInlining(simulator, fcpGraph, deferredIds, wastedBytesMap) {


And can you also add a comment that we don't inline JS because we assume they can get a [defer] attribute

paulirish · 2018-04-24T22:54:04Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+    const fcpTsInMs = traceOfTab.timestamps.firstContentfulPaint / 1000;
+
+    const nodeTimingMap = fcpSimulation.optimisticEstimate.nodeTiming;
+    const nodesByUrl = keyByUrl(Array.from(nodeTimingMap.keys()));


can we instead just have a helper function of like getNodeTimingForUrl(nodeTimingMap, url){ ..

and the helper function loops through the keys to find the matching record?

paulirish · 2018-04-24T22:54:57Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+    const fcpSimulation = await artifacts.requestFirstContentfulPaint(metricComputationData);
+    const fcpTsInMs = traceOfTab.timestamps.firstContentfulPaint / 1000;
+
+    const nodeTimingMap = fcpSimulation.optimisticEstimate.nodeTiming;


side note i wish this was called nodeTimings or nodeTimingMap to differentiate from the singular case within it.

yeah we can rename in separate PR 👍

patrickhulce · 2018-04-25T18:29:00Z

@paulirish all done 'cept the helper method business, not 100% I understood what that wins us

brendankenny

apparently too slow

brendankenny · 2018-04-25T19:39:39Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+
+      const {node, nodeTiming} = nodesByUrl[resource.tag.url];
+
+      // Mark this node and all it's dependents as deferrable


brendankenny · 2018-04-25T19:45:40Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+  static async computeWastedCSSBytes(artifacts, context) {
+    const wastedBytesByUrl = new Map();
+    try {
+      const results = await UnusedCSS.audit(artifacts, context);


maybe a TODO that this should be pulled out into a computed artifact?

brendankenny · 2018-04-25T19:45:59Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+      for (const item of results.details.items) {
+        wastedBytesByUrl.set(item.url, item.wastedBytes);
+      }
+    } catch (_) {}


whats the failure mode here?

assume all bytes were used and inline all CSS

brendankenny · 2018-04-25T19:46:24Z

lighthouse-core/audits/byte-efficiency/render-blocking-resources.js

+    const simulatorData = {devtoolsLog, settings: context.settings};
+    const traceOfTab = await artifacts.requestTraceOfTab(trace);
+    const simulator = await artifacts.requestLoadSimulator(simulatorData);
+    const wastedBytesMap = await RenderBlockingResources.computeWastedCSSBytes(artifacts, context);


wastedCssBytesMap? Or it's more like cssBytesByUrl or something in function

core: merge render blocking audits to lantern

29b3409

patrickhulce added the I/O-Expedite label Apr 19, 2018

patrickhulce requested review from brendankenny and paulirish as code owners April 19, 2018 17:53

devtools-bot added the waiting4reviewer label Apr 19, 2018

patrickhulce added waiting4committer and removed waiting4reviewer labels Apr 20, 2018

paulirish mentioned this pull request Apr 20, 2018

[meta] 2018 I/O Burndown #5008

Closed

82 tasks

option #4

7f449c1

devtools-bot added waiting4reviewer and removed waiting4committer labels Apr 20, 2018

patrickhulce added 2 commits April 20, 2018 16:31

tests

c2b340c

Merge branch 'master' into combine_render_blocking

e08bb88

paulirish requested changes Apr 24, 2018

View reviewed changes

patrickhulce added 2 commits April 25, 2018 11:24

feedback

f5d3b65

Merge branch 'master' into combine_render_blocking

6d53604

helper functions ftw

2e88795

paulirish approved these changes Apr 25, 2018

View reviewed changes

patrickhulce merged commit b1ecfac into master Apr 25, 2018

patrickhulce deleted the combine_render_blocking branch April 25, 2018 19:44

brendankenny reviewed Apr 25, 2018

View reviewed changes

patrickhulce mentioned this pull request Apr 25, 2018

core(render-blocking): address followup feedback #5039

Merged

paulirish removed the waiting4reviewer label Apr 30, 2018

kdzwinel pushed a commit to kdzwinel/lighthouse that referenced this pull request Aug 16, 2018

core: merge render blocking audits to lantern (GoogleChrome#4995)

cc58e8f


		const {node, nodeTiming} = nodesByUrl[resource.tag.url];

		// Mark this node and all it's dependents as deferrable

core: merge render blocking audits to lantern #4995

core: merge render blocking audits to lantern #4995

Conversation

patrickhulce commented Apr 19, 2018 • edited Loading

paulirish commented Apr 20, 2018

paulirish commented Apr 20, 2018

patrickhulce commented Apr 20, 2018

patrickhulce commented Apr 23, 2018

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 Apr 24, 2018 • 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

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

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

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 commented Apr 25, 2018

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

Choose a reason for hiding this comment

patrickhulce commented Apr 19, 2018 •

edited

Loading

paulirish Apr 24, 2018 •

edited

Loading