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.

Sign up for GitHub

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

Jump to bottom

perf: request block stylesheets and images on redirect pass #2168

Merged
merged 4 commits into from
May 9, 2017
Merged

perf: request block stylesheets and images on redirect pass #2168

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ec74058
perf: request block stylesheets and images on redirect pass
patrickhulce May 5, 2017
c2f92c3
nits and comments
patrickhulce May 8, 2017
c93b2fb
add catch for m58
patrickhulce May 8, 2017
091f82b
move comments
patrickhulce May 8, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions lighthouse-core/config/default.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ module.exports = {
{
"passName": "redirectPass",
"useThrottling": false,
// Speed up the redirect pass by blocking stylesheets, fonts, and images
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love having this in the default config as an example for custom configs

"blockedUrlPatterns": ["*.css", "*.jpg", "*.jpeg", "*.png", "*.gif", "*.svg", "*.ttf", "*.woff", "*.woff2"],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah few random thoughts here

  1. we don't have any gatherers that load new content that wasn't already requested by the page
  2. 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'))
  3. 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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

"gatherers": [
"http-redirect",
"html-without-javascript"
Expand Down
15 changes: 12 additions & 3 deletions lighthouse-core/gather/driver.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -852,9 +852,18 @@ class Driver {
return collectUsage;
}

blockUrlPatterns(urlPatterns) {
const promiseArr = urlPatterns.map(url => this.sendCommand('Network.addBlockedURL', {url}));
return Promise.all(promiseArr);
/**
* @param {!Array<string>} urls URL patterns to block. Wildcards ('*') are allowed.
* @return {!Promise}
*/
blockUrlPatterns(urls) {
return this.sendCommand('Network.setBlockedURLs', {urls})
.catch(err => {
// TODO: remove this handler once m59 hits stable
if (!/wasn't found/.test(err.message)) {
throw err;
}
});
}

/**
Expand Down
13 changes: 9 additions & 4 deletions lighthouse-core/gather/gather-runner.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,12 @@ let GathererResults; // eslint-disable-line no-unused-vars
* iv. evaluateScriptOnLoad rescue native Promise from potential polyfill
* v. cleanAndDisableBrowserCaches
* vi. clearDataForOrigin
* vii. blockUrlPatterns
*
* 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set patterns for network request blocking

and you can remove "vii. blockUrlPatterns" from above.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* iii. all gatherers' beforePass()
* B. GatherRunner.pass()
* i. beginTrace (if requested) & beginDevtoolsLog
* ii. GatherRunner.loadPage()
Expand Down Expand Up @@ -112,7 +112,6 @@ class GatherRunner {
.then(_ => driver.dismissJavaScriptDialogs())
.then(_ => resetStorage && driver.cleanAndDisableBrowserCaches())
.then(_ => resetStorage && driver.clearDataForOrigin(options.url))
.then(_ => driver.blockUrlPatterns(options.flags.blockedUrlPatterns || []))
.then(_ => gathererResults.UserAgent = [driver.getUserAgent()]);
}

Expand Down Expand Up @@ -171,9 +170,15 @@ class GatherRunner {
* @return {!Promise}
*/
static beforePass(options, gathererResults) {
const blockedUrls = (options.config.blockedUrlPatterns || [])
.concat(options.flags.blockedUrlPatterns || []);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do unions of url patterns work? Last one wins? (just curious)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aside: we really need to get the overloaded naming of options under control here :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how do unions of url patterns work? Last one wins?

oh, is ! not supported? I guess in that case it doesn't matter

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

! isnt supported, correct.

const blankPage = options.config.blankPage;
const blankDuration = options.config.blankDuration;
const pass = GatherRunner.loadBlank(options.driver, blankPage, blankDuration);
const pass = GatherRunner.loadBlank(options.driver, blankPage, blankDuration)
// Set request blocking before any network activity
// No "clearing" is done at the end of the pass since blockUrlPatterns([]) will unset all if
// neccessary at the beginning of the next pass.
.then(() => options.driver.blockUrlPatterns(blockedUrls));

return options.config.gatherers.reduce((chain, gatherer) => {
return chain.then(_ => {
Expand Down
34 changes: 23 additions & 11 deletions lighthouse-core/test/gather/gather-runner-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ function getMockedEmulationDriver(emulationFn, netThrottleFn, cpuThrottleFn, blo
case 'Emulation.setDeviceMetricsOverride':
fn = emulationFn;
break;
case 'Network.addBlockedURL':
case 'Network.setBlockedURLs':
fn = blockUrlFn;
break;
default:
Expand Down Expand Up @@ -303,25 +303,37 @@ describe('GatherRunner', function() {
});

it('tells the driver to block given URL patterns when blockedUrlPatterns is given', () => {
const receivedUrlPatterns = [];
const urlPatterns = ['http://*.evil.com', '.jpg', '.woff2'];
let receivedUrlPatterns = null;
const driver = getMockedEmulationDriver(null, null, null, params => {
receivedUrlPatterns.push(params.url);
receivedUrlPatterns = params.urls;
});

return GatherRunner.setupDriver(driver, {}, {flags: {blockedUrlPatterns: urlPatterns.slice()}})
.then(() => assert.deepStrictEqual(receivedUrlPatterns.sort(), urlPatterns.sort()));
return GatherRunner.beforePass({
driver,
flags: {
blockedUrlPatterns: ['http://*.evil.com', '.jpg', '.woff2'],
},
config: {
blockedUrlPatterns: ['*.jpeg'],
gatherers: [],
},
}).then(() => assert.deepStrictEqual(
receivedUrlPatterns.sort(),
['*.jpeg', '.jpg', '.woff2', 'http://*.evil.com']
));
});

it('does not throw when blockedUrlPatterns is not given', () => {
const receivedUrlPatterns = [];
let receivedUrlPatterns = null;
const driver = getMockedEmulationDriver(null, null, null, params => {
receivedUrlPatterns.push(params.url);
receivedUrlPatterns = params.urls;
});

return GatherRunner.setupDriver(driver, {}, {flags: {}})
.then(() => assert.equal(receivedUrlPatterns.length, 0))
.catch(() => assert.ok(false));
return GatherRunner.beforePass({
driver,
flags: {},
config: {gatherers: []},
}).then(() => assert.deepStrictEqual(receivedUrlPatterns, []));
});

it('tells the driver to begin tracing', () => {
Expand Down