perf: request block stylesheets and images on redirect pass #2168
Conversation
There was a problem hiding this comment.
nice cleanup on the test. :)
oh and fixing the fact that addBlockedUrls hasn't worked for a few months now. :)
we're not explicitly clearing our blocked urls at the end of a pass, but instead relying on a subsequent pass doing Network.setBlockedUrls({ urls: [] }) to clear things out. Can you add a comment on the blockUrlPatterns method that mentions this trick?
(it seems fine but it gave me a doubletake)
lighthouse-core/config/default.js
Outdated
| @@ -27,6 +27,18 @@ module.exports = { | |||
| { | |||
| "passName": "redirectPass", | |||
| "useThrottling": false, | |||
| // Speed up the redirect pass by blocking stylesheets, fonts, and images | |||
| "blockedUrlPatterns": [ | |||
| "*.css", | |||
There was a problem hiding this comment.
formatting nit: this is pretty low signal info, compared to the rest of config.
can these all share a line?
| @@ -45,7 +45,8 @@ let GathererResults; // eslint-disable-line no-unused-vars | |||
| * 2. For each pass in the config: | |||
| * A. GatherRunner.beforePass() | |||
| * i. navigate to about:blank | |||
| * ii. all gatherers' beforePass() | |||
| * ii. Enable network request blocking for specified patterns | |||
There was a problem hiding this comment.
set patterns for network request blocking
and you can remove "vii. blockUrlPatterns" from above.
lighthouse-core/gather/driver.js
Outdated
| blockUrlPatterns(urlPatterns) { | ||
| const promiseArr = urlPatterns.map(url => this.sendCommand('Network.addBlockedURL', {url})); | ||
| return Promise.all(promiseArr); | ||
| blockUrlPatterns(urls) { |
Yeah good call done, blocking will cease when we disconnect, right? |
lighthouse-core/gather/driver.js
Outdated
| * @return {!Promise} | ||
| */ | ||
| blockUrlPatterns(urls) { | ||
| return this.sendCommand('Network.setBlockedURLs', {urls}); |
There was a problem hiding this comment.
unfortunately this command was renamed/added recently..
commit: https://chromium.googlesource.com/chromium/src/+/01c043f5f744262981becd3d45a79981ad3f6b67
m58 (base rev 454471 ) which shipped to stable 2 weeks ago doesn't have this method. It has the add/remove pair instead.
IMO this is optional, and our other use of requestblocking is unused so i think we can ignore the failure. let's catch and exclude errors?
doublecheck this works with a stable m58 build
There was a problem hiding this comment.
what's going to be our policy going forward about handling stable chrome and protocol changes? it seems like it's mostly taken a backseat and we emphasize canary, should we add a stable config to travis if we want to support it (#1480)?
confirmed working now (with an error log) in 58
lighthouse-core/gather/driver.js
Outdated
| return Promise.all(promiseArr); | ||
| /** | ||
| * This method sets the URL patterns to be blocked and is called at the beginning of each pass. | ||
| * No "clearing" is done at the end of the pass since the next pass will set to [] if necessary. |
There was a problem hiding this comment.
since some future gatherers or other consumers of lighthouse (e.g. performanceExperiment) may want to use this method, probably makes more sense to move the bulk of this comment into beforePass where it's used like this
lighthouse-core/gather/driver.js
Outdated
| /** | ||
| * This method sets the URL patterns to be blocked and is called at the beginning of each pass. | ||
| * No "clearing" is done at the end of the pass since the next pass will set to [] if necessary. | ||
| * @param {!Array<string>} urls |
There was a problem hiding this comment.
maybe worth adding param doc similar to what the protocol docs has? ("URL patterns to block. Wildcards ('*') are allowed.")
| @@ -171,9 +170,12 @@ class GatherRunner { | |||
| * @return {!Promise} | |||
| */ | |||
| static beforePass(options, gathererResults) { | |||
| const blockedUrls = (options.config.blockedUrlPatterns || []) | |||
| .concat(options.flags.blockedUrlPatterns || []); | |||
There was a problem hiding this comment.
how do unions of url patterns work? Last one wins? (just curious)
There was a problem hiding this comment.
aside: we really need to get the overloaded naming of options under control here :)
There was a problem hiding this comment.
how do unions of url patterns work? Last one wins?
oh, is ! not supported? I guess in that case it doesn't matter
| @@ -27,6 +27,8 @@ module.exports = { | |||
| { | |||
| "passName": "redirectPass", | |||
| "useThrottling": false, | |||
| // Speed up the redirect pass by blocking stylesheets, fonts, and images | |||
There was a problem hiding this comment.
love having this in the default config as an example for custom configs
| @@ -27,6 +27,8 @@ module.exports = { | |||
| { | |||
| "passName": "redirectPass", | |||
| "useThrottling": false, | |||
| // Speed up the redirect pass by blocking stylesheets, fonts, and images | |||
| "blockedUrlPatterns": ["*.css", "*.jpg", "*.jpeg", "*.png", "*.gif", "*.svg", "*.ttf", "*.woff", "*.woff2"], | |||
There was a problem hiding this comment.
not sure about this one, but does it make sense to have these apply across beforePass, pass, and afterPass? Maybe it would make more sense/be less surprising to have them blocked only during pass, analogous to recordTrace, useThrottling, etc cc/ @paulirish
There was a problem hiding this comment.
yeah few random thoughts here
- we don't have any gatherers that load new content that wasn't already requested by the page
- we should probably discourage that behavior as something outside of Lighthouse realm anyway, and suggest requesting on the node side if a third party integration wants to do that rather than an
evaluateAsync(fetch('my-url')) - since afterPass doesn't freeze the page or anything and we're not guaranteed to be done at the 25s timeout it felt weird to unblock these urls while the page could feasibly still be requesting or retrying them
There was a problem hiding this comment.
we don't have any gatherers that load new content that wasn't already requested by the page
that's actually why I brought it up, so it wouldn't surprise us when someone does write a gatherer that tries it :)
we should probably discourage that behavior as something outside of Lighthouse realm anyway, and suggest requesting on the node side if a third party integration wants to do that rather than an evaluateAsync(fetch('my-url'))
but that's definitely something we rely on when we need to get around authentication or whatever. As an easy workaround to some of those issues, fetching in the context of the page can't be beat.
since afterPass doesn't freeze the page or anything and we're not guaranteed to be done at the 25s timeout it felt weird to unblock these urls while the page could feasibly still be requesting or retrying them
yeah, good point, and this is what really separates it from useThrottling. While matching their behavior makes some sense from a user-surprise perspective, disabling useThrottling in afterPass means the run goes faster due to potential work going faster, while disabling url blocking potentially means the run going slower due to (potentially) making work.
My real hesitation is magic while having no data about actual downsides. This is why we need users making custom gatherers :)
In the meantime, I suspect we'll have to revisit scheduling of all of this for #1826 anyways, so I'm good with this.
shaves off ~10s on certain sites like theverge.com