-
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
tools, benchmark: test util benchmark #16050
Conversation
benchmark/util/type-check.js
Outdated
@@ -38,7 +38,7 @@ function main(conf) { | |||
const util = process.binding('util'); | |||
const types = require('internal/util/types'); | |||
|
|||
const n = (+conf.millions * 1e6) | 0; | |||
const n = (+conf.n * 1e6) | 0; |
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.
This would increase n
from 5 * 1e6
to 1e7 * 1e6
, which is going to cost more time if the benchmark is run normally. Can you change the default n to 5e6
and use that value as-is(without multiplying) instead?
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.
Ah yes, I agree that is a better solution. (Please keep the number coercion bits though.)
test/parallel/test-benchmark-util.js
Outdated
|
||
const runBenchmark = require('../common/benchmark'); | ||
|
||
runBenchmark('util', ['n=1', 'arguments=0']); |
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.
Setting arguments=0
would turn the arg
in the test undefined
. I believe we are good with n=1
only.
@joyeecheung thank you so much for your thoughtful feedback! ❤️ |
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.
LGTM, CI: https://ci.nodejs.org/job/node-test-pull-request/10632/
(which probably won't fully pass regardless but hey)
Hmm, timed out on SmartOS. [1]
Edit: looks like we are taking too long still for some reason, the longest other benchmark test is
|
My guess is that it just spawns too many child processes for slow machines. It still takes 10s on my i7 2.6ghz. So, this just got a bit more complex, but to get this to reliably pass on all systems there are two options (I think):
Maybe @nodejs/testing can advise on whether option 1 is reasonable. |
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.
Thanks for taking this on! Before this lands, we need some more options added to that array that is the second argument to runBenchmark()
. Basically, anything that has more than one option in any of the util benchmark files will need an option passed so that we aren't running every combination of options for each file. While that would be a thorough test, it will time out in CI for some hosts. The goal of these benchmark tests are to flag benchmark changes that totally break everything, rather than thoroughly test the files, so it's OK if there is code in the benchmarks that end up being skipped because of this.
So, you'll probably want to make the array ['argument=false', 'input=', 'method=Array', 'n=1', 'option=none']
plus a few more options in there.
To get an idea of how the options affect the way the benchmark runs, you can try adding elements to the array one at a time and see how it affects the output of ./node test/parallel/test-benchmark-util.js
.
There may be other pitfalls that pop up--we'll see. Be persistent. These benchmark tests aren't difficult to write, but they do require a bit of fussing and patience to get right.
@Fishrock123 I'd be a pretty hard "no" on option 1. Option 2 could work, but the usual way we handle this is to set more items in the settings array so that the benchmark has fewer combinations of settings to run. See my comment above and/or look at existing (This was a "good second commit" task from Code + Learn. It's totally do-able but likely to require more back-and-forth and experimentation than the first tasks given at the event.) |
@Trott @Fishrock123 If we are talking about running one file at a time, Also this reminds me of the |
@Trott @Fishrock123 OK I think this is not related to "too many processes at the same time" because (We don't have a |
With the right options, this test will run plenty fast. Just given the options I put in my comment above (which is a partial list--still more to add, but I left that as an exercise for the contributor), the run time on my laptop for the test is reduced from 19 seconds to less than 3 seconds. With the right arguments this test will be no problem at all.
If we wanted to do that (which we don't in this case!), we could use |
c20929b
to
78ebc37
Compare
I went ahead and made the changes involving the arguments/settings. Hope that's OK. This should be good to land, I think. @joyeecheung @Fishrock123 PTAL |
(Oh, run time is now down to less than 1.5 seconds locally for me. 🎉) |
Create a minimal test for the util benchmark files. PR-URL: nodejs#16050 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Landed in 50fe1a8. Thanks for the contribution! 🎉 |
Create a minimal test for the util benchmark files. PR-URL: #16050 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Should this be backported to cc/ @joyeecheung @Trott regarding suitability for backporting. The commit cherry-picks fine, but we get this test failure:
|
I wouldn't bother backporting this unless someone wanted to do it as an exercise. It's great for code changes going forward, but we don't need it on old release lines. We've gotten by just fine thus far without it. :-D |
Create a minimal test for the util benchmark files. PR-URL: nodejs/node#16050 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Create a minimal test for the util benchmark files. PR-URL: nodejs/node#16050 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Create a minimal test for the util benchmark files. PR-URL: nodejs/node#16050 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
coverage for benchmarking util
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
doc, benchmark