Measure and visualize perf metrics #1936

wwwillchen · 2017-03-30T18:02:15Z

Follow-up work:

Do a side-by-side perf metrics comparison with a previous version of Lighthouse (allows you to easily see how a commit will change the metrics on real-world sites)
Run this on a compute instance in Asia to test the hypothesis that the high variance for international sites is driven by network latency

Fixes part of #1924

paulirish · 2017-03-30T20:00:27Z

lighthouse-core/audits/time-to-interactive.js

@@ -79,6 +79,10 @@ class TTIMetric extends Audit {

      const fmpTiming = fMPtsInMS - navStartTsInMS;

+      const traceEndTsInMS = trace.traceEvents.reduce(
+        (acc, traceEvent) => Math.max(acc, traceEvent.ts), 0) / 1000;


lgtm. we have tabTrace.processEvents, which is already sorted, but it's missing other processes (like browser, in particular). After considering it, we definitely want the last last TS in the trace, regardless of process. So let's do it this way. :)

Also this Math.max reduce is crazy fast. On a 61MB trace, this completed in 15ms. :) Meanwhile a sort() took ~160ms.

paulirish · 2017-03-30T20:01:50Z

lighthouse-plots/README.md

+```
+# View all commands
+$ cd lighthouse-plots
+$ npm run


let's upgrade the hipster value, shall we @wwwillchen ? :)

yarn run yarn measure yarn analyze

let's upgrade the hipster value

people running yarn generally know how to translate npm commands to yarn. I'd wager there are many people who use yarn who don't know how to translate the other way (or at the very least, would have to look it up)

@brendankenny brings up a good point - I'll leave these alone for now.

patrickhulce

awesome stuff!!

patrickhulce · 2017-03-30T19:12:14Z

lighthouse-plots/measure.js

+ */
+async function singleRunAnalysis(url, id, ignoreRun) {
+  const parsedURL = parseURL(url);
+  const host = `${parsedURL.host}-${parsedURL.pathname}`


nit: maybe just string.replace(/[./]+/g, '-')

can we call this filenameHost or something?

Also I wrote this bit for traceviewer in case you want to reuse:

var illegalRe = /[\/\?<>\\:\*\|":]/g; var controlRe = /[\x00-\x1f\x80-\x9f]/g; var reservedRe = /^\.+$/; return filename .replace(illegalRe, '.') .replace(controlRe, '\u2022') .replace(reservedRe, '') .replace(/\s+/g, '_');

patrickhulce · 2017-03-30T20:10:02Z

lighthouse-plots/measure.js

+    const id = i.toString();
+    // Might not want to record the first run because it has different perf characteristics from
+    // subsequent runs (e.g. DNS cache which can't be easily reset between runs)
+    const isFirstRun = false;


did you end up running with the first run disabled?

I had the first run enabled - but I'm going to disable it going forward.

patrickhulce · 2017-03-30T20:15:01Z

lighthouse-plots/analyze.js

@@ -0,0 +1,159 @@
+'use strict';


need license on all these

patrickhulce · 2017-03-30T20:16:32Z

lighthouse-plots/analyze.js

+// TODO: annotate
+function main() {
+  const allResults = [];
+  fs.readdirSync(OUT_PATH).map(siteDir => {


is out path the "out" of the previous step? so input to this script? I was kinda confused at first

Yeah - it's like how Chromium uses out/ to store all generated files and sometimes uses out/ as inputs to generate additional files.

patrickhulce · 2017-03-30T20:17:10Z

lighthouse-plots/analyze.js

+  const siteResults = [];
+  fs.readdirSync(sitePath).forEach(runDir => {
+    const runPath = path.resolve(sitePath, runDir);
+    const lighthouseReportPath = path.resolve(runPath, 'lighthouse.json');


want to pass this in to readResult instead?

brendankenny

looks great!

bikeshed territory: it would be good to drop the lighthouse- prefix on the directory (lighthouse-core et al. were a mistake we'll correct someday :)

But also plots may not be the best descriptor since it's just as much a test runner and and aggregator. Something to do with analysis or monitoring? plots might work too, though :)

brendankenny · 2017-03-30T20:11:07Z

lighthouse-plots/README.md

+```
+# View all commands
+$ cd lighthouse-plots
+$ npm run


