brendankenny
commented
7 years ago weren't we switching to some (future) debugger-protocol-based detection for this? Or was that something else
Open ebidel opened 7 years ago
brendankenny
commented
7 years ago weren't we switching to some (future) debugger-protocol-based detection for this? Or was that something else
ebidel
commented
7 years ago IIRC, @pavelfeldman was going to do something with blocking resources in the protocol.
paulirish
commented
5 years ago Exposing a render-blocking bit in the protocol would solve a lot of problems.
connorjclark
commented
5 years ago 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
commented
5 years ago 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
commented
5 years ago 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 blinkpat 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
commented
5 years ago 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
commented
5 years ago 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
commented
5 years ago 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
commented
5 years ago 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;
}
}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
commented
5 years ago Yup. It's a flow event so there's a matching event at the end of the async stretch:
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
commented
5 years ago 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
commented
5 years ago 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.
patrickhulce
commented
5 years ago 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
commented
5 years ago 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.
2) 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
commented
5 years ago 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.
Only called from creation of inline css from style elements. Pending counter is decreased once the inline css is parsed.
3) ProcessingInstruction::Process
XML stylesheets. Move along now.
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
commented
3 years ago 🎉 soon we'll have a trace event for render-blocking styles: https://chromium-review.googlesource.com/c/chromium/src/+/2626665
yoavweiss
commented
3 years ago The CL still has a bunch of open questions in the form of TODOs. I'd love y'all's opinions on the answers:
/cc @paullewis @pmeenan
pmeenan
commented
3 years ago Speaking for WebPageTest:
yoavweiss
commented
3 years ago
- 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
commented
3 years ago https://chromium-review.googlesource.com/c/chromium/src/+/2626665 landed! (including redirect support) Let the tracing begin!! :)
paulirish
commented
3 years ago 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
commented
3 years ago 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=200in_body_parser_blocking. I think we want to include it here, and rely on the "after fcp?" check we do in the audit.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).<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.<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@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
commented
3 years ago 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
commented
3 years ago 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
commented
3 years ago
<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. 😄
adamraine
commented
9 months ago 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"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.
https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/gather/gatherers/dobetterweb/tags-blocking-first-paint.js#L58 only checks for
scriptin<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>:document.body.lastElementChild).script,template, and anything else in the default stylesheet that's hidden by default.node.offsetParent === nullto determine if it is being rendered. Note: cannot usegetComputedStyle(node).display === 'nonewon'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