Reland "refactor(core): cleanup feature flags for js source inclusion" #19519

nayeemrmn · 2023-06-15T11:33:24Z

Relands #19463. This time the ExtensionFileSourceCode enum is preserved, so this effectively just splits feature include_js_for_snapshotting into exclude_js_sources and runtime_js_sources, adds a force_include_js_sources option on extension!(), and unifies ext::Init_ops_and_esm() and ext::init_ops() into ext::init().

…ag-cleanup-reland

mmastrac

Added a few comments -- will let @bartlomieju look at this before we go further on it but thanks for putting it back together with the changes I made to the runtime.

core/01_core.js

core/modules/loaders.rs

…ag-cleanup-reland

bartlomieju

Marking as "request changes" because we need a few more passes before landing.

bartlomieju · 2023-06-16T00:21:38Z

cli/build.rs

+ code: ExtensionFileSourceCode::LoadAtRuntime(
+ PathBuf::from("../runtime/js/99_main.js"),


Two things here: a) my opinion is that ExtensionFileSourceCode is super helpful and we should keep it because this immediately caught my attention; b) why is this file loaded at runtime and not snapshotted?

why is this file loaded at runtime and not snapshotted?

It's loaded at the 'runtime' of this build script, as it was before. I renamed the variant to match precisely what it does since it's a public API and can be used with extension builders or customizers. A name like LoadFromFsDuringSnapshot is counterintuitive because it might trick users into thinking its filtered out or otherwise guarded against when not snapshotting, whereas this is more cautionary to avoid using it outside of build scripts.

