Skip to content

benchmark: add test runner hooks and options #63754

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.

Sign up for GitHub

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

Merged
nodejs-github-bot merged 5 commits into from
Aug 9, 2026
Merged

benchmark: add test runner hooks and options #63754

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2567352
benchmark: add test runner hooks and options
luanmuniz Jun 5, 2026
b9188c0
benchmark: avoid switch in test runner benchmarks
luanmuniz Jun 7, 2026
7a8461a
benchmark: exercise test runner hook bodies
luanmuniz Jun 8, 2026
d09c413
benchmark: calibrate test runner benchmarks
luanmuniz Jun 25, 2026
c39085e
benchmark: remove optimization guard
luanmuniz Jul 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
benchmark: exercise test runner hook bodies 
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:
#48931 (comment)
https://www.mail-archive.com/v8-users@googlegroups.com/msg05521.html

Signed-off-by: Luan Muniz <luan@luanmuniz.com.br>
  • Loading branch information
@luanmuniz
luanmuniz committed Jun 26, 2026
commit 7a8461a28ba073cb00e283c7bf20a34604f72a58
6 changes: 3 additions & 3 deletions benchmark/test_runner/hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,6 @@ const bench = common.createBenchmark(main, {
flags: ['--test-reporter=./benchmark/fixtures/empty-test-reporter.js'],
});

const noop = () => {};

const hookList = {
before: before,
after: after,
Expand All @@ -32,7 +30,9 @@ const hookList = {
function run(loopAmount, avoidV8Optimization, hookFn) {
for (let i = 0; i < loopAmount; i++) {
describe(`${i}`, () => {
hookFn(noop);
hookFn(() => {
avoidV8Optimization = i;

@RafaelGSS RafaelGSS Jul 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's still unnecessary. This is not what will prevent V8 from optimising this code away. This variable name just provide a false sense this is the reason this code won't be optimized away.

@luanmuniz luanmuniz Jul 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RafaelGSS Can you provide more details for your suggestion? Is your suggestion removing the variable and leaving it as a noop, or renaming it? If renaming it, could you suggest something? That way we don't delay this PR further with naming discussion.

@RafaelGSS RafaelGSS Jul 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing it and leaving as noop

@luanmuniz luanmuniz Jul 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Removed avoidV8Optimization and left the callbacks as no-ops. If the current implementation is acceptable, could the thread be resolved and CI started?

});

it(`${i}`, () => {
avoidV8Optimization = i;
Expand Down