From 4657ac3a0dbce59e0277cabb2c7c6bd1a69d8a8f Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sun, 11 Aug 2024 08:04:47 +0300 Subject: [PATCH] Revert "introduce Table and use for interned values" This reverts commit 9a3111ce278dff8da2a1c7102dcdfb2e016b61ae. --- .../src/setup_interned_struct.rs | 18 +-- .../salsa-macro-rules/src/setup_tracked_fn.rs | 14 +- src/interned.rs | 75 +++++++--- src/lib.rs | 1 - src/table.rs | 133 ------------------ src/zalsa.rs | 9 -- src/zalsa_local.rs | 43 ------ 7 files changed, 72 insertions(+), 221 deletions(-) delete mode 100644 src/table.rs diff --git a/components/salsa-macro-rules/src/setup_interned_struct.rs b/components/salsa-macro-rules/src/setup_interned_struct.rs index 4ea3f7fa..1d2c868d 100644 --- a/components/salsa-macro-rules/src/setup_interned_struct.rs +++ b/components/salsa-macro-rules/src/setup_interned_struct.rs @@ -52,7 +52,7 @@ macro_rules! setup_interned_struct { $(#[$attr])* #[derive(Copy, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)] $vis struct $Struct<$db_lt>( - salsa::Id, + std::ptr::NonNull >>, std::marker::PhantomData < & $db_lt salsa::plumbing::interned::Value < $Struct<'static> > > ); @@ -66,11 +66,11 @@ macro_rules! setup_interned_struct { const DEBUG_NAME: &'static str = stringify!($Struct); type Data<$db_lt> = ($($field_ty,)*); type Struct<$db_lt> = $Struct<$db_lt>; - fn struct_from_id<'db>(id: salsa::Id) -> Self::Struct<'db> { - $Struct(id, std::marker::PhantomData) + unsafe fn struct_from_raw<'db>(ptr: std::ptr::NonNull<$zalsa_struct::Value>) -> Self::Struct<'db> { + $Struct(ptr, std::marker::PhantomData) } - fn deref_struct(s: Self::Struct<'_>) -> salsa::Id { - s.0 + fn deref_struct(s: Self::Struct<'_>) -> &$zalsa_struct::Value { + unsafe { s.0.as_ref() } } } @@ -89,13 +89,13 @@ macro_rules! setup_interned_struct { impl $zalsa::AsId for $Struct<'_> { fn as_id(&self) -> salsa::Id { - self.0 + unsafe { self.0.as_ref() }.as_id() } } impl<$db_lt> $zalsa::LookupId<$db_lt> for $Struct<$db_lt> { fn lookup_id(id: salsa::Id, db: &$db_lt dyn $zalsa::Database) -> Self { - Self(id, std::marker::PhantomData) + $Configuration::ingredient(db).interned_value(id) } } @@ -144,7 +144,7 @@ macro_rules! setup_interned_struct { // FIXME(rust-lang/rust#65991): The `db` argument *should* have the type `dyn Database` $Db: ?Sized + $zalsa::Database, { - let fields = $Configuration::ingredient(db).fields(db.as_dyn_database(), self); + let fields = $Configuration::ingredient(db).fields(self); $zalsa::maybe_clone!( $field_option, $field_ty, @@ -156,7 +156,7 @@ macro_rules! setup_interned_struct { /// 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 fields = $Configuration::ingredient(db).fields(this); let mut f = f.debug_struct(stringify!($Struct)); $( let f = f.field(stringify!($field_id), &fields.$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 c96ea786..9a072a93 100644 --- a/components/salsa-macro-rules/src/setup_tracked_fn.rs +++ b/components/salsa-macro-rules/src/setup_tracked_fn.rs @@ -96,7 +96,7 @@ macro_rules! setup_tracked_fn { if $needs_interner { #[derive(Copy, Clone)] struct $InternedData<$db_lt>( - salsa::Id, + std::ptr::NonNull<$zalsa::interned::Value<$Configuration>>, std::marker::PhantomData<&$db_lt $zalsa::interned::Value<$Configuration>>, ); @@ -114,14 +114,14 @@ macro_rules! setup_tracked_fn { type Struct<$db_lt> = $InternedData<$db_lt>; - fn struct_from_id<$db_lt>( - id: salsa::Id, + unsafe fn struct_from_raw<$db_lt>( + ptr: std::ptr::NonNull<$zalsa::interned::Value>, ) -> Self::Struct<$db_lt> { - $InternedData(id, std::marker::PhantomData) + $InternedData(ptr, std::marker::PhantomData) } - fn deref_struct(s: Self::Struct<'_>) -> salsa::Id { - s.0 + fn deref_struct(s: Self::Struct<'_>) -> &$zalsa::interned::Value { + unsafe { s.0.as_ref() } } } } else { @@ -191,7 +191,7 @@ macro_rules! setup_tracked_fn { fn id_to_input<$db_lt>(db: &$db_lt Self::DbView, key: salsa::Id) -> Self::Input<$db_lt> { $zalsa::macro_if! { if $needs_interner { - $Configuration::intern_ingredient(db).data(db, key).clone() + $Configuration::intern_ingredient(db).data(key).clone() } else { $zalsa::LookupId::lookup_id(key, db.as_dyn_database()) } diff --git a/src/interned.rs b/src/interned.rs index c8af68ba..65e56437 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -1,7 +1,10 @@ +use crossbeam::atomic::AtomicCell; use std::fmt; use std::hash::Hash; use std::marker::PhantomData; +use std::ptr::NonNull; +use crate::alloc::Alloc; use crate::durability::Durability; use crate::id::AsId; use crate::ingredient::fmt_index; @@ -24,16 +27,24 @@ pub trait Configuration: Sized + 'static { /// The end user struct type Struct<'db>: Copy; - /// Create an end-user struct from the salsa id + /// Create an end-user struct from the underlying raw pointer. /// /// This call is an "end-step" to the tracked struct lookup/creation /// process in a given revision: it occurs only when the struct is newly /// created or, if a struct is being reused, after we have updated its /// fields (or confirmed it is green and no updates are required). - fn struct_from_id<'db>(id: Id) -> Self::Struct<'db>; + /// + /// # Safety + /// + /// Requires that `ptr` represents a "confirmed" value in this revision, + /// which means that it will remain valid and immutable for the remainder of this + /// revision, represented by the lifetime `'db`. + unsafe fn struct_from_raw<'db>(ptr: NonNull>) -> Self::Struct<'db>; - /// Deref the struct to yield the underlying id. - fn deref_struct(s: Self::Struct<'_>) -> Id; + /// Deref the struct to yield the underlying value struct. + /// Since we are still part of the `'db` lifetime in which the struct was created, + /// this deref is safe, and the value-struct fields are immutable and verified. + fn deref_struct(s: Self::Struct<'_>) -> &Value; } pub trait InternedData: Sized + Eq + Hash + Clone {} @@ -55,6 +66,14 @@ pub struct IngredientImpl { /// Deadlock requirement: We access `value_map` while holding lock on `key_map`, but not vice versa. key_map: FxDashMap, Id>, + /// Maps from an interned id to its data. + /// + /// Deadlock requirement: We access `value_map` while holding lock on `key_map`, but not vice versa. + value_map: FxDashMap>>, + + /// counter for the next id. + counter: AtomicCell, + /// Stores the revision when this interned ingredient was last cleared. /// You can clear an interned table at any point, deleting all its entries, /// but that will make anything dependent on those entries dirty and in need @@ -93,6 +112,8 @@ where Self { ingredient_index, key_map: Default::default(), + value_map: Default::default(), + counter: AtomicCell::default(), reset_at: Revision::start(), } } @@ -101,10 +122,6 @@ where unsafe { std::mem::transmute(data) } } - unsafe fn from_internal_data<'db>(&'db self, data: &C::Data<'static>) -> &C::Data<'db> { - unsafe { std::mem::transmute(data) } - } - pub fn intern_id<'db>( &'db self, db: &'db dyn crate::Database, @@ -132,7 +149,7 @@ where if let Some(guard) = self.key_map.get(&internal_data) { let id = *guard; drop(guard); - return C::struct_from_id(id); + return self.interned_value(id); } match self.key_map.entry(internal_data.clone()) { @@ -140,38 +157,58 @@ where dashmap::mapref::entry::Entry::Occupied(entry) => { let id = *entry.get(); drop(entry); - C::struct_from_id(id) + self.interned_value(id) } // We won any races so should intern the data dashmap::mapref::entry::Entry::Vacant(entry) => { - let zalsa = db.zalsa(); - let table = zalsa.table(); - let next_id = zalsa_local.allocate(table, self.ingredient_index, internal_data); + let next_id = self.counter.fetch_add(1); + let next_id = crate::id::Id::from_u32(next_id); + let value = self.value_map.entry(next_id).or_insert(Alloc::new(Value { + id: next_id, + fields: internal_data, + })); + let value_raw = value.as_raw(); + drop(value); entry.insert(next_id); - C::struct_from_id(next_id) + // SAFETY: Items are only removed from the `value_map` with an `&mut self` reference. + unsafe { C::struct_from_raw(value_raw) } } } } + pub fn interned_value(&self, id: Id) -> C::Struct<'_> { + let r = self.value_map.get(&id).unwrap(); + + // SAFETY: Items are only removed from the `value_map` with an `&mut self` reference. + unsafe { C::struct_from_raw(r.as_raw()) } + } + /// Lookup the data for an interned value based on its id. /// Rarely used since end-users generally carry a struct with a pointer directly /// to the interned item. - pub fn data<'db>(&'db self, db: &'db dyn Database, id: Id) -> &'db C::Data<'db> { - let internal_data = db.zalsa().table().get::>(id); - unsafe { self.from_internal_data(internal_data) } + pub fn data(&self, id: Id) -> &C::Data<'_> { + C::deref_struct(self.interned_value(id)).data() } /// Lookup the fields from an interned struct. /// Note that this is not "leaking" since no dependency edge is required. - pub fn fields<'db>(&'db self, db: &'db dyn Database, s: C::Struct<'db>) -> &'db C::Data<'db> { - self.data(db, C::deref_struct(s)) + pub fn fields<'db>(&'db self, s: C::Struct<'db>) -> &'db C::Data<'db> { + C::deref_struct(s).data() + } + + /// Variant of `data` that takes a (unnecessary) database argument. + /// This exists because tracked functions sometimes use true interning and sometimes use + /// [`IdentityInterner`][], which requires the database argument. + pub fn data_with_db<'db, DB: ?Sized>(&'db self, id: Id, _db: &'db DB) -> &'db C::Data<'db> { + self.data(id) } pub fn reset(&mut self, revision: Revision) { assert!(revision > self.reset_at); self.reset_at = revision; self.key_map.clear(); + self.value_map.clear(); } } diff --git a/src/lib.rs b/src/lib.rs index 03b157a5..b6e82c83 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -22,7 +22,6 @@ mod revision; mod runtime; mod salsa_struct; mod storage; -mod table; mod tracked_struct; mod update; mod views; diff --git a/src/table.rs b/src/table.rs deleted file mode 100644 index 56682fd9..00000000 --- a/src/table.rs +++ /dev/null @@ -1,133 +0,0 @@ -use std::{any::Any, mem::MaybeUninit}; - -use append_only_vec::AppendOnlyVec; -use crossbeam::atomic::AtomicCell; -use parking_lot::Mutex; - -use crate::{Id, IngredientIndex}; - -const PAGE_LEN_BITS: usize = 10; -const PAGE_LEN_MASK: usize = PAGE_LEN - 1; -const PAGE_LEN: usize = 1 << PAGE_LEN_BITS; - -pub struct Table { - pages: AppendOnlyVec>, -} - -pub struct Page { - /// The ingredient for elements on this page. - #[allow(dead_code)] // pretty sure we'll need this eventually - ingredient: IngredientIndex, - - /// Number of elements of `data` that are initialized. - allocated: AtomicCell, - - /// The "allocation lock" is held when we allocate a new entry. - /// - /// It ensures that we can load the index, initialize it, and then update the length atomically - /// with respect to other allocations. - /// - /// We could avoid it if we wanted, we'd just have to be a bit fancier in our reasoning - /// (for example, the bounds check in `Page::get` no longer suffices to truly guarantee - /// that the data is initialized). - allocation_lock: Mutex<()>, - - /// Vector with data. This is always created with the capacity/length of `PAGE_LEN` - /// and uninitialized data. As we initialize new entries, we increment `allocated`. - data: Vec>, -} - -#[derive(Copy, Clone)] -pub struct PageIndex(usize); - -#[derive(Copy, Clone)] -pub struct SlotIndex(usize); - -impl Default for Table { - fn default() -> Self { - Self { - pages: AppendOnlyVec::new(), - } - } -} - -impl Table { - pub fn get(&self, id: Id) -> &T { - let (page, slot) = split_id(id); - let page_ref = self.page::(page); - page_ref.get(slot) - } - - pub fn page(&self, page: PageIndex) -> &Page { - self.pages[page.0].downcast_ref::>().unwrap() - } - - pub fn push_page(&self, ingredient: IngredientIndex) -> PageIndex { - let page = Box::new(>::new(ingredient)); - PageIndex(self.pages.push(page)) - } -} - -impl Page { - fn new(ingredient: IngredientIndex) -> Self { - let mut data = Vec::with_capacity(PAGE_LEN); - unsafe { - data.set_len(PAGE_LEN); - } - Self { - ingredient, - allocated: Default::default(), - allocation_lock: Default::default(), - data, - } - } - - pub(crate) fn get(&self, slot: SlotIndex) -> &T { - let len = self.allocated.load(); - assert!(slot.0 < len); - unsafe { self.data[slot.0].assume_init_ref() } - } - - pub(crate) fn allocate(&self, page: PageIndex, value: T) -> Result { - let guard = self.allocation_lock.lock(); - let index = self.allocated.load(); - if index == PAGE_LEN { - return Err(value); - } - - let data = &self.data[index]; - let data = data.as_ptr() as *mut T; - unsafe { std::ptr::write(data, value) }; - - self.allocated.store(index + 1); - drop(guard); - - Ok(make_id(page, SlotIndex(index))) - } -} - -impl Drop for Page { - fn drop(&mut self) { - let mut data = std::mem::replace(&mut self.data, vec![]); - let len = self.allocated.load(); - unsafe { - data.set_len(len); - drop(std::mem::transmute::>, Vec>(data)); - } - } -} - -fn make_id(page: PageIndex, slot: SlotIndex) -> Id { - assert!(slot.0 < PAGE_LEN); - assert!(page.0 < (1 << (32 - PAGE_LEN_BITS))); - let page = page.0 as u32; - let slot = slot.0 as u32; - Id::from_u32(page << PAGE_LEN_BITS | slot) -} - -fn split_id(id: Id) -> (PageIndex, SlotIndex) { - let id = id.as_u32() as usize; - let slot = id & PAGE_LEN_MASK; - let page = id >> PAGE_LEN_BITS; - (PageIndex(page), SlotIndex(slot)) -} diff --git a/src/zalsa.rs b/src/zalsa.rs index 817bf760..4e3a6f72 100644 --- a/src/zalsa.rs +++ b/src/zalsa.rs @@ -9,7 +9,6 @@ use crate::cycle::CycleRecoveryStrategy; use crate::ingredient::{Ingredient, Jar}; use crate::nonce::{Nonce, NonceGenerator}; use crate::runtime::{Runtime, WaitResult}; -use crate::table::Table; use crate::views::Views; use crate::zalsa_local::ZalsaLocal; use crate::{Database, DatabaseKeyIndex, Durability, Revision}; @@ -110,9 +109,6 @@ pub struct Zalsa { /// The runtime for this particular salsa database handle. /// Each handle gets its own runtime, but the runtimes have shared state between them. runtime: Runtime, - - /// Data for instances - table: Table, } impl Zalsa { @@ -124,7 +120,6 @@ impl Zalsa { ingredients_vec: AppendOnlyVec::new(), ingredients_requiring_reset: AppendOnlyVec::new(), runtime: Runtime::default(), - table: Table::default(), } } @@ -136,10 +131,6 @@ impl Zalsa { self.nonce } - pub(crate) fn table(&self) -> &Table { - &self.table - } - /// **NOT SEMVER STABLE** pub fn add_or_lookup_jar_by_type(&self, jar: &dyn Jar) -> IngredientIndex { { diff --git a/src/zalsa_local.rs b/src/zalsa_local.rs index 725b8f27..19362a97 100644 --- a/src/zalsa_local.rs +++ b/src/zalsa_local.rs @@ -1,4 +1,3 @@ -use rustc_hash::FxHashMap; use tracing::debug; use crate::active_query::ActiveQuery; @@ -6,8 +5,6 @@ use crate::durability::Durability; use crate::key::DatabaseKeyIndex; use crate::key::DependencyIndex; use crate::runtime::StampedValue; -use crate::table::PageIndex; -use crate::table::Table; use crate::tracked_struct::Disambiguator; use crate::zalsa::IngredientIndex; use crate::Cancelled; @@ -15,9 +12,7 @@ use crate::Cycle; use crate::Database; use crate::Event; use crate::EventKind; -use crate::Id; use crate::Revision; -use std::any::Any; use std::cell::RefCell; use std::sync::Arc; @@ -36,50 +31,12 @@ pub struct ZalsaLocal { /// Unwinding note: pushes onto this vector must be popped -- even /// during unwinding. query_stack: RefCell>>, - - /// Stores the most recent page for a given ingredient. - /// This is thread-local to avoid contention. - most_recent_pages: RefCell>, } impl ZalsaLocal { pub(crate) fn new() -> Self { ZalsaLocal { query_stack: RefCell::new(Some(vec![])), - most_recent_pages: RefCell::new(FxHashMap::default()), - } - } - - /// Allocate a new id in `table` for the given ingredient - /// storing `value`. Remembers the most recent page from this - /// thread and attempts to reuse it. - pub(crate) fn allocate<'t, T: Any + Send + Sync>( - &self, - table: &Table, - ingredient: IngredientIndex, - mut value: T, - ) -> Id { - // Find the most recent page, pushing a page if needed - let mut page = *self - .most_recent_pages - .borrow_mut() - .entry(ingredient) - .or_insert_with(|| table.push_page::(ingredient)); - - loop { - // Try to allocate an entry on that page - let page_ref = table.page::(page); - match page_ref.allocate(page, value) { - // If succesful, return - Ok(id) => return id, - - // Otherwise, create a new page and try again - Err(v) => { - value = v; - page = table.push_page::(ingredient); - self.most_recent_pages.borrow_mut().insert(ingredient, page); - } - } } }