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

Plots: make measure script more flexible (CLI args) #2183

Merged
merged 30 commits into from
May 31, 2017
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
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: 1 addition & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -47,4 +47,4 @@ lighthouse-cli/types/*.map
lighthouse-core/report/partials/templates/
lighthouse-core/report/templates/*.js

plots/out/
plots/out**
69 changes: 47 additions & 22 deletions plots/ab-screenshot/analyze.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ const path = require('path');
const opn = require('opn');
const args = require('yargs').argv;

const runs = args['runs'] || 1;

const Metrics = require('../../lighthouse-core/lib/traces/pwmetrics-events');

const constants = require('../constants');
Expand Down Expand Up @@ -84,25 +86,60 @@ function aggregate(outPathA, outPathB) {
if (!utils.isDir(sitePathB)) {
return;
}
const siteScreenshotsComparison = {
siteName: siteDir,
runA: analyzeSingleRunScreenshots(sitePathA),
runB: analyzeSingleRunScreenshots(sitePathB)
};
results.push(siteScreenshotsComparison);

for (let i = 0; i < runs; i++) {
const runDirA = getRunDir(sitePathA, i);
const runDirB = getRunDir(sitePathB, i);

const runPathA = path.resolve(sitePathA, runDirA);
const runPathB = path.resolve(sitePathB, runDirB);

const lighthouseFileA = path.resolve(runPathA, constants.LIGHTHOUSE_RESULTS_FILENAME);
const lighthouseFileB = path.resolve(runPathB, constants.LIGHTHOUSE_RESULTS_FILENAME);

if (!utils.isFile(lighthouseFileA) || !utils.isFile(lighthouseFileB)) {
continue;
}

const siteScreenshotsComparison = {
siteName: `${siteDir} runA: ${runDirA} runB: ${runDirB}`,
runA: analyzeSingleRunScreenshots(runPathA),
runB: analyzeSingleRunScreenshots(runPathB),
};
results.push(siteScreenshotsComparison);
}
});

return results;
}

/**
* Analyzes the screenshots for the first run of a particular site.
* @param {string} sitePath
* @param {number} runIndex
* @return {string}
*/
function getRunDir(sitePath, runIndex) {
return sortAndFilterRunFolders(fs.readdirSync(sitePath))[runIndex];
}

/**
* @param {!Array<string>} folders
* @return {!Array<string>}
*/
function sortAndFilterRunFolders(folders) {
return folders
.filter(folder => folder !== '.DS_Store')
.map(folder => Number(folder))
.sort((a, b) => a - b)
.map(folder => folder.toString());
}

/**
* Analyzes the screenshots for the first run of a particular site.
* @param {string} runPath
* @return {!SingleRunScreenshots}
*/
function analyzeSingleRunScreenshots(sitePath) {
const runDir = sortAndFilterRunFolders(fs.readdirSync(sitePath))[0];
const runPath = path.resolve(sitePath, runDir);
function analyzeSingleRunScreenshots(runPath) {
const lighthouseResultsPath = path.resolve(runPath, constants.LIGHTHOUSE_RESULTS_FILENAME);
const lighthouseResults = JSON.parse(fs.readFileSync(lighthouseResultsPath));

Expand Down Expand Up @@ -152,18 +189,6 @@ function analyzeSingleRunScreenshots(sitePath) {
}
}

/**
* @param {!Array<string>} folders
* @return {!Array<string>}
*/
function sortAndFilterRunFolders(folders) {
return folders
.filter(folder => folder !== '.DS_Store')
.map(folder => Number(folder))
.sort((a, b) => a - b)
.map(folder => folder.toString());
}

/**
* Marks the first screenshot that happens after a particular perf timing.
* @param {SingleRunScreenshots} results
Expand Down
158 changes: 132 additions & 26 deletions plots/measure.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,34 @@ const path = require('path');
const parseURL = require('url').parse;

const mkdirp = require('mkdirp');
const args = require('yargs')
Copy link
Member

Choose a reason for hiding this comment

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

with the new style of invoking, can you update the readme?

to me, it makes more sense to use the node script.js invocation rather than the npm run one now that we have a CLI. but no big deal.

.wrap(Math.min(process.stdout.columns, 120))
.help('help')
.usage('node measure.js [options]')
.example('node $0 -n 3 --sites-path ./sample-sites.json')
.example('node $0 --site https://google.com/')
.example('node $0 --subset')
.describe({
'n': 'Number of runs to do per site',
'reuse-chrome': 'Reuse the same Chrome instance across all site runs',
'keep-first-run': 'By default if you use --reuse-chrome, the first run results are discarded',
})
.group(
['disable-device-emulation', 'disable-cpu-throttling', 'disable-network-throttling'],
'Chrome DevTools settings:')
Copy link
Member

Choose a reason for hiding this comment

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

let's call this Lighthouse settings:

.describe({
'disable-device-emulation': 'Disable Nexus 5X emulation',
'disable-cpu-throttling': 'Disable CPU throttling',
'disable-network-throttling': 'Disable network throttling',
})
.group(['sites-path', 'subset', 'site'], 'Options to specify sites:')
.describe({
'sites-path': 'Include relative path of a json file with urls to run',
'subset': 'Measure a subset of popular sites',
'site': 'Include a specific site url to run',
})
.boolean(['disable-device-emulation', 'disable-cpu-throttling', 'disable-network-throttling'])
.argv;

const constants = require('./constants.js');
const utils = require('./utils.js');
Expand All @@ -31,9 +59,10 @@ const ChromeLauncher = require('../chrome-launcher/chrome-launcher.js');
const Printer = require('../lighthouse-cli/printer');
const assetSaver = require('../lighthouse-core/lib/asset-saver.js');

const NUMBER_OF_RUNS = 20;
const keepFirstRun = args['keep-first-run'] || !args['reuse-chrome'];
Copy link
Member

Choose a reason for hiding this comment

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

you can use camelcase instead of this square bracket dance.

args.keepFirstRun etc. yargs does it for you.

const numberOfRuns = args['n'] || 3;

const URLS = [
const SITES = [
// Flagship sites
'https://nytimes.com',
'https://flipkart.com',
Expand All @@ -59,19 +88,18 @@ const URLS = [
// Also removed sites that don't have significant index pages:
// "t.co", "popads.net", "onclickads.net", "microsoftonline.com", "onclckds.com", "cnzz.com",
// "live.com", "adf.ly", "googleusercontent.com",

'https://google.com',
'https://www.google.com/search?q=flowers',
'https://youtube.com',
'https://facebook.com',
'https://baidu.com',
'https://wikipedia.org',
'https://en.wikipedia.org/wiki/Google',
'https://yahoo.com',
'https://amazon.com',
'http://www.qq.com/',
'https://taobao.com',
'https://vk.com',
'https://twitter.com',
'https://instagram.com',
'https://mobile.twitter.com/ChromeDevTools',
'https://www.instagram.com/stephencurry30',
'http://www.hao123.cn/',
'http://www.sohu.com/',
'https://sina.com.cn',
Expand Down Expand Up @@ -119,47 +147,117 @@ const URLS = [
'https://bbc.com'
];

/**
* Launches Chrome once at the beginning, runs all the analysis,
* and then kills Chrome.
* TODO(chenwilliam): measure the overhead of starting chrome, if it's minimal
* then open a fresh Chrome instance for each run.
*/
function getUrls() {
if (args['sites-path']) {
Copy link
Member

Choose a reason for hiding this comment

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

since we already have this, why not toss the default SITES into a file and have sites-path default to that one.

return require(path.resolve(__dirname, args['sites-path']));
}

if (args['site']) {
return [args['site']];
}

if (args['subset']) {
return [
Copy link
Member

Choose a reason for hiding this comment

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

how about we just slice the top of the full sites list instead of repeating here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not DRY but I wanted to both have a set of sites that were useful for cpu/network analysis (basically the most visible / meaningful sites) and preserve the original order of the sites list since it explains in comments how they were sourced.

Copy link
Member

Choose a reason for hiding this comment

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

and it seems like we could just extract this into a diff sites_subset.js file?

'https://en.wikipedia.org/wiki/Google',
'https://mobile.twitter.com/ChromeDevTools',
'https://www.instagram.com/stephencurry30',
'https://amazon.com',
'https://nytimes.com',
'https://www.google.com/search?q=flowers',

'https://flipkart.com',
'http://www.espn.com/',
'https://www.washingtonpost.com/pwa/',
'http://www.npr.org/',
'http://www.booking.com/',
'https://youtube.com',
'https://reddit.com',
'https://ebay.com',
'https://stackoverflow.com',
'https://apple.com',

// Could not run nasa on gin3g
'https://www.nasa.gov/',
];
}

return SITES;
}

const URLS = getUrls();

function main() {
if (numberOfRuns === 1 && !keepFirstRun) {
console.log('ERROR: You are only doing one run and re-using chrome, but did not specify --keep-first-run');
return;
}

if (utils.isDir(constants.OUT_PATH)) {
console.log('ERROR: Found output from previous run at: ', constants.OUT_PATH);
console.log('Please run: npm run clean');
return;
}

return ChromeLauncher.launch({port: 9222})
.then(launcher => {
return runAnalysis()
if (args['reuse-chrome']) {
ChromeLauncher.launch().then(launcher => {
return runAnalysisWithExistingChromeInstances(launcher)
.catch(err => console.error(err))
.then(() => launcher.kill());
});
return;
}
runAnalysisWithNewChromeInstances();
}

main();

/**
* Launches a new Chrome instance for each site run.
* Returns a promise chain that analyzes all the sites n times.
* @return {!Promise}
*/
function runAnalysisWithNewChromeInstances() {
Copy link
Member

Choose a reason for hiding this comment

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

👍

let promise = Promise.resolve();

for (let i = 0; i < numberOfRuns; i++) {
// Averages out any order-dependent effects such as memory pressure
utils.shuffle(URLS);

const id = i.toString();
const isFirstRun = i === 0;
const ignoreRun = keepFirstRun ? false : isFirstRun;
for (const url of URLS) {
promise = promise.then(() => {
return ChromeLauncher.launch().then(launcher => {
return singleRunAnalysis(url, id, launcher, {ignoreRun})
.catch(err => console.error(err))
.then(() => launcher.kill());
})
.catch(err => console.error(err));
});
}
}
return promise;
}

/**
* Reuses existing Chrome instance for all site runs.
* Returns a promise chain that analyzes all the sites n times.
* @param {!Launcher} launcher
* @return {!Promise}
*/
function runAnalysis() {
function runAnalysisWithExistingChromeInstances(launcher) {
let promise = Promise.resolve();

// Running it n + 1 times because the first run is deliberately ignored
// because it has different perf characteristics from subsequent runs
// (e.g. DNS cache which can't be easily reset between runs)
for (let i = 0; i <= NUMBER_OF_RUNS; i++) {
for (let i = 0; i < numberOfRuns; i++) {
// Averages out any order-dependent effects such as memory pressure
utils.shuffle(URLS);

const id = i.toString();
const isFirstRun = i === 0;
const ignoreRun = keepFirstRun ? false : isFirstRun;
for (const url of URLS) {
promise = promise.then(() => singleRunAnalysis(url, id, {ignoreRun: isFirstRun}));
promise = promise.then(() => singleRunAnalysis(url, id, launcher, {ignoreRun}));
}
}
return promise;
Expand All @@ -169,10 +267,11 @@ function runAnalysis() {
* Analyzes a site a single time using lighthouse.
* @param {string} url
* @param {string} id
* @param {!Launcher} launcher
* @param {{ignoreRun: boolean}} options
* @return {!Promise}
*/
function singleRunAnalysis(url, id, {ignoreRun}) {
function singleRunAnalysis(url, id, launcher, {ignoreRun}) {
console.log('Measuring site:', url, 'run:', id);
const parsedURL = parseURL(url);
const urlBasedFilename = sanitizeURL(`${parsedURL.host}-${parsedURL.pathname}`);
Expand All @@ -182,20 +281,27 @@ function singleRunAnalysis(url, id, {ignoreRun}) {
}
const outputPath = path.resolve(runPath, constants.LIGHTHOUSE_RESULTS_FILENAME);
const assetsPath = path.resolve(runPath, 'assets');
return analyzeWithLighthouse(url, outputPath, assetsPath, {ignoreRun});
return analyzeWithLighthouse(launcher, url, outputPath, assetsPath, {ignoreRun});
}

/**
* Runs lighthouse and save the artifacts (not used directly by plots,
* but may be helpful for debugging outlier runs).
* @param {!Launcher} launcher
* @param {string} url
* @param {string} outputPath
* @param {string} assetsPath
* @param {{ignoreRun: boolean}} options
* @return {!Promise}
*/
function analyzeWithLighthouse(url, outputPath, assetsPath, {ignoreRun}) {
const flags = {output: 'json'};
function analyzeWithLighthouse(launcher, url, outputPath, assetsPath, {ignoreRun}) {
const flags = {
Copy link
Member

Choose a reason for hiding this comment

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

if ignoreRun is true then there's no reason to have throttling on. I'd force disable cpu & network throttling in these cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

output: 'json',
disableCpuThrottling: args['disable-cpu-throttling'],
disableNetworkThrottling: args['disable-network-throttling'],
disableDeviceEmulation: args['disable-device-emulation'],
port: launcher.port,
};
return lighthouse(url, flags, config)
.then(lighthouseResults => {
if (ignoreRun) {
Expand Down
29 changes: 29 additions & 0 deletions plots/plot-per-site.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
<!--
Copy link
Member

Choose a reason for hiding this comment

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

rename this to bars-per-site ?

Copyright 2017 Google Inc. All rights reserved.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->

<!doctype html>
<html>

<head>
<script src="https://cdn.plot.ly/plotly-1.25.2.min.js"></script>
</head>

<body>
Copy link
Member

Choose a reason for hiding this comment

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

add this page to the <nav> of the other two html files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

<script src="./out/generatedResults.js"></script>
<script src="./plot-per-site.js"></script>
</body>

</html>
Loading