From 383607f26ccdf995717990ca3d32b0fb5471cd97 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Wed, 17 Jun 2026 13:32:50 +0000 Subject: [PATCH 01/10] refactor: erase configuration from interned storage --- src/interned.rs | 241 +++++++++++++++++++++++++----------------------- 1 file changed, 128 insertions(+), 113 deletions(-) diff --git a/src/interned.rs b/src/interned.rs index 1e750d0ef..cc13354d3 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -95,17 +95,17 @@ pub struct IngredientImpl { shift: u32, /// Sharded data that can only be accessed through a lock. - shards: Box<[CachePadded>>]>, + shards: Box<[CachePadded>]>, /// A queue of recent revisions in which values were interned. - revision_queue: RevisionQueue, + revision_queue: RevisionQueue, memo_table_types: Arc, _marker: PhantomData C>, } -struct IngredientShard { +struct IngredientShard { /// Maps from data to the existing interned ID for that data. /// /// This doesn't hold the fields themselves to save memory, instead it points @@ -113,10 +113,10 @@ struct IngredientShard { key_map: hashbrown::HashTable, /// An intrusive linked list for LRU. - lru: LinkedList>, + lru: LinkedList, } -impl Default for IngredientShard { +impl Default for IngredientShard { fn default() -> Self { Self { lru: LinkedList::default(), @@ -129,24 +129,33 @@ impl Default for IngredientShard { // ingredient lock, and values are only ever linked to a single list on the ingredient. unsafe impl Sync for Value {} -intrusive_adapter!(ValueAdapter = UnsafeRef>: Value { link => LinkedListLink } where C: Configuration); +// SAFETY: `LinkedListLink`, `memos`, and `shared` are only accessed while holding the +// ingredient shard lock. +unsafe impl Sync for ValueHeader {} + +intrusive_adapter!(ValueHeaderAdapter = UnsafeRef: ValueHeader { link => LinkedListLink }); /// Struct storing the interned fields. pub struct Value where C: Configuration, { - /// The index of the shard containing this value. - shard: u16, - - /// An intrusive linked list for LRU. - link: LinkedListLink, + /// Configuration-independent state used to manage this value. + header: ValueHeader, /// The interned fields for this value. /// /// These are valid for read-only access as long as the lock is held /// or the value has been validated in the current revision. fields: UnsafeCell>, +} + +struct ValueHeader { + /// The index of the shard containing this value. + shard: u16, + + /// An intrusive linked list for LRU. + link: LinkedListLink, /// Memos attached to this interned value. /// @@ -159,8 +168,11 @@ where shared: UnsafeCell, } +#[cfg(all(not(feature = "shuttle"), target_pointer_width = "64"))] +const _: [(); std::mem::size_of::()] = [(); std::mem::size_of::<[usize; 7]>()]; + /// Shared value data can only be read through the lock. -#[repr(Rust, packed)] // Allow `durability` to be stored in the padding of the outer `Value` struct. +#[repr(Rust, packed)] // Avoid padding around `durability` inside `ValueHeader`. #[derive(Clone, Copy)] struct ValueShared { /// The interned ID for this value. @@ -190,14 +202,14 @@ struct ValueShared { impl ValueShared { /// Returns `true` if this value slot can be reused when interning, and should be added to the LRU. - fn is_reusable(&self) -> bool { - Self::is_reusable_with_durability::(self.durability) + fn is_reusable(&self, revisions: NonZeroUsize) -> bool { + Self::is_reusable_with_durability(self.durability, revisions) } /// Returns `true` if a value slot with the given durability can be reused when interning. - fn is_reusable_with_durability(durability: Durability) -> bool { + fn is_reusable_with_durability(durability: Durability, revisions: NonZeroUsize) -> bool { // Garbage collection is disabled. - if C::REVISIONS == IMMORTAL { + if revisions == IMMORTAL { return false; } @@ -211,13 +223,14 @@ impl ValueShared { } /// Record a dependency on this value if its slot can be reused. - fn report_tracked_read_if_reusable( + fn report_tracked_read_if_reusable( zalsa_local: &ZalsaLocal, index: DatabaseKeyIndex, current_revision: Revision, durability: Durability, + revisions: NonZeroUsize, ) { - if Self::is_reusable_with_durability::(durability) { + if Self::is_reusable_with_durability(durability, revisions) { zalsa_local.report_tracked_read_simple(index, durability, current_revision); } else { // The value cannot be reused, so the dependency edge is unnecessary. Its durability @@ -252,7 +265,7 @@ where let heap_size = C::heap_size(self.fields()); // SAFETY: The caller guarantees we hold the lock for the shard containing the value, so we // have at-least read-only access to the value's memos. - let memos = unsafe { &*self.memos.get() }; + let memos = unsafe { &*self.header.memos.get() }; // SAFETY: The caller guarantees this is the correct types table. let memos = unsafe { memo_table_types.attach_memos(memos) }; @@ -305,7 +318,7 @@ where ingredient_index, hasher: FxBuildHasher, memo_table_types: Arc::new(MemoTableTypes::default()), - revision_queue: RevisionQueue::default(), + revision_queue: RevisionQueue::new(C::REVISIONS), shift: usize::BITS - shards.trailing_zeros(), shards: (0..shards).map(|_| Default::default()).collect(), _marker: PhantomData, @@ -387,7 +400,9 @@ where { // Record the current revision as active. let current_revision = zalsa.current_revision(); - self.revision_queue.record(current_revision); + if C::REVISIONS != IMMORTAL { + self.revision_queue.record(current_revision); + } // Hash the value before acquiring the lock. let hash = self.hasher.hash_one(&key); @@ -409,7 +424,7 @@ where let index = self.database_key_index(id); // SAFETY: We hold the lock for the shard containing the value. - let value_shared = unsafe { &mut *value.shared.get() }; + let value_shared = unsafe { &mut *value.header.shared.get() }; // Validate the value in this revision to avoid reuse. if { value_shared.last_interned_at } < current_revision { @@ -422,31 +437,31 @@ where }) }); - if value_shared.is_reusable::() { + if value_shared.is_reusable(C::REVISIONS) { // Move the value to the front of the LRU list. // // SAFETY: We hold the lock for the shard containing the value, and `value` is // a reusable value that was previously interned, so is in the list. - unsafe { shard.lru.cursor_mut_from_ptr(value).remove() }; + unsafe { shard.lru.cursor_mut_from_ptr(&value.header).remove() }; // SAFETY: The value pointer is valid for the lifetime of the database // and never accessed mutably directly. - unsafe { shard.lru.push_front(UnsafeRef::from_raw(value)) }; + unsafe { shard.lru.push_front(UnsafeRef::from_raw(&value.header)) }; } } if let Some((_, stamp)) = zalsa_local.active_query() { - let was_reusable = value_shared.is_reusable::(); + let was_reusable = value_shared.is_reusable(C::REVISIONS); // Record the maximum durability across all queries that intern this value. value_shared.durability = std::cmp::max(value_shared.durability, stamp.durability); // If the value is no longer reusable, i.e. the durability increased, remove it // from the LRU. - if was_reusable && !value_shared.is_reusable::() { + if was_reusable && !value_shared.is_reusable(C::REVISIONS) { // SAFETY: We hold the lock for the shard containing the value, and `value` // was previously reusable, so is in the list. - unsafe { shard.lru.cursor_mut_from_ptr(value).remove() }; + unsafe { shard.lru.cursor_mut_from_ptr(&value.header).remove() }; } } @@ -455,11 +470,12 @@ where // See `intern_id_cold` for why we need to use `current_revision` here. Note that just // because this value was previously interned does not mean it was previously interned // by *our query*, so the same considerations apply. - ValueShared::report_tracked_read_if_reusable::( + ValueShared::report_tracked_read_if_reusable( zalsa_local, index, current_revision, value_shared.durability, + C::REVISIONS, ); return value_shared.id; @@ -481,9 +497,9 @@ where // Otherwise, try to reuse a stale slot. let mut cursor = shard.lru.back_mut(); - while let Some(value) = cursor.get() { + while let Some(header) = cursor.get() { // SAFETY: We hold the lock for the shard containing the value. - let value_shared = unsafe { &mut *value.shared.get() }; + let value_shared = unsafe { &mut *header.shared.get() }; // The value must not have been read in the current revision to be collected // soundly, but we also do not want to collect values that have been read recently. @@ -506,6 +522,8 @@ where .unwrap_or((Durability::MAX, Revision::max())); let old_id = value_shared.id; + let value = zalsa.table().get::>(old_id); + debug_assert!(std::ptr::eq(header, &value.header)); // Increment the generation of the ID, as if we allocated a new slot. // @@ -534,11 +552,12 @@ where // Record a dependency on the new value if its slot can be reused. // // See `intern_id_cold` for why we need to use `current_revision` here. - ValueShared::report_tracked_read_if_reusable::( + ValueShared::report_tracked_read_if_reusable( zalsa_local, index, current_revision, durability, + C::REVISIONS, ); // Insert the replacement while the old slot is still intact. `insert_unique` @@ -550,8 +569,8 @@ where // Remove the value from the LRU list. // - // SAFETY: The value pointer is valid for the lifetime of the database. - let value = unsafe { &*UnsafeRef::into_raw(cursor.remove().unwrap()) }; + let removed_header = UnsafeRef::into_raw(cursor.remove().unwrap()); + debug_assert!(std::ptr::eq(removed_header, &value.header)); // Remove the previous value from the ID map. // @@ -580,18 +599,18 @@ where last_interned_at, }; - if value_shared.is_reusable::() { + if value_shared.is_reusable(C::REVISIONS) { // Move the value to the front of the LRU list. // - // SAFETY: The value pointer is valid for the lifetime of the database. - // and never accessed mutably directly. - shard.lru.push_front(unsafe { UnsafeRef::from_raw(value) }); + // SAFETY: The value pointer is valid for the lifetime of the database + // and is never accessed mutably directly. + unsafe { shard.lru.push_front(UnsafeRef::from_raw(&value.header)) }; } // SAFETY: We hold the lock for the shard containing the value, and the // value has not been interned in the current revision, so no references to // it can exist. - let memo_table = unsafe { &mut *value.memos.get() }; + let memo_table = unsafe { &mut *value.header.memos.get() }; // Free the memos associated with the previous interned value. // @@ -625,7 +644,7 @@ where zalsa: &Zalsa, zalsa_local: &ZalsaLocal, assemble: impl FnOnce(Id, Key) -> C::Fields<'db>, - shard: &mut IngredientShard, + shard: &mut IngredientShard, shard_index: usize, hash: u64, ) -> crate::Id @@ -645,18 +664,20 @@ where // Allocate the value slot. let (id, value) = zalsa_local.allocate(zalsa, self.ingredient_index, |id| Value:: { - shard: shard_index as u16, - link: LinkedListLink::new(), - // SAFETY: We only ever access the memos of a value that we allocated through - // our `MemoTableTypes`. - memos: UnsafeCell::new(unsafe { MemoTable::new(self.memo_table_types()) }), + header: ValueHeader { + shard: shard_index as u16, + link: LinkedListLink::new(), + // SAFETY: We only ever access the memos of a value that we allocated through + // our `MemoTableTypes`. + memos: UnsafeCell::new(unsafe { MemoTable::new(self.memo_table_types()) }), + shared: UnsafeCell::new(ValueShared { + id, + durability, + last_interned_at, + }), + }, // SAFETY: We call `from_internal_data` to restore the correct lifetime before access. fields: UnsafeCell::new(unsafe { self.to_internal_data(assemble(id, key)) }), - shared: UnsafeCell::new(ValueShared { - id, - durability, - last_interned_at, - }), }); // Insert the newly allocated ID. @@ -672,11 +693,12 @@ where // the query has changed without a corresponding input changing. Using `current_revision` // for dependencies on interned values encodes the fact that interned IDs are not stable // across revisions. - ValueShared::report_tracked_read_if_reusable::( + ValueShared::report_tracked_read_if_reusable( zalsa_local, index, current_revision, durability, + C::REVISIONS, ); zalsa.event(&|| { @@ -694,19 +716,19 @@ where &self, id: Id, zalsa: &Zalsa, - shard: &mut IngredientShard, + shard: &mut IngredientShard, hash: u64, value: &Value, ) { // SAFETY: We hold the lock for the shard containing the value. - let value_shared = unsafe { &mut *value.shared.get() }; + let value_shared = unsafe { &mut *value.header.shared.get() }; - if value_shared.is_reusable::() { + if value_shared.is_reusable(C::REVISIONS) { // Add the value to the front of the LRU list. // // SAFETY: The value pointer is valid for the lifetime of the database - // and never accessed mutably directly. - shard.lru.push_front(unsafe { UnsafeRef::from_raw(value) }); + // and is never accessed mutably directly. + unsafe { shard.lru.push_front(UnsafeRef::from_raw(&value.header)) }; } // SAFETY: We hold the lock for the shard containing the value. @@ -816,12 +838,12 @@ where debug_assert!( { - let _shard = self.shards[value.shard as usize].lock(); + let _shard = self.shards[value.header.shard as usize].lock(); // SAFETY: We hold the lock for the shard containing the value. - let value_shared = unsafe { &mut *value.shared.get() }; + let value_shared = unsafe { &mut *value.header.shared.get() }; - !value_shared.is_reusable::() || { + !value_shared.is_reusable(C::REVISIONS) || { let last_changed_revision = zalsa.last_changed_revision(value_shared.durability); ({ value_shared.last_interned_at }) >= last_changed_revision @@ -873,14 +895,15 @@ where // TODO: Grab all locks eagerly. zalsa.table().slots_of::>().map(move |(_, value)| { let id = if should_lock { - // SAFETY: `value.shard` is guaranteed to be in-bounds for `self.shards`. - let _shard = unsafe { self.shards.get_unchecked(value.shard as usize) }.lock(); + // SAFETY: `value.header.shard` is guaranteed to be in-bounds for `self.shards`. + let _shard = + unsafe { self.shards.get_unchecked(value.header.shard as usize) }.lock(); // SAFETY: We hold the lock for the shard containing the value. - unsafe { (*value.shared.get()).id } + unsafe { (*value.header.shared.get()).id } } else { // SAFETY: The caller guarantees we hold the lock for the shard containing the value. - unsafe { (*value.shared.get()).id } + unsafe { (*value.header.shared.get()).id } }; StructEntry { @@ -942,15 +965,17 @@ where ) -> VerifyResult { // Record the current revision as active. let current_revision = zalsa.current_revision(); - self.revision_queue.record(current_revision); + if C::REVISIONS != IMMORTAL { + self.revision_queue.record(current_revision); + } let value = zalsa.table().get::>(input); - // SAFETY: `value.shard` is guaranteed to be in-bounds for `self.shards`. - let _shard = unsafe { self.shards.get_unchecked(value.shard as usize) }.lock(); + // SAFETY: `value.header.shard` is guaranteed to be in-bounds for `self.shards`. + let _shard = unsafe { self.shards.get_unchecked(value.header.shard as usize) }.lock(); // SAFETY: We hold the lock for the shard containing the value. - let value_shared = unsafe { &mut *value.shared.get() }; + let value_shared = unsafe { &mut *value.header.shared.get() }; // The slot was reused. if value_shared.id.generation() > input.generation() { @@ -1104,12 +1129,12 @@ where // SAFETY: The fact that we have a pointer to the `Value` means it must // have been interned, and thus validated, in the current revision. // Caller obligation demands this pointer to be valid. - unsafe { (*this).memos.get() } + unsafe { (*this).header.memos.get() } } #[inline(always)] fn memos_mut(&mut self) -> &mut crate::table::memo::MemoTable { - self.memos.get_mut() + self.header.memos.get_mut() } } @@ -1118,42 +1143,35 @@ where /// An interned value is considered stale if it has not been read in the past `REVS` /// revisions. However, we only consider revisions in which interned values were actually /// read, as revisions may be created in bursts. -struct RevisionQueue { +struct RevisionQueue { lock: Mutex<()>, /// Recent revisions, stored inline for the default configuration. revisions: SmallVec<[AtomicRevision; DEFAULT_REVISIONS]>, - _configuration: PhantomData C>, } // `#[salsa::interned(revisions = usize::MAX)]` disables garbage collection. const IMMORTAL: NonZeroUsize = NonZeroUsize::MAX; -impl Default for RevisionQueue { - fn default() -> RevisionQueue { - let revisions = if C::REVISIONS == IMMORTAL { +impl RevisionQueue { + fn new(capacity: NonZeroUsize) -> Self { + let revisions = if capacity == IMMORTAL { SmallVec::new() } else { - (0..C::REVISIONS.get()) + (0..capacity.get()) .map(|_| AtomicRevision::start()) .collect() }; - RevisionQueue { + Self { lock: Mutex::new(()), revisions, - _configuration: PhantomData, } } -} -impl RevisionQueue { /// Record the given revision as active. #[inline] fn record(&self, revision: Revision) { - // Garbage collection is disabled. - if C::REVISIONS == IMMORTAL { - return; - } + debug_assert!(!self.revisions.is_empty()); // Fast-path: We already recorded this revision. if self.revisions[0].load() >= revision { @@ -1175,7 +1193,7 @@ impl RevisionQueue { // Otherwise, update the queue, maintaining sorted order. // // Note that this should only happen once per revision. - for i in (1..C::REVISIONS.get()).rev() { + for i in (1..self.revisions.len()).rev() { self.revisions[i].store(self.revisions[i - 1].load()); } @@ -1185,12 +1203,10 @@ impl RevisionQueue { /// Returns `true` if the given revision is old enough to be considered stale. #[inline] fn is_stale(&self, revision: Revision) -> bool { - // Garbage collection is disabled. - if C::REVISIONS == IMMORTAL { + let Some(oldest) = self.revisions.last() else { return false; - } - - let oldest = self.revisions[C::REVISIONS.get() - 1].load(); + }; + let oldest = oldest.load(); // If we have not recorded `REVS` revisions yet, nothing can be stale. if oldest == Revision::start() { @@ -1200,16 +1216,13 @@ impl RevisionQueue { revision < oldest } - /// Returns `true` if `C::REVISIONS` revisions have been recorded as active, + /// Returns `true` if the configured number of revisions have been recorded as active, /// i.e. enough data has been recorded to start garbage collection. #[inline] fn is_primed(&self) -> bool { - // Garbage collection is disabled. - if C::REVISIONS == IMMORTAL { - return false; - } - - self.revisions[C::REVISIONS.get() - 1].load() > Revision::start() + self.revisions + .last() + .is_some_and(|oldest| oldest.load() > Revision::start()) } } @@ -1554,7 +1567,7 @@ mod persistence { use serde::ser::{SerializeMap, SerializeStruct}; use serde::{Deserialize, de}; - use super::{Configuration, IngredientImpl, Value, ValueShared}; + use super::{Configuration, IngredientImpl, Value, ValueHeader, ValueShared}; use crate::plumbing::Ingredient; use crate::table::memo::MemoTable; use crate::zalsa::Zalsa; @@ -1589,7 +1602,7 @@ mod persistence { for (_, value) in zalsa.table().slots_of::>() { // SAFETY: The safety invariant of `Ingredient::serialize` ensures we have exclusive access // to the database. - let id = unsafe { (*value.shared.get()).id }; + let id = unsafe { (*value.header.shared.get()).id }; map.serialize_entry(&id.as_bits(), value)?; } @@ -1608,13 +1621,13 @@ mod persistence { { let mut value = serializer.serialize_struct("Value,", 3)?; - let Value { - fields, + let Value { fields, header } = self; + let ValueHeader { shared, shard: _, link: _, memos: _, - } = self; + } = header; // SAFETY: The safety invariant of `Ingredient::serialize` ensures we have exclusive access // to the database. @@ -1700,19 +1713,21 @@ mod persistence { let shard = unsafe { &mut *ingredient.shards.get_unchecked(shard_index).lock() }; let value = Value:: { - shard: shard_index as u16, - link: LinkedListLink::new(), - // SAFETY: We only ever access the memos of a value that we allocated through - // our `MemoTableTypes`. - memos: UnsafeCell::new(unsafe { - MemoTable::new(ingredient.memo_table_types()) - }), + header: ValueHeader { + shard: shard_index as u16, + link: LinkedListLink::new(), + // SAFETY: We only ever access the memos of a value that we allocated through + // our `MemoTableTypes`. + memos: UnsafeCell::new(unsafe { + MemoTable::new(ingredient.memo_table_types()) + }), + shared: UnsafeCell::new(ValueShared { + id, + durability: value.durability, + last_interned_at: value.last_interned_at, + }), + }, fields: UnsafeCell::new(value.fields.0), - shared: UnsafeCell::new(ValueShared { - id, - durability: value.durability, - last_interned_at: value.last_interned_at, - }), }; // Force initialize the relevant page. From 9e486e73137e885182cf72c81ec2511c0fe8b5f8 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 27 Jun 2026 15:46:06 +0000 Subject: [PATCH 02/10] perf: avoid interned table lookup during reuse --- src/interned.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/interned.rs b/src/interned.rs index cc13354d3..c78a61226 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -136,11 +136,21 @@ unsafe impl Sync for ValueHeader {} intrusive_adapter!(ValueHeaderAdapter = UnsafeRef: ValueHeader { link => LinkedListLink }); /// Struct storing the interned fields. +/// +/// # Layout +/// +/// This type uses the C representation because the intrusive LRU stores pointers to `header`, +/// and the stale-slot reuse path casts those pointers back to `Value`. +/// Therefore, `header` must remain the first field so that it has the same address as the +/// containing `Value`. +#[repr(C)] pub struct Value where C: Configuration, { /// Configuration-independent state used to manage this value. + /// + /// This must remain the first field; see the type's layout documentation. header: ValueHeader, /// The interned fields for this value. @@ -522,8 +532,10 @@ where .unwrap_or((Durability::MAX, Revision::max())); let old_id = value_shared.id; - let value = zalsa.table().get::>(old_id); - debug_assert!(std::ptr::eq(header, &value.header)); + // SAFETY: `Value` uses the C representation and `header` is its first field, so + // both pointers have the same address. Every header in this LRU belongs to a + // `Value` allocated by this ingredient. + let value = unsafe { &*std::ptr::from_ref(header).cast::>() }; // Increment the generation of the ID, as if we allocated a new slot. // @@ -1823,7 +1835,7 @@ mod tests { #[test] fn revision_queue_records_each_revision_once() { - let queue = RevisionQueue::::default(); + let queue = RevisionQueue::new(TestConfiguration::REVISIONS); let revision = Revision::start().next(); // Simulate two threads that both passed the fast-path check before taking the lock. From 738decc2eba164bc02281f5da98d019b7b3085d9 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 27 Jun 2026 16:05:21 +0000 Subject: [PATCH 03/10] fix: preserve provenance for interned LRU pointers --- src/interned.rs | 49 ++++++++++++++++++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 13 deletions(-) diff --git a/src/interned.rs b/src/interned.rs index c78a61226..6a586243c 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -255,6 +255,12 @@ impl Value where C: Configuration, { + /// Returns a pointer to the header that retains provenance for the complete value. + #[inline] + fn header_ptr(&self) -> *const ValueHeader { + std::ptr::from_ref(self).cast() + } + /// Fields of this interned struct. #[cfg(feature = "salsa_unstable")] pub fn fields(&self) -> &C::Fields<'static> { @@ -456,7 +462,11 @@ where // SAFETY: The value pointer is valid for the lifetime of the database // and never accessed mutably directly. - unsafe { shard.lru.push_front(UnsafeRef::from_raw(&value.header)) }; + unsafe { + shard + .lru + .push_front(UnsafeRef::from_raw(value.header_ptr())) + }; } } @@ -507,9 +517,16 @@ where // Otherwise, try to reuse a stale slot. let mut cursor = shard.lru.back_mut(); - while let Some(header) = cursor.get() { + while let Some(header) = cursor.as_cursor().clone_pointer() { + let header = UnsafeRef::into_raw(header); + + // SAFETY: LRU pointers are created from `Value::header_ptr`, which retains provenance + // for the complete `Value`. `Value` uses the C representation and `header` is + // its first field, so both pointers have the same address. + let value = unsafe { &*header.cast::>() }; + // SAFETY: We hold the lock for the shard containing the value. - let value_shared = unsafe { &mut *header.shared.get() }; + let value_shared = unsafe { &mut *value.header.shared.get() }; // The value must not have been read in the current revision to be collected // soundly, but we also do not want to collect values that have been read recently. @@ -532,11 +549,6 @@ where .unwrap_or((Durability::MAX, Revision::max())); let old_id = value_shared.id; - // SAFETY: `Value` uses the C representation and `header` is its first field, so - // both pointers have the same address. Every header in this LRU belongs to a - // `Value` allocated by this ingredient. - let value = unsafe { &*std::ptr::from_ref(header).cast::>() }; - // Increment the generation of the ID, as if we allocated a new slot. // // If the ID is at its maximum generation, we are forced to leak the slot. @@ -616,7 +628,11 @@ where // // SAFETY: The value pointer is valid for the lifetime of the database // and is never accessed mutably directly. - unsafe { shard.lru.push_front(UnsafeRef::from_raw(&value.header)) }; + unsafe { + shard + .lru + .push_front(UnsafeRef::from_raw(value.header_ptr())) + }; } // SAFETY: We hold the lock for the shard containing the value, and the @@ -740,7 +756,11 @@ where // // SAFETY: The value pointer is valid for the lifetime of the database // and is never accessed mutably directly. - unsafe { shard.lru.push_front(UnsafeRef::from_raw(&value.header)) }; + unsafe { + shard + .lru + .push_front(UnsafeRef::from_raw(value.header_ptr())) + }; } // SAFETY: We hold the lock for the shard containing the value. @@ -907,9 +927,9 @@ where // TODO: Grab all locks eagerly. zalsa.table().slots_of::>().map(move |(_, value)| { let id = if should_lock { + let shard_index = value.header.shard as usize; // SAFETY: `value.header.shard` is guaranteed to be in-bounds for `self.shards`. - let _shard = - unsafe { self.shards.get_unchecked(value.header.shard as usize) }.lock(); + let _shard = unsafe { self.shards.get_unchecked(shard_index) }.lock(); // SAFETY: We hold the lock for the shard containing the value. unsafe { (*value.header.shared.get()).id } @@ -1183,7 +1203,10 @@ impl RevisionQueue { /// Record the given revision as active. #[inline] fn record(&self, revision: Revision) { - debug_assert!(!self.revisions.is_empty()); + debug_assert!( + !self.revisions.is_empty(), + "cannot record revisions when interned garbage collection is disabled" + ); // Fast-path: We already recorded this revision. if self.revisions[0].load() >= revision { From 2a7411934ca4e22b876324d1fc2696745362209f Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 27 Jun 2026 18:28:24 +0000 Subject: [PATCH 04/10] refactor: erase interned cold paths --- src/interned.rs | 485 ++++++++++++++++++++++++++---------------------- 1 file changed, 266 insertions(+), 219 deletions(-) diff --git a/src/interned.rs b/src/interned.rs index 6a586243c..a932985b8 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -129,8 +129,10 @@ impl Default for IngredientShard { // ingredient lock, and values are only ever linked to a single list on the ingredient. unsafe impl Sync for Value {} -// SAFETY: `LinkedListLink`, `memos`, and `shared` are only accessed while holding the -// ingredient shard lock. +// SAFETY: `LinkedListLink` and `shared` are only accessed while holding the ingredient shard +// lock. `memos` can also be accessed without that lock through `Slot::memos`, but those accesses +// are shared and `MemoTable` supports concurrent shared access internally. Mutable access to +// `memos` requires exclusive access to the value. unsafe impl Sync for ValueHeader {} intrusive_adapter!(ValueHeaderAdapter = UnsafeRef: ValueHeader { link => LinkedListLink }); @@ -251,6 +253,139 @@ impl ValueShared { } } +/// Clears the given memo table. +/// +/// # Safety +/// +/// `memo_table` must belong to the value identified by `id` in `ingredient_index` and must have +/// been created with `memo_table_types`. The caller must have exclusive access to the table. +unsafe fn clear_memos( + zalsa: &Zalsa, + ingredient_index: IngredientIndex, + memo_table_types: &MemoTableTypes, + memo_table: &mut MemoTable, + id: Id, +) { + // SAFETY: The caller guarantees this is the correct types table. + let table = unsafe { memo_table_types.attach_memos_mut(memo_table) }; + + // `Database::salsa_event` is a user supplied callback which may panic + // in that case we need a drop guard to free the memo table + struct TableDropGuard<'a>(MemoTableWithTypesMut<'a>); + + impl Drop for TableDropGuard<'_> { + fn drop(&mut self) { + // SAFETY: We have `&mut MemoTable`, so no more references to these memos exist and we are good + // to drop them. + unsafe { self.0.drop() }; + } + } + + let mut table_guard = TableDropGuard(table); + + // SAFETY: We have `&mut MemoTable`, so no more references to these memos exist and we are good + // to drop them. + unsafe { + table_guard.0.take_memos(|memo_ingredient_index, memo| { + let ingredient_index = + zalsa.ingredient_index_for_memo(ingredient_index, memo_ingredient_index); + + let executor = DatabaseKeyIndex::new(ingredient_index, id); + + zalsa.event(&|| Event::new(EventKind::DidDiscard { key: executor })); + + memo.remove_outputs(zalsa, executor); + }) + }; + + std::mem::forget(table_guard); + + // Reset the table after having dropped any memos. + memo_table.reset(); +} + +struct ReusableSlot { + header: *const ValueHeader, + old_id: Id, + new_id: Id, +} + +/// Finds the least-recently-used stale slot whose generation can be incremented. +/// +/// # Safety +/// +/// The caller must hold `shard`'s lock, and every pointer in its LRU must refer to a live +/// `ValueHeader` belonging to the shard. +unsafe fn find_reusable_slot( + revision_queue: &RevisionQueue, + current_revision: Revision, + shard: &mut IngredientShard, +) -> Option { + let mut cursor = shard.lru.back_mut(); + + while let Some(header) = cursor.as_cursor().clone_pointer() { + let header = UnsafeRef::into_raw(header); + + // SAFETY: Guaranteed by the caller. + let value_shared = unsafe { &mut *(*header).shared.get() }; + + // The list is sorted by LRU, so if the tail is not stale, no slot is stale. + if !revision_queue.is_stale(value_shared.last_interned_at) { + return None; + } + + // We should never reuse a value that was accessed in the current revision. + debug_assert!({ value_shared.last_interned_at } < current_revision); + + let old_id = value_shared.id; + if let Some(new_id) = old_id.next_generation() { + return Some(ReusableSlot { + header, + old_id, + new_id, + }); + } + + // This slot can never be reused. Remove it and retry with the previous element. + cursor.remove().unwrap(); + cursor = shard.lru.back_mut(); + } + + None +} + +fn insert_unique(shard: &mut IngredientShard, hash: u64, id: Id, hasher: &dyn Fn(&Id) -> u64) { + shard.key_map.insert_unique(hash, id, hasher); +} + +/// Inserts a newly allocated value into the LRU and key map. +/// +/// # Safety +/// +/// The caller must hold `shard`'s lock. `header` must have been produced by `Value::header_ptr` +/// for the live value identified by `id` in this shard, and `hasher` must hash values while that +/// lock is held. +unsafe fn insert_id( + id: Id, + shard: &mut IngredientShard, + hash: u64, + header: *const ValueHeader, + revisions: NonZeroUsize, + hasher: &dyn Fn(&Id) -> u64, +) { + // SAFETY: Guaranteed by the caller. + let value_shared = unsafe { &mut *(*header).shared.get() }; + + if value_shared.is_reusable(revisions) { + // SAFETY: Guaranteed by the caller, including full-value provenance. + unsafe { shard.lru.push_front(UnsafeRef::from_raw(header)) }; + } + + insert_unique(shard, hash, id, hasher); + + debug_assert_eq!(hash, hasher(&id)); +} + impl Value where C: Configuration, @@ -429,7 +564,7 @@ where let found_value = Cell::new(None); // SAFETY: We hold the lock for the shard containing the value. - let eq = |id: &_| unsafe { Self::value_eq(*id, &key, zalsa, &found_value) }; + let eq = |id: &_| unsafe { Self::value_eq(zalsa, *id, &key, &found_value) }; // Attempt a fast-path lookup of already interned data. if let Some(&id) = shard.key_map.find(hash, eq) { @@ -504,9 +639,9 @@ where // Fill up the table for the first few revisions without attempting garbage collection. if !self.revision_queue.is_primed() { return self.intern_id_cold( - key, zalsa, zalsa_local, + key, assemble, shard, shard_index, @@ -515,151 +650,141 @@ where } // Otherwise, try to reuse a stale slot. - let mut cursor = shard.lru.back_mut(); - - while let Some(header) = cursor.as_cursor().clone_pointer() { - let header = UnsafeRef::into_raw(header); - - // SAFETY: LRU pointers are created from `Value::header_ptr`, which retains provenance - // for the complete `Value`. `Value` uses the C representation and `header` is - // its first field, so both pointers have the same address. - let value = unsafe { &*header.cast::>() }; - - // SAFETY: We hold the lock for the shard containing the value. - let value_shared = unsafe { &mut *value.header.shared.get() }; - - // The value must not have been read in the current revision to be collected - // soundly, but we also do not want to collect values that have been read recently. - // - // Note that the list is sorted by LRU, so if the tail of the list is not stale, we - // will not find any stale slots. - if !self.revision_queue.is_stale(value_shared.last_interned_at) { - break; - } - - // We should never reuse a value that was accessed in the current revision. - debug_assert!({ value_shared.last_interned_at } < current_revision); - - // Record the durability of the current query on the interned value. - let (durability, last_interned_at) = zalsa_local - .active_query() - .map(|(_, stamp)| (stamp.durability, current_revision)) - // If there is no active query this durability does not actually matter. - // `last_interned_at` needs to be `Revision::MAX`, see the `intern_access_in_different_revision` test. - .unwrap_or((Durability::MAX, Revision::max())); + // SAFETY: We hold the shard lock and all LRU pointers refer to live values in this shard. + let Some(slot) = + (unsafe { find_reusable_slot(&self.revision_queue, current_revision, shard) }) + else { + // If we could not find a stale slot, we are forced to allocate a new one. + return self.intern_id_cold( + zalsa, + zalsa_local, + key, + assemble, + shard, + shard_index, + hash, + ); + }; - let old_id = value_shared.id; - // Increment the generation of the ID, as if we allocated a new slot. - // - // If the ID is at its maximum generation, we are forced to leak the slot. - let Some(new_id) = value_shared.id.next_generation() else { - // Remove the value from the LRU list as we will never be able to - // collect it. - cursor.remove().unwrap(); + // SAFETY: Each shard belongs to a single `IngredientImpl`, and only `Value`s from + // that ingredient are inserted into its LRU. The pointer retains complete-value + // provenance, and `Value` uses the C representation with `header` as its first field. + let value = unsafe { &*slot.header.cast::>() }; - // Retry with the previous element. - cursor = shard.lru.back_mut(); + // SAFETY: We hold the lock for the shard containing the value. + let value_shared = unsafe { &mut *value.header.shared.get() }; - continue; - }; + // Record the durability of the current query on the interned value. + let (durability, last_interned_at) = zalsa_local + .active_query() + .map(|(_, stamp)| (stamp.durability, current_revision)) + // If there is no active query this durability does not actually matter. + // `last_interned_at` needs to be `Revision::MAX`, see the + // `intern_access_in_different_revision` test. + .unwrap_or((Durability::MAX, Revision::max())); - // Assemble and hash the replacement before mutating the existing slot. Both operations - // can invoke user code and panic. - // SAFETY: We call `from_internal_data` to restore the correct lifetime before access. - let new_fields = unsafe { self.to_internal_data(assemble(new_id, key)) }; + // Assemble and hash the replacement before mutating the existing slot. Both operations + // can invoke user code and panic. + // SAFETY: We call `from_internal_data` to restore the correct lifetime before access. + let new_fields = unsafe { self.to_internal_data(assemble(slot.new_id, key)) }; - // SAFETY: We hold the lock for the shard containing the value. - let old_hash = self.hasher.hash_one(unsafe { &*value.fields.get() }); + // SAFETY: We hold the lock for the shard containing the value. + let old_hash = self.hasher.hash_one(unsafe { &*value.fields.get() }); - let index = self.database_key_index(new_id); + let index = self.database_key_index(slot.new_id); - // Record a dependency on the new value if its slot can be reused. - // - // See `intern_id_cold` for why we need to use `current_revision` here. - ValueShared::report_tracked_read_if_reusable( - zalsa_local, - index, - current_revision, - durability, - C::REVISIONS, - ); + // Record a dependency on the new value if its slot can be reused. + // + // See `intern_id_cold` for why we need to use `current_revision` here. + ValueShared::report_tracked_read_if_reusable( + zalsa_local, + index, + current_revision, + durability, + C::REVISIONS, + ); - // Insert the replacement while the old slot is still intact. `insert_unique` - // currently performs any rehashing before inserting, so a panic in user hashing - // leaves the old value reachable. Revisit this assumption when updating hashbrown. - // SAFETY: We hold the lock for the shard containing every value passed to `hasher`. - let hasher = |id: &_| unsafe { self.value_hash(*id, zalsa) }; - shard.key_map.insert_unique(hash, new_id, hasher); + // Insert the replacement while the old slot is still intact. `insert_unique` + // currently performs any rehashing before inserting, so a panic in user hashing + // leaves the old value reachable. Revisit this assumption when updating hashbrown. + // SAFETY: We hold the lock for the shard containing every value passed to `hasher`. + let hasher = |id: &_| unsafe { self.value_hash(zalsa, *id) }; + insert_unique(shard, hash, slot.new_id, &hasher); + + // Remove the value from the LRU list. + // SAFETY: We hold the shard lock and `value` is currently in the LRU. + let removed_header = UnsafeRef::into_raw( + unsafe { shard.lru.cursor_mut_from_ptr(&value.header).remove() }.unwrap(), + ); + debug_assert!(std::ptr::eq(removed_header, &value.header)); - // Remove the value from the LRU list. - // - let removed_header = UnsafeRef::into_raw(cursor.remove().unwrap()); - debug_assert!(std::ptr::eq(removed_header, &value.header)); + // Remove the previous value from the ID map. + // + // Note that while the ID stays the same when a slot is reused, the fields, + // and thus the hash, will change, so we need to re-insert the value into the + // map. Crucially, we know that the hashes for the old and new fields both map + // to the same shard, because we determined the initial shard based on the new + // fields and only accessed the LRU list for that shard. + shard + .key_map + .find_entry(old_hash, |found_id: &Id| *found_id == slot.old_id) + .expect("interned value in LRU so must be in key_map") + .remove(); + + // Replace the fields without dropping the previous value until the slot is consistent. + // + // SAFETY: We hold the lock for the shard containing the value, and the + // value has not been interned in the current revision, so no references to + // it can exist. + let old_fields = unsafe { std::mem::replace(&mut *value.fields.get(), new_fields) }; + + // Mark the slot as reused. + *value_shared = ValueShared { + id: slot.new_id, + durability, + last_interned_at, + }; - // Remove the previous value from the ID map. - // - // Note that while the ID stays the same when a slot is reused, the fields, - // and thus the hash, will change, so we need to re-insert the value into the - // map. Crucially, we know that the hashes for the old and new fields both map - // to the same shard, because we determined the initial shard based on the new - // fields and only accessed the LRU list for that shard. - shard - .key_map - .find_entry(old_hash, |found_id: &Id| *found_id == old_id) - .expect("interned value in LRU so must be in key_map") - .remove(); - - // Replace the fields without dropping the previous value until the slot is consistent. + if value_shared.is_reusable(C::REVISIONS) { + // Move the value to the front of the LRU list. // - // SAFETY: We hold the lock for the shard containing the value, and the - // value has not been interned in the current revision, so no references to - // it can exist. - let old_fields = unsafe { std::mem::replace(&mut *value.fields.get(), new_fields) }; - - // Mark the slot as reused. - *value_shared = ValueShared { - id: new_id, - durability, - last_interned_at, + // SAFETY: The value pointer is valid for the lifetime of the database + // and is never accessed mutably directly. + unsafe { + shard + .lru + .push_front(UnsafeRef::from_raw(value.header_ptr())) }; + } - if value_shared.is_reusable(C::REVISIONS) { - // Move the value to the front of the LRU list. - // - // SAFETY: The value pointer is valid for the lifetime of the database - // and is never accessed mutably directly. - unsafe { - shard - .lru - .push_front(UnsafeRef::from_raw(value.header_ptr())) - }; - } - - // SAFETY: We hold the lock for the shard containing the value, and the - // value has not been interned in the current revision, so no references to - // it can exist. - let memo_table = unsafe { &mut *value.header.memos.get() }; - - // Free the memos associated with the previous interned value. - // - // SAFETY: The memo table belongs to a value that we allocated, so it has the - // correct type. - unsafe { self.clear_memos(zalsa, memo_table, old_id) }; + // SAFETY: We hold the lock for the shard containing the value, and the + // value has not been interned in the current revision, so no references to + // it can exist. + let memo_table = unsafe { &mut *value.header.memos.get() }; - drop(old_fields); + // Free the memos associated with the previous interned value. + // + // SAFETY: The memo table belongs to a value allocated with these memo-table types. + unsafe { + clear_memos( + zalsa, + self.ingredient_index, + &self.memo_table_types, + memo_table, + slot.old_id, + ) + }; - zalsa.event(&|| { - Event::new(EventKind::DidReuseInternedValue { - key: index, - revision: current_revision, - }) - }); + drop(old_fields); - return new_id; - } + zalsa.event(&|| { + Event::new(EventKind::DidReuseInternedValue { + key: index, + revision: current_revision, + }) + }); - // If we could not find any stale slots, we are forced to allocate a new one. - self.intern_id_cold(key, zalsa, zalsa_local, assemble, shard, shard_index, hash) + slot.new_id } /// The cold path for interning a value, allocating a new slot. @@ -668,9 +793,9 @@ where #[allow(clippy::too_many_arguments)] fn intern_id_cold<'db, Key>( &'db self, - key: Key, zalsa: &Zalsa, zalsa_local: &ZalsaLocal, + key: Key, assemble: impl FnOnce(Id, Key) -> C::Fields<'db>, shard: &mut IngredientShard, shard_index: usize, @@ -687,7 +812,8 @@ where .active_query() .map(|(_, stamp)| (stamp.durability, current_revision)) // If there is no active query this durability does not actually matter. - // `last_interned_at` needs to be `Revision::MAX`, see the `intern_access_in_different_revision` test. + // `last_interned_at` needs to be `Revision::MAX`, see the + // `intern_access_in_different_revision` test. .unwrap_or((Durability::MAX, Revision::max())); // Allocate the value slot. @@ -709,7 +835,10 @@ where }); // Insert the newly allocated ID. - self.insert_id(id, zalsa, shard, hash, value); + // SAFETY: We hold the lock for `shard`; `value` is the live value identified by `id` in + // that shard, and `header_ptr` retains provenance for the complete value. + let hasher = |id: &_| unsafe { self.value_hash(zalsa, *id) }; + unsafe { insert_id(id, shard, hash, value.header_ptr(), C::REVISIONS, &hasher) }; let index = self.database_key_index(id); @@ -739,94 +868,12 @@ where id } - /// Inserts a newly interned value ID into the LRU list and key map. - fn insert_id( - &self, - id: Id, - zalsa: &Zalsa, - shard: &mut IngredientShard, - hash: u64, - value: &Value, - ) { - // SAFETY: We hold the lock for the shard containing the value. - let value_shared = unsafe { &mut *value.header.shared.get() }; - - if value_shared.is_reusable(C::REVISIONS) { - // Add the value to the front of the LRU list. - // - // SAFETY: The value pointer is valid for the lifetime of the database - // and is never accessed mutably directly. - unsafe { - shard - .lru - .push_front(UnsafeRef::from_raw(value.header_ptr())) - }; - } - - // SAFETY: We hold the lock for the shard containing the value. - let hasher = |id: &_| unsafe { self.value_hash(*id, zalsa) }; - - // Insert the value into the ID map. - shard.key_map.insert_unique(hash, id, hasher); - - debug_assert_eq!(hash, { - let value = zalsa.table().get::>(id); - - // SAFETY: We hold the lock for the shard containing the value. - unsafe { self.hasher.hash_one(&*value.fields.get()) } - }); - } - - /// Clears the given memo table. - /// - /// # Safety - /// - /// The `MemoTable` must belong to a `Value` of the correct type. - pub(crate) unsafe fn clear_memos(&self, zalsa: &Zalsa, memo_table: &mut MemoTable, id: Id) { - // SAFETY: The caller guarantees this is the correct types table. - let table = unsafe { self.memo_table_types.attach_memos_mut(memo_table) }; - - // `Database::salsa_event` is a user supplied callback which may panic - // in that case we need a drop guard to free the memo table - struct TableDropGuard<'a>(MemoTableWithTypesMut<'a>); - - impl Drop for TableDropGuard<'_> { - fn drop(&mut self) { - // SAFETY: We have `&mut MemoTable`, so no more references to these memos exist and we are good - // to drop them. - unsafe { self.0.drop() }; - } - } - - let mut table_guard = TableDropGuard(table); - - // SAFETY: We have `&mut MemoTable`, so no more references to these memos exist and we are good - // to drop them. - unsafe { - table_guard.0.take_memos(|memo_ingredient_index, memo| { - let ingredient_index = - zalsa.ingredient_index_for_memo(self.ingredient_index, memo_ingredient_index); - - let executor = DatabaseKeyIndex::new(ingredient_index, id); - - zalsa.event(&|| Event::new(EventKind::DidDiscard { key: executor })); - - memo.remove_outputs(zalsa, executor); - }) - }; - - std::mem::forget(table_guard); - - // Reset the table after having dropped any memos. - memo_table.reset(); - } - // Hashes the value by its fields. // // # Safety // // The lock must be held for the shard containing the value. - unsafe fn value_hash<'db>(&'db self, id: Id, zalsa: &'db Zalsa) -> u64 { + unsafe fn value_hash<'db>(&'db self, zalsa: &'db Zalsa, id: Id) -> u64 { // This closure is only called if the table is resized. So while it's expensive // to lookup all values, it will only happen rarely. let value = zalsa.table().get::>(id); @@ -841,9 +888,9 @@ where // // The lock must be held for the shard containing the value. unsafe fn value_eq<'db, Key>( + zalsa: &'db Zalsa, id: Id, key: &Key, - zalsa: &'db Zalsa, found_value: &Cell>>, ) -> bool where From 9288e6e8d571253435d718f78de07a4ae02b3209 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 27 Jun 2026 19:17:57 +0000 Subject: [PATCH 05/10] perf: specialize interned reuse checks --- src/interned.rs | 57 +++++++++++++++++++++++++------------------------ 1 file changed, 29 insertions(+), 28 deletions(-) diff --git a/src/interned.rs b/src/interned.rs index a932985b8..a66efc28d 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -214,14 +214,17 @@ struct ValueShared { impl ValueShared { /// Returns `true` if this value slot can be reused when interning, and should be added to the LRU. - fn is_reusable(&self, revisions: NonZeroUsize) -> bool { - Self::is_reusable_with_durability(self.durability, revisions) + fn is_reusable(&self) -> bool { + Self::is_reusable_with_durability::(self.durability) } /// Returns `true` if a value slot with the given durability can be reused when interning. - fn is_reusable_with_durability(durability: Durability, revisions: NonZeroUsize) -> bool { + /// + /// This intentionally remains generic so the compiler specializes the immortal and reusable + /// configurations instead of branching on `REVISIONS` at runtime. + fn is_reusable_with_durability(durability: Durability) -> bool { // Garbage collection is disabled. - if revisions == IMMORTAL { + if C::REVISIONS == IMMORTAL { return false; } @@ -235,14 +238,13 @@ impl ValueShared { } /// Record a dependency on this value if its slot can be reused. - fn report_tracked_read_if_reusable( + fn report_tracked_read_if_reusable( zalsa_local: &ZalsaLocal, index: DatabaseKeyIndex, current_revision: Revision, durability: Durability, - revisions: NonZeroUsize, ) { - if Self::is_reusable_with_durability(durability, revisions) { + if Self::is_reusable_with_durability::(durability) { zalsa_local.report_tracked_read_simple(index, durability, current_revision); } else { // The value cannot be reused, so the dependency edge is unnecessary. Its durability @@ -363,20 +365,17 @@ fn insert_unique(shard: &mut IngredientShard, hash: u64, id: Id, hasher: &dyn Fn /// # Safety /// /// The caller must hold `shard`'s lock. `header` must have been produced by `Value::header_ptr` -/// for the live value identified by `id` in this shard, and `hasher` must hash values while that -/// lock is held. +/// for the live value identified by `id` in this shard, `reusable` must reflect whether that value +/// is reusable, and `hasher` must hash values while that lock is held. unsafe fn insert_id( id: Id, shard: &mut IngredientShard, hash: u64, header: *const ValueHeader, - revisions: NonZeroUsize, + reusable: bool, hasher: &dyn Fn(&Id) -> u64, ) { - // SAFETY: Guaranteed by the caller. - let value_shared = unsafe { &mut *(*header).shared.get() }; - - if value_shared.is_reusable(revisions) { + if reusable { // SAFETY: Guaranteed by the caller, including full-value provenance. unsafe { shard.lru.push_front(UnsafeRef::from_raw(header)) }; } @@ -588,7 +587,7 @@ where }) }); - if value_shared.is_reusable(C::REVISIONS) { + if value_shared.is_reusable::() { // Move the value to the front of the LRU list. // // SAFETY: We hold the lock for the shard containing the value, and `value` is @@ -606,14 +605,14 @@ where } if let Some((_, stamp)) = zalsa_local.active_query() { - let was_reusable = value_shared.is_reusable(C::REVISIONS); + let was_reusable = value_shared.is_reusable::(); // Record the maximum durability across all queries that intern this value. value_shared.durability = std::cmp::max(value_shared.durability, stamp.durability); // If the value is no longer reusable, i.e. the durability increased, remove it // from the LRU. - if was_reusable && !value_shared.is_reusable(C::REVISIONS) { + if was_reusable && !value_shared.is_reusable::() { // SAFETY: We hold the lock for the shard containing the value, and `value` // was previously reusable, so is in the list. unsafe { shard.lru.cursor_mut_from_ptr(&value.header).remove() }; @@ -625,12 +624,11 @@ where // See `intern_id_cold` for why we need to use `current_revision` here. Note that just // because this value was previously interned does not mean it was previously interned // by *our query*, so the same considerations apply. - ValueShared::report_tracked_read_if_reusable( + ValueShared::report_tracked_read_if_reusable::( zalsa_local, index, current_revision, value_shared.durability, - C::REVISIONS, ); return value_shared.id; @@ -696,12 +694,11 @@ where // Record a dependency on the new value if its slot can be reused. // // See `intern_id_cold` for why we need to use `current_revision` here. - ValueShared::report_tracked_read_if_reusable( + ValueShared::report_tracked_read_if_reusable::( zalsa_local, index, current_revision, durability, - C::REVISIONS, ); // Insert the replacement while the old slot is still intact. `insert_unique` @@ -745,7 +742,7 @@ where last_interned_at, }; - if value_shared.is_reusable(C::REVISIONS) { + if value_shared.is_reusable::() { // Move the value to the front of the LRU list. // // SAFETY: The value pointer is valid for the lifetime of the database @@ -834,11 +831,16 @@ where fields: UnsafeCell::new(unsafe { self.to_internal_data(assemble(id, key)) }), }); + // SAFETY: We hold the lock for the shard containing the value. + let reusable = unsafe { (&*value.header.shared.get()).is_reusable::() }; + // Insert the newly allocated ID. - // SAFETY: We hold the lock for `shard`; `value` is the live value identified by `id` in - // that shard, and `header_ptr` retains provenance for the complete value. + // SAFETY: We hold the lock for the shard containing every value passed to `hasher`. let hasher = |id: &_| unsafe { self.value_hash(zalsa, *id) }; - unsafe { insert_id(id, shard, hash, value.header_ptr(), C::REVISIONS, &hasher) }; + // SAFETY: We hold the lock for `shard`; `value` is the live value identified by `id` in + // that shard, `reusable` was read from the value, and `header_ptr` retains provenance for + // the complete value. + unsafe { insert_id(id, shard, hash, value.header_ptr(), reusable, &hasher) }; let index = self.database_key_index(id); @@ -850,12 +852,11 @@ where // the query has changed without a corresponding input changing. Using `current_revision` // for dependencies on interned values encodes the fact that interned IDs are not stable // across revisions. - ValueShared::report_tracked_read_if_reusable( + ValueShared::report_tracked_read_if_reusable::( zalsa_local, index, current_revision, durability, - C::REVISIONS, ); zalsa.event(&|| { @@ -922,7 +923,7 @@ where // SAFETY: We hold the lock for the shard containing the value. let value_shared = unsafe { &mut *value.header.shared.get() }; - !value_shared.is_reusable(C::REVISIONS) || { + !value_shared.is_reusable::() || { let last_changed_revision = zalsa.last_changed_revision(value_shared.durability); ({ value_shared.last_interned_at }) >= last_changed_revision From cc186cd7ed40b02ad87a616001a5496d024f1fd5 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 27 Jun 2026 19:29:22 +0000 Subject: [PATCH 06/10] fix: satisfy Rust 1.85 Clippy --- src/interned.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/interned.rs b/src/interned.rs index a66efc28d..13f66f2a9 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -709,8 +709,8 @@ where insert_unique(shard, hash, slot.new_id, &hasher); // Remove the value from the LRU list. - // SAFETY: We hold the shard lock and `value` is currently in the LRU. let removed_header = UnsafeRef::into_raw( + // SAFETY: We hold the shard lock and `value` is currently in the LRU. unsafe { shard.lru.cursor_mut_from_ptr(&value.header).remove() }.unwrap(), ); debug_assert!(std::ptr::eq(removed_header, &value.header)); @@ -832,7 +832,7 @@ where }); // SAFETY: We hold the lock for the shard containing the value. - let reusable = unsafe { (&*value.header.shared.get()).is_reusable::() }; + let reusable = unsafe { (*value.header.shared.get()).is_reusable::() }; // Insert the newly allocated ID. // SAFETY: We hold the lock for the shard containing every value passed to `hasher`. From 5b68123379942ff570b66a2e431dc4961d12c8cd Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 27 Jun 2026 19:36:11 +0000 Subject: [PATCH 07/10] fix: update persisted interned insertion --- src/interned.rs | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/interned.rs b/src/interned.rs index 13f66f2a9..1aba55875 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -1837,8 +1837,17 @@ mod persistence { "values are serialized in allocation order" ); + // SAFETY: We hold the lock for the shard containing the value. + let reusable = unsafe { (*value.header.shared.get()).is_reusable::() }; + // Insert the newly allocated ID into our ingredient. - ingredient.insert_id(id, zalsa, shard, hash, value); + // SAFETY: We hold the lock for the shard containing every value passed to + // `hasher`. + let hasher = |id: &_| unsafe { ingredient.value_hash(zalsa, *id) }; + // SAFETY: We hold the lock for `shard`; `value` is the live value identified by + // `id` in that shard, `reusable` was read from the value, and `header_ptr` retains + // provenance for the complete value. + unsafe { super::insert_id(id, shard, hash, value.header_ptr(), reusable, &hasher) }; } Ok(()) From 8df165bd356e66ce714683d443bfe8c3335b6311 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sun, 28 Jun 2026 09:23:05 +0000 Subject: [PATCH 08/10] refactor: clarify interned erasure safety --- src/interned.rs | 437 ++++++++++++++++++++++++++++-------------------- 1 file changed, 256 insertions(+), 181 deletions(-) diff --git a/src/interned.rs b/src/interned.rs index 1aba55875..662cd256a 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -125,16 +125,17 @@ impl Default for IngredientShard { } } -// SAFETY: `LinkedListLink` is `!Sync`, however, the linked list is only accessed through the -// ingredient lock, and values are only ever linked to a single list on the ingredient. +// SAFETY: `IngredientShard` is only accessed through its mutex. Its LRU contains pointers to live, +// stable values from this ingredient, and those pointers and their links are accessed only while +// holding that mutex. +unsafe impl Send for IngredientShard {} + +// SAFETY: `fields`, `header.link`, and `header.shared` are mutated only while holding the ingredient +// shard lock. `fields` is read only while holding that lock or after validation in the current +// revision. `header.memos` supports concurrent shared access, and is mutated only with exclusive +// access or after stale-slot reuse guarantees no shared references remain. unsafe impl Sync for Value {} -// SAFETY: `LinkedListLink` and `shared` are only accessed while holding the ingredient shard -// lock. `memos` can also be accessed without that lock through `Slot::memos`, but those accesses -// are shared and `MemoTable` supports concurrent shared access internally. Mutable access to -// `memos` requires exclusive access to the value. -unsafe impl Sync for ValueHeader {} - intrusive_adapter!(ValueHeaderAdapter = UnsafeRef: ValueHeader { link => LinkedListLink }); /// Struct storing the interned fields. @@ -180,9 +181,6 @@ struct ValueHeader { shared: UnsafeCell, } -#[cfg(all(not(feature = "shuttle"), target_pointer_width = "64"))] -const _: [(); std::mem::size_of::()] = [(); std::mem::size_of::<[usize; 7]>()]; - /// Shared value data can only be read through the lock. #[repr(Rust, packed)] // Avoid padding around `durability` inside `ValueHeader`. #[derive(Clone, Copy)] @@ -255,136 +253,17 @@ impl ValueShared { } } -/// Clears the given memo table. -/// -/// # Safety -/// -/// `memo_table` must belong to the value identified by `id` in `ingredient_index` and must have -/// been created with `memo_table_types`. The caller must have exclusive access to the table. -unsafe fn clear_memos( - zalsa: &Zalsa, - ingredient_index: IngredientIndex, - memo_table_types: &MemoTableTypes, - memo_table: &mut MemoTable, - id: Id, -) { - // SAFETY: The caller guarantees this is the correct types table. - let table = unsafe { memo_table_types.attach_memos_mut(memo_table) }; - - // `Database::salsa_event` is a user supplied callback which may panic - // in that case we need a drop guard to free the memo table - struct TableDropGuard<'a>(MemoTableWithTypesMut<'a>); - - impl Drop for TableDropGuard<'_> { - fn drop(&mut self) { - // SAFETY: We have `&mut MemoTable`, so no more references to these memos exist and we are good - // to drop them. - unsafe { self.0.drop() }; - } - } - - let mut table_guard = TableDropGuard(table); - - // SAFETY: We have `&mut MemoTable`, so no more references to these memos exist and we are good - // to drop them. - unsafe { - table_guard.0.take_memos(|memo_ingredient_index, memo| { - let ingredient_index = - zalsa.ingredient_index_for_memo(ingredient_index, memo_ingredient_index); - - let executor = DatabaseKeyIndex::new(ingredient_index, id); - - zalsa.event(&|| Event::new(EventKind::DidDiscard { key: executor })); - - memo.remove_outputs(zalsa, executor); - }) - }; - - std::mem::forget(table_guard); - - // Reset the table after having dropped any memos. - memo_table.reset(); -} - struct ReusableSlot { header: *const ValueHeader, old_id: Id, new_id: Id, } -/// Finds the least-recently-used stale slot whose generation can be incremented. -/// -/// # Safety -/// -/// The caller must hold `shard`'s lock, and every pointer in its LRU must refer to a live -/// `ValueHeader` belonging to the shard. -unsafe fn find_reusable_slot( - revision_queue: &RevisionQueue, - current_revision: Revision, - shard: &mut IngredientShard, -) -> Option { - let mut cursor = shard.lru.back_mut(); - - while let Some(header) = cursor.as_cursor().clone_pointer() { - let header = UnsafeRef::into_raw(header); - - // SAFETY: Guaranteed by the caller. - let value_shared = unsafe { &mut *(*header).shared.get() }; - - // The list is sorted by LRU, so if the tail is not stale, no slot is stale. - if !revision_queue.is_stale(value_shared.last_interned_at) { - return None; - } - - // We should never reuse a value that was accessed in the current revision. - debug_assert!({ value_shared.last_interned_at } < current_revision); - - let old_id = value_shared.id; - if let Some(new_id) = old_id.next_generation() { - return Some(ReusableSlot { - header, - old_id, - new_id, - }); - } - - // This slot can never be reused. Remove it and retry with the previous element. - cursor.remove().unwrap(); - cursor = shard.lru.back_mut(); - } - - None -} - +/// Keeps `HashTable::insert_unique` and its rehashing logic independent of `C`. fn insert_unique(shard: &mut IngredientShard, hash: u64, id: Id, hasher: &dyn Fn(&Id) -> u64) { shard.key_map.insert_unique(hash, id, hasher); } -/// Inserts a newly allocated value into the LRU and key map. -/// -/// # Safety -/// -/// The caller must hold `shard`'s lock. `header` must have been produced by `Value::header_ptr` -/// for the live value identified by `id` in this shard, `reusable` must reflect whether that value -/// is reusable, and `hasher` must hash values while that lock is held. -unsafe fn insert_id( - id: Id, - shard: &mut IngredientShard, - hash: u64, - header: *const ValueHeader, - reusable: bool, - hasher: &dyn Fn(&Id) -> u64, -) { - if reusable { - // SAFETY: Guaranteed by the caller, including full-value provenance. - unsafe { shard.lru.push_front(UnsafeRef::from_raw(header)) }; - } - - insert_unique(shard, hash, id, hasher); - - debug_assert_eq!(hash, hasher(&id)); -} - impl Value where C: Configuration, @@ -648,9 +527,9 @@ where } // Otherwise, try to reuse a stale slot. - // SAFETY: We hold the shard lock and all LRU pointers refer to live values in this shard. - let Some(slot) = - (unsafe { find_reusable_slot(&self.revision_queue, current_revision, shard) }) + // SAFETY: We hold the lock for this ingredient's shard, and `current_revision` is the + // database's current revision. + let Some((slot, value)) = (unsafe { self.find_reusable_slot(current_revision, shard) }) else { // If we could not find a stale slot, we are forced to allocate a new one. return self.intern_id_cold( @@ -664,11 +543,6 @@ where ); }; - // SAFETY: Each shard belongs to a single `IngredientImpl`, and only `Value`s from - // that ingredient are inserted into its LRU. The pointer retains complete-value - // provenance, and `Value` uses the C representation with `header` as its first field. - let value = unsafe { &*slot.header.cast::>() }; - // SAFETY: We hold the lock for the shard containing the value. let value_shared = unsafe { &mut *value.header.shared.get() }; @@ -709,11 +583,8 @@ where insert_unique(shard, hash, slot.new_id, &hasher); // Remove the value from the LRU list. - let removed_header = UnsafeRef::into_raw( - // SAFETY: We hold the shard lock and `value` is currently in the LRU. - unsafe { shard.lru.cursor_mut_from_ptr(&value.header).remove() }.unwrap(), - ); - debug_assert!(std::ptr::eq(removed_header, &value.header)); + // SAFETY: We hold the shard lock and `value` is currently in the LRU. + unsafe { shard.lru.cursor_mut_from_ptr(&value.header).remove() }; // Remove the previous value from the ID map. // @@ -730,9 +601,8 @@ where // Replace the fields without dropping the previous value until the slot is consistent. // - // SAFETY: We hold the lock for the shard containing the value, and the - // value has not been interned in the current revision, so no references to - // it can exist. + // SAFETY: `find_reusable_slot` guarantees that the value is reusable and stale, so no + // references to its fields remain. We still hold the shard lock. let old_fields = unsafe { std::mem::replace(&mut *value.fields.get(), new_fields) }; // Mark the slot as reused. @@ -754,23 +624,14 @@ where }; } - // SAFETY: We hold the lock for the shard containing the value, and the - // value has not been interned in the current revision, so no references to - // it can exist. + // SAFETY: `find_reusable_slot` guarantees that the value is reusable and stale, so no + // references to its memos remain. We still hold the shard lock. let memo_table = unsafe { &mut *value.header.memos.get() }; // Free the memos associated with the previous interned value. // // SAFETY: The memo table belongs to a value allocated with these memo-table types. - unsafe { - clear_memos( - zalsa, - self.ingredient_index, - &self.memo_table_types, - memo_table, - slot.old_id, - ) - }; + unsafe { self.clear_memos(zalsa, memo_table, slot.old_id) }; drop(old_fields); @@ -831,16 +692,8 @@ where fields: UnsafeCell::new(unsafe { self.to_internal_data(assemble(id, key)) }), }); - // SAFETY: We hold the lock for the shard containing the value. - let reusable = unsafe { (*value.header.shared.get()).is_reusable::() }; - // Insert the newly allocated ID. - // SAFETY: We hold the lock for the shard containing every value passed to `hasher`. - let hasher = |id: &_| unsafe { self.value_hash(zalsa, *id) }; - // SAFETY: We hold the lock for `shard`; `value` is the live value identified by `id` in - // that shard, `reusable` was read from the value, and `header_ptr` retains provenance for - // the complete value. - unsafe { insert_id(id, shard, hash, value.header_ptr(), reusable, &hasher) }; + self.insert_id(id, zalsa, shard, hash, value); let index = self.database_key_index(id); @@ -869,6 +722,197 @@ where id } + /// Inserts a newly interned value ID into the LRU list and key map. + fn insert_id( + &self, + id: Id, + zalsa: &Zalsa, + shard: &mut IngredientShard, + hash: u64, + value: &Value, + ) { + /// Inserts a newly allocated value without depending on `C`. + /// + /// # Safety + /// + /// The caller must hold `shard`'s lock. `header` must have been produced by + /// `Value::header_ptr` for the live value identified by `id` in this shard, `reusable` must + /// reflect whether that value is reusable, and `hasher` must hash values while that lock is + /// held. + unsafe fn inner( + id: Id, + shard: &mut IngredientShard, + hash: u64, + header: *const ValueHeader, + reusable: bool, + hasher: &dyn Fn(&Id) -> u64, + ) { + if reusable { + // SAFETY: Guaranteed by the caller, including full-value provenance. + unsafe { shard.lru.push_front(UnsafeRef::from_raw(header)) }; + } + + insert_unique(shard, hash, id, hasher); + + debug_assert_eq!(hash, hasher(&id)); + } + + // SAFETY: We hold the lock for the shard containing the value. + let reusable = unsafe { (*value.header.shared.get()).is_reusable::() }; + + // SAFETY: We hold the lock for the shard containing every value passed to `hasher`. + let hasher = |id: &_| unsafe { self.value_hash(zalsa, *id) }; + + // SAFETY: We hold the lock for `shard`; `value` is the live value identified by `id` in + // that shard, `reusable` was read from the value, and `header_ptr` retains provenance for + // the complete value. + unsafe { inner(id, shard, hash, value.header_ptr(), reusable, &hasher) }; + } + + /// Finds a reusable slot and reconstructs its typed value. + /// + /// # Safety + /// + /// `shard` must be a locked shard from this ingredient, and `current_revision` must be the + /// database's current revision. Every LRU entry must have been inserted from a live `Value` + /// allocated for this ingredient using `Value::header_ptr`, which retains full-allocation + /// provenance. Those allocations must remain stable for the ingredient's lifetime. + /// + /// The LRU contains only reusable values. Therefore, a slot returned as stale has no + /// outstanding references to its fields or memos and may be mutated while the lock is held. + unsafe fn find_reusable_slot( + &self, + current_revision: Revision, + shard: &mut IngredientShard, + ) -> Option<(ReusableSlot, &Value)> { + /// Finds the least-recently-used stale slot whose generation can be incremented. + /// + /// # Safety + /// + /// The caller must hold `shard`'s lock, and every pointer in its LRU must refer to a live + /// `ValueHeader` belonging to the shard. + unsafe fn inner( + revision_queue: &RevisionQueue, + current_revision: Revision, + shard: &mut IngredientShard, + ) -> Option { + let mut cursor = shard.lru.back_mut(); + + while let Some(header) = cursor.as_cursor().clone_pointer() { + let header = UnsafeRef::into_raw(header); + + // SAFETY: Guaranteed by the caller. + let value_shared = unsafe { &mut *(*header).shared.get() }; + + // The list is sorted by LRU, so if the tail is not stale, no slot is stale. + if !revision_queue.is_stale(value_shared.last_interned_at) { + return None; + } + + // We should never reuse a value that was accessed in the current revision. + debug_assert!({ value_shared.last_interned_at } < current_revision); + + let old_id = value_shared.id; + if let Some(new_id) = old_id.next_generation() { + return Some(ReusableSlot { + header, + old_id, + new_id, + }); + } + + // This slot can never be reused. Remove it and retry with the previous element. + cursor.remove().unwrap(); + cursor = shard.lru.back_mut(); + } + + None + } + + // SAFETY: Guaranteed by the caller. + let slot = unsafe { inner(&self.revision_queue, current_revision, shard) }?; + + // SAFETY: The caller guarantees that this is a shard from `self`. Its LRU contains only + // header pointers produced by `Value::header_ptr` for stable values allocated by this + // ingredient. `Value` has the C representation and `header` is statically asserted to + // be at offset zero. + let value = unsafe { &*slot.header.cast::>() }; + + Some((slot, value)) + } + + /// Clears the given memo table. + /// + /// # Safety + /// + /// `memo_table` must belong to the value identified by `id` in this ingredient. The caller + /// must have exclusive access to the table. + pub(crate) unsafe fn clear_memos(&self, zalsa: &Zalsa, memo_table: &mut MemoTable, id: Id) { + /// Clears the given memo table without depending on `C`. + /// + /// # Safety + /// + /// `memo_table` must belong to the value identified by `id` in `ingredient_index` and must + /// have been created with `memo_table_types`. The caller must have exclusive access to the + /// table. + unsafe fn inner( + zalsa: &Zalsa, + ingredient_index: IngredientIndex, + memo_table_types: &MemoTableTypes, + memo_table: &mut MemoTable, + id: Id, + ) { + // SAFETY: The caller guarantees this is the correct types table. + let table = unsafe { memo_table_types.attach_memos_mut(memo_table) }; + + // `Database::salsa_event` is a user supplied callback which may panic + // in that case we need a drop guard to free the memo table + struct TableDropGuard<'a>(MemoTableWithTypesMut<'a>); + + impl Drop for TableDropGuard<'_> { + fn drop(&mut self) { + // SAFETY: We have `&mut MemoTable`, so no more references to these memos exist and we are good + // to drop them. + unsafe { self.0.drop() }; + } + } + + let mut table_guard = TableDropGuard(table); + + // SAFETY: We have `&mut MemoTable`, so no more references to these memos exist and we are good + // to drop them. + unsafe { + table_guard.0.take_memos(|memo_ingredient_index, memo| { + let ingredient_index = + zalsa.ingredient_index_for_memo(ingredient_index, memo_ingredient_index); + + let executor = DatabaseKeyIndex::new(ingredient_index, id); + + zalsa.event(&|| Event::new(EventKind::DidDiscard { key: executor })); + + memo.remove_outputs(zalsa, executor); + }) + }; + + std::mem::forget(table_guard); + + // Reset the table after having dropped any memos. + memo_table.reset(); + } + + // SAFETY: The caller guarantees the table belongs to this ingredient's value, so these are + // the correct ingredient index and memo-table types. + unsafe { + inner( + zalsa, + self.ingredient_index, + &self.memo_table_types, + memo_table, + id, + ) + } + } + // Hashes the value by its fields. // // # Safety @@ -975,9 +1019,9 @@ where // TODO: Grab all locks eagerly. zalsa.table().slots_of::>().map(move |(_, value)| { let id = if should_lock { - let shard_index = value.header.shard as usize; - // SAFETY: `value.header.shard` is guaranteed to be in-bounds for `self.shards`. - let _shard = unsafe { self.shards.get_unchecked(shard_index) }.lock(); + let _shard = + // SAFETY: `value.header.shard` is guaranteed to be in-bounds for `self.shards`. + unsafe { self.shards.get_unchecked(value.header.shard as usize) }.lock(); // SAFETY: We hold the lock for the shard containing the value. unsafe { (*value.header.shared.get()).id } @@ -1837,17 +1881,8 @@ mod persistence { "values are serialized in allocation order" ); - // SAFETY: We hold the lock for the shard containing the value. - let reusable = unsafe { (*value.header.shared.get()).is_reusable::() }; - // Insert the newly allocated ID into our ingredient. - // SAFETY: We hold the lock for the shard containing every value passed to - // `hasher`. - let hasher = |id: &_| unsafe { ingredient.value_hash(zalsa, *id) }; - // SAFETY: We hold the lock for `shard`; `value` is the live value identified by - // `id` in that shard, `reusable` was read from the value, and `header_ptr` retains - // provenance for the complete value. - unsafe { super::insert_id(id, shard, hash, value.header_ptr(), reusable, &hasher) }; + ingredient.insert_id(id, zalsa, shard, hash, value); } Ok(()) @@ -1880,6 +1915,46 @@ mod persistence { } } +mod _static_assertions { + use std::mem; + + #[cfg(all(not(feature = "shuttle"), target_pointer_width = "64"))] + use super::ValueHeader; + use super::{Configuration, Value}; + use crate::{Id, plumbing}; + + const _: [(); 0] = [(); mem::offset_of!(Value, header)]; + + #[cfg(all(not(feature = "shuttle"), target_pointer_width = "64"))] + const _: [(); mem::size_of::()] = [(); mem::size_of::<[usize; 7]>()]; + + struct DummyConfiguration; + + impl Configuration for DummyConfiguration { + const LOCATION: crate::ingredient::Location = + crate::ingredient::Location { file: "", line: 0 }; + const DEBUG_NAME: &'static str = ""; + const PERSIST: bool = false; + + type Fields<'db> = (); + type Struct<'db> = Id; + + fn serialize(_: &Self::Fields<'_>, _: S) -> Result + where + S: plumbing::serde::Serializer, + { + unimplemented!() + } + + fn deserialize<'de, D>(_: D) -> Result, D::Error> + where + D: plumbing::serde::Deserializer<'de>, + { + unimplemented!() + } + } +} + #[cfg(test)] mod tests { use super::*; From 725a0c4553e497aaa5f2107fbc21ca67b29ad994 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sun, 28 Jun 2026 10:11:53 +0000 Subject: [PATCH 09/10] refactor: simplify interned insertion helpers --- src/interned.rs | 54 +++++++++++++++++++++++++++++++------------------ 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/src/interned.rs b/src/interned.rs index 662cd256a..f25f2ed42 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -259,11 +259,6 @@ struct ReusableSlot { new_id: Id, } -/// Keeps `HashTable::insert_unique` and its rehashing logic independent of `C`. -fn insert_unique(shard: &mut IngredientShard, hash: u64, id: Id, hasher: &dyn Fn(&Id) -> u64) { - shard.key_map.insert_unique(hash, id, hasher); -} - impl Value where C: Configuration, @@ -442,7 +437,7 @@ where let found_value = Cell::new(None); // SAFETY: We hold the lock for the shard containing the value. - let eq = |id: &_| unsafe { Self::value_eq(zalsa, *id, &key, &found_value) }; + let eq = |id: &_| unsafe { Self::value_eq(*id, &key, zalsa, &found_value) }; // Attempt a fast-path lookup of already interned data. if let Some(&id) = shard.key_map.find(hash, eq) { @@ -551,8 +546,7 @@ where .active_query() .map(|(_, stamp)| (stamp.durability, current_revision)) // If there is no active query this durability does not actually matter. - // `last_interned_at` needs to be `Revision::MAX`, see the - // `intern_access_in_different_revision` test. + // `last_interned_at` needs to be `Revision::MAX`, see the `intern_access_in_different_revision` test. .unwrap_or((Durability::MAX, Revision::max())); // Assemble and hash the replacement before mutating the existing slot. Both operations @@ -579,8 +573,8 @@ where // currently performs any rehashing before inserting, so a panic in user hashing // leaves the old value reachable. Revisit this assumption when updating hashbrown. // SAFETY: We hold the lock for the shard containing every value passed to `hasher`. - let hasher = |id: &_| unsafe { self.value_hash(zalsa, *id) }; - insert_unique(shard, hash, slot.new_id, &hasher); + let hasher = |id: &_| unsafe { self.value_hash(*id, zalsa) }; + insert_unique_erased(shard, hash, slot.new_id, &hasher); // Remove the value from the LRU list. // SAFETY: We hold the shard lock and `value` is currently in the LRU. @@ -670,8 +664,7 @@ where .active_query() .map(|(_, stamp)| (stamp.durability, current_revision)) // If there is no active query this durability does not actually matter. - // `last_interned_at` needs to be `Revision::MAX`, see the - // `intern_access_in_different_revision` test. + // `last_interned_at` needs to be `Revision::MAX`, see the `intern_access_in_different_revision` test. .unwrap_or((Durability::MAX, Revision::max())); // Allocate the value slot. @@ -693,7 +686,9 @@ where }); // Insert the newly allocated ID. - self.insert_id(id, zalsa, shard, hash, value); + // SAFETY: We hold this ingredient's shard lock, and `value` is the live value identified by + // `id` that we just allocated in that shard. + unsafe { self.insert_id(id, zalsa, shard, hash, value) }; let index = self.database_key_index(id); @@ -723,7 +718,12 @@ where } /// Inserts a newly interned value ID into the LRU list and key map. - fn insert_id( + /// + /// # Safety + /// + /// `shard` must be a locked shard from this ingredient, and `value` must be the live, stable + /// `Value` identified by `id` in that shard. + unsafe fn insert_id( &self, id: Id, zalsa: &Zalsa, @@ -752,16 +752,17 @@ where unsafe { shard.lru.push_front(UnsafeRef::from_raw(header)) }; } - insert_unique(shard, hash, id, hasher); + insert_unique_erased(shard, hash, id, hasher); debug_assert_eq!(hash, hasher(&id)); } // SAFETY: We hold the lock for the shard containing the value. - let reusable = unsafe { (*value.header.shared.get()).is_reusable::() }; + let value_shared = unsafe { &mut *value.header.shared.get() }; + let reusable = value_shared.is_reusable::(); // SAFETY: We hold the lock for the shard containing every value passed to `hasher`. - let hasher = |id: &_| unsafe { self.value_hash(zalsa, *id) }; + let hasher = |id: &_| unsafe { self.value_hash(*id, zalsa) }; // SAFETY: We hold the lock for `shard`; `value` is the live value identified by `id` in // that shard, `reusable` was read from the value, and `header_ptr` retains provenance for @@ -918,7 +919,7 @@ where // # Safety // // The lock must be held for the shard containing the value. - unsafe fn value_hash<'db>(&'db self, zalsa: &'db Zalsa, id: Id) -> u64 { + unsafe fn value_hash<'db>(&'db self, id: Id, zalsa: &'db Zalsa) -> u64 { // This closure is only called if the table is resized. So while it's expensive // to lookup all values, it will only happen rarely. let value = zalsa.table().get::>(id); @@ -933,9 +934,9 @@ where // // The lock must be held for the shard containing the value. unsafe fn value_eq<'db, Key>( - zalsa: &'db Zalsa, id: Id, key: &Key, + zalsa: &'db Zalsa, found_value: &Cell>>, ) -> bool where @@ -1038,6 +1039,16 @@ where } } +/// Inserts an ID while keeping the hasher and rehashing logic independent of `C`. +fn insert_unique_erased( + shard: &mut IngredientShard, + hash: u64, + id: Id, + hasher: &dyn Fn(&Id) -> u64, +) { + shard.key_map.insert_unique(hash, id, hasher); +} + /// An interned struct entry. pub struct StructEntry<'db, C> where @@ -1882,7 +1893,10 @@ mod persistence { ); // Insert the newly allocated ID into our ingredient. - ingredient.insert_id(id, zalsa, shard, hash, value); + // + // SAFETY: We hold this ingredient's shard lock, and `value` is the live value + // identified by `id` that we just allocated in that shard. + unsafe { ingredient.insert_id(id, zalsa, shard, hash, value) }; } Ok(()) From 132d710638897f6fbee339dc5526ff16c31c6a23 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sun, 28 Jun 2026 10:30:41 +0000 Subject: [PATCH 10/10] refactor: align interned cold path with upstream --- src/interned.rs | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/interned.rs b/src/interned.rs index f25f2ed42..5c8a08997 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -511,9 +511,9 @@ where // Fill up the table for the first few revisions without attempting garbage collection. if !self.revision_queue.is_primed() { return self.intern_id_cold( + key, zalsa, zalsa_local, - key, assemble, shard, shard_index, @@ -528,9 +528,9 @@ where else { // If we could not find a stale slot, we are forced to allocate a new one. return self.intern_id_cold( + key, zalsa, zalsa_local, - key, assemble, shard, shard_index, @@ -645,9 +645,9 @@ where #[allow(clippy::too_many_arguments)] fn intern_id_cold<'db, Key>( &'db self, + key: Key, zalsa: &Zalsa, zalsa_local: &ZalsaLocal, - key: Key, assemble: impl FnOnce(Id, Key) -> C::Fields<'db>, shard: &mut IngredientShard, shard_index: usize, @@ -802,10 +802,15 @@ where while let Some(header) = cursor.as_cursor().clone_pointer() { let header = UnsafeRef::into_raw(header); - // SAFETY: Guaranteed by the caller. + // SAFETY: The caller guarantees that `header` points to a live value in this shard + // and that we hold the shard lock, which grants exclusive access to `shared`. let value_shared = unsafe { &mut *(*header).shared.get() }; - // The list is sorted by LRU, so if the tail is not stale, no slot is stale. + // The value must not have been read in the current revision to be collected + // soundly, but we also do not want to collect values that have been read recently. + // + // Note that the list is sorted by LRU, so if the tail of the list is not stale, we + // will not find any stale slots. if !revision_queue.is_stale(value_shared.last_interned_at) { return None; } @@ -814,6 +819,10 @@ where debug_assert!({ value_shared.last_interned_at } < current_revision); let old_id = value_shared.id; + + // Increment the generation of the ID, as if we allocated a new slot. + // + // If the ID is at its maximum generation, we are forced to leak the slot. if let Some(new_id) = old_id.next_generation() { return Some(ReusableSlot { header,