Repository: denoland/deno_core
Pull request #491 (2024-01-26 00:21:34)

feat: Add ModuleLoadResponse
Checking if having this enum is beneficial and can help optimize some of the code.

In many situation embedders will rely on `ModuleLoader::prepare_module_load` 
and assemble the whole graph themselves. Then when actually loading modules,
they will wrap the already ready value into an empty future that needs to be boxed.


Patch:
diff --git a/core/examples/ts_module_loader.rs b/core/examples/ts_module_loader.rs
index a23d66f..e279a73 100644
--- a/core/examples/ts_module_loader.rs
+++ b/core/examples/ts_module_loader.rs
@@ -6,7 +6,6 @@
 
 use std::cell::RefCell;
 use std::collections::HashMap;
-use std::pin::Pin;
 use std::rc::Rc;
 
 use anyhow::anyhow;
@@ -20,17 +19,16 @@ use deno_core::error::AnyError;
 use deno_core::resolve_import;
 use deno_core::resolve_path;
 use deno_core::JsRuntime;
+use deno_core::ModuleLoadResponse;
 use deno_core::ModuleLoader;
 use deno_core::ModuleSource;
 use deno_core::ModuleSourceCode;
-use deno_core::ModuleSourceFuture;
 use deno_core::ModuleSpecifier;
 use deno_core::ModuleType;
 use deno_core::RequestedModuleType;
 use deno_core::ResolutionKind;
 use deno_core::RuntimeOptions;
 use deno_core::SourceMapGetter;
-use futures::FutureExt;
 
 #[derive(Clone)]
 struct SourceMapStore(Rc<RefCell<HashMap<String, Vec<u8>>>>);
@@ -69,7 +67,7 @@ impl ModuleLoader for TypescriptModuleLoader {
     _maybe_referrer: Option<&ModuleSpecifier>,
     _is_dyn_import: bool,
     _requested_module_type: RequestedModuleType,
-  ) -> Pin<Box<ModuleSourceFuture>> {
+  ) -> ModuleLoadResponse {
     let source_maps = self.source_maps.clone();
     fn load(
       source_maps: SourceMapStore,
@@ -128,7 +126,7 @@ impl ModuleLoader for TypescriptModuleLoader {
       ))
     }
 
-    futures::future::ready(load(source_maps, module_specifier)).boxed_local()
+    ModuleLoadResponse::Sync(load(source_maps, module_specifier))
   }
 }
 
diff --git a/core/lib.rs b/core/lib.rs
index db9eeea..4ef5f49 100644
--- a/core/lib.rs
+++ b/core/lib.rs
@@ -103,6 +103,7 @@ pub use crate::modules::FsModuleLoader;
 pub use crate::modules::ModuleCodeBytes;
 pub use crate::modules::ModuleCodeString;
 pub use crate::modules::ModuleId;
+pub use crate::modules::ModuleLoadResponse;
 pub use crate::modules::ModuleLoader;
 pub use crate::modules::ModuleSource;
 pub use crate::modules::ModuleSourceCode;
diff --git a/core/modules/loaders.rs b/core/modules/loaders.rs
index 3f38001..97b93fc 100644
--- a/core/modules/loaders.rs
+++ b/core/modules/loaders.rs
@@ -15,7 +15,6 @@ use crate::ModuleSourceCode;
 use anyhow::anyhow;
 use anyhow::Context;
 use anyhow::Error;
-use futures::future::ready;
 use futures::future::FutureExt;
 use std::cell::RefCell;
 use std::collections::HashMap;
@@ -24,6 +23,18 @@ use std::future::Future;
 use std::pin::Pin;
 use std::rc::Rc;
 
+/// Result of calling `ModuleLoader::load`.
+pub enum ModuleLoadResponse {
+  /// Source file is available synchronously - eg. embedder might have
+  /// collected all the necessary sources in `ModuleLoader::prepare_module_load`.
+  /// Slightly cheaper than `Async` as it avoids boxing.
+  Sync(Result<ModuleSource, Error>),
+
+  /// Source file needs to be loaded. Requires boxing due to recrusive
+  /// nature of module loading.
+  Async(Pin<Box<ModuleSourceFuture>>),
+}
+
 pub trait ModuleLoader {
   /// Returns an absolute URL.
   /// When implementing an spec-complaint VM, this should be exactly the
@@ -52,7 +63,7 @@ pub trait ModuleLoader {
     maybe_referrer: Option<&ModuleSpecifier>,
     is_dyn_import: bool,
     requested_module_type: RequestedModuleType,
-  ) -> Pin<Box<ModuleSourceFuture>>;
+  ) -> ModuleLoadResponse;
 
   /// This hook can be used by implementors to do some preparation
   /// work before starting loading of modules.
@@ -92,7 +103,7 @@ impl ModuleLoader for NoopModuleLoader {
     maybe_referrer: Option<&ModuleSpecifier>,
     _is_dyn_import: bool,
     _requested_module_type: RequestedModuleType,
-  ) -> Pin<Box<ModuleSourceFuture>> {
+  ) -> ModuleLoadResponse {
     let maybe_referrer = maybe_referrer
       .map(|s| s.as_str())
       .unwrap_or("(no referrer)");
@@ -101,7 +112,7 @@ impl ModuleLoader for NoopModuleLoader {
         "Module loading is not supported; attempted to load: \"{module_specifier}\" from \"{maybe_referrer}\"",
       )
     );
-    async move { Err(err) }.boxed_local()
+    ModuleLoadResponse::Sync(Err(err))
   }
 }
 
@@ -147,28 +158,24 @@ impl ModuleLoader for ExtModuleLoader {
     _maybe_referrer: Option<&ModuleSpecifier>,
     _is_dyn_import: bool,
     _requested_module_type: RequestedModuleType,
-  ) -> Pin<Box<ModuleSourceFuture>> {
+  ) -> ModuleLoadResponse {
     let sources = self.sources.borrow();
     let source = match sources.get(specifier.as_str()) {
       Some(source) => source,
-      None => return futures::future::err(anyhow!("Specifier \"{}\" was not passed as an extension module and was not included in the snapshot.", specifier)).boxed_local(),
+      None => return ModuleLoadResponse::Sync(Err(anyhow!("Specifier \"{}\" was not passed as an extension module and was not included in the snapshot.", specifier))),
     };
     self
       .used_specifiers
       .borrow_mut()
       .insert(specifier.to_string());
-    let result = source.load();
-    match result {
-      Ok(code) => {
-        let res = ModuleSource::new(
-          ModuleType::JavaScript,
-          ModuleSourceCode::String(code),
-          specifier,
-        );
-        return futures::future::ok(res).boxed_local();
-      }
-      Err(err) => return futures::future::err(err).boxed_local(),
-    }
+    let result = source.load().map(|code| {
+      ModuleSource::new(
+        ModuleType::JavaScript,
+        ModuleSourceCode::String(code),
+        specifier,
+      )
+    });
+    ModuleLoadResponse::Sync(result)
   }
 
   fn prepare_load(
@@ -212,24 +219,20 @@ impl ModuleLoader for LazyEsmModuleLoader {
     _maybe_referrer: Option<&ModuleSpecifier>,
     _is_dyn_import: bool,
     _requested_module_type: RequestedModuleType,
-  ) -> Pin<Box<ModuleSourceFuture>> {
+  ) -> ModuleLoadResponse {
     let sources = self.sources.borrow();
     let source = match sources.get(specifier.as_str()) {
       Some(source) => source,
-      None => return futures::future::err(anyhow!("Specifier \"{}\" cannot be lazy-loaded as it was not included in the binary.", specifier)).boxed_local(),
+      None => return ModuleLoadResponse::Sync(Err(anyhow!("Specifier \"{}\" cannot be lazy-loaded as it was not included in the binary.", specifier))),
     };
-    let result = source.load();
-    match result {
-      Ok(code) => {
-        let res = ModuleSource::new(
-          ModuleType::JavaScript,
-          ModuleSourceCode::String(code),
-          specifier,
-        );
-        return futures::future::ok(res).boxed_local();
-      }
-      Err(err) => return futures::future::err(err).boxed_local(),
-    }
+    let result = source.load().map(|code| {
+      ModuleSource::new(
+        ModuleType::JavaScript,
+        ModuleSourceCode::String(code),
+        specifier,
+      )
+    });
+    ModuleLoadResponse::Sync(result)
   }
 
   fn prepare_load(
@@ -288,9 +291,9 @@ impl ModuleLoader for FsModuleLoader {
     _maybe_referrer: Option<&ModuleSpecifier>,
     _is_dynamic: bool,
     requested_module_type: RequestedModuleType,
-  ) -> Pin<Box<ModuleSourceFuture>> {
+  ) -> ModuleLoadResponse {
     let module_specifier = module_specifier.clone();
-    async move {
+    let fut = async move {
       let path = module_specifier.to_file_path().map_err(|_| {
         generic_error(format!(
           "Provided module specifier \"{module_specifier}\" is not a file URL."
@@ -331,7 +334,9 @@ impl ModuleLoader for FsModuleLoader {
       );
       Ok(module)
     }
-    .boxed_local()
+    .boxed_local();
+
+    ModuleLoadResponse::Async(fut)
   }
 }
 
@@ -377,7 +382,7 @@ impl ModuleLoader for StaticModuleLoader {
     _maybe_referrer: Option<&ModuleSpecifier>,
     _is_dyn_import: bool,
     _requested_module_type: RequestedModuleType,
-  ) -> Pin<Box<ModuleSourceFuture>> {
+  ) -> ModuleLoadResponse {
     let res = if let Some(code) = self.map.get(module_specifier) {
       Ok(ModuleSource::new(
         ModuleType::JavaScript,
@@ -387,7 +392,7 @@ impl ModuleLoader for StaticModuleLoader {
     } else {
       Err(generic_error("Module not found"))
     };
-    ready(res).boxed_local()
+    ModuleLoadResponse::Sync(res)
   }
 }
 
@@ -458,7 +463,7 @@ impl<L: ModuleLoader> ModuleLoader for TestingModuleLoader<L> {
     maybe_referrer: Option<&ModuleSpecifier>,
     is_dyn_import: bool,
     requested_module_type: RequestedModuleType,
-  ) -> Pin<Box<ModuleSourceFuture>> {
+  ) -> ModuleLoadResponse {
     self.load_count.set(self.load_count.get() + 1);
     self.log.borrow_mut().push(module_specifier.clone());
     self.loader.load(
diff --git a/core/modules/map.rs b/core/modules/map.rs
index c7706a5..82568a4 100644
--- a/core/modules/map.rs
+++ b/core/modules/map.rs
@@ -23,6 +23,7 @@ use crate::runtime::JsRealm;
 use crate::ExtensionFileSource;
 use crate::FastString;
 use crate::JsRuntime;
+use crate::ModuleLoadResponse;
 use crate::ModuleSource;
 use crate::ModuleSpecifier;
 use anyhow::bail;
@@ -1536,11 +1537,14 @@ impl ModuleMap {
     }
 
     let specifier = ModuleSpecifier::parse(module_specifier)?;
-    let source = futures::executor::block_on(async {
-      loader
-        .load(&specifier, None, false, RequestedModuleType::None)
-        .await
-    })?;
+
+    let load_response =
+      loader.load(&specifier, None, false, RequestedModuleType::None);
+
+    let source = match load_response {
+      ModuleLoadResponse::Sync(result) => result,
+      ModuleLoadResponse::Async(fut) => futures::executor::block_on(fut),
+    }?;
 
     self.lazy_load_es_module_from_code(
       scope,
diff --git a/core/modules/mod.rs b/core/modules/mod.rs
index a44275b..6f3dd69 100644
--- a/core/modules/mod.rs
+++ b/core/modules/mod.rs
@@ -30,6 +30,7 @@ pub(crate) use loaders::ExtModuleLoader;
 pub use loaders::ExtModuleLoaderCb;
 pub use loaders::FsModuleLoader;
 pub(crate) use loaders::LazyEsmModuleLoader;
+pub use loaders::ModuleLoadResponse;
 pub use loaders::ModuleLoader;
 pub use loaders::NoopModuleLoader;
 pub use loaders::StaticModuleLoader;
diff --git a/core/modules/recursive_load.rs b/core/modules/recursive_load.rs
index 65cf8a7..4004ac1 100644
--- a/core/modules/recursive_load.rs
+++ b/core/modules/recursive_load.rs
@@ -8,6 +8,7 @@ use crate::modules::ModuleRequest;
 use crate::modules::RequestedModuleType;
 use crate::modules::ResolutionKind;
 use crate::resolve_url;
+use crate::ModuleLoadResponse;
 use crate::ModuleLoader;
 use crate::ModuleSource;
 use anyhow::Error;
@@ -265,14 +266,17 @@ impl RecursiveModuleLoad {
               if visited_as_alias.borrow().contains(specifier.as_str()) {
                 return Ok(None);
               }
-              let load_result = loader
-                .load(
-                  &specifier,
-                  Some(&referrer),
-                  is_dynamic_import,
-                  requested_module_type,
-                )
-                .await;
+              let load_response = loader.load(
+                &specifier,
+                Some(&referrer),
+                is_dynamic_import,
+                requested_module_type,
+              );
+
+              let load_result = match load_response {
+                ModuleLoadResponse::Sync(result) => result,
+                ModuleLoadResponse::Async(fut) => fut.await,
+              };
               if let Ok(source) = &load_result {
                 if let Some(found_specifier) = &source.module_url_found {
                   visited_as_alias
@@ -337,14 +341,16 @@ impl Stream for RecursiveModuleLoad {
           let is_dynamic_import = inner.is_dynamic_import();
           let requested_module_type = requested_module_type.clone();
           async move {
-            let result = loader
-              .load(
-                &module_specifier,
-                maybe_referrer.as_ref(),
-                is_dynamic_import,
-                requested_module_type,
-              )
-              .await;
+            let load_response = loader.load(
+              &module_specifier,
+              maybe_referrer.as_ref(),
+              is_dynamic_import,
+              requested_module_type,
+            );
+            let result = match load_response {
+              ModuleLoadResponse::Sync(result) => result,
+              ModuleLoadResponse::Async(fut) => fut.await,
+            };
             result.map(|s| Some((module_request, s)))
           }
           .boxed_local()


Test patch:
diff --git a/core/modules/tests.rs b/core/modules/tests.rs
index bbe0a70..83b6dee 100644
--- a/core/modules/tests.rs
+++ b/core/modules/tests.rs
@@ -19,7 +19,6 @@ use crate::runtime::JsRuntimeForSnapshot;
 use crate::FastString;
 use crate::ModuleCodeString;
 use crate::ModuleSource;
-use crate::ModuleSourceFuture;
 use crate::ModuleSpecifier;
 use crate::ModuleType;
 use crate::ResolutionKind;
@@ -242,11 +241,13 @@ impl ModuleLoader for MockLoader {
     _maybe_referrer: Option<&ModuleSpecifier>,
     _is_dyn_import: bool,
     _requested_module_type: RequestedModuleType,
-  ) -> Pin<Box<ModuleSourceFuture>> {
+  ) -> ModuleLoadResponse {
     let mut loads = self.loads.lock();
     loads.push(module_specifier.to_string());
     let url = module_specifier.to_string();
-    DelayedSourceCodeFuture { url, counter: 0 }.boxed()
+    ModuleLoadResponse::Async(
+      DelayedSourceCodeFuture { url, counter: 0 }.boxed(),
+    )
   }
 }
 
@@ -1543,7 +1544,7 @@ async fn no_duplicate_loads() {
       _maybe_referrer: Option<&ModuleSpecifier>,
       _is_dyn_import: bool,
       _requested_module_type: RequestedModuleType,
-    ) -> Pin<Box<ModuleSourceFuture>> {
+    ) -> ModuleLoadResponse {
       let found_specifier =
         if module_specifier.as_str() == "https://example.com/foo.js" {
           Some("https://example.com/v1/foo.js".to_string())
@@ -1574,7 +1575,7 @@ async fn no_duplicate_loads() {
         module_url_specified: module_specifier.clone().into(),
         module_url_found: found_specifier.map(|s| s.into()),
       };
-      async move { Ok(module_source) }.boxed_local()
+      ModuleLoadResponse::Sync(Ok(module_source))
     }
   }
 
diff --git a/testing/checkin/runner/ts_module_loader.rs b/testing/checkin/runner/ts_module_loader.rs
index f72a2d9..5d87299 100644
--- a/testing/checkin/runner/ts_module_loader.rs
+++ b/testing/checkin/runner/ts_module_loader.rs
@@ -2,7 +2,6 @@
 use std::cell::RefCell;
 use std::collections::HashMap;
 use std::path::Path;
-use std::pin::Pin;
 use std::rc::Rc;
 
 use anyhow::bail;
@@ -15,18 +14,16 @@ use deno_core::error::AnyError;
 use deno_core::resolve_import;
 use deno_core::ExtensionFileSource;
 use deno_core::ExtensionFileSourceCode;
+use deno_core::ModuleLoadResponse;
 use deno_core::ModuleLoader;
 use deno_core::ModuleSource;
 use deno_core::ModuleSourceCode;
-use deno_core::ModuleSourceFuture;
 use deno_core::ModuleSpecifier;
 use deno_core::ModuleType;
 use deno_core::RequestedModuleType;
 use deno_core::ResolutionKind;
 use deno_core::SourceMapGetter;
 
-use futures::FutureExt;
-
 #[derive(Clone, Default)]
 struct SourceMapStore(Rc<RefCell<HashMap<String, Vec<u8>>>>);
 
@@ -65,7 +62,7 @@ impl ModuleLoader for TypescriptModuleLoader {
     _maybe_referrer: Option<&ModuleSpecifier>,
     _is_dyn_import: bool,
     _requested_module_type: RequestedModuleType,
-  ) -> Pin<Box<ModuleSourceFuture>> {
+  ) -> ModuleLoadResponse {
     let source_maps = self.source_maps.clone();
     fn load(
       source_maps: SourceMapStore,
@@ -129,7 +126,7 @@ impl ModuleLoader for TypescriptModuleLoader {
       ))
     }
 
-    futures::future::ready(load(source_maps, module_specifier)).boxed_local()
+    ModuleLoadResponse::Sync(load(source_maps, module_specifier))
   }
 }