From 81293440befe43f1d8f5e6b1261d40bfe44f9155 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bartek=20Iwa=C5=84czuk?= <biwanczuk@gmail.com>
Date: Wed, 8 Mar 2023 07:43:26 -0400
Subject: [PATCH] refactor(runtime): conditionally register Extension with
 source files (#18068)

Since we are snapshotting extension source at build time, there's no
need to define list of sources for the extension at runtime.

This commit changes "deno_node" extension by removing "init_polyfill"
function in favor of "init_polyfill_ops_and_esm()" and "init_polyfill_ops()".

The former is used during snapshot and when "deno_runtime" is compiled
with "dont_create_runtime_snapshot" cargo feature flag. The latter is used
when running a worker from an existing snapshot.

This is a start of a bigger refactor to all extensions - thanks to this
change, we don't have to iterate over all defined source files for extension at
runtime, and because of that we don't have to create a filepath for each of the
source files. It's not a big deal, but we are iterating over 300 files on each start,
and concatenating 3 strings before creating a "PathBuf" for ~200 of them.
This is already visible on the startup flamegraphs and should be avoided.
---
 cli/build.rs          |  2 +-
 ext/node/lib.rs       | 14 +++++++++++++-
 runtime/build.rs      |  2 +-
 runtime/web_worker.rs |  5 ++++-
 runtime/worker.rs     | 17 ++++++++++++-----
 5 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/cli/build.rs b/cli/build.rs
index 21e702aed9..ddcd08778c 100644
--- a/cli/build.rs
+++ b/cli/build.rs
@@ -343,7 +343,7 @@ fn create_cli_snapshot(snapshot_path: PathBuf) {
     deno_io::init(Default::default()),
     deno_fs::init::<PermissionsContainer>(false),
     deno_node::init::<PermissionsContainer>(None), // No --unstable.
-    deno_node::init_polyfill(),
+    deno_node::init_polyfill_ops_and_esm(),
     deno_ffi::init::<PermissionsContainer>(false),
     deno_net::init::<PermissionsContainer>(
       None, false, // No --unstable.
diff --git a/ext/node/lib.rs b/ext/node/lib.rs
index 411151f2fb..c492d93d87 100644
--- a/ext/node/lib.rs
+++ b/ext/node/lib.rs
@@ -95,7 +95,19 @@ fn op_node_build_os() -> String {
     .to_string()
 }
 
-pub fn init_polyfill() -> Extension {
+pub fn init_polyfill_ops() -> Extension {
+  Extension::builder(env!("CARGO_PKG_NAME"))
+    .ops(vec![
+      crypto::op_node_create_hash::decl(),
+      crypto::op_node_hash_update::decl(),
+      crypto::op_node_hash_digest::decl(),
+      crypto::op_node_hash_clone::decl(),
+      op_node_build_os::decl(),
+    ])
+    .build()
+}
+
+pub fn init_polyfill_ops_and_esm() -> Extension {
   let esm_files = include_js_files!(
     dir "polyfills",
     "_core.ts",
diff --git a/runtime/build.rs b/runtime/build.rs
index 6da7ec4199..07b56945f6 100644
--- a/runtime/build.rs
+++ b/runtime/build.rs
@@ -281,7 +281,7 @@ mod startup_snapshot {
       // FIXME(bartlomieju): these extensions are specified last, because they
       // depend on `runtime`, even though it should be other way around
       deno_node::init::<Permissions>(None),
-      deno_node::init_polyfill(),
+      deno_node::init_polyfill_ops_and_esm(),
     ];
 
     if let Some(additional_extension) = maybe_additional_extension {
diff --git a/runtime/web_worker.rs b/runtime/web_worker.rs
index f07ecd27f8..4417c13ef4 100644
--- a/runtime/web_worker.rs
+++ b/runtime/web_worker.rs
@@ -433,7 +433,10 @@ impl WebWorker {
         options.unsafely_ignore_certificate_errors.clone(),
       ),
       deno_napi::init::<PermissionsContainer>(),
-      deno_node::init_polyfill(),
+      // TODO(bartlomieju): this should be conditional on `dont_create_runtime_snapshot`
+      // cargo feature and should use `init_polyfill_ops` or `init_polyfill_ops_and_esm`
+      // if the feature is enabled
+      deno_node::init_polyfill_ops(),
       deno_node::init::<PermissionsContainer>(options.npm_resolver),
       ops::os::init_for_worker(),
       ops::permissions::init(),
diff --git a/runtime/worker.rs b/runtime/worker.rs
index c10f9f36eb..e624cb2b4f 100644
--- a/runtime/worker.rs
+++ b/runtime/worker.rs
@@ -214,8 +214,7 @@ impl MainWorker {
       CreateCache(Arc::new(create_cache_fn))
     });
 
-    // Internal modules
-    let mut extensions: Vec<Extension> = vec![
+    let mut extensions = vec![
       // Web APIs
       deno_webidl::init(),
       deno_console::init(),
@@ -263,7 +262,6 @@ impl MainWorker {
         options.unsafely_ignore_certificate_errors.clone(),
       ),
       deno_napi::init::<PermissionsContainer>(),
-      deno_node::init_polyfill(),
       deno_node::init::<PermissionsContainer>(options.npm_resolver),
       ops::os::init(exit_code.clone()),
       ops::permissions::init(),
@@ -273,9 +271,18 @@ impl MainWorker {
       deno_http::init(),
       deno_flash::init::<PermissionsContainer>(unstable),
       ops::http::init(),
-      // Permissions ext (worker specific state)
-      perm_ext,
     ];
+
+    // TODO(bartlomieju): finish this work, currently only `deno_node` is different
+    // as it has the most files
+    #[cfg(feature = "dont_create_runtime_snapshot")]
+    extensions.push(deno_node::init_polyfill_ops_and_esm());
+
+    #[cfg(not(feature = "dont_create_runtime_snapshot"))]
+    extensions.push(deno_node::init_polyfill_ops());
+
+    extensions.push(perm_ext);
+
     extensions.extend(std::mem::take(&mut options.extensions));
 
     #[cfg(not(feature = "dont_create_runtime_snapshot"))]