-
-
Notifications
You must be signed in to change notification settings - Fork 33.4k
benchmark: new options setup and teardown #59120
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
Conversation
Review requested:
|
dc8ce06
to
d5c3f59
Compare
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.
@RafaelGSS, if you want to do another over this! Would appreciate that, it's ready to get merged! |
@RafaelGSS pinging you just to remind you about the PR😅 |
n: [t], | ||
}, { | ||
setup() { | ||
tmpdir.refresh(); |
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.
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
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.
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"
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.
@RafaelGSS ping
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.
So they aren't the same. Most of the time the teardown is necessary after each configuration
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.
@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!
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.
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
.
Closes: #58488
This commit introduces two new options to the Benchmark tool:
setup
andteardown
. Thesetup
option runs before any forks are created, andteardown
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.