From 769aeb7fe3d19ddb5c66add2fa02587a5f1ecf6c Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Sat, 11 Jul 2020 17:45:01 -0700 Subject: [PATCH 1/5] Add milli function to format milliseconds --- src/format.ts | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/format.ts b/src/format.ts index 9bd35346..0bfdf0f6 100644 --- a/src/format.ts +++ b/src/format.ts @@ -276,7 +276,7 @@ const runtimeConfidenceIntervalDimension: Dimension = { alignment: 'right', }, format: (r: ResultStats) => - formatConfidenceInterval(r.stats.meanCI, (n) => n.toFixed(2) + 'ms'), + formatConfidenceInterval(r.stats.meanCI, (n) => milli(n) + 'ms'), }; function formatDifference({absolute, relative}: Difference): string { @@ -284,21 +284,19 @@ function formatDifference({absolute, relative}: Difference): string { if (absolute.low > 0 && relative.low > 0) { word = `[bold red]{slower}`; rel = `${percent(relative.low)}% [gray]{-} ${percent(relative.high)}%`; - abs = - `${absolute.low.toFixed(2)}ms [gray]{-} ${absolute.high.toFixed(2)}ms`; + abs = `${milli(absolute.low)}ms [gray]{-} ${milli(absolute.high)}ms`; } else if (absolute.high < 0 && relative.high < 0) { word = `[bold green]{faster}`; rel = `${percent(-relative.high)}% [gray]{-} ${percent(-relative.low)}%`; - abs = `${- absolute.high.toFixed(2)}ms [gray]{-} ${ - - absolute.low.toFixed(2)}ms`; + abs = `${milli(-absolute.high)}ms [gray]{-} ${milli(-absolute.low)}ms`; } else { word = `[bold blue]{unsure}`; - rel = `${colorizeSign(relative.low, (n) => percent(n))}% [gray]{-} ${ - colorizeSign(relative.high, (n) => percent(n))}%`; - abs = `${colorizeSign(absolute.low, (n) => n.toFixed(2))}ms [gray]{-} ${ - colorizeSign(absolute.high, (n) => n.toFixed(2))}ms`; + rel = `${colorizeSign(relative.low, percent)}% [gray]{-} ${ + colorizeSign(relative.high, percent)}%`; + abs = `${colorizeSign(absolute.low, milli)}ms [gray]{-} ${ + colorizeSign(absolute.high, milli)}ms`; } return ansi.format(`${word}\n${rel}\n${abs}`); } @@ -307,6 +305,10 @@ function percent(n: number): string { return (n * 100).toFixed(0); } +function milli(n: number): string { + return n.toFixed(2); +} + /** * Create a function that will return the shortest unambiguous label for a * result, given the full array of results. From f6eab79e87529160357da599d10f236aed4279e9 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Sat, 11 Jul 2020 17:49:09 -0700 Subject: [PATCH 2/5] Move units into formatting functions --- src/format.ts | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/format.ts b/src/format.ts index 0bfdf0f6..c76879e3 100644 --- a/src/format.ts +++ b/src/format.ts @@ -275,38 +275,38 @@ const runtimeConfidenceIntervalDimension: Dimension = { tableConfig: { alignment: 'right', }, - format: (r: ResultStats) => - formatConfidenceInterval(r.stats.meanCI, (n) => milli(n) + 'ms'), + format: (r: ResultStats) => formatConfidenceInterval(r.stats.meanCI, milli), }; function formatDifference({absolute, relative}: Difference): string { let word, rel, abs; if (absolute.low > 0 && relative.low > 0) { word = `[bold red]{slower}`; - rel = `${percent(relative.low)}% [gray]{-} ${percent(relative.high)}%`; - abs = `${milli(absolute.low)}ms [gray]{-} ${milli(absolute.high)}ms`; + rel = `${percent(relative.low)} [gray]{-} ${percent(relative.high)}`; + abs = `${milli(absolute.low)} [gray]{-} ${milli(absolute.high)}`; } else if (absolute.high < 0 && relative.high < 0) { word = `[bold green]{faster}`; - rel = `${percent(-relative.high)}% [gray]{-} ${percent(-relative.low)}%`; - abs = `${milli(-absolute.high)}ms [gray]{-} ${milli(-absolute.low)}ms`; + rel = `${percent(-relative.high)} [gray]{-} ${percent(-relative.low)}`; + abs = `${milli(-absolute.high)} [gray]{-} ${milli(-absolute.low)}`; } else { word = `[bold blue]{unsure}`; - rel = `${colorizeSign(relative.low, percent)}% [gray]{-} ${ - colorizeSign(relative.high, percent)}%`; - abs = `${colorizeSign(absolute.low, milli)}ms [gray]{-} ${ - colorizeSign(absolute.high, milli)}ms`; + rel = `${colorizeSign(relative.low, percent)} [gray]{-} ${ + colorizeSign(relative.high, percent)}`; + abs = `${colorizeSign(absolute.low, milli)} [gray]{-} ${ + colorizeSign(absolute.high, milli)}`; } + return ansi.format(`${word}\n${rel}\n${abs}`); } function percent(n: number): string { - return (n * 100).toFixed(0); + return (n * 100).toFixed(0) + '%'; } function milli(n: number): string { - return n.toFixed(2); + return n.toFixed(2) + 'ms'; } /** From b5c95a1d70c91bdb7165bd3cb691aac20075aea7 Mon Sep 17 00:00:00 2001 From: Andre Wiggins Date: Sat, 11 Jul 2020 18:01:27 -0700 Subject: [PATCH 3/5] Use formatConfidenceInterval in formatDifference --- src/format.ts | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/src/format.ts b/src/format.ts index c76879e3..e0d266f4 100644 --- a/src/format.ts +++ b/src/format.ts @@ -282,20 +282,18 @@ function formatDifference({absolute, relative}: Difference): string { let word, rel, abs; if (absolute.low > 0 && relative.low > 0) { word = `[bold red]{slower}`; - rel = `${percent(relative.low)} [gray]{-} ${percent(relative.high)}`; - abs = `${milli(absolute.low)} [gray]{-} ${milli(absolute.high)}`; + rel = formatConfidenceInterval(relative, percent); + abs = formatConfidenceInterval(absolute, milli); } else if (absolute.high < 0 && relative.high < 0) { word = `[bold green]{faster}`; - rel = `${percent(-relative.high)} [gray]{-} ${percent(-relative.low)}`; - abs = `${milli(-absolute.high)} [gray]{-} ${milli(-absolute.low)}`; + rel = formatConfidenceInterval(negate(relative), percent); + abs = formatConfidenceInterval(negate(absolute), milli); } else { word = `[bold blue]{unsure}`; - rel = `${colorizeSign(relative.low, percent)} [gray]{-} ${ - colorizeSign(relative.high, percent)}`; - abs = `${colorizeSign(absolute.low, milli)} [gray]{-} ${ - colorizeSign(absolute.high, milli)}`; + rel = formatConfidenceInterval(relative, (n) => colorizeSign(n, percent)); + abs = formatConfidenceInterval(absolute, (n) => colorizeSign(n, milli)); } return ansi.format(`${word}\n${rel}\n${abs}`); @@ -309,6 +307,13 @@ function milli(n: number): string { return n.toFixed(2) + 'ms'; } +function negate(ci: ConfidenceInterval): ConfidenceInterval { + return { + low: -ci.high, + high: -ci.low, + }; +} + /** * Create a function that will return the shortest unambiguous label for a * result, given the full array of results. From f788b36c4c533b585b80faa9a5f4ccc211e511ff Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Thu, 23 Jul 2020 12:10:29 -0700 Subject: [PATCH 4/5] Respond to comments from PR #170 --- README.md | 6 ++++-- src/configfile.ts | 18 ++++++++++++++++-- src/measure.ts | 14 ++++++++++++-- src/test/configfile_test.ts | 3 ++- src/util.ts | 10 ---------- 5 files changed, 34 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index b76999ac..92d11497 100644 --- a/README.md +++ b/README.md @@ -118,9 +118,11 @@ And in your config file: The following performance entry types are supported: - [`measure`](https://developer.mozilla.org/en-US/docs/Web/API/PerformanceMeasure): - Retrieve the `duration` of a user-defined interval between two marks. + Retrieve the `duration` of a user-defined interval between two marks. Use for + measuring the timing of a specific chunk of your code. - [`mark`](https://developer.mozilla.org/en-US/docs/Web/API/User_Timing_API/Using_the_User_Timing_API#Performance_measures): - Retrieve the `startTime` of a user-defined instant. + Retrieve the `startTime` of a user-defined instant. Use for measuring the time + between initial page navigation and a specific point in your code. - [`paint`](https://developer.mozilla.org/en-US/docs/Web/API/PerformancePaintTiming): Retrieve the `startTime` of a built-in paint measurement (e.g. `first-contentful-paint`). diff --git a/src/configfile.ts b/src/configfile.ts index a517999f..b0410211 100644 --- a/src/configfile.ts +++ b/src/configfile.ts @@ -15,7 +15,7 @@ import * as jsonschema from 'jsonschema'; import {BrowserConfig, BrowserName, parseBrowserConfigString, validateBrowserConfig} from './browser'; import {Config, parseHorizons, urlFromLocalPath} from './config'; import * as defaults from './defaults'; -import {BenchmarkSpec, Measurement, PackageDependencyMap} from './types'; +import {BenchmarkSpec, Measurement, measurements, PackageDependencyMap} from './types'; import {isHttpUrl} from './util'; /** @@ -276,7 +276,8 @@ export async function parseConfigFile(parsedJson: unknown): const result = jsonschema.validate(parsedJson, schema, {propertyName: 'config'}); if (result.errors.length > 0) { - throw new Error(result.errors[0].toString()); + throw new Error( + [...new Set(result.errors.map(customizeJsonSchemaError))].join('\n')); } const validated = parsedJson as ConfigFile; const root = validated.root || '.'; @@ -299,6 +300,19 @@ export async function parseConfigFile(parsedJson: unknown): }; } +/** + * Some of the automatically generated jsonschema errors are unclear, e.g. when + * there is a union of complex types they are reported as "[schema1], + * [schema2]" etc. + */ +function customizeJsonSchemaError(error: jsonschema.ValidationError): string { + if (error.property.match(/^config\.benchmarks\[\d+\]\.measurement$/)) { + return `${error.property} is not one of: ${[...measurements].join(', ')}` + + ' or an object like `performanceEntry: string`'; + } + return error.toString(); +} + async function parseBenchmark(benchmark: ConfigFileBenchmark, root: string): Promise> { const spec: Partial = {}; diff --git a/src/measure.ts b/src/measure.ts index 9dbac6e3..b2b284a7 100644 --- a/src/measure.ts +++ b/src/measure.ts @@ -13,7 +13,7 @@ import * as webdriver from 'selenium-webdriver'; import {Server} from './server'; import {BenchmarkSpec, PerformanceEntryCriteria} from './types'; -import {escapeStringLiteral, throwUnreachable} from './util'; +import {throwUnreachable} from './util'; /** * Try to take a measurement in milliseconds from the given browser. Returns @@ -115,4 +115,14 @@ async function queryForExpression( } return result; } -} \ No newline at end of file +} + +/** + * Escape a string such that it can be safely embedded in a JavaScript template + * literal (backtick string). + */ +function escapeStringLiteral(unescaped: string): string { + return unescaped.replace(/\\/g, '\\\\') + .replace(/`/g, '\\`') + .replace(/\$/g, '\\$'); +} diff --git a/src/test/configfile_test.ts b/src/test/configfile_test.ts index 32da65bb..bd481af4 100644 --- a/src/test/configfile_test.ts +++ b/src/test/configfile_test.ts @@ -413,7 +413,8 @@ suite('config', () => { }], }; await assert.isRejected( - parseConfigFile(config), 'config.benchmarks[0].measurement'); + parseConfigFile(config), + 'config.benchmarks[0].measurement is not one of: callback, fcp'); }); test('sampleSize too small', async () => { diff --git a/src/util.ts b/src/util.ts index 53b37bb2..5925e640 100644 --- a/src/util.ts +++ b/src/util.ts @@ -49,16 +49,6 @@ export async function runNpm( return promisify(execFile)(npmCmd, args, options).then(({stdout}) => stdout); } -/** - * Escape a string such that it can be safely embedded in a JavaScript template - * literal (backtick string). - */ -export function escapeStringLiteral(unescaped: string): string { - return unescaped.replace(/\\/g, '\\\\') - .replace(/`/g, '\\`') - .replace(/\$/g, '\\$'); -} - /** * Promisified version of setTimeout. */ From 322cacd89704d560b406e0e05596f803ead9ca6e Mon Sep 17 00:00:00 2001 From: Alexander Marks Date: Thu, 23 Jul 2020 12:52:48 -0700 Subject: [PATCH 5/5] Respond to comments from PR #172 --- src/cli.ts | 4 +- src/{automatic.ts => runner.ts} | 79 +++++++++++++++++---------------- 2 files changed, 42 insertions(+), 41 deletions(-) rename src/{automatic.ts => runner.ts} (86%) diff --git a/src/cli.ts b/src/cli.ts index 12c256f0..ad4e7f7a 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -24,7 +24,7 @@ import {Server} from './server'; import {ResultStatsWithDifferences} from './stats'; import {prepareVersionDirectory, makeServerPlans} from './versions'; import {manualMode} from './manual'; -import {AutomaticMode} from './automatic'; +import {Runner} from './runner'; import {runNpm} from './util'; const installedVersion = (): string => @@ -150,7 +150,7 @@ $ tach http://example.com await manualMode(config, servers); } else { - const runner = new AutomaticMode(config, servers); + const runner = new Runner(config, servers); try { return await runner.run(); } finally { diff --git a/src/automatic.ts b/src/runner.ts similarity index 86% rename from src/automatic.ts rename to src/runner.ts index ac8eaed4..a22f2b95 100644 --- a/src/automatic.ts +++ b/src/runner.ts @@ -34,14 +34,14 @@ interface Browser { initialTabHandle: string; } -export class AutomaticMode { - private config: Config; - private specs: BenchmarkSpec[]; - private servers: Map; - private browsers = new Map(); - private bar: ProgressBar; +export class Runner { + private readonly config: Config; + private readonly specs: BenchmarkSpec[]; + private readonly servers: Map; + private readonly browsers = new Map(); + private readonly bar: ProgressBar; + private readonly specResults = new Map(); private completeGithubCheck?: (markdown: string) => void; - private specResults = new Map(); private hitTimeout = false; constructor(config: Config, servers: Map) { @@ -140,38 +140,39 @@ export class AutomaticMode { private async takeAdditionalSamples() { const {config, specs, specResults} = this; - if (config.timeout > 0) { - console.log(); - const timeoutMs = config.timeout * 60 * 1000; // minutes -> millis - const startMs = Date.now(); - let run = 0; - let sample = 0; - let elapsed = 0; - while (true) { - if (horizonsResolved(this.makeResults(), config.horizons)) { - console.log(); - break; - } - if (elapsed >= timeoutMs) { - this.hitTimeout = true; - break; - } - // Run batches of 10 additional samples at a time for more presentable - // sample sizes, and to nudge sample sizes up a little. - for (let i = 0; i < 10; i++) { - sample++; - for (const spec of specs) { - run++; - elapsed = Date.now() - startMs; - const remainingSecs = - Math.max(0, Math.round((timeoutMs - elapsed) / 1000)); - const mins = Math.floor(remainingSecs / 60); - const secs = remainingSecs % 60; - process.stdout.write( - `\r${spinner[run % spinner.length]} Auto-sample ${sample} ` + - `(timeout in ${mins}m${secs}s)` + ansi.erase.inLine(0)); - specResults.get(spec)!.push(await this.takeSample(spec)); - } + if (config.timeout <= 0) { + return; + } + console.log(); + const timeoutMs = config.timeout * 60 * 1000; // minutes -> millis + const startMs = Date.now(); + let run = 0; + let sample = 0; + let elapsed = 0; + while (true) { + if (horizonsResolved(this.makeResults(), config.horizons)) { + console.log(); + break; + } + if (elapsed >= timeoutMs) { + this.hitTimeout = true; + break; + } + // Run batches of 10 additional samples at a time for more presentable + // sample sizes, and to nudge sample sizes up a little. + for (let i = 0; i < 10; i++) { + sample++; + for (const spec of specs) { + run++; + elapsed = Date.now() - startMs; + const remainingSecs = + Math.max(0, Math.round((timeoutMs - elapsed) / 1000)); + const mins = Math.floor(remainingSecs / 60); + const secs = remainingSecs % 60; + process.stdout.write( + `\r${spinner[run % spinner.length]} Auto-sample ${sample} ` + + `(timeout in ${mins}m${secs}s)` + ansi.erase.inLine(0)); + specResults.get(spec)!.push(await this.takeSample(spec)); } } }