Repository navigation
fix(scripting/node): run promise continuations after events and ref calls - #4404
Open
OneEarJoe5 wants to merge 1 commit into
Open
OneEarJoe5 wants to merge 1 commit into
OneEarJoe5 wants to merge 1 commit into
Conversation
…alls Events and ref calls from other resources call straight into the runtime, not through its uv loop, so the microtasks and nextTicks they queue only ran on the next resource tick. Every `await` on an event or a callback from another resource (e.g. an oxmysql query from JS) took ~50 ms. Mark the runtime after an event or ref call and run those queued continuations in TickFast, which already runs every millisecond, inside a node::CallbackScope like node does after its own callbacks. That happens outside of any JS, so nothing runs in the middle of other code, and callers still get to attach their handlers to a returned promise first. While the profiler records, they get their own "callbacks" scope for the resource, since they no longer run inside its tick. Signed-off-by: OneEarJoe5 <212275319+OneEarJoe5@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Goal of this PR
In server-side JS, every
awaiton something that comes back from another resource takes ~50 ms, one server frame. That's any event from another resource and any callback/export from another resource, e.g.awaitonexports.oxmysql.query(...)from a JS resource, or a Lua resource calling back into JS. The other resource answers right away, but the promise continuation only runs on our next tick.Events and ref calls go straight into the runtime (
TriggerEvent/CallRef), not through its uv loop. The context's microtask queue is explicit, so the continuations they queue sit there until something drains it, which is the resource tick (process._tickCallback()intimer.js) or the next uv callback, if there is one. I mentioned this one in #4402, that PR doesn't change it.How is this PR achieving the goal
TriggerEventandCallRefset a flag on the runtime after the call.TickFast, which already runs every millisecond for every node runtime (UvLoopTimer), checks the flag after itsuv_runand, if set, opens and closes anode::CallbackScope. That's what node does after its own callbacks: it runs the microtasks and theprocess.nextTickqueue and reports unhandled rejections.callbacks (<resource>), instead of disappearing from recordings.I first tried running them right when the outermost event/ref call returns. That gave false unhandled rejection warnings for async callbacks, because the caller only attaches its handler to the returned promise after
CallRefreturns. Doing it inTickFastavoids that and costs at most ~1 ms.This PR applies to the following area(s)
Server, ScRT: JS
Successfully tested on
Platforms: Windows (server), Linux (server, WSL2 Ubuntu 24.04)
Windows: server built from current master (5412ed8), with and without this change. Linux: before = the official 38621 artifact (built from that same commit), after = this branch built with the alpine builder from
code/tools/ci/docker-builder. Test resources: one JS resource that answers from its own event loop (like a driver after I/O,timers.setImmediatethenemit/ calling the callback), one Lua resource that answers from its scheduler, one JS resource that awaits all of it, plus oxmysql 2.14.1 against a local MariaDB on Windows.awaitan event answered by another JS resourceawaita callback passed to another JS resource's exportawaita callback a Lua resource calls from its scheduler (from the call to the continuation)SELECT 1from JSnextTickinside itprofiler record, 5 ms of work in each continuationtick (mttest)callbacks (mttest)(252 ms for 50 × 5 ms)CPU cost, process CPU time (
process.cpuUsage), 3 runs × 5 rounds each on an otherwise idle machine:Also ran my JS timer test resource from #4402 (timers, errors, natives, profiler, resource restart from outside, heap) and txAdmin 8.1.2 starting and running the server with these resources: unchanged, no heartbeat problems.
The client isn't affected:
citizen-scripting-nodeis server-only.Not tested: C# resources calling back into JS (same
CallRefpath, I just didn't write a C# test resource).Checklist