feat(core): Add support for modules in realms. #15760

andreubotella · 2022-09-04T03:32:28Z

andreubotella · 2022-09-05T03:51:48Z

With these changes, deno bench is now hanging forever after the benchmarks are run. This seems to be because the code that drives the benchmarks waits on a mpsc receiver until all senders have been dropped, and one of them is being kept alive:

deno/cli/tools/bench.rs

Line 407 in 16dbf4a

while let Some(event) = receiver.recv().await {

That one sender is being kept alive because now a realm's module map is stored in the context slot (rather than in the isolate slot), and because of denoland/rusty_v8#1066 it ends up not getting dropped even after the isolate is dropped.

Edit (October 8th): This is fixed now.

andreubotella · 2022-10-16T11:47:33Z

#16286 changed the module map, that was in a slot in the isolate backed by the IsolateAnnex, to use a raw V8 slot in the isolate. But this PR changes the module map to be a slot in the context's annex, so some of the perf gains from that PR will regress. But I expect #16215 to land before this PR, after which the module map could be stored in a raw slot in the context.

andreubotella · 2022-10-17T23:10:31Z

While starting to look into writing tests for this, I realized that in this PR I'm using a single ModuleLoader instance for all of an isolate's realms. There module maps are still separate per realm, even if they share a loader, but since ModuleLoader implementations might have state, we might need to consider the implications. If we add an Option<Fn() -> Rc<dyn ModuleLoader>> field to RuntimeOptions (plus the current module_loader field for the main realm), we would also let embedders disallow module imports in ShadowRealms, which might be worth considering. And the returned module loaders might be references to the main module loader, of course.

andreubotella · 2023-01-17T03:51:16Z

This PR got stuck for a while while most of the realm code got reverted because it was blocking performance work. All of the PRs that were preconditions for modules have now been relanded, so I have rewritten this PR from scratch and will continue to work on it now.

andreubotella · 2023-01-17T04:19:08Z

PTAL @bartlomieju @littledivy. Although this is probably not ready for merging, and the comments #15760 (comment) and #15760 (comment) are still things to fix, it would be good to get some initial reviews.

bartlomieju · 2023-01-21T13:23:55Z

PTAL @bartlomieju @littledivy. Although this is probably not ready for merging, and the comments #15760 (comment) and #15760 (comment) are still things to fix, it would be good to get some initial reviews.

Sorry Andreu, I was out for most of the week. I'll put it into my TODO list for the next week and provide initial review.

bartlomieju

To be honest I'm not sure why #16215 got closed without a merge. @littledivy could you give a rationale?

bartlomieju · 2023-01-30T21:20:30Z

core/modules.rs

+    global_realm
+      .instantiate_module(runtime.v8_isolate(), mod_b)
+      .unwrap();


A lot of these changes seem superficial - how about we still keep all of these methods on the JsRuntime and they still internally call into the "global realm"? I believe in 99% cases we won't be handling multiple realms and it just makes the common path more complicated.

Using the global realm when a different realm should be used might lead to crossing objects from the global realm into the ShadowRealm, or vice versa, allowing prototype pollution across the ShadowRealm boundary. Requiring these methods (as well as handle_scope, execute_script, etc.) to be accessed through a JsRealm forces you to think about which realm you need to work with, and also draws reviewers' attention to it.

Using the global realm when a different realm should be used might lead to crossing objects from the global realm into the ShadowRealm, or vice versa

How is that possible? I thought that if you try to act on an object from a different context you'd get a V8 panic

You get a panic for different isolates, not contexts. Although V8 keeps track of in which context an object was created, this isn't really a thing in the spec, and using an object from another context is something you can do just fine in the browser with iframes (iframe.contentWindow.document), or in Node.js's vm module (vm.runInNewContext("({})")).

Okay, seems like a valid reason to have separate API then.

bartlomieju

Sorry for slow review @andreubotella.

The question about ModuleLoader is an interesting one and I don't have a good answer right now. Does spec say anything about it?

core/runtime.rs

bartlomieju

Also we need to figure out what we're gonna do with snapshotting. I think for the first pass we should not care about additional realms and just make sure that snapshotting still works with a "global realm"

andreubotella · 2023-01-31T01:53:44Z

The question about ModuleLoader is an interesting one and I don't have a good answer right now. Does spec say anything about it?

The HTML spec requires a level of "caching" in module fetches between a page's main and shadow realms, such that if a module is imported in the main realm it should not be re-fetched when imported from the shadow realm, and vice versa. I haven't looked at this code in a long time, but last time I checked Deno did some caching for module sources that would already take care of this.

But as I was looking at the spec, I realized that import maps were recently merged into the HTML spec, and it's not clear if anyone has looked in any depth at how they interact with ShadowRealm. Merging the HTML PR as is would mean that import maps don't apply in ShadowRealms, which seems reasonable, but I asked in a comment just in case. In this case, shadow realms would need a different module loader from the main realm, since they'd have to have a flag to not use import maps.

So I guess we should probably change RuntimeOptions to have a module loader for the main realm, and one for all shadow realms. Since they're Option<Rc<dyn ModuleLoader>>, both can be references to the same module loader object, for embedders that don't support import maps.

Also we need to figure out what we're gonna do with snapshotting. I think for the first pass we should not care about additional realms and just make sure that snapshotting still works with a "global realm"

WFM

andreubotella · 2023-02-05T16:22:44Z

Since JsRuntime::create_realm can be used to create "regular" (non-shadow) realms, for example if we ever support the Node.js vm module, I thought it was better to require the realm creator to provide a module loader. Then in #16211 (when I find time to rebase that PR) we can have a module loader that gets used for every shadow reallm.

bartlomieju · 2023-02-06T22:43:41Z

Let's wait on #17648 before we merge this PR.

This will help make reviews easier for denoland#15760, which moves a number of methods related to module loading from `JsRuntime` into `JsRealm`.

This will help make reviews easier for #15760, which moves a number of methods related to module loading from `JsRuntime` into `JsRealm`.

This change makes realms other than the main one support modules by having a module map associated to each realm, rather than one per module. As part of this, it also: - Adds an argument to `JsRuntime::create_realm` to set the realm's module loader. This allows different realms to have different module loading strategies (different import maps, for example). - Moves all of the methods of `JsRuntime` related to module loading into `JsRealm`. To minimize changing unrelated code, the public and crate-private methods in `JsRuntime` are kept, with their implementation replaced by a call to the corresponding method of the main realm's `JsRealm`. - Removes the `module_map` argument to the `EventLoopPendingState` constructor, instead accessing each realm's corresponding module map as part of the existing iteration. - Changes the parts of `JsRuntime::poll_event_loop` that deal with module evaluation and detecting stalled top-level awaits to support multiple module maps and multiple top-level module evaluations at the same time. - Moves `pending_mod_evaluate` and `pending_dyn_mod_evaluate` from `JsRuntimeState` to `ContextState`. Towards #13239.

andreubotella · 2023-04-08T23:50:55Z

I think this PR is now ready for review. The commit message contains a somewhat detailed list of changes.

I thought changing the module map to be stored on a context slot would make things slower, but the basic exec_time benchmarks don't show a difference. @littledivy, are there benchmarks specific to module loading that I should be running instead?

Main branch:

Benchmark 1: /home/abotella/Projects/forks/denoland/deno/target/release/deno run --reload cli/tests/testdata/run/002_hello.ts 
  Time (mean ± σ):      18.1 ms ±   0.9 ms    [User: 11.2 ms, System: 7.9 ms]
  Range (min … max):    16.9 ms …  23.3 ms    157 runs

Benchmark 2: /home/abotella/Projects/forks/denoland/deno/target/release/deno run --reload cli/tests/testdata/run/003_relative_import.ts
  Time (mean ± σ):      17.8 ms ±   0.6 ms    [User: 11.2 ms, System: 7.7 ms]
  Range (min … max):    17.1 ms …  21.0 ms    156 runs
 
Benchmark 3: /home/abotella/Projects/forks/denoland/deno/target/release/deno run cli/tests/testdata/run/002_hello.ts
  Time (mean ± σ):      18.0 ms ±   1.3 ms    [User: 11.1 ms, System: 8.0 ms]
  Range (min … max):    16.9 ms …  27.8 ms    156 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
Benchmark 4: /home/abotella/Projects/forks/denoland/deno/target/release/deno run cli/tests/testdata/run/003_relative_import.ts
  Time (mean ± σ):      18.1 ms ±   1.0 ms    [User: 11.1 ms, System: 8.1 ms]
  Range (min … max):    17.1 ms …  25.8 ms    159 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
Benchmark 5: /home/abotella/Projects/forks/denoland/deno/target/release/deno run cli/tests/testdata/run/error_001.ts ; test $? -eq 1
  Time (mean ± σ):      18.3 ms ±   0.7 ms    [User: 11.1 ms, System: 8.2 ms]
  Range (min … max):    17.3 ms …  20.7 ms    156 runs
 
Benchmark 6: /home/abotella/Projects/forks/denoland/deno/target/release/deno run --reload --no-check cli/tests/testdata/run/002_hello.ts 
  Time (mean ± σ):      18.7 ms ±   1.2 ms    [User: 11.7 ms, System: 8.1 ms]
  Range (min … max):    17.0 ms …  28.6 ms    152 runs
 
Benchmark 7: /home/abotella/Projects/forks/denoland/deno/target/release/deno run --allow-read cli/tests/testdata/workers/bench_startup.ts
  Time (mean ± σ):     732.8 ms ±  10.0 ms    [User: 600.6 ms, System: 182.6 ms]
  Range (min … max):   720.6 ms … 754.7 ms    10 runs

Benchmark 8: /home/abotella/Projects/forks/denoland/deno/target/release/deno run --allow-read cli/tests/testdata/workers/bench_round_robin.ts 
  Time (mean ± σ):     173.4 ms ±   8.8 ms    [User: 367.1 ms, System: 51.8 ms]
  Range (min … max):   146.8 ms … 183.6 ms    16 runs

This PR:

Benchmark 1: /home/abotella/Projects/forks/denoland/deno/target/release/deno run --reload cli/tests/testdata/run/002_hello.ts 
  Time (mean ± σ):      18.5 ms ±   2.2 ms    [User: 11.4 ms, System: 8.2 ms]
  Range (min … max):    16.8 ms …  34.5 ms    155 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Benchmark 2: /home/abotella/Projects/forks/denoland/deno/target/release/deno run --reload cli/tests/testdata/run/003_relative_import.ts 
  Time (mean ± σ):      18.1 ms ±   0.6 ms    [User: 11.0 ms, System: 8.2 ms]
  Range (min … max):    17.1 ms …  20.7 ms    159 runs
 
Benchmark 3: /home/abotella/Projects/forks/denoland/deno/target/release/deno run cli/tests/testdata/run/002_hello.ts 
  Time (mean ± σ):      17.9 ms ±   0.8 ms    [User: 11.1 ms, System: 7.9 ms]
  Range (min … max):    16.9 ms …  24.4 ms    159 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
Benchmark 4: /home/abotella/Projects/forks/denoland/deno/target/release/deno run cli/tests/testdata/run/003_relative_import.ts 
  Time (mean ± σ):      18.2 ms ±   1.0 ms    [User: 11.0 ms, System: 8.3 ms]
  Range (min … max):    17.1 ms …  23.1 ms    160 runs
 
Benchmark 5: /home/abotella/Projects/forks/denoland/deno/target/release/deno run cli/tests/testdata/run/error_001.ts ; test $? -eq 1
  Time (mean ± σ):      18.3 ms ±   0.8 ms    [User: 10.9 ms, System: 8.4 ms]
  Range (min … max):    17.3 ms …  24.8 ms    149 runs
 
  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet system without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.
 
Benchmark 6: /home/abotella/Projects/forks/denoland/deno/target/release/deno run --reload --no-check cli/tests/testdata/run/002_hello.ts 
  Time (mean ± σ):      17.9 ms ±   0.5 ms    [User: 11.1 ms, System: 7.9 ms]
  Range (min … max):    17.2 ms …  20.2 ms    153 runs
 
Benchmark 7: /home/abotella/Projects/forks/denoland/deno/target/release/deno run --allow-read cli/tests/testdata/workers/bench_startup.ts 
  Time (mean ± σ):     731.0 ms ±   5.8 ms    [User: 604.0 ms, System: 179.8 ms]
  Range (min … max):   719.9 ms … 739.5 ms    10 runs
 
Benchmark 8: /home/abotella/Projects/forks/denoland/deno/target/release/deno run --allow-read cli/tests/testdata/workers/bench_round_robin.ts 
  Time (mean ± σ):     174.2 ms ±   5.7 ms    [User: 375.9 ms, System: 53.5 ms]
  Range (min … max):   162.1 ms … 185.3 ms    16 runs

bartlomieju · 2023-04-10T22:38:03Z

@andreubotella looking at the benchmarks you provided there's about 0.3ms regression in the startup benchmark - it's something we'll need to address before landing. I'm gonna give a thorough review this week. Thanks for factoring out parts of core/runtime.rs to core/realm.rs. It makes it much easier to review.

This will help make reviews easier for #15760, which moves a number of methods related to module loading from `JsRuntime` into `JsRealm`.

bartlomieju

This looks great Andreu. I'm still worried about the regression in startup time benchmarks and I believe we should address it before landing.

Could you remind me of the reason why we need global_realm to be an Option? If we removed the Option we could get rid of a lot of clones.

bartlomieju · 2023-04-14T21:54:25Z

core/runtime.rs

@@ -628,6 +599,15 @@ impl JsRuntime {
        })),
      );

+      // TODO(andreubotella): Set up ExtModuleLoader.


Does that TODO mean that not internal code in currently available in realms?

I haven't tested ESM internal code for non-main realms at all. I was planning to delve into that after this PR landed.

Sounds good - I think it would be as simple as checking is Deno.core.getPromiseDetails is defined

bartlomieju · 2023-04-14T21:54:35Z

core/runtime.rs

+        self.op_state(),
+        self.snapshot_options == snapshot_util::SnapshotOptions::Load,
+      )));
+      // TODO(andreubotella): Snapshotted data?


What do you mean by this TODO?

I'm talking about these lines as the module map is being initialized in the JsRuntime constructor:

deno/core/runtime.rs

Lines 513 to 518 in 4317055

if let Some(snapshotted_data) = maybe_snapshotted_data {

let scope =

&mut v8::HandleScope::with_context(&mut isolate, global_context);

let mut module_map = module_map_rc.borrow_mut();

module_map.update_with_snapshotted_data(scope, snapshotted_data);

}

Right, so without it I believe Deno.core.* APIs won't be available

bartlomieju · 2023-04-14T21:55:36Z

core/runtime.rs

      snapshot_util::set_snapshotted_data(
        &mut scope,
-        context,
+        realm.context().clone(),


I'm a bit worried about all the clones that are happening when accessing context. But let's address that in a follow up

core/runtime.rs

andreubotella · 2023-04-14T22:17:50Z

Could you remind me of the reason why we need global_realm to be an Option? If we removed the Option we could get rid of a lot of clones.

Only because state is created before the isolate, so you can't populate that field at that point.

bartlomieju · 2023-04-14T22:18:59Z

Could you remind me of the reason why we need global_realm to be an Option? If we removed the Option we could get rid of a lot of clones.

Only because state is created before the isolate, so you can't populate that field at that point.

Maybe we could put an empty v8::Global there?

…documentation

bartlomieju · 2023-04-14T22:33:10Z

Could you remind me of the reason why we need global_realm to be an Option? If we removed the Option we could get rid of a lot of clones.

Only because state is created before the isolate, so you can't populate that field at that point.

Maybe we could put an empty v8::Global there?

Appears that's currently not possible, because v8::Global APIs still require a valid Isolate handle. I'll think about it more.

andreubotella · 2023-04-14T22:44:07Z

Appears that's currently not possible, because v8::Global APIs still require a valid Isolate handle. I'll think about it more.

I think I'd be fine with initializing global_realm to MaybeUninit::zeroed().assume_init(). This would need care when setting it, since we don't want to call the Drop impl of v8::Global.

andreubotella · 2023-04-14T22:50:07Z

Appears that's currently not possible, because v8::Global APIs still require a valid Isolate handle. I'll think about it more.

I think I'd be fine with initializing global_realm to MaybeUninit::zeroed().assume_init(). This would need care when setting it, since we don't want to call the Drop impl of v8::Global.

Oh, never mind, that by itself would be undefined behavior since v8::Global contains a NonNull field.

bartlomieju · 2023-04-14T23:51:55Z

@piscisaureus suggested other solution. I will test it first on current main branch to see if it's feasible to use and let you know.

andreubotella · 2023-07-24T06:36:28Z

This has now landed on the deno_core repo. Closing.

This was referenced Sep 19, 2022

Context slots are never dropped if there are Globals to the context when the isolate is dropped denoland/rusty_v8#1066

Closed

feat: Guaranteed finalizers denoland/rusty_v8#1076

Merged

andreubotella force-pushed the modules-in-realms branch from bae389b to eb0b5f4 Compare October 8, 2022 11:32

This was referenced Sep 18, 2024

Support for realms/contexts in deno_core (in preparation for ShadowRealm) denoland/deno_core#911

Open

[WIP] Implement the ShadowRealm constructor #16211

Draft

[wip] perf(runtime): use raw v8 slots #16215

Closed

andreubotella force-pushed the modules-in-realms branch from 630db54 to 8bd63f1 Compare January 17, 2023 03:44

andreubotella mentioned this pull request Jan 18, 2023

feat: ES module snapshotting #17460

Merged

bartlomieju reviewed Jan 30, 2023

View reviewed changes

core/runtime.rs Outdated Show resolved Hide resolved

core/runtime.rs Outdated Show resolved Hide resolved

core/runtime.rs Outdated Show resolved Hide resolved

bartlomieju reviewed Jan 30, 2023

View reviewed changes

andreubotella mentioned this pull request Feb 22, 2023

[Feature/Question] Reuse JsRuntime and MainWorker, or create a new v8::Isolate after running execute_script #17861

Closed

andreubotella force-pushed the modules-in-realms branch from e101c91 to 3f7efd2 Compare April 8, 2023 04:41

andreubotella mentioned this pull request Apr 8, 2023

cleanup(core): Move JsRealm into a separate module #18641

Merged

bartlomieju pushed a commit that referenced this pull request Apr 8, 2023

cleanup(core): Move JsRealm into a separate module (#18641)

8c4f37d

This will help make reviews easier for #15760, which moves a number of methods related to module loading from `JsRuntime` into `JsRealm`.

andreubotella force-pushed the modules-in-realms branch from 9bc24b0 to a3cfbf2 Compare April 8, 2023 16:14

andreubotella marked this pull request as ready for review April 8, 2023 16:15

bartlomieju requested a review from crowlKats April 10, 2023 22:36

bartlomieju requested review from bartlomieju and littledivy April 10, 2023 22:36

levex pushed a commit that referenced this pull request Apr 12, 2023

cleanup(core): Move JsRealm into a separate module (#18641)

c7c439c

This will help make reviews easier for #15760, which moves a number of methods related to module loading from `JsRuntime` into `JsRealm`.

Merge branch 'main' into modules-in-realms

68963cb

bartlomieju reviewed Apr 14, 2023

View reviewed changes

andreubotella added 2 commits April 15, 2023 00:29

Add CreateRealmOptions

fa76954

Remove mention of module_loader from the JsRuntime::create_realm …

e451c24

…documentation

Fix doc test

d846e09

nayeemrmn mentioned this pull request May 23, 2023

refactor(core): remove ext: modules from the module map #19040

Merged

andreubotella mentioned this pull request Jul 17, 2023

feat(realms): Add support for modules in realms. denoland/deno_core#41

Merged

andreubotella closed this Jul 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core): Add support for modules in realms. #15760

feat(core): Add support for modules in realms. #15760

andreubotella commented Sep 4, 2022 •

edited

Loading

andreubotella commented Sep 5, 2022 •

edited

Loading

andreubotella commented Oct 16, 2022

andreubotella commented Oct 17, 2022 •

edited

Loading

andreubotella commented Jan 17, 2023

andreubotella commented Jan 17, 2023 •

edited

Loading

bartlomieju commented Jan 21, 2023

bartlomieju left a comment

bartlomieju Jan 30, 2023

andreubotella Jan 30, 2023

bartlomieju Jan 30, 2023

andreubotella Jan 31, 2023 •

edited

Loading

bartlomieju Jan 31, 2023

bartlomieju left a comment

bartlomieju left a comment

andreubotella commented Jan 31, 2023 •

edited

Loading

andreubotella commented Feb 5, 2023

bartlomieju commented Feb 6, 2023

andreubotella commented Apr 8, 2023 •

edited

Loading

bartlomieju commented Apr 10, 2023

bartlomieju left a comment

bartlomieju Apr 14, 2023

andreubotella Apr 14, 2023

bartlomieju Apr 14, 2023

bartlomieju Apr 14, 2023

andreubotella Apr 14, 2023

bartlomieju Apr 14, 2023

bartlomieju Apr 14, 2023

andreubotella commented Apr 14, 2023

bartlomieju commented Apr 14, 2023

bartlomieju commented Apr 14, 2023

andreubotella commented Apr 14, 2023

andreubotella commented Apr 14, 2023 •

edited

Loading

bartlomieju commented Apr 14, 2023

andreubotella commented Jul 24, 2023

	if let Some(snapshotted_data) = maybe_snapshotted_data {
	let scope =
	&mut v8::HandleScope::with_context(&mut isolate, global_context);
	let mut module_map = module_map_rc.borrow_mut();
	module_map.update_with_snapshotted_data(scope, snapshotted_data);
	}

feat(core): Add support for modules in realms. #15760

feat(core): Add support for modules in realms. #15760

Conversation

andreubotella commented Sep 4, 2022 • edited Loading

andreubotella commented Sep 5, 2022 • edited Loading

andreubotella commented Oct 16, 2022

andreubotella commented Oct 17, 2022 • edited Loading

andreubotella commented Jan 17, 2023

andreubotella commented Jan 17, 2023 • edited Loading

bartlomieju commented Jan 21, 2023

bartlomieju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreubotella Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

andreubotella commented Jan 31, 2023 • edited Loading

andreubotella commented Feb 5, 2023

bartlomieju commented Feb 6, 2023

andreubotella commented Apr 8, 2023 • edited Loading

bartlomieju commented Apr 10, 2023

bartlomieju left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreubotella commented Apr 14, 2023

bartlomieju commented Apr 14, 2023

bartlomieju commented Apr 14, 2023

andreubotella commented Apr 14, 2023

andreubotella commented Apr 14, 2023 • edited Loading

bartlomieju commented Apr 14, 2023

andreubotella commented Jul 24, 2023

andreubotella commented Sep 4, 2022 •

edited

Loading

andreubotella commented Sep 5, 2022 •

edited

Loading

andreubotella commented Oct 17, 2022 •

edited

Loading

andreubotella commented Jan 17, 2023 •

edited

Loading

andreubotella Jan 31, 2023 •

edited

Loading

andreubotella commented Jan 31, 2023 •

edited

Loading

andreubotella commented Apr 8, 2023 •

edited

Loading

andreubotella commented Apr 14, 2023 •

edited

Loading