core: add cap to amp stylesheet links for simulated throttling #11069

adamraine · 2020-07-07T22:40:54Z

Summary
Since AMP defers loading external stylesheets if they take more than 1s we need to cap the duration in render blocking resources at 1s. The cap has already been applied for active throttling and this PR will do the same for simulated throttling.

@patrickhulce we currently just cap any stylesheet when AMP is loaded. Let me know if that is too broad.

Related Issues/PRs
#2832

connorjclark · 2020-07-07T23:10:51Z

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

@@ -54,6 +55,12 @@ function getNodesAndTimingByUrl(nodeTimings) {
    const nodeTiming = nodeTimings.get(node);
    if (!nodeTiming) return;

+    if (usesAMP && node.record.resourceType === 'Stylesheet'


We'll definitely need a beefy comment here on why this is fair and not totally cheating/playing nice for AMP.

yes, a big beefy comment with a link to our invitation for any stack owner with similar logic to do this would be awesome 👍

it'd also be great to link to the amp source (it should be in that issue) so that later we can double check that the behavior still exists.

patrickhulce

Thanks @adamraine this is a great start!

To @connorjclark 's point, it would also be nice if the override logic were extracted to computeStackSpecificTiming functions where the AMP-specific logic (and potential future others) could live rather than a boolean parameter intermingled in the core code. For one it makes it clear where future stacks would be added, and for another it marks the stack-specific logic as clear extra optional niceness instead of a core part of the logic of this audit.

To that end, I don't think we're going to want the nodeTiming mutation done inside getNodesAndTimingByUrl if we're going to be capping the overall savings as well.

There are two parts to cap.

wastedMs per stylesheet
overallWastedMs

Mutating the object that's supposed to reflect the true timings makes this need slightly less clear and doesn't fully solve the second part of the problem. Assuming we extract two computeStackSpecificTiming functions for the two problems, we can the override just the "wasted" variables instead of the simulated request data. How does that sound?

Unit tests for this would also be great :)

patrickhulce · 2020-07-07T23:51:16Z

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

@@ -54,6 +55,12 @@ function getNodesAndTimingByUrl(nodeTimings) {
    const nodeTiming = nodeTimings.get(node);
    if (!nodeTiming) return;

+    if (usesAMP && node.record.resourceType === 'Stylesheet'


yes, a big beefy comment with a link to our invitation for any stack owner with similar logic to do this would be awesome 👍

it'd also be great to link to the amp source (it should be in that issue) so that later we can double check that the behavior still exists.

patrickhulce · 2020-07-07T23:58:24Z

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

@@ -95,7 +103,7 @@ class RenderBlockingResources extends Audit {
    const fcpSimulation = await FirstContentfulPaint.request(metricComputationData, context);
    const fcpTsInMs = traceOfTab.timestamps.firstContentfulPaint / 1000;

-    const nodesByUrl = getNodesAndTimingByUrl(fcpSimulation.optimisticEstimate.nodeTimings);
+    const nodesByUrl = getNodesAndTimingByUrl(fcpSimulation.optimisticEstimate.nodeTimings, usesAMP);


I don't think this is the layer at which we're going to want to cap the timing, see more in my overall review comment

connorjclark · 2020-07-08T19:09:22Z

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

 * @param {Node} node
 * @param {LH.Gatherer.Simulation.NodeTiming} nodeTiming
 * @param {LH.Artifacts.DetectedStack[]} Stacks
 */
 function computeStackSpecificTiming(node, nodeTiming, Stacks) {
-  let stackSpecificTiming = Object.assign({}, nodeTiming);
-  if(!Stacks) return stackSpecificTiming;
+  const stackSpecificTiming = Object.assign({}, nodeTiming);


nit: prefer object spreading.

Suggested change

const stackSpecificTiming = Object.assign({}, nodeTiming);

const stackSpecificTiming = {...nodeTiming};

patrickhulce

nice @adamraine we're getting there 👍

friendly reminder that adding some early unit tests will be your friend here and help catch potential bugs! :)

patrickhulce · 2020-07-08T23:59:14Z

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

-    const originalEstimate = simulator.simulate(fcpGraph).timeInMs;
+  static estimateSavingsWithGraphs(simulator, fcpGraph, deferredIds, wastedCssBytesByUrl, Stacks) {
+    const originalEstimate = simulator.simulate(fcpGraph, {label: 'rbr-estimate'}).timeInMs;
+    const originalNodeTimings = Simulator.ALL_NODE_TIMINGS.get('rbr-estimate');


very clever use of the API surface! :)

I don't think we've ever used the label to do anything more than output debug information before though, and I'm not sure it's worth doing it this time around when simulate already returns the nodeTimings

patrickhulce · 2020-07-09T00:18:36Z

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

+        const nodeTiming = originalNodeTimings.get(node);
+        if (nodeTiming) {
+          const stackSpecificTiming = computeStackSpecificTiming(node, nodeTiming, Stacks);
+          stackSpecificEstimate -= nodeTiming.duration - stackSpecificTiming.duration;


I'm trying to wrap my head around how this works, so let me see if I follow what you're trying to do here.

we lookup the node timing in the "before" version of the graph

compute its stack-specific timing

take the difference from the non-stack-specific timing and subtract it from the overall "before" estimate

this lowers the effective "before" estimate to better reflect the fact that we might be overestimating it

right?

This is on the right track but I think it will run into issues with parallel requests.

Consider the page dependency graph

|-index.html 1000ms |- style-1.css 2000ms |- style-2.css 2000ms |- style-3.css 2000ms

In this case we'll subtract 3 full seconds off the before time which was only 3s to begin with. A fully correct solution in this style would require threading the duration changes all the way down through core lantern, but that's a bit more involved than I think we need here :)

Instead, you could adjust all of the nodeTimings coming from simulate and recompute const originalEstimate as the Math.max(...Array.from(adjustedNodeTimings.map(timing => timing.endTime))), you will also have to adjust the endTime of any dependent requests of the stylesheet (see line 137 for an example of how to find dependent nodes)

patrickhulce · 2020-07-09T00:22:36Z

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

+  const stackSpecificTiming = Object.assign({}, nodeTiming);
+  if (!Stacks) return stackSpecificTiming;
+  if (Stacks.some(stack => stack.id === 'amp')) {
+    // AMP will load a linked stylesheet asynchronously if it has not been loaded after 1 second [https://github.com/ampproject/amphtml/blob/master/src/font-stylesheet-timeout.js]


protip: press y while on a github blob to turn the shortcut ref into a permalink of the current hash, this way if amp ever renames its master branch to main or moves the file future readers will still be able to find what you were talking about :)

Suggested change

// AMP will load a linked stylesheet asynchronously if it has not been loaded after 1 second [https://github.com/ampproject/amphtml/blob/master/src/font-stylesheet-timeout.js]

// AMP will load a linked stylesheet asynchronously if it has not been loaded after 1 second

// https://github.com/ampproject/amphtml/blob/8e03ac2f315774070651584a7e046ff24212c9b1/src/font-stylesheet-timeout.js#L54-L59

patrickhulce · 2020-07-09T00:24:06Z

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

+  const stackSpecificTiming = Object.assign({}, nodeTiming);
+  if (!Stacks) return stackSpecificTiming;
+  if (Stacks.some(stack => stack.id === 'amp')) {
+    // AMP will load a linked stylesheet asynchronously if it has not been loaded after 1 second [https://github.com/ampproject/amphtml/blob/master/src/font-stylesheet-timeout.js]


based on their latest master it actually looks like they cap the stylesheet end load time to 2.1s now instead of the stylesheet load time to 1s which is pretty neat, we'll need to adjust our logic accordingly

patrickhulce · 2020-07-09T00:24:44Z

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

+  if (Stacks.some(stack => stack.id === 'amp')) {
+    // AMP will load a linked stylesheet asynchronously if it has not been loaded after 1 second [https://github.com/ampproject/amphtml/blob/master/src/font-stylesheet-timeout.js]
+    // Any potential savings for AMP stylesheet nodes must therefore be capped at 1 second.
+    // https://github.com/GoogleChrome/lighthouse/issues/2832#issuecomment-591066081


let's move this comment line to the jsdoc description of the function explaining this is where any stack-specific overrides should happen

patrickhulce

spoke in our video call, but going back to the original issue I think we have to do a little more around dependent requests.

I added an example trace from an AMP page we should try to test against :)

patrickhulce · 2020-07-10T18:54:01Z

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

@@ -60,6 +60,56 @@ function getNodesAndTimingByUrl(nodeTimings) {
  return urlMap;
 }

+/**
+ * Adjust the timing of a node and it's dependencies to account for stack specific overrides.


Suggested change

* Adjust the timing of a node and it's dependencies to account for stack specific overrides.

* Adjust the timing of a node and its dependencies to account for stack specific overrides.

sorry if that was been copied from one of my comments 😆

patrickhulce · 2020-07-10T18:54:50Z

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

+
+/**
+ * Any stack specific timing overrides should go in this function.
+ * https://github.com/GoogleChrome/lighthouse/issues/2832#issuecomment-591066081


Suggested change

* https://github.com/GoogleChrome/lighthouse/issues/2832#issuecomment-591066081

* @see https://github.com/GoogleChrome/lighthouse/issues/2832#issuecomment-591066081

patrickhulce · 2020-07-10T18:57:23Z

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

@@ -170,13 +228,18 @@ class RenderBlockingResources extends Audit {
      return !canDeferRequest;
    }));

+    // Recalculate the "before" time based on our adjusted node timings.
+    const stackSpecificEstimate = Math.max(...Array.from(


Keeping with the global name change WDYT about...

Suggested change

const stackSpecificEstimate = Math.max(...Array.from(

const estimateBeforeInline = Math.max(...Array.from(

patrickhulce · 2020-07-10T19:01:29Z

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

      // Saving 1000 + 1000 + 100ms for TCP handshake + 1 RT savings + server response time
      assert.equal(result, 2100);
    });
+
+    it('computes savings for stylesheets deferred by AMP', () => {


Suggested change

it('computes savings for stylesheets deferred by AMP', () => {

it('does not report savings from AMP-stack when document already exceeds 2.1s', () => {

googlebot · 2020-07-13T15:25:05Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

patrickhulce

LGTM 🎉

It's worth continued noting in the issue that this audit will still produce results because AMP's method does still use render-blocking requests. It just limits the impact they have to ~2.1s. Since most the comments in that issue are complaining about results that are less than this threshold, I wouldn't expect many of them to be satisfied but at least we know our results are technically sound now :)

Before	After

patrickhulce · 2020-07-13T18:55:47Z

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

+  const difference = nodeTiming.duration - stackSpecificTiming.duration;
+  if (!difference) return;
+
+  // Adjust timing of all dependent nodes


Suggested change

// Adjust timing of all dependent nodes

// AMP's method of removal of stylesheets effectively removes all dependent nodes from the FCP graph

// Ensure all dependent nodes are not affecting our estimated savings either.

just to make it clear this is still AMP specific if we ever add more

patrickhulce · 2020-07-13T19:02:16Z

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

+  node.traverse(node => {
+    adjustedNodeTimings.delete(node);


nit: shadowing can be confusing to those who are trying to understand it for the first time :)

Suggested change

node.traverse(node => {

adjustedNodeTimings.delete(node);

node.traverse(childNode => {

adjustedNodeTimings.delete(childNode);

adamraine · 2020-07-13T19:57:11Z

@googlebot I consent.

patrickhulce · 2020-07-13T20:37:56Z

@googlebot I consent.

googlebot · 2020-07-13T20:38:41Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

add basic cap to amp stylesheet

e96bf0c

adamraine requested a review from patrickhulce July 7, 2020 22:40

adamraine requested a review from a team as a code owner July 7, 2020 22:40

devtools-bot assigned patrickhulce Jul 7, 2020

devtools-bot added the waiting4reviewer label Jul 7, 2020

connorjclark reviewed Jul 7, 2020

View reviewed changes

patrickhulce suggested changes Jul 8, 2020

View reviewed changes

adamraine added 2 commits July 8, 2020 15:56

Switch to computeStackSpecificTiming

6bbaf19

Use new function for simulated time estimate

5503dab

vercel bot deployed to Preview July 8, 2020 18:14 View deployment

googlebot added the cla: yes label Jul 8, 2020

patrickhulce added waiting4committer and removed waiting4reviewer labels Jul 8, 2020

adamraine added 2 commits July 8, 2020 18:53

Added comment

3e0a3a6

lint

c64699c

vercel bot deployed to Preview July 8, 2020 19:01 View deployment

adamraine added waiting4reviewer and removed waiting4committer labels Jul 8, 2020

connorjclark reviewed Jul 8, 2020

View reviewed changes

patrickhulce suggested changes Jul 9, 2020

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Jul 9, 2020

adamraine added 8 commits July 9, 2020 14:36

Use node timings returned directly

839dac2

Add basic test case

5ef44f2

Switch cap from 1s to 2.1s

f32c400

Nits

746b5a5

Switch to parrallel friendly override

2ba9e44

Add comments

b21242d

Add parrallel stylesheet to test case

d6122ac

Lint

1d28744

adamraine added the waiting4reviewer label Jul 10, 2020

add real amp example

1272174

vercel bot deployed to Preview July 10, 2020 19:48 View deployment

patrickhulce reviewed Jul 10, 2020

View reviewed changes

Start to test case

c5fe981

vercel bot deployed to Preview July 13, 2020 15:24 View deployment

googlebot added cla: no and removed cla: yes labels Jul 13, 2020

patrickhulce added waiting4committer and removed waiting4reviewer labels Jul 13, 2020

adamraine added 3 commits July 13, 2020 13:30

Add test case details.items

6de0773

nits

cdbe797

lint

510992d

vercel bot deployed to Preview July 13, 2020 17:41 View deployment

adamraine added waiting4reviewer and removed waiting4committer labels Jul 13, 2020

patrickhulce approved these changes Jul 13, 2020

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Jul 13, 2020

nits

bd5a7f8

vercel bot deployed to Preview July 13, 2020 19:54 View deployment

googlebot added cla: yes and removed cla: no labels Jul 13, 2020

adamraine merged commit 259e222 into master Jul 13, 2020

adamraine deleted the amp-style-cap branch July 13, 2020 20:48

patrickhulce mentioned this pull request Jul 20, 2020

Font style sheets in AMP misclassified as render blocking. #2832

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core: add cap to amp stylesheet links for simulated throttling #11069

core: add cap to amp stylesheet links for simulated throttling #11069

adamraine commented Jul 7, 2020

connorjclark Jul 7, 2020

patrickhulce Jul 7, 2020

patrickhulce left a comment

patrickhulce Jul 7, 2020

patrickhulce Jul 7, 2020

connorjclark Jul 8, 2020

patrickhulce left a comment

patrickhulce Jul 8, 2020

patrickhulce Jul 9, 2020

patrickhulce Jul 9, 2020

patrickhulce Jul 9, 2020

patrickhulce Jul 9, 2020

patrickhulce left a comment

patrickhulce Jul 10, 2020

patrickhulce Jul 10, 2020

patrickhulce Jul 10, 2020

patrickhulce Jul 10, 2020

googlebot commented Jul 13, 2020

patrickhulce left a comment

patrickhulce Jul 13, 2020

patrickhulce Jul 13, 2020

patrickhulce Jul 13, 2020

adamraine commented Jul 13, 2020

patrickhulce commented Jul 13, 2020

googlebot commented Jul 13, 2020

	const stackSpecificTiming = Object.assign({}, nodeTiming);
	const stackSpecificTiming = {...nodeTiming};

	// AMP will load a linked stylesheet asynchronously if it has not been loaded after 1 second [https://github.com/ampproject/amphtml/blob/master/src/font-stylesheet-timeout.js]
	// AMP will load a linked stylesheet asynchronously if it has not been loaded after 1 second
	// https://github.com/ampproject/amphtml/blob/8e03ac2f315774070651584a7e046ff24212c9b1/src/font-stylesheet-timeout.js#L54-L59

	* Adjust the timing of a node and it's dependencies to account for stack specific overrides.
	* Adjust the timing of a node and its dependencies to account for stack specific overrides.

	* https://github.com/GoogleChrome/lighthouse/issues/2832#issuecomment-591066081
	* @see https://github.com/GoogleChrome/lighthouse/issues/2832#issuecomment-591066081

	const stackSpecificEstimate = Math.max(...Array.from(
	const estimateBeforeInline = Math.max(...Array.from(

	it('computes savings for stylesheets deferred by AMP', () => {
	it('does not report savings from AMP-stack when document already exceeds 2.1s', () => {

	// Adjust timing of all dependent nodes
	// AMP's method of removal of stylesheets effectively removes all dependent nodes from the FCP graph
	// Ensure all dependent nodes are not affecting our estimated savings either.

core: add cap to amp stylesheet links for simulated throttling #11069

core: add cap to amp stylesheet links for simulated throttling #11069

Conversation

adamraine commented Jul 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce 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

patrickhulce 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 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

googlebot commented Jul 13, 2020

patrickhulce 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

adamraine commented Jul 13, 2020

patrickhulce commented Jul 13, 2020

googlebot commented Jul 13, 2020