deno
deno copied to clipboard
denoland
feat(core): Add support for async ops in realms
This is one proposal to support async ops in realms, that does not need the promise ring to be moved to Rust (see #14396). I will also submit another PR that does depend on the Rust promise ring, so they can be compared.
Pull request #14019 enabled initial support for realms, but it did not include support for async ops anywhere other than the main realm. The main issue was 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 creates a ContextState struct, similar to JsRuntimeState but stored in a slot of each v8::Context, which contains a js_recv_cb callback for each realm. Combined with a new list of known realms, which stores them as v8::Weak<v8::Context>, and a change in the #[op] macro to pass the current context to queue_async_op, this makes it possible to send the results of promises for different realms to their 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.
Towards #13239.
Hi! We have some interest in this feature. Can we help with it somehow?
Hi. Sorry for the late reply. I'm currently working on this on my spare time, and I've been a bit busy for the last two weeks.
I wonder what is driving your interest in this PR. Is it having ShadowRealm available in Deno, or are you users of deno_core and want to be able to use contexts for some reason? In either case, there is still some work needed before contexts in deno_core are completely usable, unless you don't care at all about making ESM imports possible outside the main realm (see #13239). And I guess some of the bullet points in that issue might be done in parallel by someone else.
I wonder what is driving your interest in this PR. Is it having
ShadowRealmavailable in Deno, or are you users ofdeno_coreand want to be able to use contexts for some reason? In either case, there is still some work needed before contexts indeno_coreare completely usable, unless you don't care at all about making ESM imports possible outside the main realm (see #13239). And I guess some of the bullet points in that issue might be done in parallel by someone else.
Hi, Andreu. We are users of deno_core and would like to execute code concurrently in the same Isolate without sharing the same global object between executions. ESM imports aren't necessary for our use case. We can use deno_core as it is today, but it would be nice to use contexts to further insulate the executions without using an Isolate.
As you, I have been busy. I wanted to try out your PR and write some tests, but didn't have time to do it yet.
If you get around to start writing tests at some point, please comment so in this issue. I will as well, if I get to it first.
Ciao, Andreu. I made an async version of the op sync test at the end of the runtime.rs and found that the create_realm() needed a call to init_cbs().
It must exist a better way to get the result of the promise returned by the async function in the test, but I'm not that savvy to find it. If you incorporate the patch, please feel free to refactor it.
realm_async_test_and_init_cbs.patch.txt
An amend: init_cbs() indirectly uses v8__Isolate__GetCurrentContext() to obtain the context, which I understand isn't necessarily the context belonging to the Realm parameter. Am I wrong?
realm_async_test_and_init_cbs.patch.1.txt
An amend:
init_cbs()indirectly usesv8__Isolate__GetCurrentContext()to obtain the context, which I understand isn't necessarily the context belonging to the Realm parameter. Am I wrong?
I applied the patch. Thank you so much!
~~Edit: I also added this test to #14396, which is an alternative way of adding support for async ops in realms.~~
I've started working on module support for realms, which will be a follow-up to this PR, and I noticed that it's quite inconvenient to have the return value of JsRealm::state be a reference tied to the isolate's lifetime. I'll change it to return Rc<RefCell<ContextState>> instead.
Benchmarks (cpu: AMD Ryzen 7 3700X 8-Core Processor):
main:
file:///home/abotella/Projects/forks/denoland/deno/cli/bench/deno_common.js
benchmark time (avg) (min … max) p75 p99 p995
------------------------------------------------------- -----------------------------
date_now 37.16 ns/iter (35.67 ns … 98.89 ns) 38.37 ns 70.47 ns 71.59 ns
op_void_sync 80.46 ns/iter (77.34 ns … 101.59 ns) 81.77 ns 91.95 ns 92.34 ns
op_void_async 649.62 ns/iter (253.54 ns … 2.65 µs) 935.34 ns 2.65 µs 2.65 µs
perf_now 194.76 ns/iter (183.3 ns … 388.77 ns) 190.45 ns 240.05 ns 245.63 ns
url_parse 1.78 µs/iter (1.72 µs … 1.96 µs) 1.84 µs 1.96 µs 1.96 µs
blob_text_large 1.76 ms/iter (236.38 µs … 17.08 ms) 1.48 ms 9.19 ms 9.2 ms
b64_rt_long 1.72 ms/iter (778.1 µs … 16.59 ms) 1.34 ms 12.45 ms 13.74 ms
b64_rt_short 440.37 ns/iter (410.71 ns … 499.22 ns) 451.91 ns 497.76 ns 499.22 ns
read_zero 501.1 ns/iter (487.09 ns … 512.16 ns) 503.62 ns 510.15 ns 512.16 ns
write_null 432.93 ns/iter (427.15 ns … 442.55 ns) 435.22 ns 441.32 ns 442.55 ns
read_128k 11 µs/iter (6.17 µs … 14.94 µs) 13.43 µs 14.94 µs 14.94 µs
request_new 8.53 µs/iter (4.21 µs … 25.77 µs) 10.36 µs 25.77 µs 25.77 µs
This branch:
file:///home/abotella/Projects/forks/denoland/deno/cli/bench/deno_common.js
benchmark time (avg) (min … max) p75 p99 p995
------------------------------------------------------- -----------------------------
date_now 37.64 ns/iter (36.1 ns … 98.12 ns) 38.99 ns 50.76 ns 72.33 ns
op_void_sync 84.08 ns/iter (80.02 ns … 100.01 ns) 84.41 ns 94.67 ns 95.5 ns
op_void_async 695.95 ns/iter (308.96 ns … 2.92 µs) 922.92 ns 2.92 µs 2.92 µs
perf_now 187.88 ns/iter (180.14 ns … 857.81 ns) 187.34 ns 193.64 ns 217.28 ns
url_parse 1.83 µs/iter (1.78 µs … 1.96 µs) 1.88 µs 1.96 µs 1.96 µs
blob_text_large 1.77 ms/iter (288.81 µs … 21.97 ms) 1.47 ms 10.57 ms 14.31 ms
b64_rt_long 1.26 ms/iter (781.4 µs … 12.38 ms) 1.32 ms 7.24 ms 10.68 ms
b64_rt_short 428.21 ns/iter (410.16 ns … 485.48 ns) 430.14 ns 469.79 ns 485.48 ns
read_zero 523.04 ns/iter (515.93 ns … 561.51 ns) 524.76 ns 537.23 ns 561.51 ns
write_null 445.97 ns/iter (439.23 ns … 452.66 ns) 449.33 ns 451.98 ns 452.66 ns
read_128k 12.59 µs/iter (2.31 µs … 17.3 ms) 6.01 µs 112.86 µs 129.97 µs
request_new 4.43 µs/iter (2.64 µs … 11.76 ms) 3.07 µs 45.7 µs 73.79 µs
I also benched a patch on this branch that avoids iterating through all contexts in JsRuntime::resolve_async_ops by hackily building a hashmap of contexts (hackily since v8::Context doesn't impl Hash). I didn't expect to see a big difference since all of these benchmarks deal with a single realm, but still:
file:///home/abotella/Projects/forks/denoland/deno/cli/bench/deno_common.js
benchmark time (avg) (min … max) p75 p99 p995
------------------------------------------------------- -----------------------------
date_now 37.36 ns/iter (35.95 ns … 117.49 ns) 38.8 ns 48.75 ns 56.74 ns
op_void_sync 82.47 ns/iter (79.55 ns … 98.2 ns) 82.7 ns 94.89 ns 95.96 ns
op_void_async 701.87 ns/iter (316.81 ns … 2.79 µs) 870.42 ns 2.79 µs 2.79 µs
perf_now 185.66 ns/iter (165.17 ns … 1.15 µs) 171.88 ns 408.93 ns 931.32 ns
url_parse 1.77 µs/iter (1.72 µs … 1.85 µs) 1.81 µs 1.85 µs 1.85 µs
blob_text_large 1.67 ms/iter (180.34 µs … 17.36 ms) 1.46 ms 10.03 ms 17.03 ms
b64_rt_long 1.26 ms/iter (776.49 µs … 13.03 ms) 1.33 ms 7.98 ms 11.43 ms
b64_rt_short 415.37 ns/iter (401.51 ns … 471 ns) 424.77 ns 450.14 ns 471 ns
read_zero 501.5 ns/iter (494.34 ns … 524.79 ns) 504.31 ns 508.84 ns 524.79 ns
write_null 472.82 ns/iter (459.55 ns … 529.2 ns) 474.77 ns 500.39 ns 529.2 ns
read_128k 13.66 µs/iter (8.05 µs … 24.52 µs) 15.35 µs 24.52 µs 24.52 µs
request_new 16.98 µs/iter (2.73 µs … 41.29 ms) 4.89 µs 60.14 µs 250.56 µs
(On this last one, request_new seems to be quite variable, but running the benchmark various times, it seems like the average is usually around 6µs, rather than 17.)
Should I run other benchmarks?
cc @bartlomieju
@andreubotella thanks for running these benchmarks. It appears there's no noticeable perf hit with this PR. I'll try to review and land for v1.25
