diff --git a/Cargo.lock b/Cargo.lock index 649c7d0e..c7b804bc 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1037,6 +1037,7 @@ dependencies = [ "document-features", "dunce", "futures", + "hashbrown 0.17.0", "indexmap", "json-strip-comments", "mimalloc", diff --git a/Cargo.toml b/Cargo.toml index f3fd2a17..d93160c3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -91,6 +91,12 @@ indexmap = { version = "2.14.0", features = ["serde"] } json-strip-comments = "3.1.1" rustc-hash = { version = "2.1.2", default-features = false, features = ["std"] } thiserror = "2.0.18" +# Backing table for `src/interner.rs`. `HashTable` is the piece that off-the- +# shelf interners cannot offer: it looks up by a caller-supplied hash, so the +# interner reuses the `FxHash` `ResolverPath` already computed instead of hashing +# every path a second time. Version tracks what `indexmap` resolves to, so no +# extra copy of hashbrown enters the graph. +hashbrown = { version = "0.17.0", default-features = false, features = ["inline-more"] } pnp = { version = "0.12.9", optional = true } diff --git a/deny.toml b/deny.toml index 1c4e9974..6170711a 100644 --- a/deny.toml +++ b/deny.toml @@ -92,7 +92,8 @@ allow = [ "MIT", "Apache-2.0", "BSD-2-Clause", - "Unicode-3.0", + "BSD-2-Clause-Patent", + "Unicode-3.0", "Zlib" #"Apache-2.0 WITH LLVM-exception", ] diff --git a/examples/resolver.rs b/examples/resolver.rs index 21684121..ad9b5e2f 100644 --- a/examples/resolver.rs +++ b/examples/resolver.rs @@ -40,10 +40,10 @@ async fn main() { }; let mut sorted_file_deps = ctx.file_dependencies.iter().collect::>(); - sorted_file_deps.sort_by_key(|p| p.as_path()); + sorted_file_deps.sort_by_key(|p| p.as_std_path()); println!("file_deps: {:#?}", sorted_file_deps); let mut sorted_missing = ctx.missing_dependencies.iter().collect::>(); - sorted_missing.sort_by_key(|p| p.as_path()); + sorted_missing.sort_by_key(|p| p.as_std_path()); println!("missing_deps: {:#?}", sorted_missing); } diff --git a/src/cache.rs b/src/cache.rs index 14acda2b..80e8dcf1 100644 --- a/src/cache.rs +++ b/src/cache.rs @@ -5,7 +5,6 @@ use std::{ hash::{BuildHasherDefault, Hash, Hasher}, io, ops::Deref, - path::PathBuf, sync::Arc, }; @@ -18,7 +17,7 @@ use tokio::sync::OnceCell as OnceLock; use crate::{ context::ResolveContext as Ctx, package_json::{off_to_location, PackageJson}, - resolver_path::{hash_path, ResolverPath}, + resolver_path::{ResolverPath, ToResolverPath}, FileMetadata, FileSystem, JSONError, ResolveError, ResolveOptions, TsConfig, }; @@ -26,7 +25,7 @@ use crate::{ pub struct Cache { pub(crate) fs: Fs, paths: DashSet>, - tsconfigs: DashMap, BuildHasherDefault>, + tsconfigs: DashMap, BuildHasherDefault>, } impl Cache { @@ -44,7 +43,7 @@ impl Cache { } pub fn value(&self, path: &Utf8Path) -> CachedPath { - let hash = hash_path(path.as_std_path()); + let hash = hash_utf8_path(path); if let Some(cache_entry) = self.paths.get((hash, path).borrow() as &dyn CacheKey) { return cache_entry.clone(); } @@ -58,22 +57,27 @@ impl Cache { data } + /// `path` must already be interned by the caller. `config_file` is constant + /// per resolver but this is re-entered several times per resolve, so + /// interning here on every hit would serialize every thread on the same + /// shard lock for a hash that never changes; see `ResolverGeneric`'s + /// `tsconfig_config_file` field, computed once at construction. pub async fn tsconfig( &self, root: bool, - path: &Utf8Path, + path: ResolverPath, callback: F, // callback for modifying tsconfig with `extends` ) -> Result, ResolveError> where F: FnOnce(TsConfig) -> Fut + Send, Fut: Send + Future>, { - if let Some(tsconfig_ref) = self.tsconfigs.get(path.as_std_path()) { + if let Some(tsconfig_ref) = self.tsconfigs.get(&path) { return Ok(Arc::clone(tsconfig_ref.value())); } let meta = self.fs.metadata(path.as_std_path()).await.ok(); let tsconfig_path = if meta.is_some_and(|m| m.is_file) { - Cow::Borrowed(path) + Cow::Borrowed(path.as_utf8_path()) } else if meta.is_some_and(|m| m.is_dir) { Cow::Owned(path.join("tsconfig.json")) } else { @@ -96,9 +100,7 @@ impl Cache { })?; tsconfig = callback(tsconfig).await?; let tsconfig = Arc::new(tsconfig.build()); - self - .tsconfigs - .insert(path.as_std_path().to_path_buf(), Arc::clone(&tsconfig)); + self.tsconfigs.insert(path, Arc::clone(&tsconfig)); Ok(tsconfig) } } @@ -117,11 +119,17 @@ impl PartialEq for CachedPath { // Compare through std `Path`, not camino's `Utf8Path`. std `Path`/`Components` // equality has a raw-byte `memcmp` fast path (its own comment: "for hashmap // lookups"), whereas camino's `Utf8Path` always walks components with no fast - // path — that regressed this hottest cache-lookup equality. `as_std_path()` - // also preserves the per-platform semantics that match `hash_path` - // (byte-wise on Unix; component-wise on Windows, so `pack1/foo` and - // `pack1\foo` stay the same entry). A plain `as_str()` byte compare would be - // inconsistent with the component-wise hash on Windows and break dedup there. + // path — that regressed this hottest cache-lookup equality. + // + // Note this is *not* fully consistent with `hash_utf8_path` on Unix: this + // equality is component-wise (via std `Path`), so e.g. `/a/b` and `/a/b/` + // compare equal, while `hash_utf8_path` hashes the raw bytes and gives them + // different hashes on Unix. That gap predates this branch (the old + // `hash_path` had the same mismatch). It cannot produce a wrong lookup + // result — differing hashes just land in different `DashSet` buckets, so + // this `PartialEq` is never consulted for them — the only cost is a + // duplicate `CachedPath` entry (and duplicate metadata probe) for paths + // that differ only in trailing or repeated separators. self.0.path.as_std_path() == other.0.path.as_std_path() } } @@ -158,21 +166,26 @@ pub struct CachedPathImpl { path: Box, parent: Option, meta: OnceLock>, - canonicalized: OnceLock>, + canonicalized: OnceLock>, node_modules: OnceLock>, package_json: OnceLock>>, - /// Memoized `/package.json` `ResolverPath` for the - /// `missing_dependencies` push that fires on every `package_json` cache-hit - /// `None` (~97% of `package_json` calls in dep-tracking workloads). + /// Memoized interned form of `self.path`. Without it every `is_file` / + /// `is_dir` dependency push would re-enter the interner (bin lock + probe). + dep_path: std::sync::OnceLock, + /// Memoized `/node_modules`. `cached_node_modules` replays a + /// missing-dependency push on every cache hit where `node_modules` is + /// absent, which is most directory levels during an upward walk. + node_modules_dep_path: std::sync::OnceLock, + /// Memoized `/package.json` for the `missing_dependencies` push + /// that fires on every `package_json` cache-hit `None` (~97% of + /// `package_json` calls in dep-tracking workloads). package_json_dep_path: std::sync::OnceLock, } -impl From<&CachedPathImpl> for ResolverPath { - /// Reuse the cache-side `FxHash` (already computed in `Cache::value`); the - /// only remaining work is one `Arc::from(&Path)` to materialize the shared - /// path buffer for the `ResolveContext` sink. - fn from(cached: &CachedPathImpl) -> Self { - Self::from_parts(cached.hash, Arc::from(cached.path.as_std_path())) +impl ToResolverPath for CachedPathImpl { + #[inline] + fn to_resolver_path(&self) -> ResolverPath { + self.dep_path() } } @@ -186,16 +199,32 @@ impl CachedPathImpl { canonicalized: OnceLock::new(), node_modules: OnceLock::new(), package_json: OnceLock::new(), + dep_path: std::sync::OnceLock::new(), + node_modules_dep_path: std::sync::OnceLock::new(), package_json_dep_path: std::sync::OnceLock::new(), } } + fn dep_path(&self) -> ResolverPath { + self + .dep_path + .get_or_init(|| self.path.to_resolver_path()) + .clone() + } + + fn node_modules_dep_path(&self) -> ResolverPath { + self + .node_modules_dep_path + .get_or_init(|| self.path.join("node_modules").to_resolver_path()) + .clone() + } + /// Without this cache, each `None` cache-hit on `package_json` would - /// re-`join` + re-allocate the `Arc` + re-hash on every push. + /// re-`join` + re-intern on every push. fn package_json_dep_path(&self) -> ResolverPath { self .package_json_dep_path - .get_or_init(|| self.path.join("package.json").into()) + .get_or_init(|| self.path.join("package.json").to_resolver_path()) .clone() } @@ -243,11 +272,12 @@ impl CachedPathImpl { ) } - pub async fn realpath(&self, fs: &Fs) -> io::Result { + pub async fn realpath(&self, fs: &Fs) -> io::Result { // Cache hit: avoid the heap-allocated `Box::pin` for the cache-miss state machine - // by returning before delegating to the boxed recursive helper. + // by returning before delegating to the boxed recursive helper. Both arms are a + // refcount bump — neither allocates or re-enters the interner. if let Some(cached) = self.canonicalized.get() { - return Ok(cached.clone().unwrap_or_else(|| self.path.to_path_buf())); + return Ok(cached.clone().unwrap_or_else(|| self.dep_path())); } self.realpath_uncached(fs).await } @@ -255,7 +285,7 @@ impl CachedPathImpl { fn realpath_uncached<'a, Fs: FileSystem + Send + Sync>( &'a self, fs: &'a Fs, - ) -> BoxFuture<'a, io::Result> { + ) -> BoxFuture<'a, io::Result> { Box::pin(async move { self .canonicalized @@ -268,10 +298,12 @@ impl CachedPathImpl { return fs .canonicalize(self.path.as_std_path()) .await - .map(|path| Some(Utf8PathBuf::from_path_buf(path).expect("path should be UTF-8"))); + .map(|path| Some(path.to_resolver_path())); } if let Some(parent) = self.parent() { - let mut real_path = parent.realpath(fs).await?; + // Reuse the parent's realpath as a mutable buffer for the final + // component instead of rebuilding the whole path. + let mut real_path = parent.realpath(fs).await?.to_path_buf(); // Unnormalized paths (e.g. from alias values or absolute specifiers) can // end in `..`, where `file_name()` returns `None` and would silently drop // the component; POSIX semantics pop it after the parent is resolved. @@ -282,13 +314,13 @@ impl CachedPathImpl { } _ => {} } - return Ok(Some(real_path)); + return Ok(Some(real_path.to_resolver_path())); } Ok(None) }) .await .cloned() - .map(|r| r.unwrap_or_else(|| self.path.to_path_buf())) + .map(|r| r.unwrap_or_else(|| self.dep_path())) }) } @@ -313,8 +345,8 @@ impl CachedPathImpl { if let Some(nm) = self.node_modules.get() { // Replay ctx tracking from the cold path: module_directory -> is_dir calls // ctx.add_missing_dependency when node_modules doesn't exist on disk. - if nm.is_none() { - ctx.add_missing_dependency(self.path.join("node_modules")); + if nm.is_none() && ctx.missing_dependencies.is_some() { + ctx.add_missing_dependency(&self.node_modules_dep_path()); } return nm.clone(); } @@ -374,7 +406,7 @@ impl CachedPathImpl { Some(package_json) => ctx.add_file_dependency(&package_json.path), None => { if ctx.missing_dependencies.is_some() { - ctx.add_missing_dependency(self.package_json_dep_path()); + ctx.add_missing_dependency(&self.package_json_dep_path()); } } } @@ -389,13 +421,17 @@ impl CachedPathImpl { return Ok(None); }; let real_path = if options.symlinks { - self.realpath(fs).await?.join("package.json") + self + .realpath(fs) + .await? + .join("package.json") + .to_resolver_path() } else { - package_json_path.clone() + package_json_path.to_resolver_path() }; match PackageJson::parse( - package_json_path.clone().into(), - real_path.into(), + package_json_path.to_resolver_path(), + real_path, package_json_string, ) { Ok(v) => Ok(Some(Arc::new(v))), @@ -412,7 +448,7 @@ impl CachedPathImpl { if let Some(err) = serde_err { Err(ResolveError::from_serde_json_error( - package_json_path.into(), + package_json_path.as_std_path().to_path_buf(), &err, Some(package_json_string), )) @@ -420,7 +456,7 @@ impl CachedPathImpl { let (line, column) = off_to_location(&package_json_string, parse_err.index()); Err(ResolveError::JSON(JSONError { - path: package_json_path.into(), + path: package_json_path.as_std_path().to_path_buf(), message: parse_err.error().to_string(), line, column, @@ -441,12 +477,12 @@ impl CachedPathImpl { Ok(None) => { // Avoid an allocation by making this lazy if ctx.missing_dependencies.is_some() { - ctx.add_missing_dependency(self.package_json_dep_path()); + ctx.add_missing_dependency(&self.package_json_dep_path()); } } Err(_) => { if ctx.file_dependencies.is_some() { - ctx.add_file_dependency(self.path.join("package.json")); + ctx.add_file_dependency(&self.package_json_dep_path()); } } } @@ -467,8 +503,10 @@ impl Hash for dyn CacheKey + '_ { impl PartialEq for dyn CacheKey + '_ { fn eq(&self, other: &Self) -> bool { - // std `Path` equality (memcmp fast path + per-platform semantics matching - // `hash_path`); see `CachedPath`'s `PartialEq` for the full rationale. + // std `Path` equality (memcmp fast path); not fully consistent with + // `hash_utf8_path` on Unix — see `CachedPath`'s `PartialEq` for the full + // rationale and its benign consequence (duplicate cache entries, never a + // wrong lookup). self.tuple().1.as_std_path() == other.tuple().1.as_std_path() } } @@ -503,3 +541,129 @@ impl Hasher for IdentityHasher { self.0 } } + +/// Hash a path for the `CachedPath` `DashSet` key. +/// +/// Bulk-writes the raw bytes on unix — the std `Path::hash` impl walks +/// components (utf8 split + per-segment write), which is materially more +/// expensive on this, the hottest lookup in the crate. On other platforms it +/// goes through `Path` so `pack1/foo` and `pack1\foo` stay one entry, matching +/// `CachedPath`'s `PartialEq`. +/// +/// On Unix this bulk byte hash is *not* equivalent to `CachedPath`'s +/// `PartialEq`, which compares component-wise through std `Path`: paths that +/// differ only by a trailing or repeated separator (e.g. `/a/b` vs `/a/b/`) +/// are `PartialEq`-equal but hash differently here. That only costs a +/// duplicate `CachedPath` entry for such paths — it can never produce a wrong +/// lookup result, since mismatched hashes simply land in different buckets +/// and `PartialEq` is never consulted for them. +#[inline] +fn hash_utf8_path(path: &Utf8Path) -> u64 { + let mut hasher = FxHasher::default(); + #[cfg(unix)] + hasher.write(path.as_str().as_bytes()); + #[cfg(not(unix))] + path.as_std_path().hash(&mut hasher); + hasher.finish() +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::FileSystemOs; + + #[tokio::test] + async fn repeated_dep_conversions_reuse_one_memoized_handle() { + let cache = Cache::new(FileSystemOs::default()); + let cached = cache.value(Utf8Path::new("/a/b/c.js")); + + let first = cached.to_resolver_path(); + let second = cached.to_resolver_path(); + assert_eq!(first.as_str().as_ptr(), second.as_str().as_ptr()); + // Memoized: the second call must not have gone through the interner at all. + assert!(cached.dep_path.get().is_some()); + } + + #[tokio::test] + async fn node_modules_and_package_json_dep_paths_are_memoized() { + let cache = Cache::new(FileSystemOs::default()); + let cached = cache.value(Utf8Path::new("/a/b")); + + // Assert the structure, not the literal. Paths are interned verbatim, so + // the separator here is whatever `Utf8Path::join` picks: on Windows + // `/a/b` + `node_modules` is `/a/b\node_modules`, keeping the base's `/` + // and appending a `\`. Pinning that string would be testing camino, and + // it is exactly what broke this test before. What matters is that the memo + // yields `/`. + let node_modules = cached.node_modules_dep_path(); + let package_json = cached.package_json_dep_path(); + assert_eq!(node_modules.file_name(), Some("node_modules")); + assert_eq!(package_json.file_name(), Some("package.json")); + assert_eq!(node_modules.parent(), Some(Utf8Path::new("/a/b"))); + assert_eq!(package_json.parent(), Some(Utf8Path::new("/a/b"))); + + assert!(cached.node_modules_dep_path.get().is_some()); + assert!(cached.package_json_dep_path.get().is_some()); + assert_eq!( + cached.node_modules_dep_path().as_str().as_ptr(), + cached.node_modules_dep_path().as_str().as_ptr() + ); + } + + /// The reason this crate uses a refcounted interner rather than a + /// leak-forever one. + /// + /// rspack calls `resolver_factory.clear_cache()` on every rebuild (via + /// `plugin_driver.clear_cache`), so a dev server rebuilds this cache + /// continuously. If interned paths outlived it, RSS would climb + /// monotonically and never come back down. + /// + /// Uses a path no other test resolves, and asserts on that one handle's + /// refcount. A global tally would be flaky: the interner is process-wide, so + /// a shared fixture path is held by every parallel test's cache at once. + /// + /// The interner's table owns a reference of its own, so the floor here is 2, + /// not 1 — the entry itself is reclaimed later, by a sweep. + #[tokio::test] + async fn clearing_the_cache_releases_interned_paths() { + let cache = Cache::new(FileSystemOs::default()); + let unique = Utf8Path::new("/only/this/test/interns/this/exact/path.js"); + + let cached = cache.value(unique); + let handle = cached.to_resolver_path(); + drop(cached); + + let held = handle.refcount(); + assert!( + held >= 3, + "the cache and this test should both hold this path on top of the \ + interner's own reference, got refcount {held}" + ); + + cache.clear(); + + assert_eq!( + handle.refcount(), + 2, + "clear() must drop every handle the cache held, leaving this test's \ + handle and the interner's own reference" + ); + } + + #[test] + fn hash_utf8_path_matches_the_platform_contract() { + let path = Utf8Path::new("/a/b/c.js"); + let mut expected = FxHasher::default(); + #[cfg(unix)] + expected.write(path.as_str().as_bytes()); + #[cfg(not(unix))] + path.as_std_path().hash(&mut expected); + assert_eq!(hash_utf8_path(path), expected.finish()); + + // Equal paths hash equally — the invariant `CachedPath`'s DashSet depends on. + assert_eq!( + hash_utf8_path(Utf8Path::new("/x/y")), + hash_utf8_path(&Utf8PathBuf::from("/x/y")) + ); + } +} diff --git a/src/context.rs b/src/context.rs index 24b38196..84a0dcf5 100644 --- a/src/context.rs +++ b/src/context.rs @@ -1,6 +1,9 @@ use std::ops::{Deref, DerefMut}; -use crate::{error::ResolveError, resolver_path::ResolverPath}; +use crate::{ + error::ResolveError, + resolver_path::{ResolverPath, ToResolverPath}, +}; #[derive(Debug, Default, Clone)] pub struct ResolveContext(ResolveContextImpl); @@ -59,19 +62,21 @@ impl ResolveContext { self.missing_dependencies.replace(vec![]); } - // Accepts anything convertible to `ResolverPath`. The conversion (which - // includes the `Arc` allocation for `&Path` / `PathBuf` callers, or - // hash reuse for `&CachedPathImpl`) only runs inside the `Some` branch, so - // `resolve()` calls without a context still pay zero. - pub fn add_file_dependency>(&mut self, dep: P) { + // Accepts anything path-shaped, by reference. Taking `&P` (not `P`) matters: + // interning is a global-lock + hashtable probe with a permanent allocation on + // miss, so it must only happen inside the `Some` branch — a by-value `dep` + // would be evaluated (and interned) before the `if let` ever runs, silently + // paying that cost on every call even when `resolve()` was invoked without a + // context and these fields are `None`. + pub fn add_file_dependency(&mut self, dep: &P) { if let Some(deps) = &mut self.file_dependencies { - deps.push(dep.into()); + deps.push(dep.to_resolver_path()); } } - pub fn add_missing_dependency>(&mut self, dep: P) { + pub fn add_missing_dependency(&mut self, dep: &P) { if let Some(deps) = &mut self.missing_dependencies { - deps.push(dep.into()); + deps.push(dep.to_resolver_path()); } } diff --git a/src/interner.rs b/src/interner.rs new file mode 100644 index 00000000..b032fbcc --- /dev/null +++ b/src/interner.rs @@ -0,0 +1,667 @@ +//! A refcounted, sharded global string interner that reclaims its own entries. +//! +//! Exists because the two obvious off-the-shelf choices each fail one half of +//! what [`crate::ResolverPath`] needs: +//! +//! - `ustr` never frees, so a dev server's RSS climbs monotonically across +//! rebuilds. +//! - `internment::ArcIntern` frees, but its container hashes the string with +//! `ahash` behind a `DashMap`, so it re-hashes a string whose hash the caller +//! already computed — measured at 2.5 full-string hashes per intern, and +//! 1004 Ir per intern against `ustr`'s 161. +//! +//! This one takes the hash as a parameter and keeps it in the entry header, so +//! interning hashes the string exactly once and [`Interned::hash`] is a load +//! rather than a rehash. That header is also why entries are hand-allocated +//! instead of being `Arc`: an `Arc` is a fat pointer with nowhere to +//! put the hash, which would push every stored path from 8 bytes to 24. +//! +//! # Reclamation +//! +//! **The table owns a reference.** An entry's count is one for the table plus +//! one per live handle, so dropping a handle can never reach zero and never has +//! to free anything — [`Interned::drop`] is a bare decrement, with no lock, no +//! table lookup, and no race to lose. +//! +//! Freeing happens in [`Shard::sweep`], which removes every entry whose count +//! is back down to one. Sweeps are triggered by insertions, at a threshold that +//! scales with the shard, so each insertion amortizes to a constant number of +//! checks however large the table grows. Memory therefore comes back on a +//! bounded delay rather than instantly, which is what a dev server needs: the +//! table cannot grow without bound when paths are transient, and it holds +//! exactly the live set when they are retained. +//! +//! # Soundness +//! +//! Sweeping on `count == 1` rests on one invariant: +//! +//! > Every operation that takes a count from 1 to 2 happens under that shard's +//! > lock. +//! +//! - The only way to obtain a handle to a table entry is [`Interner::intern`], +//! which holds at least the read lock while it clones. +//! - A sweep holds the **write** lock, so no `intern` can be mid-clone. +//! - [`Interned::clone`] takes no lock, but its caller already holds a handle, +//! so the count it raises is at least 2 — never a 1. +//! +//! A count of 1 observed under the write lock therefore means the table is the +//! only owner and no one else can reach the entry. `Arc` reasons about its own +//! count the same way; the relaxed load is ordered by the lock, since a clone +//! that released the read lock happens-before the sweeper's write lock. + +use std::{ + alloc::{self, Layout}, + ptr::NonNull, + slice, str, + sync::{ + atomic::{AtomicUsize, Ordering}, + PoisonError, RwLock, + }, +}; + +use hashbrown::HashTable; + +/// Interner entry: header followed by the string bytes in the same allocation. +/// +/// One allocation rather than a header plus a `Box` because interning is +/// allocation-bound, and doubling the allocation count per entry is directly +/// visible in the resolver benchmarks. +#[repr(C)] +struct Entry { + /// One for the table, plus one per live [`Interned`]. + count: AtomicUsize, + /// The caller's hash of the string. Stored so the handle stays one pointer + /// wide and [`Interned::hash`] costs a load instead of a re-hash. + hash: u64, + len: usize, + // `len` bytes of UTF-8 follow. +} + +/// Offset of the string bytes within an entry allocation. +/// +/// `[u8]` has alignment 1, so `Layout::extend` places it immediately after the +/// header with no padding — `Entry::layout` debug-asserts that this constant +/// and the computed offset agree. Spelling it as a constant keeps `Entry::str`, +/// which every `as_str()` goes through, down to one add. +const PAYLOAD_OFFSET: usize = std::mem::size_of::(); + +impl Entry { + /// Layout of a `len`-byte entry, plus the offset of the string bytes. + /// + /// `alloc` and `dealloc` must agree exactly, so both go through here. + fn layout(len: usize) -> (Layout, usize) { + let (layout, offset) = Layout::new::() + .extend(Layout::array::(len).expect("interned string length fits in a Layout")) + .expect("interner entry layout"); + debug_assert_eq!(offset, PAYLOAD_OFFSET); + (layout.pad_to_align(), offset) + } + + /// Allocate an entry holding `s`, with the count already at two: one for the + /// table it is about to enter, one for the handle the caller gets back. + fn alloc(s: &str, hash: u64) -> NonNull { + let (layout, offset) = Self::layout(s.len()); + // SAFETY: `layout` has non-zero size — the header alone is non-empty. + let Some(ptr) = NonNull::new(unsafe { alloc::alloc(layout) }) else { + alloc::handle_alloc_error(layout) + }; + // `alloc` returns memory aligned to `layout`, which starts from + // `Layout::new::()`, so this is `Self`'s alignment by construction. + let ptr = ptr.cast::(); + // SAFETY: `ptr` is freshly allocated for `layout`, so it is valid for + // writes of the header, and of `s.len()` bytes at `offset`. The two ranges + // cannot overlap `s`, which lives in the caller's memory. + unsafe { + ptr.as_ptr().write(Self { + count: AtomicUsize::new(2), + hash, + len: s.len(), + }); + ptr + .as_ptr() + .cast::() + .add(offset) + .copy_from_nonoverlapping(s.as_ptr(), s.len()); + } + ptr + } + + /// # Safety + /// + /// `ptr` must come from [`Entry::alloc`], must not have been deallocated, + /// and must already be out of its shard with a count of one. + unsafe fn dealloc(ptr: NonNull) { + // SAFETY: the caller guarantees `ptr` is a live entry, so reading `len` + // reproduces the layout it was allocated with. + let (layout, _) = Self::layout(unsafe { ptr.as_ref() }.len); + // SAFETY: same allocation and same layout as `alloc` used. + unsafe { alloc::dealloc(ptr.as_ptr().cast::(), layout) } + } + + /// # Safety + /// + /// `ptr` must point at a live entry, and the returned reference must not + /// outlive the handle or table slot that keeps it alive. + #[inline] + unsafe fn str<'a>(ptr: NonNull) -> &'a str { + // SAFETY: the caller guarantees `ptr` is live. + let len = unsafe { ptr.as_ref() }.len; + // SAFETY: `alloc` wrote exactly `len` bytes of a `&str` at + // `PAYLOAD_OFFSET`, so the range is initialized, in bounds, and UTF-8. + unsafe { + let bytes = slice::from_raw_parts(ptr.as_ptr().cast::().add(PAYLOAD_OFFSET), len); + str::from_utf8_unchecked(bytes) + } + } +} + +/// `NonNull` is neither `Send` nor `Sync`, so it cannot go in a `static` +/// directly. Entries are: their bytes are immutable and `count` is atomic. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +struct EntryPtr(NonNull); + +// SAFETY: see the type's doc comment — an entry is immutable apart from its +// atomic count, and freeing one requires the shard's write lock. +unsafe impl Send for EntryPtr {} +// SAFETY: as above. +unsafe impl Sync for EntryPtr {} + +/// Enough shards that a build's worth of resolver threads rarely collide, +/// matching what `ustr` uses. Must stay a power of two — [`shard_index`] slices +/// the index out of the hash's top bits. +const SHARDS: usize = 64; +const SHARD_BITS: u32 = SHARDS.trailing_zeros(); +const _: () = assert!(SHARDS.is_power_of_two()); + +/// Smallest table a shard bothers sweeping. Below this, walking the table costs +/// more than the handful of entries it could reclaim. +const MIN_SWEEP_THRESHOLD: usize = 64; + +struct Shard { + table: HashTable, + inserts_since_sweep: usize, +} + +impl Shard { + const fn new() -> Self { + Self { + table: HashTable::new(), + inserts_since_sweep: 0, + } + } + + /// Sweep once insertions since the last sweep reach half the table. Scaling + /// the threshold with the table is what keeps this amortized constant: a + /// sweep walks `len` entries and buys `len / 2` insertions, so the per-insert + /// cost stays flat however large the table grows. + fn threshold(&self) -> usize { + MIN_SWEEP_THRESHOLD.max(self.table.len() / 2) + } + + /// Remove every entry no handle refers to, returning them for the caller to + /// free once the lock is released. + fn sweep(&mut self) -> Vec { + self.inserts_since_sweep = 0; + self + .table + .extract_if(|&mut EntryPtr(entry)| { + // SAFETY: entries leave the table only here, under the write lock, so + // everything still reachable is live. + unsafe { entry.as_ref() }.count.load(Ordering::Acquire) == 1 + }) + .collect() + } +} + +/// A string interner. Distinct instances have distinct tables. +/// +/// The crate uses one global instance; this is a type rather than a set of free +/// functions so that tests can work against an isolated interner instead of +/// racing each other through process-wide state. +pub struct Interner { + shards: [RwLock; SHARDS], +} + +impl Interner { + pub const fn new() -> Self { + Self { + shards: [const { RwLock::new(Shard::new()) }; SHARDS], + } + } + + /// Intern `s`, whose hash the caller has already computed. + /// + /// `hash` must be a deterministic function of `s` alone; two calls with the + /// same string and different hashes would create two entries for it and break + /// the one-entry-per-string guarantee that [`Interned::ptr_eq`] relies on. It + /// is not otherwise constrained — [`crate::ResolverPath`] deliberately passes a + /// hash that folds equivalent Windows spellings together, which only makes + /// those spellings share a shard and a bucket. + pub fn intern(&self, s: &str, hash: u64) -> Interned { + let lock = &self.shards[shard_index(hash)]; + + // Hit path: a read lock, so threads interning paths that are already known + // — the common case once a build is warm — never wait on each other. + { + let shard = lock.read().unwrap_or_else(PoisonError::into_inner); + if let Some(ptr) = find(&shard.table, s, hash) { + return ptr; + } + } + + // Miss path. The read lock was released above, so re-check: another thread + // may have interned `s` in that window. + let mut shard = lock.write().unwrap_or_else(PoisonError::into_inner); + if let Some(ptr) = find(&shard.table, s, hash) { + return ptr; + } + + let ptr = Entry::alloc(s, hash); + shard + .table + .insert_unique(hash, EntryPtr(ptr), |&EntryPtr(entry)| { + // SAFETY: reachable table entries are live. + unsafe { entry.as_ref() }.hash + }); + shard.inserts_since_sweep += 1; + let doomed = if shard.inserts_since_sweep > shard.threshold() { + shard.sweep() + } else { + Vec::new() + }; + drop(shard); + + // Freeing outside the lock keeps the critical section to table work. + free_all(doomed); + Interned { ptr } + } + + /// Reclaim every entry no handle refers to, across all shards. + /// + /// Sweeps are otherwise driven by insertions, so a shard that has gone quiet + /// keeps its dead entries indefinitely. Call this to collect them anyway. + /// + /// Test-gated only because nothing in the crate calls it yet. The natural + /// caller is `Cache::clear()` — rspack runs it on every rebuild — which would + /// turn the bounded-delay reclamation into immediate reclamation. + #[cfg(test)] + pub fn sweep(&self) { + for lock in &self.shards { + let doomed = lock.write().unwrap_or_else(PoisonError::into_inner).sweep(); + free_all(doomed); + } + } + + /// How many entries the interner holds. Diagnostic and test use — the count + /// includes entries that are dead but not yet swept. + #[cfg(test)] + pub fn len(&self) -> usize { + self + .shards + .iter() + .map(|lock| { + let shard = lock.read().unwrap_or_else(PoisonError::into_inner); + let len = shard.table.len(); + drop(shard); + len + }) + .sum() + } +} + +impl Default for Interner { + fn default() -> Self { + Self::new() + } +} + +impl Drop for Interner { + /// Free what the table still owns. Without this, dropping an `Interner` + /// leaks every entry in it — the global one never drops, but tests build + /// their own, and a leak checker sees them. + /// + /// Entries a handle still refers to are deliberately leaked instead. An + /// [`Interned`] does not borrow from its `Interner`, so freeing those would + /// leave the holder with a dangling pointer; outliving the interner is a bug + /// at the call site, and leaking is the safe way to report it. + fn drop(&mut self) { + for lock in &mut self.shards { + let shard = lock.get_mut().unwrap_or_else(PoisonError::into_inner); + for EntryPtr(entry) in shard.table.drain() { + // SAFETY: `&mut self` means no other thread can reach these entries, + // and the count tells us whether any handle still can. + if unsafe { entry.as_ref() }.count.load(Ordering::Acquire) == 1 { + // SAFETY: table-only, and just drained out of the table. + unsafe { Entry::dealloc(entry) } + } else { + debug_assert!( + false, + "an Interned outlived its Interner; the handle is now dangling" + ); + } + } + } + } +} + +/// Look `s` up and take a reference to it. The caller must hold the shard lock. +#[inline] +fn find(table: &HashTable, s: &str, hash: u64) -> Option { + let &EntryPtr(ptr) = table.find(hash, |&EntryPtr(entry)| { + // SAFETY: entries leave the table only under the write lock, which the + // caller's lock excludes, so everything reachable here is live. + unsafe { Entry::str(entry) == s } + })?; + // SAFETY: as above. + unsafe { ptr.as_ref() } + .count + .fetch_add(1, Ordering::Relaxed); + Some(Interned { ptr }) +} + +/// # Panics +/// +/// Never; the entries come from a sweep, which only yields what it removed. +fn free_all(doomed: Vec) { + for EntryPtr(entry) in doomed { + // SAFETY: a sweep yields only entries it removed from its table under the + // write lock, each with a count of one — the table's own reference. + unsafe { Entry::dealloc(entry) } + } +} + +/// Sharded by the **top** bits of the hash: `hashbrown` indexes buckets with +/// the low bits, so sharding on those would give every entry in a shard the +/// same bucket index. +#[inline] +fn shard_index(hash: u64) -> usize { + (hash >> (u64::BITS - SHARD_BITS)) as usize +} + +/// The process-wide interner. +static GLOBAL: Interner = Interner::new(); + +/// Intern `s` into the process-wide interner. See [`Interner::intern`]. +#[inline] +pub fn intern(s: &str, hash: u64) -> Interned { + GLOBAL.intern(s, hash) +} + +/// Entry count of the process-wide interner. See [`Interner::len`]. +#[cfg(test)] +pub fn live_entries() -> usize { + GLOBAL.len() +} + +/// A refcounted handle to a globally interned string. +/// +/// One pointer wide. Not `Copy`: the count has to be maintained. +pub struct Interned { + ptr: NonNull, +} + +// SAFETY: `Entry` is immutable apart from `count`, which is atomic, and an +// entry is freed only under its shard's write lock (see module docs). +unsafe impl Send for Interned {} +// SAFETY: as above — sharing `&Interned` only exposes immutable string bytes. +unsafe impl Sync for Interned {} + +impl Interned { + #[inline] + pub fn as_str(&self) -> &str { + // SAFETY: `self` holds a reference, so the entry is live, and the returned + // borrow is tied to `self`. + unsafe { Entry::str(self.ptr) } + } + + /// The hash handed to [`intern`], read back from the entry header. + #[inline] + pub fn hash(&self) -> u64 { + // SAFETY: `self` holds a reference, so the entry is live. + unsafe { self.ptr.as_ref() }.hash + } + + /// Whether both handles name the same interner entry. + /// + /// Interning guarantees one entry per distinct string, so this settles + /// equality for byte-identical strings. + #[inline] + pub fn ptr_eq(&self, other: &Self) -> bool { + self.ptr == other.ptr + } + + /// One for the table plus one per live handle. Test and diagnostic use — the + /// count is a snapshot and can change concurrently. + #[cfg(test)] + pub(crate) fn refcount(&self) -> usize { + // SAFETY: `self` holds a reference, so the entry is live. + unsafe { self.ptr.as_ref() }.count.load(Ordering::Acquire) + } +} + +impl Clone for Interned { + #[inline] + fn clone(&self) -> Self { + // No lock: the caller holds a handle, so this raises a count of at least + // two, never the 1 -> 2 transition a sweep must not miss. + // SAFETY: `self` holds a reference, so the entry is live. + unsafe { self.ptr.as_ref() } + .count + .fetch_add(1, Ordering::Relaxed); + Self { ptr: self.ptr } + } +} + +impl Drop for Interned { + #[inline] + fn drop(&mut self) { + // The table's own reference keeps this from reaching zero, so there is + // nothing to free and nothing to lock. `Release` pairs with the acquire the + // sweeper gets from the shard's write lock. + // SAFETY: `self` holds a reference, so the entry is live. + unsafe { self.ptr.as_ref() } + .count + .fetch_sub(1, Ordering::Release); + } +} + +#[cfg(test)] +mod tests { + use std::{sync::Arc, thread}; + + use super::{Interned, Interner}; + + fn h(s: &str) -> u64 { + use std::hash::Hasher as _; + let mut hasher = rustc_hash::FxHasher::default(); + hasher.write(s.as_bytes()); + hasher.finish() + } + + /// Tests use their own interner rather than the global one, so they cannot + /// perturb each other's counts through process-wide state. + fn get(interner: &Interner, s: &str) -> Interned { + interner.intern(s, h(s)) + } + + #[test] + fn equal_strings_share_one_entry() { + let interner = Interner::new(); + let a = get(&interner, "/a/b/c.js"); + let b = get(&interner, "/a/b/c.js"); + assert!(a.ptr_eq(&b)); + assert_eq!(a.as_str(), "/a/b/c.js"); + assert_eq!(interner.len(), 1); + } + + #[test] + fn different_strings_get_different_entries() { + let interner = Interner::new(); + assert!(!get(&interner, "/a.js").ptr_eq(&get(&interner, "/b.js"))); + assert_eq!(interner.len(), 2); + } + + #[test] + fn the_empty_string_round_trips() { + // Zero-length payload is the one case where the entry is header-only. + let interner = Interner::new(); + let a = get(&interner, ""); + assert_eq!(a.as_str(), ""); + assert!(a.ptr_eq(&get(&interner, ""))); + } + + #[test] + fn hash_is_returned_verbatim() { + // The interner must not re-derive the hash: `ResolverPath` relies on getting + // back exactly what it computed, including a Windows-folded value that + // does not match the stored bytes. + let interner = Interner::new(); + assert_eq!( + interner.intern("/x.js", 0xDEAD_BEEF_1234_5678).hash(), + 0xDEAD_BEEF_1234_5678 + ); + } + + #[test] + fn colliding_hashes_stay_distinct_entries() { + // Windows folding hands equal hashes to different strings on purpose, so + // the table must disambiguate by bytes rather than trusting the hash. + let interner = Interner::new(); + let a = interner.intern("/one", 0x5555_5555_5555_5555); + let b = interner.intern("/two", 0x5555_5555_5555_5555); + assert!(!a.ptr_eq(&b)); + assert_eq!(a.as_str(), "/one"); + assert_eq!(b.as_str(), "/two"); + } + + #[test] + fn refcount_counts_the_table_plus_every_handle() { + let interner = Interner::new(); + let a = get(&interner, "/a.js"); + assert_eq!(a.refcount(), 2, "the table holds one reference of its own"); + let b = a.clone(); + assert_eq!(a.refcount(), 3); + drop(b); + assert_eq!(a.refcount(), 2); + } + + #[test] + fn sweeping_reclaims_entries_no_handle_refers_to() { + let interner = Interner::new(); + drop(get(&interner, "/dead.js")); + let alive = get(&interner, "/alive.js"); + assert_eq!(interner.len(), 2, "dropping a handle does not remove it"); + + interner.sweep(); + + assert_eq!(interner.len(), 1); + assert_eq!( + alive.as_str(), + "/alive.js", + "a held entry must survive the sweep" + ); + } + + #[test] + fn a_held_entry_survives_repeated_sweeps() { + let interner = Interner::new(); + let held = get(&interner, "/held.js"); + for _ in 0..10 { + interner.sweep(); + assert!(held.ptr_eq(&get(&interner, "/held.js"))); + } + assert_eq!(held.as_str(), "/held.js"); + } + + #[test] + fn one_shot_strings_do_not_grow_the_table_without_bound() { + // The property the insert-driven sweep exists for: a build that interns a + // hundred thousand paths and immediately discards them must not accumulate + // them. The bound is roughly SHARDS * MIN_SWEEP_THRESHOLD. + let interner = Interner::new(); + for i in 0..100_000 { + drop(get(&interner, &format!("/transient/{i}.js"))); + } + let len = interner.len(); + assert!(len < 8_192, "table grew to {len}, which is not bounded"); + } + + #[test] + fn the_insert_that_triggers_a_sweep_keeps_its_own_entry() { + // A sweep fires from inside the very call that inserted a new entry, and + // that entry has no handle yet — the caller's `Interned` is built after the + // sweep returns. What saves it is `Entry::alloc` starting the count at two: + // one for the table, one pre-paid for the handle about to be returned. Get + // that wrong and an insert would free the string it just returned. + let interner = Interner::new(); + let mut before = 0; + let mut sweeps = 0; + + for i in 0..20_000 { + let key = format!("/trigger/{i}.js"); + let handle = get(&interner, &key); + + // A shrinking table is the only externally visible sign that this + // particular insert swept. + let after = interner.len(); + if after < before { + sweeps += 1; + assert_eq!( + handle.as_str(), + key, + "the insert that swept lost the entry it had just returned" + ); + assert!( + get(&interner, &key).ptr_eq(&handle), + "the swept-through entry was evicted and re-allocated" + ); + } + before = after; + drop(handle); + } + + assert!( + sweeps > 0, + "no insert ever tripped a sweep, so this asserted nothing" + ); + } + + #[test] + fn concurrent_intern_and_drop_is_sound() { + // Threads race interning and discarding a small key space while sweeps fire + // underneath them, driven by the one-shot strings. A sweep that freed an + // entry another thread had just cloned surfaces here under ASan or Miri. + let interner = Arc::new(Interner::new()); + let barrier = Arc::new(std::sync::Barrier::new(8)); + let threads: Vec<_> = (0..8) + .map(|t| { + let interner = Arc::clone(&interner); + let barrier = Arc::clone(&barrier); + thread::spawn(move || { + barrier.wait(); + for i in 0..5000 { + let shared = format!("/shared/{}", i % 4); + let handle = get(&interner, &shared); + assert_eq!(handle.as_str(), shared); + let cloned = handle.clone(); + drop(handle); + assert_eq!(cloned.as_str(), shared); + + let once = format!("/once/{t}/{i}"); + assert_eq!(get(&interner, &once).as_str(), once); + } + }) + }) + .collect(); + for thread in threads { + thread.join().expect("interner race thread panicked"); + } + + interner.sweep(); + assert_eq!( + interner.len(), + 0, + "every handle is gone, so a full sweep must empty the table" + ); + } +} diff --git a/src/lib.rs b/src/lib.rs index 76e188f9..d19c43a3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -53,6 +53,7 @@ mod cache; mod context; mod error; mod file_system; +mod interner; mod options; mod package_json; mod path; @@ -96,7 +97,7 @@ pub use crate::{ }, package_json::{JSONValue, ModuleType, PackageJson}, resolution::Resolution, - resolver_path::ResolverPath, + resolver_path::{IdentityHasher, ResolverPath, ResolverPathSet, ToResolverPath}, }; type ResolveResult = Result, ResolveError>; @@ -105,10 +106,10 @@ type ResolveResult = Result, ResolveError>; #[derive(Debug, Default, Clone)] pub struct ResolveContext { /// Files that were found on file system - pub file_dependencies: FxHashSet, + pub file_dependencies: ResolverPathSet, /// Dependencies that were not found on file system - pub missing_dependencies: FxHashSet, + pub missing_dependencies: ResolverPathSet, } /// Resolver with the current operating system as the file system @@ -122,8 +123,13 @@ pub struct ResolverGeneric { alias_trie: AliasTrie, /// Pre-built byte-trie over `options.fallback` keys. fallback_trie: AliasTrie, + /// `options.tsconfig.config_file` interned once at construction, since it is + /// constant per resolver but looked up on every `require_without_parse` entry + /// into `load_tsconfig_paths`. Interning it per-call would serialize every + /// thread on the same interner shard for a hash that never changes. + tsconfig_config_file: Option, #[cfg(feature = "yarn_pnp")] - pnp_manifest: Arc>, + pnp_manifest: Arc>, /// Paths that have been searched and confirmed to have no `.pnp.cjs` reachable by filesystem walk. #[cfg(feature = "yarn_pnp")] pnp_no_manifest_cache: Arc>, @@ -148,11 +154,16 @@ impl ResolverGeneric { let options = options.sanitize(); let alias_trie = AliasTrie::build(&options.alias); let fallback_trie = AliasTrie::build(&options.fallback); + let tsconfig_config_file = options + .tsconfig + .as_ref() + .map(|t| t.config_file.to_resolver_path()); Self { options, cache: Arc::new(Cache::new(Fs::default())), alias_trie, fallback_trie, + tsconfig_config_file, #[cfg(feature = "yarn_pnp")] pnp_manifest: Arc::new(arc_swap::ArcSwapOption::empty()), #[cfg(feature = "yarn_pnp")] @@ -167,11 +178,16 @@ impl ResolverGeneric { let options = options.sanitize(); let alias_trie = AliasTrie::build(&options.alias); let fallback_trie = AliasTrie::build(&options.fallback); + let tsconfig_config_file = options + .tsconfig + .as_ref() + .map(|t| t.config_file.to_resolver_path()); Self { options, cache: Arc::new(Cache::new(file_system)), alias_trie, fallback_trie, + tsconfig_config_file, #[cfg(feature = "yarn_pnp")] pnp_manifest: Arc::new(arc_swap::ArcSwapOption::empty()), #[cfg(feature = "yarn_pnp")] @@ -186,11 +202,16 @@ impl ResolverGeneric { let options = options.sanitize(); let alias_trie = AliasTrie::build(&options.alias); let fallback_trie = AliasTrie::build(&options.fallback); + let tsconfig_config_file = options + .tsconfig + .as_ref() + .map(|t| t.config_file.to_resolver_path()); Self { options, cache: Arc::clone(&self.cache), alias_trie, fallback_trie, + tsconfig_config_file, #[cfg(feature = "yarn_pnp")] pnp_manifest: Arc::clone(&self.pnp_manifest), #[cfg(feature = "yarn_pnp")] @@ -750,18 +771,18 @@ impl ResolverGeneric { &self, cached_path: &CachedPath, ctx: &mut Ctx, - ) -> Result { + ) -> Result { if self.options.symlinks { cached_path .realpath(&self.cache.fs) .await .map(|path| { - ctx.add_file_dependency(path.as_path()); + ctx.add_file_dependency(&path); path }) .map_err(ResolveError::from) } else { - Ok(cached_path.to_path_buf()) + Ok(cached_path.to_resolver_path()) } } @@ -1003,7 +1024,10 @@ impl ResolverGeneric { #[cfg(feature = "yarn_pnp")] #[cfg_attr(feature = "enable_instrument", tracing::instrument(level=tracing::Level::DEBUG, skip_all, fields(path = cached_path.path().as_str())))] - fn find_pnp_manifest(&self, cached_path: &CachedPath) -> Option> { + fn find_pnp_manifest( + &self, + cached_path: &CachedPath, + ) -> Option> { // 1. Already have a manifest → return it (covers global cache paths too) if let Some(manifest) = self.pnp_manifest.load_full() { return Some(manifest); @@ -1032,7 +1056,7 @@ impl ResolverGeneric { tracing::debug!("use manifest path: {:?}", manifest_path); let manifest = pnp::load_pnp_manifest(&manifest_path).ok()?; - let manifest = Arc::new((manifest_path, manifest)); + let manifest = Arc::new((manifest_path.to_resolver_path(), manifest)); let previous = self .pnp_manifest @@ -1184,8 +1208,7 @@ impl ResolverGeneric { // 5. let MATCH = PACKAGE_EXPORTS_RESOLVE(pathToFileURL(SCOPE), // "." + X.slice("name".length), `package.json` "exports", ["node", "require"]) // defined in the ESM resolver. - let package_url = - Utf8Path::from_path(package_json.directory()).expect("path should be UTF-8"); + let package_url = package_json.directory(); // Note: The subpath is not prepended with a dot on purpose // because `package_exports_resolve` matches subpath without the leading dot. for exports in package_json.exports_fields(&self.options.exports_fields) { @@ -1286,9 +1309,7 @@ impl ResolverGeneric { } ctx.with_resolving_alias(new_specifier.to_string()); ctx.with_fully_specified(false); - let cached_path = self - .cache - .value(Utf8Path::from_path(package_json.directory()).expect("path should be UTF-8")); + let cached_path = self.cache.value(package_json.directory()); self .require(&cached_path, new_specifier, ctx) .await @@ -1492,15 +1513,19 @@ impl ResolverGeneric { let Some(tsconfig_options) = &self.options.tsconfig else { return Ok(None); }; + let config_file = self + .tsconfig_config_file + .clone() + .expect("computed alongside options.tsconfig at construction"); let tsconfig = self .load_tsconfig( /* root */ true, - Utf8Path::from_path(&tsconfig_options.config_file).expect("path should be UTF-8"), + config_file, &tsconfig_options.references, ) .await?; for dependency in &tsconfig.file_dependencies { - ctx.add_file_dependency(dependency.as_path()); + ctx.add_file_dependency(dependency); } let paths = tsconfig.resolve(cached_path.path(), specifier); for path in paths { @@ -1516,7 +1541,7 @@ impl ResolverGeneric { fn load_tsconfig<'a>( &'a self, root: bool, - path: &'a Utf8Path, + path: ResolverPath, references: &'a TsconfigReferences, ) -> BoxFuture<'a, Result, ResolveError>> { let fut = async move { @@ -1589,7 +1614,7 @@ impl ResolverGeneric { .cache .tsconfig( /* root */ true, - &reference_tsconfig_path, + reference_tsconfig_path.to_resolver_path(), |mut reference_tsconfig| { let current_path = current_path.clone(); async move { @@ -1666,7 +1691,7 @@ impl ResolverGeneric { let extended_tsconfig = self .load_tsconfig( /* root */ false, - &extended_tsconfig_path, + extended_tsconfig_path.to_resolver_path(), &TsconfigReferences::Disabled, ) .await?; @@ -1938,7 +1963,7 @@ impl ResolverGeneric { if specifier == "#" { return Err(ResolveError::InvalidModuleSpecifier( specifier.to_string(), - package_json.path.clone(), + package_json.path.as_std_path().to_path_buf(), )); } } @@ -1946,7 +1971,7 @@ impl ResolverGeneric { .package_imports_exports_resolve( specifier, imports, - Utf8Path::from_path(package_json.directory()).expect("path should be UTF-8"), + package_json.directory(), /* is_imports */ true, &self.options.condition_names, ctx, @@ -1962,7 +1987,7 @@ impl ResolverGeneric { if has_imports { Err(ResolveError::PackageImportNotDefined( specifier.to_string(), - package_json.path.clone(), + package_json.path.as_std_path().to_path_buf(), )) } else { Ok(None) diff --git a/src/package_json/simd.rs b/src/package_json/simd.rs index d9759856..a4488cbc 100644 --- a/src/package_json/simd.rs +++ b/src/package_json/simd.rs @@ -5,7 +5,6 @@ use std::{ fmt::{Debug, Formatter}, marker::PhantomData, - path::{Path, PathBuf}, }; use camino::Utf8Path; @@ -17,7 +16,7 @@ use simd_json::{ to_borrowed_value, BorrowedValue, Error as SimdParseError, ObjectHasher, }; -use crate::{path::PathUtil, ResolveError}; +use crate::{path::PathUtil, ResolveError, ResolverPath}; pub type JSONMap<'a> = simd_json::borrowed::Object<'a>; @@ -135,10 +134,10 @@ impl Default for JSONCell { #[derive(Debug, Default)] pub struct PackageJson { /// Path to `package.json`. Contains the `package.json` filename. - pub path: PathBuf, + pub path: ResolverPath, /// Realpath to `package.json`. Contains the `package.json` filename. - pub realpath: PathBuf, + pub realpath: ResolverPath, /// The "name" field defines your package's name. /// The "name" field can be used in addition to the "exports" field to self-reference a package using its name. @@ -190,7 +189,11 @@ impl From for ParseError { impl PackageJson { /// # Panics /// # Errors - pub(crate) fn parse(path: PathBuf, realpath: PathBuf, json: Vec) -> Result { + pub(crate) fn parse( + path: ResolverPath, + realpath: ResolverPath, + json: Vec, + ) -> Result { if json.starts_with(&BOM) { return Err(ParseError { message: "BOM character found".to_string(), @@ -276,7 +279,7 @@ impl PackageJson { /// # Panics /// /// * When the package.json path is misconfigured. - pub fn directory(&self) -> &Path { + pub fn directory(&self) -> &Utf8Path { debug_assert!(self .realpath .file_name() @@ -373,7 +376,7 @@ impl PackageJson { return Self::alias_value(path, value); } } else { - let dir = Utf8Path::from_path(self.path.parent().unwrap()).expect("path should be UTF-8"); + let dir = self.path.parent().unwrap(); for (key, value) in object { let joined = dir.normalize_with(key.to_string()); if joined == path { diff --git a/src/resolution.rs b/src/resolution.rs index 0acab18f..cac13138 100644 --- a/src/resolution.rs +++ b/src/resolution.rs @@ -4,14 +4,12 @@ use std::{ sync::Arc, }; -use camino::Utf8PathBuf; - -use crate::package_json::PackageJson; +use crate::{package_json::PackageJson, resolver_path::ResolverPath}; /// The final path resolution with optional `?query` and `#fragment` #[derive(Clone)] pub struct Resolution { - pub(crate) path: Utf8PathBuf, + pub(crate) path: ResolverPath, /// path query `?query`, contains `?`. pub(crate) query: Option, @@ -46,9 +44,17 @@ impl Resolution { self.path.as_std_path() } + /// Returns the interned path without query and fragment. + /// + /// Zero-copy: hand this to a downstream store instead of `path()` to avoid + /// re-allocating and re-hashing the string. + pub fn resolver_path(&self) -> ResolverPath { + self.path.clone() + } + /// Returns the path without query and fragment pub fn into_path_buf(self) -> PathBuf { - self.path.into_std_path_buf() + self.path.as_std_path().to_path_buf() } /// Returns the path query `?query`, contains the leading `?` @@ -68,7 +74,7 @@ impl Resolution { /// Returns the full path with query and fragment pub fn full_path(&self) -> PathBuf { - let mut path = self.path.clone().into_string(); + let mut path = self.path.as_str().to_owned(); if let Some(query) = &self.query { path.push_str(query); } @@ -93,3 +99,21 @@ async fn test() { assert_eq!(resolution.full_path(), PathBuf::from("foo?query#fragment")); assert_eq!(resolution.into_path_buf(), PathBuf::from("foo")); } + +#[tokio::test] +async fn resolver_path_accessor_is_the_same_pointer_as_the_stored_path() { + let resolution = Resolution { + path: "foo".into(), + query: None, + fragment: None, + package_json: None, + }; + assert_eq!(resolution.resolver_path().as_str(), "foo"); + assert_eq!( + resolution.resolver_path().as_str().as_ptr(), + ResolverPath::new("foo").as_str().as_ptr() + ); + // The legacy accessors keep their signatures. + assert_eq!(resolution.path(), Path::new("foo")); + assert_eq!(resolution.into_path_buf(), PathBuf::from("foo")); +} diff --git a/src/resolver_path.rs b/src/resolver_path.rs index 45e3883f..c4f633e2 100644 --- a/src/resolver_path.rs +++ b/src/resolver_path.rs @@ -1,216 +1,729 @@ -#[cfg(unix)] -use std::os::unix::ffi::OsStrExt; use std::{ + collections::HashSet, fmt, - hash::{Hash, Hasher}, + hash::{BuildHasherDefault, Hash, Hasher}, ops::Deref, path::{Path, PathBuf}, - sync::Arc, }; use camino::{Utf8Path, Utf8PathBuf}; use rustc_hash::FxHasher; -/// A path returned in [`crate::ResolveContext`] dependencies, paired with a -/// precomputed `FxHash` of the path bytes. +use crate::interner::{self, Interned}; + +/// A globally interned UTF-8 path. +/// +/// One pointer wide. Every distinct path string is stored once no matter how +/// many consumers hold it, so the same path handed to N downstream stores +/// costs N pointers rather than N copies. +/// +/// Equality is a pointer comparison whenever the strings are byte-identical, +/// and hashing is a single `u64` load from the entry header, so `ResolverPathSet` +/// lookups cost one `write_u64`. On Windows, spellings that differ only in +/// separators or drive case are distinct entries, so both fall back to +/// `Path`'s component semantics to keep them one key. Paths are stored +/// verbatim — the folding lives in the comparison, not in what gets interned. +/// +/// The interner is a static inside this crate, so rspack shares it by using +/// this type rather than by agreeing on a third-party crate version. /// -/// Downstream consumers (rspack) place these into hash collections keyed by -/// the precomputed hash, avoiding repeated hashing of long absolute paths on -/// every insert and lookup. +/// # Lifetime /// -/// Hash and equality are kept aligned per platform so the standard -/// `a == b ⇒ hash(a) == hash(b)` contract holds: -/// - On Unix, both hash and equality compare the raw `OsStr` bytes (fast bulk -/// `write`; matches the resolver's cache-side `Cache::value` hash). -/// - On other platforms, both go through `Path` (component-walked hash and -/// normalized equality), so e.g. `pack1/foo` and `pack1\\foo` are treated as -/// the same dependency on Windows — same behavior `PathBuf` had before. +/// Entries are refcounted and **dropped when the last handle goes away**. This +/// is what lets `Cache::clear()` actually return memory: rspack clears the +/// resolver cache on every rebuild, so a leak-forever interner would make RSS +/// climb monotonically in a dev server. The flip side is that `as_str()` +/// borrows from `self` rather than being `'static`. #[derive(Clone)] -pub struct ResolverPath { - hash: u64, - path: Arc, +pub struct ResolverPath(Interned); + +/// Hash a path the way `PartialEq` compares it, so `a == b` implies equal +/// hashes on every platform. +/// +/// Unix hashes the raw bytes — `hash_utf8_path` has always compared resolver +/// paths byte-wise there. Windows walks components, matching `Path`'s own +/// `Hash`, so `C:/a/b`, `C:\a\b`, `C:\a\b\` and `c:\a\b` land together. +#[inline] +fn hash_path_str(s: &str) -> u64 { + let mut hasher = FxHasher::default(); + #[cfg(windows)] + Path::new(s).hash(&mut hasher); + #[cfg(not(windows))] + hasher.write(s.as_bytes()); + hasher.finish() } -impl ResolverPath { - pub fn new(path: Arc) -> Self { - let hash = hash_path(&path); - Self { hash, path } +#[inline] +const fn is_sep(c: u8) -> bool { + c == b'/' || c == b'\\' +} + +/// Rewrite `s` into canonical Windows form: `\` separators, no repeated +/// separators, no trailing separator (roots excepted), and an uppercase +/// drive letter — matching the folding std's `Path` applies via +/// `Prefix::Disk` before comparing paths in `Hash`/`Eq`. +/// +/// Returns `None` when `s` is already canonical — the common case — so the +/// caller interns the original `&str` without allocating. +/// +/// Paths beginning with two separators (UNC and verbatim, e.g. `\\?\C:\x` or +/// `\\server\share`) pass through untouched, including the verbatim disk +/// prefix's drive-letter case: std folds that case too (`VerbatimDisk` goes +/// through the same `parse_drive`), so leaving it alone here is a known, +/// accepted divergence from `Path`'s `Hash`/`Eq` for verbatim paths — the +/// `dunce` dependency already keeps the resolver off UNC/verbatim paths, so +/// this code path isn't reached in practice. +/// +/// Platform-independent on purpose. camino's `Utf8Path::components()` picks its +/// separator set at compile time, so it cannot express Windows semantics on a +/// unix host — this would otherwise be untestable off Windows. +// NOT CURRENTLY WIRED UP. `ResolverPath::new` used to call this on Windows, but +// `ResolverPath` also carries strings that are not canonical filesystem paths — +// rspack stores caller-supplied dependency specifiers such as +// `addBuildDependency("./build.txt")` in path sets — and rewriting those +// changed values observable through the public JS API (`./build.txt` came back +// as `.\build.txt`). Correctness of the stored string wins over dedup for now; +// the function and its tests are kept so re-enabling it behind a separate +// canonicalizing constructor stays cheap. +#[cfg_attr( + not(test), + expect(dead_code, reason = "kept for reference; see comment above") +)] +fn normalize_windows_separators(s: &str) -> Option { + let bytes = s.as_bytes(); + + if bytes.len() >= 2 && is_sep(bytes[0]) && is_sep(bytes[1]) { + return None; + } + + // A lowercase drive letter (`c:\...`) is not canonical on its own — std + // folds it to uppercase (`Prefix::Disk`) before comparing paths — even + // when every separator is already fine. + let mut needs_rewrite = bytes.len() >= 2 && bytes[0].is_ascii_lowercase() && bytes[1] == b':'; + let mut prev_was_sep = false; + for (i, &c) in bytes.iter().enumerate() { + if is_sep(c) { + // A trailing separator only counts as canonical when it is the root + // itself (`\`) or a drive root (`C:\`). + if c == b'/' || prev_was_sep || (i + 1 == bytes.len() && i > 0 && bytes[i - 1] != b':') { + needs_rewrite = true; + break; + } + prev_was_sep = true; + } else { + prev_was_sep = false; + } + } + if !needs_rewrite { + return None; } - /// Construct without recomputing the hash. + let fold_drive = bytes.len() >= 2 && bytes[1] == b':'; + let mut out = String::with_capacity(s.len()); + let mut prev_was_sep = false; + for (i, ch) in s.chars().enumerate() { + if ch == '/' || ch == '\\' { + if !prev_was_sep { + out.push('\\'); + } + prev_was_sep = true; + } else if i == 0 && fold_drive && ch.is_ascii_lowercase() { + out.push(ch.to_ascii_uppercase()); + prev_was_sep = false; + } else { + out.push(ch); + prev_was_sep = false; + } + } + + if out.len() > 1 && out.ends_with('\\') && !out.ends_with(":\\") { + out.pop(); + } + + Some(out) +} + +impl ResolverPath { + /// Intern `path` verbatim and return a handle to it. /// - /// # Precondition - /// `hash` MUST equal `hash_path(path)`. Violating this breaks `HashSet`'s - /// bucketing invariant — entries become unfindable and deduplication stops - /// working. Not `unsafe` because the failure mode is a logic bug rather - /// than UB. + /// The string is stored exactly as given — rspack puts caller-supplied + /// specifiers such as `addBuildDependency("./build.txt")` into path sets and + /// reads them back through the JS API, so rewriting them here would change + /// observable values. See the note on `normalize_windows_separators`. + /// + /// On Windows this means distinct spellings of one path (`C:/a/b` vs + /// `C:\a\b`) intern to distinct handles. They still compare and hash equal — + /// `PartialEq`/`Hash` fold them via `Path`'s component semantics rather than + /// the stored bytes — so using them as set or map keys still dedups. #[inline] - pub(crate) fn from_parts(hash: u64, path: Arc) -> Self { - Self { hash, path } + pub fn new(path: &str) -> Self { + Self(interner::intern(path, hash_path_str(path))) } + /// Borrows from `self`, not `'static`: the entry is freed once the last + /// handle drops. #[inline] - pub fn as_path(&self) -> &Path { - &self.path + pub fn as_str(&self) -> &str { + self.0.as_str() } #[inline] - pub fn as_arc(&self) -> &Arc { - &self.path + pub fn as_utf8_path(&self) -> &Utf8Path { + Utf8Path::new(self.as_str()) } #[inline] - pub fn into_arc(self) -> Arc { - self.path + pub fn as_std_path(&self) -> &Path { + Path::new(self.as_str()) } - /// The precomputed `FxHash` of the path bytes. + /// The hash used for set and map lookups, computed once at construction and + /// read back from the interner entry header. + /// + /// Matches [`PartialEq`] per platform: raw bytes on unix, `Path` components + /// on Windows. #[inline] pub fn precomputed_hash(&self) -> u64 { - self.hash + self.0.hash() } -} -/// Hash a path with `FxHasher`, matching the bytes-on-unix optimization used by -/// the resolver's internal cache so [`ResolverPath`] values constructed from a -/// `CachedPath` produce the same `u64` as values constructed from a `&Path`. -#[inline] -pub fn hash_path(path: &Path) -> u64 { - let mut hasher = FxHasher::default(); - // The std `Path::hash` impl walks components (utf8 split + per-segment - // write); a single bulk `write` of the raw bytes is materially cheaper on - // the resolver hot path. - #[cfg(unix)] - hasher.write(path.as_os_str().as_bytes()); - #[cfg(not(unix))] - path.hash(&mut hasher); - hasher.finish() + /// How many handles currently share this interned string. Test/diagnostic + /// use — the count is a snapshot and can change concurrently. + #[cfg(test)] + pub(crate) fn refcount(&self) -> usize { + self.0.refcount() + } } -impl Hash for ResolverPath { +impl Default for ResolverPath { + /// The empty path — a handle to a real interned `""`, not a dangling one. #[inline] - fn hash(&self, state: &mut H) { - state.write_u64(self.hash); + fn default() -> Self { + Self::new("") } } impl PartialEq for ResolverPath { - /// Mirror `hash_path`'s per-platform scheme so the `a == b ⇒ hash(a) == - /// hash(b)` invariant holds: raw `OsStr` bytes on Unix (matches the - /// bulk-byte hash), component-normalized `Path::eq` elsewhere (matches - /// `Path::hash`). + /// Byte-identical strings share one interner entry, so the pointer check + /// settles them — on unix that is the whole story, matching the byte-wise + /// comparison `hash_utf8_path` has always used for resolver paths. + /// + /// Windows additionally folds spellings: `Path`'s `Eq` walks components, so + /// `C:/a/b`, `C:\a\b`, `C:\a\\b`, `C:\a\b\` and `c:\a\b` are all one path. + /// Since [`ResolverPath::new`] stores the string verbatim those are distinct + /// entries, and only the component walk can tell they are equal. The hash + /// check in front of it is a cheap reject, valid because `hash_path_str` + /// hashes components on Windows too. + #[inline] fn eq(&self, other: &Self) -> bool { - #[cfg(unix)] + if self.0.ptr_eq(&other.0) { + return true; + } + #[cfg(windows)] { - self.path.as_os_str() == other.path.as_os_str() + self.precomputed_hash() == other.precomputed_hash() + && self.as_std_path() == other.as_std_path() } - #[cfg(not(unix))] + #[cfg(not(windows))] { - self.path == other.path + false } } } impl Eq for ResolverPath {} +impl Hash for ResolverPath { + /// One `write_u64` of the hash computed at construction. + /// + /// The single write is load-bearing, not stylistic: [`ResolverPathSet`] is keyed + /// by [`IdentityHasher`], whose `write` only reads a value when handed + /// exactly 8 bytes and **silently yields 0** otherwise. Hashing the path + /// inline here would feed it component-sized writes and collapse every key + /// into one bucket with no error. + #[inline] + fn hash(&self, state: &mut H) { + state.write_u64(self.precomputed_hash()); + } +} + impl Deref for ResolverPath { - type Target = Path; + type Target = Utf8Path; + #[inline] fn deref(&self) -> &Self::Target { - &self.path + self.as_utf8_path() + } +} + +impl AsRef for ResolverPath { + #[inline] + fn as_ref(&self) -> &Utf8Path { + self.as_utf8_path() } } impl AsRef for ResolverPath { + #[inline] fn as_ref(&self) -> &Path { - &self.path + self.as_std_path() } } -impl From for ResolverPath { - fn from(path: PathBuf) -> Self { - Self::new(Arc::from(path)) +impl AsRef for ResolverPath { + #[inline] + fn as_ref(&self) -> &str { + self.as_str() + } +} + +impl fmt::Debug for ResolverPath { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + self.as_utf8_path().fmt(f) + } +} + +impl fmt::Display for ResolverPath { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.write_str(self.as_str()) + } +} + +/// A `HashSet` keyed by the interner's precomputed hash. +/// +/// Spelled with [`IdentityHasher`] rather than a set-local hasher so rspack's +/// `ArcPathSet` is the same concrete type and can take these sets by +/// `mem::take` instead of re-bucketing every element. +pub type ResolverPathSet = HashSet>; + +/// Passes an already-computed hash straight through. +/// +/// [`ResolverPath::hash`] writes the hash it computed at construction, so re-mixing +/// it here would be wasted work. Downstream spells its own path sets with this +/// exact type so it can `mem::take` ours instead of re-bucketing every element. +/// +/// Only `write_u64` is meaningful. Anything else is a misuse — the key type is +/// not `ResolverPath` — and would silently produce 0 for every key, so it panics in +/// debug builds rather than quietly degrading the map into a linked list. +#[derive(Default, Clone, Copy)] +pub struct IdentityHasher(u64); + +impl Hasher for IdentityHasher { + #[inline] + fn write(&mut self, bytes: &[u8]) { + debug_assert!( + false, + "IdentityHasher only accepts write_u64; got a {}-byte write. The key \ + type is probably not ResolverPath.", + bytes.len() + ); + // Release builds: fold the bytes rather than yielding 0, so a misuse + // degrades performance instead of collapsing every key into one bucket. + let mut h = FxHasher::default(); + h.write(bytes); + self.0 = h.finish(); + } + + #[inline] + fn write_u64(&mut self, n: u64) { + self.0 = n; + } + + #[inline] + fn finish(&self) -> u64 { + self.0 } } -impl From<&Path> for ResolverPath { - fn from(path: &Path) -> Self { - Self::new(Arc::from(path)) +/// Convert any path-shaped value into an interned [`ResolverPath`]. +/// +/// The two `std::path` implementations panic on non-UTF-8 input, matching what +/// the resolver already does at every `Path -> Utf8Path` boundary (see the +/// `expect("path should be UTF-8")` calls in `lib.rs` and `cache.rs`). +pub trait ToResolverPath { + fn to_resolver_path(&self) -> ResolverPath; +} + +impl ToResolverPath for str { + #[inline] + fn to_resolver_path(&self) -> ResolverPath { + ResolverPath::new(self) } } -impl From<&PathBuf> for ResolverPath { - fn from(path: &PathBuf) -> Self { - Self::new(Arc::from(path.as_path())) +impl ToResolverPath for String { + #[inline] + fn to_resolver_path(&self) -> ResolverPath { + ResolverPath::new(self) } } -impl From> for ResolverPath { - fn from(path: Arc) -> Self { - Self::new(path) +impl ToResolverPath for Utf8Path { + #[inline] + fn to_resolver_path(&self) -> ResolverPath { + ResolverPath::new(self.as_str()) } } -// The resolver stores paths internally as UTF-8 (`camino`); these accept the -// internal representation directly while keeping the stored buffer an -// `Arc` so the `as_arc`/`into_arc` output contract stays unchanged. -impl From for ResolverPath { - fn from(path: Utf8PathBuf) -> Self { - Self::new(Arc::from(path.into_std_path_buf())) +impl ToResolverPath for Utf8PathBuf { + #[inline] + fn to_resolver_path(&self) -> ResolverPath { + ResolverPath::new(self.as_str()) } } -impl From<&Utf8Path> for ResolverPath { - fn from(path: &Utf8Path) -> Self { - Self::new(Arc::from(path.as_std_path())) +impl ToResolverPath for Path { + #[inline] + fn to_resolver_path(&self) -> ResolverPath { + ResolverPath::new(self.to_str().expect("path should be UTF-8")) } } -impl fmt::Debug for ResolverPath { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.path.fmt(f) +impl ToResolverPath for PathBuf { + #[inline] + fn to_resolver_path(&self) -> ResolverPath { + self.as_path().to_resolver_path() + } +} + +impl ToResolverPath for ResolverPath { + #[inline] + fn to_resolver_path(&self) -> ResolverPath { + self.clone() + } +} + +impl From<&T> for ResolverPath { + #[inline] + fn from(value: &T) -> Self { + value.to_resolver_path() + } +} + +impl From for ResolverPath { + #[inline] + fn from(value: Utf8PathBuf) -> Self { + value.to_resolver_path() + } +} + +impl From for ResolverPath { + #[inline] + fn from(value: PathBuf) -> Self { + value.to_resolver_path() } } #[cfg(test)] mod tests { + use std::{ + collections::HashSet, + hash::{Hash, Hasher}, + }; + + use camino::Utf8Path; + use super::*; #[test] - fn hash_is_path_byte_hash() { - let p: &Path = Path::new("/a/b/c.js"); - let rp = ResolverPath::from(p); - assert_eq!(rp.precomputed_hash(), hash_path(p)); + fn default_is_the_empty_path() { + assert_eq!(ResolverPath::default().as_str(), ""); } + /// The handle must stay one pointer wide. It is stored by the million + /// downstream — every dependency set, every `Vec` — so widening it + /// (by caching the hash beside the pointer instead of in the interner entry, + /// say) shows up directly as `memcpy` in the resolver benchmarks. #[test] - fn equal_paths_have_equal_hashes() { - let a = ResolverPath::from(PathBuf::from("/x/y")); - let b = ResolverPath::from(Path::new("/x/y")); + fn the_handle_is_one_pointer_wide() { + assert_eq!( + std::mem::size_of::(), + std::mem::size_of::() + ); + } + + #[test] + fn same_string_is_same_pointer() { + let a = ResolverPath::new("/a/b/c.js"); + let b = ResolverPath::new("/a/b/c.js"); + assert_eq!(a.as_str().as_ptr(), b.as_str().as_ptr()); assert_eq!(a, b); - assert_eq!(a.precomputed_hash(), b.precomputed_hash()); } #[test] - fn writes_u64_into_hasher() { - use std::{collections::HashSet, hash::BuildHasherDefault}; + fn different_strings_are_not_equal() { + assert_ne!(ResolverPath::new("/a/b"), ResolverPath::new("/a/c")); + } - #[derive(Default)] - struct IdHasher(u64); - impl Hasher for IdHasher { - fn write(&mut self, _: &[u8]) { - unreachable!() - } - fn write_u64(&mut self, n: u64) { - self.0 = n; - } - fn finish(&self) -> u64 { - self.0 - } + /// `ResolverPathSet` is keyed by [`IdentityHasher`], so this is the hash that + /// actually decides bucketing. Folding it through that hasher — rather than a + /// generic one — is what proves `Hash` still delivers exactly 8 bytes: the + /// identity hasher silently yields 0 for any other width. + fn set_hash(p: &ResolverPath) -> u64 { + let mut hasher = IdentityHasher::default(); + p.hash(&mut hasher); + hasher.finish() + } + + #[cfg(windows)] + #[test] + fn windows_spellings_of_one_path_are_equal_and_hash_alike() { + // Stored verbatim, so these are genuinely distinct interned handles — + // the folding has to come from `PartialEq`/`Hash`, not from interning. + let canonical = ResolverPath::new(r"C:\a\b"); + for spelling in ["C:/a/b", r"C:/a\b", r"C:\a\\b", r"C:\a\b\", r"c:\a\b"] { + let p = ResolverPath::new(spelling); + assert_ne!( + p.as_str(), + canonical.as_str(), + "{spelling:?} must be stored verbatim, not rewritten" + ); + assert_eq!(p, canonical, "{spelling:?} should compare equal"); + assert_eq!( + set_hash(&p), + set_hash(&canonical), + "{spelling:?} hashes differently, which would break set bucketing" + ); } + } + + #[cfg(windows)] + #[test] + fn windows_distinct_paths_stay_distinct() { + assert_ne!(ResolverPath::new(r"C:\a\b"), ResolverPath::new(r"C:\a\c")); + assert_ne!(ResolverPath::new(r"C:\a\b"), ResolverPath::new(r"D:\a\b")); + } + + #[cfg(windows)] + #[test] + fn windows_equal_paths_are_one_key_in_a_set() { + let mut set: ResolverPathSet = HashSet::default(); + set.insert(ResolverPath::new(r"C:\a\b")); + assert!(set.contains(&ResolverPath::new("C:/a/b"))); + assert!(set.contains(&ResolverPath::new(r"c:\a\b"))); + set.insert(ResolverPath::new("C:/a/b")); + assert_eq!(set.len(), 1, "equal spellings must collapse to one entry"); + } + + #[test] + fn hash_delivers_eight_bytes_to_the_identity_hasher() { + // Guards both branches of `Hash`: a regression that forwarded + // `Path::hash` straight through would write component-sized chunks, and + // `IdentityHasher` would silently return 0 rather than fail. + assert_ne!( + set_hash(&ResolverPath::new("/some/long/path/segment.js")), + 0 + ); + } + + #[test] + fn equal_paths_always_hash_equal() { + let a = ResolverPath::new("/a/b"); + let b = ResolverPath::new("/a/b"); + assert_eq!(a, b); + assert_eq!(set_hash(&a), set_hash(&b)); + } + + #[test] + // Unix only: there `Hash` forwards the interner's precomputed byte hash + // directly. Windows folds spellings through `Path`'s component semantics + // instead, so the two deliberately differ — that branch is covered by + // `windows_spellings_of_one_path_are_equal_and_hash_alike` and by + // `hash_delivers_eight_bytes_to_the_identity_hasher`. + #[cfg(not(windows))] + fn hash_is_the_precomputed_hash() { + let p = ResolverPath::new("/x/y"); + let mut hasher = std::collections::hash_map::DefaultHasher::new(); + p.hash(&mut hasher); + // The value written into the hasher is the precomputed one, not a + // re-hash of the bytes. + let mut expected = std::collections::hash_map::DefaultHasher::new(); + expected.write_u64(p.precomputed_hash()); + assert_eq!(hasher.finish(), expected.finish()); + } + + #[test] + fn works_in_an_identity_hashed_set() { + let mut set: ResolverPathSet = HashSet::default(); + set.insert(ResolverPath::new("/a/b")); + assert!(set.contains(&ResolverPath::new("/a/b"))); + assert!(!set.contains(&ResolverPath::new("/a/c"))); + } + + #[test] + fn derefs_to_utf8_path() { + let p = ResolverPath::new("/a/b/c.js"); + assert_eq!(p.file_name(), Some("c.js")); + assert_eq!(p.parent(), Some(Utf8Path::new("/a/b"))); + assert_eq!(p.extension(), Some("js")); + assert_eq!(p.join("d.ts"), Utf8Path::new("/a/b/c.js/d.ts")); + } + + #[test] + fn debug_prints_the_path_not_the_handle() { + // `Debug`/`Display` render the path, not the handle wrapping it. + // Platform-independent because interning is verbatim — the string comes + // back exactly as passed in on every platform. + let p = ResolverPath::new("/a/b"); + assert_eq!(format!("{p:?}"), "\"/a/b\""); + assert_eq!(format!("{p}"), "/a/b"); + } + + #[test] + fn as_ref_targets_compile_and_agree() { + let p = ResolverPath::new("/a/b"); + let as_utf8: &Utf8Path = p.as_ref(); + let std_path: &std::path::Path = p.as_ref(); + let str_ref: &str = p.as_ref(); + assert_eq!(as_utf8.as_str(), str_ref); + assert_eq!(std_path, std::path::Path::new("/a/b")); + } + + #[test] + fn identity_hasher_receives_eight_bytes() { + // `IdentityHasher::write` silently produces a 0 hash unless it gets + // exactly 8 bytes. `ResolverPath::hash` goes through the default `write_u64`, + // which forwards `u64::to_ne_bytes()` — exactly 8. Guard that invariant so + // a future change to `Hash` cannot silently collapse every key to bucket 0. + let mut hasher = IdentityHasher::default(); + ResolverPath::new("/some/long/path/that/is/not/eight/bytes").hash(&mut hasher); + assert_ne!(hasher.finish(), 0); + } + + #[test] + fn already_canonical_windows_paths_need_no_rewrite() { + assert_eq!(normalize_windows_separators(r"C:\a\b"), None); + assert_eq!(normalize_windows_separators(r"C:\"), None); + assert_eq!(normalize_windows_separators(r"\"), None); + assert_eq!(normalize_windows_separators("a"), None); + } + + #[test] + fn forward_slashes_become_backslashes() { + assert_eq!( + normalize_windows_separators("C:/a/b").as_deref(), + Some(r"C:\a\b") + ); + assert_eq!( + normalize_windows_separators(r"C:/a\b").as_deref(), + Some(r"C:\a\b") + ); + } + + #[test] + fn repeated_separators_collapse() { + assert_eq!( + normalize_windows_separators(r"C:\a\\b").as_deref(), + Some(r"C:\a\b") + ); + assert_eq!( + normalize_windows_separators("C://a//b").as_deref(), + Some(r"C:\a\b") + ); + } + + #[test] + fn trailing_separator_is_dropped_but_roots_survive() { + assert_eq!( + normalize_windows_separators(r"C:\a\b\").as_deref(), + Some(r"C:\a\b") + ); + assert_eq!(normalize_windows_separators("C:/").as_deref(), Some(r"C:\")); + assert_eq!(normalize_windows_separators(r"C:\"), None); + } + + #[test] + fn unc_and_verbatim_paths_pass_through_untouched() { + // std disables separator normalization for verbatim prefixes, and `dunce` + // already keeps the resolver off UNC paths — leaving both alone is both + // correct and the conservative choice. + assert_eq!(normalize_windows_separators(r"\\?\C:\a/b"), None); + assert_eq!(normalize_windows_separators("//?/C:/a/b"), None); + assert_eq!(normalize_windows_separators(r"\\server\share\a"), None); + } + + #[test] + fn non_ascii_segments_survive_normalization() { + assert_eq!( + normalize_windows_separators("C:/项目/源码").as_deref(), + Some(r"C:\项目\源码") + ); + } + + #[test] + fn lowercase_drive_letter_is_folded_to_uppercase() { + assert_eq!( + normalize_windows_separators(r"c:\a\b").as_deref(), + Some(r"C:\a\b") + ); + assert_eq!( + normalize_windows_separators("c:/a/b").as_deref(), + Some(r"C:\a\b") + ); + assert_eq!( + normalize_windows_separators(r"c:\").as_deref(), + Some(r"C:\") + ); + // Already uppercase and otherwise canonical: still no rewrite. + assert_eq!(normalize_windows_separators(r"C:\a\b"), None); + } + + #[test] + fn case_is_folded_only_on_the_drive_letter() { + assert_eq!( + normalize_windows_separators(r"c:\Foo\BAR.ts").as_deref(), + Some(r"C:\Foo\BAR.ts") + ); + // No drive letter at all — nothing to fold. + assert_eq!(normalize_windows_separators(r"relative\Path"), None); + } + + // `equivalent_windows_spellings_intern_to_one_pointer` used to live here. It + // asserted every spelling interned to one pointer, which only held while + // `ResolverPath::new` normalized. Interning is verbatim now, so the spellings are + // distinct handles that compare and hash equal instead — asserted by + // `windows_spellings_of_one_path_are_equal_and_hash_alike` above, which also + // checks the strings really are stored distinctly. + + #[test] + fn to_resolver_path_accepts_every_path_flavor() { + use camino::Utf8PathBuf; + + let expected = ResolverPath::new("/a/b"); + assert_eq!("/a/b".to_resolver_path(), expected); + assert_eq!(String::from("/a/b").to_resolver_path(), expected); + assert_eq!(Utf8Path::new("/a/b").to_resolver_path(), expected); + assert_eq!(Utf8PathBuf::from("/a/b").to_resolver_path(), expected); + assert_eq!(std::path::Path::new("/a/b").to_resolver_path(), expected); + assert_eq!( + std::path::PathBuf::from("/a/b").to_resolver_path(), + expected + ); + assert_eq!(expected.to_resolver_path(), expected); + } + + #[test] + fn from_impls_match_to_resolver_path() { + use camino::Utf8PathBuf; + + let expected = ResolverPath::new("/a/b"); + assert_eq!(ResolverPath::from("/a/b"), expected); + assert_eq!(ResolverPath::from(Utf8Path::new("/a/b")), expected); + assert_eq!(ResolverPath::from(Utf8PathBuf::from("/a/b")), expected); + assert_eq!(ResolverPath::from(std::path::Path::new("/a/b")), expected); + assert_eq!( + ResolverPath::from(std::path::PathBuf::from("/a/b")), + expected + ); + } + + #[test] + #[should_panic(expected = "path should be UTF-8")] + #[cfg(unix)] + fn non_utf8_std_path_panics_like_the_rest_of_the_resolver() { + use std::{ffi::OsStr, os::unix::ffi::OsStrExt}; - let mut set: HashSet> = HashSet::default(); - set.insert(ResolverPath::from(Path::new("/a/b"))); - assert!(set.contains(&ResolverPath::from(PathBuf::from("/a/b")))); + let bad = std::path::Path::new(OsStr::from_bytes(b"/a/\xff/b")); + let _ = bad.to_resolver_path(); } } diff --git a/src/tests/alias.rs b/src/tests/alias.rs index d42922a1..947e145b 100644 --- a/src/tests/alias.rs +++ b/src/tests/alias.rs @@ -237,13 +237,21 @@ async fn alias_is_full_path() { } for path in ctx.file_dependencies { - assert_eq!(path.as_path(), path.normalize(), "{path:?}"); - check_slash(&path); + assert_eq!( + path.as_std_path(), + path.as_std_path().normalize(), + "{path:?}" + ); + check_slash(path.as_std_path()); } for path in ctx.missing_dependencies { - assert_eq!(path.as_path(), path.normalize(), "{path:?}"); - check_slash(&path); + assert_eq!( + path.as_std_path(), + path.as_std_path().normalize(), + "{path:?}" + ); + check_slash(path.as_std_path()); if let Some(path) = path.parent() { assert!(!path.is_file(), "{path:?} must not be a file"); } diff --git a/src/tests/dependencies.rs b/src/tests/dependencies.rs index be9fe04f..683bf9d6 100644 --- a/src/tests/dependencies.rs +++ b/src/tests/dependencies.rs @@ -59,10 +59,8 @@ mod warm_cache_missing_dependencies { mod windows { use std::path::PathBuf; - use rustc_hash::FxHashSet; - use super::super::memory_fs::MemoryFS; - use crate::{ResolveContext, ResolveOptions, ResolverGeneric, ResolverPath}; + use crate::{ResolveContext, ResolveOptions, ResolverGeneric, ResolverPath, ResolverPathSet}; fn file_system() -> MemoryFS { MemoryFS::new(&[ @@ -159,11 +157,11 @@ mod windows { .await .map(|r| r.full_path()); assert_eq!(resolved, Ok(PathBuf::from(result))); - let file_dependencies: FxHashSet = file_dependencies + let file_dependencies: ResolverPathSet = file_dependencies .iter() .map(|p| ResolverPath::from(PathBuf::from(p))) .collect(); - let missing_dependencies: FxHashSet = missing_dependencies + let missing_dependencies: ResolverPathSet = missing_dependencies .iter() .map(|p| ResolverPath::from(PathBuf::from(p))) .collect(); diff --git a/src/tests/extensions.rs b/src/tests/extensions.rs index 66e971ed..300e53ff 100644 --- a/src/tests/extensions.rs +++ b/src/tests/extensions.rs @@ -1,10 +1,8 @@ //! -use rustc_hash::FxHashSet; - use crate::{ EnforceExtension, Resolution, ResolveContext, ResolveError, ResolveOptions, Resolver, - ResolverPath, + ResolverPath, ResolverPathSet, }; #[tokio::test] @@ -67,7 +65,7 @@ async fn default_enforce_extension() { ); assert_eq!( ctx.file_dependencies, - FxHashSet::from_iter([ + ResolverPathSet::from_iter([ ResolverPath::from(f.join("foo.ts")), ResolverPath::from(f.join("package.json")), ]) @@ -95,14 +93,14 @@ async fn respect_enforce_extension() { ); assert_eq!( ctx.file_dependencies, - FxHashSet::from_iter([ + ResolverPathSet::from_iter([ ResolverPath::from(f.join("foo.ts")), ResolverPath::from(f.join("package.json")), ]) ); assert_eq!( ctx.missing_dependencies, - FxHashSet::from_iter([ResolverPath::from(f.join("foo"))]) + ResolverPathSet::from_iter([ResolverPath::from(f.join("foo"))]) ); } diff --git a/src/tests/incorrect_description_file.rs b/src/tests/incorrect_description_file.rs index a21ae525..782962eb 100644 --- a/src/tests/incorrect_description_file.rs +++ b/src/tests/incorrect_description_file.rs @@ -1,9 +1,8 @@ //! -use rustc_hash::FxHashSet; - use crate::{ JSONError, Resolution, ResolveContext, ResolveError, ResolveOptions, Resolver, ResolverPath, + ResolverPathSet, }; // should not resolve main in incorrect description file #1 @@ -25,7 +24,7 @@ async fn incorrect_description_file_1() { assert!(matches!(resolution, Err(ResolveError::JSON(_)))); assert_eq!( ctx.file_dependencies, - FxHashSet::from_iter([ + ResolverPathSet::from_iter([ ResolverPath::from(f.join("pack1")), ResolverPath::from(f.join("pack1/package.json")), ]) diff --git a/src/tests/interning.rs b/src/tests/interning.rs new file mode 100644 index 00000000..c85d55da --- /dev/null +++ b/src/tests/interning.rs @@ -0,0 +1,105 @@ +//! End-to-end guarantees for `ResolverPath` interning. +//! +//! These assert on **pointer identity**, never on a global entry count or +//! `total_allocated()` — the interner is process-global and shared with every +//! other test running in parallel, so any global-count assertion is flaky by +//! construction. + +use std::path::PathBuf; + +use crate::{ResolveContext, ResolveOptions, Resolver}; + +/// `fixtures/enhanced_resolve/test/fixtures/extensions` — the same fixture and +/// options `src/tests/extensions.rs` resolves `./foo` against. +fn fixture_and_resolver() -> (PathBuf, Resolver) { + let f = super::fixture().join("extensions"); + let resolver = Resolver::new(ResolveOptions { + extensions: vec![".ts".into(), String::new(), ".js".into()], + ..ResolveOptions::default() + }); + (f, resolver) +} + +#[tokio::test] +async fn repeated_resolves_share_one_pointer_per_dependency() { + let (f, resolver) = fixture_and_resolver(); + + let mut first: Option> = None; + + for _ in 0..3 { + let mut ctx = ResolveContext::default(); + resolver + .resolve_with_context(&f, "./foo", &mut ctx) + .await + .expect("should resolve"); + + let mut snapshot: Vec<(String, *const u8)> = ctx + .file_dependencies + .iter() + .map(|p| (p.as_str().to_owned(), p.as_str().as_ptr())) + .collect(); + snapshot.sort_by(|a, b| a.0.cmp(&b.0)); + + assert!( + !snapshot.is_empty(), + "the fixture must produce at least one file dependency, \ + otherwise this test passes without proving anything" + ); + + match &first { + None => first = Some(snapshot), + Some(baseline) => assert_eq!( + baseline, &snapshot, + "every dependency path must be the same interned pointer on every \ + resolve — a differing pointer means a fresh copy was allocated" + ), + } + } +} + +#[tokio::test] +async fn equal_paths_from_different_sources_are_one_pointer() { + let (f, resolver) = fixture_and_resolver(); + + let mut ctx = ResolveContext::default(); + resolver + .resolve_with_context(&f, "./foo", &mut ctx) + .await + .expect("should resolve"); + + assert!( + !ctx.file_dependencies.is_empty(), + "the fixture must produce at least one file dependency" + ); + + for dep in &ctx.file_dependencies { + let reinterned = crate::ResolverPath::new(dep.as_str()); + assert_eq!( + dep.as_str().as_ptr(), + reinterned.as_str().as_ptr(), + "re-interning {dep} must hit the existing entry" + ); + } +} + +/// One-off memory evidence for the plan's headline claim: repeated pushes of +/// the same dependency path cost one interner entry, not one allocation per +/// push. Not run on normal `cargo test` — global counts are shared with every +/// other test in the binary, so they are only meaningful in isolation. +/// +/// Run with: `cargo test --all-features -- --ignored --nocapture interning` +#[tokio::test] +#[ignore = "prints process-global interner stats; run in isolation with --ignored"] +async fn report_interner_memory_usage() { + let (f, resolver) = fixture_and_resolver(); + + for _ in 0..100 { + let mut ctx = ResolveContext::default(); + resolver + .resolve_with_context(&f, "./foo", &mut ctx) + .await + .expect("should resolve"); + } + + eprintln!("interner: {} live entries", crate::interner::live_entries()); +} diff --git a/src/tests/missing.rs b/src/tests/missing.rs index c41e644e..f403d256 100644 --- a/src/tests/missing.rs +++ b/src/tests/missing.rs @@ -59,7 +59,11 @@ async fn test() { let _ = resolver.resolve_with_context(&f, specifier, &mut ctx).await; for path in ctx.file_dependencies { - assert_eq!(path.as_path(), path.normalize(), "{path:?}"); + assert_eq!( + path.as_std_path(), + path.as_std_path().normalize(), + "{path:?}" + ); } for path in missing_dependencies { @@ -110,11 +114,19 @@ async fn alias_and_extensions() { let _ = resolver.resolve_with_context(&f, "react-dom/client", &mut ctx); for path in ctx.file_dependencies { - assert_eq!(path.as_path(), path.normalize(), "{path:?}"); + assert_eq!( + path.as_std_path(), + path.as_std_path().normalize(), + "{path:?}" + ); } for path in ctx.missing_dependencies { - assert_eq!(path.as_path(), path.normalize(), "{path:?}"); + assert_eq!( + path.as_std_path(), + path.as_std_path().normalize(), + "{path:?}" + ); if let Some(path) = path.parent() { assert!(!path.is_file(), "{path:?} must not be a file"); } diff --git a/src/tests/mod.rs b/src/tests/mod.rs index 2ccfd495..210c0c5b 100644 --- a/src/tests/mod.rs +++ b/src/tests/mod.rs @@ -9,6 +9,7 @@ mod fallback; mod full_specified; mod imports_field; mod incorrect_description_file; +mod interning; mod main_field; mod memory_fs; mod missing; diff --git a/src/tests/package_json.rs b/src/tests/package_json.rs index 59f83b5e..3fcb9614 100644 --- a/src/tests/package_json.rs +++ b/src/tests/package_json.rs @@ -1,15 +1,13 @@ #[cfg(test)] mod tests { - use std::path::PathBuf; - - use crate::{package_json::ParseError, PackageJson}; + use crate::{package_json::ParseError, PackageJson, ResolverPath}; #[tokio::test] async fn test_json_with_bom() { - let mock_path = PathBuf::from("package.json"); + let mock_path = ResolverPath::new("package.json"); let json_with_bom = b"\xEF\xBB\xBF{\"name\": \"example-package\"}".to_vec(); - let result = PackageJson::parse(mock_path.clone(), mock_path.clone(), json_with_bom).err(); + let result = PackageJson::parse(mock_path.clone(), mock_path, json_with_bom).err(); assert_eq!( result, @@ -22,20 +20,20 @@ mod tests { #[tokio::test] async fn test_normal_json() { - let mock_path = PathBuf::from("package.json"); + let mock_path = ResolverPath::new("package.json"); let json_with_bom = r##"{"name": "example-package"}"##.as_bytes().to_vec(); - let parsed = PackageJson::parse(mock_path.clone(), mock_path.clone(), json_with_bom).unwrap(); + let parsed = PackageJson::parse(mock_path.clone(), mock_path, json_with_bom).unwrap(); assert_eq!(parsed.name.unwrap(), "example-package"); } #[tokio::test] async fn test_broken_json() { - let mock_path = PathBuf::from("package.json"); + let mock_path = ResolverPath::new("package.json"); let json_with_bom = r##"{"broken":"string"##.as_bytes().to_vec(); - let parsed_err = PackageJson::parse(mock_path.clone(), mock_path.clone(), json_with_bom).err(); + let parsed_err = PackageJson::parse(mock_path.clone(), mock_path, json_with_bom).err(); assert_eq!( parsed_err, @@ -49,10 +47,10 @@ mod tests { #[tokio::test] async fn test_empty_string() { - let mock_path = PathBuf::from("package.json"); + let mock_path = ResolverPath::new("package.json"); let json_with_bom = " ".as_bytes().to_vec(); - let parse_error = PackageJson::parse(mock_path.clone(), mock_path.clone(), json_with_bom) + let parse_error = PackageJson::parse(mock_path.clone(), mock_path, json_with_bom) .err() .unwrap(); @@ -64,4 +62,30 @@ mod tests { } ); } + + #[tokio::test] + async fn package_json_path_is_one_interned_pointer_across_resolves() { + use crate::{ResolveOptions, Resolver}; + + let f = crate::tests::fixture().join("extensions"); + let resolver = Resolver::new(ResolveOptions { + extensions: vec![".ts".into(), String::new(), ".js".into()], + ..ResolveOptions::default() + }); + + let a = resolver.resolve(&f, "./foo").await.expect("should resolve"); + let b = resolver + .resolve(&f, "./foo.ts") + .await + .expect("should resolve"); + + let (Some(pa), Some(pb)) = (a.package_json(), b.package_json()) else { + panic!("both resolutions should carry a package.json"); + }; + assert_eq!( + pa.path.as_str().as_ptr(), + pb.path.as_str().as_ptr(), + "the same package.json path must be one interned pointer, not two copies" + ); + } } diff --git a/src/tests/pnp.rs b/src/tests/pnp.rs index 3d55ee16..13c70a62 100644 --- a/src/tests/pnp.rs +++ b/src/tests/pnp.rs @@ -150,12 +150,7 @@ async fn pnp_resolve_description_file() { let r = resolver.resolve(&fixture, &full_path).await.unwrap(); assert_eq!( - r.package_json - .unwrap() - .path - .to_str() - .expect("path should be UTF-8") - .to_string(), + r.package_json.unwrap().path.as_str().to_string(), Utf8Path::from_path(&fixture) .expect("path should be UTF-8") .join(".yarn/cache/preact-npm-10.25.4-2dd2c0aa44-33a009d614.zip/node_modules/preact") diff --git a/src/tests/tsconfig_project_references.rs b/src/tests/tsconfig_project_references.rs index 6ea71e73..258babbe 100644 --- a/src/tests/tsconfig_project_references.rs +++ b/src/tests/tsconfig_project_references.rs @@ -84,6 +84,45 @@ async fn tsconfig_file_as_file_dependencies() { } } +#[tokio::test] +async fn tsconfig_dependency_is_one_interned_pointer_across_resolves() { + let f = super::fixture_root().join("tsconfig/cases/project_references"); + + let resolver = Resolver::new(ResolveOptions { + tsconfig: Some(TsconfigOptions { + config_file: f.join("app"), + references: TsconfigReferences::Auto, + }), + ..ResolveOptions::default() + }); + + let shared_tsconfig = f.join("app/tsconfig.json"); + let expected = ResolverPath::from(&shared_tsconfig); + + let mut pointers = vec![]; + for (dir, request) in [ + (f.join("project_b/src"), "@/index.ts"), + (f.join("project_a"), "@/index.ts"), + ] { + let mut ctx = ResolveContext::default(); + resolver + .resolve_with_context(&dir, request, &mut ctx) + .await + .expect("should resolve"); + let dep = ctx + .file_dependencies + .get(&expected) + .expect("app/tsconfig.json must be a file dependency"); + pointers.push(dep.as_str().as_ptr()); + } + + assert!( + pointers.windows(2).all(|w| w[0] == w[1]), + "the same tsconfig.json must be one interned pointer across resolves, \ + not one heap copy per resolve" + ); +} + #[tokio::test] async fn disabled() { let f = super::fixture_root().join("tsconfig/cases/project_references"); diff --git a/src/tsconfig.rs b/src/tsconfig.rs index 04c4fc45..9ebdd4da 100644 --- a/src/tsconfig.rs +++ b/src/tsconfig.rs @@ -5,10 +5,14 @@ use indexmap::{IndexMap, IndexSet}; use rustc_hash::FxHasher; use serde::Deserialize; -use crate::path::PathUtil; +use crate::{ + path::PathUtil, + resolver_path::{ResolverPath, ToResolverPath}, +}; pub type CompilerOptionsPathsMap = IndexMap, BuildHasherDefault>; -pub type FileDependencies = IndexSet>; +pub type FileDependencies = + IndexSet>; #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] #[serde(untagged)] @@ -87,13 +91,13 @@ impl TsConfig { let mut tsconfig: Self = serde_json::from_str("{}")?; tsconfig.root = root; tsconfig.path = path.to_path_buf(); - tsconfig.file_dependencies.insert(path.to_path_buf()); + tsconfig.file_dependencies.insert(path.to_resolver_path()); return Ok(tsconfig); } let mut tsconfig: Self = serde_json::from_str(json)?; tsconfig.root = root; tsconfig.path = path.to_path_buf(); - tsconfig.file_dependencies.insert(path.to_path_buf()); + tsconfig.file_dependencies.insert(path.to_resolver_path()); let directory = tsconfig.directory().to_path_buf(); if let Some(base_url) = &tsconfig.compiler_options.base_url { // keep the `${configDir}` template variable in the baseUrl