diff --git a/components/salsa-macro-rules/src/setup_interned_struct.rs b/components/salsa-macro-rules/src/setup_interned_struct.rs index e8b8af182..f3eeb83c3 100644 --- a/components/salsa-macro-rules/src/setup_interned_struct.rs +++ b/components/salsa-macro-rules/src/setup_interned_struct.rs @@ -69,21 +69,29 @@ macro_rules! setup_interned_struct { /// Key to use during hash lookups. Each field is some type that implements `Lookup` /// for the owned type. This permits interning with an `&str` when a `String` is required and so forth. - struct StructKey<$db_lt, $($indexed_ty: $zalsa::interned::Lookup<$field_ty>),*>( + #[derive(Hash)] + struct StructKey<$db_lt, $($indexed_ty),*>( $($indexed_ty,)* std::marker::PhantomData<&$db_lt ()>, ); - impl<$db_lt, $($indexed_ty: $zalsa::interned::Lookup<$field_ty>),*> $zalsa::interned::Lookup> - for StructKey<$db_lt, $($indexed_ty),*> { + impl<$db_lt, $($indexed_ty,)*> $zalsa::interned::HashEqLike> + for StructData<$db_lt> + where + $($field_ty: $zalsa::interned::HashEqLike<$indexed_ty>),* + { fn hash(&self, h: &mut H) { - $($zalsa::interned::Lookup::hash(&self.$field_index, &mut *h);)* + $($zalsa::interned::HashEqLike::<$indexed_ty>::hash(&self.$field_index, &mut *h);)* } - fn eq(&self, data: &StructData<$db_lt>) -> bool { - ($($zalsa::interned::Lookup::eq(&self.$field_index, &data.$field_index) && )* true) + fn eq(&self, data: &StructKey<$db_lt, $($indexed_ty),*>) -> bool { + ($($zalsa::interned::HashEqLike::<$indexed_ty>::eq(&self.$field_index, &data.$field_index) && )* true) } + } + + impl<$db_lt, $($indexed_ty: $zalsa::interned::Lookup<$field_ty>),*> $zalsa::interned::Lookup> + for StructKey<$db_lt, $($indexed_ty),*> { #[allow(unused_unit)] fn into_owned(self) -> StructData<$db_lt> { @@ -158,14 +166,17 @@ macro_rules! setup_interned_struct { } impl<$db_lt> $Struct<$db_lt> { - pub fn $new_fn<$Db>(db: &$db_lt $Db, $($field_id: impl $zalsa::interned::Lookup<$field_ty>),*) -> Self + pub fn $new_fn<$Db, $($indexed_ty: $zalsa::interned::Lookup<$field_ty> + std::hash::Hash,)*>(db: &$db_lt $Db, $($field_id: $indexed_ty),*) -> Self where // FIXME(rust-lang/rust#65991): The `db` argument *should* have the type `dyn Database` $Db: ?Sized + salsa::Database, + $( + $field_ty: $zalsa::interned::HashEqLike<$indexed_ty>, + )* { let current_revision = $zalsa::current_revision(db); $Configuration::ingredient(db).intern(db.as_dyn_database(), - StructKey::<$db_lt>($($field_id,)* std::marker::PhantomData::default())) + StructKey::<$db_lt>($($field_id,)* std::marker::PhantomData::default()), |_, data| ($($zalsa::interned::Lookup::into_owned(data.$field_index),)*)) } $( diff --git a/components/salsa-macro-rules/src/setup_tracked_fn.rs b/components/salsa-macro-rules/src/setup_tracked_fn.rs index ce5d313d1..accffbf44 100644 --- a/components/salsa-macro-rules/src/setup_tracked_fn.rs +++ b/components/salsa-macro-rules/src/setup_tracked_fn.rs @@ -249,7 +249,7 @@ macro_rules! setup_tracked_fn { use salsa::plumbing as $zalsa; let key = $zalsa::macro_if! { if $needs_interner { - $Configuration::intern_ingredient($db).intern_id($db.as_dyn_database(), ($($input_id),*)) + $Configuration::intern_ingredient($db).intern_id($db.as_dyn_database(), ($($input_id),*), |_, data| data) } else { $zalsa::AsId::as_id(&($($input_id),*)) } @@ -285,7 +285,7 @@ macro_rules! setup_tracked_fn { let result = $zalsa::macro_if! { if $needs_interner { { - let key = $Configuration::intern_ingredient($db).intern_id($db.as_dyn_database(), ($($input_id),*)); + let key = $Configuration::intern_ingredient($db).intern_id($db.as_dyn_database(), ($($input_id),*), |_, data| data); $Configuration::fn_ingredient($db).fetch($db, key) } } else { diff --git a/src/input.rs b/src/input.rs index eac540abf..e4bbffd85 100644 --- a/src/input.rs +++ b/src/input.rs @@ -117,7 +117,7 @@ impl IngredientImpl { None }; - let id = zalsa_local.allocate(zalsa.table(), self.ingredient_index, || Value:: { + let id = zalsa_local.allocate(zalsa.table(), self.ingredient_index, |_| Value:: { fields, stamps, memos: Default::default(), diff --git a/src/interned.rs b/src/interned.rs index 62ef4fe62..76b1dda9b 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -1,6 +1,7 @@ +use dashmap::SharedValue; + use crate::accumulator::accumulated_map::InputAccumulatedValues; use crate::durability::Durability; -use crate::id::AsId; use crate::ingredient::fmt_index; use crate::key::DependencyIndex; use crate::plumbing::{Jar, JarAux}; @@ -120,20 +121,34 @@ where unsafe { std::mem::transmute(data) } } - pub fn intern_id<'db>( + pub fn intern<'db, Key>( &'db self, db: &'db dyn crate::Database, - data: impl Lookup>, - ) -> crate::Id { - C::deref_struct(self.intern(db, data)).as_id() + key: Key, + assemble: impl FnOnce(Id, Key) -> C::Data<'db>, + ) -> C::Struct<'db> + where + Key: Hash, + C::Data<'db>: HashEqLike, + { + C::struct_from_id(self.intern_id(db, key, assemble)) } /// Intern data to a unique reference. - pub fn intern<'db>( + pub fn intern_id<'db, Key>( &'db self, db: &'db dyn crate::Database, - data: impl Lookup>, - ) -> C::Struct<'db> { + key: Key, + assemble: impl FnOnce(Id, Key) -> C::Data<'db>, + ) -> crate::Id + where + Key: Hash, + // We'd want the following predicate, but this currently implies `'static` due to a rustc + // bug + // for<'db> C::Data<'db>: HashEqLike, + // so instead we go with this and transmute the lifetime in the `eq` closure + C::Data<'db>: HashEqLike, + { let zalsa_local = db.zalsa_local(); zalsa_local.report_tracked_read( DependencyIndex::for_table(self.ingredient_index), @@ -142,51 +157,53 @@ where InputAccumulatedValues::Empty, ); - // Optimisation to only get read lock on the map if the data has already - // been interned. - // We need to use the raw API for this lookup. See the [`Lookup`][] trait definition for an explanation of why. - let data_hash = { - let mut hasher = self.key_map.hasher().build_hasher(); - data.hash(&mut hasher); - hasher.finish() + // Optimization to only get read lock on the map if the data has already been interned. + let data_hash = self.key_map.hasher().hash_one(&key); + let shard = &self.key_map.shards()[self.key_map.determine_shard(data_hash as _)]; + let eq = |(data, _): &_| { + // SAFETY: it's safe to go from Data<'static> to Data<'db> + // shrink lifetime here to use a single lifetime in Lookup::eq(&StructKey<'db>, &C::Data<'db>) + let data: &C::Data<'db> = unsafe { std::mem::transmute(data) }; + HashEqLike::eq(data, &key) }; - let shard = self.key_map.determine_shard(data_hash as _); + { - let lock = self.key_map.shards()[shard].read(); - if let Some(bucket) = lock.find(data_hash, |(a, _)| { - // SAFETY: it's safe to go from Data<'static> to Data<'db> - // shrink lifetime here to use a single lifetime in Lookup::eq(&StructKey<'db>, &C::Data<'db>) - let a: &C::Data<'db> = unsafe { std::mem::transmute(a) }; - Lookup::eq(&data, a) - }) { + let lock = shard.read(); + if let Some(bucket) = lock.find(data_hash, eq) { // SAFETY: Read lock on map is held during this block - return C::struct_from_id(unsafe { *bucket.as_ref().1.get() }); + return unsafe { *bucket.as_ref().1.get() }; } - }; - - let data = data.into_owned(); - - let internal_data = unsafe { self.to_internal_data(data) }; + } - match self.key_map.entry(internal_data.clone()) { + let mut lock = shard.write(); + match lock.find_or_find_insert_slot(data_hash, eq, |(element, _)| { + self.key_map.hasher().hash_one(element) + }) { // Data has been interned by a racing call, use that ID instead - dashmap::mapref::entry::Entry::Occupied(entry) => { - let id = *entry.get(); - drop(entry); - C::struct_from_id(id) - } - + Ok(slot) => unsafe { *slot.as_ref().1.get() }, // We won any races so should intern the data - dashmap::mapref::entry::Entry::Vacant(entry) => { + Err(slot) => { let zalsa = db.zalsa(); let table = zalsa.table(); - let next_id = zalsa_local.allocate(table, self.ingredient_index, || Value:: { - data: internal_data, + let id = zalsa_local.allocate(table, self.ingredient_index, |id| Value:: { + data: unsafe { self.to_internal_data(assemble(id, key)) }, memos: Default::default(), syncs: Default::default(), }); - entry.insert(next_id); - C::struct_from_id(next_id) + unsafe { + lock.insert_in_slot( + data_hash, + slot, + (table.get::>(id).data.clone(), SharedValue::new(id)), + ) + }; + debug_assert_eq!( + data_hash, + self.key_map + .hasher() + .hash_one(table.get::>(id).data.clone()) + ); + id } } } @@ -312,6 +329,10 @@ where &self.syncs } } +pub trait HashEqLike { + fn hash(&self, h: &mut H); + fn eq(&self, data: &O) -> bool; +} /// The `Lookup` trait is a more flexible variant on [`std::borrow::Borrow`] /// and [`std::borrow::ToOwned`]. @@ -328,12 +349,14 @@ where /// requires that `&(K1...)` be convertible to `&ViewStruct` which just isn't /// possible. `Lookup` instead offers direct `hash` and `eq` methods. pub trait Lookup { - fn hash(&self, h: &mut H); - fn eq(&self, data: &O) -> bool; fn into_owned(self) -> O; } - -impl Lookup for T +impl Lookup for T { + fn into_owned(self) -> T { + self + } +} +impl HashEqLike for T where T: Hash + Eq, { @@ -344,30 +367,18 @@ where fn eq(&self, data: &T) -> bool { self == data } - - fn into_owned(self) -> T { - self - } } impl Lookup for &T where - T: Clone + Eq + Hash, + T: Clone, { - fn hash(&self, h: &mut H) { - Hash::hash(self, &mut *h); - } - - fn eq(&self, data: &T) -> bool { - *self == data - } - fn into_owned(self) -> T { Clone::clone(self) } } -impl<'a, T> Lookup> for &'a T +impl<'a, T> HashEqLike> for &'a T where T: ?Sized + Hash + Eq, Box: From<&'a T>, @@ -378,48 +389,58 @@ where fn eq(&self, data: &Box) -> bool { **self == **data } +} + +impl<'a, T> Lookup> for &'a T +where + T: ?Sized + Hash + Eq, + Box: From<&'a T>, +{ fn into_owned(self) -> Box { Box::from(self) } } impl Lookup for &str { + fn into_owned(self) -> String { + self.to_owned() + } +} +impl HashEqLike<&str> for String { fn hash(&self, h: &mut H) { Hash::hash(self, &mut *h) } - fn eq(&self, data: &String) -> bool { - self == data - } - - fn into_owned(self) -> String { - self.to_owned() + fn eq(&self, data: &&str) -> bool { + self == *data } } -impl + Clone + Lookup, T> Lookup> for &[A] { +impl> HashEqLike<&[A]> for Vec { fn hash(&self, h: &mut H) { Hash::hash(self, h); } - fn eq(&self, data: &Vec) -> bool { + fn eq(&self, data: &&[A]) -> bool { self.len() == data.len() && data.iter().enumerate().all(|(i, a)| &self[i] == a) } - +} +impl + Clone + Lookup, T> Lookup> for &[A] { fn into_owned(self) -> Vec { self.iter().map(|a| Lookup::into_owned(a.clone())).collect() } } -impl + Clone + Lookup, T> Lookup> for [A; N] { +impl> HashEqLike<[A; N]> for Vec { fn hash(&self, h: &mut H) { Hash::hash(self, h); } - fn eq(&self, data: &Vec) -> bool { + fn eq(&self, data: &[A; N]) -> bool { self.len() == data.len() && data.iter().enumerate().all(|(i, a)| &self[i] == a) } - +} +impl + Clone + Lookup, T> Lookup> for [A; N] { fn into_owned(self) -> Vec { self.into_iter() .map(|a| Lookup::into_owned(a.clone())) @@ -427,15 +448,16 @@ impl + Clone + Lookup, T> Lookup< } } -impl Lookup for &Path { +impl HashEqLike<&Path> for PathBuf { fn hash(&self, h: &mut H) { Hash::hash(self, h); } - fn eq(&self, data: &PathBuf) -> bool { + fn eq(&self, data: &&Path) -> bool { self == data } - +} +impl Lookup for &Path { fn into_owned(self) -> PathBuf { self.to_owned() } diff --git a/src/lib.rs b/src/lib.rs index 8cc739fab..53dddcfd4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -132,6 +132,7 @@ pub mod plumbing { pub mod interned { pub use crate::interned::Configuration; + pub use crate::interned::HashEqLike; pub use crate::interned::IngredientImpl; pub use crate::interned::JarImpl; pub use crate::interned::Lookup; diff --git a/src/table.rs b/src/table.rs index 47296c7b7..233314116 100644 --- a/src/table.rs +++ b/src/table.rs @@ -228,7 +228,7 @@ impl Page { pub(crate) fn allocate(&self, page: PageIndex, value: V) -> Result where - V: FnOnce() -> T, + V: FnOnce(Id) -> T, { let guard = self.allocation_lock.lock(); let index = self.allocated.load(Ordering::Acquire); @@ -237,14 +237,15 @@ impl Page { } // Initialize entry `index` + let id = make_id(page, SlotIndex::new(index)); let data = &self.data[index]; - unsafe { (*data.get()).write(value()) }; + unsafe { (*data.get()).write(value(id)) }; // Update the length (this must be done after initialization!) self.allocated.store(index + 1, Ordering::Release); drop(guard); - Ok(make_id(page, SlotIndex::new(index))) + Ok(id) } } diff --git a/src/tracked_struct.rs b/src/tracked_struct.rs index 97d3f9680..197687082 100644 --- a/src/tracked_struct.rs +++ b/src/tracked_struct.rs @@ -320,7 +320,7 @@ where current_deps: &StampedValue<()>, fields: C::Fields<'db>, ) -> Id { - let value = || Value { + let value = |_| Value { updated_at: AtomicCell::new(Some(current_revision)), durability: current_deps.durability, fields: unsafe { self.to_static(fields) }, @@ -339,7 +339,7 @@ where // Overwrite the free-list entry. Use `*foo = ` because the entry // has been previously initialized and we want to free the old contents. unsafe { - *data_raw = value(); + *data_raw = value(id); } id diff --git a/src/zalsa_local.rs b/src/zalsa_local.rs index 3fb28e7e2..07524de2f 100644 --- a/src/zalsa_local.rs +++ b/src/zalsa_local.rs @@ -58,7 +58,7 @@ impl ZalsaLocal { &self, table: &Table, ingredient: IngredientIndex, - mut value: impl FnOnce() -> T, + mut value: impl FnOnce(Id) -> T, ) -> Id { // Find the most recent page, pushing a page if needed let mut page = *self @@ -71,7 +71,7 @@ impl ZalsaLocal { // Try to allocate an entry on that page let page_ref = table.page::(page); match page_ref.allocate(page, value) { - // If succesful, return + // If succesfull, return Ok(id) => return id, // Otherwise, create a new page and try again diff --git a/tests/interned-structs_self_ref.rs b/tests/interned-structs_self_ref.rs new file mode 100644 index 000000000..269724b8a --- /dev/null +++ b/tests/interned-structs_self_ref.rs @@ -0,0 +1,182 @@ +//! Test that a `tracked` fn on a `salsa::input` +//! compiles and executes successfully. + +use std::convert::identity; + +use test_log::test; + +#[test] +fn interning_returns_equal_keys_for_equal_data() { + let db = salsa::DatabaseImpl::new(); + let s1 = InternedString::new(&db, "Hello, ".to_string(), identity); + let s2 = InternedString::new(&db, "World, ".to_string(), |_| s1); + let s1_2 = InternedString::new(&db, "Hello, ", identity); + let s2_2 = InternedString::new(&db, "World, ", |_| s2); + assert_eq!(s1, s1_2); + assert_eq!(s2, s2_2); +} +// Recursive expansion of interned macro +// #[salsa::interned] +// struct InternedString<'db> { +// data: String, +// other: InternedString<'db>, +// } +// ====================================== + +#[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)] +struct InternedString<'db>( + salsa::Id, + std::marker::PhantomData<&'db salsa::plumbing::interned::Value>>, +); + +#[allow(warnings)] +const _: () = { + use salsa::plumbing as zalsa_; + use zalsa_::interned as zalsa_struct_; + type Configuration_ = InternedString<'static>; + #[derive(Clone)] + struct StructData<'db>(String, InternedString<'db>); + + impl<'db> Eq for StructData<'db> {} + impl<'db> PartialEq for StructData<'db> { + fn eq(&self, other: &Self) -> bool { + self.0 == other.0 + } + } + + impl<'db> std::hash::Hash for StructData<'db> { + fn hash(&self, state: &mut H) { + self.0.hash(state); + } + } + + #[doc = r" Key to use during hash lookups. Each field is some type that implements `Lookup`"] + #[doc = r" for the owned type. This permits interning with an `&str` when a `String` is required and so forth."] + #[derive(Hash)] + struct StructKey<'db, T0>(T0, std::marker::PhantomData<&'db ()>); + + impl<'db, T0> zalsa_::interned::HashEqLike> for StructData<'db> + where + String: zalsa_::interned::HashEqLike, + { + fn hash(&self, h: &mut H) { + zalsa_::interned::HashEqLike::::hash(&self.0, &mut *h); + } + fn eq(&self, data: &StructKey<'db, T0>) -> bool { + (zalsa_::interned::HashEqLike::::eq(&self.0, &data.0) && true) + } + } + impl zalsa_struct_::Configuration for Configuration_ { + const DEBUG_NAME: &'static str = "InternedString"; + type Data<'a> = StructData<'a>; + type Struct<'a> = InternedString<'a>; + fn struct_from_id<'db>(id: salsa::Id) -> Self::Struct<'db> { + InternedString(id, std::marker::PhantomData) + } + fn deref_struct(s: Self::Struct<'_>) -> salsa::Id { + s.0 + } + } + impl Configuration_ { + pub fn ingredient(db: &Db) -> &zalsa_struct_::IngredientImpl + where + Db: ?Sized + zalsa_::Database, + { + static CACHE: zalsa_::IngredientCache> = + zalsa_::IngredientCache::new(); + CACHE.get_or_create(db.as_dyn_database(), || { + db.zalsa() + .add_or_lookup_jar_by_type(&>::default()) + }) + } + } + impl zalsa_::AsId for InternedString<'_> { + fn as_id(&self) -> salsa::Id { + self.0 + } + } + impl zalsa_::FromId for InternedString<'_> { + fn from_id(id: salsa::Id) -> Self { + Self(id, std::marker::PhantomData) + } + } + unsafe impl Send for InternedString<'_> {} + + unsafe impl Sync for InternedString<'_> {} + + impl std::fmt::Debug for InternedString<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + Self::default_debug_fmt(*self, f) + } + } + impl zalsa_::SalsaStructInDb for InternedString<'_> { + fn lookup_ingredient_index( + aux: &dyn zalsa_::JarAux, + ) -> core::option::Option { + aux.lookup_jar_by_type(&>::default()) + } + } + + unsafe impl zalsa_::Update for InternedString<'_> { + unsafe fn maybe_update(old_pointer: *mut Self, new_value: Self) -> bool { + if unsafe { *old_pointer } != new_value { + unsafe { *old_pointer = new_value }; + true + } else { + false + } + } + } + impl<'db> InternedString<'db> { + pub fn new + std::hash::Hash>( + db: &'db Db_, + data: T0, + other: impl FnOnce(InternedString<'db>) -> InternedString<'db>, + ) -> Self + where + Db_: ?Sized + salsa::Database, + String: zalsa_::interned::HashEqLike, + { + let current_revision = zalsa_::current_revision(db); + Configuration_::ingredient(db).intern( + db.as_dyn_database(), + StructKey::<'db>(data, std::marker::PhantomData::default()), + |id, data| { + StructData( + zalsa_::interned::Lookup::into_owned(data.0), + other(zalsa_::FromId::from_id(id)), + ) + }, + ) + } + fn data(self, db: &'db Db_) -> String + where + Db_: ?Sized + zalsa_::Database, + { + let fields = Configuration_::ingredient(db).fields(db.as_dyn_database(), self); + std::clone::Clone::clone((&fields.0)) + } + fn other(self, db: &'db Db_) -> InternedString<'db> + where + Db_: ?Sized + zalsa_::Database, + { + let fields = Configuration_::ingredient(db).fields(db.as_dyn_database(), self); + std::clone::Clone::clone((&fields.1)) + } + #[doc = r" Default debug formatting for this struct (may be useful if you define your own `Debug` impl)"] + pub fn default_debug_fmt(this: Self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + zalsa_::with_attached_database(|db| { + let fields = Configuration_::ingredient(db).fields(db.as_dyn_database(), this); + let mut f = f.debug_struct("InternedString"); + let f = f.field("data", &fields.0); + let f = f.field("other", &fields.1); + f.finish() + }) + .unwrap_or_else(|| { + f.debug_tuple("InternedString") + .field(&zalsa_::AsId::as_id(&this)) + .finish() + }) + } + } +};