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

core(perf): add timing instrumentation to measure runtime perf #3745

Merged
merged 40 commits into from
Oct 25, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
b8830cb
core(perf): add instrumentation to measure LH runtime perf
paulirish Jun 22, 2018
df7e1b4
gatherEntries and no more .timing.total
paulirish Jun 22, 2018
f1a2909
fixup typing of traces.
paulirish Jun 22, 2018
c46548d
feedback.
paulirish Jun 22, 2018
ab9386d
split lib & cli
paulirish Jun 25, 2018
7c155b9
add tests
paulirish Jun 25, 2018
2a45eb5
report the total.
paulirish Jun 25, 2018
046780b
gather:true, rather than gatherEntries
paulirish Jun 25, 2018
4e77f81
Add additional instrumentation
paulirish Jun 26, 2018
c1dddac
Merge branch 'master' into timing
paulirish Aug 15, 2018
6df7096
fix tests
paulirish Aug 15, 2018
6b36595
Merge branch 'master' into timing
patrickhulce Aug 24, 2018
135f960
rebase
patrickhulce Aug 24, 2018
2715db9
use l-logger 1.1.0
paulirish Aug 27, 2018
4807052
instrument getBenchmarkIndex
paulirish Aug 27, 2018
fc8817f
minor
paulirish Aug 28, 2018
46e66ab
instrument computed artifacts
paulirish Aug 28, 2018
9125d05
keep artifacts.Timing up to date
paulirish Aug 28, 2018
b5cdd0c
Merge branch 'master' into timing
paulirish Sep 25, 2018
680a985
feedback
paulirish Sep 25, 2018
f1045a7
Merge branch 'master' into timing
paulirish Sep 25, 2018
17d90e0
Merge branch 'master' into timing
patrickhulce Oct 16, 2018
1a0c8be
integrity for new packages
patrickhulce Oct 16, 2018
caf144a
lint
patrickhulce Oct 16, 2018
6046bb4
feedback and tests
patrickhulce Oct 16, 2018
01aeac7
last things?
patrickhulce Oct 16, 2018
852561b
move artifacts.Timing
patrickhulce Oct 16, 2018
3971b1f
type check
patrickhulce Oct 16, 2018
8bf6cc1
more nits
patrickhulce Oct 16, 2018
88c888b
Merge branch 'master' into timing
patrickhulce Oct 18, 2018
bcfaf7c
more feedback
patrickhulce Oct 18, 2018
c72ac0c
Update typings/artifacts.d.ts
brendankenny Oct 19, 2018
274693a
Update artifacts.d.ts
paulirish Oct 19, 2018
bd03859
drop try/catch
paulirish Oct 19, 2018
011f272
Merge branch 'master' into timing
paulirish Oct 19, 2018
434f788
use status object
connorjclark Oct 24, 2018
2ba317f
Merge branch 'master' into timing
connorjclark Oct 25, 2018
8c7b0a2
update lh logger. dedupe time measures
connorjclark Oct 25, 2018
508506a
better code
connorjclark Oct 25, 2018
4a2e662
pr changes
connorjclark Oct 25, 2018
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
1 change: 1 addition & 0 deletions lighthouse-cli/test/cli/run-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ describe('CLI run', function() {
Object.keys(results.audits).length,
Object.keys(lhr.audits).length);
assert.deepStrictEqual(results.timing, lhr.timing);
assert.ok(results.timing.total !== 0);
Copy link
Member

Choose a reason for hiding this comment

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

what is this testing?


fs.unlinkSync(filename);
});
Expand Down
10 changes: 9 additions & 1 deletion lighthouse-core/config/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,8 @@ class Config {
* @param {LH.Flags=} flags
*/
constructor(configJSON, flags) {
const status = {msg: 'Create config', id: 'lh:init:config'};
log.time(status, 'verbose');
let configPath = flags && flags.configPath;

if (!configJSON) {
Expand Down Expand Up @@ -364,6 +366,7 @@ class Config {
// TODO(bckenny): until tsc adds @implements support, assert that Config is a ConfigJson.
/** @type {LH.Config.Json} */
const configJson = this; // eslint-disable-line no-unused-vars
log.timeEnd(status);
}

/**
Expand Down Expand Up @@ -748,6 +751,8 @@ class Config {
* @return {Config['audits']}
*/
static requireAudits(audits, configPath) {
const status = {msg: 'Requiring audits', id: 'lh:config:requireAudits'};
log.time(status, 'verbose');
const expandedAudits = Config.expandAuditShorthand(audits);
if (!expandedAudits) {
return null;
Expand Down Expand Up @@ -779,6 +784,7 @@ class Config {

const mergedAuditDefns = mergeOptionsOfItems(auditDefns);
mergedAuditDefns.forEach(audit => assertValidAudit(audit.implementation, audit.path));
log.timeEnd(status);
return mergedAuditDefns;
}

Expand Down Expand Up @@ -820,6 +826,8 @@ class Config {
if (!passes) {
return null;
}
const status = {msg: 'Requiring gatherers', id: 'lh:config:requireGatherers'};
log.time(status, 'verbose');

const coreList = Runner.getGathererList();
const fullPasses = passes.map(pass => {
Expand Down Expand Up @@ -853,7 +861,7 @@ class Config {

return Object.assign(pass, {gatherers: mergedDefns});
});

log.timeEnd(status);
return fullPasses;
}
}
Expand Down
4 changes: 4 additions & 0 deletions lighthouse-core/gather/computed/computed-artifact.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
'use strict';

const ArbitraryEqualityMap = require('../../lib/arbitrary-equality-map');
const log = require('lighthouse-logger');

class ComputedArtifact {
/**
Expand Down Expand Up @@ -59,12 +60,15 @@ class ComputedArtifact {
return computed;
}

const status = {msg: `Computing artifact: ${this.name}`, id: `lh:computed:${this.name}`};
log.time(status, 'verbose');
// Need to cast since `this.compute_(...)` returns the concrete return type
// of the base class's compute_, not the called derived class's.
const artifactPromise = /** @type {ReturnType<this['compute_']>} */ (
this.compute_(requiredArtifacts, this._allComputedArtifacts));
this._cache.set(requiredArtifacts, artifactPromise);

artifactPromise.then(() => log.timeEnd(status));
return artifactPromise;
}
}
Expand Down
18 changes: 14 additions & 4 deletions lighthouse-core/gather/driver.js
Original file line number Diff line number Diff line change
Expand Up @@ -136,25 +136,35 @@ class Driver {
* @return {Promise<LH.Crdp.Browser.GetVersionResponse & {milestone: number}>}
*/
async getBrowserVersion() {
const status = {msg: 'Getting browser version', id: 'lh:gather:getVersion'};
log.time(status, 'verbose');
const version = await this.sendCommand('Browser.getVersion');
const match = version.product.match(/\/(\d+)/); // eg 'Chrome/71.0.3577.0'
const milestone = match ? parseInt(match[1]) : 0;
log.timeEnd(status);
return Object.assign(version, {milestone});
}

/**
* Computes the ULTRADUMB™ benchmark index to get a rough estimate of device class.
* @return {Promise<number>}
*/
getBenchmarkIndex() {
return this.evaluateAsync(`(${pageFunctions.ultradumbBenchmarkString})()`);
async getBenchmarkIndex() {
const status = {msg: 'Benchmarking machine', id: 'lh:gather:getBenchmarkIndex'};
log.time(status);
const indexVal = await this.evaluateAsync(`(${pageFunctions.ultradumbBenchmarkString})()`);
log.timeEnd(status);
return indexVal;
}

/**
* @return {Promise<void>}
*/
connect() {
return this._connection.connect();
async connect() {
const status = {msg: 'Connecting to browser', id: 'lh:init:connect'};
log.time(status);
await this._connection.connect();
log.timeEnd(status);
}

/**
Expand Down
79 changes: 62 additions & 17 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,11 @@ class GatherRunner {
url = constants.defaultPassConfig.blankPage,
duration = constants.defaultPassConfig.blankDuration
) {
const status = {msg: 'Resetting state with about:blank', id: 'lh:gather:loadBlank'};
log.time(status);
await driver.gotoURL(url);
await new Promise(resolve => setTimeout(resolve, duration));
log.timeEnd(status);
}

/**
Expand All @@ -103,7 +106,8 @@ class GatherRunner {
* @return {Promise<void>}
*/
static async setupDriver(driver, options) {
log.log('status', 'Initializing…');
const status = {msg: 'Initializing…', id: 'lh:gather:setupDriver'};
log.time(status);
const resetStorage = !options.settings.disableStorageReset;
// Enable emulation based on settings
await driver.assertNoSameOriginServiceWorkerClients(options.requestedUrl);
Expand All @@ -114,21 +118,27 @@ class GatherRunner {
await driver.dismissJavaScriptDialogs();
await driver.listenForSecurityStateChanges();
if (resetStorage) await driver.clearDataForOrigin(options.requestedUrl);
log.timeEnd(status);
}

/**
* @param {Driver} driver
* @return {Promise<void>}
*/
static disposeDriver(driver) {
log.log('status', 'Disconnecting from browser...');
return driver.disconnect().catch(err => {
static async disposeDriver(driver) {
const status = {msg: 'Disconnecting from browser...', id: 'lh:gather:disconnect'};

log.time(status);
try {
await driver.disconnect();
} catch (err) {
// Ignore disconnecting error if browser was already closed.
// See https://github.com/GoogleChrome/lighthouse/issues/1583
if (!(/close\/.*status: 500$/.test(err.message))) {
log.error('GatherRunner disconnect', err.message);
}
});
}
log.timeEnd(status);
}

/**
Expand Down Expand Up @@ -183,6 +193,8 @@ class GatherRunner {
* @return {Promise<void>}
*/
static async beforePass(passContext, gathererResults) {
const bpStatus = {msg: `Running beforePass methods`, id: `lh:gather:beforePass`};
log.time(bpStatus, 'verbose');
const blockedUrls = (passContext.passConfig.blockedUrlPatterns || [])
.concat(passContext.settings.blockedUrlPatterns || []);
const blankPage = passContext.passConfig.blankPage;
Expand All @@ -198,10 +210,17 @@ class GatherRunner {
const gatherer = gathererDefn.instance;
// Abuse the passContext to pass through gatherer options
passContext.options = gathererDefn.options || {};
const status = {
msg: `Retrieving setup: ${gatherer.name}`,
id: `lh:gather:beforePass:${gatherer.name}`,
};
log.time(status, 'verbose');
const artifactPromise = Promise.resolve().then(_ => gatherer.beforePass(passContext));
gathererResults[gatherer.name] = [artifactPromise];
await artifactPromise.catch(() => {});
log.timeEnd(status);
}
log.timeEnd(bpStatus);
}

/**
Expand All @@ -220,9 +239,12 @@ class GatherRunner {
const recordTrace = config.recordTrace;
const isPerfRun = !settings.disableStorageReset && recordTrace && config.useThrottling;

const gatherernames = gatherers.map(g => g.instance.name).join(', ');
const status = 'Loading page & waiting for onload';
log.log('status', status, gatherernames);
const status = {
msg: 'Loading page & waiting for onload',
id: 'lh:gather:loadPage',
args: [gatherers.map(g => g.instance.name).join(', ')],
};
log.time(status);

// Clear disk & memory cache if it's a perf run
if (isPerfRun) await driver.cleanBrowserCaches();
Expand All @@ -233,19 +255,28 @@ class GatherRunner {

// Navigate.
await GatherRunner.loadPage(driver, passContext);
log.log('statusEnd', status);
log.timeEnd(status);

const pStatus = {msg: `Running pass methods`, id: `lh:gather:pass`};
log.time(pStatus, 'verbose');
for (const gathererDefn of gatherers) {
const gatherer = gathererDefn.instance;
// Abuse the passContext to pass through gatherer options
passContext.options = gathererDefn.options || {};
const status = {
msg: `Retrieving in-page: ${gatherer.name}`,
id: `lh:gather:pass:${gatherer.name}`,
};
log.time(status);
const artifactPromise = Promise.resolve().then(_ => gatherer.pass(passContext));

const gathererResult = gathererResults[gatherer.name] || [];
gathererResult.push(artifactPromise);
gathererResults[gatherer.name] = gathererResult;
await artifactPromise.catch(() => {});
log.timeEnd(status);
}
log.timeEnd(pStatus);
}

/**
Expand All @@ -263,16 +294,20 @@ class GatherRunner {

let trace;
if (config.recordTrace) {
log.log('status', 'Retrieving trace');
const status = {msg: 'Retrieving trace', id: `lh:gather:getTrace`};
log.time(status);
trace = await driver.endTrace();
log.verbose('statusEnd', 'Retrieving trace');
log.timeEnd(status);
}

const status = 'Retrieving devtoolsLog and network records';
log.log('status', status);
const status = {
msg: 'Retrieving devtoolsLog & network records',
id: `lh:gather:getDevtoolsLog`,
};
log.time(status);
const devtoolsLog = driver.endDevtoolsLog();
const networkRecords = NetworkRecorder.recordsFromLogs(devtoolsLog);
log.verbose('statusEnd', status);
log.timeEnd(status);

let pageLoadError = GatherRunner.getPageLoadError(passContext.url, networkRecords);
// If the driver was offline, a page load error is expected, so do not save it.
Expand All @@ -293,13 +328,18 @@ class GatherRunner {
trace,
};

const apStatus = {msg: `Running afterPass methods`, id: `lh:gather:afterPass`};
// Disable throttling so the afterPass analysis isn't throttled
await driver.setThrottling(passContext.settings, {useThrottling: false});
log.time(apStatus, 'verbose');

for (const gathererDefn of gatherers) {
const gatherer = gathererDefn.instance;
const status = `Retrieving: ${gatherer.name}`;
log.log('status', status);
const status = {
msg: `Retrieving: ${gatherer.name}`,
id: `lh:gather:afterPass:${gatherer.name}`,
};
log.time(status);

// Add gatherer options to the passContext.
passContext.options = gathererDefn.options || {};
Expand All @@ -314,8 +354,9 @@ class GatherRunner {
gathererResult.push(artifactPromise);
gathererResults[gatherer.name] = gathererResult;
await artifactPromise.catch(() => {});
log.verbose('statusEnd', status);
log.timeEnd(status);
}
log.timeEnd(apStatus);

// Resolve on tracing data using passName from config.
return passData;
Expand Down Expand Up @@ -358,6 +399,9 @@ class GatherRunner {
// Take only unique LighthouseRunWarnings.
baseArtifacts.LighthouseRunWarnings = Array.from(new Set(baseArtifacts.LighthouseRunWarnings));

// Take the timing entries we've gathered so far.
baseArtifacts.Timing = log.getTimeEntries();

// TODO(bckenny): correct Partial<LH.GathererArtifacts> at this point to drop cast.
return /** @type {LH.Artifacts} */ ({...baseArtifacts, ...gathererArtifacts});
}
Expand All @@ -377,6 +421,7 @@ class GatherRunner {
devtoolsLogs: {},
settings: options.settings,
URL: {requestedUrl: options.requestedUrl, finalUrl: ''},
Timing: [],
};
}

Expand Down
8 changes: 8 additions & 0 deletions lighthouse-core/lib/asset-saver.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@ async function loadArtifacts(basePath) {
artifacts.traces[passName] = Array.isArray(trace) ? {traceEvents: trace} : trace;
});

if (Array.isArray(artifacts.Timing)) {
// Any Timing entries in saved artifacts will have a different timeOrigin than the auditing phase
// The `gather` prop is read later in generate-timing-trace and they're added to a separate track of trace events
artifacts.Timing.forEach(entry => (entry.gather = true));
}
Copy link
Member

Choose a reason for hiding this comment

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

should this happen in gather-runner.js? After line 399

Copy link
Member Author

Choose a reason for hiding this comment

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

We only want to break the trace into two tracks if they're done at different times. (Classic -G then -A workflow)

So that's why we assign here to force a diff track.

return artifacts;
}

Expand All @@ -74,6 +79,8 @@ async function loadArtifacts(basePath) {
* @return {Promise<void>}
*/
async function saveArtifacts(artifacts, basePath) {
const status = {msg: 'Saving artifacts', id: 'lh:assetSaver:saveArtifacts'};
log.time(status);
mkdirp.sync(basePath);
rimraf.sync(`${basePath}/*${traceSuffix}`);
rimraf.sync(`${basePath}/${artifactsFilename}`);
Expand All @@ -95,6 +102,7 @@ async function saveArtifacts(artifacts, basePath) {
const restArtifactsString = JSON.stringify(restArtifacts, null, 2);
fs.writeFileSync(`${basePath}/${artifactsFilename}`, restArtifactsString, 'utf8');
log.log('Artifacts saved to disk in folder:', basePath);
log.timeEnd(status);
}

/**
Expand Down
Loading