Add benchmarks for node only mode and mock timers.#64097
Conversation
|
Review requested:
|
|
|
||
| for (let i = 0; i < selected; i++) { | ||
| test(`selected-${i}`, { only: true }, () => { | ||
| avoidV8Optimization = i; |
There was a problem hiding this comment.
This is unnecessary; it's a clojure. Adding an assert.ok is probably more useful.
There was a problem hiding this comment.
Hi @RafaelGSS, thanks for taking a look.
Just to make sure I understand the suggestion: are you suggesting replacing the avoidV8Optimization assignment with an assertion inside the benchmarked section?
I added the avoidV8Optimization bit to avoid making the benchmarked callback completely empty, as detailed in the previous PR: #63754 (comment)
My concern with adding assert.ok() inside the callback is that the assertion itself would become part of what is being measured. Adding extra work inside the benchmark was also something @avivkeller raised in the previous PR: #63754 (comment)
I’m happy to adjust it either way. Please let me know how to proceed.
Thank you for the guidance.
Obs: I see the comments about calbrate-n on the first PR and i will push that change very soon here too
The only benchmark covers creating selected and non-selected tests when running with --test-only. The mock timers benchmark covers enabling timer mocks, setTimeout, setInterval, setImmediate, scheduler.wait, AbortSignal.timeout, mocked Date.now(), setTime(), and runAll(). Refs: nodejs#55723 Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
Use calibrated n values for the test runner mock-timers and test-only benchmarks. Reduce the test options benchmark configurations to keep the benchmark suite smaller. Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
723caba to
8e49e7c
Compare
|
Hi there, sorry for the force-push. Like the other branch, 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 numbers are already adjusted. cc @RafaelGSS |
Adds another small set of
node:testbenchmarks for areas listed in #55723.A continuation of the work I started in #63754
This is a different PR because it brings a different set of attention points, and to keep both PR easy to review.
This PR covers:
onlymode with--test-onlymock.timers.enable()setTimeoutsetIntervalsetImmediatescheduler.waitAbortSignal.timeoutDate.now()mock.timers.setTime()mock.timers.runAll()Notes
enable()+reset()instead of just enable. This happens because an error is thrown if enable is called multiple times without reset.scheduler.waitmode includes the finalPromise.all()inside the measured section. My evaluation was that it was more appropriate to do so than not, since the scheduled waits are promise-based, but I am happy to adjust the structure if reviewers prefer a narrower measure.Issue found while writing this
TLDR: I found an issue (that doesn't affect this PR) with using
setImmediateandrunAlltogether while writing the benchmark; guidance welcome;While writing the benchmark, I found that
setImmediatedoesn't work withrunAll(). The mocked setImmediate is scheduled with a special-1time so it runs before zero-delay timers.runAll()computeslongestTimer.runAt - this.#nowinmock_timers.js:816, which becomes-1, andtick()rejects negative time in mock_timers.js:556.I would love to open a new PR to fix this issue, but this is beyond my context of how the team working on this operates and what they think would be an appropriate fix. If the person reviewing the PR could provide some guidance on the best way to handle this issue, it would be much appreciated.
This issue doesn't impact this PR, all benchmarks still work with the current code (With tick(0) instead of runAll() for setImmediate).