diff --git a/src/function/fetch.rs b/src/function/fetch.rs index f3f79bfac..d6de9d9cb 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -25,7 +25,6 @@ where let _span = crate::tracing::debug_span!("fetch", query = ?database_key_index).entered(); let memo = self.refresh_memo(db, zalsa, zalsa_local, id); - // SAFETY: We just refreshed the memo so it is guaranteed to contain a value now. let memo_value = unsafe { memo.value.as_ref().unwrap_unchecked() }; @@ -168,16 +167,13 @@ where } // no provisional value; create/insert/return initial provisional value return match C::CYCLE_STRATEGY { - // SAFETY: We do not access the query stack reentrantly. - CycleRecoveryStrategy::Panic => unsafe { - zalsa_local.with_query_stack_unchecked(|stack| { - panic!( - "dependency graph cycle when querying {database_key_index:#?}, \ + CycleRecoveryStrategy::Panic => zalsa_local.with_query_stack(|stack| { + panic!( + "dependency graph cycle when querying {database_key_index:#?}, \ set cycle_fn/cycle_initial to fixpoint iterate.\n\ Query stack:\n{stack:#?}", - ); - }) - }, + ); + }), CycleRecoveryStrategy::Fixpoint => { crate::tracing::debug!( "hit cycle at {database_key_index:#?}, \ diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index dcc17bb22..20e82d1fa 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -108,16 +108,13 @@ where return None; } ClaimResult::Cycle { .. } => match C::CYCLE_STRATEGY { - // SAFETY: We do not access the query stack reentrantly. - CycleRecoveryStrategy::Panic => unsafe { - db.zalsa_local().with_query_stack_unchecked(|stack| { - panic!( - "dependency graph cycle when validating {database_key_index:#?}, \ + CycleRecoveryStrategy::Panic => db.zalsa_local().with_query_stack(|stack| { + panic!( + "dependency graph cycle when validating {database_key_index:#?}, \ set cycle_fn/cycle_initial to fixpoint iterate.\n\ Query stack:\n{stack:#?}", - ); - }) - }, + ); + }), CycleRecoveryStrategy::FallbackImmediate => { return Some(VerifyResult::unchanged()); } @@ -339,38 +336,32 @@ where return true; } - // SAFETY: We do not access the query stack reentrantly. - unsafe { - zalsa_local.with_query_stack_unchecked(|stack| { - cycle_heads.iter().all(|cycle_head| { - stack - .iter() - .rev() - .find(|query| query.database_key_index == cycle_head.database_key_index) - .map(|query| query.iteration_count()) - .or_else(|| { - // If this is a cycle head is owned by another thread that is blocked by this ingredient, - // check if it has the same iteration count. - let ingredient = zalsa.lookup_ingredient( - cycle_head.database_key_index.ingredient_index(), - ); - let wait_result = ingredient - .wait_for(zalsa, cycle_head.database_key_index.key_index()); - - if !wait_result.is_cycle_with_other_thread() { - return None; - } + zalsa_local.with_query_stack(|stack| { + cycle_heads.iter().all(|cycle_head| { + stack + .iter() + .rev() + .find(|query| query.database_key_index == cycle_head.database_key_index) + .map(|query| query.iteration_count()) + .or_else(|| { + // If this is a cycle head is owned by another thread that is blocked by this ingredient, + // check if it has the same iteration count. + let ingredient = zalsa + .lookup_ingredient(cycle_head.database_key_index.ingredient_index()); + let wait_result = + ingredient.wait_for(zalsa, cycle_head.database_key_index.key_index()); + + if !wait_result.is_cycle_with_other_thread() { + return None; + } - let provisional_status = ingredient.provisional_status( - zalsa, - cycle_head.database_key_index.key_index(), - )?; - provisional_status.iteration() - }) - == Some(cycle_head.iteration_count) - }) + let provisional_status = ingredient + .provisional_status(zalsa, cycle_head.database_key_index.key_index())?; + provisional_status.iteration() + }) + == Some(cycle_head.iteration_count) }) - } + }) } /// VerifyResult::Unchanged if the memo's value and `changed_at` time is up-to-date in the diff --git a/src/function/memo.rs b/src/function/memo.rs index 810e5b268..a478b1d46 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -236,17 +236,14 @@ impl<'db, C: Configuration> Memo<'db, C> { return true; } - // SAFETY: We do not access the query stack reentrantly. - unsafe { - zalsa_local.with_query_stack_unchecked(|stack| { - cycle_heads.iter().all(|cycle_head| { - stack - .iter() - .rev() - .any(|query| query.database_key_index == cycle_head.database_key_index) - }) + zalsa_local.with_query_stack(|stack| { + cycle_heads.iter().all(|cycle_head| { + stack + .iter() + .rev() + .any(|query| query.database_key_index == cycle_head.database_key_index) }) - } + }) } /// Cycle heads that should be propagated to dependent queries. diff --git a/src/zalsa_local.rs b/src/zalsa_local.rs index 8bc49f22b..294ab0843 100644 --- a/src/zalsa_local.rs +++ b/src/zalsa_local.rs @@ -1,4 +1,4 @@ -use std::cell::{RefCell, UnsafeCell}; +use std::cell::RefCell; use std::panic::UnwindSafe; use std::ptr::{self, NonNull}; @@ -32,14 +32,14 @@ pub struct ZalsaLocal { /// Stores the most recent page for a given ingredient. /// This is thread-local to avoid contention. - most_recent_pages: UnsafeCell>, + most_recent_pages: RefCell>, } impl ZalsaLocal { pub(crate) fn new() -> Self { ZalsaLocal { query_stack: RefCell::new(QueryStack::default()), - most_recent_pages: UnsafeCell::new(FxHashMap::default()), + most_recent_pages: RefCell::new(FxHashMap::default()), } } @@ -65,17 +65,16 @@ impl ZalsaLocal { .memo_table_types() .clone() }; - - // SAFETY: `ZalsaLocal` is `!Sync`, and we never expose a reference to this field, - // so we have exclusive access. - let most_recent_pages = unsafe { &mut *self.most_recent_pages.get() }; - // Find the most recent page, pushing a page if needed - let mut page = *most_recent_pages.entry(ingredient).or_insert_with(|| { - zalsa - .table() - .fetch_or_push_page::(ingredient, memo_types) - }); + let mut page = *self + .most_recent_pages + .borrow_mut() + .entry(ingredient) + .or_insert_with(|| { + zalsa + .table() + .fetch_or_push_page::(ingredient, memo_types) + }); loop { // Try to allocate an entry on that page @@ -90,7 +89,7 @@ impl ZalsaLocal { Err(v) => { value = v; page = zalsa.table().push_page::(ingredient, memo_types()); - most_recent_pages.insert(ingredient, page); + self.most_recent_pages.borrow_mut().insert(ingredient, page); } } } @@ -102,76 +101,50 @@ impl ZalsaLocal { database_key_index: DatabaseKeyIndex, iteration_count: IterationCount, ) -> ActiveQueryGuard<'_> { - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.with_query_stack_unchecked_mut(|stack| { - stack.push_new_query(database_key_index, iteration_count); - - ActiveQueryGuard { - local_state: self, - database_key_index, - #[cfg(debug_assertions)] - push_len: stack.len(), - } - }) + let mut query_stack = self.query_stack.borrow_mut(); + query_stack.push_new_query(database_key_index, iteration_count); + ActiveQueryGuard { + local_state: self, + database_key_index, + #[cfg(debug_assertions)] + push_len: query_stack.len(), } } /// Executes a closure within the context of the current active query stacks (mutable). - /// - /// # Safety - /// - /// The closure cannot access the query stack reentrantly, whether mutable or immutable. #[inline(always)] - pub(crate) unsafe fn with_query_stack_unchecked_mut( + pub(crate) fn with_query_stack_mut( &self, - f: impl UnwindSafe + FnOnce(&mut QueryStack) -> R, + c: impl UnwindSafe + FnOnce(&mut QueryStack) -> R, ) -> R { - // SAFETY: The caller guarantees that the query stack will not be accessed reentrantly. - // Additionally, `ZalsaLocal` is `!Sync`, and we never expose a reference to the query - // stack except through this method, so we have exclusive access. - unsafe { f(&mut self.query_stack.try_borrow_mut().unwrap_unchecked()) } + c(&mut self.query_stack.borrow_mut()) } - /// Executes a closure within the context of the current active query stacks. - /// - /// # Safety - /// - /// No mutable references to the query stack can exist while the closure is executed. #[inline(always)] - pub(crate) unsafe fn with_query_stack_unchecked( - &self, - f: impl UnwindSafe + FnOnce(&QueryStack) -> R, - ) -> R { - // SAFETY: The caller guarantees that the query stack will not being accessed mutably. - // Additionally, `ZalsaLocal` is `!Sync`, and we never expose a reference to the query - // stack except through this method, so we have exclusive access. - unsafe { f(&self.query_stack.try_borrow().unwrap_unchecked()) } + pub(crate) fn with_query_stack(&self, c: impl UnwindSafe + FnOnce(&QueryStack) -> R) -> R { + c(&mut self.query_stack.borrow()) } #[inline(always)] pub(crate) fn try_with_query_stack( &self, - f: impl UnwindSafe + FnOnce(&QueryStack) -> R, + c: impl UnwindSafe + FnOnce(&QueryStack) -> R, ) -> Option { self.query_stack .try_borrow() .ok() .as_ref() - .map(|stack| f(stack)) + .map(|stack| c(stack)) } /// Returns the index of the active query along with its *current* durability/changed-at /// information. As the query continues to execute, naturally, that information may change. pub(crate) fn active_query(&self) -> Option<(DatabaseKeyIndex, Stamp)> { - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.with_query_stack_unchecked(|stack| { - stack - .last() - .map(|active_query| (active_query.database_key_index, active_query.stamp())) - }) - } + self.with_query_stack(|stack| { + stack + .last() + .map(|active_query| (active_query.database_key_index, active_query.stamp())) + }) } /// Add an output to the current query's list of dependencies @@ -182,43 +155,34 @@ impl ZalsaLocal { index: IngredientIndex, value: A, ) -> Result<(), ()> { - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.with_query_stack_unchecked_mut(|stack| { - if let Some(top_query) = stack.last_mut() { - top_query.accumulate(index, value); - Ok(()) - } else { - Err(()) - } - }) - } + self.with_query_stack_mut(|stack| { + if let Some(top_query) = stack.last_mut() { + top_query.accumulate(index, value); + Ok(()) + } else { + Err(()) + } + }) } /// Add an output to the current query's list of dependencies pub(crate) fn add_output(&self, entity: DatabaseKeyIndex) { - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.with_query_stack_unchecked_mut(|stack| { - if let Some(top_query) = stack.last_mut() { - top_query.add_output(entity) - } - }) - } + self.with_query_stack_mut(|stack| { + if let Some(top_query) = stack.last_mut() { + top_query.add_output(entity) + } + }) } /// Check whether `entity` is an output of the currently active query (if any) pub(crate) fn is_output_of_active_query(&self, entity: DatabaseKeyIndex) -> bool { - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.with_query_stack_unchecked_mut(|stack| { - if let Some(top_query) = stack.last_mut() { - top_query.is_output(entity) - } else { - false - } - }) - } + self.with_query_stack_mut(|stack| { + if let Some(top_query) = stack.last_mut() { + top_query.is_output(entity) + } else { + false + } + }) } /// Register that currently active query reads the given input @@ -239,21 +203,18 @@ impl ZalsaLocal { changed_at ); - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.with_query_stack_unchecked_mut(|stack| { - if let Some(top_query) = stack.last_mut() { - top_query.add_read( - input, - durability, - changed_at, - has_accumulated, - accumulated_inputs, - cycle_heads, - ); - } - }) - } + self.with_query_stack_mut(|stack| { + if let Some(top_query) = stack.last_mut() { + top_query.add_read( + input, + durability, + changed_at, + has_accumulated, + accumulated_inputs, + cycle_heads, + ); + } + }) } /// Register that currently active query reads the given input @@ -271,14 +232,11 @@ impl ZalsaLocal { changed_at ); - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.with_query_stack_unchecked_mut(|stack| { - if let Some(top_query) = stack.last_mut() { - top_query.add_read_simple(input, durability, changed_at); - } - }) - } + self.with_query_stack_mut(|stack| { + if let Some(top_query) = stack.last_mut() { + top_query.add_read_simple(input, durability, changed_at); + } + }) } /// Register that the current query read an untracked value @@ -288,14 +246,11 @@ impl ZalsaLocal { /// * `current_revision`, the current revision #[inline(always)] pub(crate) fn report_untracked_read(&self, current_revision: Revision) { - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.with_query_stack_unchecked_mut(|stack| { - if let Some(top_query) = stack.last_mut() { - top_query.add_untracked_read(current_revision); - } - }) - } + self.with_query_stack_mut(|stack| { + if let Some(top_query) = stack.last_mut() { + top_query.add_untracked_read(current_revision); + } + }) } /// Update the top query on the stack to act as though it read a value @@ -303,14 +258,11 @@ impl ZalsaLocal { // FIXME: Use or remove this. #[allow(dead_code)] pub(crate) fn report_synthetic_read(&self, durability: Durability, revision: Revision) { - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.with_query_stack_unchecked_mut(|stack| { - if let Some(top_query) = stack.last_mut() { - top_query.add_synthetic_read(durability, revision); - } - }) - } + self.with_query_stack_mut(|stack| { + if let Some(top_query) = stack.last_mut() { + top_query.add_synthetic_read(durability, revision); + } + }) } /// Called when the active queries creates an index from the @@ -325,42 +277,33 @@ impl ZalsaLocal { /// * the disambiguator index #[track_caller] pub(crate) fn disambiguate(&self, key: IdentityHash) -> (Stamp, Disambiguator) { - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.with_query_stack_unchecked_mut(|stack| { - let top_query = stack.last_mut().expect( - "cannot create a tracked struct disambiguator outside of a tracked function", - ); - let disambiguator = top_query.disambiguate(key); - (top_query.stamp(), disambiguator) - }) - } + self.with_query_stack_mut(|stack| { + let top_query = stack.last_mut().expect( + "cannot create a tracked struct disambiguator outside of a tracked function", + ); + let disambiguator = top_query.disambiguate(key); + (top_query.stamp(), disambiguator) + }) } #[track_caller] pub(crate) fn tracked_struct_id(&self, identity: &Identity) -> Option { - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.with_query_stack_unchecked(|stack| { - let top_query = stack - .last() - .expect("cannot create a tracked struct ID outside of a tracked function"); - top_query.tracked_struct_ids().get(identity) - }) - } + self.with_query_stack(|stack| { + let top_query = stack + .last() + .expect("cannot create a tracked struct ID outside of a tracked function"); + top_query.tracked_struct_ids().get(identity) + }) } #[track_caller] pub(crate) fn store_tracked_struct_id(&self, identity: Identity, id: Id) { - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.with_query_stack_unchecked_mut(|stack| { - let top_query = stack - .last_mut() - .expect("cannot store a tracked struct ID outside of a tracked function"); - top_query.tracked_struct_ids_mut().insert(identity, id); - }) - } + self.with_query_stack_mut(|stack| { + let top_query = stack + .last_mut() + .expect("cannot store a tracked struct ID outside of a tracked function"); + top_query.tracked_struct_ids_mut().insert(identity, id); + }) } #[cold] @@ -979,18 +922,15 @@ pub(crate) struct ActiveQueryGuard<'me> { impl ActiveQueryGuard<'_> { /// Initialize the tracked struct ids with the values from the prior execution. pub(crate) fn seed_tracked_struct_ids(&self, tracked_struct_ids: &[(Identity, Id)]) { - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.local_state.with_query_stack_unchecked_mut(|stack| { - #[cfg(debug_assertions)] - assert_eq!(stack.len(), self.push_len); - let frame = stack.last_mut().unwrap(); - assert!(frame.tracked_struct_ids().is_empty()); - frame - .tracked_struct_ids_mut() - .clone_from_slice(tracked_struct_ids); - }) - } + self.local_state.with_query_stack_mut(|stack| { + #[cfg(debug_assertions)] + assert_eq!(stack.len(), self.push_len); + let frame = stack.last_mut().unwrap(); + assert!(frame.tracked_struct_ids().is_empty()); + frame + .tracked_struct_ids_mut() + .clone_from_slice(tracked_struct_ids); + }) } /// Append the given `outputs` to the query's output list. @@ -1003,29 +943,23 @@ impl ActiveQueryGuard<'_> { QueryOriginRef::DerivedUntracked(_) ); - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.local_state.with_query_stack_unchecked_mut(|stack| { - #[cfg(debug_assertions)] - assert_eq!(stack.len(), self.push_len); - let frame = stack.last_mut().unwrap(); - frame.seed_iteration(durability, changed_at, edges, untracked_read); - }) - } + self.local_state.with_query_stack_mut(|stack| { + #[cfg(debug_assertions)] + assert_eq!(stack.len(), self.push_len); + let frame = stack.last_mut().unwrap(); + frame.seed_iteration(durability, changed_at, edges, untracked_read); + }) } /// Invoked when the query has successfully completed execution. fn complete(self) -> QueryRevisions { - // SAFETY: We do not access the query stack reentrantly. - let query = unsafe { - self.local_state.with_query_stack_unchecked_mut(|stack| { - stack.pop_into_revisions( - self.database_key_index, - #[cfg(debug_assertions)] - self.push_len, - ) - }) - }; + let query = self.local_state.with_query_stack_mut(|stack| { + stack.pop_into_revisions( + self.database_key_index, + #[cfg(debug_assertions)] + self.push_len, + ) + }); std::mem::forget(self); query } @@ -1041,15 +975,12 @@ impl ActiveQueryGuard<'_> { impl Drop for ActiveQueryGuard<'_> { fn drop(&mut self) { - // SAFETY: We do not access the query stack reentrantly. - unsafe { - self.local_state.with_query_stack_unchecked_mut(|stack| { - stack.pop( - self.database_key_index, - #[cfg(debug_assertions)] - self.push_len, - ); - }) - }; + self.local_state.with_query_stack_mut(|stack| { + stack.pop( + self.database_key_index, + #[cfg(debug_assertions)] + self.push_len, + ); + }); } }