Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement a layout-blocking property on networkrequests in Blink #2065

Open
ebidel opened this issue Apr 25, 2017 · 30 comments
Open

Implement a layout-blocking property on networkrequests in Blink #2065

ebidel opened this issue Apr 25, 2017 · 30 comments
Assignees

Comments

@ebidel
Copy link
Contributor

ebidel commented Apr 25, 2017

https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js#L58 only checks for script in <head>.

We should expand that to cover blocking scripts within <body> (example). False positives will likely be trickier to deal with. Strawman below.

For all scripts in <body>:

  1. Determine if the script is before the end of the page (document.body.lastElementChild).
  2. If it's not at the end, check if there's rendered DOM after it. Filter out script, template, and anything else in the default stylesheet that's hidden by default.
  3. For all the sibling nodes after the script, check node.offsetParent === null to determine if it is being rendered. Note: cannot use getComputedStyle(node).display === 'none won't work as expected.

We'll need to also update the reference do, which talks about scripts in the head:
https://developers.google.com/web/tools/lighthouse/audits/blocking-resources

cc @igrigorik

@brendankenny
Copy link
Member

weren't we switching to some (future) debugger-protocol-based detection for this? Or was that something else

@ebidel
Copy link
Contributor Author

ebidel commented Apr 25, 2017

IIRC, @pavelfeldman was going to do something with blocking resources in the protocol.

@paulirish paulirish changed the title Render blocking JS: consider scripts in <body> which can also block FP Implement a layout-blocking property on networkrequests in Blink Apr 25, 2019
@paulirish
Copy link
Member

Exposing a render-blocking bit in the protocol would solve a lot of problems.

@connorjclark
Copy link
Collaborator

connorjclark commented Apr 26, 2019

TraceParserBlockingScript - https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/script/html_parser_script_runner.cc?type=cs&sq=package:chromium&g=0&l=89

These trace events tell us exactly what blocks the rendering, and for how long.

I'm not seeing an equivalent trace for render blocking stylesheets.

@paulirish
Copy link
Member

i see these two pieces as well:

kAttributeLayoutBlocking in net stack.

Document::HaveRenderBlockingResourcesLoaded() in blink

pat meenan should know more as he used to work on this.

@connorjclark
Copy link
Collaborator

connorjclark commented Apr 26, 2019

kAttributeLayoutBlocking in net stack.

The kAttributeLayoutBlocking bit doesn't seem to ever be set. There's one place: https://cs.chromium.org/chromium/src/services/network/resource_scheduler.cc?l=617&rcl=150163ac7f9ace1ab8a9400bccf6029c32826703 but it looks like it's just copying from another attributes?

Document::HaveRenderBlockingResourcesLoaded() in blink

pat meenan should know more as he used to work on this.

not seeing a way to get the blocking stylesheet from the style engine.

Looks like the html parser should be doing the same with stylesheets, but doesn't yet...according to this TODO: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/script/html_parser_script_runner.cc?l=435-440&rcl=cd815ad39e59ea3a66e67206f0105b1dd298da5f


We can easily get blocking information from the trace for scripts. For stylesheets, looks like some Chromium changes would be needed. But, AFAIU, it's pretty simple to determine what stylesheets block paint - any in the head, never in the body, and the duration it blocked for is how long the request took. Is that right?

@connorjclark
Copy link
Collaborator

Anywho, how would this information be used? would the only change be replacing all of ./lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js w/ reading of the trace?

@patrickhulce
Copy link
Collaborator

We can easily get blocking information from the trace for scripts.

Just checking if we're on the same page. How do we easily determine this? :)

@connorjclark
Copy link
Collaborator

My hunch is that https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/script/html_parser_script_runner.cc?type=cs&sq=package:chromium&g=0&l=111-126 will do nicely. I haven't tried yet :)

@connorjclark
Copy link
Collaborator

connorjclark commented Jun 19, 2019

Comparing the YieldParserForScript* events w/ what tags-blocking-first-paint.js currently outputs. Ignore CSS, since this event doesn't handle that. The one YieldParserForScriptLoadAndBlockingResources event lines up with what we determine to be render blocking (bundle-9222fb3-81f9bc2.js - see trace below). I suppose we can ignore the rest (YieldParserForScriptLoad is not blocking in the way we care about).

However, the timestamp on the event suggests it blocked for a few microseconds, whereas we currently suggest it to be 40ms? Is that right?