let's upgrade the hipster value

people running yarn generally know how to translate npm commands to yarn. I'd wager there are many people who use yarn who don't know how to translate the other way (or at the very least, would have to look it up)

brendankenny · 2017-03-30T20:13:58Z

lighthouse-plots/analyze.js

@@ -0,0 +1,159 @@
+'use strict';


need license headers on all of these

brendankenny · 2017-03-30T20:16:46Z

lighthouse-plots/measure.js

+  'https://bbc.com'
+];
+
+async function main() {


we're supporting node 6, so no async/await yet, sadly

oh, just saw the node 7 note in the readme. I'd personally prefer if we had one node target for the whole codebase, but others can weigh in

For now the plots code is designed for our use and not targeting the use-cases of LH users. I think we should keep the scope pretty constrained. As such, the requirement of node 7 feels good.

I'll rewrite with promises.

As such, the requirement of node 7 feels good

you just want to sneak in more async/await :P Node LTS will go to 8 in October, which isn't that long from now.

The biggest issue for me (besides consistency) is probably testing...ideally we'll have tests for this code to make sure it doesn't get out of sync with things, and that would require testing the node version in CI to only run plots tests on node 7, etc. Easier to just use promises for now

brendankenny · 2017-03-30T20:25:15Z

lighthouse-plots/measure.js

+
+async function main() {
+  if (utils.isDir(OUT_PATH)) {
+    console.log('Please remove or move ./out/');


I'm not sure if I'd know how to interpret this message if I saw it on the command line :) Maybe just mention results from a previous run were stored there or something to give context about why it's there (it didn't come with the repo) and that it's expected if you've run it before?

brendankenny · 2017-03-30T20:28:15Z

lighthouse-plots/analyze.js

+/**
+ * @typedef {!{site: string, results: !RunResults}}
+ */
+let SiteResults;


can add // eslint-disable-line no-unused-vars to these lines to make eslint happy

brendankenny · 2017-03-30T20:41:22Z

lighthouse-plots/measure.js

+ * @param {string} id
+ * @param {boolean} ignoreRun
+ */
+async function singleRunAnalysis(url, id, ignoreRun) {


this is definitely boolean-trapish (it's not obvious from singleRunAnalysis('https://example.com', 5, false) how that false is going to affect things), though I don't have a great suggestion around it. One option would be to separate analysis from saving and have runAnalysis call both singleRunAnalysis and then with the results make a conditional call to a saveAnalysis function. That keeps isFirstRun logic isolated there.

It is unfortunate you have to recreate so much of bin.ts to do all this yourself

I used an options object parameter so it's more readable, e.g. singleRunAnalysis('https://example.com', 5, {ignoreRun: false})

brendankenny · 2017-03-30T20:47:22Z

lighthouse-plots/config/perf-metrics.json

@@ -0,0 +1,43 @@
+{


can you reuse the existing perf.json in the config directory? Would be nice to reduce work to keep configs updated to latest format (e.g. dropping aggregations soon)

ya seems like you can reuse perf.json

brendankenny · 2017-03-30T20:49:09Z

lighthouse-plots/config/perf-metrics.json

+    "estimated-input-latency",
+    "time-to-interactive"
+  ],
+  "aggregations": [{


do you care at all about aggregations? Seems like you're only looking at audit results. I believe you can just drop the field completely (or at the very least do "aggregations": [])

brendankenny · 2017-03-30T20:50:44Z

lighthouse-plots/analyze.js

+// TODO: annotate
+function main() {
+  const allResults = [];
+  fs.readdirSync(OUT_PATH).map(siteDir => {


s/map/forEach?

brendankenny · 2017-03-30T21:02:02Z

lighthouse-plots/package.json

+  "scripts": {
+    "measure": "node measure.js",
+    "analyze": "node analyze.js"
+  }


maybe add something like clean and have it rimraf out/?

paulirish · 2017-03-30T22:40:23Z

But also plots may not be the best descriptor since it's just as much a test runner and and aggregator. Something to do with analysis or monitoring? plots might work too, though :)

I suggested plots because mostly because of completion. (The subset of the word "analysis" you type before hitting tab. ;) Plus its a single syllable and cute. However I do agree it doesn't describe things very completely.

paulirish · 2017-03-30T22:44:55Z

lighthouse-plots/analyze.js

+  );
+  const audits = data.audits;
+  return {
+    firstMeaningfulPaint: {


I'm thinking it'd be advantageous to reuse the definitions inside of pwmetrics-events.js. They are primarily used for adding the LH metrics back into the trace, but the external pwmetrics module also depends on it.

As a usecase, @patrickhulce is adding some new TTI values now and we'll want to have them both embedded in the trace and summarized here in plots.

I didn't capture score over there, but I'm okay adding it. I'm also OK with ignoring score completely for now.

paulirish · 2017-03-30T22:45:57Z

lighthouse-plots/analyze.js

+const GENERATED_RESULTS_PATH = path.resolve(OUT_PATH, 'generatedResults.js');
+const LIGHTHOUSE_RESULTS_FILENAME = 'lighthouse.json';
+
+const METRICS = [


this might be addressed anyhow, but I didn't see why this was neccessary. Isn't it the same as object.keys on the readResult value?

paulirish · 2017-03-30T22:47:35Z

lighthouse-plots/config/perf-metrics.json

@@ -0,0 +1,43 @@
+{


ya seems like you can reuse perf.json

paulirish · 2017-03-30T22:51:14Z

lighthouse-plots/measure.js

+  'https://bbc.com'
+];
+
+async function main() {


For now the plots code is designed for our use and not targeting the use-cases of LH users. I think we should keep the scope pretty constrained. As such, the requirement of node 7 feels good.

paulirish · 2017-03-30T22:54:11Z

lighthouse-plots/measure.js

+    } catch (err) {
+      await launcher.run();
+    }
+    await runAnalysis();


This is something i'd actually like to A/B test with this framework. (To see how much of a difference it makes). DNS costs will definitely be in relaunched ones.

In this model, it favors quicker collection, but also aligns closer with how people run LH with the extension and in devtools. So I think it's slightly better to debug that scenario first.

paulirish · 2017-03-30T23:01:28Z

lighthouse-plots/measure.js

+ */
+async function singleRunAnalysis(url, id, ignoreRun) {
+  const parsedURL = parseURL(url);
+  const host = `${parsedURL.host}-${parsedURL.pathname}`


can we call this filenameHost or something?

Also I wrote this bit for traceviewer in case you want to reuse:

var illegalRe = /[\/\?<>\\:\*\|":]/g; var controlRe = /[\x00-\x1f\x80-\x9f]/g; var reservedRe = /^\.+$/; return filename .replace(illegalRe, '.') .replace(controlRe, '\u2022') .replace(reservedRe, '') .replace(/\s+/g, '_');

paulirish · 2017-03-30T23:01:55Z

lighthouse-plots/package.json

+  "scripts": {
+    "measure": "node measure.js",
+    "analyze": "node analyze.js"
+  }


wwwillchen

I think I've addressed all of the comments in this PR. Please take another look. Thanks!

@paulirish - I think you mentioned over hangouts there was an onload event you wanted me to add. Can you point me to the trace event?

I noticed there's a weird issue with nvm install on travis (e.g. https://travis-ci.org/GoogleChrome/lighthouse/jobs/218671384#L263)

wwwillchen · 2017-03-31T00:25:31Z

lighthouse-plots/package.json

+  "scripts": {
+    "measure": "node measure.js",
+    "analyze": "node analyze.js"
+  }


wwwillchen · 2017-03-31T00:38:19Z

lighthouse-plots/measure.js

+
+async function main() {
+  if (utils.isDir(OUT_PATH)) {
+    console.log('Please remove or move ./out/');


wwwillchen · 2017-03-31T00:39:13Z

lighthouse-plots/measure.js

+main();
+
+async function runAnalysis() {
+  // Running it n + 1 times because the first run might not be recorded


Updated the wording.

wwwillchen · 2017-03-31T00:47:58Z

lighthouse-plots/measure.js

+ * @param {string} id
+ * @param {boolean} ignoreRun
+ */
+async function singleRunAnalysis(url, id, ignoreRun) {


I used an options object parameter so it's more readable, e.g. singleRunAnalysis('https://example.com', 5, {ignoreRun: false})

wwwillchen · 2017-03-31T01:02:07Z

lighthouse-plots/measure.js

+ */
+async function singleRunAnalysis(url, id, ignoreRun) {
+  const parsedURL = parseURL(url);
+  const host = `${parsedURL.host}-${parsedURL.pathname}`


wwwillchen · 2017-03-31T01:02:23Z

lighthouse-plots/measure.js

+  for (let i = 0; i <= NUMBER_OF_RUNS; i++) {
+    const id = i.toString();
+    // Might not want to record the first run because it has different perf characteristics from
+    // subsequent runs (e.g. DNS cache which can't be easily reset between runs)


wwwillchen · 2017-04-04T22:55:48Z

plots/measure.js

+
+function createConfig() {
+  const config = require('../lighthouse-core/config/perf.json');
+  config.audits = [


feels fragile to hardcode this but don't know of a better way.

I agree, using your own config seemed fine to me. This specific case makes sense to have a metric only config without the cruft of the other gatherers and whatnot slowing things down. @paulirish @brendankenny why use perf.json?

created a trimmed down config here - lighthouse-core/config/plots.json

brendankenny · 2017-04-04T23:35:07Z

I noticed there's a weird issue with nvm install on travis

yeah, I was just looking at that. Looks like there are nvm download failures and it's defaulting to node 4.

brendankenny · 2017-04-04T23:36:36Z

looks like it's nodejs/nodejs.org#1191

patrickhulce

looking good! pulling the metrics to chart from pwmetrics should keep it easy to evaluate our incoming TTIs :D

patrickhulce · 2017-04-05T17:30:17Z

lighthouse-core/lib/traces/pwmetrics-events.js

+ * @param {string} path
+ * @return {*}
+ */
+function safeGet(object, path) {


nit: we usually put these at the top of the file

patrickhulce · 2017-04-05T17:35:58Z

plots/measure.js

+
+const URLS = [
+  // Flagship sites
+  'https://nytimes.com',


might also be worth throwing in some of these

patrickhulce · 2017-04-05T17:39:48Z

plots/measure.js

+
+function createConfig() {
+  const config = require('../lighthouse-core/config/perf.json');
+  config.audits = [


I agree, using your own config seemed fine to me. This specific case makes sense to have a metric only config without the cruft of the other gatherers and whatnot slowing things down. @paulirish @brendankenny why use perf.json?

patrickhulce · 2017-04-05T17:44:54Z

plots/analyze.js

+ * @param {!Array<!SiteResults>} results
+ * @return {!FormattedResults}
+ */
+function formatResults(results) {


nit: maybe rename so it's clearer this basically groups by metric name

paulirish · 2017-04-05T21:46:18Z

plots/chart.js

+      })
+      .reverse();
+
+    const layout = {title: title + ' ' + type};


i saw some plots where the X axis didnt' start at 0. can we make sure it always does?

paulirish · 2017-04-05T21:50:58Z

plots/README.md

+$ npm run analyze
+
+# View visualization
+# Open chart.html in browser


can you rename to index.html for ease of use

paulirish · 2017-04-05T22:17:37Z

plots/chart.js

+      .reverse();
+
+    const layout = {title: title + ' ' + type};
+    Plotly.newPlot(createChartElement(1000), data, layout);


since we flood all these plots at once i see my machine stall for 5s while plotly goes to work.

here's an incremental raf-scheduler for them: d5405e7

wwwillchen

Updated per second round of comments. Please take another look.

wwwillchen · 2017-04-05T23:10:54Z

plots/README.md

+$ npm run analyze
+
+# View visualization
+# Open chart.html in browser


wwwillchen · 2017-04-05T23:11:04Z

lighthouse-core/lib/traces/pwmetrics-events.js

+ * @param {string} path
+ * @return {*}
+ */
+function safeGet(object, path) {


wwwillchen · 2017-04-05T23:13:27Z

plots/analyze.js

+ * @param {!Array<!SiteResults>} results
+ * @return {!FormattedResults}
+ */
+function formatResults(results) {


wwwillchen · 2017-04-05T23:19:54Z

plots/measure.js

+
+const URLS = [
+  // Flagship sites
+  'https://nytimes.com',


paulirish

I'm happy with this. @brendankenny you good?

googlebot · 2017-04-06T02:16:04Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

wwwillchen · 2017-04-06T19:07:58Z

@googlebot it's ok

paulirish · 2017-04-06T19:36:00Z

@googlebot shh bby is ok

brendankenny

a few more things, mostly style :)

brendankenny · 2017-04-06T21:31:54Z

.eslintignore

@@ -21,3 +21,6 @@ lighthouse-cli/types/*.js
 # Handlebar-templates
 lighthouse-core/report/templates/report-templates.js
 lighthouse-core/report/partials/templates/report-partials.js
+
+# Generated files
+plots/out/**


should add this to .npmignore as well. Also, do we want to ship plots with the npm module? Could be useful, but we might want to wait on that, so we could npmignore plots/ entirely for now

Let's npmignore plots entirely for now. Once it's been more thoroughly tested and documented, then it makes sense to ship it with the npm module.

Let's npmignore plots entirely for now

SGTM

brendankenny · 2017-04-06T22:13:40Z

lighthouse-core/lib/traces/pwmetrics-events.js

+ * @param {string} path
+ * @return {*}
+ */
+function safeGet(object, path) {


not wild about this, mostly because it completely disables typing. Is the issue that the intermediate objects are sometimes undefined?

Right - this happens when the audit fails. An alternative is to always to have the intermediate objects be created even when the audit fails.

An alternative is to always to have the intermediate objects be created even when the audit fails.

I think it's reasonable not to have an extendedInfo if the audit failed, but if it didn't fail, all the extendedInfo paths are filled or explicitly null (or whatever) at the leaf properties.

I'm sighing via code review because I'm currently in the process of re-enabling closure compiler for the codebase and this will all have to be undone again :)

I don't want to block plots/ on having to hunt down all the current failure modes though, so we can go with this for now and file an issue about extendedInfo structure guarantees (which having a type check test will help with :)

brendankenny · 2017-04-06T22:26:28Z

plots/index.html

+    <script src="./chart.js"></script>
+</body>
+
+</html>


brendankenny · 2017-04-06T22:26:56Z

plots/analyze.js

+let SiteResults; // eslint-disable-line no-unused-vars
+
+/**
+ * @typedef {Array<{runId: string, metrics: !Array<!Metric>}>}


!Array, I think?

I wasn't sure whether to put nullability operator for typedefs since I do !RunResults above, but I'll add the ! to be consistent w/ lighthouse

I wasn't sure whether to put nullability operator for typedefs since I do !RunResults above, but I'll add the ! to be consistent w/ lighthouse

yeah, my understanding (though I'd have to bring up old threads to be sure) is that without the ! the typedef would be for (null|!Array<...>), and so even if the typedef is used as non-nullable !RunResults, that would be treated as the union of the empty set and the typedef, so still nullable as (null|!Array).

brendankenny · 2017-04-06T22:28:44Z

plots/analyze.js

+let Metric; // eslint-disable-line no-unused-vars
+
+/**
+ * @typedef {Object<string, !Array<{site: string, metrics: !Array<!{timing: string}>}>}


For metrics: !Array<!{timing: string}>}> you don't need the inner !. Also should timing really be a number?

Yep - good catch

brendankenny · 2017-04-06T22:38:13Z

plots/measure.js

+  // (e.g. DNS cache which can't be easily reset between runs)
+  for (let i = 0; i <= NUMBER_OF_RUNS; i++) {
+    // Averages out any order-dependent effects such as memory pressure
+    utils.shuffle(URLS);


brendankenny · 2017-04-06T22:41:42Z

plots/measure.js

+}
+
+/**
+ * @typedef {{ignoreRun: boolean}}


nit: if it's just the one property I'd say just inline in the uses below

brendankenny · 2017-04-06T22:49:13Z

plots/analyze.js

+
+main();
+
+/**


function description doc strings on these (and the other files) would be 👍 😄

brendankenny · 2017-04-06T22:50:12Z

plots/index.html

+See the License for the specific language governing permissions and
+limitations under the License.
+-->
+


two space indent in this file

brendankenny · 2017-04-06T22:51:22Z

plots/index.html

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


we don't have to do this in this PR, but it might be good to move plotly to a devDependency so this works locally

Made a note to do this in a follow-up.

a00ad41cb947ab4322437ee2e1079065c83d6b94 init 8f406f93a9f802b7887db21797c7f468a124d3d6 working f88c01d2113f294ba3a26a42ee0ae2c1a84a3ae3 fixin e1d52f007999f518e24ef110e31dcbb6cc6e133c done 6415b7bf59c3b82e57cff967817e82492c9d1cfe done nit fixes update unit test onLoad event fix pr fb

wwwillchen

@brendankenny please take another look, thanks.

wwwillchen · 2017-04-07T00:13:46Z

.eslintignore

@@ -21,3 +21,6 @@ lighthouse-cli/types/*.js
 # Handlebar-templates
 lighthouse-core/report/templates/report-templates.js
 lighthouse-core/report/partials/templates/report-partials.js
+
+# Generated files
+plots/out/**


Let's npmignore plots entirely for now. Once it's been more thoroughly tested and documented, then it makes sense to ship it with the npm module.

wwwillchen · 2017-04-07T00:19:40Z

lighthouse-core/lib/traces/pwmetrics-events.js

+ * @param {string} path
+ * @return {*}
+ */
+function safeGet(object, path) {


Right - this happens when the audit fails. An alternative is to always to have the intermediate objects be created even when the audit fails.

wwwillchen · 2017-04-07T00:48:26Z

plots/analyze.js

+
+main();
+
+/**


wwwillchen · 2017-04-07T00:50:36Z

plots/analyze.js

+let SiteResults; // eslint-disable-line no-unused-vars
+
+/**
+ * @typedef {Array<{runId: string, metrics: !Array<!Metric>}>}


I wasn't sure whether to put nullability operator for typedefs since I do !RunResults above, but I'll add the ! to be consistent w/ lighthouse

wwwillchen · 2017-04-07T00:53:14Z

plots/analyze.js

+let Metric; // eslint-disable-line no-unused-vars
+
+/**
+ * @typedef {Object<string, !Array<{site: string, metrics: !Array<!{timing: string}>}>}


Yep - good catch

wwwillchen · 2017-04-07T00:54:06Z

plots/index.html

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


Made a note to do this in a follow-up.

wwwillchen · 2017-04-07T00:55:08Z

plots/index.html

+See the License for the specific language governing permissions and
+limitations under the License.
+-->
+


wwwillchen · 2017-04-07T00:59:50Z

plots/measure.js

+
+const NUMBER_OF_RUNS = 20;
+
+const URLS = [


It's about ~20 sec per run * 72 sites * 20 runs = 8 hours. We can create a narrower list (e.g. the most problematic sites) when we generate plots as part of the lighthouse's CI process.

wwwillchen · 2017-04-07T01:00:21Z

plots/measure.js

+    return;
+  }
+
+  const launcher = new ChromeLauncher({port: 9222, autoSelectChrome: true});


wwwillchen · 2017-04-07T01:02:38Z

plots/measure.js

+}
+
+/**
+ * @typedef {{ignoreRun: boolean}}


brendankenny · 2017-04-07T22:38:55Z

.eslintignore

@@ -21,3 +21,6 @@ lighthouse-cli/types/*.js
 # Handlebar-templates
 lighthouse-core/report/templates/report-templates.js
 lighthouse-core/report/partials/templates/report-partials.js
+
+# Generated files
+plots/out/**


Let's npmignore plots entirely for now

SGTM

brendankenny · 2017-04-07T22:44:44Z

lighthouse-core/lib/traces/pwmetrics-events.js

+ * @param {string} path
+ * @return {*}
+ */
+function safeGet(object, path) {


An alternative is to always to have the intermediate objects be created even when the audit fails.

I think it's reasonable not to have an extendedInfo if the audit failed, but if it didn't fail, all the extendedInfo paths are filled or explicitly null (or whatever) at the leaf properties.

I'm sighing via code review because I'm currently in the process of re-enabling closure compiler for the codebase and this will all have to be undone again :)

I don't want to block plots/ on having to hunt down all the current failure modes though, so we can go with this for now and file an issue about extendedInfo structure guarantees (which having a type check test will help with :)

brendankenny · 2017-04-07T22:47:34Z

plots/measure.js

+
+const NUMBER_OF_RUNS = 20;
+
+const URLS = [


It's about ~20 sec per run * 72 sites * 20 runs = 8 hours. We can create a narrower list (e.g. the most problematic sites) when we generate plots as part of the lighthouse's CI process

yeah, I like that. Maybe we can have it be a few sets of URLs, the full run does all of them, and then flags for different subsets (and even a single URL)

brendankenny · 2017-04-07T22:49:37Z

plots/measure.js

+/**
+ * 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


@paulirish also wanted to test the opposite, does not opening a new chrome every time have a significant impact on metrics, since e.g. users don't (usually) open a new instance every time they want to test their site in devtools. Will you file an issue to track this?

brendankenny · 2017-04-07T22:54:39Z

plots/analyze.js

+let SiteResults; // eslint-disable-line no-unused-vars
+
+/**
+ * @typedef {Array<{runId: string, metrics: !Array<!Metric>}>}


I wasn't sure whether to put nullability operator for typedefs since I do !RunResults above, but I'll add the ! to be consistent w/ lighthouse

yeah, my understanding (though I'd have to bring up old threads to be sure) is that without the ! the typedef would be for (null|!Array<...>), and so even if the typedef is used as non-nullable !RunResults, that would be treated as the union of the empty set and the typedef, so still nullable as (null|!Array).

brendankenny

whoops, commented too soon.

LGTM! 💹📉📈📊

do you want to use #1924 as a meta bug for remaining tasks or close that one and open something new? I think main thing is testing effect of launching (or not) a new Chrome for every test.

Also adding tests so we don't break plots/ :)

wwwillchen · 2017-04-07T23:28:33Z

Thanks. I created a meta-issue to track remaining work for plots: #1980

paulirish mentioned this pull request Mar 30, 2017

[meta] I/O Burndown #1937

Closed

52 tasks

paulirish reviewed Mar 30, 2017

View reviewed changes

patrickhulce reviewed Mar 30, 2017

View reviewed changes

brendankenny requested changes Mar 30, 2017

View reviewed changes

wwwillchen changed the title ~~WIP: Plots~~ Measure and visualize perf metrics Mar 30, 2017

paulirish reviewed Mar 30, 2017

View reviewed changes

patrickhulce mentioned this pull request Mar 31, 2017

feat(tti): increase robustness of perf metrics #1947

Merged

brendankenny mentioned this pull request Apr 3, 2017

fix: add more helpful error message when fMP is missing #1959

Merged

wwwillchen force-pushed the plots branch 2 times, most recently from 87057c2 to f81c466 Compare April 4, 2017 22:36

wwwillchen commented Apr 4, 2017

View reviewed changes

patrickhulce reviewed Apr 5, 2017

View reviewed changes

paulirish requested changes Apr 5, 2017

View reviewed changes

wwwillchen commented Apr 6, 2017

View reviewed changes

paulirish approved these changes Apr 6, 2017

View reviewed changes

brendankenny requested changes Apr 6, 2017

View reviewed changes

patrickhulce mentioned this pull request Apr 7, 2017

TTI: Update scoring PODR #787

Closed

wwwillchen and others added 2 commits April 6, 2017 18:07

fix tests.

a14cc3e

wwwillchen force-pushed the plots branch from 8a416e8 to a14cc3e Compare April 7, 2017 01:08

nit

b5bd772

wwwillchen commented Apr 7, 2017

View reviewed changes

brendankenny reviewed Apr 7, 2017

View reviewed changes

brendankenny approved these changes Apr 7, 2017

View reviewed changes

brendankenny merged commit c43eb09 into GoogleChrome:master Apr 7, 2017

wwwillchen mentioned this pull request Apr 7, 2017

[meta] Plots: improve infrastructure for analyzing perf metrics #1980

Closed

2 tasks

Measure and visualize perf metrics #1936

Measure and visualize perf metrics #1936

Conversation

wwwillchen commented Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Mar 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wwwillchen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wwwillchen Apr 5, 2017 • edited Loading

Choose a reason for hiding this comment

brendankenny commented Apr 4, 2017 • edited Loading

brendankenny commented Apr 4, 2017

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wwwillchen commented Mar 30, 2017 •

edited

Loading

wwwillchen Apr 5, 2017 •

edited

Loading

brendankenny commented Apr 4, 2017 •

edited

Loading