node
node copied to clipboard
nodejs
src: rewrite task runner in c++
This is a rewrite of the task runner in C++. The benchmark speaks for themselves with a caveat of removing support for --env-file and related CLI flags in the task runner. I think the performance can be improved further more since somehow we still interact with V8.
While moving the implementation to C++, I've moved the tests for escapeShell to C++ as well, since we can't write a unit test running on JS side anymore. (Ref: please take a look at cctest/test_node_task_runner.cc
I'll investigate further after this pull-request to reduce the task runner to around ~5ms.
❯ hyperfine '../node/main-branch --run test' '../node/cpp-rewrite --run test' 'npm run test' -i
Benchmark 1: ../node/main-branch --run test
Time (mean ± σ): 28.9 ms ± 0.9 ms [User: 24.2 ms, System: 3.4 ms]
Range (min … max): 27.5 ms … 31.7 ms 96 runs
Warning: Ignoring non-zero exit code.
Benchmark 2: ../node/cpp-rewrite --run test
Time (mean ± σ): 18.3 ms ± 0.6 ms [User: 16.0 ms, System: 1.5 ms]
Range (min … max): 17.5 ms … 20.8 ms 139 runs
Warning: Ignoring non-zero exit code.
Benchmark 3: npm run test
Time (mean ± σ): 148.8 ms ± 2.0 ms [User: 131.9 ms, System: 22.1 ms]
Range (min … max): 145.3 ms … 154.5 ms 19 runs
Warning: Ignoring non-zero exit code.
Summary
../node/cpp-rewrite --run test ran
1.58 ± 0.07 times faster than ../node/main-branch --run test
8.14 ± 0.29 times faster than npm run test
cc @nodejs/performance
This should’ve had the @nodejs/test_runner team tagged.
I assume this change is fine, and desirable, but I’d like to hear from them what it might mean for future development of the test runner. Would moving the core (or more than the core) of the test runner into C++ mean that developing new test runner features would become difficult or impossible for the current test runner contributors? Or even if it would, maybe that’s okay since the test runner is close to feature complete by this point?
This should’ve had the @nodejs/test_runner team tagged.
I assume this change is fine, and desirable, but I’d like to hear from them what it might mean for future development of the test runner. Would moving the core (or more than the core) of the test runner into C++ mean that developing new test runner features would become difficult or impossible for the current test runner contributors? Or even if it would, maybe that’s okay since the test runner is close to feature complete by this point?
This was also my immediate thought. Given the current rate of change, I'm concerned this optimization will slow down new feature development too much, while this is the first time I hear a need for more performance.
Edit: this is about the task runner, not the test runner
this is about the task runner, not the test runner
Sorry, my mistake!
For node --run, I kind of feel like it's important to support --env-file? It's a common use case to define environment variables for running scripts or starting servers.
Failed to start CI
⚠ Something was pushed to the Pull Request branch since the last approving review. ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/8824696454
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
@anonrig Linting issue?
09:43:24 + make lint-py-build PYTHON=python3
09:43:25 Pip installing ruff on Python 3.8.0...
09:43:25 python3 -m pip install --upgrade --target tools/pip/site-packages ruff==0.3.4 || \
09:43:25 python3 -m pip install --upgrade --system --target tools/pip/site-packages ruff==0.3.4
09:43:26 Collecting ruff==0.3.4
09:43:26 Using cached ruff-0.3.4-py3-none-manylinux_2_17_x86_64.manylinux2014_x86_64.whl (8.7 MB)
09:43:26 Installing collected packages: ruff
09:43:27 Successfully installed ruff-0.3.4
09:43:27
09:43:27 [notice] A new release of pip is available: 23.1.2 -> 24.0
09:43:27 [notice] To update, run: python3 -m pip install --upgrade pip
09:43:27 + make lint-ci PYTHON=python3
09:43:27 Running JS linter...
09:46:09 Makefile:1397: recipe for target 'lint-js-ci' failed
09:46:09 make: *** [lint-js-ci] Error 1
09:46:09 + cat test-eslint.tap
09:46:09 + grep -v '^ok\|^TAP version 13\|^1\.\.'
09:46:09 + sed '/^\s*$/d'
09:46:09 not ok 796 - /home/iojs/build/workspace/node-test-linter/lib/internal/vm/module.js
09:46:09 ---
09:46:09 message: Out of ASCIIbetical order - PromiseResolve >= PromisePrototypeThen
09:46:09 severity: error
09:46:09 data:
09:46:09 line: 16
09:46:09 column: 3
09:46:09 ruleId: node-core/alphabetize-primordials
09:46:09 ...
09:46:09 + exit 1
09:46:10 Build step 'Execute shell' marked build as failure
09:46:10 Performing Post build task...
09:46:10 Match found for : : True
09:46:10 Logical operation result is TRUE
09:46:10 Running script : #/bin/bash
09:46:10
09:46:10 set -x
09:46:10 mkdir out
09:46:10 tap2junit -i test-eslint.tap -o out/test.xml
09:46:10 [node-test-linter] $ /bin/sh -xe /tmp/jenkins13065436921414028231.sh
09:46:10 + set -x
09:46:10 + mkdir out
09:46:10 + tap2junit -i test-eslint.tap -o out/test.xml
09:46:10 POST BUILD TASK : SUCCESS
09:46:10 END OF POST BUILD TASK : 0
09:46:10 Recording test results
09:46:11 [Checks API] No suitable checks publisher found.
09:46:11 Collecting metadata...
09:46:11 Metadata collection done.
09:46:11 Notifying upstream projects of job completion
09:46:11 Finished: FAILURE
@anonrig Linting issue?
@lemire Yes, but the Windows issue seems to be real. I guess it's time for me to setup the Windows machine...
Maybe we should rename it script runner if this keeps getting happening 😅
Maybe we should rename it script runner if this keeps getting happening 😅
Yeah or just refer to it as node --run.
Is performance really the bottle neck of the task runner?
Putting code into Node.js core makes it harder to contribute to by people only familiar with contributing to the wider npm module ecosystem.
Converting the code within Node.js core to C++ makes it even harder by requiring knowledge in C++ as well.
Is there a policy decision or similar that performance is preferable over maintainability?
Is performance really the bottle neck of the task runner?
Task runner currently takes 30ms. Before that, it took 200ms (using npm). The goal of implementing it on Node.js core was due to performance.
Currently, out of 30ms, only 5ms is lost on the actual child process, but the rest, 83%, is spent on non-task runner related things such as initializing V8. In order to reduce this unnecessary cost, we have to implement it in C++.
Putting code into Node.js core makes it harder to contribute to by people only familiar with contributing to the wider npm module ecosystem.
The goal of Node.js task runner is not to replace npm or npm module ecosystem. It's merely a helper cli function to achieve what a subset of npm run does, but in a really performant and fast way.
Converting the code within Node.js core to C++ makes it even harder by requiring knowledge in C++ as well.
You are indeed correct, but it's almost impossible to contribute to Node.js without writing (or at least understanding) Node.js, due to the internals written in C++. Hence, C++ knowledge is required at some level.
Is there a policy decision or similar that performance is preferable over maintainability?
There is no policy decision regarding this. Let's not forget that the goal of porting Node.js task runner into Node.js core is performance, not maintenance.
Let's not forget that the goal of porting Node.js task runner into Node.js core is performance, not maintenance.
This is kind of the policy for this specific feature then. Is it documented as the goal anywhere?
This is kind of the policy for this specific feature then. Is it documented as the goal anywhere?
@voxpelli We don't document feature specific goals anywhere. The original PR which added node --run in the first place shows my intentions and my goals: https://github.com/nodejs/node/pull/52190.
@anonrig If someone wants to provide the equivalent node --run using pure JavaScript, with no concern for performance, is it not the case that it can be done, without even contributing directly to Node.js ?
This is kind of the policy for this specific feature then. Is it documented as the goal anywhere?
We don't document feature specific goals anywhere. The original PR which added
node --runin the first place shows my intentions and my goals: #52190.
It would have been good because then you could have pointed me to such a goal, or I could have found it myself, and there would be no need for a discussion 🙂
For future reference: Lots of discussions on this topic is happening on Twitter in the threads that followed https://x.com/yagiznizipli/status/1785999524143464681?s=46&t=1mQKe1AKaQ-2YwRjxDfrOg