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

benchmark: add progress indicator to compare.js #10823

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 15, 2017

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:

out

cc @AndreasMadsen @mscdex

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

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

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.

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

@AndreasMadsen AndreasMadsen Jan 15, 2017

Choose a reason for hiding this comment

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

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
Copy link
Contributor

mscdex commented Jan 15, 2017

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 joyeecheung force-pushed the bench-progress branch 3 times, most recently from b8b0acc to a5519f1 Compare January 17, 2017 16:54
@joyeecheung
Copy link
Member Author

@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:

out

--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
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Contributor

mscdex commented Jan 17, 2017

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.

Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

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

I left a few comments:

const kStartOfQueue = 0;

const showProgress = cli.optional.progress === 'true';
var progress;
Copy link
Member

Choose a reason for hiding this comment

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

let?

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

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Presumably to at least regularly update the elapsed time.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, of cause!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, see the comments of interval in the constructor.

@@ -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')) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

@joyeecheung
Copy link
Member Author

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

New demo:
out

@joyeecheung
Copy link
Member Author

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)

screen shot 2017-01-23 at 11 37 01 pm

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

Benchmark.prototype._run = function() {
const self = this;
sendResult({
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed & Fixed. Thanks for catching that!

Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

Choose a reason for hiding this comment

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

LGTM

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

Choose a reason for hiding this comment

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

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

@mscdex
Copy link
Contributor

mscdex commented Jan 23, 2017

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
Copy link
Member

AndreasMadsen commented Jan 23, 2017

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
Copy link
Member

jasnell commented Jan 24, 2017

No objections for it being the default

@mscdex
Copy link
Contributor

mscdex commented Jan 24, 2017

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

@AndreasMadsen
Copy link
Member

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

@mscdex
Copy link
Contributor

mscdex commented Jan 24, 2017

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

@joyeecheung
Copy link
Member Author

@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():

screen shot 2017-01-24 at 8 12 22 pm

If the error happens after bench.end():

screen shot 2017-01-24 at 8 22 01 pm

@joyeecheung
Copy link
Member Author

@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
Copy link
Contributor

mscdex commented Jan 24, 2017

@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
Copy link
Member

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
Copy link
Contributor

mscdex commented Jan 24, 2017

@AndreasMadsen When would that ever be the case?

@AndreasMadsen
Copy link
Member

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

@mscdex
Copy link
Contributor

mscdex commented Jan 24, 2017

@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
Copy link
Member Author

@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
Copy link
Member Author

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

@joyeecheung
Copy link
Member Author

@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
Copy link
Member Author

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

@joyeecheung
Copy link
Member Author

CI failure is unrelated. Landed in 60d77bd.

joyeecheung added a commit that referenced this pull request Jan 30, 2017
* 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]>
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
* 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]>
@italoacasas italoacasas mentioned this pull request Jan 31, 2017
@joyeecheung joyeecheung deleted the bench-progress branch February 19, 2017 17:42
@jasnell
Copy link
Member

jasnell commented Mar 7, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: benchmark runner progress indicator
5 participants