Skip to content
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

benchmark: add misc/startup-cli-version benchmark #50684

Merged
merged 1 commit into from Nov 15, 2023

Conversation

joyeecheung
Copy link
Member

This benchmarks the startup of various CLI tools that are already checked into the source code. We use --version because the output tends to be minimal and fewer operations are done to generate these so that the startup cost is still dominated by a more indispensible part of the CLI.

@nodejs-github-bot nodejs-github-bot added the benchmark Issues and PRs related to the benchmark subsystem. label Nov 12, 2023
@joyeecheung joyeecheung force-pushed the startup-cli branch 5 times, most recently from 90e9da6 to 29b034f Compare November 12, 2023 09:55
@GeoffreyBooth
Copy link
Member

You could add TypeScript too.

Are all of these CommonJS? Is there anything we can add to benchmark ESM startup?

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 12, 2023

You could add TypeScript too.

We don’t have the TypeScript CLI checked in, only one library bundle is in the fixtures (so that's supposed to be require()d). Though that could probably be added to the other startup benchmark or as a CommonJS bundle loading benchmark of its own. Probably out of scope for this one though.

Are all of these CommonJS? Is there anything we can add to benchmark ESM startup?

I can’t find any existing CLI checked into the code base using ESM. If we have something in the future or if any one of them gets updated to use ESM then that can be covered. Otherwise I guess this is just what we have for now. This just serves as “since we already have some real-world CLIs checked in the code base, might as well use them for some benchmarking to ensure we don’t regress real-world CLIs”. Whether these real-world CLIs uses ESM is up to them.

If we really want to check ESM performance for CLIs that we don’t use, I’d suggest doing it in a different repo (the CLIs listed here are already making the checkout humongous, but then we at least have some proper use of them other than benchmarking). I'd say the same applies to the maintenance of this list - if for some reason we no longer use any of them (I doubt that though, maybe eslint can go away in the future? But the other two probably won't), we should just remove them from this benchmark and should't keep them in the codebase just for benchmarking.

This benchmarks the startup of various CLI tools that are already
checked into the source code. We use --version because the output
tends to be minimal and fewer operations are done to generate
these so that the startup cost is still dominated by a more
indispensible part of the CLI.
@anonrig anonrig added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 12, 2023
@GeoffreyBooth
Copy link
Member

I can’t find any existing CLI checked into the code base using ESM. If we have something in the future or if any one of them gets updated to use ESM then that can be covered. Otherwise I guess this is just what we have for now.

Yeah, this is something we need but I agree I don’t want to add unnecessary dependencies to the repo.

You could perhaps have another set of spawn calls that include --import 'data:text/javascript,' (a no-op --import) to trigger the handling of the entry point to be by the ESM loader. Everything after the initial entry would still be handled by the CommonJS loader, but at least we would be benchmarking the “startup to ESM entry” part of the flow.

@joyeecheung
Copy link
Member Author

joyeecheung commented Nov 12, 2023

You could perhaps have another set of spawn calls that include --import 'data:text/javascript,' (a no-op --import) to trigger the handling of the entry point to be by the ESM loader.

That sounds like a separate benchmark than this one, which is about real world CLIs. I haven’t seen any CLIs that would be loaded this way in the real world (maybe there are, but I just haven’t seen any)

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2023
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung mentioned this pull request Nov 13, 2023
6 tasks
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 15, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 15, 2023
@nodejs-github-bot nodejs-github-bot merged commit 59b27d6 into nodejs:main Nov 15, 2023
54 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 59b27d6

targos pushed a commit that referenced this pull request Nov 23, 2023
This benchmarks the startup of various CLI tools that are already
checked into the source code. We use --version because the output
tends to be minimal and fewer operations are done to generate
these so that the startup cost is still dominated by a more
indispensible part of the CLI.

PR-URL: #50684
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
martenrichter pushed a commit to martenrichter/node that referenced this pull request Nov 26, 2023
This benchmarks the startup of various CLI tools that are already
checked into the source code. We use --version because the output
tends to be minimal and fewer operations are done to generate
these so that the startup cost is still dominated by a more
indispensible part of the CLI.

PR-URL: nodejs#50684
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Nov 28, 2023
UlisesGascon pushed a commit that referenced this pull request Dec 11, 2023
This benchmarks the startup of various CLI tools that are already
checked into the source code. We use --version because the output
tends to be minimal and fewer operations are done to generate
these so that the startup cost is still dominated by a more
indispensible part of the CLI.

PR-URL: #50684
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@UlisesGascon UlisesGascon mentioned this pull request Dec 12, 2023
AdamMajer added a commit to AdamMajer/node that referenced this pull request Dec 13, 2023
tools/node_modules is removed from the tarball so it should not be used
as part of unit tests or benchmarks.

Fixes: nodejs#51145
Refs: nodejs#50684
AdamMajer added a commit to AdamMajer/node that referenced this pull request Dec 15, 2023
tools/node_modules is removed from the tarball so it should not be used
as part of unit tests or benchmarks.

Fixes: nodejs#51145
Refs: nodejs#50684
UlisesGascon pushed a commit that referenced this pull request Dec 19, 2023
This benchmarks the startup of various CLI tools that are already
checked into the source code. We use --version because the output
tends to be minimal and fewer operations are done to generate
these so that the startup cost is still dominated by a more
indispensible part of the CLI.

PR-URL: #50684
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this pull request Dec 25, 2023
tools/node_modules is removed from the tarball so it should not be used
as part of unit tests or benchmarks.

Fixes: #51145
Refs: #50684
PR-URL: #51146
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
tools/node_modules is removed from the tarball so it should not be used
as part of unit tests or benchmarks.

Fixes: #51145
Refs: #50684
PR-URL: #51146
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
AdamMajer added a commit to AdamMajer/node that referenced this pull request Feb 13, 2024
In previous version of this fix, I've simply added a check if the tested
tool is available or not. Unfortuntelly, this fails when only the first
tool is to be run as part of the test-benchmark-misc, and it doesn't
exist.

benchmark/test-benchmark-misc
...
AssertionError [ERR_ASSERTION]: benchmark file not running exactly one
configuration in test:
...
misc/startup-cli-version.js
...

The solution is to move the tool that is not present in a tarball down
the list.

Fixes: nodejs#51146
Refs: nodejs#50684
richardlau pushed a commit that referenced this pull request Mar 25, 2024
tools/node_modules is removed from the tarball so it should not be used
as part of unit tests or benchmarks.

Fixes: #51145
Refs: #50684
PR-URL: #51146
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. benchmark Issues and PRs related to the benchmark subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants