feat(core): Reland support for async ops in realms #17204
Merged
Conversation
This file contains 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
Currently realms are supported on `deno_core`, but there was no support for async ops anywhere other than the main realm. The main issue is that the `js_recv_cb` callback, which resolves promises corresponding to async ops, was only set for the main realm, so async ops in other realms would never resolve. Furthermore, promise ID's are specific to each realm, which meant that async ops from other realms would result in a wrong promise from the main realm being resolved. This change takes the `ContextState` struct added in denoland#17050, and adds to it a `js_recv_cb` callback for each realm. Combined with the fact that that same PR also added a list of known realms to `JsRuntimeState`, and that denoland#17174 made `OpCtx` instances realm-specific and had them include an index into that list of known realms, this makes it possible to know the current realm in the `queue_async_op` and `queue_fast_async_op` methods, and therefore to send the results of promises for each realm to that realm, and prevent the ID's from getting mixed up. Additionally, since promise ID's are no longer unique to the isolate, having a single set of unrefed ops doesn't work. This change therefore also moves `unrefed_ops` from `JsRuntimeState` to `ContextState`, and adds the lengths of the unrefed op sets for all known realms to get the total number of unrefed ops to compare in the event loop. This PR is a reland of denoland#14734 after it was reverted in denoland#16366, except that `ContextState` and `JsRuntimeState::known_realms` were previously relanded in denoland#17050. Another significant difference with the original PR is passing around an index into `JsRuntimeState::known_realms` instead of a `v8::Global<v8::Context>` to identify the realm, because async op queuing in fast calls cannot call into V8, and therefore cannot have access to V8 globals. This also simplified the implementation of `resolve_async_ops`. Co-authored-by: Luis Malheiro <[email protected]>
Open
13 tasks
|
main: this PR: There seems to be a clear decrease in performance in this benchmark. Now, |
|
I got rid of a |
|
PTAL @bartlomieju @littledivy |
littledivy
reviewed
Jan 5, 2023
littledivy
reviewed
Jan 6, 2023
|
@littledivy this looks good to me now - I think having a separate code path for non-default realm is a good idea. |
|
Oooops, looks like #17301 causes the tests to fail, could you update them @andreubotella? |
bartlomieju
pushed a commit
that referenced
this pull request
Jan 16, 2023
Currently realms are supported on `deno_core`, but there was no support for async ops anywhere other than the main realm. The main issue is that the `js_recv_cb` callback, which resolves promises corresponding to async ops, was only set for the main realm, so async ops in other realms would never resolve. Furthermore, promise ID's are specific to each realm, which meant that async ops from other realms would result in a wrong promise from the main realm being resolved. This change takes the `ContextState` struct added in #17050, and adds to it a `js_recv_cb` callback for each realm. Combined with the fact that that same PR also added a list of known realms to `JsRuntimeState`, and that #17174 made `OpCtx` instances realm-specific and had them include an index into that list of known realms, this makes it possible to know the current realm in the `queue_async_op` and `queue_fast_async_op` methods, and therefore to send the results of promises for each realm to that realm, and prevent the ID's from getting mixed up. Additionally, since promise ID's are no longer unique to the isolate, having a single set of unrefed ops doesn't work. This change therefore also moves `unrefed_ops` from `JsRuntimeState` to `ContextState`, and adds the lengths of the unrefed op sets for all known realms to get the total number of unrefed ops to compare in the event loop. This PR is a reland of #14734 after it was reverted in #16366, except that `ContextState` and `JsRuntimeState::known_realms` were previously relanded in #17050. Another significant difference with the original PR is passing around an index into `JsRuntimeState::known_realms` instead of a `v8::Global<v8::Context>` to identify the realm, because async op queuing in fast calls cannot call into V8, and therefore cannot have access to V8 globals. This also simplified the implementation of `resolve_async_ops`. Co-authored-by: Luis Malheiro <[email protected]>
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.
Currently realms are supported on
deno_core, but there was no support for async ops anywhere other than the main realm. The main issue is that thejs_recv_cbcallback, which resolves promises corresponding to async ops, was only set for the main realm, so async ops in other realms would never resolve. Furthermore, promise ID's are specific to each realm, which meant that async ops from other realms would result in a wrong promise from the main realm being resolved.This change takes the
ContextStatestruct added in #17050, and adds to it ajs_recv_cbcallback for each realm. Combined with the fact that that same PR also added a list of known realms toJsRuntimeState, and that #17174 madeOpCtxinstances realm-specific and had them include an index into that list of known realms, this makes it possible to know the current realm in thequeue_async_opandqueue_fast_async_opmethods, and therefore to send the results of promises for each realm to that realm, and prevent the ID's from getting mixed up.Additionally, since promise ID's are no longer unique to the isolate, having a single set of unrefed ops doesn't work. This change therefore also moves
unrefed_opsfromJsRuntimeStatetoContextState, and adds the lengths of the unrefed op sets for all known realms to get the total number of unrefed ops to compare in the event loop.This PR is a reland of #14734 after it was reverted in #16366, except that
ContextStateandJsRuntimeState::known_realmswere previously relanded in #17050. Another significant difference with the original PR is passing around an index intoJsRuntimeState::known_realmsinstead of av8::Global<v8::Context>to identify the realm, because async op queuing in fast calls cannot call into V8, and therefore cannot have access to V8 globals. This also simplified the implementation ofresolve_async_ops.cc @lmalheiro, since the this reland includes a test they provided for the original PR.