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

Add NodeJS v20 script runtime #2479

Draft
wants to merge 11 commits into
base: master
Choose a base branch
from
Draft

Conversation

martonp96
Copy link

@martonp96 martonp96 commented Apr 23, 2024

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

  • Code compiles and has been tested successfully.
  • Code explains itself well and/or is documented.
  • My commit message explains what the changes do and what they are for.
  • No extra compilation warnings are added by these changes.

@github-actions github-actions bot added the triage Needs a preliminary assessment to determine the urgency and required action label Apr 23, 2024
{
SharedPushEnvironment pushed(this);

uv_run(m_uvLoop, UV_RUN_NOWAIT);
Copy link
Contributor

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.

Copy link
Contributor

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?

Copy link
Author

@martonp96 martonp96 Apr 25, 2024

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.

Copy link
Contributor

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)

Copy link
Author

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.

@thorium-cfx thorium-cfx added the ScRT: JS Issues/PRs related to the JavaScript scripting runtime label Apr 23, 2024
@blattersturm

This comment was marked as outdated.

@SymbianMoe
Copy link

finally <3

@Yum1x
Copy link

Yum1x commented May 5, 2024

@martonp96 do you have plans to implement Node 22? it'll also be LTS

@martonp96
Copy link
Author

@martonp96 do you have plans to implement Node 22? it'll also be LTS

@Yum1x yes, its planned when it becomes LTS officially

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ScRT: JS Issues/PRs related to the JavaScript scripting runtime triage Needs a preliminary assessment to determine the urgency and required action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants