refactor(core): cleanup feature flags for js source inclusion #19463

nayeemrmn · 2023-06-11T15:29:30Z

Remove ExtensionFileSourceCode::LoadedFromFsDuringSnapshot and feature include_js_for_snapshotting since they leak paths that are only applicable in this repo to embedders. Replace with feature exclude_js_sources. Additionally the feature force_include_js_sources allows negating it, if both features are set. We need both of these because features are additive and there must be a way of force including sources for snapshot creation while still having the exclude_js_sources feature. force_include_js_sources is only set for build deps, so sources are still excluded from the final binary.

You can also specify force_include_js_sources on any extension to override the above features for that extension. Towards #19398.

But there was still the snapshot-from-snapshot situation where code could be executed twice, I addressed that by making mod_evaluate() and scripts like core/01_core.js behave idempotently. This allowed unifying ext::init_ops() and ext::init_ops_and_esm() into ext::init().

…e-per-crate

ext/broadcast_channel/lib.rs

…e-per-crate

mmastrac

LGTM with latest changes. Much cleaner.

bartlomieju

I'm late to the party, but I have a few questions. Overall I will revert this PR because it causes 3.5Mb regression in the binary size, because the source code is included in the binary for snapshotted code.

bartlomieju · 2023-06-13T22:02:40Z

core/inspector.rs

+          PollState::Parked => {
+            w.parked_thread.replace(thread::current());
+          }


Why was this change needed?

I meant to inquire if that was correct but forgot.. There were some inspector test failures on fc0f65a because that variant was being hit now for some reason. That fixed it. But I don't know what changed there.

Huh, interesting! I would be interested in fixing this for sure, any chance you could try to create a test that would hit this code path? Inspector is quite brittle and esoteric bit of code so I would encourage a lot of caution when changing its logic.

any chance you could try to create a test that would hit this code path?

It's hit by the test failures on https://github.com/denoland/deno/actions/runs/5242046717/jobs/9464946684, caused by something in the rest of this PR.

core/modules.rs

bartlomieju · 2023-06-13T22:03:39Z

Cargo.lock

@@ -949,6 +949,7 @@ dependencies = [
 "anyhow",
 "bytes",
 "deno_ast",
+ "deno_core",


I wonder if this will cause problems during publishing... I never seen crate depend on itself

It's a dev dependency to turn the force-include feature on for testing.

bartlomieju · 2023-06-13T22:05:27Z

core/extensions.rs

-          std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR")).join($file)
-        ),
-      },)+
+        code: include_str!($file),


This will cause the source code to be included in the resulting binary even though it's snapshotted. Which is visible in the benchmark page https://deno.com/benchmarks for binary size

It's supposed to work because no ExtensionFileSources are registered in the final binary in the first place, they are replaced by an empty vec in include_js_files!(). I debug printed the esm files for extension builders after the ext.esm() call and they were all empty. But something apparently went wrong...

One difference this PR makes is that the static sources are included in the compilation of the build.rs scripts, where as before they were loaded at runtime of the build.rs. That shouldn't make a difference should it?

One difference this PR makes is that the static sources are included in the compilation of the build.rs scripts, where as before they were loaded at runtime of the build.rs. That shouldn't make a difference should it?

I would assume that would be the case, I can't say for sure off the top of my head.

bartlomieju · 2023-06-13T22:06:13Z

core/runtime.rs

+    if status == v8::ModuleStatus::Evaluated {
+      let (sender, receiver) = oneshot::channel();
+      sender.send(Ok(())).unwrap();
+      return receiver;
+    }
+    if status == v8::ModuleStatus::Errored {
+      let (sender, receiver) = oneshot::channel();
+      let exception = module.get_exception();
+      sender
+        .send(exception_to_err_result(tc_scope, exception, false))
+        .unwrap();
+      return receiver;
+    }


I really don't like this bit - while it might be fine for our own internal code, this will hide problems in module loading logic for user code

…denoland#19463)" This reverts commit ceb03cf.

bartlomieju · 2023-06-13T22:09:40Z

Overall I like this cleanup - it would be ideal to have only a single method for defining extensions, but we need stricter control using cargo flags to decide if something should be included or not. I'm working on #19399 which also touches this subject, I will raise an alternative PR (which will probably build on this PR).

@nayeemrmn

#19490) … (#19463)" This reverts commit ceb03cf. This is being reverted because it causes 3.5Mb increase in the binary size, due to runtime JS code being included in the binary, even though it's already snapshotted. CC @nayeemrmn

bartlomieju · 2023-06-13T23:51:13Z

@nayeemrmn I suggest we land this PR in more bite-sized pieces - I like the idea of having only ext::init() and what is does is controlled via Cargo flags. Can you revert changes to ExtensionFileSourceCode::LoadedFromFsDuringSnapshot so we don't regress on the binary size?

Remove `ExtensionFileSourceCode::LoadedFromFsDuringSnapshot` and feature `include_js_for_snapshotting` since they leak paths that are only applicable in this repo to embedders. Replace with feature `exclude_js_sources`. Additionally the feature `force_include_js_sources` allows negating it, if both features are set. We need both of these because features are additive and there must be a way of force including sources for snapshot creation while still having the `exclude_js_sources` feature. `force_include_js_sources` is only set for build deps, so sources are still excluded from the final binary. You can also specify `force_include_js_sources` on any extension to override the above features for that extension. Towards #19398. But there was still the snapshot-from-snapshot situation where code could be executed twice, I addressed that by making `mod_evaluate()` and scripts like `core/01_core.js` behave idempotently. This allowed unifying `ext::init_ops()` and `ext::init_ops_and_esm()` into `ext::init()`.

@nayeemrmn

#19490) … (#19463)" This reverts commit ceb03cf. This is being reverted because it causes 3.5Mb increase in the binary size, due to runtime JS code being included in the binary, even though it's already snapshotted. CC @nayeemrmn

#19519) 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()`.

nayeemrmn added 5 commits June 11, 2023 16:14

refactor: exclude extension js sources per-crate

9823e1c

remove drop check

f330ea6

always include js for tests

fb83473

force include js sources in dev deps

1e6cde9

remove debug and redundant dev dep

fc0f65a

nayeemrmn marked this pull request as draft June 12, 2023 10:10

nayeemrmn added 2 commits June 12, 2023 11:38

why

3a95ea7

Merge remote-tracking branch 'upstream/main' into extension-js-exclud…

11a5f12

…e-per-crate

nayeemrmn marked this pull request as ready for review June 12, 2023 10:40

mmastrac suggested changes Jun 12, 2023

View reviewed changes

ext/broadcast_channel/lib.rs Outdated Show resolved Hide resolved

nayeemrmn added 2 commits June 13, 2023 12:59

back to all or nothing extension sources

f0ed345

Merge remote-tracking branch 'upstream/main' into extension-js-exclud…

4253c51

…e-per-crate

nayeemrmn changed the title ~~refactor: exclude extension js sources per-crate~~ refactor(core): cleanup feature flags for js source inclusion Jun 13, 2023

mmastrac approved these changes Jun 13, 2023

View reviewed changes

mmastrac merged commit ceb03cf into denoland:main Jun 13, 2023

nayeemrmn deleted the extension-js-exclude-per-crate branch June 13, 2023 17:14

bartlomieju reviewed Jun 13, 2023

View reviewed changes

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

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

8bcf81f

…denoland#19463)" This reverts commit ceb03cf.

bartlomieju mentioned this pull request Jun 13, 2023

Revert "refactor(core): cleanup feature flags for js source inclusion… #19490

Merged

nayeemrmn mentioned this pull request Jun 15, 2023

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

Merged

nayeemrmn mentioned this pull request Jul 26, 2023

Add support for forced-included or forced-loaded scripts to extension! denoland/deno_core#90

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(core): cleanup feature flags for js source inclusion #19463

refactor(core): cleanup feature flags for js source inclusion #19463

nayeemrmn commented Jun 11, 2023 •

edited

Loading

mmastrac left a comment

bartlomieju left a comment

bartlomieju Jun 13, 2023

nayeemrmn Jun 13, 2023

bartlomieju Jun 13, 2023

nayeemrmn Jun 13, 2023

bartlomieju Jun 13, 2023

nayeemrmn Jun 13, 2023

bartlomieju Jun 13, 2023

nayeemrmn Jun 13, 2023

bartlomieju Jun 13, 2023

bartlomieju Jun 13, 2023

bartlomieju commented Jun 13, 2023

bartlomieju commented Jun 13, 2023

refactor(core): cleanup feature flags for js source inclusion #19463

refactor(core): cleanup feature flags for js source inclusion #19463

Conversation

nayeemrmn commented Jun 11, 2023 • edited Loading

mmastrac left a comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented Jun 13, 2023

bartlomieju commented Jun 13, 2023

nayeemrmn commented Jun 11, 2023 •

edited

Loading