benchmark: add progress indicator to compare.js #10823

joyeecheung · 2017-01-15T19:09:52Z

Print statistics about scheduled runs, completed runs,
configurations, etc. to stderr.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

benchmark

Fixes: #8659

Didn't give too much thought about the format so any ideas would be definitely welcome!

Demo:

cc @AndreasMadsen @mscdex

AndreasMadsen

I don't have an opinion. I just use node ./compare.js | tee file.csv and I know the benchmark I'm running fairly well. But I think you can do better in terms of code readability.

AndreasMadsen · 2017-01-15T19:50:00Z

benchmark/compare.js

-                `${data.rate}, ${data.time}`);
+      console.log(`"${job.binary}", "${job.filename}", "${conf}", ` +
+                  `${data.rate}, ${data.time}`);
+    } else if (showProgress && data.type === 'config') {


I find this code rather hard to read, I don't understand it so it is hard to be constructive, but maybe:

reduce the amount of global variables.

explain lines, runLines and allLines.

wrap complexity in a function.

mscdex · 2017-01-15T20:18:36Z

When I made the original suggestion I more or less had in mind something like node's make test progress indicator: something that would show how many completed benchmarks out of how many total benchmarks (not just runs) and also time elapsed. When new information is written out, the same line is used to reduce screen clutter.

Example:

[00:00:02|%   6| 1/32]

I'm not sure what, if any additional information placed after the end of the above indicator like the make test progress indicator has would be worth adding. Otherwise something like the above would be fine at the very least.

joyeecheung · 2017-01-17T17:15:54Z

@mscdex @AndreasMadsen I've updated the code to wrap the progress-related logic into a separate class and print a self-overwriting progress bar. PTAL.

New demo:

mscdex · 2017-01-17T17:47:16Z

benchmark/compare.js

+  --runs     30                 number of samples
+  --filter   pattern            string to filter benchmark scripts
+  --set      variable=value     set benchmark variable (can be repeated)
+  --progress true|false         show benchmark progress indicator


What if we just make it so that it can be enabled with just --progress? We may need to specially check for it in the arg parsing since it doesn't have a value associated with it, I'm not sure.

The arg parser has some support, it defaults to true when no value is passed. However it will always expect the next argument to be the value, even if it starts with --. For example --process --runs 20 will not work. The feature is not used anywhere, it is just an implementation detail.

mscdex · 2017-01-17T17:48:18Z

Looks better, but I'm thinking we will want to add some kind of labels if we're going to add more fields as it's not immediately obvious what those values represent.

AndreasMadsen

I do prefer this, but I still don't have much of an opinion on it.

I left a few comments:

AndreasMadsen · 2017-01-17T17:44:03Z

benchmark/compare.js

+const kStartOfQueue = 0;
+
+const showProgress = cli.optional.progress === 'true';
+var progress;


AndreasMadsen · 2017-01-17T17:48:45Z

benchmark/_benchmark_progress.js

+function pad(input, minLength, fill) {
+  var result = input + '';
+  while (result.length < minLength) {
+    result = fill + result;


Maybe use return result + fill.repeat(Math.max(0, minLength - result.length)).

AndreasMadsen · 2017-01-17T17:53:30Z

benchmark/_benchmark_progress.js

+  startQueue(index) {
+    this.kStartOfQueue = index;
+    this.currentFile = this.queue[index].filename;
+    this.interval = setInterval(() => {


Why is this interval required? It seams to me that all state updating functions calls updateProgress.

Presumably to at least regularly update the elapsed time.

Ah, of cause!

Yes, see the comments of interval in the constructor.

AndreasMadsen · 2017-01-17T17:58:44Z

benchmark/common.js

@@ -128,6 +128,13 @@ Benchmark.prototype.http = function(options, cb) {

 Benchmark.prototype._run = function() {
  const self = this;
+  if (process.env.hasOwnProperty('NODE_RUN_BENCHMARK_PROGRESS')) {


suggestion: we could just always send this information when possible, e.g. replace with if (process.send). Conditionals that require a larger context knowledge are often hard to understand.

AndreasMadsen · 2017-01-17T18:08:07Z

benchmark/_benchmark_progress.js

+    const percent = pad(Math.floor(completedRate * 100), 3, ' ');
+    const caption = finished ? 'Done\n' : this.currentFile;
+    return `[${getTime(diff)}|% ${percent}` +
+          `| ${fraction(completedRuns, scheduledRuns)}` +


I think it would make more sense if this was fraction(completedFiles, this.benchmarks.length).

joyeecheung · 2017-01-23T15:34:01Z

@AndreasMadsen @mscdex I've added some labels and changed the parser to accept --progress. PTAL.

New demo:

joyeecheung · 2017-01-23T15:37:56Z

Also putting --progress in the middle runs just fine(there aren't any tests for benchmark tools so I'll just post the pic here)

AndreasMadsen · 2017-01-23T16:07:37Z

benchmark/common.js

@@ -128,6 +128,11 @@ Benchmark.prototype.http = function(options, cb) {

 Benchmark.prototype._run = function() {
  const self = this;
+  sendResult({


You should check that this works when running benchmarks individually (for example node benchmark/url/url-format.js), I don't think it will.

Indeed & Fixed. Thanks for catching that!

AndreasMadsen

LGTM

AndreasMadsen · 2017-01-23T18:47:08Z

benchmark/compare.js

@@ -3,6 +3,7 @@
 const fork = require('child_process').fork;
 const path = require('path');
 const CLI = require('./_cli.js');
+const BenchmarkProgress = require('./_benchmark_progress');


nit: I personally prefer to be explicit with .js, but I don't think we have a rule.

mscdex · 2017-01-23T21:14:18Z

Does anyone have any objections to just making --progress the default when stderr is a tty? We'd need to add a --no-progress.

AndreasMadsen · 2017-01-23T21:25:22Z

On second thought, how does this work when there is an error in one of the benchmark scripts, or something else that also prints to stderr?

jasnell · 2017-01-24T02:34:49Z

No objections for it being the default

mscdex · 2017-01-24T05:17:51Z

@AndreasMadsen If we have benchmarks printing to stderr, I'd consider that a bug that should be fixed...

AndreasMadsen · 2017-01-24T06:58:25Z

@mscdex of cause, but I would like to be able to read the error such that I can fix it.

mscdex · 2017-01-24T08:44:36Z

@AndreasMadsen That typically won't be an issue since exceptions and the like will end on a newline, so nothing should be erased.

joyeecheung · 2017-01-24T12:24:55Z

@AndreasMadsen There are logic handling child processes exiting with non-zero exit code(both in common.js and compare.js) so the whole thing will be stopped when there are uncaught exceptions in a benchmark script.

If the error happens when the benchmark is parsed(syntax error), or somewhere in main but before bench.end():

If the error happens after bench.end():

joyeecheung · 2017-01-24T12:37:31Z

@mscdex Don't have an opinion about making it a default (I always type a bunch of arguments when running compare.js so one more --progress doesn't really matter :P) If we make it a default then we need to notify collaborators when this lands, because people would see weird stuff coming out if they don't redirect stdout...

mscdex · 2017-01-24T17:43:47Z

@joyeecheung I'm just viewing it in the same light as make test: having no information doesn't seem particularly beneficial, especially for long running benchmarks (e.g. http). We're already checking if stderr is a tty anyway so if they redirect stderr to a file it shouldn't show any progress text.

AndreasMadsen · 2017-01-24T19:29:18Z

If we make it a default then we need to notify collaborators when this lands, because people would see weird stuff coming out if they don't redirect stdout...

We can show --progress if (process.stderr.isTTY && !process.stdout.isTTY)

mscdex · 2017-01-24T19:38:00Z

@AndreasMadsen When would that ever be the case?

AndreasMadsen · 2017-01-24T20:14:52Z

@mscdex Would that not be the case if you run node compare.js ... > results.csv?

mscdex · 2017-01-24T23:06:41Z

@AndreasMadsen Ah, I see what you're saying. Yes, implicitly enabling --progress (in the absence of --no-progress) in that scenario would be ok.

joyeecheung · 2017-01-26T13:45:31Z

@AndreasMadsen Great idea! Updated to print progress by default when stderr is TTY and stdout is not. PTAL.

CI: https://ci.nodejs.org/job/node-test-pull-request/6069/

joyeecheung · 2017-01-28T15:54:16Z

Plan to land this if there is nothing else to be addressed.

joyeecheung · 2017-01-29T15:49:08Z

@mscdex Can I have a LGTM?

* Print the progress bar and the current benchmark to stderr when stderr is TTY and stdout is not. * Allow cli arguments without values via setting.boolArgs * Add `--no-progress` option

joyeecheung · 2017-01-30T18:27:02Z

Squashed. One more CI before landing: https://ci.nodejs.org/job/node-test-pull-request/6112/

joyeecheung · 2017-01-30T19:04:26Z

CI failure is unrelated. Landed in 60d77bd.

* Print the progress bar and the current benchmark to stderr when stderr is TTY and stdout is not. * Allow cli arguments without values via setting.boolArgs * Add --no-progress option PR-URL: #10823 Fixes: #8659 Reviewed-By: Andreas Madsen <[email protected]>

jasnell · 2017-03-07T22:38:13Z

Would require a backport PR to land on either v6 or v4 (if at all)

nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Jan 15, 2017

AndreasMadsen reviewed Jan 15, 2017

View reviewed changes

joyeecheung force-pushed the bench-progress branch 3 times, most recently from b8b0acc to a5519f1 Compare January 17, 2017 16:54

mscdex reviewed Jan 17, 2017

View reviewed changes

AndreasMadsen reviewed Jan 17, 2017

View reviewed changes

joyeecheung force-pushed the bench-progress branch from a5519f1 to ebd4922 Compare January 23, 2017 15:13

AndreasMadsen reviewed Jan 23, 2017

View reviewed changes

AndreasMadsen approved these changes Jan 23, 2017

View reviewed changes

joyeecheung force-pushed the bench-progress branch from 38f46dc to a5e5a4c Compare January 26, 2017 13:42

benchmark: add progress indicator to compare.js

e27fb9f

* Print the progress bar and the current benchmark to stderr when stderr is TTY and stdout is not. * Allow cli arguments without values via setting.boolArgs * Add `--no-progress` option

joyeecheung force-pushed the bench-progress branch from a5e5a4c to e27fb9f Compare January 30, 2017 18:26

joyeecheung closed this Jan 30, 2017

italoacasas mentioned this pull request Jan 31, 2017

v7.5.0 proposal #11062

Merged

joyeecheung deleted the bench-progress branch February 19, 2017 17:42

jasnell added dont-land-on-v4.x labels Mar 7, 2017

benchmark: add progress indicator to compare.js #10823

benchmark: add progress indicator to compare.js #10823

Conversation

joyeecheung commented Jan 15, 2017 • edited Loading

Checklist

Affected core subsystem(s)

AndreasMadsen left a comment • edited Loading

Choose a reason for hiding this comment

AndreasMadsen Jan 15, 2017 • edited Loading

Choose a reason for hiding this comment

mscdex commented Jan 15, 2017 • edited Loading

joyeecheung commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Jan 17, 2017

AndreasMadsen 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

joyeecheung commented Jan 23, 2017

joyeecheung commented Jan 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndreasMadsen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscdex commented Jan 23, 2017 • edited Loading

AndreasMadsen commented Jan 23, 2017 • edited Loading

jasnell commented Jan 24, 2017

mscdex commented Jan 24, 2017 • edited Loading

AndreasMadsen commented Jan 24, 2017

mscdex commented Jan 24, 2017

joyeecheung commented Jan 24, 2017

joyeecheung commented Jan 24, 2017

mscdex commented Jan 24, 2017 • edited Loading

AndreasMadsen commented Jan 24, 2017

mscdex commented Jan 24, 2017

AndreasMadsen commented Jan 24, 2017

mscdex commented Jan 24, 2017

joyeecheung commented Jan 26, 2017

joyeecheung commented Jan 28, 2017

joyeecheung commented Jan 29, 2017

joyeecheung commented Jan 30, 2017

joyeecheung commented Jan 30, 2017

jasnell commented Mar 7, 2017

joyeecheung commented Jan 15, 2017 •

edited

Loading

AndreasMadsen left a comment •

edited

Loading

AndreasMadsen Jan 15, 2017 •

edited

Loading

mscdex commented Jan 15, 2017 •

edited

Loading

mscdex commented Jan 23, 2017 •

edited

Loading

AndreasMadsen commented Jan 23, 2017 •

edited

Loading

mscdex commented Jan 24, 2017 •

edited

Loading

mscdex commented Jan 24, 2017 •

edited

Loading