src: rewrite task runner in c++

[anonrig]

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

[nodejs-github-bot]

Review requested:

• @nodejs/gyp
• @nodejs/startup

[mcollina]

lgtm

[joyeecheung]

std::regex has a bit of problem in terms of platform support and performance when I ported js2c to C++. I'd suggest just write a wrapper operating on the bytes as you find the characters, or you can use std::string::includes and std::string::search if the performance of this doesn't matter.

[anonrig]

I can replace this for sure but the rest of it is going to be a problem

[benjamingr]

Mostly LGTM, two issues (RE and mem leak pending)

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/58650/

[lemire]

@anonrig

I will review 'soon'.

Please be patient.

[GeoffreyBooth]

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?

[GeoffreyBooth]

this is about the task runner, not the test runner

Sorry, my mistake!

[GeoffreyBooth]

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.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/58850/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/58852/

[voxpelli]

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?

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/58862/

[anonrig]

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.

[voxpelli]

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?

[anonrig]

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: #52190.

[lemire]

@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 ?

[voxpelli]

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 --run in 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

[voxpelli]

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 ?

@lemire Isn't that what the implementation in lib/internal/main/run.js essentially already provides?

And npm etc of course already provides it.

Question is what is best for the node.js project. Essentially: Taking more of an undici approach or this more tightly integrated approach.

Speaking as a co-maintainer of npm-run-all2, it could for sure be intriguing if eg. it could use the same core run logic as here and possibly also if there would be a route to possibly upstream parts of that to the node.js task runner if there would be a wider community interest in that. This PR essentially shuts the door on both of those.

[lemire]

@voxpelli

Isn't that what the implementation in lib/internal/main/run.js essentially already provides?

I think you may not have examined @anonrig's work fully. It was always motivated by performance and contains significant C++ code (prior to this PR). You are just pointing a part of the implementation that was in JavaScript.

Speaking as a co-maintainer of npm-run-all2, it could for sure be intriguing if eg. it could use the same core run logic as here and possibly also if there would be a route to possibly upstream parts of that to the node.js task runner if there would be a wider community interest in that. This PR essentially shuts the door on both of those

Here is how the functionality has been submitted...

The purpose of this pull-request to offer a fast alternative to npm run xxx. With this pull-request, node supports node run test which executes test command inside package.json scripts.

Just so we are clear, are we in agreement that npm run xxx remains? That it can be further expanded and that you can seek to contribute to npm run xxx if you so desire, with or without this PR? That node --run xxx as proposed as a fast alternative?

[lemire]

@anonrig Let me try something.

[voxpelli]

Here is how the functionality has been submitted...

The purpose of this pull-request to offer a fast alternative to npm run xxx. With this pull-request, node supports node run test which executes test command inside package.json scripts.

Just so we are clear, are we in agreement that npm run xxx remains? That it can be further expanded and that you can seek to contribute to npm run xxx if you so desire, with or without this PR? That node --run xxx as proposed as a fast alternative?

@lemire You're making this into something about my personal desires when it's not.

I wrote:

Question is what is best for the node.js project. Essentially: Taking more of an undici approach or this more tightly integrated approach.

I'm confident that @anonrig has understood my point though so any further discussion on this topic from me will happen outside of this issue when/if/where there's a broader discussion on this topic and how it aligns with current or future goals and needs for Node.js as a project. I do hope such a discussion happen and I would gladly be invited to or involved in it.

[benjamingr]

I understand both sentiments (contribution should be easy, performance should be good). In this particular feature I think that since this dramatically effects the startup of every process lunched through the task runner performance triumphs.

[lemire]

I expect to have a PR on your PR in a few minutes. But this can be merged as-is in my opinion.

[nodejs-github-bot]

Landed in c5cfdd4