diff --git a/lighthouse-core/gather/driver.js b/lighthouse-core/gather/driver.js
index b9d58f6d..5bd8bb09 100644
--- a/lighthouse-core/gather/driver.js
+++ b/lighthouse-core/gather/driver.js
@@ -120,6 +120,9 @@ class Driver {
       // accidentally excluding microtasks, we don't want to assume a parent event will always exist
       'v8.execute',

+      // For render blocking events.
+      "blink",
+
       // For extracting UserTiming marks/measures
       'blink.user_timing',

diff --git a/lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js b/lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js
index 3639482c..ef8658d3 100644
--- a/lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js
+++ b/lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js
@@ -183,8 +183,15 @@ class TagsBlockingFirstPaint extends Gatherer {
    * @param {LH.Gatherer.LoadData} loadData
    * @return {Promise<LH.Artifacts['TagsBlockingFirstPaint']>}
    */
-  afterPass(passContext, loadData) {
-    return TagsBlockingFirstPaint.findBlockingTags(passContext.driver, loadData.networkRecords);
+  async afterPass(passContext, loadData) {
+    console.log('trace events',
+      JSON.stringify(
+        loadData.trace.traceEvents.filter(e => e.cat === 'blink' && e.name.includes('YieldParserForScript')), null, 2
+      )
+    );
+    const result = await TagsBlockingFirstPaint.findBlockingTags(passContext.driver, loadData.networkRecords);
+    console.log('TagsBlockingFirstPaint', result);
+    return result;
   }
 }
trace
trace events [
  {
    "pid": 69856,
    "tid": 775,
    "ts": 965930628658,
    "ph": "X",
    "cat": "blink",
    "name": "YieldParserForScriptLoadAndBlockingResources",
    "bind_id": "0xb0745d742a9c7617",
    "dur": 3,
    "tdur": 3,
    "id": "0x87d0f7bf",
    "flow_out": true,
    "tts": 643605,
    "args": {
      "data": {
        "url": "https://www.coursehero.com/sym-assets/js/bundle-9222fb3-81f9bc2.js",
        "frame": "0x37c1c42230"
      }
    }
  },
  {
    "pid": 69856,
    "tid": 775,
    "ts": 965930940993,
    "ph": "X",
    "cat": "blink",
    "name": "YieldParserForScriptLoad",
    "bind_id": "0xb0745d742a86830f",
    "dur": 3,
    "tdur": 3,
    "id": "0x87d0f7bf",
    "flow_out": true,
    "tts": 867661,
    "args": {
      "data": {
        "url": "https://cdn.polyfill.io/v3/polyfill.min.js?flags=gated&features=default%2CArray.prototype.includes%2CArray.prototype.find%2Cfetch%2CObject.entries%2CObject.values%2CObject.entries",
        "frame": "0x37c1c42230"
      }
    }
  },
  {
    "pid": 69856,
    "tid": 775,
    "ts": 965930998575,
    "ph": "X",
    "cat": "blink",
    "name": "YieldParserForScriptLoad",
    "bind_id": "0xb0745d742a858657",
    "dur": 3,
    "tdur": 3,
    "id": "0x87d0f7bf",
    "flow_out": true,
    "tts": 918301,
    "args": {
      "data": {
        "url": "https://www.coursehero.com/sym-assets/js/bundle-73feeba-52d7ed3.js",
        "frame": "0x37c1c42230"
      }
    }
  },
  {
    "pid": 69856,
    "tid": 775,
    "ts": 965931156043,
    "ph": "X",
    "cat": "blink",
    "name": "YieldParserForScriptLoad",
    "bind_id": "0xb0745d742a852587",
    "dur": 3,
    "tdur": 4,
    "id": "0x87d0f7bf",
    "flow_out": true,
    "tts": 1054256,
    "args": {
      "data": {
        "url": "https://www.coursehero.com/sym-assets/js/homepage-app-a3962be-473015a.js",
        "frame": "0x37c1c42230"
      }
    }
  },
  {
    "pid": 69856,
    "tid": 775,
    "ts": 965931184642,
    "ph": "X",
    "cat": "blink",
    "name": "YieldParserForScriptLoad",
    "bind_id": "0xb0745d742a851ee7",
    "dur": 3,
    "tdur": 2,
    "id": "0x87d0f7bf",
    "flow_out": true,
    "tts": 1082153,
    "args": {
      "data": {
        "url": "https://unpkg.com/scrollreveal/dist/scrollreveal.min.js",
        "frame": "0x37c1c42230"
      }
    }
  }
]
TagsBlockingFirstPaint [ { tag:
     { tagName: 'LINK',
       url: 'https://www.coursehero.com/assets/fonts/HaasGrotesk.css',
       href: 'https://www.coursehero.com/assets/fonts/HaasGrotesk.css',
       rel: 'stylesheet',
       media: '',
       disabled: false,
       mediaChanges: [] },
    transferSize: 576,
    startTime: 965930.613553,
    endTime: 965930.643827 },
  { tag:
     { tagName: 'LINK',
       url:
        'https://www.coursehero.com/sym-assets/css/8c1573b-b7e91e9.css',
       href:
        'https://www.coursehero.com/sym-assets/css/8c1573b-b7e91e9.css',
       rel: 'stylesheet',
       media: '',
       disabled: false,
       mediaChanges: [] },
    transferSize: 17838,
    startTime: 965930.613919,
    endTime: 965930.648468 },
  { tag:
     { tagName: 'LINK',
       url:
        'https://www.coursehero.com/sym-assets/css/9f8ded5-8c476ff.css',
       href:
        'https://www.coursehero.com/sym-assets/css/9f8ded5-8c476ff.css',
       rel: 'stylesheet',
       media: '',
       disabled: false,
       mediaChanges: [] },
    transferSize: 10416,
    startTime: 965930.614239,
    endTime: 965930.647011 },
  { tag:
     { tagName: 'LINK',
       url:
        'https://www.coursehero.com/sym-assets/css/dd9725c-1f96b36.css',
       href:
        'https://www.coursehero.com/sym-assets/css/dd9725c-1f96b36.css',
       rel: 'stylesheet',
       media: '',
       disabled: false,
       mediaChanges: [] },
    transferSize: 2164,
    startTime: 965930.614607,
    endTime: 965930.644407 },
  { tag:
     { tagName: 'SCRIPT',
       url:
        'https://www.coursehero.com/sym-assets/js/bundle-9222fb3-81f9bc2.js',
       src:
        'https://www.coursehero.com/sym-assets/js/bundle-9222fb3-81f9bc2.js',
       mediaChanges: [] },
    transferSize: 187747,
    startTime: 965930.614998,
    endTime: 965930.659793 } ]

@patrickhulce
Copy link
Collaborator

patrickhulce commented Jun 19, 2019

I'm assuming the time it's reporting (in dur) is fundamentally different. i.e. it was probably discovered and requested much earlier by the lookahead parser and so by the time we actually get around to parsing it's basically already available.

(No clue if this is true or not, just my hunch :)

@paulirish
Copy link
Member

Yup. It's a flow event so there's a matching event at the end of the async stretch:
image

I tried this out with fast3g/4xcpu throttling... and the duration of this flow event generally matches with what I see as the Network panel download time (though that's not really accurate either).. 2543ms vs 2560ms.

@connorjclark
Copy link
Collaborator

Thanks @paulirish.

So a YieldParserForScriptLoadAndBlockingResources via HTMLParserScriptRunner::ExecuteScriptsWaitingForParsing event will emit when the HTML parser must pause before executing a script - there are two cases for this: 1) the script itself isn't done loading and 2) some stylesheets before the script are not done loading. This seems to cover cases where 1) the script should be made async/defer and 2) the stylesheets should be preloaded.

The relevant flow trace event is: YieldParserForScriptLoadAndBlockingResources -> HTMLParserScriptRunner ExecuteScript.

This comment suggests that the html parser doesn't have the same blocking mechanism for stylesheets. But, I don't think it's needed, AFAIK a stylesheet only blocks if there is a script to block for, and this case is handled in HTMLParserScriptRunner::ExecuteScriptsWaitingForParsing.

I suppose what's left is to determine what stylesheets (the resource the script is waiting on) is doing the blocking.

@connorjclark
Copy link
Collaborator

Running against sfgate.com shows the big difference here: we'd be reporting on what the browser actually blocked on, not what it could possibly block on. Note just the one script from inspecting the traces, but many from the current artifact code.

patch
diff --git a/lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js b/lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js
index 3639482c..19dbf89b 100644
--- a/lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js
+++ b/lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js
@@ -183,8 +183,42 @@ class TagsBlockingFirstPaint extends Gatherer {
    * @param {LH.Gatherer.LoadData} loadData
    * @return {Promise<LH.Artifacts['TagsBlockingFirstPaint']>}
    */
-  afterPass(passContext, loadData) {
-    return TagsBlockingFirstPaint.findBlockingTags(passContext.driver, loadData.networkRecords);
+  async afterPass(passContext, loadData) {
+    const flowEvents = loadData.trace.traceEvents.filter(e => {
+      if (e.cat !== 'blink') return false;
+      if (!(e.flow_in || e.flow_out)) return false;
+
+      const relevantName = [
+        'YieldParserForScriptLoadAndBlockingResources',
+        'HTMLParserScriptRunner ExecuteScript',
+      ].includes(e.name);
+      return relevantName;
+    });
+
+    const flowEventByBindId = new Map();
+    for (const event of flowEvents) {
+      if (!flowEventByBindId.has(event.bind_id)) {
+        flowEventByBindId.set(event.bind_id, []);
+      }
+      flowEventByBindId.get(event.bind_id).push(event);
+    }
+    const pairedFlowEvents = [...flowEventByBindId.values()].map(events => {
+      return {
+        start: events[0],
+        end: events[1],
+      };
+    }).filter(pair => pair.start && pair.end);
+    for (const pairedFlowEvent of pairedFlowEvents) {
+      console.log(
+        JSON.stringify(pairedFlowEvent, null, 2),
+        (pairedFlowEvent.end.ts - pairedFlowEvent.start.ts) / 1000,
+        'milliseconds'
+      );
+    }
+
+    const result = await TagsBlockingFirstPaint.findBlockingTags(passContext.driver, loadData.networkRecords);
+    console.log('TagsBlockingFirstPaint', result.filter(data => data.tag.tagName !== 'LINK'));
+    return result;
   }
 }
output (sfgate.com)
{
  "start": {
    "pid": 85002,
    "tid": 775,
    "ts": 973787108087,
    "ph": "X",
    "cat": "blink",
    "name": "YieldParserForScriptLoadAndBlockingResources",
    "bind_id": "0xb0afc75e4168f855",
    "dur": 3,
    "tdur": 2,
    "id": "0x8835ecdd",
    "flow_out": true,
    "tts": 744071,
    "args": {
      "data": {
        "url": "https://m.sfgate.com/external/js/global.header.v9.46.2.js",
        "frame": "0x3ba7b62230"
      }
    }
  },
  "end": {
    "pid": 85002,
    "tid": 775,
    "ts": 973787126641,
    "ph": "X",
    "cat": "blink",
    "name": "HTMLParserScriptRunner ExecuteScript",
    "bind_id": "0xb0afc75e4168f855",
    "dur": 18056,
    "tdur": 17391,
    "id": "0x8835ecdd",
    "flow_in": true,
    "tts": 762557,
    "args": {
      "data": {
        "url": "https://m.sfgate.com/external/js/global.header.v9.46.2.js",
        "frame": "0x3ba7b62230"
      }
    }
  }
} 18.554 milliseconds
TagsBlockingFirstPaint [ { tag:
     { tagName: 'SCRIPT',
       url: 'https://m.sfgate.com/js/hdn/utils/polyfill.min.js',
       src: 'https://m.sfgate.com/js/hdn/utils/polyfill.min.js',
       mediaChanges: [] },
    transferSize: 34956,
    startTime: 973786.966896,
    endTime: 973786.982421 },
  { tag:
     { tagName: 'SCRIPT',
       url:
        'https://m.sfgate.com/js/hdn/utils/modernizr-2.6.2.min.js?v9.46.2',
       src:
        'https://m.sfgate.com/js/hdn/utils/modernizr-2.6.2.min.js?v9.46.2',
       mediaChanges: [] },
    transferSize: 6438,
    startTime: 973786.967206,
    endTime: 973786.97681 },
  { tag:
     { tagName: 'SCRIPT',
       url: 'https://m.sfgate.com/external/js/global.header.v9.46.2.js',
       src: 'https://m.sfgate.com/external/js/global.header.v9.46.2.js',
       mediaChanges: [] },
    transferSize: 63701,
    startTime: 973786.968687,
    endTime: 973787.005913 },
  { tag:
     { tagName: 'SCRIPT',
       url: 'https://treg.hearstnp.com/treg.js',
       src: 'https://treg.hearstnp.com/treg.js',
       mediaChanges: [] },
    transferSize: 4707,
    startTime: 973786.96901,
    endTime: 973787.051682 },
  { tag:
     { tagName: 'SCRIPT',
       url: 'https://aps.hearstnp.com/Scripts/loadAds.js',
       src: 'https://aps.hearstnp.com/Scripts/loadAds.js',
       mediaChanges: [] },
    transferSize: 2271,
    startTime: 973786.969418,
    endTime: 973787.052713 },
  { tag:
     { tagName: 'SCRIPT',
       url: 'https://aps.hearstnp.com/SRO/GetJS?url=m.sfgate.com',
       src: 'https://aps.hearstnp.com/SRO/GetJS?url=m.sfgate.com',
       mediaChanges: [] },
    transferSize: 129742,
    startTime: 973787.169064,
    endTime: 973787.191761 },
  { tag:
     { tagName: 'SCRIPT',
       url: 'https://static.chartbeat.com/js/chartbeat_mab.js',
       src: 'https://static.chartbeat.com/js/chartbeat_mab.js',
       mediaChanges: [] },
    transferSize: 8251,
    startTime: 973786.969643,
    endTime: 973787.157903 },
  { tag:
     { tagName: 'SCRIPT',
       url:
        'https://mb.moatads.com/yi.js?qn=(%2BIb%7Cj8o%3FJei%2F*%5EJqD(aD%5Bqir1fcSC%3AU%3FWOvTh%7CzFK%3F%5B%22l!j%3F%5DVC8p%3D%2Fi%24%2Bc%3DN%2CNl%3F%3Ba%7B*%3Ce%23VX%25%5Em%5EQv%22%605%5Eh%22%25%3ACXof5cxPcs86Jo8bYLaXBjA%3AmQ)%3CF!tAbjrzJ%3BgoVYGVxc%40lQQV%23tc3%2Fh%7C%3FVKV%3BW5.NO)WxX*C%24%3D!L2ixXimPdK%3Eo%26)FK%3A%3AatASYUby%3D(tN%23V.x9.rV0%2F%60Ew_G)%3C1)uRZeLnt6%3D%3Dh_GW3r4cXrU%40%2B(aBUFj8V&qp=00000&is=&iv=6&qt=0&gz=0&hh=0&hn=0&tw=null&qc=0&qd=0&qf=412&qe=660&qh=412&qg=660&qm=420&qa=0&qb=0&qi=0&qj=0&po=1-0020002000002120&qr=0&url=https%3A%2F%2Fm.sfgate.com%2F&confidence=2&pcode=hearstnewsprebidheader515009925453&callback=MoatNadoAllJsonpRequest_48605942',
       src:
        'https://mb.moatads.com/yi.js?qn=(%2BIb%7Cj8o%3FJei%2F*%5EJqD(aD%5Bqir1fcSC%3AU%3FWOvTh%7CzFK%3F%5B%22l!j%3F%5DVC8p%3D%2Fi%24%2Bc%3DN%2CNl%3F%3Ba%7B*%3Ce%23VX%25%5Em%5EQv%22%605%5Eh%22%25%3ACXof5cxPcs86Jo8bYLaXBjA%3AmQ)%3CF!tAbjrzJ%3BgoVYGVxc%40lQQV%23tc3%2Fh%7C%3FVKV%3BW5.NO)WxX*C%24%3D!L2ixXimPdK%3Eo%26)FK%3A%3AatASYUby%3D(tN%23V.x9.rV0%2F%60Ew_G)%3C1)uRZeLnt6%3D%3Dh_GW3r4cXrU%40%2B(aBUFj8V&qp=00000&is=&iv=6&qt=0&gz=0&hh=0&hn=0&tw=null&qc=0&qd=0&qf=412&qe=660&qh=412&qg=660&qm=420&qa=0&qb=0&qi=0&qj=0&po=1-0020002000002120&qr=0&url=https%3A%2F%2Fm.sfgate.com%2F&confidence=2&pcode=hearstnewsprebidheader515009925453&callback=MoatNadoAllJsonpRequest_48605942',
       mediaChanges: [] },
    transferSize: 1384,
    startTime: 973787.661529,
    endTime: 973788.077003 } ]

@patrickhulce
Copy link
Collaborator

Perhaps I'm still completely missing the boat here, but was what I was spitballing anywhere close to reality afterall?

It sounds to me like the events in the trace are what was actually blocking as the load happened, i.e. we're only really getting data on the current observed bottleneck and not the full picture of what's render-blocking that we would need for simulation.

@connorjclark
Copy link
Collaborator

connorjclark commented Jun 20, 2019

It sounds to me like the events in the trace are what was actually blocking as the load happened, i.e. we're only really getting data on the current observed bottleneck and not the full picture of what's render-blocking that we would need for simulation.

yeah that seems right ... fixing whatever script the parser blocks on could just reveal the very next script as being blocking, without ever giving us the full picture.

the parser blocks on the result of Document::IsScriptExecutionReady:

  1. imports

checks for a blocked state on an HTMLImport which is set here.

  1. script blocking stylesheets

Just checks a counter, which is only incremented here.

But perhaps adding hooks to these two things (there's no existing trace for these things like the one in the parser) would suffer from the same problem.

@connorjclark
Copy link
Collaborator

connorjclark commented Jun 21, 2019

Four ways a style sheet blocks the parser via the pending_script_blocking_stylesheets mechanism:

  1. StyleElement::StartLoadingDynamicSheet

This covers stylesheets loaded via @import (just for inline styles). If the resource is cached and doesn't need to be fetched, the pending counter is never incremented.

  1. StyleEngine::CreateSheet

Only called from creation of inline css from style elements. Pending counter is decreased once the inline css is parsed.

  1. ProcessingInstruction::Process

XML stylesheets. Move along now.

  1. LinkStyle::AddPendingSheet

This class handles any <link ref=stylesheet> element. Only considered blocking if these conditions hold true. The pending counter is decreased when the resource is fetched and the stylesheet is parsed.

Increments the counter for @import here.

Not sure where any of these @imports decrements the counter when done.

@paulirish
Copy link
Member

🎉 soon we'll have a trace event for render-blocking styles: https://chromium-review.googlesource.com/c/chromium/src/+/2626665

@yoavweiss
Copy link

The CL still has a bunch of open questions in the form of TODOs. I'd love y'all's opinions on the answers:

  • Do we need to set the render blocking value for redirects? Or can y'all do the chaining on the trace consumer side?
  • There's no way ATM to distinguish imports done from inside blocking styles from those done from inside of partially blocking (in-body) styles. How bad of a problem is this?
  • It's unclear to me if document.written styles behave the same as dynamically injected styles. I'm currently reporting them similarly, but not sure that's correct. Does any know? (If not, I'll create test cases and find out...)
  • I still need to pipe the "partially blocking" status from the preload scanner, to detect in-body styles. Should this be a blocker?

/cc @paullewis @pmeenan

@pmeenan
Copy link

pmeenan commented Jan 21, 2021

Speaking for WebPageTest:

  • Chaining the blocking value is easy enough on the client side. It'd obviously be easier if it flowed through but it's not a big deal.
  • Does the import have any useful initiator information? If so then we could potentially walk it but css historically has been pretty bad at providing initiators.
  • I believe all styles that are added to the DOM before the parser has left the head are all treated the same, at least by the loader logic. Doesn't matter how they were attached to the document.
  • I expect most will initially be fetched by the preload scanner so it feels like it should be important.

@yoavweiss
Copy link

  • Chaining the blocking value is easy enough on the client side. It'd obviously be easier if it flowed through but it's not a big deal.

OK, thanks! I left it out for now, but may tackle it in the future.

  • Does the import have any useful initiator information? If so then we could potentially walk it but css historically has been pretty bad at providing initiators.

I fixed that with https://chromium-review.googlesource.com/c/chromium/src/+/2626665/13..14

  • I believe all styles that are added to the DOM before the parser has left the head are all treated the same, at least by the loader logic. Doesn't matter how they were attached to the document.

Seems like the link_style.cc call site only considers "created by parser" styles as blocking (even if they are still high priority)

  • I expect most will initially be fetched by the preload scanner so it feels like it should be important.

Yup. Added that support in https://chromium-review.googlesource.com/c/chromium/src/+/2626665/14..15

@yoavweiss
Copy link

https://chromium-review.googlesource.com/c/chromium/src/+/2626665 landed! (including redirect support)
Let the tracing begin!! :)

@paulirish
Copy link
Member

chromium-review.googlesource.com/c/chromium/src/+/2626665 landed! (including redirect support)

update 1: there were some followups, too. see https://chromium-review.googlesource.com/q/hashtag:devtools-cwv+(status:open%20OR%20status:merged)

update 2: render-blocking status for scripts is now also landed! 🎉

@connorjclark
Copy link
Collaborator

connorjclark commented Feb 18, 2021

Here's what I have so far. I'll highlight the differences between renderBlocking trace and Lighthouse's manual TagsBlockingFirstPaint on dbw_tester.html.


renderBlocking trace
(+ 1) http://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js
(+ 2) http://localhost:10200/dobetterweb/dbw_disabled.css?delay=200&isdisabled
http://localhost:10200/dobetterweb/dbw_tester.css?delay=100
(+ 3) http://localhost:10200/dobetterweb/dbw_tester.css?delay=2000&async=true
http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200
(+ 4) http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&async=true
http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped
http://localhost:10200/dobetterweb/dbw_tester.js
http://localhost:10200/dobetterweb/fcp-delayer.js?delay=5000
(+ 5) http://localhost:10200/dobetterweb/third_party/aggressive-promise-polyfill.js
http://localhost:10200/dobetterweb/unknown404.css?delay=200

Tags artifact
(- 6) http://localhost:10200/dobetterweb/dbw_partial_a.html?delay=200
http://localhost:10200/dobetterweb/dbw_tester.css?delay=100
http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200
http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped
http://localhost:10200/dobetterweb/dbw_tester.js
http://localhost:10200/dobetterweb/fcp-delayer.js?delay=5000
http://localhost:10200/dobetterweb/unknown404.css?delay=200
  • (+ 1) This script is in the body, so TBFP ignored it. It's still blocking, so the trace says it is in_body_parser_blocking. I think we want to include it here, and rely on the "after fcp?" check we do in the audit.
  • (+ 2) This link element is disabled, which still (for some reason...) issues a network request, but the parser does not block on it. However, the trace marks it as blocking. According to the links in our comments, webkit behaves the same way (and my manual testing confirmed the same for Chrome).
  • (+ 3) <link rel="preload" href="./dbw_tester.css?delay=2000&async=true" as="style" onload="this.rel = 'stylesheet'">. The trace marks this as blocking, but TBFP (correctly?) ignores preloaded links.
  • (+ 4) Similar to (+ 2).
  • (+ 5) Similar to (+ 1).
  • (- 6) <link rel="import" href="./dbw_partial_a.html?delay=200">. This is render blocking, but the trace has no renderBlocking value for this request.

@yoavweiss, items 2, 3, 4 and 6 don't match my expectations here. WDYT?


Every request on the page + renderBlocking from the trace.

http://localhost:10200/dobetterweb/dbw_tester.css?delay=2000&async=true blocking
http://localhost:10200/dobetterweb/dbw_tester.css?delay=100 blocking
http://localhost:10200/dobetterweb/unknown404.css?delay=200 blocking
http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200 blocking
http://localhost:10200/dobetterweb/dbw_disabled.css?delay=200&isdisabled blocking
http://localhost:10200/dobetterweb/dbw_partial_a.html?delay=200 undefined
http://localhost:10200/dobetterweb/dbw_partial_b.html?delay=200&isasync undefined
http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped blocking
http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&async=true blocking
http://localhost:10200/dobetterweb/dbw_tester.js blocking
http://localhost:10200/dobetterweb/empty_module.js?delay=500 non_blocking
http://localhost:10200/dobetterweb/fcp-delayer.js?delay=5000 blocking
http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?iar1 undefined
http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?iar2 undefined
http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr1 undefined
http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr2 undefined
http://localhost:10200/dobetterweb/lighthouse-480x318.jpg?isr3 undefined
http://localhost:10200/dobetterweb/lighthouse-480x318.jpg undefined
http://localhost:10200/dobetterweb/lighthouse-rotating.gif undefined
http://localhost:10200/dobetterweb/third_party/aggressive-promise-polyfill.js in_body_parser_blocking
http://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js in_body_parser_blocking
http://localhost:10200/dobetterweb/empty.css non_blocking
http://localhost:10200/dobetterweb/dbw_tester.css?scriptActivated&delay=200 dynamically_injected_non_blocking
http://localhost:10200/dobetterweb/dbw_tester.html undefined
blob:http://localhost:10200/5db62985-e667-4f67-a590-d79b415699f7 undefined
filesystem:http://localhost:10200/temporary/empty-0.6280479975293367.png undefined
http://localhost:10200/favicon.ico undefined

@paulirish
Copy link
Member

paulirish commented Feb 18, 2021

@yoavweiss, items 2, 3, 4 and 6 don't match my expectations here. WDYT?

my take on 'em:

late in-body script (jquery.min.js aka +1):.. @yoav is the table in Chrome Resource Priorities and Scheduling still accurate? IMO calling this blocking is opinion so i'm glad the trace event gives identifies it as in-body and lets us decide :)

<link disabled> (dbw_disabled aka +2): wow yes this triggers a network request. what? why? spec actually somewhat suggests there shouldn't be a request. (even if the request is necessary, i don't see why it should be blocking)

<link preload css> (dbw_tester.css?delay=2000& aka +3).. the protocol reports the initator as the preload line of html (via parser), so i think its the preload and the requests are not attributed to the attached onload js handler. it doesn't feel like preloaded css requests are blocking to me.

<link rel="import"> (aka -6) trace event didnt catch it but I don't think it was in-scope for yoav. that said, it'd be nice to mark 'em in blink as blocking.

@yoavweiss
Copy link

late in-body script (jquery.min.js aka +1):.. @yoav is the table in Chrome Resource Priorities and Scheduling still accurate? IMO calling this blocking is opinion so i'm glad the trace event gives identifies it as in-body and lets us decide :)

The table should largely still be accurate. Agree this is somewhere where y'all can be more opinionated :)
(the script is blocking stuff below it, which may or may not be rendering critical)

<link disabled> (dbw_disabled aka +2): wow yes this triggers a network request. what? why? spec actually somewhat suggests there shouldn't be a request. (even if the request is necessary, i don't see why it should be blocking)

Agree this should not trigger a fetch at all, and definitely not a high priority/blocking one. As it stands (from casually looking at the code), I think it does, so the reporting seems correct.

/cc @foolip and @mfreed7 who may have opinions on the compat implications of this.

<link preload css> (dbw_tester.css?delay=2000& aka +3).. the protocol reports the initator as the preload line of html (via parser), so i think its the preload and the requests are not attributed to the attached onload js handler. it doesn't feel like preloaded css requests are blocking to me.

Should not be blocking, so this sounds like a reporting bug. I'll look into it.

<link rel="import"> (aka -6) trace event didnt catch it but I don't think it was in-scope for yoav. that said, it'd be nice to mark 'em in blink as blocking.

Haven't we deprecated those?

@yoavweiss
Copy link

Should not be blocking, so this sounds like a reporting bug. I'll look into it.

https://chromium-review.googlesource.com/c/chromium/src/+/2791427

@mfreed7
Copy link

mfreed7 commented Mar 30, 2021

<link disabled> (dbw_disabled aka +2): wow yes this triggers a network request. what? why? spec actually somewhat suggests there shouldn't be a request. (even if the request is necessary, i don't see why it should be blocking)

Agree this should not trigger a fetch at all, and definitely not a high priority/blocking one. As it stands (from casually looking at the code), I think it does, so the reporting seems correct.

/cc @foolip and @mfreed7 who may have opinions on the compat implications of this.

I also agree that <link disabled> should not trigger a fetch. We only recently changed the behavior here, but that seems to have settled out without compat issues. Perhaps we could try making this change now? I'm happy to give it a try, unless you'd like to do it.

<link preload css> (dbw_tester.css?delay=2000& aka +3).. the protocol reports the initator as the preload line of html (via parser), so i think its the preload and the requests are not attributed to the attached onload js handler. it doesn't feel like preloaded css requests are blocking to me.

Should not be blocking, so this sounds like a reporting bug. I'll look into it.

I just +1'd your CL for this.

<link rel="import"> (aka -6) trace event didnt catch it but I don't think it was in-scope for yoav. that said, it'd be nice to mark 'em in blink as blocking.

Haven't we deprecated those?

What's a <link rel=import>? That's so Feb 2021 - they're now gone. 😄

bhackett1024 pushed a commit to replayio/chromium that referenced this issue Mar 30, 2021
Currently markup based preloads are considered blocking [1].
This CL makes sure that they're considered non-blocking, along with
dynamically added preloads.

[1] GoogleChrome/lighthouse#2065 (comment)

Change-Id: Ie11b67be1992751342f16bb3aee62b519b9582ff

Bug: 1193760
Change-Id: Ie11b67be1992751342f16bb3aee62b519b9582ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2791427
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/master@{#867694}
mjfroman pushed a commit to mjfroman/moz-libwebrtc-third-party that referenced this issue Oct 14, 2022
Currently markup based preloads are considered blocking [1].
This CL makes sure that they're considered non-blocking, along with
dynamically added preloads.

[1] GoogleChrome/lighthouse#2065 (comment)

Change-Id: Ie11b67be1992751342f16bb3aee62b519b9582ff

Bug: 1193760
Change-Id: Ie11b67be1992751342f16bb3aee62b519b9582ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2791427
Reviewed-by: Mason Freed <masonf@chromium.org>
Commit-Queue: Yoav Weiss <yoavweiss@chromium.org>
Cr-Commit-Position: refs/heads/master@{#867694}
GitOrigin-RevId: e25639bedc5e4a37374789ef46cb1fb704170842
@adamraine
Copy link
Member

https://chromium-review.googlesource.com/c/chromium/src/+/5208571

With the latest changes to Chrome, the trace's renderBlocking signal matches Lighthouse's TagsBlockingFirstPaint for dbw_tester.html:

❯❯❯ cat latest-run/artifacts.json | jq ".TagsBlockingFirstPaint.[] | .tag.url"
"http://localhost:10200/dobetterweb/dbw_tester.css?delay=100"
"http://localhost:10200/dobetterweb/unknown404.css?delay=200"
"http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200"
"http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped"
"http://localhost:10200/dobetterweb/dbw_tester.js"
"http://localhost:10200/dobetterweb/fcp-delayer.js?delay=5000"

❯❯❯ cat latest-run/defaultPass.trace.json | jq '.traceEvents.[] | select(.name == "ResourceSendRequest" and .args.data.renderBlocking == "blocking") | .args.data.url'
"http://localhost:10200/dobetterweb/dbw_tester.css?delay=100"
"http://localhost:10200/dobetterweb/unknown404.css?delay=200"
"http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200"
"http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped"
"http://localhost:10200/dobetterweb/dbw_tester.js"
"http://localhost:10200/dobetterweb/fcp-delayer.js?delay=5000"

@adamraine
Copy link
Member

I'm using the following script to compare URLs:

url=$1

node cli $1 -G --quiet --chrome-flags="--headless=new"

frame_id=$(cat latest-run/defaultPass.trace.json | jq -r '.traceEvents.[] | select(.name == "TracingStartedInBrowser") | .args.data.frames[0].frame')

old_urls=$(cat latest-run/artifacts.json | jq -r ".TagsBlockingFirstPaint.[] | .tag.url" | sort)
new_urls=$(cat latest-run/defaultPass.trace.json | jq -r ".traceEvents.[] | select(.name == \"ResourceSendRequest\" and .args.data.renderBlocking == \"blocking\" and .args.data.frame == \"$frame_id\") | .args.data.url" | sort)

sdiff <(echo "$old_urls") <(echo "$new_urls")

The outputs match on most sites that I tested (https://espn.com, https://cnn.com, https://theverge.com) but I did notice that https://store.google.com differs because resources are marked as in_body_parser_blocking instead of blocking. We should ensure that the shared trace engine accounts for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants