Skip to content

Conversation

0hmX
Copy link
Contributor

@0hmX 0hmX commented Jul 19, 2025

Closes: #58488

This commit introduces two new options to the Benchmark tool: setup and teardown. The setup option runs before any forks are created, and teardown executes after all forks have completed.

Additionally, a necessary update to a test file, as identified in the linked issue, has been included. I've incorporated both the new feature and the test file update into this pull request.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/performance
  • @nodejs/tsc

@0hmX 0hmX force-pushed the 58488 branch 4 times, most recently from dc8ce06 to d5c3f59 Compare July 19, 2025 17:02
0hmX added 2 commits July 21, 2025 07:32
This commit introduces two new options to the Benchmark tool: setup
and teardown. The setup option runs before any forks are created,
and teardown executes after all forks have completed.
@0hmX
Copy link
Contributor Author

0hmX commented Jul 21, 2025

@RafaelGSS, if you want to do another over this! Would appreciate that, it's ready to get merged!

@0hmX
Copy link
Contributor Author

0hmX commented Jul 24, 2025

@RafaelGSS pinging you just to remind you about the PR😅

n: [t],
}, {
setup() {
tmpdir.refresh();
Copy link
Member

Choose a reason for hiding this comment

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

Is it exactly equal to the previous behaviour? Your documentation states:

A function to be run once by the main "controller"
process before any benchmark child processes are spawned.

However, this should run once per configuration, as it does on function main(). Either your documentation is wrong, or the setup and teardown shouldn't work like that; instead, it should be called once per configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a log to createEntryPoint

Current output

$ node benchmark/module/module-require.js
called createEntryPoint
module/module-require.js n=10000 type=".js": 9,197.64355525931
called createEntryPoint
module/module-require.js n=10000 type=".json": 19,692.5488212058
called createEntryPoint
module/module-require.js n=10000 type="dir": 8,185.132800456524

My Pr

$ node benchmark/module/module-require.js
called setup
called createEntryPoint
module/module-require.js n=10000 type=".js": 9,507.804393764822
module/module-require.js n=10000 type=".json": 18,387.40575971879
module/module-require.js n=10000 type="dir": 7,686.528215393394
called teardown

However, this should run once per configuration, as it does on function main(). Either your documentation is wrong, or the setup and teardown shouldn't work like that; instead, it should be called once per configuration

It runs once per configuration, and the documentation is also accurate. Here is my reason to say that. createConfig creates a controller, source code reads the config. So the controller is the one doing everything;

  createBenchmark(fn, configs, options) {
    return new Benchmark(fn, configs, options);
  }

which is why I said

A function to be run once by the main "controller"

Copy link
Contributor Author

@0hmX 0hmX Jul 27, 2025

Choose a reason for hiding this comment

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

@RafaelGSS ping

Copy link
Member

Choose a reason for hiding this comment

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

So they aren't the same. Most of the time the teardown is necessary after each configuration

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@RafaelGSS, thanks for the explanation.

Sorry for being dumb and ignoring your previous comment about the per configuration. I understand now. But now I am unsure which part of the codebase I should look at or how the API for this new feature will look. If you have any ideas, please share them. Else, I will close this PR a few days from now.

Thanks again for your time!

Copy link
Member

Choose a reason for hiding this comment

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

I think that's why this feature doesn't exist. We fork the process for every configuration, so there's no way to "wrap" a setup and teardown to each configuration as functions aren't passed across environments -- That's why people usually do it in the main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support per-benchmark-suite setup and teardown
3 participants