From adafb6651db7843d56ed649611dcf9bf552b1ed1 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 19 Dec 2024 18:36:23 -0500 Subject: [PATCH 1/2] perf: do not clone version information --- src/registry.rs | 4 ++-- src/resolution/graph.rs | 2 +- src/resolution/snapshot.rs | 25 ++++++++++++++++--------- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/registry.rs b/src/registry.rs index 679f6f3..dc2ad6e 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -32,8 +32,8 @@ impl NpmPackageInfo { pub fn version_info( &self, nv: &PackageNv, - ) -> Result { - match self.versions.get(&nv.version).cloned() { + ) -> Result<&NpmPackageVersionInfo, NpmPackageVersionNotFound> { + match self.versions.get(&nv.version) { Some(version_info) => Ok(version_info), None => Err(NpmPackageVersionNotFound(nv.clone())), } diff --git a/src/resolution/graph.rs b/src/resolution/graph.rs index 55073ba..2d9a324 100644 --- a/src/resolution/graph.rs +++ b/src/resolution/graph.rs @@ -971,7 +971,7 @@ impl<'a, TNpmRegistryApi: NpmRegistryApi> let version_info = package_info .version_info(&pkg_nv) .map_err(NpmPackageVersionResolutionError::VersionNotFound)?; - self.dep_entry_cache.store(pkg_nv.clone(), &version_info)? + self.dep_entry_cache.store(pkg_nv.clone(), version_info)? }; (pkg_nv, deps) diff --git a/src/resolution/snapshot.rs b/src/resolution/snapshot.rs index cfbecda..25c7e72 100644 --- a/src/resolution/snapshot.rs +++ b/src/resolution/snapshot.rs @@ -882,7 +882,7 @@ pub fn incomplete_snapshot_from_lockfile( }) } -#[derive(Debug, Error)] +#[derive(Debug, Clone, Error)] #[error("Integrity check failed for package: \"{package_display_id}\". Unable to verify that the package is the same as when the lockfile was generated. @@ -902,7 +902,7 @@ pub struct IntegrityCheckFailedError { pub filename: String, } -#[derive(Debug, Error)] +#[derive(Debug, Error, Clone)] pub enum SnapshotFromLockfileError { #[error(transparent)] PackageInfoLoad(#[from] NpmRegistryPackageInfoLoadError), @@ -947,15 +947,21 @@ pub async fn snapshot_from_lockfile<'a>( .package_info(&nv.name) .await .map_err(SnapshotFromLockfileError::PackageInfoLoad)?; - package_info - .version_info(nv) - .map_err(|e| SnapshotFromLockfileError::VersionNotFound { source: e }) + Ok((package_info, nv)) })) }; let mut version_infos = get_version_infos(); let mut i = 0; let mut packages = Vec::with_capacity(incomplete_snapshot.packages.len()); while let Some(result) = version_infos.next().await { + let result = result + .as_ref() + .map_err(|e: &SnapshotFromLockfileError| e.clone()) + .and_then(|(package_info, nv)| { + package_info + .version_info(nv) + .map_err(|e| SnapshotFromLockfileError::VersionNotFound { source: e }) + }); match result { Ok(version_info) => { let snapshot_package = &incomplete_snapshot.packages[i]; @@ -982,14 +988,15 @@ pub async fn snapshot_from_lockfile<'a>( packages.push(SerializedNpmResolutionSnapshotPackage { id: snapshot_package.id.clone(), dependencies: snapshot_package.dependencies.clone(), - dist: version_info.dist, + dist: version_info.dist.clone(), system: NpmResolutionPackageSystemInfo { - cpu: version_info.cpu, - os: version_info.os, + cpu: version_info.cpu.clone(), + os: version_info.os.clone(), }, optional_dependencies: version_info .optional_dependencies - .into_keys() + .keys() + .cloned() .collect(), bin: version_info.bin.clone(), scripts: version_info.scripts.clone(), From fa76c59cb48ec11fd682ef7565b19ab8796b46c6 Mon Sep 17 00:00:00 2001 From: David Sherret Date: Thu, 19 Dec 2024 18:48:12 -0500 Subject: [PATCH 2/2] reduce cloning in TestNpmRegistryApi as it was very noisy in benchmarks --- src/registry.rs | 51 +++++++++++++++++++++++++------------------------ 1 file changed, 26 insertions(+), 25 deletions(-) diff --git a/src/registry.rs b/src/registry.rs index dc2ad6e..125ff4f 100644 --- a/src/registry.rs +++ b/src/registry.rs @@ -388,7 +388,7 @@ pub trait NpmRegistryApi { /// purposes. Construct everything on the same thread. #[derive(Clone, Default, Debug)] pub struct TestNpmRegistryApi { - package_infos: Rc>>, + package_infos: Rc>>>, } impl TestNpmRegistryApi { @@ -396,7 +396,7 @@ impl TestNpmRegistryApi { let previous = self .package_infos .borrow_mut() - .insert(name.to_string(), info); + .insert(name.to_string(), Arc::new(info)); assert!(previous.is_none()); } @@ -415,8 +415,9 @@ impl TestNpmRegistryApi { pub fn with_package(&self, name: &str, f: impl FnOnce(&mut NpmPackageInfo)) { self.ensure_package(name); let mut infos = self.package_infos.borrow_mut(); - let info = infos.get_mut(name).unwrap(); - f(info); + let mut info = infos.get_mut(name).unwrap().as_ref().clone(); + f(&mut info); + infos.insert(name.to_string(), Arc::new(info)); } pub fn add_dist_tag(&self, package_name: &str, tag: &str, version: &str) { @@ -438,22 +439,22 @@ impl TestNpmRegistryApi { integrity: Option<&str>, ) { self.ensure_package(name); - let mut infos = self.package_infos.borrow_mut(); - let info = infos.get_mut(name).unwrap(); - let version = Version::parse_from_npm(version).unwrap(); - if !info.versions.contains_key(&version) { - info.versions.insert( - version.clone(), - NpmPackageVersionInfo { - version, - dist: NpmPackageVersionDistInfo { - integrity: integrity.map(|s| s.to_string()), + self.with_package(name, |info| { + let version = Version::parse_from_npm(version).unwrap(); + if !info.versions.contains_key(&version) { + info.versions.insert( + version.clone(), + NpmPackageVersionInfo { + version, + dist: NpmPackageVersionDistInfo { + integrity: integrity.map(|s| s.to_string()), + ..Default::default() + }, ..Default::default() }, - ..Default::default() - }, - ); - } + ); + } + }) } pub fn with_version_info( @@ -463,11 +464,11 @@ impl TestNpmRegistryApi { ) { let (name, version) = package; self.ensure_package_version(name, version); - let mut infos = self.package_infos.borrow_mut(); - let info = infos.get_mut(name).unwrap(); - let version = Version::parse_from_npm(version).unwrap(); - let version_info = info.versions.get_mut(&version).unwrap(); - f(version_info); + self.with_package(name, |info| { + let version = Version::parse_from_npm(version).unwrap(); + let version_info = info.versions.get_mut(&version).unwrap(); + f(version_info); + }); } pub fn add_dependency(&self, package: (&str, &str), entry: (&str, &str)) { @@ -537,11 +538,11 @@ impl NpmRegistryApi for TestNpmRegistryApi { name: &str, ) -> Result, NpmRegistryPackageInfoLoadError> { let infos = self.package_infos.borrow(); - Ok(Arc::new(infos.get(name).cloned().ok_or_else(|| { + Ok(infos.get(name).cloned().ok_or_else(|| { NpmRegistryPackageInfoLoadError::PackageNotExists { package_name: name.into(), } - })?)) + })?) } }