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.

Sign up for GitHub

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

Jump to bottom

benchmark: add csv and plotting script #777

Closed
wants to merge 3 commits into from
Closed

benchmark: add csv and plotting script #777

wants to merge 3 commits into from

Conversation

brendanashworth
Copy link
Contributor

This pull request consists of two commits:

dda466b: Adds an environment variable OUTPUT_FORMAT to be parsed when a benchmark is ran, that now allows for CSV output when it is set to csv. It will default back to standard behavior when it is not set, or when it is set to default. Best for shell redirection into a .csv file.

For each commit, longer and better explanations are in each commit description.

394735d: Adds a script for graphing CSV output of the benchmarks that allows for easy X coordinates and grouping. Here is the given example.

The reasoning behind the new graphing script is that it makes it easier to visualize where the improvements can be made when optimizing, allows for more visual speed comparisons between commits, and that the already-present plot.R script hasn't been touched for a long time and doesn't do the same functionality.

@brendanashworth
benchmark: add output format option [csv]
dda466b 
This commit adds an `OUTPUT_FORMAT` environment variable option for
all benchmark tests that allow either 'csv' or 'default' output. Default
output has been left unchanged, and csv output prints out the csv
headers along with the csv formatted per-test output, each test also
seperated by a newline.

It can be used like the following:
$ OUTPUT_FORMAT=csv iojs benchmark/common.js http

Not specifying the OUTPUT_FORMAT env var will default to 'default'.
Specifying a bad value will throw an error.
@brendanashworth
Copy link
Contributor Author

If anyone wants more examples, here is one for http/end-vs-write-end.js and one for buffers/buffer-write.js.

@KyleAMathews
Copy link

I'd love to see performance graphs made public ala http://arewefastyet.com/ and its siblings.

bnoordhuis
bnoordhuis reviewed Feb 11, 2015
View reviewed changes
benchmark/common.js
console.log('%s: %s', heading, value.toFixed(5));
else if (outputFormat == 'csv')
console.log('%s,%s', heading, value.toFixed(5));
}
Copy link
Member

Choose a reason for hiding this comment

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

Does the combination silent === true && outputFormat === 'csv' make sense? If not, you could write this as:

if (!silent && outputFormat == 'default')
  // ...
else if (outputFormat == 'csv')
  // ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks - I ended up taking your suggestion about the silent mode though, so this'll be changed anyways in the upcoming commit

@bnoordhuis
Copy link
Member

Suggestion: replace NODE_BENCH_SILENT with OUTPUT_FORMAT=silent.

@bnoordhuis
Copy link
Member

LGTM with some suggestions and one style nit.

@brendanashworth
benchmark: add output format option [csv]
21ae52c 
This commit adds an `OUTPUT_FORMAT` environment variable option for
all benchmark tests that allow either 'csv' or 'default' output. Default
output has been left unchanged, and csv output prints out the csv
headers along with the csv formatted per-test output, each test also
seperated by a newline.

It can be used like the following:
$ OUTPUT_FORMAT=csv iojs benchmark/common.js http

Not specifying the OUTPUT_FORMAT env var will default to 'default'.
Specifying a bad value will throw an error.
@brendanashworth
Copy link
Contributor Author

Made the modifications and took your suggestion, @bnoordhuis. I'd like at least one more person to look at the R code though before this is merged since I'm not an R wizard.

@brendanashworth
benchmark: add plot_csv R graphing script
6389c58 
This commit adds a graphing script (in R) for graphing the CSV output
of a benchmark. It can be run like this:

```
$ OUTPUT_FORMAT=csv iojs benchmark/http/client-request-body.js >
data.csv
$ ./benchmark/plot_csv.R data.csv graph.png bytes type
```

This will graph the output to `graph.png`, using the output's `bytes`
value as X and the result value for each as Y. Output will be grouped
by `type`.

Running as the example yields a beautiful graph like this:
http://pbrd.co/1vBhUfy.
@brendanashworth
Copy link
Contributor Author

Got a user from the #R IRC channel to give me some tips (thanks ernst). Could you please take another look @bnoordhuis? And I'm unsure whether the IRC user's details should go on the signed commit message. Do you have a verdict on that?

@brendanashworth
Copy link
Contributor Author

@iojs/collaborators

@mscdex
Copy link
Contributor

mscdex commented Mar 13, 2015

What is the use case for the silent option?

Also this is for a separate issue, but it would be neat to have JSON output too.

@brendanashworth
Copy link
Contributor Author

@mscdex Unsure why anyone would use that, but I didn't want to remove it in an unrelated PR. I just kept it in (with backwards compat) and put it under the new option.

JSON output would be awesome.

@bnoordhuis
Copy link
Member

@brendanashworth Sorry, missed your earlier comment. LGTM.

@brendanashworth
Copy link
Contributor Author

I shalt merge tonight if nobody else comments.

brendanashworth added a commit that referenced this pull request Mar 17, 2015
@brendanashworth
benchmark: add plot_csv R graphing script
97d8d49 
This commit adds a graphing script (in R) for graphing the CSV output
of a benchmark. It can be run like this:

```
$ OUTPUT_FORMAT=csv iojs benchmark/http/client-request-body.js >
data.csv
$ ./benchmark/plot_csv.R data.csv graph.png bytes type
```

This will graph the output to `graph.png`, using the output's `bytes`
value as X and the result value for each as Y. Output will be grouped
by `type`.

Running as the example yields a beautiful graph like this:
http://pbrd.co/1vBhUfy.

PR-URL: #777
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
brendanashworth added a commit that referenced this pull request Mar 17, 2015
@brendanashworth
benchmark: add output format option [csv]
c638dad 
This commit adds an `OUTPUT_FORMAT` environment variable option for
all benchmark tests that allow either 'csv' or 'default' output. Default
output has been left unchanged, and csv output prints out the csv
headers along with the csv formatted per-test output, each test also
seperated by a newline.

It can be used like the following:
$ OUTPUT_FORMAT=csv iojs benchmark/common.js http

Not specifying the OUTPUT_FORMAT env var will default to 'default'.
Specifying a bad value will throw an error.

PR-URL: #777
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@brendanashworth
Copy link
Contributor Author

Thanks, merged in c638dad and 97d8d49 after a rebase.

@rvagg rvagg mentioned this pull request Mar 17, 2015
Closed
@brendanashworth brendanashworth deleted the add-benchmark-plot branch April 6, 2015 04:14
@ChALkeR ChALkeR added the benchmark Issues and PRs related to the benchmark subsystem. label Feb 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brendanashworth @KyleAMathews @bnoordhuis @mscdex @ChALkeR