deno icon indicating copy to clipboard operation
deno copied to clipboard

Published 20 hours ago • verified publisher icon denoland

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

Open andreubotella opened this issue 3 years ago • 9 comments

Part of #13239.

andreubotella avatar Sep 04 '22 03:09 andreubotella

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:

https://github.com/denoland/deno/blob/16dbf4adc390c9fb7656372b42811c1929e755dd/cli/tools/bench.rs#L407

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 avatar Sep 05 '22 03:09 andreubotella

#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 avatar Oct 16 '22 11:10 andreubotella

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 avatar Oct 17 '22 23:10 andreubotella

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 avatar Jan 17 '23 03:01 andreubotella

PTAL @bartlomieju @littledivy. Although this is probably not ready for merging, and the comments https://github.com/denoland/deno/pull/15760#issuecomment-1279953186 and https://github.com/denoland/deno/pull/15760#issuecomment-1281613105 are still things to fix, it would be good to get some initial reviews.

andreubotella avatar Jan 17 '23 04:01 andreubotella

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 avatar Jan 21 '23 13:01 bartlomieju

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 avatar Jan 31 '23 01:01 andreubotella

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.

andreubotella avatar Feb 05 '23 16:02 andreubotella

Let's wait on https://github.com/denoland/deno/pull/17648 before we merge this PR.

bartlomieju avatar Feb 06 '23 22:02 bartlomieju

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

andreubotella avatar Apr 08 '23 23:04 andreubotella

@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.

bartlomieju avatar Apr 10 '23 22:04 bartlomieju

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.

andreubotella avatar Apr 14 '23 22:04 andreubotella

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?

bartlomieju avatar Apr 14 '23 22:04 bartlomieju

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.

bartlomieju avatar Apr 14 '23 22:04 bartlomieju

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 avatar Apr 14 '23 22:04 andreubotella

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.

andreubotella avatar Apr 14 '23 22:04 andreubotella

@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.

bartlomieju avatar Apr 14 '23 23:04 bartlomieju

This has now landed on the deno_core repo. Closing.

andreubotella avatar Jul 24 '23 06:07 andreubotella