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

child_process: add fast path for spawn #57196

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

Conversation

ronag
Copy link
Member

@ronag ronag commented Feb 24, 2025

No description provided.

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run. labels Feb 24, 2025
@ronag
Copy link
Member Author

ronag commented Feb 24, 2025

@ronag
Copy link
Member Author

ronag commented Feb 25, 2025

Unrelated CI benchmark failure...

10:42:39 ../src/node_config_file.h:35:32: error: ‘simdjson’ has not been declared
10:42:39    35 |   ParseResult ParseNodeOptions(simdjson::ondemand::object* node_options_object);
10:42:39       |                                ^~~~~~~~
10:42:39 ../src/node_config_file.h:35:58: error: expected ‘,’ or ‘...’ before ‘*’ token
10:42:39    35 |   ParseResult ParseNodeOptions(simdjson::ondemand::object* node_options_object);
10:42:39       |                                                          ^
10:42:39 make[1]: *** [libnode.target.mk:510: /w/bnch-comp/node/out/Release/obj.target/libnode/src/node.o] Error 1

Who's a good ping on that? @nodejs/benchmarking

@atlowChemi
Copy link
Member

ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1661

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1662/

                                   confidence improvement accuracy (*)   (**)  (***)
child_process/spawn-echo.js n=1000        ***     16.87 %       ±1.27% ±1.69% ±2.21%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case, there are 1 comparisons, you can thus
expect the following amount of false-positive results:
  0.05 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.01 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

@atlowChemi atlowChemi added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 25, 2025
@aduh95
Copy link
Contributor

aduh95 commented Feb 25, 2025

nit: I find the commit message a bit confusing, our guidelines asks for the first word to be an imperative verb, which "spawn" is, but is not used as such here. My suggestion would be child_process: add fast path for `spawn`

@nodejs-github-bot
Copy link
Collaborator

@ronag ronag changed the title child_process: spawn fast path child_process: add fast path for spawn Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants