refactor(path): replace ResolverPath with interned UstrPath - #290
Draft
stormslowly wants to merge 27 commits into
Draft
refactor(path): replace ResolverPath with interned UstrPath#290stormslowly wants to merge 27 commits into
stormslowly wants to merge 27 commits into
Conversation
… pedantic under --all-targets
std's Path hash/eq folds the drive letter to uppercase (Prefix::Disk), so c:\a\b and C:\a\b are one dependency today. Byte-wise interning without this fold would split that into two paths, defeating the dedup this task exists to preserve.
Swap ResolveContext's file/missing dependency collections, all add_file_dependency/add_missing_dependency call sites, and CachedPathImpl's output channel from the Arc<Path>-backed ResolverPath to the interned UstrPath. ResolverPath and its hash_path helper are gone; the CachedPath DashSet's hash helper moves into cache.rs as the private hash_utf8_path, unrelated to interning. This is a pure type swap with no memoization yet (that's next): every add_*_dependency call does a naive to_ustr_path() intern lookup.
…ext path add_file_dependency/add_missing_dependency took P by value, so ToUstrPath::to_ustr_path (and the Ustr::from intern it does under missing_dependencies/file_dependencies == None) ran unconditionally at every call site before the `if let Some(deps)` guard could skip it. Interning takes a global sharded lock and permanently allocates on miss, so this silently added lock contention and unbounded allocation to the default resolve() path, which never populates a context. Switch both methods to <P: ToUstrPath + ?Sized>(&mut self, dep: &P) so the interning stays inside the Some branch, and revert the 8 call sites this affected back to passing references. Drop the needless_pass_by_value allows this shadowed. Also add a direct test for hash_utf8_path (cache.rs), which lost its only coverage when resolver_path.rs was deleted.
Repeated is_file/is_dir dependency pushes for the same CachedPathImpl re-entered the global ustr interner (FxHash + sharded mutex + probe) on every call. Memoize the interned path, the node_modules dependency path, and route the package_json cache-miss error branch through the already-memoized package_json path, so repeats degrade to a Copy. The node_modules_dep_path call site (cached_node_modules's cache-hit replay) previously had no dependency-tracking guard around it, since it built a plain Utf8PathBuf that only got interned inside the guarded add_missing_dependency body. Memoizing moves the intern call to the call site itself, so it now needs its own ctx.missing_dependencies.is_some() guard to keep interning gated behind active dependency tracking.
Resolution.path, CachedPathImpl.canonicalized, and realpath()'s return type move from Utf8PathBuf to the interned, Copy UstrPath. Both arms of realpath's cache-hit fast path and both branches of load_realpath now avoid allocating.
Proves the UstrPath work through the real resolve path: repeated resolves push the same interned pointer (not a fresh allocation) per dependency, and re-interning an already-seen path hits the existing entry. Adds an ignored test that prints ustr::num_entries()/total_allocated() as one-off memory evidence, reproducible via `cargo test -- --ignored --nocapture`.
repeated_resolves_share_one_pointer_per_dependency compared snapshots across resolves but never checked they were non-empty, so an empty file_dependencies set would have passed without proving anything.
Cache::tsconfig interned `path` on every call, including cache hits. Since options.tsconfig.config_file is constant per resolver, every thread hitting load_tsconfig_paths (re-entered per resolve for top-level lookups, alias candidates/redirects, and package self/imports subpaths) serialized on the same ustr interner bin mutex for a hash that never changes. Cache::tsconfig now takes an already-interned UstrPath instead of interning internally. ResolverGeneric caches options.tsconfig.config_file as UstrPath once at construction and passes it straight through on the hot path; the cold extends/ references call sites still intern inline.
…e-export Three comments in cache.rs still referred to hash_path, deleted when it moved into this file and was renamed hash_utf8_path, and claimed the unix byte-wise hash matches CachedPath's PartialEq. It does not: PartialEq is component-wise via std Path, so paths differing only in trailing/repeated separators compare equal but hash differently on unix. This predates this branch and never produces a wrong result (mismatched hashes just land in different DashSet buckets), only a duplicate cache entry — the comments now say so honestly. Also documents why ustr_path.rs re-exports ustr::IdentityHasher verbatim instead of a local newtype: rspack's ArcPathSet needs the exact same type for mem::take to work. That upstream item is #[doc(hidden)] and therefore outside semver; the ustr version pin in Cargo.toml is what actually protects the re-export.
Merging this PR will improve performance by 9.19%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Memory | resolver[single-thread] |
7 MB | 6.1 MB | +14.31% |
| ⚡ | Memory | resolver[tsconfig resolve] |
28.5 KB | 25.4 KB | +12.39% |
| ⚡ | Memory | resolver[resolve from symlinks] |
12.9 MB | 11.9 MB | +8.24% |
| ⚡ | Simulation | resolver[tsconfig resolve] |
4.8 ms | 4.5 ms | +7.14% |
| ⚡ | Memory | resolver[[single-threaded]resolve with many extensions] |
12.1 MB | 11.6 MB | +4.15% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing refactor/interned-ustr-path (704b58c) with main (ab14d9d)
Two assertions hardcoded unix separators and failed on windows-latest. The implementation is correct: UstrPath::new canonicalizes /a/b to \a\b on Windows by design, so the CI failure was the assertions testing normalization by accident instead of what they meant to test. Spelled the expectations out per platform rather than rebuilding them with join().to_ustr_path(), which would only prove the code agrees with itself.
stormslowly
added a commit
to web-infra-dev/rspack
that referenced
this pull request
Jul 31, 2026
rspack-resolver replaces ResolverPath with UstrPath, an 8-byte Copy handle into the ustr-fxhash global interner. Both crates already depend on ustr-fxhash 1.0.1, so they share a single interner static. Minimal adaptation to keep the workspace compiling: - rspack_paths re-exports UstrPath/ToUstrPath/UstrPathSet, and ArcResolverPathSet becomes UstrPathSet - PackageJson::directory() now returns &Utf8Path, so the DescriptionData boundary goes through as_std_path(); DescriptionData keeps its PathBuf field because it is cacheable(with = As<PortablePath>) and changing it would touch the persistent-cache format - UstrPath is UTF-8 by construction, so the binding layer uses as_str() instead of to_string_lossy() From<UstrPath> for ArcPath is a placeholder: it materializes an Arc per conversion, which is exactly the allocation this change exists to remove. It is here so downstream compiles and can be exercised. Replacing ArcPath with UstrPath outright is the follow-up that actually banks the win. The [patch.crates-io] entry pins the resolver to its PR branch rather than a local path so remote environments can build this too. It is temporary and tracks rstackjs/rspack-resolver#290; drop it once that lands and is published.
2 tasks
`UstrPath::new` rewrote Windows paths into canonical form (`\` separators,
collapsed and trailing separators removed, uppercase drive letter) so every
spelling of a path interned to one pointer, matching the folding `Path`'s
component-wise `Hash`/`Eq` provides.
That is right for a cache key but wrong for a general string handle. rspack
stores caller-supplied specifiers in path sets — a loader calling
`addBuildDependency("./build.txt")` puts that literal into `BuildInfo`'s
`build_dependencies` — and the rewrite was observable through the public JS
API, where `./build.txt` came back as `.\build.txt`. It also stripped the
trailing separator from directory entries, so `node_modules/` stopped
matching the `**/node_modules/**` ignore glob in rspack's watcher.
Store the string verbatim. Distinct Windows spellings of one path now intern
to distinct handles and compare unequal; if that dedup is needed later it
belongs in a separate canonicalizing constructor rather than in every
intern. `normalize_windows_separators` and its tests are kept for that.
Interning stores paths verbatim, so on Windows the spellings `Path` treats as one path — `C:/a/b`, `C:\a\b`, `C:\a\\b`, `C:\a\b\`, `c:\a\b` — become distinct handles and, under a derived `PartialEq`, compared unequal. Using them as set or map keys therefore stopped deduplicating, which is the behaviour `hash_path`'s component-wise `Path::hash` used to provide. Fold the spellings in the comparison rather than in what gets interned: `PartialEq` tries the interned pointer first and falls back to `Path`'s component semantics on Windows, and `Hash` matches it. The stored string is still exactly what the caller passed, so rspack's caller-supplied specifiers survive the round trip unchanged. Both `Hash` branches end in a single `write_u64`. That is required, not stylistic: `UstrPathSet` is keyed by `ustr::IdentityHasher`, whose `write` only reads a value when handed exactly 8 bytes and silently yields 0 otherwise, so forwarding `Path::hash` directly would collapse every key into one bucket with no error. The new tests fold through that hasher rather than a generic one so a regression there fails loudly. Also drops a stale `expect(dead_code)` on `is_sep`, which no longer holds now that the normalizer is unwired — it was breaking `cargo check --all-features` on every platform and Check Wasm in CI.
…reed `ustr` never frees an interned string. rspack calls `resolver_factory.clear_cache()` on every rebuild (via `plugin_driver.clear_cache`, from both `rebuild.rs` and the scopeguard in `compiler/mod.rs`), so a dev server rebuilds this cache continuously — and with a leak-forever interner the strings it drops stay resident, making RSS climb monotonically and never come back down. `internment::ArcIntern` refcounts instead: the entry goes away with the last handle. A new test in `cache.rs` pins that down — it interns a path no other test uses, checks the cache is holding it, then asserts the refcount falls back to 1 after `Cache::clear()`. Notable consequences: - `UstrPath` is no longer `Copy`. Every handoff is a refcount bump now, and the eight sites that relied on `Copy` (`*self`, `.copied()`, `*get_or_init`) clone explicitly. - `as_str()` no longer returns `&'static str`. It cannot: the entry is freed once the last handle drops. - The struct carries its own hash rather than using `ArcIntern`'s. Two reasons. `ArcIntern` hashes the pointer, but paths are stored verbatim, so on Windows `C:/a/b` and `C:\a\b` are distinct allocations that must still compare and hash alike — a pointer hash would break `a == b implies hash(a) == hash(b)`. Pointers are also aligned, so the low bits `IdentityHasher` buckets on are always zero. Computing it once at construction keeps lookups a single `write_u64` and keeps both platforms' semantics correct. - `IdentityHasher` is now defined here instead of re-exported from `ustr`. The local one panics in debug on any write that is not `write_u64`, and folds the bytes in release: the ustr version silently yielded 0 for other widths, which would collapse a misused map into one bucket with no error. - `internment`'s arc feature pulls ahash -> getrandom 0.3, which will not build for wasm32-unknown-unknown without a named backend. Handled with a target-scoped dependency plus `.cargo/config.toml`, so it applies only to that target — it is a CI sanity check here, and the shipped wasm artifact (wasm32-wasip1-threads) needs none of it. Verified wasm-bindgen stays out of the native and wasi dependency trees.
`node_modules_and_package_json_dep_paths_are_memoized` still asserted the literal `\a\b\node_modules`, left over from when `UstrPath::new` normalized separators. Interning is verbatim now, so `/a/b` keeps its slashes and only the joined component gets a `\` — the real value is `/a/b\node_modules`. Assert `file_name()` and `parent()` instead: pinning the joined string tests camino's platform behaviour, not the memo, and that is what broke twice. `getrandom` carries no code reference — it exists only to enable the wasm_js backend feature for wasm32-unknown-unknown — so cargo-shear reported it as unused. Listed under package.metadata.cargo-shear.
…terner `internment::ArcIntern` costs 1004 Ir per intern against `ustr`'s 161, which callgrind attributes to two things it cannot avoid through its API: its `DashMap` container re-hashes the string with `ahash` (2.5 full-string hashes per intern) even though `UstrPath` has already computed an `FxHash` for it, and the handle has to carry that hash separately because the pointer is not usable as one, widening every stored path to 16 bytes. The replacement takes the hash as a parameter and keeps it in the entry header, so interning hashes once and the handle is back to one pointer. Entry storage is a single inline allocation rather than a header plus a boxed str. Concurrency rests on one invariant: every mutation that can drive a refcount to zero, and every mutation that could raise one back from zero, happens under that entry's shard lock. `clone` is the one unlocked mutation and can do neither, since its caller holds a handle. That removes the need for a resurrection retry protocol at the cost of one uncontended lock per drop, measured at 2.5 Ir. Measured on the resolver benchmarks (callgrind, est_cycles vs main): intern drops to 246 Ir, and the aggregate regression goes from +2.64% to +0.90% — single-thread +8.45% to +2.78%, [mt]resolve +7.34% to +1.98%. The remaining gap to `ustr` is the allocation a freed entry needs on its next use, which is the price of releasing memory at all. Dropping `internment` also drops ahash and getrandom, so the wasm32-unknown-unknown backend workaround goes away with it.
Most drops release a temporary handle while the cache still holds the path, so they never reach zero and never need the table — yet every one of them was taking a shard lock, serializing resolver threads on a mutex they had no work to do behind. The decrement moves out of the lock. Reaching zero is now a *claim* on the entry rather than ownership of it, so the drop path re-establishes both facts it used to get for free, in this order: 1. Look the entry up by address, without dereferencing it. A concurrent dropper may already have freed it, and removal happens under this lock, so absence means someone else won the claim. 2. Re-check the count. A concurrent `intern` may have resurrected the entry between the decrement and the lock; the count then names exactly the handles that exist, so the dropper walks away. `intern` is unchanged: it increments under the lock, and doing so to a zero-count entry is a valid resurrection rather than a race to detect. `concurrent_zero_crossings_are_sound` targets both windows — eight threads over a two-key space, every handle dropped immediately. Deleting either check above makes it fail reliably; it passed three for three before, which is why the test is written to keep entries at zero rather than merely busy. Drop falls from 53.5 Ir to 34.7 Ir per call. The contention this removes is invisible to callgrind, which serializes threads — CodSpeed's multi-threaded benchmarks are the measurement that can see it.
Freeing an entry the moment its last handle went away made every drop a potential table mutation, which is why the drop path needed a lock, and then — once that lock moved off the fast path — a two-window race analysis and a sanitizer run to trust it. It also meant a `Cache::clear()` threw away entries the very next resolve would re-intern: 84% of interns were misses, at 246 Ir each. The table now owns a reference of its own. A handle's drop can no longer reach zero, so it is a bare decrement — no lock, no lookup, nothing to race. Entries leave in `Shard::sweep`, which drops those back down to the table's single reference, triggered by insertions at a threshold that scales with the shard so the amortized cost per insertion stays constant. Soundness collapses back to one line: every 1 -> 2 transition happens under the shard lock, because the only way to get a handle to a table entry is `intern`, which holds at least the read lock, and a sweep holds the write lock. `clone` needs no lock since its caller's handle already puts the count at 2 or more. Lookups take a read lock rather than an exclusive one, so threads interning known paths stop queueing behind each other. Measured against main (callgrind, est_cycles): the aggregate goes from +0.90% to -0.69%, ahead of even the leak-forever `ustr` baseline at -0.38%. Interning halves to 122.7 Ir because entries survive `clear_cache()` and the next iteration hits instead of missing, and allocations land at 80,042 — exactly `ustr`'s count, 3,002 below main — while the table stays bounded. `Interner` grows a `Drop` so a dropped instance frees what it owns; an entry a handle still refers to is leaked rather than freed, since `Interned` does not borrow from the interner. ASan on Linux reports no errors and no leaks.
A sweep can fire from inside the same `intern` call that inserted a new entry, and at that moment the entry has no handle — the caller's `Interned` is built after the sweep returns. What keeps the sweep from collecting it is `Entry::alloc` starting the count at two: one for the table, one pre-paid for the handle on its way out. That is easy to undo. Starting the count at one and incrementing before the return reads as the more symmetric shape, and it makes `intern` free the very string it hands back. Nothing failed on that mistake except a heap corruption trap, far from the line that caused it. The test watches for the table shrinking to spot which insert swept, then asserts that insert's own entry survived and was not silently re-allocated. It also asserts a sweep happened at all, so it cannot pass by never reaching the case.
`UstrPath` was named after its backing store, and that store has now changed three times — ustr, then internment, then this crate's own interner. The name went stale on the first change and is simply wrong after the last one, since no `ustr` is involved anywhere. `ResolverPath` names the role instead: the path type this crate hands out. Whatever the interner does next, that stays true. It is also the name main uses for the same slot, so downstream keeps the identifier it already has. Reusing the old name is safe here because the contract carried over. Equality was raw bytes on unix and `Path` components on Windows; interning stores one entry per distinct string, so pointer equality means the same thing on unix, and Windows still folds through `Path`. What genuinely changed — no longer `Copy`, no `Ord`, `as_str()` borrowing rather than `'static` — all fails to compile rather than passing quietly. Mechanical throughout: `ToUstrPath` -> `ToResolverPath`, `UstrPathSet` -> `ResolverPathSet`, `to_ustr_path` -> `to_resolver_path`, `src/ustr_path.rs` -> `src/resolver_path.rs`. Comments that named `ustr`'s bin mutex now name the shard lock that replaced it. `src/interner.rs` still mentions `ustr` where it means the crate — as a rejected alternative and as the precedent for the shard count.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Dependency paths handed out through
ResolveContextare stored per-module downstream. The sametsconfig.jsonends up in every module'sfile_dependencies, and each push allocated a freshArc<Path>— so a 10k-module build held 10k separate heap copies of one string.ResolverPath { Arc<Path>, u64 }, 24 bytesUstrPath(Ustr), 8 bytes,Copymemcmprspack already depends on the same
ustr-fxhash 1.0.1, so both crates share one interner static: a path interned here is already interned for rspack.ResolveContext's sets are now keyed byustr::IdentityHasher, the same concrete type as rspack'sArcPathSet, so rspack canmem::takethem instead of re-bucketing every element.What
UstrPath— 8-byteCopyhandle,Deref<Target = Utf8Path>(so the wholePath-shaped API comes for free),Hashwrites the interner's precomputedFxHash.ToUstrPath— conversion entry point, implemented forstr,String,Utf8Path,Utf8PathBuf,Path,PathBuf,UstrPath. The twostd::pathimpls panic on non-UTF-8 withpath should be UTF-8, matching what the resolver already did at everyPath -> Utf8Pathboundary.UstrPath::newis the only interning entry point, and therefore the only place Windows normalization happens — separators unified to\, repeated separators collapsed, trailing separator dropped (roots excepted), drive letter uppercased. This preserves the dedup semanticsstd::path::Path's component-wiseHash/Eqused to provide on Windows.CachedPathImpl(dep_path,node_modules_dep_path,package_json_dep_path) so repeat dependency pushes cost aCopyrather than re-entering the interner.Cache::tsconfigtakes an already-interned handle; the configuredconfig_fileis interned once at resolver construction. Interning on every lookup would have serialized all threads on oneparking_lotbin mutex, since a resolver'sconfig_fileis a constant string andustrpicks its bin from the hash's high bits.Interning is confined to terminal paths. Intermediate joins (extension probing,
node_moduleswalks) still land inUtf8PathBuf— most of those paths do not exist, and interned strings are never freed.Breaking changes — needs a paired rspack change
ResolverPathis removed.as_path()becomesas_std_path();as_arc()/into_arc()are gone.PackageJson::path/realpathareUstrPath;PackageJson::directory()returns&Utf8Path.Resolutiongainsustr_path()as a zero-copy exit.path(),into_path_buf(),full_path(),query(),fragment(),package_json()keep their exact signatures.ResolveContext's dependency sets areUstrPathSet.UstrPathdoes not implementOrd. Anything sorting dependency paths needssort_by_key(|p| p.as_std_path()).Resolution'sPartialEqnarrows fromUtf8PathBuf::eq(component-wise) to pointer equality. On unix/a/bvs/a/b/and/a//bvs/a/bnow compare unequal.Resolution::path()/full_path()/into_path_buf(),PackageJson::path/realpath/directory(), and every dependency-set entry — not just their hash/eq.resolve("c:/proj", "./foo.js")now yieldsC:\proj\foo.js.Cache::clear()no longer reclaims path memory. The bound is the project's distinct path count;?query/#fragmentare separateResolutionfields and never enter a path.Two accepted divergences from std
Pathon Windows: UNC/verbatim paths (\\?\…,\\server\share) pass through untouched, and.components are not dropped. Unix already behaved the latter way, so this makes the two platforms consistent rather than regressing Windows.