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

tools: skip common extensions on js2c #52211

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

anonrig
Copy link
Member

@anonrig anonrig commented Mar 25, 2024

Reduces system calls on js2c.cc for IsDirectory function by eliminating .js and .mjs extensions.

cc @nodejs/cpp-reviewers

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Mar 25, 2024
@anonrig anonrig force-pushed the improve-js2c branch 2 times, most recently from d5ed5f8 to 05fb9a2 Compare March 25, 2024 16:51
tools/js2c.cc Outdated Show resolved Hide resolved
tools/js2c.cc Outdated Show resolved Hide resolved
@anonrig anonrig added the request-ci Add this label to start a Jenkins CI on a PR. label Mar 25, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Mar 25, 2024
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

What happens if someone accidentally create a directory that ends with .js or .mjs? Does this actually make enough difference in the build time to stop guarding against it?

@anonrig
Copy link
Member Author

anonrig commented Mar 29, 2024

What happens if someone accidentally create a directory that ends with .js or .mjs? Does this actually make enough difference in the build time to stop guarding against it?

I think it's highly unlikely for someone to create a folder ending with .js, but I see your point. How can I benchmark this @joyeecheung @lemire? Any suggestions?

@lemire
Copy link
Member

lemire commented Mar 30, 2024

I would benchmark IsDirectory. It should be 'easy' to write a benchmark.

Here is what I would in practical terms.

  1. Compare head-to-head with Bun and Deno. If everything is right, then they should all performance more or less the same.
  2. Do syscall tracing (e.g.. dtruss under macOS). They should all do the same syscalls.

Since Node uses C++17, you can just do std::filesystem::is_directory. This may or may not be a good idea.

@joyeecheung
Copy link
Member

joyeecheung commented Mar 31, 2024

It seems to be an overkill microbenchmarking a utility that’s only used at build time? Shouldn’t the only metrics that matter be the % of build time this reduces?

Note that, this was ported from a >10x slower Python script that had been used for many years and even then nobody really cared how slow it was….(and I think during development many people just use the configure option to load the scripts from disk, and skip JS2C entirely)

@joyeecheung
Copy link
Member

joyeecheung commented Mar 31, 2024

Also to benchmark it, I would just

git checkout pr_branch
./configure --ninja
ninja -C out/Release node
rm out/Release/gen/node_javascript.cc
time ninja -C out/Release node # data point 1
git checkout main
rm out/Release/gen/node_javascript.cc
time ninja -C out/Release node # data point 2

(Using make works too. Just make sure that you don't set --node-builtin-modules-path when you configure, because that just let the build skips JS2C entirely...even though quite a few contributors use this, I believe).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants