{"action":"created","issue":{"url":"https://api.github.com/repos/nodejs/node/issues/7094","repository_url":"https://api.github.com/repos/nodejs/node","labels_url":"https://api.github.com/repos/nodejs/node/issues/7094/labels{/name}","comments_url":"https://api.github.com/repos/nodejs/node/issues/7094/comments","events_url":"https://api.github.com/repos/nodejs/node/issues/7094/events","html_url":"https://github.com/nodejs/node/pull/7094","id":157934177,"number":7094,"title":"Refactor benchmark tools for statistical significance","user":{"login":"AndreasMadsen","id":505333,"avatar_url":"https://avatars.githubusercontent.com/u/505333?v=3","gravatar_id":"","url":"https://api.github.com/users/AndreasMadsen","html_url":"https://github.com/AndreasMadsen","followers_url":"https://api.github.com/users/AndreasMadsen/followers","following_url":"https://api.github.com/users/AndreasMadsen/following{/other_user}","gists_url":"https://api.github.com/users/AndreasMadsen/gists{/gist_id}","starred_url":"https://api.github.com/users/AndreasMadsen/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AndreasMadsen/subscriptions","organizations_url":"https://api.github.com/users/AndreasMadsen/orgs","repos_url":"https://api.github.com/users/AndreasMadsen/repos","events_url":"https://api.github.com/users/AndreasMadsen/events{/privacy}","received_events_url":"https://api.github.com/users/AndreasMadsen/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/nodejs/node/labels/benchmark","name":"benchmark","color":"009800"}],"state":"open","locked":false,"assignee":{"login":"Fishrock123","id":1093990,"avatar_url":"https://avatars.githubusercontent.com/u/1093990?v=3","gravatar_id":"","url":"https://api.github.com/users/Fishrock123","html_url":"https://github.com/Fishrock123","followers_url":"https://api.github.com/users/Fishrock123/followers","following_url":"https://api.github.com/users/Fishrock123/following{/other_user}","gists_url":"https://api.github.com/users/Fishrock123/gists{/gist_id}","starred_url":"https://api.github.com/users/Fishrock123/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Fishrock123/subscriptions","organizations_url":"https://api.github.com/users/Fishrock123/orgs","repos_url":"https://api.github.com/users/Fishrock123/repos","events_url":"https://api.github.com/users/Fishrock123/events{/privacy}","received_events_url":"https://api.github.com/users/Fishrock123/received_events","type":"User","site_admin":false},"assignees":[{"login":"Fishrock123","id":1093990,"avatar_url":"https://avatars.githubusercontent.com/u/1093990?v=3","gravatar_id":"","url":"https://api.github.com/users/Fishrock123","html_url":"https://github.com/Fishrock123","followers_url":"https://api.github.com/users/Fishrock123/followers","following_url":"https://api.github.com/users/Fishrock123/following{/other_user}","gists_url":"https://api.github.com/users/Fishrock123/gists{/gist_id}","starred_url":"https://api.github.com/users/Fishrock123/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Fishrock123/subscriptions","organizations_url":"https://api.github.com/users/Fishrock123/orgs","repos_url":"https://api.github.com/users/Fishrock123/repos","events_url":"https://api.github.com/users/Fishrock123/events{/privacy}","received_events_url":"https://api.github.com/users/Fishrock123/received_events","type":"User","site_admin":false}],"milestone":null,"comments":25,"created_at":"2016-06-01T15:18:38Z","updated_at":"2016-07-05T08:33:38Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/nodejs/node/pulls/7094","html_url":"https://github.com/nodejs/node/pull/7094","diff_url":"https://github.com/nodejs/node/pull/7094.diff","patch_url":"https://github.com/nodejs/node/pull/7094.patch"},"body":"##### Checklist\r\n\r\n- [x] tests and code linting passes\r\n- [ ] a test and/or benchmark is included\r\n- [x] documentation is changed or added\r\n- [x] the commit message follows commit guidelines\r\n\r\n##### Affected core subsystem(s)\r\nbenchmark\r\n\r\n##### Description of change\r\n\r\nI have been rather confused about the benchmark suite and I don't think it is as user friendly as the rest of nodecore. This PR attempt to remove most of the confusion I was facing when I started using it. Primarily it:\r\n\r\n* removes unused/undocumented files\r\n* allows partially setting the benchmarks variables using process arguments.\r\n* refactor `compare.js` such comparing node versions and getting statistical significance is easy.\r\n* refactor the `plot.R` tool (now called `scatter`) to show a scatter plot with confidence bars.\r\n* refactor cli tools such the cli API is more homogeneous.\r\n* documents all the tools.\r\n* removes the implicit `process.exit(0)` after `bench.end()`.\r\n* uses `process.send` to avoid most parsing (the benchmark cli arguments haven't changed).\r\n\r\nThe specifics are documented in the commit messages. Please also see the the [new README](https://github.com/AndreasMadsen/node/blob/benchmark-refactor/benchmark/README.md) as quite a lot have changed (be sure the to check my spelling!).\r\n\r\n_Note that some benchmark takes a very long time to complete, e.g. `timers/timers.js\ttype=depth thousands=500` takes 11.25 min. Thus running it 30 times for statistical significance is unreasonable. I suspect the only reason why it is set to so many iterations is to get a small variance, but with the the new `compare` tool the variance can be estimated instead of being reduced. Thus we can reduce the number of iterations and still get the information we need. But I suggest we do that in another pull request, as is very different discussion._\r\n\r\n**Motivation (long story)**: I wanted to benchmark the effect of some `async_wrap` changes. I went to the `benchmark/` directory and read the [README](https://github.com/nodejs/node/blob/master/benchmark/README.md). However I quickly discovered that it was primarily about running benchmarks a single time and how to write benchmarks. And most importantly it didn't explain how to compare two node versions. _This is now documented in the [new README](https://github.com/AndreasMadsen/node/blob/benchmark-refactor/benchmark/README.md)._\r\n\r\nI then had to search for the tools myself and discovered the large amount of benchmarks files which where not put into categorized directories. I assumed they where somehow extra significant, but in reality they just appear to be unused. _These files are now removed._\r\n\r\nAfter discovering the compare tool, which has the cli API\r\n```\r\nnode benchmark/compare.js\r\n            <node-binary1> <node-binary2> +\r\n            [--html] [--red|-r] [--green|-g] +\r\n            [-- <type> [testFilter]]\r\n```\r\nI was confused about what the `--red`, `--green` was and how the `node-binary1` and `node-binary2` compared, should I write `./node-old ./node-new` or `./node-new ./node-old` if I wanted a positive improvement factor to signify an improvement? _The new compare API is_:\r\n```\r\nusage: ./node benchmark/compare.js <type> ...\r\n  --new    ./new-node-binary  new node binary (required)\r\n  --old    ./old-node-binary  old node binary (required)\r\n  --runs   30                 number of samples\r\n  --filter pattern            string to filter benchmark scripts\r\n  --var    variable=value     set benchmark variable (can be repeated)\r\n```\r\n\r\nAfter understanding `common.js` this it was still unclear if the performance was statistically significant different. I tried running the benchmark 5 times and got that 4/5 was an improvement, I was expecting it to have the same performance or be slower. (spoiler: it wasn't significant). _The compare.js script now runs the benchmarks many times (30 by default) and there is an R script to analyse the csv results._\r\n\r\nAt this point I wanted to do a rewrite of the benchmark tools (not the benchmarks themself) and changed a few other things in the process as well. - I'm a mathematician so I care a lot about statistical significance :)\r\n"},"comment":{"url":"https://api.github.com/repos/nodejs/node/issues/comments/230418306","html_url":"https://github.com/nodejs/node/pull/7094#issuecomment-230418306","issue_url":"https://api.github.com/repos/nodejs/node/issues/7094","id":230418306,"user":{"login":"addaleax","id":899444,"avatar_url":"https://avatars.githubusercontent.com/u/899444?v=3","gravatar_id":"","url":"https://api.github.com/users/addaleax","html_url":"https://github.com/addaleax","followers_url":"https://api.github.com/users/addaleax/followers","following_url":"https://api.github.com/users/addaleax/following{/other_user}","gists_url":"https://api.github.com/users/addaleax/gists{/gist_id}","starred_url":"https://api.github.com/users/addaleax/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/addaleax/subscriptions","organizations_url":"https://api.github.com/users/addaleax/orgs","repos_url":"https://api.github.com/users/addaleax/repos","events_url":"https://api.github.com/users/addaleax/events{/privacy}","received_events_url":"https://api.github.com/users/addaleax/received_events","type":"User","site_admin":false},"created_at":"2016-07-05T08:33:38Z","updated_at":"2016-07-05T08:33:38Z","body":"We’re having R scripts in `benchmark/` right now, too… not that a note in the README wouldn’t be helpful anyway. :)"}}
{"actor":{"display_login":"addaleax"}}