perf(ext/node): reduce some allocations in require (#25197)

2 files changed, 52 insertions, 53 deletions

diff --git a/ext/node/ops/require.rs b/ext/node/ops/require.rs
index d074234c3..def1d9758 100644
--- a/ext/node/ops/require.rs
+++ b/ext/node/ops/require.rs
@@ -10,6 +10,7 @@ use deno_core::JsRuntimeInspector;
 use deno_core::ModuleSpecifier;
 use deno_core::OpState;
 use deno_fs::FileSystemRc;
+use deno_package_json::PackageJsonRc;
 use node_resolver::NodeModuleKind;
 use node_resolver::NodeResolutionMode;
 use node_resolver::REQUIRE_CONDITIONS;
@@ -22,7 +23,6 @@ use crate::NodePermissions;
 use crate::NodeRequireResolverRc;
 use crate::NodeResolverRc;
 use crate::NpmResolverRc;
-use crate::PackageJson;
 
 fn ensure_read_permission<P>(
   state: &mut OpState,
@@ -70,7 +70,7 @@ pub fn op_require_init_paths() -> Vec<String> {
 
   // let mut paths = paths
   //   .into_iter()
-  //   .map(|p| p.to_string_lossy().to_string())
+  //   .map(|p| p.to_string_lossy().into_owned())
   //   .collect();
 
   // if !node_path.is_empty() {
@@ -98,15 +98,13 @@ where
 {
   let fs = state.borrow::<FileSystemRc>();
   // Guarantee that "from" is absolute.
-  let from_url = if from.starts_with("file:///") {
-    Url::parse(&from)?
+  let from = if from.starts_with("file:///") {
+    url_to_file_path(&Url::parse(&from)?)?
   } else {
-    deno_core::resolve_path(
-      &from,
-      &(fs.cwd().map_err(AnyError::from)).context("Unable to get CWD")?,
-    )?
+    let current_dir =
+      &(fs.cwd().map_err(AnyError::from)).context("Unable to get CWD")?;
+    deno_core::normalize_path(current_dir.join(from))
   };
-  let from = url_to_file_path(&from_url)?;
 
   ensure_read_permission::<P>(state, &from)?;
 
@@ -117,7 +115,7 @@ where
       let bytes = from_str.as_bytes();
       if bytes[from_str.len() - 1] == b'\\' && bytes[from_str.len() - 2] == b':'
       {
-        let p = from_str.to_owned() + "node_modules";
+        let p = format!("{}node_modules", from_str);
         return Ok(vec![p]);
       }
     }
@@ -129,12 +127,12 @@ where
     }
   }
 
-  let mut paths = vec![];
+  let mut paths = Vec::with_capacity(from.components().count());
   let mut current_path = from.as_path();
   let mut maybe_parent = Some(current_path);
   while let Some(parent) = maybe_parent {
     if !parent.ends_with("node_modules") {
-      paths.push(parent.join("node_modules").to_string_lossy().to_string());
+      paths.push(parent.join("node_modules").to_string_lossy().into_owned());
     }
     current_path = parent;
     maybe_parent = current_path.parent();
@@ -158,7 +156,7 @@ pub fn op_require_proxy_path(#[string] filename: String) -> String {
 
   if trailing_slash {
     let p = PathBuf::from(filename);
-    p.join("noop.js").to_string_lossy().to_string()
+    p.join("noop.js").to_string_lossy().into_owned()
   } else {
     filename
   }
@@ -200,7 +198,7 @@ pub fn op_require_resolve_deno_dir(
       }),
     )
     .ok()
-    .map(|p| p.to_string_lossy().to_string())
+    .map(|p| p.to_string_lossy().into_owned())
 }
 
 #[op2(fast)]
@@ -251,7 +249,7 @@ pub fn op_require_resolve_lookup_paths(
   // }
 
   let p = PathBuf::from(parent_filename);
-  Some(vec![p.parent().unwrap().to_string_lossy().to_string()])
+  Some(vec![p.parent().unwrap().to_string_lossy().into_owned()])
 }
 
 #[op2(fast)]
@@ -295,24 +293,23 @@ where
   let fs = state.borrow::<FileSystemRc>();
   let canonicalized_path =
     deno_core::strip_unc_prefix(fs.realpath_sync(&path)?);
-  Ok(canonicalized_path.to_string_lossy().to_string())
+  Ok(canonicalized_path.to_string_lossy().into_owned())
 }
 
-fn path_resolve(parts: Vec<String>) -> String {
-  assert!(!parts.is_empty());
-  let mut p = PathBuf::from(&parts[0]);
-  if parts.len() > 1 {
-    for part in &parts[1..] {
-      p = p.join(part);
-    }
+fn path_resolve<'a>(mut parts: impl Iterator<Item = &'a str>) -> PathBuf {
+  let mut p = PathBuf::from(parts.next().unwrap());
+  for part in parts {
+    p = p.join(part);
   }
-  normalize_path(p).to_string_lossy().to_string()
+  normalize_path(p)
 }
 
 #[op2]
 #[string]
 pub fn op_require_path_resolve(#[serde] parts: Vec<String>) -> String {
-  path_resolve(parts)
+  path_resolve(parts.iter().map(|s| s.as_str()))
+    .to_string_lossy()
+    .into_owned()
 }
 
 #[op2]
@@ -322,7 +319,7 @@ pub fn op_require_path_dirname(
 ) -> Result<String, AnyError> {
   let p = PathBuf::from(request);
   if let Some(parent) = p.parent() {
-    Ok(parent.to_string_lossy().to_string())
+    Ok(parent.to_string_lossy().into_owned())
   } else {
     Err(generic_error("Path doesn't have a parent"))
   }
@@ -335,7 +332,7 @@ pub fn op_require_path_basename(
 ) -> Result<String, AnyError> {
   let p = PathBuf::from(request);
   if let Some(path) = p.file_name() {
-    Ok(path.to_string_lossy().to_string())
+    Ok(path.to_string_lossy().into_owned())
   } else {
     Err(generic_error("Path doesn't have a file name"))
   }
@@ -365,7 +362,7 @@ where
       let fs = state.borrow::<FileSystemRc>();
       if let Ok(cwd) = fs.cwd() {
         ensure_read_permission::<P>(state, &cwd)?;
-        return Ok(Some(cwd.to_string_lossy().to_string()));
+        return Ok(Some(cwd.to_string_lossy().into_owned()));
       }
     }
   }
@@ -388,9 +385,7 @@ where
 
   let node_resolver = state.borrow::<NodeResolverRc>();
   let pkg = node_resolver
-    .get_closest_package_json(
-      &Url::from_file_path(parent_path.unwrap()).unwrap(),
-    )
+    .get_closest_package_json_from_path(&PathBuf::from(parent_path.unwrap()))
     .ok()
     .flatten();
   if pkg.is_none() {
@@ -457,7 +452,7 @@ where
 pub fn op_require_as_file_path(#[string] file_or_url: String) -> String {
   if let Ok(url) = Url::parse(&file_or_url) {
     if let Ok(p) = url.to_file_path() {
-      return p.to_string_lossy().to_string();
+      return p.to_string_lossy().into_owned();
     }
   }
 
@@ -469,7 +464,7 @@ pub fn op_require_as_file_path(#[string] file_or_url: String) -> String {
 pub fn op_require_resolve_exports<P>(
   state: &mut OpState,
   uses_local_node_modules_dir: bool,
-  #[string] modules_path: String,
+  #[string] modules_path_str: String,
   #[string] _request: String,
   #[string] name: String,
   #[string] expansion: String,
@@ -482,22 +477,22 @@ where
   let npm_resolver = state.borrow::<NpmResolverRc>();
   let node_resolver = state.borrow::<NodeResolverRc>();
 
-  let pkg_path = if npm_resolver
-    .in_npm_package_at_file_path(&PathBuf::from(&modules_path))
+  let modules_path = PathBuf::from(&modules_path_str);
+  let pkg_path = if npm_resolver.in_npm_package_at_file_path(&modules_path)
     && !uses_local_node_modules_dir
   {
     modules_path
   } else {
-    let original = modules_path.clone();
-    let mod_dir = path_resolve(vec![modules_path, name]);
-    if fs.is_dir_sync(Path::new(&mod_dir)) {
+    let mod_dir =
+      path_resolve([modules_path_str.as_str(), name.as_str()].into_iter());
+    if fs.is_dir_sync(&mod_dir) {
       mod_dir
     } else {
-      original
+      modules_path
     }
   };
-  let Some(pkg) = node_resolver
-    .load_package_json(&PathBuf::from(&pkg_path).join("package.json"))?
+  let Some(pkg) =
+    node_resolver.load_package_json(&pkg_path.join("package.json"))?
   else {
     return Ok(None);
   };
@@ -527,18 +522,15 @@ where
 pub fn op_require_read_closest_package_json<P>(
   state: &mut OpState,
   #[string] filename: String,
-) -> Result<Option<PackageJson>, AnyError>
+) -> Result<Option<PackageJsonRc>, AnyError>
 where
   P: NodePermissions + 'static,
 {
-  ensure_read_permission::<P>(
-    state,
-    PathBuf::from(&filename).parent().unwrap(),
-  )?;
+  let filename = PathBuf::from(filename);
+  ensure_read_permission::<P>(state, filename.parent().unwrap())?;
   let node_resolver = state.borrow::<NodeResolverRc>().clone();
   node_resolver
-    .get_closest_package_json(&Url::from_file_path(filename).unwrap())
-    .map(|maybe_pkg| maybe_pkg.map(|pkg| (*pkg).clone()))
+    .get_closest_package_json_from_path(&filename)
     .map_err(AnyError::from)
 }
 
@@ -547,7 +539,7 @@ where
 pub fn op_require_read_package_scope<P>(
   state: &mut OpState,
   #[string] package_json_path: String,
-) -> Option<PackageJson>
+) -> Option<PackageJsonRc>
 where
   P: NodePermissions + 'static,
 {
@@ -561,7 +553,6 @@ where
     .load_package_json(&package_json_path)
     .ok()
     .flatten()
-    .map(|pkg| (*pkg).clone())
 }
 
 #[op2]
@@ -610,7 +601,7 @@ pub fn op_require_break_on_next_statement(state: &mut OpState) {
 
 fn url_to_file_path_string(url: &Url) -> Result<String, AnyError> {
   let file_path = url_to_file_path(url)?;
-  Ok(file_path.to_string_lossy().to_string())
+  Ok(file_path.to_string_lossy().into_owned())
 }
 
 fn url_to_file_path(url: &Url) -> Result<PathBuf, AnyError> {
diff --git a/ext/node_resolver/analyze.rs b/ext/node_resolver/analyze.rs
index 8d6a73424..8231cdc8a 100644
--- a/ext/node_resolver/analyze.rs
+++ b/ext/node_resolver/analyze.rs
@@ -1,5 +1,6 @@
 // Copyright 2018-2024 the Deno authors. All rights reserved. MIT license.
 
+use std::borrow::Cow;
 use std::collections::BTreeSet;
 use std::collections::HashSet;
 use std::path::Path;
@@ -583,9 +584,16 @@ fn not_found(path: &str, referrer: &Path) -> AnyError {
   std::io::Error::new(std::io::ErrorKind::NotFound, msg).into()
 }
 
-fn escape_for_double_quote_string(text: &str) -> String {
-  text.replace('\\', "\\\\").replace('"', "\\\"")
+fn escape_for_double_quote_string(text: &str) -> Cow<str> {
+  // this should be rare, so doing a scan first before allocating is ok
+  if text.chars().any(|c| matches!(c, '"' | '\\')) {
+    // don't bother making this more complex for perf because it's rare
+    Cow::Owned(text.replace('\\', "\\\\").replace('"', "\\\""))
+  } else {
+    Cow::Borrowed(text)
+  }
 }
+
 #[cfg(test)]
 mod tests {
   use super::*;