-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add NodeJS v20 script runtime #2479
base: master
Are you sure you want to change the base?
Conversation
{ | ||
SharedPushEnvironment pushed(this); | ||
|
||
uv_run(m_uvLoop, UV_RUN_NOWAIT); |
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.
Dropping the EnvironmentScope changeset here and using a nested libuv loop here seems risky - this would lead to any libuv callbacks being delayed by the server main tick rate, which could slow down any nested IO operations by a lot. I'm hoping that you folks have deliberated this and are certain you wish to go this way.
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.
What is the need for a libuv loop here?
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.
@blattersturm
Indeed, thanks for pointing it out. I added a workaround for this case, however i'm not too happy about this solution. In general an optimal scenario is when workarounds are not needed, neither in the component nor within the nodejs src. Hopefully in the future we can work on this.
@thorium-cfx
Node has its own libuv and v8 dependencies built with the binary, in order to have it easier from the embedder side, we have to rely on those instead of external ones like before, thats why its there.
E: there is still a lot of testing left before it can go to production, of course more problems can come up with this approach, but speaking from past experience, it should be mostly fine.
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.
A "high frequency tick method" dedicated to pumping every Node resource's uv loop (even worse if it also runs the main scheduler tick!) is not the way to go here either. People at times run hundreds of resources, and this runs counter to e.g. the tickless work done prior, even though this was meant to focus on client performance, the same would apply to server-side overhead.
Tight coupling to reduce ScRT enumeration/tick overhead still adds per-runtime cost, makes a mess of the code, and still adds timer overhead/delay since event signaling is not timer-bound at all. There really isn't any way to cheap out of something like the EnvironmentScope stuff, unless upstream nowadays has something like this via e.g. async hooks. If maintaining this patch set is problematic, consider suggesting this use case to upstream Node.js maintainers instead, given it's a common embedder use case to wish to embed in an existing libuv event loop (and the uv dependency isn't the painful one to wish to sync with upstream- it's the v8 one).
Perhaps a cheap trick could be to somehow hoist events from the Node.js event loop into the server event loop and wrap callbacks that way rather than patching Node.js to enter/exit environments cleanly on callbacks.
(moved here from a top-level comment)
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.
For this approach, i'm entirely relying on past experience, where it just works fine with a high amount of resources, even though most gamemode logic was on server side. Since using this ScRT is optional, i would like to see if this punishes performance with real world examples while in the meantime we can work on alternative solutions like the ones you recommended.
This comment was marked as outdated.
This comment was marked as outdated.
finally <3 |
…on use the old node runtime
…node16 instead of node_rt
@martonp96 do you have plans to implement Node 22? it'll also be LTS |
@Yum1x yes, its planned when it becomes LTS officially |
Goal of this PR
The goal of this PR is to add a new NodeJS v20 script runtime, so developers can experiment and migrate their server scripts to a newer version, enabling them to have the up-to-date NodeJS experience.
How is this PR achieving the goal
By creating a new server side component, so old and new node runtimes can work independently side by side.
Most of the code is reused from the old runtime in a well organized manner.
This PR applies to the following area(s)
Server, ScRT: Node
Successfully tested on
Windows, Linux
Platforms: Windows, Linux
Checklist