(FTR the first PR removed ExtensionFileSourceCode because there was no load-at-runtime case. It worked by just bundling everything since it only applied to build.rs contexts. I still don't know where the size increase came from 🙁)

I don't understand the size increase either but I guess the linker cannot prove the source wasn't used.

bartlomieju · 2023-06-16T00:22:51Z

core/00_primordials.js

+ // Provide bootstrap namespace
+ globalThis.__bootstrap ??= {};
+ const key = Symbol.for("00_primordials.js");
+ if (globalThis.__bootstrap[key]) {
+ return;
+ }
+ globalThis.__bootstrap[key] = true;


Do I get it right that this file can be actually executed multiple times when creating snapshots? Does it mean that all files that are snapshotted have this caveat or maybe it's only limited to "scripts" (ie. not ES modules).

Yeah, it's just a trade-off of unifying init_ops_and_esm() and init_ops(). ES modules already have identities attached to their specifiers, with just the minor change of making mod_evaluate() idempotent.

core/Cargo.toml

core/extensions.rs

core/inspector.rs

core/runtime/jsruntime.rs

runtime/js.rs

…ag-cleanup-reland

nayeemrmn · 2023-06-18T14:43:59Z

core/extensions.rs

- $(
- ext.esm(vec![ExtensionFileSource {
- specifier: "ext:setup",
- code: ExtensionFileSourceCode::IncludedInBinary($esm_setup_script),
- }]);
- )?


I've removed this feature since it's unused and doesn't make sense because it uses the same specifier and can't be used for more than one extension.

…ag-cleanup-reland

bartlomieju · 2023-06-20T21:38:30Z

@nayeemrmn we're OOO this week. I'll review your PR over the weekend.

bartlomieju

LGTM.

I'm gonna be watching benchmarks page after landing this PR to make sure there's no regression in the binary/snapshot size.

bartlomieju · 2023-06-25T07:32:42Z

core/runtime/jsruntime.rs

+ {
+ let module_map_rc = self.module_map.clone();
+ let module_map = module_map_rc.borrow();
+ let handle = module_map.handles.get(mod_id).unwrap().clone();
+ let mut scope = realm.handle_scope(self.v8_isolate());
+ let handle = v8::Local::new(&mut scope, handle);
+ if handle.get_status() == v8::ModuleStatus::Evaluated {
+ continue;
+ }


Much better :)

bartlomieju · 2023-06-25T08:50:14Z

@nayeemrmn there's still 750kB increase in the binary size :(

bartlomieju · 2023-06-25T09:39:54Z

I'm still trying to figure out what's going on here - all the files are marked as LoadAtRuntime and I don't see any file being include_str!() during the build process.

bartlomieju · 2023-06-25T10:39:16Z

I'm still debugging it, but I can definitely see all JavaScript sources loaded for snapshotting being included in the binary. Is there any other way they could have been included besides include_js_files!?

EDIT: I might have been wrong, opening deno (latest release) in binary editor I can see these files included as well - they are probably inside the snapshot. So probably a red herring.

However there's a difference - in latest release there appears to be 290 files included, while after this PR there are 308 files. So appears we have 18 duplicate files.

nayeemrmn · 2023-06-25T16:52:26Z

However there's a difference - in latest release there appears to be 290 files included, while after this PR there are 308 files. So appears we have 18 duplicate files.

I think you're using the debug binary to get 308. Comparing the canary releases for a181ceb and 28a4f3d, I get 291 both before and after (a file was added in #19223). I assume we're both searching with the copyright header.

Last thing I can think of is the duplicate calls to load_side_module() on cli snapshot creation which ends up calling .instantiate_module() for each module handle again. It shouldn't do anything for a kEvaluated module but who knows.
Edit: Nope not this.

bartlomieju · 2023-06-26T09:33:26Z

I think you're using the debug binary to get 308. Comparing the canary releases for a181ceb and 28a4f3d, I get 291 both before and after (a file was added in #19223). I assume we're both searching with the copyright header.

Yikes, that might have been it. I compared it to the latest released version. And yes, I was looking at the copyright headers in the produced binary.

Last thing I can think of is the duplicate calls to load_side_module() on cli snapshot creation which ends up calling .instantiate_module() for each module handle again. It shouldn't do anything for a kEvaluated module but who knows.
Edit: Nope not this.

That's a good pointer, but I believe V8 is smart enough, not to create a duplicate module entries.

Let's wait a few hours before we dig into it more - I think we will abandon the idea of not snapshotting ext/node (because of some other work and decisions). While I think this PR cleans up a lot, the fact that we can't establish where this binary increase is coming from is really worrisome.

…nclusion" (denoland#19519)" This reverts commit 28a4f3d.

#19611) …nclusion" (#19519)" This reverts commit 28a4f3d. This change causes failures when used outside Deno repo: ``` ============================================================ Deno has panicked. This is a bug in Deno. Please report this at https://github.com/denoland/deno/issues/new. If you can reliably reproduce this panic, include the reproduction steps and re-run with the RUST_BACKTRACE=1 env var set and include the backtrace in your report. Platform: linux x86_64 Version: 1.34.3+b37b286 Args: ["/opt/hostedtoolcache/deno/0.0.0-b37b286f7fa68d5656f7c180f6127bdc38cf2cf5/x64/deno", "test", "--doc", "--unstable", "--allow-all", "--coverage=./cov"] thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Failed to read "/home/runner/work/deno/deno/core/00_primordials.js" Caused by: No such file or directory (os error 2)', core/runtime/jsruntime.rs:699:8 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ```

nayeemrmn added 2 commits June 15, 2023 12:27

Reland "refactor(core): cleanup feature flags for js source inclusion"

14442ed

Merge remote-tracking branch 'upstream/main' into snapshot-feature-fl…

7b5ff8c

…ag-cleanup-reland

mmastrac reviewed Jun 15, 2023

View reviewed changes

core/01_core.js Outdated Show resolved Hide resolved

core/modules/loaders.rs Show resolved Hide resolved

nayeemrmn added 5 commits June 15, 2023 18:00

Merge remote-tracking branch 'upstream/main' into snapshot-feature-fl…

e58e028

…ag-cleanup-reland

use filename symbol on repeat check

415bfe5

Merge remote-tracking branch 'upstream/main' into snapshot-feature-fl…

7b3af29

…ag-cleanup-reland

fmt

da3377f

Merge remote-tracking branch 'upstream/main' into snapshot-feature-fl…

66802ce

…ag-cleanup-reland

bartlomieju requested changes Jun 16, 2023

View reviewed changes

nayeemrmn added 11 commits June 16, 2023 15:32

Merge remote-tracking branch 'upstream/main' into snapshot-feature-fl…

9300fd1

…ag-cleanup-reland

restore SOURCE_CODE_FOR_99_MAIN_JS and PATH_FOR_99_MAIN_JS

6c48156

better docstring for features

1d86e79

add comment in mod_evaluate()

6ecc32c

revert to IncludedInBinary

94ce243

init() -> init_ext()

71e8365

Merge remote-tracking branch 'upstream/main' into snapshot-feature-fl…

a125942

…ag-cleanup-reland

no redundant run_event_loop()

96f8867

reset ci

88a0906

esm_files and js_files are non-optional

6bf03c7

remove esm_setup_script

eccd307

nayeemrmn commented Jun 18, 2023

View reviewed changes

nayeemrmn added 2 commits June 19, 2023 22:58

Merge remote-tracking branch 'upstream/main' into snapshot-feature-fl…

79328ac

…ag-cleanup-reland

revert idempotent mod_evaluate()

494600b

bartlomieju added 2 commits June 25, 2023 08:50

Merge branch 'main' into snapshot-feature-flag-cleanup-reland

3f67076

update test

1e4d9f9

bartlomieju approved these changes Jun 25, 2023

View reviewed changes

bartlomieju merged commit 28a4f3d into denoland:main Jun 25, 2023
10 checks passed

nayeemrmn deleted the snapshot-feature-flag-cleanup-reland branch June 25, 2023 15:23

bartlomieju added a commit to bartlomieju/deno that referenced this pull request Jun 26, 2023

Revert "Reland "refactor(core): cleanup feature flags for js source i…

be9eb86

…nclusion" (denoland#19519)" This reverts commit 28a4f3d.

bartlomieju mentioned this pull request Jun 26, 2023

Revert "Reland "refactor(core): cleanup feature flags for js source i… #19611

Merged

This was referenced Jul 1, 2023

refactor(core): don't use extension macro for core js #19616

Merged

fix: ignore already evaluated extension modules denoland/deno_core#7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reland "refactor(core): cleanup feature flags for js source inclusion" #19519

Reland "refactor(core): cleanup feature flags for js source inclusion" #19519

nayeemrmn commented Jun 15, 2023

mmastrac left a comment

bartlomieju left a comment

bartlomieju Jun 16, 2023

nayeemrmn Jun 16, 2023

mmastrac Jun 16, 2023

bartlomieju Jun 16, 2023

nayeemrmn Jun 16, 2023

nayeemrmn Jun 18, 2023

bartlomieju commented Jun 20, 2023

bartlomieju left a comment

bartlomieju Jun 25, 2023

bartlomieju commented Jun 25, 2023

bartlomieju commented Jun 25, 2023

bartlomieju commented Jun 25, 2023 •

edited

Loading

nayeemrmn commented Jun 25, 2023 •

edited

Loading

bartlomieju commented Jun 26, 2023

		code: ExtensionFileSourceCode::LoadAtRuntime(
		PathBuf::from("../runtime/js/99_main.js"),

Reland "refactor(core): cleanup feature flags for js source inclusion" #19519

Reland "refactor(core): cleanup feature flags for js source inclusion" #19519

Conversation

nayeemrmn commented Jun 15, 2023

mmastrac left a comment

Choose a reason for hiding this comment

bartlomieju left a comment

Choose a reason for hiding this comment

bartlomieju Jun 16, 2023

Choose a reason for hiding this comment

nayeemrmn Jun 16, 2023

Choose a reason for hiding this comment

mmastrac Jun 16, 2023

Choose a reason for hiding this comment

bartlomieju Jun 16, 2023

Choose a reason for hiding this comment

nayeemrmn Jun 16, 2023

Choose a reason for hiding this comment

nayeemrmn Jun 18, 2023

Choose a reason for hiding this comment

bartlomieju commented Jun 20, 2023

bartlomieju left a comment

Choose a reason for hiding this comment

bartlomieju Jun 25, 2023

Choose a reason for hiding this comment

bartlomieju commented Jun 25, 2023

bartlomieju commented Jun 25, 2023

bartlomieju commented Jun 25, 2023 • edited Loading

nayeemrmn commented Jun 25, 2023 • edited Loading

bartlomieju commented Jun 26, 2023

bartlomieju commented Jun 25, 2023 •

edited

Loading

nayeemrmn commented Jun 25, 2023 •

edited

Loading