benchmark: add test runner hooks and options#63754
Conversation
|
Review requested:
|
nodejs-github-bot
added
benchmark
| describe(`${i}`, () => { | ||
| switch (hook) { | ||
| case 'before': | ||
| before(noop); | ||
| break; | ||
| case 'after': | ||
| after(noop); | ||
| break; | ||
| case 'beforeEach': | ||
| beforeEach(noop); | ||
| break; | ||
| case 'afterEach': | ||
| afterEach(noop); | ||
| break; | ||
| case 'none': | ||
| break; | ||
| } |
There was a problem hiding this comment.
This is also benchmarking the switch statement, which we don't want.
|
@avivkeller Changes done! Obs: |
| // eslint-disable-next-line prefer-const | ||
| let avoidV8Optimization = 0; |
There was a problem hiding this comment.
It is let because we rewrite the values through the benchmark, and there is no particular reason to be 0, just didn't want to leave it undefined, and the original value of the variable shouldn't have an impact on this.
To be 100% honest, I don't quite understand the mechanics for this. I never dug into v8 optimizations before. The variable is there because I decided to follow the existing patterns inside this particular folder.
But please check this reference link and this for the comment that originally introduced this specific variable (Seems like the link is not working 100%, opening the discussion so just crtl+f for benchmark/test_runner/global-concurrent-tests.js and there should be a resolved review from H4ad with the reference)
Obs: I just realized the hooks are noops, and given that this variable works as described, I shouldn't have a noop there, so I'm pushing code so the hooks also have minimal execution within the function. Thanks for calling my attention to this.
|
@RafaelGSS @avivkeller Both feedbacks are addressed. Please let me know if it's resolved or if any changes are still needed. |
|
Ping @RafaelGSS @avivkeller I'm waiting for this PR to be done so I can continue working on the other benchmarks and hopefully close the Ref issue. I have additional local benchmarks for |
| } = require('node:test'); | ||
|
|
||
| const bench = common.createBenchmark(main, { | ||
| n: [1, 10, 100, 1000], |
There was a problem hiding this comment.
Are these numbers arbitrarily chosen? Please use https://github.com/nodejs/node/blob/main/benchmark/calibrate-n.js
There was a problem hiding this comment.
Yes and No. I took those numbers from another benchmark in the same folder since I didn't know exactly how the configurations were used forward.
That being said, I didn't know about calibrate-n and how it worked, but after some study, I think I understand it now. I'm running it for both this and the PR for mock-timers. I will post the results here as soon as I have them and update the numbers with the best one found.
RafaelGSS
left a comment
RafaelGSS
left a comment
There was a problem hiding this comment.
I'm uncertain that the number of configs for the benchmarks is useful. Our benchmark suite is already too big, any configuration you add increases the suite run exponentially.
It would be great to share the current results too.
| const { it } = require('node:test'); | ||
|
|
||
| const bench = common.createBenchmark(main, { | ||
| n: [1, 10, 100, 1000], |
Add benchmarks for node:test hooks and test options. The hooks benchmark covers before, after, beforeEach, and afterEach, with a none mode as the baseline. The test options benchmark covers skip and todo behavior. This adds coverage for part of the benchmark/test_runner gaps tracked in the issue. Refs: nodejs#55723 Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
Select each benchmark variant before starting the timer so the measured loop does not include dispatch overhead. Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
Update the hooks benchmark so registered hooks perform the same anti-optimization assignment as test bodies instead of calling a noop. This keeps the measured hook path from using an empty callback. Links for more information on this actions: nodejs#48931 (comment) https://www.mail-archive.com/v8-users@googlegroups.com/msg05521.html Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
Use calibrated n values for the test runner hooks and options benchmarks. Reduce the test options benchmark configurations to keep the benchmark suite smaller. Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
luanmuniz
force-pushed
the
benchmark-test-runner-hooks-options
branch
from
d8f7882 to
d4ab1a9
Compare
|
Hi there, sorry for the force-push. I had a local git history issue while cleaning up the branch, and I force-pushed to get the PR back into the intended state. Apologies for the noise. I’ll be more careful with the branch history going forward. I've used calibrate-n, and there are the results in case you want to take a look: The script asked for 10K for both benchmarks, but in my opinion, the Also, I found a small bug in the calibrate-n code: it wasn't using the correct arguments and was always using the const defaults instead. Is it best to open another PR for just that, or add that change here? CC @RafaelGSS |
4 participants
Add benchmarks for node:test hooks and test options. The hooks benchmark covers before, after, beforeEach, and afterEach, with a none mode as the baseline. The test options benchmark covers skip and todo behavior. This adds coverage for part of the benchmark/test_runner gaps tracked in the issue.
Refs: #55723
I eventually want to continue working on the Ref issue, and do the other benchmarks, but I decided to start with the simple ones that I can use existing files as a reference, get a feel for how the project works. Starting small and all of that. This will be my first (hopefully of many) contribution.
Please let me know if any changes or actions are required from me. I will wait for feedback before start tackling the other parts of the ref issue
Edit: I had some time today, and I started working on the other benchmarks, and I do have
only,mock.timersandmock.modulebenchmarks ready too. I will await for feedback on the current code before pushing the other benchmarks. Please let me know if you prefer for me to push a different PR those since they have a bit more nuance, especially the mock.module one (Still nothing major tho)