-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Changes from 17 commits
2b7da18
5577cf7
9d11132
4655cbe
d38d070
a7eaf8e
adb59a9
57b763e
506dc30
b8c4577
ed44705
39f6b4c
5338f89
d6cabdb
52ef9ef
7f214eb
72ae90d
963ec9e
da6581b
1447442
922d7ab
f637160
2170a9e
3d4c291
cc59d4d
f50a21f
8555129
e93c300
92a927a
ebdea6e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,34 @@ const path = require('path'); | |
const parseURL = require('url').parse; | ||
|
||
const mkdirp = require('mkdirp'); | ||
const args = require('yargs') | ||
.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:') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's call this |
||
.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'); | ||
|
@@ -31,9 +59,29 @@ 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 DISABLE_DEVICE_EMULATION = args['disable-device-emulation']; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tbh seems kinda odd to establish these all as uppercase consts as they are dependent on CLI input. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah - I removed them and just inlined their usage |
||
const DISABLE_CPU_THROTTLING = args['disable-cpu-throttling']; | ||
const DISABLE_NETWORK_THROTTLING = args['disable-network-throttling']; | ||
const REUSE_CHROME = args['reuse-chrome']; | ||
const KEEP_FIRST_RUN = args['keep-first-run'] || !REUSE_CHROME; | ||
const SITES_PATH = args['sites-path']; | ||
const SUBSET = args['subset']; | ||
const SITE = args['site']; | ||
const CUSTOM_NUMBER_OF_RUNS = args['n']; | ||
|
||
const URLS = [ | ||
// Running it n + 1 times if 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) | ||
const NUMBER_OF_RUNS = (CUSTOM_NUMBER_OF_RUNS || 20) + (KEEP_FIRST_RUN ? 0 : 1); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. doesn't seem like an uppercase const as its super conditional. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agreed There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's unexpected that number_of_runs == 1 will provide two runs for each site. Can we make sure the number here will match # of actual runs done? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK - I updated it so there's no +1 run. I added a check in |
||
|
||
const FLAGS = { | ||
output: 'json', | ||
disableCpuThrottling: DISABLE_CPU_THROTTLING, | ||
disableNetworkThrottling: DISABLE_NETWORK_THROTTLING, | ||
disableDeviceEmulation: DISABLE_DEVICE_EMULATION, | ||
}; | ||
|
||
const SITES = [ | ||
// Flagship sites | ||
'https://nytimes.com', | ||
'https://flipkart.com', | ||
|
@@ -60,18 +108,18 @@ const URLS = [ | |
// "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', | ||
|
@@ -119,46 +167,112 @@ 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 (SITES_PATH) { | ||
return require(path.resolve(__dirname, SITES_PATH)); | ||
} | ||
|
||
if (SITE) { | ||
return [SITE]; | ||
} | ||
|
||
if (SUBSET) { | ||
return [ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (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; | ||
} | ||
|
||
const launcher = ChromeLauncher.launch(); | ||
|
||
return launcher.then(() => runAnalysis()) | ||
.catch(err => console.error(err)) | ||
.then(() => launcher.kill()); | ||
if (REUSE_CHROME) { | ||
ChromeLauncher.launch({port: 9222}).then(launcher => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've been running multiple plots at a time recently.. Launcher is getting a unique port by default, but you have to get it off Can you support this functionality? You could just pass it along like this or create measure instances that hold this state.. wdyt? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yar. Done. |
||
return runAnalysisWithExistingChromeInstances() | ||
.catch(err => console.error(err)) | ||
.then(() => launcher.kill()); | ||
}); | ||
return; | ||
} else { | ||
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() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
let promise = Promise.resolve(); | ||
|
||
for (let i = 0; i < NUMBER_OF_RUNS; i++) { | ||
// Averages out any order-dependent effects such as memory pressure | ||
utils.shuffle(URLS); | ||
|
||
const id = i.toString(); | ||
const isFirstRun = i === 0; | ||
const ignoreRun = KEEP_FIRST_RUN ? false : isFirstRun; | ||
for (const url of URLS) { | ||
promise = promise.then(() => { | ||
return ChromeLauncher.launch({port: 9222}).then(launcher => { | ||
return singleRunAnalysis(url, id, {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. | ||
* @return {!Promise} | ||
*/ | ||
function runAnalysis() { | ||
function runAnalysisWithExistingChromeInstances() { | ||
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 < NUMBER_OF_RUNS; i++) { | ||
// Averages out any order-dependent effects such as memory pressure | ||
utils.shuffle(URLS); | ||
|
||
const id = i.toString(); | ||
const isFirstRun = i === 0; | ||
const ignoreRun = KEEP_FIRST_RUN ? false : isFirstRun; | ||
for (const url of URLS) { | ||
promise = promise.then(() => singleRunAnalysis(url, id, {ignoreRun: isFirstRun})); | ||
promise = promise.then(() => singleRunAnalysis(url, id, {ignoreRun})); | ||
} | ||
} | ||
return promise; | ||
|
@@ -194,8 +308,7 @@ function singleRunAnalysis(url, id, {ignoreRun}) { | |
* @return {!Promise} | ||
*/ | ||
function analyzeWithLighthouse(url, outputPath, assetsPath, {ignoreRun}) { | ||
const flags = {output: 'json'}; | ||
return lighthouse(url, flags, config) | ||
return lighthouse(url, FLAGS, config) | ||
.then(lighthouseResults => { | ||
if (ignoreRun) { | ||
console.log('First load of site. Results not being saved to disk.'); | ||
|
@@ -205,7 +318,7 @@ function analyzeWithLighthouse(url, outputPath, assetsPath, {ignoreRun}) { | |
.saveAssets(lighthouseResults.artifacts, lighthouseResults.audits, assetsPath) | ||
.then(() => { | ||
lighthouseResults.artifacts = undefined; | ||
return Printer.write(lighthouseResults, flags.output, outputPath); | ||
return Printer.write(lighthouseResults, FLAGS.output, outputPath); | ||
}); | ||
}) | ||
.catch(err => console.error(err)); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<!-- | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add this page to the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> |
There was a problem hiding this comment.
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 thenpm run
one now that we have a CLI. but no big deal.