-
Notifications
You must be signed in to change notification settings - Fork 5.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
feat(core): Deno.core.setNextTickCallback #12771
Conversation
scope.perform_microtask_checkpoint(); | ||
} | ||
|
||
// TODO(bartlomieju): Node also checks for absence of "rejection_to_warn" |
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.
Currently there's no handling of "unhandledRejection" in Deno so this conditional is skipped, I'm sure it will be problematic when trying to enable more of Node tests
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.
Side effect is that process.nextTick()
doesn't behave properly if error is thrown during its callback
I verified that this PR allows us to polyfill |
fn has_tick_scheduled( | ||
scope: &mut v8::HandleScope, | ||
_args: v8::FunctionCallbackArguments, | ||
mut rv: v8::ReturnValue, | ||
) { | ||
let state_rc = JsRuntime::state(scope); | ||
let state = state_rc.borrow(); | ||
rv.set(to_v8(scope, state.has_tick_scheduled).unwrap()); | ||
} |
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.
Node does this by means of an ArrayBuffer, to avoid (slow) JS -> C++ calls. Not a blocker, merely an observation.
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.
Agreed, let's address in a follow up
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.
Actually I'm confused here - it seems to me that it's done by having a TickInfo
bound from C++ to JS. Can you clarify what you meant?
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.
Node applies a trick where it makes a C struct available to JS land by using the struct's memory as a backing store for a typed array. That way C++ and JS land can communicate without expensive JS <-> C++ calls, they just read and write the struct's fields.
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 see, I'll be happy to explore this path, let's discuss it offline in details and I'll raise another PR
|
||
let js_macrotask_cb_handles = state.borrow().js_macrotask_cbs.clone(); |
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.
This also clones all the v8::Global handles in the vec? That's a fairly expensive operation.
Also, I think I'm missing where js_macrotask_cbs
is emptied. It looks like currently it only grows, never shrinks.
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.
Yeah I believe it does clone global handles, in practice there should be no more than two of them, but I can try not to clone them at all (however it's risky because I'd have to hold onto borrow of state and there's nothing preventing callback from using state as well, ergo we might run into BorrowError
).
Also, I think I'm missing where js_macrotask_cbs is emptied. It looks like currently it only grows, never shrinks.
Correct, but until this PR there was only a single macrotask callback allowed (used by timers), this PR changes it to allow multiple callbacks. In practice there will be one callback for timers, one for process.nextTick, there might be more of them if users pull multiple versions of deno_std
(and thus register multiple process.nextTick
callback.
return Ok(()); | ||
} | ||
|
||
let js_nexttick_cb_handles = state.borrow().js_nexttick_cbs.clone(); |
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.
Non-blocking observation: state.borrow()
gets called a lot. It's more efficient to borrow once and drop when necessary.
It's possible it doesn't matter in practice because the compiler "sees through" and elides borrow() calls.
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.
Please see comment above about risk of running into BorrowError
Co-authored-by: Ben Noordhuis <[email protected]>
See denoland#12771 and denoland#12814. Needed to reland denoland#12908, which was reverted in denoland#12929.
This commit adds several new "Deno.core" bindings:
Additionally it changes "Deno.core.setMacrotaskCallback" to
allow registering multiple callbacks. All these changes were necessary
to polyfill "process.nextTick" in Node compat layer.
Closes #12735