-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
src: rewrite task runner in c++ #52609
src: rewrite task runner in c++ #52609
Conversation
Review requested:
|
0b92919
to
558c595
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
src/node_task_runner.cc
Outdated
|
||
// Check if input contains any forbidden characters | ||
// If it doesn't, return the input as is. | ||
if (!std::regex_search(input, forbidden_characters)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can replace this for sure but the rest of it is going to be a problem
558c595
to
06f7a29
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, two issues (RE and mem leak pending)
06f7a29
to
8ead08d
Compare
8ead08d
to
720350a
Compare
I will review 'soon'. Please be patient. |
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 comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, my mistake! |
For |
…perator[] instead of find_field.
584122e
to
435aed9
Compare
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? |
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++.
The goal of Node.js task runner is not to replace
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.
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. |
This is kind of the policy for this specific feature then. Is it documented as the goal anywhere? |
@anonrig If someone wants to provide the equivalent |
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 |
@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 |
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.
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 |
@anonrig Let me try something. |
@lemire You're making this into something about my personal desires when it's not. I wrote:
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect to have a PR on your PR in a few minutes. But this can be merged as-is in my opinion.
Landed in c5cfdd4 |
PR-URL: nodejs#52609 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: nodejs#52609 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: #52609 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: nodejs#52609 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
PR-URL: nodejs#52609 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Daniel Lemire <daniel@lemire.me> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
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 atcctest/test_node_task_runner.cc
I'll investigate further after this pull-request to reduce the task runner to around ~5ms.
cc @nodejs/performance