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

perf(gather-runner): Clear cache selectively per-pass #2156

Merged
merged 6 commits into from
May 9, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 6 additions & 13 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -795,19 +795,12 @@ class Driver {
.then(_ => this.online = true);
}

cleanAndDisableBrowserCaches() {
return Promise.all([
this.clearBrowserCache(),
this.disableBrowserCache()
]);
}

clearBrowserCache() {
return this.sendCommand('Network.clearBrowserCache');
}

disableBrowserCache() {
return this.sendCommand('Network.setCacheDisabled', {cacheDisabled: true});
cleanBrowserCaches() {
// Wipe entire disk cache
return this.sendCommand('Network.clearBrowserCache')
// Toggle 'Disable Cache' to evict the memory cache
.then(_ => this.sendCommand('Network.setCacheDisabled', {cacheDisabled: true}))
.then(_ => this.sendCommand('Network.setCacheDisabled', {cacheDisabled: false}));
}

clearDataForOrigin(url) {
Expand Down
25 changes: 15 additions & 10 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ let GathererResults; // eslint-disable-line no-unused-vars
* ii. beginEmulation
* iii. enableRuntimeEvents
* iv. evaluateScriptOnLoad rescue native Promise from potential polyfill
* v. cleanAndDisableBrowserCaches
* v. cleanBrowserCaches
* vi. clearDataForOrigin
*
* 2. For each pass in the config:
Expand All @@ -47,10 +47,12 @@ let GathererResults; // eslint-disable-line no-unused-vars
* ii. Enable network request blocking for specified patterns
* iii. all gatherers' beforePass()
* B. GatherRunner.pass()
* i. beginTrace (if requested) & beginDevtoolsLog
* ii. GatherRunner.loadPage()
* i. cleanBrowserCaches() (if it's a perf run)
* ii. beginDevtoolsLog()
* iii. beginTrace (if requested)
* iv. GatherRunner.loadPage()
* a. navigate to options.url (and wait for onload)
* iii. all gatherers' pass()
* v. all gatherers' pass()
* C. GatherRunner.afterPass()
* i. endTrace (if requested) & endDevtoolsLog & endThrottling
* ii. all gatherers' afterPass()
Expand Down Expand Up @@ -110,7 +112,6 @@ class GatherRunner {
.then(_ => driver.enableRuntimeEvents())
.then(_ => driver.cacheNatives())
.then(_ => driver.dismissJavaScriptDialogs())
.then(_ => resetStorage && driver.cleanAndDisableBrowserCaches())
.then(_ => resetStorage && driver.clearDataForOrigin(options.url))
.then(_ => gathererResults.UserAgent = [driver.getUserAgent()]);
}
Expand Down Expand Up @@ -201,16 +202,20 @@ class GatherRunner {
const config = options.config;
const gatherers = config.gatherers;

const recordTrace = config.recordTrace;
const isPerfRun = !options.flags.disableStorageReset && recordTrace && config.useThrottling;

const gatherernames = gatherers.map(g => g.name).join(', ');
const status = 'Loading page & waiting for onload';
log.log('status', status, gatherernames);

// Always record devtoolsLog.
driver.beginDevtoolsLog();

const pass = Promise.resolve()
// Begin tracing only if requested by config.
.then(_ => config.recordTrace && driver.beginTrace(options.flags))
// Clear disk & memory cache if it's a perf run
.then(_ => isPerfRun && driver.cleanBrowserCaches())
// Always record devtoolsLog
.then(_ => driver.beginDevtoolsLog())
// Begin tracing if requested by config.
.then(_ => recordTrace && driver.beginTrace(options.flags))
// Navigate.
.then(_ => GatherRunner.loadPage(driver, options))
.then(_ => log.log('statusEnd', status));
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/test/gather/fake-driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ module.exports = {
evaluateScriptOnLoad() {
return Promise.resolve();
},
cleanAndDisableBrowserCaches() {},
cleanBrowserCaches() {},
clearDataForOrigin() {},
cacheNatives() {
return Promise.resolve();
Expand Down
52 changes: 41 additions & 11 deletions lighthouse-core/test/gather/gather-runner-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ function getMockedEmulationDriver(emulationFn, netThrottleFn, cpuThrottleFn, blo
cacheNatives() {
return Promise.resolve();
}
cleanAndDisableBrowserCaches() {}
cleanBrowserCaches() {}
clearDataForOrigin() {}
getUserAgent() {
return Promise.resolve('Fake user agent');
Expand Down Expand Up @@ -242,10 +242,10 @@ describe('GatherRunner', function() {
});
});

it('clears the network cache and origin storage', () => {
it('clears origin storage', () => {
Copy link
Member

Choose a reason for hiding this comment

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

do you want to check that the cache gets turned off and back on again?

Copy link
Member Author

Choose a reason for hiding this comment

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

it doesnt go off and on, actually. in the ended didnt go with what the ticket explained. :)

just a one time "Clear everything" at the start of the pass.

added test for that.

Copy link
Member

Choose a reason for hiding this comment

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

it doesnt go off and on, actually

well I meant whatever this is doing here, then :P

// Toggle 'Disable Cache' to evict the memory cache
.then(_ => this.sendCommand('Network.setCacheDisabled', {cacheDisabled: true}))
.then(_ => this.sendCommand('Network.setCacheDisabled', {cacheDisabled: false}));

Copy link
Member Author

Choose a reason for hiding this comment

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

not really. :)

the only thing to really assert is that we want for the first sendCommand's promise to resolve before sending the second. (but technically that's not even necessary)

const asyncFunc = () => Promise.resolve();
const tests = {
calledDisableNetworkCache: false,
calledCleanBrowserCaches: false,
calledClearStorage: false,
};
const createCheck = variable => () => {
Expand All @@ -259,22 +259,50 @@ describe('GatherRunner', function() {
dismissJavaScriptDialogs: asyncFunc,
enableRuntimeEvents: asyncFunc,
cacheNatives: asyncFunc,
cleanAndDisableBrowserCaches: createCheck('calledDisableNetworkCache'),
cleanBrowserCaches: createCheck('calledCleanBrowserCaches'),
clearDataForOrigin: createCheck('calledClearStorage'),
blockUrlPatterns: asyncFunc,
getUserAgent: asyncFunc,
};

return GatherRunner.setupDriver(driver, {}, {flags: {}}).then(_ => {
assert.equal(tests.calledDisableNetworkCache, true);
assert.equal(tests.calledCleanBrowserCaches, false);
assert.equal(tests.calledClearStorage, true);
});
});

it('does not clear the cache & storage when disable-storage-reset flag is set', () => {
it('clears the disk & memory cache on a perf run', () => {
const asyncFunc = () => Promise.resolve();
const tests = {
calledDisableNetworkCache: false,
calledCleanBrowserCaches: false
};
const createCheck = variable => () => {
tests[variable] = true;
return Promise.resolve();
};
const driver = {
beginDevtoolsLog: asyncFunc,
beginTrace: asyncFunc,
gotoURL: asyncFunc,
cleanBrowserCaches: createCheck('calledCleanBrowserCaches')
};
const config = {
recordTrace: true,
useThrottling: true,
gatherers: []
};
const flags = {
disableStorageReset: false
};
return GatherRunner.pass({driver, config, flags}, {TestGatherer: []}).then(_ => {
assert.equal(tests.calledCleanBrowserCaches, true);
});
});

it('does not clear origin storage with flag --disable-storage-reset', () => {
const asyncFunc = () => Promise.resolve();
const tests = {
calledCleanBrowserCaches: false,
calledClearStorage: false,
};
const createCheck = variable => () => {
Expand All @@ -288,7 +316,7 @@ describe('GatherRunner', function() {
dismissJavaScriptDialogs: asyncFunc,
enableRuntimeEvents: asyncFunc,
cacheNatives: asyncFunc,
cleanAndDisableBrowserCaches: createCheck('calledDisableNetworkCache'),
cleanBrowserCaches: createCheck('calledCleanBrowserCaches'),
clearDataForOrigin: createCheck('calledClearStorage'),
blockUrlPatterns: asyncFunc,
getUserAgent: asyncFunc,
Expand All @@ -297,7 +325,7 @@ describe('GatherRunner', function() {
return GatherRunner.setupDriver(driver, {}, {
flags: {disableStorageReset: true}
}).then(_ => {
assert.equal(tests.calledDisableNetworkCache, false);
assert.equal(tests.calledCleanBrowserCaches, false);
assert.equal(tests.calledClearStorage, false);
});
});
Expand Down Expand Up @@ -357,8 +385,9 @@ describe('GatherRunner', function() {
new TestGatherer()
]
};
const flags = {};

return GatherRunner.pass({driver, config}, {TestGatherer: []}).then(_ => {
return GatherRunner.pass({driver, config, flags}, {TestGatherer: []}).then(_ => {
assert.equal(calledTrace, true);
});
});
Expand Down Expand Up @@ -405,8 +434,9 @@ describe('GatherRunner', function() {
new TestGatherer()
]
};
const flags = {};

return GatherRunner.pass({driver, config}, {TestGatherer: []}).then(_ => {
return GatherRunner.pass({driver, config, flags}, {TestGatherer: []}).then(_ => {
assert.equal(calledDevtoolsLogCollect, true);
});
});
Expand Down