-
Notifications
You must be signed in to change notification settings - Fork 29.6k
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: bump eventemitter number of iterations #746
Conversation
Some of the benchmarks that were added in commit 847b9d2 complete too quickly to draw meaningful conclusions from. Increase the number of iterations to make them run longer.
@@ -1,7 +1,7 @@ | |||
var common = require('../common.js'); | |||
var EventEmitter = require('events').EventEmitter; | |||
|
|||
var bench = common.createBenchmark(main, {n: [25e4]}); | |||
var bench = common.createBenchmark(main, {n: [5e7]}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why 5e7 on this one? (Why not just make them all, say, 2e7?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some complete more quickly than others. This particular benchmark is the quickest of them all.
LGTM though |
I did have a larger number of iterations on some of them, but I wasn't sure just how long people would want to wait to run them. I personally don't mind having them run longer. I think ultimately using the benchmark.js module will give good, reliable numbers because I believe it does some analysis on the data before generating the results. |
@mscdex Is that a LGTM? :-) |
Benchmark.js adapts based on each round. (or at least near the start..) |
@bnoordhuis Yeah LGTM |
Some of the benchmarks that were added in commit 847b9d2 complete too quickly to draw meaningful conclusions from. Increase the number of iterations to make them run longer. PR-URL: #746 Reviewed-By: Jeremiah Senkpiel <[email protected]>
Landed in 2c3121c |
Below are currently failing unit test cases ```cmd parallel/test-crypto-dh parallel/test-fs-stat parallel/test-fs-symlink parallel/test-promises-unhandled-rejections parallel/test-repl-mode parallel/test-repl-tab-complete parallel/test-regress-GH-746 parallel/test-regress-GH-io-1068 parallel/test-util-inspect-proxy parallel/test-vm-context parallel/test-vm-create-and-run-in-context parallel/test-vm-cached-data parallel/test-vm-global-identity parallel/test-vm-preserves-property parallel/test-vm-proxies parallel/test-vm-timeout addons/load-long-path/test known_issues/test-vm-function-redefinition sequential/test-vm-timeout-rethrow ``` PR-URL: nodejs/node-chakracore#71 Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Jianchun Xu <[email protected]>
Some of the benchmarks that were added in commit 847b9d2 complete too
quickly to draw meaningful conclusions from. Increase the number of
iterations to make them run longer.
R=@mscdex?