From f7a14f23097eef11725d623eca0dc58518f38817 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 30 Jun 2020 15:56:14 +0000 Subject: [PATCH] use `DatabaseKeyIndex` instead of `Dependency` --- .../salsa-macros/src/database_storage.rs | 25 +- components/salsa-macros/src/query_group.rs | 39 +- src/dependency.rs | 61 --- src/derived.rs | 17 +- src/derived/slot.rs | 387 +++++++++--------- src/input.rs | 58 +-- src/interned.rs | 61 +-- src/lib.rs | 23 +- src/plumbing.rs | 9 +- src/runtime.rs | 17 +- src/runtime/local_state.rs | 7 +- 11 files changed, 367 insertions(+), 337 deletions(-) delete mode 100644 src/dependency.rs diff --git a/components/salsa-macros/src/database_storage.rs b/components/salsa-macros/src/database_storage.rs index 65f9ad7d..36c9afdd 100644 --- a/components/salsa-macros/src/database_storage.rs +++ b/components/salsa-macros/src/database_storage.rs @@ -151,10 +151,20 @@ pub(crate) fn database(args: TokenStream, input: TokenStream) -> TokenStream { // ANCHOR_END:DatabaseStorageTypes // ANCHOR:DatabaseOps + let mut maybe_changed_ops = proc_macro2::TokenStream::new(); let mut for_each_ops = proc_macro2::TokenStream::new(); - for (QueryGroup { group_path }, group_storage) in - query_groups.iter().zip(&query_group_storage_names) + for ((QueryGroup { group_path }, group_storage), group_index) in query_groups + .iter() + .zip(&query_group_storage_names) + .zip(0_u16..) { + maybe_changed_ops.extend(quote! { + #group_index => { + let storage: &#group_storage = + >::group_storage(self); + storage.maybe_changed_since(self, input, revision) + } + }); for_each_ops.extend(quote! { let storage: &#group_storage = >::group_storage(self); @@ -163,6 +173,17 @@ pub(crate) fn database(args: TokenStream, input: TokenStream) -> TokenStream { } output.extend(quote! { impl salsa::plumbing::DatabaseOps for #database_name { + fn maybe_changed_since( + &self, + input: salsa::DatabaseKeyIndex, + revision: salsa::Revision + ) -> bool { + match input.group_index() { + #maybe_changed_ops + i => panic!("salsa: invalid group index {}", i) + } + } + fn for_each_query( &self, mut op: impl FnMut(&dyn salsa::plumbing::QueryStorageMassOps), diff --git a/components/salsa-macros/src/query_group.rs b/components/salsa-macros/src/query_group.rs index 2e98d61b..b1c965b0 100644 --- a/components/salsa-macros/src/query_group.rs +++ b/components/salsa-macros/src/query_group.rs @@ -349,8 +349,15 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream } }); + let non_transparent_queries = || { + queries.iter().filter(|q| match q.storage { + QueryStorage::Transparent => false, + _ => true, + }) + }; + // Emit the query types. - for (query, query_index) in queries.iter().zip(0_u16..) { + for (query, query_index) in non_transparent_queries().zip(0_u16..) { let fn_name = &query.fn_name; let qt = &query.query_type; @@ -364,7 +371,7 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream QueryStorage::InternedLookup { intern_query_type } => { quote!(salsa::plumbing::LookupInternedStorage<#db, Self, #intern_query_type>) } - QueryStorage::Transparent => continue, + QueryStorage::Transparent => panic!("should have been filtered"), }; let keys = &query.keys; let value = &query.value; @@ -458,11 +465,19 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream } }); + let mut maybe_changed_ops = proc_macro2::TokenStream::new(); + for (Query { fn_name, .. }, query_index) in non_transparent_queries().zip(0_u16..) { + maybe_changed_ops.extend(quote! { + #query_index => { + salsa::plumbing::QueryStorageOps::maybe_changed_since( + &*self.#fn_name, db, input, revision + ) + } + }); + } + let mut for_each_ops = proc_macro2::TokenStream::new(); - for Query { fn_name, .. } in queries - .iter() - .filter(|q| q.storage != QueryStorage::Transparent) - { + for Query { fn_name, .. } in non_transparent_queries() { for_each_ops.extend(quote! { op(&*self.#fn_name); }); @@ -500,6 +515,18 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream DB__: #trait_name + #requires, DB__: salsa::plumbing::HasQueryGroup<#group_struct>, { + #trait_vis fn maybe_changed_since( + &self, + db: &DB__, + input: salsa::DatabaseKeyIndex, + revision: salsa::Revision, + ) -> bool { + match input.query_index() { + #maybe_changed_ops + i => panic!("salsa: impossible query index {}", i), + } + } + #trait_vis fn for_each_query( &self, db: &DB__, diff --git a/src/dependency.rs b/src/dependency.rs deleted file mode 100644 index ab35e06a..00000000 --- a/src/dependency.rs +++ /dev/null @@ -1,61 +0,0 @@ -use crate::revision::Revision; -use crate::Database; -use std::fmt::Debug; -use std::hash::Hasher; -use std::ptr; -use std::sync::Arc; - -/// Unsafe proof obligations: -/// -/// - If `DB::DatabaseData: Send + Sync`, then `Self: Send + Sync` -/// - If `DB: 'static` and `DB::DatabaseData: 'static`, then `Self: 'static` -pub(crate) unsafe trait DatabaseSlot: Debug { - /// Returns true if the value of this query may have changed since - /// the given revision. - fn maybe_changed_since(&self, db: &DB, revision: Revision) -> bool; -} - -pub(crate) struct Dependency { - slot: Arc + Send + Sync>, - phantom: std::marker::PhantomData>, -} - -impl Dependency { - pub(crate) fn new(slot: Arc + '_>) -> Self { - // Unsafety note: It is safe to 'pretend' the trait object is - // Send+Sync+'static because the phantom-data will reflect the - // reality. - let slot: Arc + Send + Sync> = unsafe { std::mem::transmute(slot) }; - Self { - slot, - phantom: std::marker::PhantomData, - } - } - - pub(crate) fn maybe_changed_since(&self, db: &DB, revision: Revision) -> bool { - self.slot.maybe_changed_since(db, revision) - } -} - -impl std::hash::Hash for Dependency { - fn hash(&self, state: &mut H) - where - H: Hasher, - { - ptr::hash(&*self.slot, state) - } -} - -impl std::cmp::PartialEq for Dependency { - fn eq(&self, other: &Self) -> bool { - ptr::eq(&*self.slot, &*other.slot) - } -} - -impl std::cmp::Eq for Dependency {} - -impl std::fmt::Debug for Dependency { - fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.slot.fmt(fmt) - } -} diff --git a/src/derived.rs b/src/derived.rs index 9eafe9c7..540638b3 100644 --- a/src/derived.rs +++ b/src/derived.rs @@ -8,7 +8,7 @@ use crate::plumbing::QueryFunction; use crate::plumbing::QueryStorageMassOps; use crate::plumbing::QueryStorageOps; use crate::runtime::{FxIndexMap, StampedValue}; -use crate::{CycleError, Database, DatabaseKeyIndex, SweepStrategy}; +use crate::{CycleError, Database, DatabaseKeyIndex, Revision, SweepStrategy}; use parking_lot::RwLock; use std::convert::TryFrom; use std::marker::PhantomData; @@ -132,6 +132,19 @@ where } } + fn maybe_changed_since(&self, db: &DB, input: DatabaseKeyIndex, revision: Revision) -> bool { + assert_eq!(input.group_index, self.group_index); + assert_eq!(input.query_index, Q::QUERY_INDEX); + let slot = self + .slot_map + .read() + .get_index(input.key_index as usize) + .unwrap() + .1 + .clone(); + slot.maybe_changed_since(db, revision) + } + fn try_fetch(&self, db: &DB, key: &Q::Key) -> Result> { let slot = self.slot(key); let StampedValue { @@ -145,7 +158,7 @@ where } db.salsa_runtime() - .report_query_read(slot, durability, changed_at); + .report_query_read(slot.database_key_index(), durability, changed_at); Ok(value) } diff --git a/src/derived/slot.rs b/src/derived/slot.rs index 402055de..d1634cff 100644 --- a/src/derived/slot.rs +++ b/src/derived/slot.rs @@ -1,7 +1,5 @@ use crate::blocking_future::{BlockingFuture, Promise}; use crate::debug::TableEntry; -use crate::dependency::DatabaseSlot; -use crate::dependency::Dependency; use crate::derived::MemoizationPolicy; use crate::durability::Durability; use crate::lru::LruIndex; @@ -84,14 +82,14 @@ where durability: Durability, /// The inputs that went into our query, if we are tracking them. - inputs: MemoInputs, + inputs: MemoInputs, } /// An insertion-order-preserving set of queries. Used to track the /// inputs accessed during query execution. -pub(super) enum MemoInputs { +pub(super) enum MemoInputs { /// Non-empty set of inputs, fully known - Tracked { inputs: Arc<[Dependency]> }, + Tracked { inputs: Arc<[DatabaseKeyIndex]> }, /// Empty set of inputs, fully known. NoInputs, @@ -122,6 +120,10 @@ where } } + pub(super) fn database_key_index(&self) -> DatabaseKeyIndex { + self.database_key_index + } + pub(super) fn database_key(&self, db: &DB) -> DB::DatabaseKey { >::database_key(db, self.key.clone()) } @@ -534,6 +536,186 @@ where } } + pub(super) fn maybe_changed_since(&self, db: &DB, revision: Revision) -> bool { + let runtime = db.salsa_runtime(); + let revision_now = runtime.current_revision(); + + debug!( + "maybe_changed_since({:?}) called with revision={:?}, revision_now={:?}", + self, revision, revision_now, + ); + + // Acquire read lock to start. In some of the arms below, we + // drop this explicitly. + let state = self.state.read(); + + // Look for a memoized value. + let memo = match &*state { + // If somebody depends on us, but we have no map + // entry, that must mean that it was found to be out + // of date and removed. + QueryState::NotComputed => { + debug!("maybe_changed_since({:?}: no value", self); + return true; + } + + // This value is being actively recomputed. Wait for + // that thread to finish (assuming it's not dependent + // on us...) and check its associated revision. + QueryState::InProgress { id, waiting } => { + let other_id = *id; + debug!( + "maybe_changed_since({:?}: blocking on thread `{:?}`", + self, other_id, + ); + match self.register_with_in_progress_thread(db, runtime, other_id, waiting) { + Ok(future) => { + // Release our lock on `self.state`, so other thread can complete. + std::mem::drop(state); + + let result = future.wait().unwrap_or_else(|| db.on_propagated_panic()); + return !result.cycle.is_empty() || result.value.changed_at > revision; + } + + // Consider a cycle to have changed. + Err(_) => return true, + } + } + + QueryState::Memoized(memo) => memo, + }; + + if memo.verified_at == revision_now { + debug!( + "maybe_changed_since({:?}: {:?} since up-to-date memo that changed at {:?}", + self, + memo.changed_at > revision, + memo.changed_at, + ); + return memo.changed_at > revision; + } + + let maybe_changed; + + // If we only depended on constants, and no constant has been + // modified since then, we cannot have changed; no need to + // trace our inputs. + if memo.check_durability(db) { + std::mem::drop(state); + maybe_changed = false; + } else { + match &memo.inputs { + MemoInputs::Untracked => { + // we don't know the full set of + // inputs, so if there is a new + // revision, we must assume it is + // dirty + debug!( + "maybe_changed_since({:?}: true since untracked inputs", + self, + ); + return true; + } + + MemoInputs::NoInputs => { + std::mem::drop(state); + maybe_changed = false; + } + + MemoInputs::Tracked { inputs } => { + // At this point, the value may be dirty (we have + // to check the database-keys). If we have a cached + // value, we'll just fall back to invoking `read`, + // which will do that checking (and a bit more) -- + // note that we skip the "pure read" part as we + // already know the result. + assert!(inputs.len() > 0); + if memo.value.is_some() { + std::mem::drop(state); + return match self.read_upgrade(db, revision_now) { + Ok(v) => { + debug!( + "maybe_changed_since({:?}: {:?} since (recomputed) value changed at {:?}", + self, + v.changed_at > revision, + v.changed_at, + ); + v.changed_at > revision + } + Err(_) => true, + }; + } + + // We have a **tracked set of inputs** that need to be validated. + let inputs = inputs.clone(); + // We'll need to update the state anyway (see below), so release the read-lock. + std::mem::drop(state); + + // Iterate the inputs and see if any have maybe changed. + maybe_changed = inputs + .iter() + .filter(|&&input| db.maybe_changed_since(input, revision)) + .inspect(|input| debug!("{:?}: input `{:?}` may have changed", self, input)) + .next() + .is_some(); + } + } + } + + // Either way, we have to update our entry. + // + // Keep in mind, though, that we released the lock before checking the ipnuts and a lot + // could have happened in the interim. =) Therefore, we have to probe the current + // `self.state` again and in some cases we ought to do nothing. + { + let mut state = self.state.write(); + match &mut *state { + QueryState::Memoized(memo) => { + if memo.verified_at == revision_now { + // Since we started verifying inputs, somebody + // else has come along and updated this value + // (they may even have recomputed + // it). Therefore, we should not touch this + // memo. + // + // FIXME: Should we still return whatever + // `maybe_changed` value we computed, + // however..? It seems .. harmless to indicate + // that the value has changed, but possibly + // less efficient? (It may cause some + // downstream value to be recomputed that + // wouldn't otherwise have to be?) + } else if maybe_changed { + // We found this entry is out of date and + // nobody touch it in the meantime. Just + // remove it. + *state = QueryState::NotComputed; + } else { + // We found this entry is valid. Update the + // `verified_at` to reflect the current + // revision. + memo.verified_at = revision_now; + } + } + + QueryState::InProgress { .. } => { + // Since we started verifying inputs, somebody + // else has come along and started updated this + // value. Just leave their marker alone and return + // whatever `maybe_changed` value we computed. + } + + QueryState::NotComputed => { + // Since we started verifying inputs, somebody + // else has come along and removed this value. The + // GC can do this, for example. That's fine. + } + } + } + + maybe_changed + } + /// Helper: /// /// When we encounter an `InProgress` indicator, we need to either @@ -767,7 +949,7 @@ where MemoInputs::Tracked { inputs } => { let changed_input = inputs .iter() - .filter(|input| input.maybe_changed_since(db, verified_at)) + .filter(|&&input| db.maybe_changed_since(input, verified_at)) .next(); if let Some(input) = changed_input { @@ -818,7 +1000,7 @@ where } } -impl std::fmt::Debug for MemoInputs { +impl std::fmt::Debug for MemoInputs { fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { MemoInputs::Tracked { inputs } => { @@ -841,197 +1023,6 @@ where } } -// The unsafe obligation here is for us to assert that `Slot` is `Send + Sync + 'static`, assuming `Q::Key` and `Q::Value` -// are. We assert this with the `check_send_sync` and `check_static` -// functions below. -unsafe impl DatabaseSlot for Slot -where - Q: QueryFunction, - DB: Database + HasQueryGroup, - MP: MemoizationPolicy, -{ - fn maybe_changed_since(&self, db: &DB, revision: Revision) -> bool { - let runtime = db.salsa_runtime(); - let revision_now = runtime.current_revision(); - - debug!( - "maybe_changed_since({:?}) called with revision={:?}, revision_now={:?}", - self, revision, revision_now, - ); - - // Acquire read lock to start. In some of the arms below, we - // drop this explicitly. - let state = self.state.read(); - - // Look for a memoized value. - let memo = match &*state { - // If somebody depends on us, but we have no map - // entry, that must mean that it was found to be out - // of date and removed. - QueryState::NotComputed => { - debug!("maybe_changed_since({:?}: no value", self); - return true; - } - - // This value is being actively recomputed. Wait for - // that thread to finish (assuming it's not dependent - // on us...) and check its associated revision. - QueryState::InProgress { id, waiting } => { - let other_id = *id; - debug!( - "maybe_changed_since({:?}: blocking on thread `{:?}`", - self, other_id, - ); - match self.register_with_in_progress_thread(db, runtime, other_id, waiting) { - Ok(future) => { - // Release our lock on `self.state`, so other thread can complete. - std::mem::drop(state); - - let result = future.wait().unwrap_or_else(|| db.on_propagated_panic()); - return !result.cycle.is_empty() || result.value.changed_at > revision; - } - - // Consider a cycle to have changed. - Err(_) => return true, - } - } - - QueryState::Memoized(memo) => memo, - }; - - if memo.verified_at == revision_now { - debug!( - "maybe_changed_since({:?}: {:?} since up-to-date memo that changed at {:?}", - self, - memo.changed_at > revision, - memo.changed_at, - ); - return memo.changed_at > revision; - } - - let maybe_changed; - - // If we only depended on constants, and no constant has been - // modified since then, we cannot have changed; no need to - // trace our inputs. - if memo.check_durability(db) { - std::mem::drop(state); - maybe_changed = false; - } else { - match &memo.inputs { - MemoInputs::Untracked => { - // we don't know the full set of - // inputs, so if there is a new - // revision, we must assume it is - // dirty - debug!( - "maybe_changed_since({:?}: true since untracked inputs", - self, - ); - return true; - } - - MemoInputs::NoInputs => { - std::mem::drop(state); - maybe_changed = false; - } - - MemoInputs::Tracked { inputs } => { - // At this point, the value may be dirty (we have - // to check the database-keys). If we have a cached - // value, we'll just fall back to invoking `read`, - // which will do that checking (and a bit more) -- - // note that we skip the "pure read" part as we - // already know the result. - assert!(inputs.len() > 0); - if memo.value.is_some() { - std::mem::drop(state); - return match self.read_upgrade(db, revision_now) { - Ok(v) => { - debug!( - "maybe_changed_since({:?}: {:?} since (recomputed) value changed at {:?}", - self, - v.changed_at > revision, - v.changed_at, - ); - v.changed_at > revision - } - Err(_) => true, - }; - } - - // We have a **tracked set of inputs** that need to be validated. - let inputs = inputs.clone(); - // We'll need to update the state anyway (see below), so release the read-lock. - std::mem::drop(state); - - // Iterate the inputs and see if any have maybe changed. - maybe_changed = inputs - .iter() - .filter(|input| input.maybe_changed_since(db, revision)) - .inspect(|input| debug!("{:?}: input `{:?}` may have changed", self, input)) - .next() - .is_some(); - } - } - } - - // Either way, we have to update our entry. - // - // Keep in mind, though, that we released the lock before checking the ipnuts and a lot - // could have happened in the interim. =) Therefore, we have to probe the current - // `self.state` again and in some cases we ought to do nothing. - { - let mut state = self.state.write(); - match &mut *state { - QueryState::Memoized(memo) => { - if memo.verified_at == revision_now { - // Since we started verifying inputs, somebody - // else has come along and updated this value - // (they may even have recomputed - // it). Therefore, we should not touch this - // memo. - // - // FIXME: Should we still return whatever - // `maybe_changed` value we computed, - // however..? It seems .. harmless to indicate - // that the value has changed, but possibly - // less efficient? (It may cause some - // downstream value to be recomputed that - // wouldn't otherwise have to be?) - } else if maybe_changed { - // We found this entry is out of date and - // nobody touch it in the meantime. Just - // remove it. - *state = QueryState::NotComputed; - } else { - // We found this entry is valid. Update the - // `verified_at` to reflect the current - // revision. - memo.verified_at = revision_now; - } - } - - QueryState::InProgress { .. } => { - // Since we started verifying inputs, somebody - // else has come along and started updated this - // value. Just leave their marker alone and return - // whatever `maybe_changed` value we computed. - } - - QueryState::NotComputed => { - // Since we started verifying inputs, somebody - // else has come along and removed this value. The - // GC can do this, for example. That's fine. - } - } - } - - maybe_changed - } -} - /// Check that `Slot: Send + Sync` as long as /// `DB::DatabaseData: Send + Sync`, which in turn implies that /// `Q::Key: Send + Sync`, `Q::Value: Send + Sync`. diff --git a/src/input.rs b/src/input.rs index cfcea9a4..f16e9f43 100644 --- a/src/input.rs +++ b/src/input.rs @@ -1,5 +1,4 @@ use crate::debug::TableEntry; -use crate::dependency::DatabaseSlot; use crate::durability::Durability; use crate::plumbing::InputQueryStorageOps; use crate::plumbing::QueryStorageMassOps; @@ -71,6 +70,19 @@ where } } + fn maybe_changed_since(&self, db: &DB, input: DatabaseKeyIndex, revision: Revision) -> bool { + assert_eq!(input.group_index, self.group_index); + assert_eq!(input.query_index, Q::QUERY_INDEX); + let slot = self + .slots + .read() + .get_index(input.key_index as usize) + .unwrap() + .1 + .clone(); + slot.maybe_changed_since(db, revision) + } + fn try_fetch(&self, db: &DB, key: &Q::Key) -> Result> { let slot = self .slot(key) @@ -83,7 +95,7 @@ where } = slot.stamped_value.read().clone(); db.salsa_runtime() - .report_query_read(slot, durability, changed_at); + .report_query_read(slot.database_key_index, durability, changed_at); Ok(value) } @@ -112,6 +124,25 @@ where } } +impl Slot +where + Q: Query, + DB: Database, +{ + fn maybe_changed_since(&self, _db: &DB, revision: Revision) -> bool { + debug!( + "maybe_changed_since(slot={:?}, revision={:?})", + self, revision, + ); + + let changed_at = self.stamped_value.read().changed_at; + + debug!("maybe_changed_since: changed_at = {:?}", changed_at); + + changed_at > revision + } +} + impl QueryStorageMassOps for InputStorage where Q: Query, @@ -201,29 +232,6 @@ where } } -// Unsafe proof obligation: `Slot` is Send + Sync if the query -// key/value is Send + Sync (also, that we introduce no -// references). These are tested by the `check_send_sync` and -// `check_static` helpers below. -unsafe impl DatabaseSlot for Slot -where - Q: Query, - DB: Database, -{ - fn maybe_changed_since(&self, _db: &DB, revision: Revision) -> bool { - debug!( - "maybe_changed_since(slot={:?}, revision={:?})", - self, revision, - ); - - let changed_at = self.stamped_value.read().changed_at; - - debug!("maybe_changed_since: changed_at = {:?}", changed_at); - - changed_at > revision - } -} - /// Check that `Slot: Send + Sync` as long as /// `DB::DatabaseData: Send + Sync`, which in turn implies that /// `Q::Key: Send + Sync`, `Q::Value: Send + Sync`. diff --git a/src/interned.rs b/src/interned.rs index 73a48130..6075ac5e 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -1,5 +1,4 @@ use crate::debug::TableEntry; -use crate::dependency::DatabaseSlot; use crate::durability::Durability; use crate::intern_id::InternId; use crate::plumbing::HasQueryGroup; @@ -301,12 +300,23 @@ where } } + fn maybe_changed_since(&self, db: &DB, input: DatabaseKeyIndex, revision: Revision) -> bool { + assert_eq!(input.group_index, self.group_index); + assert_eq!(input.query_index, Q::QUERY_INDEX); + let intern_id = InternId::from(input.key_index); + let slot = self.lookup_value(db, intern_id); + slot.maybe_changed_since(db, revision) + } + fn try_fetch(&self, db: &DB, key: &Q::Key) -> Result> { let slot = self.intern_index(db, key); let changed_at = slot.interned_at; let index = slot.index; - db.salsa_runtime() - .report_query_read(slot, INTERN_DURABILITY, changed_at); + db.salsa_runtime().report_query_read( + slot.database_key_index, + INTERN_DURABILITY, + changed_at, + ); Ok(::from_intern_id(index)) } @@ -406,6 +416,12 @@ where } } + fn maybe_changed_since(&self, db: &DB, input: DatabaseKeyIndex, revision: Revision) -> bool { + let group_storage = >::group_storage(db); + let interned_storage = IQ::query_storage(group_storage); + interned_storage.maybe_changed_since(db, input, revision) + } + fn try_fetch(&self, db: &DB, key: &Q::Key) -> Result> { let index = key.as_intern_id(); let group_storage = >::group_storage(db); @@ -413,8 +429,11 @@ where let slot = interned_storage.lookup_value(db, index); let value = slot.value.clone(); let interned_at = slot.interned_at; - db.salsa_runtime() - .report_query_read(slot, INTERN_DURABILITY, interned_at); + db.salsa_runtime().report_query_read( + slot.database_key_index, + INTERN_DURABILITY, + interned_at, + ); Ok(value) } @@ -458,6 +477,17 @@ where } impl Slot { + fn maybe_changed_since(&self, db: &DB, revision: Revision) -> bool { + let revision_now = db.salsa_runtime().current_revision(); + if !self.try_update_accessed_at(revision_now) { + // if we failed to update accessed-at, then this slot was garbage collected + true + } else { + // otherwise, compare the interning with revision + self.interned_at > revision + } + } + /// Updates the `accessed_at` time to be `revision_now` (if /// necessary). Returns true if the update was successful, or /// false if the slot has been GC'd in the interim. @@ -510,27 +540,6 @@ impl Slot { } } -// Unsafe proof obligation: `Slot` is Send + Sync if the query -// key/value is Send + Sync (also, that we introduce no -// references). These are tested by the `check_send_sync` and -// `check_static` helpers below. -unsafe impl DatabaseSlot for Slot -where - DB: Database, - K: Debug, -{ - fn maybe_changed_since(&self, db: &DB, revision: Revision) -> bool { - let revision_now = db.salsa_runtime().current_revision(); - if !self.try_update_accessed_at(revision_now) { - // if we failed to update accessed-at, then this slot was garbage collected - true - } else { - // otherwise, compare the interning with revision - self.interned_at > revision - } - } -} - /// Check that `Slot: Send + Sync` as long as /// `DB::DatabaseData: Send + Sync`, which in turn implies that /// `Q::Key: Send + Sync`, `Q::Value: Send + Sync`. diff --git a/src/lib.rs b/src/lib.rs index c22de53f..52de3e29 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -7,7 +7,6 @@ //! re-execute the derived queries and it will try to re-use results //! from previous invocations as appropriate. -mod dependency; mod derived; mod doctest; mod durability; @@ -30,7 +29,7 @@ use crate::plumbing::InputQueryStorageOps; use crate::plumbing::LruQueryStorageOps; use crate::plumbing::QueryStorageMassOps; use crate::plumbing::QueryStorageOps; -use crate::revision::Revision; +pub use crate::revision::Revision; use std::fmt::{self, Debug}; use std::hash::Hash; use std::sync::Arc; @@ -421,6 +420,26 @@ pub struct DatabaseKeyIndex { key_index: u32, } +impl DatabaseKeyIndex { + /// Returns the index of the query group containing this key. + #[inline] + pub fn group_index(self) -> u16 { + self.group_index + } + + /// Returns the index of the query within its query group. + #[inline] + pub fn query_index(self) -> u16 { + self.query_index + } + + /// Returns the index of this particular query key within the query. + #[inline] + pub fn key_index(self) -> u32 { + self.key_index + } +} + /// Trait implements by all of the "special types" associated with /// each of your queries. /// diff --git a/src/plumbing.rs b/src/plumbing.rs index 6666ab53..8c5d4f43 100644 --- a/src/plumbing.rs +++ b/src/plumbing.rs @@ -17,7 +17,7 @@ pub use crate::derived::MemoizedStorage; pub use crate::input::InputStorage; pub use crate::interned::InternedStorage; pub use crate::interned::LookupInternedStorage; -pub use crate::revision::Revision; +pub use crate::{revision::Revision, DatabaseKeyIndex}; #[derive(Clone, Debug)] pub struct CycleDetected { @@ -51,6 +51,9 @@ pub trait DatabaseStorageTypes: Sized { /// Internal operations that the runtime uses to operate on the database. pub trait DatabaseOps: Sized { + /// True if the computed value for `input` may have changed since `revision`. + fn maybe_changed_since(&self, input: DatabaseKeyIndex, revision: Revision) -> bool; + /// Executes the callback for each kind of query. fn for_each_query(&self, op: impl FnMut(&dyn QueryStorageMassOps)); } @@ -150,6 +153,10 @@ where { fn new(group_index: u16) -> Self; + /// True if the value of `input`, which must be from this query, may have + /// changed since the given revision. + fn maybe_changed_since(&self, db: &DB, input: DatabaseKeyIndex, revision: Revision) -> bool; + /// Execute the query, returning the result (often, the result /// will be memoized). This is the "main method" for /// queries. diff --git a/src/runtime.rs b/src/runtime.rs index aa0a7700..9e08eae3 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -1,9 +1,7 @@ -use crate::dependency::DatabaseSlot; -use crate::dependency::Dependency; use crate::durability::Durability; use crate::plumbing::CycleDetected; use crate::revision::{AtomicRevision, Revision}; -use crate::{CycleError, Database, Event, EventKind, SweepStrategy}; +use crate::{CycleError, Database, DatabaseKeyIndex, Event, EventKind, SweepStrategy}; use log::debug; use parking_lot::lock_api::{RawRwLock, RawRwLockRecursive}; use parking_lot::{Mutex, RwLock}; @@ -380,13 +378,12 @@ where /// query had changed pub(crate) fn report_query_read<'hack>( &self, - database_slot: Arc + 'hack>, + input: DatabaseKeyIndex, durability: Durability, changed_at: Revision, ) { - let dependency = Dependency::new(database_slot); self.local_state - .report_query_read(dependency, durability, changed_at); + .report_query_read(input, durability, changed_at); } /// Reports that the query depends on some state unknown to salsa. @@ -651,7 +648,7 @@ struct ActiveQuery { /// Set of subqueries that were accessed thus far, or `None` if /// there was an untracked the read. - dependencies: Option>>, + dependencies: Option>, /// Stores the entire cycle, if one is found and this query is part of it. cycle: Vec, @@ -670,7 +667,7 @@ pub(crate) struct ComputedQueryResult { /// Complete set of subqueries that were accessed, or `None` if /// there was an untracked the read. - pub(crate) dependencies: Option>>, + pub(crate) dependencies: Option>, /// The cycle if one occured while computing this value pub(crate) cycle: Vec, @@ -687,9 +684,9 @@ impl ActiveQuery { } } - fn add_read(&mut self, dependency: Dependency, durability: Durability, revision: Revision) { + fn add_read(&mut self, input: DatabaseKeyIndex, durability: Durability, revision: Revision) { if let Some(set) = &mut self.dependencies { - set.insert(dependency); + set.insert(input); } self.durability = self.durability.min(durability); diff --git a/src/runtime/local_state.rs b/src/runtime/local_state.rs index d65ca3b0..966d0008 100644 --- a/src/runtime/local_state.rs +++ b/src/runtime/local_state.rs @@ -1,8 +1,7 @@ -use crate::dependency::Dependency; use crate::durability::Durability; use crate::runtime::ActiveQuery; use crate::runtime::Revision; -use crate::Database; +use crate::{Database, DatabaseKeyIndex}; use std::cell::{Ref, RefCell, RefMut}; /// State that is specific to a single execution thread. @@ -67,12 +66,12 @@ impl LocalState { pub(super) fn report_query_read( &self, - dependency: Dependency, + input: DatabaseKeyIndex, durability: Durability, changed_at: Revision, ) { if let Some(top_query) = self.query_stack.borrow_mut().last_mut() { - top_query.add_read(dependency, durability, changed_at); + top_query.add_read(input, durability, changed_at); } }