Skip to content

Commit 1d0714c

Browse files
nathanwhitbartlomieju
authored andcommitted
perf(npm): don't try to cache npm packages we've already cached (#28938)
With `nodeModulesDir: auto` (and `none`, to a lesser extent), we frequently repeatedly try to cache npm packages during resolution. On a (private) fresh project, I counted that `sync_resolution_with_fs` was called 478 times over the course of a fresh build. This reduces the number of calls to 8, and doing so speeds things up substantially. ``` ❯ hyperfine --warmup 2 -N "deno task build" "../deno/target/release-lite/deno task build" Benchmark 1: deno task build Time (mean ± σ): 3.652 s ± 0.042 s [User: 3.285 s, System: 2.399 s] Range (min … max): 3.587 s … 3.712 s 10 runs Benchmark 2: ../deno/target/release-lite/deno task build Time (mean ± σ): 1.356 s ± 0.007 s [User: 1.961 s, System: 1.108 s] Range (min … max): 1.345 s … 1.372 s 10 runs Summary ../deno/target/release-lite/deno task build ran 2.69 ± 0.03 times faster than deno task build ```
1 parent 55dde09 commit 1d0714c

File tree

1 file changed

+25
-1
lines changed

1 file changed

+25
-1
lines changed

cli/npm/installer/mod.rs

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ use deno_npm::NpmSystemInfo;
1313
use deno_resolver::npm::managed::NpmResolutionCell;
1414
use deno_runtime::colors;
1515
use deno_semver::package::PackageReq;
16+
use rustc_hash::FxHashSet;
1617

1718
pub use self::common::NpmPackageFsInstaller;
1819
use self::global::GlobalNpmPackageInstaller;
@@ -49,6 +50,7 @@ pub struct NpmInstaller {
4950
maybe_lockfile: Option<Arc<CliLockfile>>,
5051
npm_resolution: Arc<NpmResolutionCell>,
5152
top_level_install_flag: AtomicFlag,
53+
cached_reqs: tokio::sync::Mutex<FxHashSet<PackageReq>>,
5254
}
5355

5456
impl NpmInstaller {
@@ -100,6 +102,7 @@ impl NpmInstaller {
100102
npm_resolution_installer,
101103
maybe_lockfile,
102104
top_level_install_flag: Default::default(),
105+
cached_reqs: Default::default(),
103106
}
104107
}
105108

@@ -168,7 +171,28 @@ impl NpmInstaller {
168171
}
169172
if result.dependencies_result.is_ok() {
170173
if let Some(caching) = caching {
171-
result.dependencies_result = self.cache_packages(caching).await;
174+
// the async mutex is unfortunate, but needed to handle the edge case where two workers
175+
// try to cache the same package at the same time. we need to hold the lock while we cache
176+
// and since that crosses an await point, we need the async mutex.
177+
//
178+
// should have a negligible perf impact because acquiring the lock is still in the order of nanoseconds
179+
// while caching typically takes micro or milli seconds.
180+
let mut cached_reqs = self.cached_reqs.lock().await;
181+
let uncached = {
182+
packages
183+
.iter()
184+
.filter(|req| !cached_reqs.contains(req))
185+
.collect::<Vec<_>>()
186+
};
187+
188+
if !uncached.is_empty() {
189+
result.dependencies_result = self.cache_packages(caching).await;
190+
if result.dependencies_result.is_ok() {
191+
for req in uncached {
192+
cached_reqs.insert(req.clone());
193+
}
194+
}
195+
}
172196
}
173197
}
174198

0 commit comments

Comments
 (0)