Repository navigation
resolver: park the cached directories below a busted directory until it is read again #46960
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f419e97
95274f3
8d58a17
f90e283
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2196,7 +2196,7 @@ impl<ValueType, const COUNT: usize, const REMOVE_TRAILING_SLASHES: bool> | |
| /// is never needed by callers, only the hash. `#[inline(always)]` + the | ||
| /// const-generic branch fold to identical codegen at each monomorphization. | ||
| #[inline(always)] | ||
| fn key_hash(denormalized_key: &[u8]) -> u64 { | ||
| pub fn key_hash(denormalized_key: &[u8]) -> u64 { | ||
| let key = if REMOVE_TRAILING_SLASHES { | ||
| trim_right(denormalized_key, SEP_STR.as_bytes()) | ||
| } else { | ||
|
|
@@ -2312,6 +2312,55 @@ impl<ValueType, const COUNT: usize, const REMOVE_TRAILING_SLASHES: bool> | |
| let _key = Self::key_hash(denormalized_key); | ||
| self.index.remove(&_key).is_some() | ||
| } | ||
|
|
||
| /// [`remove`](Self::remove) that also returns the hash of the key and what the key mapped | ||
| /// to: a slot, `NOT_FOUND` or `UNASSIGNED`. A slot stays allocated, so | ||
| /// [`publish`](Self::publish) can map the hash to it again. | ||
| pub fn unpublish(&mut self, denormalized_key: &[u8]) -> (u64, Option<IndexType>) { | ||
| let _guard = self.mutex.lock(); | ||
| let hash = Self::key_hash(denormalized_key); | ||
| (hash, self.index.remove(&hash)) | ||
| } | ||
|
|
||
| /// Unmaps every key whose slot satisfies `pred` and passes its hash and slot to `unpublished`. | ||
| /// A key that maps to `NOT_FOUND` or `UNASSIGNED` has no slot and stays. | ||
|
Comment on lines
+2325
to
+2326
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub fn unpublish_where( | ||
| &mut self, | ||
| mut pred: impl FnMut(&ValueType) -> bool, | ||
| mut unpublished: impl FnMut(u64, IndexType), | ||
| ) { | ||
| let _guard = self.mutex.lock(); | ||
| let Self { | ||
| index, | ||
| overflow_list, | ||
| backing_buf, | ||
| .. | ||
| } = self; | ||
| index.retain(|&hash, &mut slot| { | ||
| if slot.index() == NOT_FOUND.index() || slot.index() == UNASSIGNED.index() { | ||
| return true; | ||
| } | ||
| let value: &ValueType = if slot.is_overflow() { | ||
| overflow_list.at_index_mut(slot) | ||
| } else { | ||
| // SAFETY: a non-sentinel, non-overflow index was assigned by `put`, which | ||
| // initialized this slot via `.write()`. | ||
| unsafe { backing_buf[slot.index() as usize].assume_init_ref() } | ||
| }; | ||
| if !pred(value) { | ||
| return true; | ||
| } | ||
| unpublished(hash, slot); | ||
| false | ||
| }); | ||
| } | ||
|
|
||
| /// Maps `hash` to a slot that [`unpublish`](Self::unpublish) or | ||
| /// [`unpublish_where`](Self::unpublish_where) returned. | ||
|
Comment on lines
+2358
to
+2359
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub fn publish(&mut self, hash: u64, slot: IndexType) { | ||
| let _guard = self.mutex.lock(); | ||
| self.index.insert(hash, slot); | ||
| } | ||
| } | ||
|
|
||
| // ────────────────────────────────────────────────────────────────────────── | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,7 +25,8 @@ | |
| // `&mut DirInfo` is ever materialized concurrently with a read — writes happen | ||
| // only inside `dir_info_uncached` while filling a freshly-`put` slot, before | ||
| // any handle to that slot escapes. All access is additionally serialized under | ||
| // the resolver mutex. | ||
| // the resolver mutex. The one field that changes later is `entries`, which is | ||
| // atomic: see `ListingIndex`. | ||
|
Comment on lines
+28
to
+29
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| // | ||
| // `as_ptr()` exposes the raw `*mut` for the few callers that still need it | ||
| // (the `dir_info_uncached` fill path and `MatchResult.dir_info` round-trip). | ||
|
|
@@ -134,7 +135,7 @@ | |
|
|
||
| // lifetime — slice into BSS-backed path storage; never individually freed | ||
| pub abs_path: &'static [u8], | ||
| pub(crate) entries: Index, | ||
| pub(crate) entries: ListingIndex, | ||
| /// Is there a "package.json" file? | ||
| // `NonNull` (not `&'static`) preserves mut-provenance. Read via `.package_json()`. | ||
| pub package_json: Option<NonNull<PackageJSON>>, | ||
|
|
@@ -148,6 +149,38 @@ | |
| pub(crate) flags: Flags, | ||
| } | ||
|
|
||
| // One slot per directory that any resolver of the process reads, never freed. | ||
| const _: () = assert!(core::mem::size_of::<DirInfo>() == 104); | ||
|
|
||
| /// `DirInfo::entries`: where the listing of the directory is in `RealFS::entries`. A bust drops | ||
| /// the listing. A re-read that finds the directory as it was keeps the slot, because its | ||
| /// subdirectories hold the index of the slot, and stores the index of the new listing here. | ||
|
Comment on lines
+155
to
+157
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| #[derive(Default)] | ||
| pub(crate) struct ListingIndex(core::sync::atomic::AtomicU32); | ||
|
|
||
| impl ListingIndex { | ||
| const OVERFLOW_SHIFT: u32 = 31; | ||
|
|
||
| pub(crate) fn new(index: Index) -> Self { | ||
| Self(core::sync::atomic::AtomicU32::new(Self::bits(index))) | ||
| } | ||
|
|
||
| #[inline] | ||
| pub(crate) fn get(&self) -> Index { | ||
| let bits = self.0.load(core::sync::atomic::Ordering::Relaxed); | ||
| Index::new(bits, bits >> Self::OVERFLOW_SHIFT != 0) | ||
| } | ||
|
|
||
| pub(crate) fn set(&self, index: Index) { | ||
| self.0 | ||
| .store(Self::bits(index), core::sync::atomic::Ordering::Relaxed); | ||
| } | ||
|
|
||
| fn bits(index: Index) -> u32 { | ||
| index.index() | (u32::from(index.is_overflow()) << Self::OVERFLOW_SHIFT) | ||
| } | ||
| } | ||
|
|
||
| impl Default for DirInfo { | ||
| fn default() -> Self { | ||
| Self { | ||
|
|
@@ -159,7 +192,7 @@ | |
| package_json_for_module_type: None, | ||
| package_json_for_dependencies: None, | ||
| abs_path: b"", | ||
| entries: Index::default(), | ||
| entries: ListingIndex::default(), | ||
| package_json: None, | ||
| tsconfig_json: None, | ||
| abs_real_path: b"", | ||
|
|
@@ -183,6 +216,51 @@ | |
| } | ||
|
|
||
| impl DirInfo { | ||
| /// Whether a re-read of a busted directory (`self`) left it as `before`, the slot it had when | ||
| /// it was busted. A subdirectory copies the pointers and `InsideNodeModules` when it is | ||
| /// filled, and reads the rest through `parent` and `enclosing_browser_scope` later. So when | ||
| /// this is true the directory keeps `before`, and what was filled below it still holds. Every | ||
| /// field is named here so that a new one does not compile until it has an answer. | ||
|
Comment on lines
+219
to
+223
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub(crate) fn is_same_after_reread(&self, before: &DirInfo) -> bool { | ||
| fn same<T>(a: Option<&'static T>, b: Option<&'static T>) -> bool { | ||
| a.map(core::ptr::from_ref) == b.map(core::ptr::from_ref) | ||
| } | ||
| let DirInfo { | ||
| parent, | ||
| enclosing_browser_scope, | ||
| package_json_for_browser_field, | ||
| enclosing_tsconfig_json, | ||
| enclosing_package_json, | ||
| package_json_for_module_type, | ||
| package_json_for_dependencies, | ||
| // The key is the same path. | ||
| abs_path: _, | ||
| // A re-read has a new listing. | ||
| entries: _, | ||
| package_json, | ||
| tsconfig_json, | ||
| abs_real_path, | ||
| flags, | ||
| } = self; | ||
| *parent == before.parent | ||
| && *enclosing_browser_scope == before.enclosing_browser_scope | ||
| && same( | ||
| *package_json_for_browser_field, | ||
| before.package_json_for_browser_field, | ||
| ) | ||
| && same(*enclosing_tsconfig_json, before.enclosing_tsconfig_json) | ||
| && same(*enclosing_package_json, before.enclosing_package_json) | ||
| && same( | ||
| *package_json_for_module_type, | ||
| before.package_json_for_module_type, | ||
| ) | ||
| && *package_json_for_dependencies == before.package_json_for_dependencies | ||
| && *package_json == before.package_json | ||
| && *tsconfig_json == before.tsconfig_json | ||
| && *abs_real_path == before.abs_real_path | ||
| && *flags == before.flags | ||
| } | ||
|
|
||
| /// Is there a "node_modules" subdirectory? | ||
| #[inline] | ||
| pub(crate) fn has_node_modules(&self) -> bool { | ||
|
|
@@ -242,7 +320,7 @@ | |
| pub fn get_entries_ref_locked(&self, generation: Generation) -> Option<&'static fs::DirEntry> { | ||
| fs::FileSystem::instance() | ||
| .fs | ||
| .entries_at_locked(self.entries, generation)? | ||
| .entries_at_locked(self.entries.get(), generation)? | ||
| .as_entries() | ||
| } | ||
|
|
||
|
|
@@ -263,7 +341,7 @@ | |
| // `MutexGuard` stores the mutex by raw pointer, so holding it does not | ||
| // keep `rfs` borrowed. | ||
| let _lock = rfs.entries_mutex.lock_guard(); | ||
| rfs.entries_at_locked(self.entries, generation)? | ||
| rfs.entries_at_locked(self.entries.get(), generation)? | ||
| .as_entries()? | ||
| .get(query) | ||
| } | ||
|
|
@@ -276,7 +354,7 @@ | |
| fs::FileSystem::instance() | ||
| .fs | ||
| .entries | ||
| .at_index(self.entries)? | ||
| .at_index(self.entries.get())? | ||
| .as_entries() | ||
| } | ||
|
|
||
|
|
@@ -378,6 +456,166 @@ | |
| } | ||
| } | ||
|
|
||
| /// A slot that a bust took out of the index. A lookup of its key settles it: see | ||
| /// `Resolver::dir_info_cached_miss`. | ||
|
Comment on lines
+459
to
+460
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| #[derive(Clone, Copy)] | ||
| pub(crate) struct Parked { | ||
| pub(crate) slot: Index, | ||
| /// True: this directory was busted. It is read again and compared with the slot. | ||
| /// False: a directory above it was. The slot is good while its parent has the slot it had. | ||
|
Comment on lines
+464
to
+465
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub(crate) busted: bool, | ||
| } | ||
|
|
||
| type ParkedMap = std::collections::HashMap< | ||
| u64, | ||
| Parked, | ||
| core::hash::BuildHasherDefault<allocators::IdentityU64Hasher>, | ||
| >; | ||
|
|
||
| /// By the hash of the key, as in the index. Every access holds `RealFS::entries_mutex`, which | ||
| /// also keeps a fill from writing the slots that are read here. | ||
|
Comment on lines
+475
to
+476
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| static PARKED: bun_threading::Guarded<ParkedMap> = | ||
| bun_threading::Guarded::new(ParkedMap::with_hasher(core::hash::BuildHasherDefault::new())); | ||
|
|
||
| /// `PARKED.len()`, for `any_parked`. | ||
| static PARKED_COUNT: core::sync::atomic::AtomicUsize = core::sync::atomic::AtomicUsize::new(0); | ||
|
|
||
| /// Whether a bust left a slot out of the index. False in a process that never busts. | ||
| #[inline] | ||
| pub(crate) fn any_parked() -> bool { | ||
| PARKED_COUNT.load(core::sync::atomic::Ordering::Relaxed) != 0 | ||
| } | ||
|
|
||
| /// Whether `abs_path` names a directory below `dir`. Either can end in a separator. | ||
| fn is_below(abs_path: &[u8], dir: &[u8]) -> bool { | ||
| let is_separator = |byte: u8| bun_paths::Platform::AUTO.is_separator(byte); | ||
| let dir = match dir.split_last() { | ||
| Some((&last, rest)) if is_separator(last) => rest, | ||
| _ => dir, | ||
| }; | ||
| abs_path.len() > dir.len() + 1 && abs_path.starts_with(dir) && is_separator(abs_path[dir.len()]) | ||
| } | ||
|
|
||
| /// Takes the directory `key` out of the index, with every directory below it: each one holds | ||
| /// what it took from the directory above it when it was filled. Returns whether `key` was in | ||
| /// the index, as `BSSMapInner::remove` does. | ||
| /// | ||
| /// The caller holds `RealFS::entries_mutex`. | ||
|
Comment on lines
+499
to
+503
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub(crate) fn bust(map: &mut HashMap, key: &[u8]) -> bool { | ||
| let (hash, unpublished) = map.unpublish(key); | ||
| let mut parked = PARKED.lock(); | ||
| let slot = unpublished | ||
| .filter(|it| it.index() != NOT_FOUND.index() && it.index() != UNASSIGNED.index()); | ||
| let Some(slot) = slot else { | ||
| // A bust of a directory above this one parked it. Now it is busted too. | ||
| let was_parked = parked | ||
| .get_mut(&hash) | ||
| .is_some_and(|it| !core::mem::replace(&mut it.busted, true)); | ||
| return unpublished.is_some() || was_parked; | ||
| }; | ||
| parked.insert(hash, Parked { slot, busted: true }); | ||
| map.unpublish_where( | ||
| |info| is_below(info.abs_path, key), | ||
| |hash, slot| { | ||
| parked.insert( | ||
| hash, | ||
| Parked { | ||
| slot, | ||
| busted: false, | ||
| }, | ||
| ); | ||
| }, | ||
| ); | ||
| PARKED_COUNT.store(parked.len(), core::sync::atomic::Ordering::Relaxed); | ||
| true | ||
| } | ||
|
|
||
| /// The slot that a bust took out of the index for the key with this hash, if one did. The | ||
| /// caller publishes the slot again or leaves it out for good. | ||
| /// | ||
| /// The caller holds `RealFS::entries_mutex`. | ||
|
Comment on lines
+533
to
+536
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub(crate) fn take_parked(hash: u64) -> Option<Parked> { | ||
| let mut parked = PARKED.lock(); | ||
| let taken = parked.remove(&hash)?; | ||
| PARKED_COUNT.store(parked.len(), core::sync::atomic::Ordering::Relaxed); | ||
| Some(taken) | ||
| } | ||
|
|
||
| /// Leaves every parked directory below `dir` out of the index for good: `dir` is not what it | ||
| /// was when they were filled. | ||
| /// | ||
| /// The caller holds `RealFS::entries_mutex`. | ||
|
Comment on lines
+544
to
+547
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub(crate) fn discard_parked_below(dir: &[u8]) { | ||
| let mut parked = PARKED.lock(); | ||
| parked.retain(|_, it| { | ||
| // SAFETY: a parked slot was assigned by `put`. `entries_mutex` is held: see `PARKED`. | ||
| let slot = unsafe { slot_ptr_at(it.slot) }; | ||
| // SAFETY: `slot_ptr_at` returns a pointer to an initialized slot. | ||
| !slot.is_some_and(|slot| is_below(unsafe { (*slot).abs_path }, dir)) | ||
| }); | ||
| PARKED_COUNT.store(parked.len(), core::sync::atomic::Ordering::Relaxed); | ||
| } | ||
|
|
||
| /// A file that the tsconfig.json of a cached directory extends. | ||
| pub(crate) struct ExtendedFile { | ||
| /// In `DirnameStore`. | ||
| pub(crate) path: &'static [u8], | ||
| /// `SourceFile::hash` of the file when the configurations that extend it read it. `None`: | ||
| /// two of them read different bytes, so one of them is not what the file has now. | ||
|
Comment on lines
+563
to
+564
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub(crate) seen: Option<Option<u64>>, | ||
| } | ||
|
|
||
| type ExtendedMap = std::collections::HashMap< | ||
| u64, | ||
| Vec<ExtendedFile>, | ||
| core::hash::BuildHasherDefault<allocators::IdentityU64Hasher>, | ||
| >; | ||
|
|
||
| /// By the hash of the key of the directory that has the file. A re-read of a busted directory | ||
| /// leaves its slot as it is when its own configuration did not change. A change to one of these | ||
| /// files is what tells it that a directory below it is still not to be published as it was. | ||
| /// Every access holds `RealFS::entries_mutex`. | ||
|
Comment on lines
+574
to
+577
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| static EXTENDED: bun_threading::Guarded<ExtendedMap> = bun_threading::Guarded::new( | ||
| ExtendedMap::with_hasher(core::hash::BuildHasherDefault::new()), | ||
| ); | ||
|
|
||
| /// `EXTENDED.len()`, for `any_extended`. | ||
| static EXTENDED_COUNT: core::sync::atomic::AtomicUsize = core::sync::atomic::AtomicUsize::new(0); | ||
|
|
||
| /// Whether any cached tsconfig.json extends another file. | ||
| #[inline] | ||
| pub(crate) fn any_extended() -> bool { | ||
| EXTENDED_COUNT.load(core::sync::atomic::Ordering::Relaxed) != 0 | ||
| } | ||
|
|
||
| /// Records what a tsconfig.json that was just loaded read from `path`, a file it extends. | ||
| /// `dir_hash` is the key hash of the directory of `path`, and `hash` is `SourceFile::hash`. | ||
|
Comment on lines
+591
to
+592
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub(crate) fn note_extended(dir_hash: u64, path: &'static [u8], hash: Option<u64>) { | ||
| let mut extended = EXTENDED.lock(); | ||
| let files = extended.entry(dir_hash).or_default(); | ||
| match files.iter_mut().find(|it| *it.path == *path) { | ||
| Some(file) => { | ||
| if file.seen != Some(hash) { | ||
| file.seen = None; | ||
| } | ||
| } | ||
| None => files.push(ExtendedFile { | ||
| path, | ||
| seen: Some(hash), | ||
| }), | ||
| } | ||
| EXTENDED_COUNT.store(extended.len(), core::sync::atomic::Ordering::Relaxed); | ||
| } | ||
|
|
||
| /// The extended files in the directory with this key hash. `put_extended` gives them back. | ||
| pub(crate) fn take_extended(dir_hash: u64) -> Option<Vec<ExtendedFile>> { | ||
| EXTENDED.lock().remove(&dir_hash) | ||
| } | ||
|
|
||
| pub(crate) fn put_extended(dir_hash: u64, files: Vec<ExtendedFile>) { | ||
| EXTENDED.lock().insert(dir_hash, files); | ||
| } | ||
|
|
||
| bitflags::bitflags! { | ||
| #[derive(Clone, Copy, Default, PartialEq, Eq, Debug)] | ||
| pub struct Flags: u8 { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1196,14 +1196,13 @@ pub mod fs { | |
|
|
||
| /// Evicts `file_path` from the directory-entry cache; returns whether | ||
| /// an entry was removed. | ||
| pub(crate) fn bust_entries_cache(&mut self, file_path: &[u8]) -> bool { | ||
| // `entries` is the process-global | ||
| // BSSMap singleton and `remove` mutates it; callers (transpiler / | ||
| // hot-reloader / VM) reach this without `RESOLVER_MUTEX`, so take | ||
| // `entries_mutex` to satisfy `EntriesMap::inner`'s aliasing | ||
| // invariant. No caller already holds it (no re-entry from | ||
| // `read_directory`/`dir_info_cached_maybe_log`). | ||
| let _g = self.entries_mutex.lock_guard(); | ||
| /// | ||
| /// The caller holds `entries_mutex`: `entries` is the process-global | ||
| /// BSSMap singleton, and the callers of `Resolver::bust_dir_cache` | ||
| /// (transpiler / hot-reloader / VM) reach it without `RESOLVER_MUTEX`. | ||
| /// `bust_dir_cache` evicts the directory from its own cache under the | ||
| /// same lock. | ||
|
Comment on lines
+1199
to
+1204
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub(crate) fn bust_entries_cache_locked(&mut self, file_path: &[u8]) -> bool { | ||
| self.entries.remove(file_path) | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code