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

Feature request: benchmark runner progress indicator #8659

Closed
mscdex opened this issue Sep 20, 2016 · 2 comments
Closed

Feature request: benchmark runner progress indicator #8659

mscdex opened this issue Sep 20, 2016 · 2 comments
Labels
benchmark Issues and PRs related to the benchmark subsystem. feature request Issues that request new features to be added to Node.js.

Comments

@mscdex
Copy link
Contributor

mscdex commented Sep 20, 2016

  • Version: n/a
  • Platform: n/a
  • Subsystem: benchmark

Since the benchmark runner defaults to 30 runs and some benchmarks can take awhile to run, it would be nice to have a progress indicator on stderr when the CSV data is being written to non-TTY (e.g. piped to R) on stdout (similar to how cURL displays download progress).

Also, changing the benchmark running logic to instead alternate the benchmark parameter combinations between the new and old executables will allow for faster displaying of results. This may require changes to the R script if it buffers its results until data for all benchmark parameter combinations are received.

@mscdex mscdex added benchmark Issues and PRs related to the benchmark subsystem. feature request Issues that request new features to be added to Node.js. labels Sep 20, 2016
@AndreasMadsen
Copy link
Member

Also, changing the benchmark running logic to instead alternate the benchmark parameter combinations between the new and old executables will allow for faster displaying of results.

Not sure I understand, do you want to change this:

const queue = [];
for (let iter = 0; iter < runs; iter++) {
  for (const filename of benchmarks) {
    for (const binary of binaries) {
      queue.push({ binary, filename, iter });
    }
  }
}

to this:

const queue = [];
for (const filename of benchmarks) {
  for (let iter = 0; iter < runs; iter++) {
    for (const binary of binaries) {
      queue.push({ binary, filename, iter });
    }
  }
}

? That should be fine, I was thinking about that too.

his may require changes to the R script if it buffers its results until data for all benchmark parameter combinations are received.

The R script definitely buffers the result. I have no idea how to make it a stream processor. Also I strongly recommend always saving the results, so at the very least you should run:

node ./benchmark/compare.js --new ./node-pr --old ./node-master -- buffer \
  | tee results.csv \
  | Rscript ./benchmark/compare.R

@mscdex
Copy link
Contributor Author

mscdex commented Sep 28, 2016

Regarding the benchmark execution order, yes, either the second code block or a variation of the second block where the two inner for-loops are swapped, doesn't matter really.

@AndreasMadsen AndreasMadsen mentioned this issue Oct 12, 2016
2 tasks
jasnell pushed a commit that referenced this issue Oct 17, 2016
This changes the execution order from "iter, file, binary" to "file,
iter, binary". This means the csv no longer has to buffered completely.

This also has the added effect that stopping compare.js early or
interfering with performance only affects a single benchmark, instead of
all of them.

Refs: #8659
PR-URL: #9064
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
jasnell pushed a commit that referenced this issue Oct 17, 2016
This changes the execution order from "iter, file, binary" to "file,
iter, binary". This means the csv no longer has to buffered completely.

This also has the added effect that stopping compare.js early or
interfering with performance only affects a single benchmark, instead of
all of them.

Refs: #8659
PR-URL: #9064
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
evanlucas pushed a commit that referenced this issue 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 <amwebdk@gmail.com>
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. feature request Issues that request new features to be added to Node.js.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants