Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Snapshots: remove "runtime/" snapshot #21137

Closed
bartlomieju opened this issue Nov 10, 2023 · 0 comments · Fixed by #21794
Closed

Snapshots: remove "runtime/" snapshot #21137

bartlomieju opened this issue Nov 10, 2023 · 0 comments · Fixed by #21794
Assignees
Labels
perf performance related refactor

Comments

@bartlomieju
Copy link
Member

Following issues need to be addressed first:

Once we do all of them, we can start to remove snapshot creating in the "runtime/" crate. This will involve several stages:

  • remove usage of the snapshot in "runtime/js.rs" - this is already gated behind a cargo feature called dont_create_runtime_snapshot:
    https://github.com/denoland/deno/blob/9010b8df53cd37f0410e08c43a194667974686a2/runtime/js.rs#L5C19-L19
    This cargo feature is already clunky to use - we don't specify it for deno_runtime build dependency in "cli":

    deno/cli/Cargo.toml

    Lines 34 to 35 in 9010b8d

    [build-dependencies]
    deno_runtime = { workspace = true, features = ["exclude_runtime_main_js", "include_js_files_for_snapshotting"] }

    but specify it for regular dependency:
    deno_runtime = { workspace = true, features = ["dont_create_runtime_snapshot", "exclude_runtime_main_js", "include_js_files_for_snapshotting"] }

    Lots of complexity for little gain...
  • remove usage of said snapshot in "runtime/worker.rs" and "runtime/web_worker.rs" - the snapshot is used as a fallback if user doesn't provide snapshot of their own:

    deno/runtime/worker.rs

    Lines 352 to 354 in 9010b8d

    startup_snapshot: options
    .startup_snapshot
    .or_else(crate::js::deno_isolate_init),

    This is really only used by the embedders of the "deno_runtime" crate (we don't use it in CLI anyway). We can handle this better by defaulting to not using a snapshot and instead embedding the source files in the binary (similarly to what happens if "__runtime_js_source" cargo feature is used). With Snapshots: factor out generation of "runtime/" snapshot to a helper module #21134 it should be very easy for embedders to create a snapshot and integrate it into their own build script

Special care needs to be taken to account for include_js_files_for_snapshotting cargo feature used both in deno_core and deno_runtime.

I expect this effort to happen over several PRs.

@bartlomieju bartlomieju added perf performance related refactor labels Nov 10, 2023
bartlomieju added a commit that referenced this issue Dec 2, 2023
This commit removes some of the technical debt related 
to snapshotting JS code:
- "cli/ops/mod.rs" and "cli/build.rs" no longer define "cli" extension
which was not required anymore
- Cargo features for "deno_runtime" crate have been unified in
"cli/Cargo.toml"
- "cli/build.rs" uses "deno_runtime::snapshot::create_runtime_snapshot"
API
instead of copy-pasting the code
- "cli/js/99_main.js" was completely removed as it's not necessary
anymore

Towards #21137
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
perf performance related refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants