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: allow compare via fine-grained filters #711

Closed
wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Feb 4, 2015

Before this commit, only benchmark targets defined in Makefile could be used. This commit allows execution of common.js directly and passing of filter arguments directly, allowing you to run either a subset of benchmarks or a single specific benchmark for comparison.

@@ -21,8 +23,14 @@ for (var i = 2; i < process.argv.length; i++) {
case '-h': case '-?': case '--help':
console.log(usage);
process.exit(0);
case '--':
benchmarks = []
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing semicolon.

@evanlucas
Copy link
Contributor

I like this :]. LGTM

default:
nodes.push(arg);
if (benchmarks !== undefined)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is splitting hairs, but can you make this Array.isArray(benchmarks)

@cjihrig
Copy link
Contributor

cjihrig commented Feb 4, 2015

Two small comments. Other than that, LGTM

Before this commit, only benchmark targets defined in Makefile could
be used. This commit allows execution of common.js directly and
passing of filter arguments directly, allowing you to run either a
subset of benchmarks or a single specific benchmark for comparison.
@mscdex
Copy link
Contributor Author

mscdex commented Feb 4, 2015

Done.


var show = 'both';
var nodes = [];
var html = false;
var benchmarks;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't [testFilter] and benchmarks be options?

https://github.com/iojs/io.js/blob/v1.x/benchmark/common.js#L62

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you mean. AFAIK using require('./common').createBenchmark() won't work well because it spawns tests with stdio: 'inherit', so you can't capture the output needed for comparing.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Fishrock123
Copy link
Contributor

LGTM other than that the naming is a little confusing.

Edit: Looks like that may be for another issue.

cjihrig pushed a commit that referenced this pull request Feb 4, 2015
Before this commit, only benchmark targets defined in Makefile could
be used. This commit allows execution of common.js directly and
passing of filter arguments directly, allowing you to run either a
subset of benchmarks or a single specific benchmark for comparison.

PR-URL: #711
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@cjihrig
Copy link
Contributor

cjihrig commented Feb 4, 2015

Thanks @mscdex. Landed in e0730ee

@cjihrig cjihrig closed this Feb 4, 2015
@mscdex mscdex deleted the enhance-bench-compare branch February 5, 2015 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants