-
Notifications
You must be signed in to change notification settings - Fork 30k
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: make compare.R easier to understand #18373
Conversation
/cc @joyeecheung |
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.
Not a R expert, but LGTM if benchmark CI is happy.
Benchmark CI: https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/103/ Looking at https://github.com/nodejs/benchmarking/blob/master/experimental/benchmarks/community-benchmark/run.sh I think this should be using the new R script for the results, cc @gareth-ellis |
The accuracy there probably means those benchmarks are just not that reliable in nature... |
LGTM, i seem to be having issues with my email notifications at the moment. As there is the extra warning about false positives in the benchmark output, I think we can be sure that the change was in this build - but i think i can make it clearer in the future.! Note, the job is actually running https://github.com/nodejs/benchmarking/blob/core-benchmark/experimental/benchmarks/community-benchmark/run.sh I'll get this into master, as that's going to lead to even more confusion (I made some changes to try and reduce output from the build, but I need to also get it to take stderr away, as we have a lot of warnings that make the rest of the output trickier to understand.) |
Landed in 368517c, thanks! |
PR-URL: #18373 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #18373 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: #18373 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
PR-URL: nodejs#18373 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>
As talked about in #18112 (comment) this shows more clearly the variance of each comparison. This should also help us prevent over-running the benchmarks. If you see an accuracy of
±0.1%
then properly you could spend fewer iterations running that ;)Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
benchmark
example output: