From 240b362c002d17bc2b676673ed1b9406683ff0c2 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Mon, 1 Apr 2024 09:10:04 -0400 Subject: [PATCH] perf(node): put pkg json into an `Rc` (#23156) Was doing a bit of debugging on why some stuff is not working in a personal project and ran a quick debug profile and saw it cloning the pkg json a lot. We should put this in an Rc. --- cli/npm/byonm.rs | 3 ++- cli/resolver.rs | 3 ++- ext/node/ops/require.rs | 11 +++++++---- ext/node/package_json.rs | 11 ++++++----- ext/node/resolution.rs | 13 +++++++------ 5 files changed, 24 insertions(+), 17 deletions(-) diff --git a/cli/npm/byonm.rs b/cli/npm/byonm.rs index d17be0e95f..1e61ce885e 100644 --- a/cli/npm/byonm.rs +++ b/cli/npm/byonm.rs @@ -3,6 +3,7 @@ use std::borrow::Cow; use std::path::Path; use std::path::PathBuf; +use std::rc::Rc; use std::sync::Arc; use deno_ast::ModuleSpecifier; @@ -52,7 +53,7 @@ impl ByonmCliNpmResolver { &self, dep_name: &str, referrer: &ModuleSpecifier, - ) -> Option { + ) -> Option> { let referrer_path = referrer.to_file_path().ok()?; let mut current_folder = referrer_path.parent()?; loop { diff --git a/cli/resolver.rs b/cli/resolver.rs index 6594bf2d4d..fc326e1b19 100644 --- a/cli/resolver.rs +++ b/cli/resolver.rs @@ -36,6 +36,7 @@ use std::collections::HashMap; use std::collections::HashSet; use std::path::Path; use std::path::PathBuf; +use std::rc::Rc; use std::sync::Arc; use crate::args::package_json::PackageJsonDeps; @@ -98,7 +99,7 @@ impl CliNodeResolver { &self, referrer: &ModuleSpecifier, permissions: &dyn NodePermissions, - ) -> Result, AnyError> { + ) -> Result>, AnyError> { self .node_resolver .get_closest_package_json(referrer, permissions) diff --git a/ext/node/ops/require.rs b/ext/node/ops/require.rs index d6771c6689..426c419956 100644 --- a/ext/node/ops/require.rs +++ b/ext/node/ops/require.rs @@ -542,10 +542,12 @@ where )?; let node_resolver = state.borrow::>(); let permissions = state.borrow::

(); - node_resolver.get_closest_package_json( - &Url::from_file_path(filename).unwrap(), - permissions, - ) + node_resolver + .get_closest_package_json( + &Url::from_file_path(filename).unwrap(), + permissions, + ) + .map(|maybe_pkg| maybe_pkg.map(|pkg| (*pkg).clone())) } #[op2] @@ -562,6 +564,7 @@ where let package_json_path = PathBuf::from(package_json_path); node_resolver .load_package_json(permissions, package_json_path) + .map(|pkg| (*pkg).clone()) .ok() } diff --git a/ext/node/package_json.rs b/ext/node/package_json.rs index 9ac3a09698..77352ae1df 100644 --- a/ext/node/package_json.rs +++ b/ext/node/package_json.rs @@ -18,9 +18,10 @@ use std::cell::RefCell; use std::collections::HashMap; use std::io::ErrorKind; use std::path::PathBuf; +use std::rc::Rc; thread_local! { - static CACHE: RefCell> = RefCell::new(HashMap::new()); + static CACHE: RefCell>> = RefCell::new(HashMap::new()); } #[derive(Clone, Debug, Serialize)] @@ -66,7 +67,7 @@ impl PackageJson { resolver: &dyn NpmResolver, permissions: &dyn NodePermissions, path: PathBuf, - ) -> Result { + ) -> Result, AnyError> { resolver.ensure_read_permission(permissions, &path)?; Self::load_skip_read_permission(fs, path) } @@ -74,7 +75,7 @@ impl PackageJson { pub fn load_skip_read_permission( fs: &dyn deno_fs::FileSystem, path: PathBuf, - ) -> Result { + ) -> Result, AnyError> { assert!(path.is_absolute()); if CACHE.with(|cache| cache.borrow().contains_key(&path)) { @@ -84,7 +85,7 @@ impl PackageJson { let source = match fs.read_text_file_sync(&path) { Ok(source) => source, Err(err) if err.kind() == ErrorKind::NotFound => { - return Ok(PackageJson::empty(path)); + return Ok(Rc::new(PackageJson::empty(path))); } Err(err) => bail!( "Error loading package.json at {}. {:#}", @@ -93,7 +94,7 @@ impl PackageJson { ), }; - let package_json = Self::load_from_string(path, source)?; + let package_json = Rc::new(Self::load_from_string(path, source)?); CACHE.with(|cache| { cache .borrow_mut() diff --git a/ext/node/resolution.rs b/ext/node/resolution.rs index 50c4e2bb5f..37598810d3 100644 --- a/ext/node/resolution.rs +++ b/ext/node/resolution.rs @@ -4,6 +4,7 @@ use std::borrow::Cow; use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; +use std::rc::Rc; use deno_core::anyhow::bail; use deno_core::anyhow::Context; @@ -252,7 +253,7 @@ impl NodeResolver { specifier, referrer, NodeModuleKind::Esm, - pkg_config.as_ref(), + pkg_config.as_deref(), conditions, mode, permissions, @@ -399,7 +400,7 @@ impl NodeResolver { let package_json = self .load_package_json(&AllowAllNodePermissions, package_json_path.clone())?; - Ok(match package_json.bin { + Ok(match &package_json.bin { Some(Value::String(_)) => { let Some(name) = &package_json.name else { bail!("'{}' did not have a name", package_json_path.display()); @@ -407,7 +408,7 @@ impl NodeResolver { vec![name.to_string()] } Some(Value::Object(o)) => { - o.into_iter().map(|(key, _)| key).collect::>() + o.iter().map(|(key, _)| key.clone()).collect::>() } _ => Vec::new(), }) @@ -1164,7 +1165,7 @@ impl NodeResolver { &self, url: &ModuleSpecifier, permissions: &dyn NodePermissions, - ) -> Result, AnyError> { + ) -> Result>, AnyError> { let Ok(file_path) = url.to_file_path() else { return Ok(None); }; @@ -1175,7 +1176,7 @@ impl NodeResolver { &self, file_path: &Path, permissions: &dyn NodePermissions, - ) -> Result, AnyError> { + ) -> Result>, AnyError> { let Some(package_json_path) = self.get_closest_package_json_path(file_path)? else { @@ -1213,7 +1214,7 @@ impl NodeResolver { &self, permissions: &dyn NodePermissions, package_json_path: PathBuf, - ) -> Result { + ) -> Result, AnyError> { PackageJson::load( &*self.fs, &*self.npm_resolver,