From e3f5eb6ee832d729f74c0178235e5389d3104413 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Sun, 3 Feb 2019 10:47:18 -0500 Subject: [PATCH 01/17] implement `#[salsa::interned]` query storage --- components/salsa-macros/src/query_group.rs | 34 +- src/interned.rs | 448 +++++++++++++++++++++ src/lib.rs | 13 + src/plumbing.rs | 11 + tests/interned.rs | 88 ++++ 5 files changed, 593 insertions(+), 1 deletion(-) create mode 100644 src/interned.rs create mode 100644 tests/interned.rs diff --git a/components/salsa-macros/src/query_group.rs b/components/salsa-macros/src/query_group.rs index 8d8e0baf..3cbe5873 100644 --- a/components/salsa-macros/src/query_group.rs +++ b/components/salsa-macros/src/query_group.rs @@ -60,6 +60,10 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream storage = QueryStorage::Input; num_storages += 1; } + "interned" => { + storage = QueryStorage::Interned; + num_storages += 1; + } "invoke" => { invoke = Some(parse_macro_input!(tts as Parenthesized).0); } @@ -195,6 +199,23 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream }); } + // For interned queries, we need `lookup_foo` + if let QueryStorage::Interned = query.storage { + let lookup_fn_name = Ident::new(&format!("lookup_{}", fn_name), fn_name.span()); + + query_fn_declarations.extend(quote! { + /// Lookup the value(s) interned with a specific key. + fn #lookup_fn_name(&mut self, value: #value) -> (#(#keys),*); + }); + + query_fn_definitions.extend(quote! { + fn #lookup_fn_name(&mut self, value: #value) -> (#(#keys),*) { + >::get_query_table(self) + .lookup(value) + } + }); + } + // A variant for the group descriptor below query_descriptor_variants.extend(quote! { #fn_name((#(#keys),*)), @@ -276,6 +297,7 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream QueryStorage::Volatile => "VolatileStorage", QueryStorage::Dependencies => "DependencyStorage", QueryStorage::Input => "InputStorage", + QueryStorage::Interned => "InternedStorage", }, Span::call_site(), ); @@ -310,7 +332,7 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream }); // Implement the QueryFunction trait for all queries except inputs. - if query.storage != QueryStorage::Input { + if query.storage.needs_query_function() { let span = query.fn_name.span(); let key_names: &Vec<_> = &(0..query.keys.len()) .map(|i| Ident::new(&format!("key{}", i), Span::call_site())) @@ -446,4 +468,14 @@ enum QueryStorage { Volatile, Dependencies, Input, + Interned, +} + +impl QueryStorage { + fn needs_query_function(self) -> bool { + match self { + QueryStorage::Input | QueryStorage::Interned => false, + QueryStorage::Memoized | QueryStorage::Volatile | QueryStorage::Dependencies => true, + } + } } diff --git a/src/interned.rs b/src/interned.rs new file mode 100644 index 00000000..e4223d17 --- /dev/null +++ b/src/interned.rs @@ -0,0 +1,448 @@ +use crate::debug::TableEntry; +use crate::plumbing::CycleDetected; +use crate::plumbing::InternedQueryStorageOps; +use crate::plumbing::QueryStorageMassOps; +use crate::plumbing::QueryStorageOps; +use crate::runtime::ChangedAt; +use crate::runtime::Revision; +use crate::runtime::StampedValue; +use crate::Query; +use crate::{Database, DiscardIf, SweepStrategy}; +use parking_lot::RwLock; +use rustc_hash::FxHashMap; +use std::collections::hash_map::Entry; +use std::convert::From; +use std::hash::Hash; + +/// Handles storage where the value is 'derived' by executing a +/// function (in contrast to "inputs"). +pub struct InternedStorage +where + Q: Query, + Q::Value: From, + Q::Value: Into, + DB: Database, +{ + tables: RwLock>, +} + +struct InternTables { + /// Map from the key to the corresponding intern-index. + map: FxHashMap, + + /// For each valid intern-index, stores the interned value. When + /// an interned value is GC'd, the entry is set to + /// `InternValue::Free` with the next free item. + values: Vec>, + + /// Index of the first free intern-index, if any. + first_free: Option, +} + +/// Newtype indicating an index into the intern table. +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +struct InternIndex { + index: u32, +} + +impl InternIndex { + fn index(self) -> usize { + self.index as usize + } +} + +impl From for InternIndex { + fn from(v: usize) -> Self { + assert!(v < (std::u32::MAX as usize)); + InternIndex { index: v as u32 } + } +} + +enum InternValue { + /// The value has not been gc'd. + Present { + value: K, + + /// When was this intern'd? + /// + /// (This informs the "changed-at" result) + interned_at: Revision, + + /// When was it accessed? + /// + /// (This informs the garbage collector) + accessed_at: Revision, + }, + + /// Free-list -- the index is the next + Free { next: Option }, +} + +impl std::panic::RefUnwindSafe for InternedStorage +where + Q: Query, + DB: Database, + Q::Key: std::panic::RefUnwindSafe, + Q::Value: From, + Q::Value: Into, + Q::Value: std::panic::RefUnwindSafe, +{ +} + +impl Default for InternedStorage +where + Q: Query, + Q::Key: Eq + Hash, + Q::Value: From, + Q::Value: Into, + DB: Database, +{ + fn default() -> Self { + InternedStorage { + tables: RwLock::new(InternTables::default()), + } + } +} + +impl Default for InternTables +where + K: Eq + Hash, +{ + fn default() -> Self { + Self { + map: Default::default(), + values: Default::default(), + first_free: Default::default(), + } + } +} + +impl InternedStorage +where + Q: Query, + Q::Key: Eq + Hash + Clone, + Q::Value: From, + Q::Value: Into, + DB: Database, +{ + fn intern_index(&self, db: &DB, key: &Q::Key) -> StampedValue { + if let Some(i) = self.intern_check(db, key) { + return i; + } + + let owned_key1 = key.to_owned(); + let owned_key2 = owned_key1.clone(); + let revision_now = db.salsa_runtime().current_revision(); + + let mut tables = self.tables.write(); + let tables = &mut *tables; + let entry = match tables.map.entry(owned_key1) { + Entry::Vacant(entry) => entry, + Entry::Occupied(entry) => { + // Somebody inserted this key while we were waiting + // for the write lock. + let index = *entry.get(); + match &tables.values[index.index()] { + InternValue::Present { + value, + interned_at, + accessed_at, + } => { + debug_assert_eq!(owned_key2, *value); + debug_assert_eq!(*accessed_at, revision_now); + return StampedValue { + value: index, + changed_at: ChangedAt { + is_constant: false, + revision: *interned_at, + }, + }; + } + + InternValue::Free { .. } => { + panic!("key {:?} should be present but is not", key,); + } + } + } + }; + + let index = match tables.first_free { + None => { + let index = InternIndex::from(tables.values.len()); + tables.values.push(InternValue::Present { + value: owned_key2, + interned_at: revision_now, + accessed_at: revision_now, + }); + index + } + + Some(i) => { + let next_free = match &tables.values[i.index()] { + InternValue::Free { next } => *next, + InternValue::Present { value, .. } => { + panic!( + "index {:?} was supposed to be free but contains {:?}", + i, value + ); + } + }; + + tables.values[i.index()] = InternValue::Present { + value: owned_key2, + interned_at: revision_now, + accessed_at: revision_now, + }; + tables.first_free = next_free; + i + } + }; + + entry.insert(index); + + StampedValue { + value: index, + changed_at: ChangedAt { + is_constant: false, + revision: revision_now, + }, + } + } + + fn intern_check(&self, db: &DB, key: &Q::Key) -> Option> { + let revision_now = db.salsa_runtime().current_revision(); + + // First, + { + let tables = self.tables.read(); + let &index = tables.map.get(key)?; + match &tables.values[index.index()] { + InternValue::Present { + interned_at, + accessed_at, + .. + } => { + if *accessed_at == revision_now { + return Some(StampedValue { + value: index, + changed_at: ChangedAt { + is_constant: false, + revision: *interned_at, + }, + }); + } + } + + InternValue::Free { .. } => { + panic!( + "key {:?} maps to index {:?} is free but should not be", + key, index + ); + } + } + } + + // Next, + let mut tables = self.tables.write(); + let &index = tables.map.get(key)?; + match &mut tables.values[index.index()] { + InternValue::Present { + interned_at, + accessed_at, + .. + } => { + *accessed_at = revision_now; + + return Some(StampedValue { + value: index, + changed_at: ChangedAt { + is_constant: false, + revision: *interned_at, + }, + }); + } + + InternValue::Free { .. } => { + panic!( + "key {:?} maps to index {:?} is free but should not be", + key, index + ); + } + } + } + + /// Given an index, lookup and clone its value, updating the + /// `accessed_at` time if necessary. + fn lookup_value(&self, db: &DB, index: u32) -> StampedValue { + let index = index as usize; + let revision_now = db.salsa_runtime().current_revision(); + + { + let tables = self.tables.read(); + match &tables.values[index] { + InternValue::Present { + accessed_at, + interned_at, + value, + } => { + if *accessed_at == revision_now { + return StampedValue { + value: value.clone(), + changed_at: ChangedAt { + is_constant: false, + revision: *interned_at, + }, + }; + } + } + + InternValue::Free { .. } => panic!("lookup of index {:?} found a free slot", index), + } + } + + let mut tables = self.tables.write(); + match &mut tables.values[index] { + InternValue::Present { + accessed_at, + interned_at, + value, + } => { + *accessed_at = revision_now; + + return StampedValue { + value: value.clone(), + changed_at: ChangedAt { + is_constant: false, + revision: *interned_at, + }, + }; + } + + InternValue::Free { .. } => panic!("lookup of index {:?} found a free slot", index), + } + } +} + +impl QueryStorageOps for InternedStorage +where + Q: Query, + Q::Key: ToOwned, + ::Owned: Eq + Hash + Clone, + Q::Value: From, + Q::Value: Into, + DB: Database, +{ + fn try_fetch( + &self, + db: &DB, + key: &Q::Key, + database_key: &DB::DatabaseKey, + ) -> Result { + let StampedValue { value, changed_at } = self.intern_index(db, key); + + db.salsa_runtime() + .report_query_read(database_key, changed_at); + + Ok(::from(value.index)) + } + + fn maybe_changed_since( + &self, + db: &DB, + revision: Revision, + key: &Q::Key, + _database_key: &DB::DatabaseKey, + ) -> bool { + match self.intern_check(db, key) { + Some(StampedValue { + value: _, + changed_at, + }) => changed_at.changed_since(revision), + None => true, + } + } + + fn is_constant(&self, _db: &DB, _key: &Q::Key) -> bool { + false + } + + fn entries(&self, _db: &DB) -> C + where + C: std::iter::FromIterator>, + { + let tables = self.tables.read(); + tables + .map + .iter() + .map(|(key, index)| TableEntry::new(key.clone(), Some(::from(index.index)))) + .collect() + } +} + +impl InternedQueryStorageOps for InternedStorage +where + Q: Query, + Q::Key: ToOwned, + ::Owned: Eq + Hash + Clone, + Q::Value: From, + Q::Value: Into, + DB: Database, +{ + fn lookup(&self, db: &DB, value: Q::Value) -> Q::Key { + let index: u32 = value.into(); + let StampedValue { + value, + changed_at: _, + } = self.lookup_value(db, index); + + // XXX -- this setup is wrong, we can't report the read. We + // should create a *second* query that is linked to this query + // somehow. Or, at least, we need a distinct entry in the + // group key so that we can implement the "maybe changed" and + // all that stuff. + + value + } +} + +impl QueryStorageMassOps for InternedStorage +where + Q: Query, + Q::Key: ToOwned, + Q::Value: From, + Q::Value: Into, + DB: Database, +{ + fn sweep(&self, db: &DB, strategy: SweepStrategy) { + let mut tables = self.tables.write(); + let revision_now = db.salsa_runtime().current_revision(); + let InternTables { + map, + values, + first_free, + } = &mut *tables; + map.retain(|key, intern_index| { + let discard = match strategy.discard_if { + DiscardIf::Never => false, + DiscardIf::Outdated => match values[intern_index.index()] { + InternValue::Present { accessed_at, .. } => accessed_at < revision_now, + + InternValue::Free { .. } => { + panic!( + "key {:?} maps to index {:?} which is free", + key, intern_index + ); + } + }, + DiscardIf::Always => false, + }; + + if discard { + values[intern_index.index()] = InternValue::Free { next: *first_free }; + *first_free = Some(*intern_index); + } + + !discard + }); + } +} diff --git a/src/lib.rs b/src/lib.rs index dab0200f..7bf2204e 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -10,6 +10,7 @@ mod derived; mod input; +mod interned; mod runtime; pub mod debug; @@ -20,6 +21,7 @@ pub mod plumbing; use crate::plumbing::CycleDetected; use crate::plumbing::InputQueryStorageOps; +use crate::plumbing::InternedQueryStorageOps; use crate::plumbing::QueryStorageMassOps; use crate::plumbing::QueryStorageOps; use derive_new::new; @@ -462,6 +464,17 @@ where }) } + /// For `#[salsa::interned]` queries only, does a reverse lookup + /// from the interned value (which must be some newtype'd integer) + /// to get the key that was interned. + pub fn lookup(&self, value: Q::Value) -> Q::Key + where + Q::Storage: plumbing::InternedQueryStorageOps, + Q::Value: Into, + { + self.storage.lookup(self.db, value) + } + /// Remove all values for this query that have not been used in /// the most recent revision. pub fn sweep(&self, strategy: SweepStrategy) diff --git a/src/plumbing.rs b/src/plumbing.rs index 01a909a9..d668cba9 100644 --- a/src/plumbing.rs +++ b/src/plumbing.rs @@ -13,6 +13,7 @@ pub use crate::derived::DependencyStorage; pub use crate::derived::MemoizedStorage; pub use crate::derived::VolatileStorage; pub use crate::input::InputStorage; +pub use crate::interned::InternedStorage; pub use crate::runtime::Revision; pub struct CycleDetected; @@ -183,6 +184,16 @@ where C: std::iter::FromIterator>; } +/// An optional trait that is implemented for "interned" storage. +pub trait InternedQueryStorageOps: Default +where + DB: Database, + Q: Query, + Q::Value: Into, +{ + fn lookup(&self, db: &DB, value: Q::Value) -> Q::Key; +} + /// An optional trait that is implemented for "user mutable" storage: /// that is, storage whose value is not derived from other storage but /// is set independently. diff --git a/tests/interned.rs b/tests/interned.rs new file mode 100644 index 00000000..ad5e175b --- /dev/null +++ b/tests/interned.rs @@ -0,0 +1,88 @@ +//! Test that you can implement a query using a `dyn Trait` setup. + +#[salsa::database(InternStorage)] +#[derive(Default)] +struct Database { + runtime: salsa::Runtime, +} + +impl salsa::Database for Database { + fn salsa_runtime(&self) -> &salsa::Runtime { + &self.runtime + } +} + +#[salsa::query_group(InternStorage)] +trait Intern { + #[salsa::interned] + fn intern1(&self, x: String) -> u32; + + #[salsa::interned] + fn intern2(&self, x: String, y: String) -> u32; + + #[salsa::interned] + fn intern_key(&self, x: String) -> InternKey; +} + +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] +pub struct InternKey(u32); + +impl From for InternKey { + fn from(v: u32) -> Self { + InternKey(v) + } +} + +impl Into for InternKey { + fn into(self) -> u32 { + self.0 + } +} + +#[test] +fn test_intern1() { + let mut db = Database::default(); + let foo0 = db.intern1(format!("foo")); + let bar0 = db.intern1(format!("bar")); + let foo1 = db.intern1(format!("foo")); + let bar1 = db.intern1(format!("bar")); + + assert_eq!(foo0, foo1); + assert_eq!(bar0, bar1); + assert_ne!(foo0, bar0); + + assert_eq!(format!("foo"), db.lookup_intern1(foo0)); + assert_eq!(format!("bar"), db.lookup_intern1(bar0)); +} + +#[test] +fn test_intern2() { + let mut db = Database::default(); + let foo0 = db.intern2(format!("x"), format!("foo")); + let bar0 = db.intern2(format!("x"), format!("bar")); + let foo1 = db.intern2(format!("x"), format!("foo")); + let bar1 = db.intern2(format!("x"), format!("bar")); + + assert_eq!(foo0, foo1); + assert_eq!(bar0, bar1); + assert_ne!(foo0, bar0); + + assert_eq!((format!("x"), format!("foo")), db.lookup_intern2(foo0)); + assert_eq!((format!("x"), format!("bar")), db.lookup_intern2(bar0)); +} + +#[test] +fn test_intern_key() { + let mut db = Database::default(); + let foo0 = db.intern_key(format!("foo")); + let bar0 = db.intern_key(format!("bar")); + let foo1 = db.intern_key(format!("foo")); + let bar1 = db.intern_key(format!("bar")); + + assert_eq!(foo0, foo1); + assert_eq!(bar0, bar1); + assert_ne!(foo0, bar0); + + assert_eq!(format!("foo"), db.lookup_intern_key(foo0)); + assert_eq!(format!("bar"), db.lookup_intern_key(bar0)); +} From 1fbd61bf87daab862448d1426e9840c5da672dbc Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 4 Feb 2019 21:01:23 +0100 Subject: [PATCH 02/17] adopt InternKey trait --- src/interned.rs | 66 +++++++++++++++++++++++++++++++---------------- src/lib.rs | 3 ++- src/plumbing.rs | 1 - tests/interned.rs | 8 +++--- 4 files changed, 49 insertions(+), 29 deletions(-) diff --git a/src/interned.rs b/src/interned.rs index e4223d17..8348bd9a 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -19,8 +19,7 @@ use std::hash::Hash; pub struct InternedStorage where Q: Query, - Q::Value: From, - Q::Value: Into, + Q::Value: InternKey, DB: Database, { tables: RwLock>, @@ -39,6 +38,38 @@ struct InternTables { first_free: Option, } +/// Trait implemented for the "key" that results from a +/// `#[salsa::intern]` query. This is basically meant to be a +/// "newtype"'d `u32`. +pub trait InternKey { + /// Create an instance of the intern-key from a `u32` value. + fn from_u32(v: u32) -> Self; + + /// Extract the `u32` with which the intern-key was created. + fn as_u32(&self) -> u32; +} + +impl InternKey for u32 { + fn from_u32(v: u32) -> Self { + v + } + + fn as_u32(&self) -> u32 { + *self + } +} + +impl InternKey for usize { + fn from_u32(v: u32) -> Self { + v as usize + } + + fn as_u32(&self) -> u32 { + assert!(*self < (std::u32::MAX as usize)); + *self as u32 + } +} + /// Newtype indicating an index into the intern table. #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] struct InternIndex { @@ -83,8 +114,7 @@ where Q: Query, DB: Database, Q::Key: std::panic::RefUnwindSafe, - Q::Value: From, - Q::Value: Into, + Q::Value: InternKey, Q::Value: std::panic::RefUnwindSafe, { } @@ -93,8 +123,7 @@ impl Default for InternedStorage where Q: Query, Q::Key: Eq + Hash, - Q::Value: From, - Q::Value: Into, + Q::Value: InternKey, DB: Database, { fn default() -> Self { @@ -121,8 +150,7 @@ impl InternedStorage where Q: Query, Q::Key: Eq + Hash + Clone, - Q::Value: From, - Q::Value: Into, + Q::Value: InternKey, DB: Database, { fn intern_index(&self, db: &DB, key: &Q::Key) -> StampedValue { @@ -326,10 +354,7 @@ where impl QueryStorageOps for InternedStorage where Q: Query, - Q::Key: ToOwned, - ::Owned: Eq + Hash + Clone, - Q::Value: From, - Q::Value: Into, + Q::Value: InternKey, DB: Database, { fn try_fetch( @@ -343,7 +368,7 @@ where db.salsa_runtime() .report_query_read(database_key, changed_at); - Ok(::from(value.index)) + Ok(::from_u32(value.index)) } fn maybe_changed_since( @@ -374,7 +399,9 @@ where tables .map .iter() - .map(|(key, index)| TableEntry::new(key.clone(), Some(::from(index.index)))) + .map(|(key, index)| { + TableEntry::new(key.clone(), Some(::from_u32(index.index))) + }) .collect() } } @@ -382,14 +409,11 @@ where impl InternedQueryStorageOps for InternedStorage where Q: Query, - Q::Key: ToOwned, - ::Owned: Eq + Hash + Clone, - Q::Value: From, - Q::Value: Into, + Q::Value: InternKey, DB: Database, { fn lookup(&self, db: &DB, value: Q::Value) -> Q::Key { - let index: u32 = value.into(); + let index: u32 = value.as_u32(); let StampedValue { value, changed_at: _, @@ -408,9 +432,7 @@ where impl QueryStorageMassOps for InternedStorage where Q: Query, - Q::Key: ToOwned, - Q::Value: From, - Q::Value: Into, + Q::Value: InternKey, DB: Database, { fn sweep(&self, db: &DB, strategy: SweepStrategy) { diff --git a/src/lib.rs b/src/lib.rs index 7bf2204e..fa526cf7 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -28,6 +28,7 @@ use derive_new::new; use std::fmt::{self, Debug}; use std::hash::Hash; +pub use crate::interned::InternKey; pub use crate::runtime::Runtime; pub use crate::runtime::RuntimeId; @@ -470,7 +471,7 @@ where pub fn lookup(&self, value: Q::Value) -> Q::Key where Q::Storage: plumbing::InternedQueryStorageOps, - Q::Value: Into, + Q::Value: InternKey, { self.storage.lookup(self.db, value) } diff --git a/src/plumbing.rs b/src/plumbing.rs index d668cba9..d4ac11d5 100644 --- a/src/plumbing.rs +++ b/src/plumbing.rs @@ -189,7 +189,6 @@ pub trait InternedQueryStorageOps: Default where DB: Database, Q: Query, - Q::Value: Into, { fn lookup(&self, db: &DB, value: Q::Value) -> Q::Key; } diff --git a/tests/interned.rs b/tests/interned.rs index ad5e175b..13c6ce7b 100644 --- a/tests/interned.rs +++ b/tests/interned.rs @@ -27,14 +27,12 @@ trait Intern { #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub struct InternKey(u32); -impl From for InternKey { - fn from(v: u32) -> Self { +impl salsa::InternKey for InternKey { + fn from_u32(v: u32) -> Self { InternKey(v) } -} -impl Into for InternKey { - fn into(self) -> u32 { + fn as_u32(&self) -> u32 { self.0 } } From 56ef78109a8756347eddb9fbb11736ac71b0ba78 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 4 Feb 2019 22:09:57 +0100 Subject: [PATCH 03/17] remove send/sync bounds --- src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib.rs b/src/lib.rs index fa526cf7..acc3ba8f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -409,7 +409,7 @@ pub trait Query: Debug + Default + Sized + 'static { type Value: Clone + Debug; /// Internal struct storing the values for the query. - type Storage: plumbing::QueryStorageOps + Send + Sync; + type Storage: plumbing::QueryStorageOps; /// Associate query group struct. type Group: plumbing::QueryGroup< From f48515747cc5f18ea60cff127f1d2a1594be8280 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 4 Feb 2019 22:21:24 +0100 Subject: [PATCH 04/17] create a true inverse key for the lookup path --- components/salsa-macros/src/query_group.rs | 106 ++++++++----- src/interned.rs | 172 +++++++++++++++++---- src/lib.rs | 12 -- src/plumbing.rs | 10 +- tests/interned.rs | 6 +- 5 files changed, 219 insertions(+), 87 deletions(-) diff --git a/components/salsa-macros/src/query_group.rs b/components/salsa-macros/src/query_group.rs index 3cbe5873..57bdfc79 100644 --- a/components/salsa-macros/src/query_group.rs +++ b/components/salsa-macros/src/query_group.rs @@ -3,7 +3,7 @@ use heck::CamelCase; use proc_macro::TokenStream; use proc_macro2::Span; use quote::ToTokens; -use syn::{parse_macro_input, FnArg, Ident, ItemTrait, ReturnType, TraitItem}; +use syn::{parse_macro_input, parse_quote, FnArg, Ident, ItemTrait, ReturnType, TraitItem, Type}; /// Implementation for `[salsa::query_group]` decorator. pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream { @@ -110,15 +110,56 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream ), }; + // For `#[salsa::interned]` keys, we create a "lookup key" automatically. + // + // For a query like: + // + // fn foo(&self, x: Key1, y: Key2) -> u32 + // + // we would create + // + // fn lookup_foo(&self, x: u32) -> (Key1, Key2) + let lookup_query = if let QueryStorage::Interned = storage { + let lookup_query_type = Ident::new( + &format!( + "{}LookupQuery", + method.sig.ident.to_string().to_camel_case() + ), + Span::call_site(), + ); + let lookup_fn_name = Ident::new( + &format!("lookup_{}", method.sig.ident.to_string()), + method.sig.ident.span(), + ); + let keys = &keys; + let lookup_value: Type = parse_quote!((#(#keys),*)); + let lookup_keys = vec![value.clone()]; + Some(Query { + query_type: lookup_query_type, + fn_name: lookup_fn_name, + attrs: vec![], // FIXME -- some automatically generated docs on this method? + storage: QueryStorage::InternedLookup { + intern_query_type: query_type.clone(), + }, + keys: lookup_keys, + value: lookup_value, + invoke: None, + }) + } else { + None + }; + queries.push(Query { query_type, - fn_name: method.sig.ident.clone(), + fn_name: method.sig.ident, attrs, storage, keys, value, invoke, }); + + queries.extend(lookup_query); } _ => (), } @@ -199,23 +240,6 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream }); } - // For interned queries, we need `lookup_foo` - if let QueryStorage::Interned = query.storage { - let lookup_fn_name = Ident::new(&format!("lookup_{}", fn_name), fn_name.span()); - - query_fn_declarations.extend(quote! { - /// Lookup the value(s) interned with a specific key. - fn #lookup_fn_name(&mut self, value: #value) -> (#(#keys),*); - }); - - query_fn_definitions.extend(quote! { - fn #lookup_fn_name(&mut self, value: #value) -> (#(#keys),*) { - >::get_query_table(self) - .lookup(value) - } - }); - } - // A variant for the group descriptor below query_descriptor_variants.extend(quote! { #fn_name((#(#keys),*)), @@ -266,6 +290,7 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream impl salsa::plumbing::QueryGroup for #group_struct where DB__: #trait_name, + DB__: salsa::plumbing::HasQueryGroup<#group_struct>, DB__: salsa::Database, { type GroupStorage = #group_storage; @@ -291,16 +316,19 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream for query in &queries { let fn_name = &query.fn_name; let qt = &query.query_type; - let storage = Ident::new( - match query.storage { - QueryStorage::Memoized => "MemoizedStorage", - QueryStorage::Volatile => "VolatileStorage", - QueryStorage::Dependencies => "DependencyStorage", - QueryStorage::Input => "InputStorage", - QueryStorage::Interned => "InternedStorage", - }, - Span::call_site(), - ); + + let db = quote! {DB}; + + let storage = match &query.storage { + QueryStorage::Memoized => quote!(salsa::plumbing::MemoizedStorage<#db, Self>), + QueryStorage::Volatile => quote!(salsa::plumbing::VolatileStorage<#db, Self>), + QueryStorage::Dependencies => quote!(salsa::plumbing::DependencyStorage<#db, Self>), + QueryStorage::Input => quote!(salsa::plumbing::InputStorage<#db, Self>), + QueryStorage::Interned => quote!(salsa::plumbing::InternedStorage<#db, Self>), + QueryStorage::InternedLookup { intern_query_type } => { + quote!(salsa::plumbing::LookupInternedStorage<#db, Self, #intern_query_type>) + } + }; let keys = &query.keys; let value = &query.value; @@ -309,16 +337,17 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream #[derive(Default, Debug)] #trait_vis struct #qt; - impl salsa::Query for #qt + impl<#db> salsa::Query<#db> for #qt where DB: #trait_name, + DB: salsa::plumbing::HasQueryGroup<#group_struct>, DB: salsa::Database, { type Key = (#(#keys),*); type Value = #value; - type Storage = salsa::plumbing::#storage; + type Storage = #storage; type Group = #group_struct; - type GroupStorage = #group_storage; + type GroupStorage = #group_storage<#db>; type GroupKey = #group_key; fn query_storage(group_storage: &Self::GroupStorage) -> &Self::Storage { @@ -331,7 +360,7 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream } }); - // Implement the QueryFunction trait for all queries except inputs. + // Implement the QueryFunction trait for queries which need it. if query.storage.needs_query_function() { let span = query.fn_name.span(); let key_names: &Vec<_> = &(0..query.keys.len()) @@ -401,6 +430,7 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream #trait_vis struct #group_storage where DB__: #trait_name, + DB__: salsa::plumbing::HasQueryGroup<#group_struct>, DB__: salsa::Database, { #storage_fields @@ -409,6 +439,7 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream impl Default for #group_storage where DB__: #trait_name, + DB__: salsa::plumbing::HasQueryGroup<#group_struct>, DB__: salsa::Database, { #[inline] @@ -462,19 +493,22 @@ struct Query { invoke: Option, } -#[derive(Debug, Clone, Copy, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq)] enum QueryStorage { Memoized, Volatile, Dependencies, Input, Interned, + InternedLookup { intern_query_type: Ident }, } impl QueryStorage { - fn needs_query_function(self) -> bool { + fn needs_query_function(&self) -> bool { match self { - QueryStorage::Input | QueryStorage::Interned => false, + QueryStorage::Input | QueryStorage::Interned | QueryStorage::InternedLookup { .. } => { + false + } QueryStorage::Memoized | QueryStorage::Volatile | QueryStorage::Dependencies => true, } } diff --git a/src/interned.rs b/src/interned.rs index 8348bd9a..0ba074c2 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -1,6 +1,6 @@ use crate::debug::TableEntry; use crate::plumbing::CycleDetected; -use crate::plumbing::InternedQueryStorageOps; +use crate::plumbing::HasQueryGroup; use crate::plumbing::QueryStorageMassOps; use crate::plumbing::QueryStorageOps; use crate::runtime::ChangedAt; @@ -25,6 +25,25 @@ where tables: RwLock>, } +/// Storage for the looking up interned things. +pub struct LookupInternedStorage +where + Q: Query, + Q::Key: InternKey, + Q::Value: Eq + Hash, + IQ: Query< + DB, + Key = Q::Value, + Value = Q::Key, + Group = Q::Group, + GroupStorage = Q::GroupStorage, + GroupKey = Q::GroupKey, + >, + DB: Database, +{ + phantom: std::marker::PhantomData<(DB, Q, IQ)>, +} + struct InternTables { /// Map from the key to the corresponding intern-index. map: FxHashMap, @@ -133,6 +152,28 @@ where } } +impl Default for LookupInternedStorage +where + Q: Query, + Q::Key: InternKey, + Q::Value: Eq + Hash, + IQ: Query< + DB, + Key = Q::Value, + Value = Q::Key, + Group = Q::Group, + GroupStorage = Q::GroupStorage, + GroupKey = Q::GroupKey, + >, + DB: Database, +{ + fn default() -> Self { + LookupInternedStorage { + phantom: std::marker::PhantomData, + } + } +} + impl Default for InternTables where K: Eq + Hash, @@ -301,7 +342,12 @@ where /// Given an index, lookup and clone its value, updating the /// `accessed_at` time if necessary. - fn lookup_value(&self, db: &DB, index: u32) -> StampedValue { + fn lookup_value( + &self, + db: &DB, + index: u32, + op: impl FnOnce(&Q::Key) -> R, + ) -> StampedValue { let index = index as usize; let revision_now = db.salsa_runtime().current_revision(); @@ -315,7 +361,7 @@ where } => { if *accessed_at == revision_now { return StampedValue { - value: value.clone(), + value: op(value), changed_at: ChangedAt { is_constant: false, revision: *interned_at, @@ -338,7 +384,7 @@ where *accessed_at = revision_now; return StampedValue { - value: value.clone(), + value: op(value), changed_at: ChangedAt { is_constant: false, revision: *interned_at, @@ -406,29 +452,6 @@ where } } -impl InternedQueryStorageOps for InternedStorage -where - Q: Query, - Q::Value: InternKey, - DB: Database, -{ - fn lookup(&self, db: &DB, value: Q::Value) -> Q::Key { - let index: u32 = value.as_u32(); - let StampedValue { - value, - changed_at: _, - } = self.lookup_value(db, index); - - // XXX -- this setup is wrong, we can't report the read. We - // should create a *second* query that is linked to this query - // somehow. Or, at least, we need a distinct entry in the - // group key so that we can implement the "maybe changed" and - // all that stuff. - - value - } -} - impl QueryStorageMassOps for InternedStorage where Q: Query, @@ -468,3 +491,98 @@ where }); } } + +impl QueryStorageOps for LookupInternedStorage +where + Q: Query, + Q::Key: InternKey, + Q::Value: Eq + Hash, + IQ: Query< + DB, + Key = Q::Value, + Value = Q::Key, + Storage = InternedStorage, + Group = Q::Group, + GroupStorage = Q::GroupStorage, + GroupKey = Q::GroupKey, + >, + DB: Database + HasQueryGroup, +{ + fn try_fetch( + &self, + db: &DB, + key: &Q::Key, + database_key: &DB::DatabaseKey, + ) -> Result { + let index: u32 = key.as_u32(); + + let group_storage = >::group_storage(db); + let interned_storage = IQ::query_storage(group_storage); + let StampedValue { value, changed_at } = + interned_storage.lookup_value(db, index, Clone::clone); + + db.salsa_runtime() + .report_query_read(database_key, changed_at); + + Ok(value) + } + + fn maybe_changed_since( + &self, + db: &DB, + revision: Revision, + key: &Q::Key, + _database_key: &DB::DatabaseKey, + ) -> bool { + let index: u32 = key.as_u32(); + + // FIXME -- This seems maybe not quite right, as it will panic + // if `key` has been removed from the map since, but it should + // return true in that event. + + let group_storage = >::group_storage(db); + let interned_storage = IQ::query_storage(group_storage); + let StampedValue { + value: (), + changed_at, + } = interned_storage.lookup_value(db, index, |_| ()); + + changed_at.changed_since(revision) + } + + fn is_constant(&self, _db: &DB, _key: &Q::Key) -> bool { + false + } + + fn entries(&self, db: &DB) -> C + where + C: std::iter::FromIterator>, + { + let group_storage = >::group_storage(db); + let interned_storage = IQ::query_storage(group_storage); + let tables = interned_storage.tables.read(); + tables + .map + .iter() + .map(|(key, index)| TableEntry::new(::from_u32(index.index), Some(key.clone()))) + .collect() + } +} + +impl QueryStorageMassOps for LookupInternedStorage +where + Q: Query, + Q::Key: InternKey, + Q::Value: Eq + Hash, + IQ: Query< + DB, + Key = Q::Value, + Value = Q::Key, + Group = Q::Group, + GroupStorage = Q::GroupStorage, + GroupKey = Q::GroupKey, + >, + DB: Database, +{ + fn sweep(&self, _db: &DB, _strategy: SweepStrategy) {} +} diff --git a/src/lib.rs b/src/lib.rs index acc3ba8f..8fc1593d 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -21,7 +21,6 @@ pub mod plumbing; use crate::plumbing::CycleDetected; use crate::plumbing::InputQueryStorageOps; -use crate::plumbing::InternedQueryStorageOps; use crate::plumbing::QueryStorageMassOps; use crate::plumbing::QueryStorageOps; use derive_new::new; @@ -465,17 +464,6 @@ where }) } - /// For `#[salsa::interned]` queries only, does a reverse lookup - /// from the interned value (which must be some newtype'd integer) - /// to get the key that was interned. - pub fn lookup(&self, value: Q::Value) -> Q::Key - where - Q::Storage: plumbing::InternedQueryStorageOps, - Q::Value: InternKey, - { - self.storage.lookup(self.db, value) - } - /// Remove all values for this query that have not been used in /// the most recent revision. pub fn sweep(&self, strategy: SweepStrategy) diff --git a/src/plumbing.rs b/src/plumbing.rs index d4ac11d5..36fa1c0e 100644 --- a/src/plumbing.rs +++ b/src/plumbing.rs @@ -14,6 +14,7 @@ pub use crate::derived::MemoizedStorage; pub use crate::derived::VolatileStorage; pub use crate::input::InputStorage; pub use crate::interned::InternedStorage; +pub use crate::interned::LookupInternedStorage; pub use crate::runtime::Revision; pub struct CycleDetected; @@ -184,15 +185,6 @@ where C: std::iter::FromIterator>; } -/// An optional trait that is implemented for "interned" storage. -pub trait InternedQueryStorageOps: Default -where - DB: Database, - Q: Query, -{ - fn lookup(&self, db: &DB, value: Q::Value) -> Q::Key; -} - /// An optional trait that is implemented for "user mutable" storage: /// that is, storage whose value is not derived from other storage but /// is set independently. diff --git a/tests/interned.rs b/tests/interned.rs index 13c6ce7b..f466e722 100644 --- a/tests/interned.rs +++ b/tests/interned.rs @@ -39,7 +39,7 @@ impl salsa::InternKey for InternKey { #[test] fn test_intern1() { - let mut db = Database::default(); + let db = Database::default(); let foo0 = db.intern1(format!("foo")); let bar0 = db.intern1(format!("bar")); let foo1 = db.intern1(format!("foo")); @@ -55,7 +55,7 @@ fn test_intern1() { #[test] fn test_intern2() { - let mut db = Database::default(); + let db = Database::default(); let foo0 = db.intern2(format!("x"), format!("foo")); let bar0 = db.intern2(format!("x"), format!("bar")); let foo1 = db.intern2(format!("x"), format!("foo")); @@ -71,7 +71,7 @@ fn test_intern2() { #[test] fn test_intern_key() { - let mut db = Database::default(); + let db = Database::default(); let foo0 = db.intern_key(format!("foo")); let bar0 = db.intern_key(format!("bar")); let foo1 = db.intern_key(format!("foo")); From 71f250d037a28b66d4f011cab7a4d10f356d976b Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 12 Mar 2019 09:26:46 -0400 Subject: [PATCH 05/17] WIP fix DiscardIf::Always, thanks matklad :) --- src/interned.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/interned.rs b/src/interned.rs index 0ba074c2..572181eb 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -479,7 +479,7 @@ where ); } }, - DiscardIf::Always => false, + DiscardIf::Always => true, }; if discard { From 7ed24f0fa31a4ab0f210bbbcb8ab44a13deb26bb Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 13 Mar 2019 05:18:23 -0400 Subject: [PATCH 06/17] use `InternIndex` also to represent indices from the user --- src/interned.rs | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/interned.rs b/src/interned.rs index 572181eb..7141006c 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -90,6 +90,9 @@ impl InternKey for usize { } /// Newtype indicating an index into the intern table. +/// +/// NB. In some cases, `InternIndex` values come directly from the +/// user and hence they are not 'trusted' to be valid or in-bounds. #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] struct InternIndex { index: u32, @@ -103,8 +106,16 @@ impl InternIndex { impl From for InternIndex { fn from(v: usize) -> Self { - assert!(v < (std::u32::MAX as usize)); - InternIndex { index: v as u32 } + InternIndex { index: v.as_u32() } + } +} + +impl From<&T> for InternIndex +where + T: InternKey, +{ + fn from(v: &T) -> Self { + InternIndex { index: v.as_u32() } } } @@ -345,10 +356,10 @@ where fn lookup_value( &self, db: &DB, - index: u32, + index: InternIndex, op: impl FnOnce(&Q::Key) -> R, ) -> StampedValue { - let index = index as usize; + let index = index.index(); let revision_now = db.salsa_runtime().current_revision(); { @@ -514,7 +525,7 @@ where key: &Q::Key, database_key: &DB::DatabaseKey, ) -> Result { - let index: u32 = key.as_u32(); + let index = InternIndex::from(key); let group_storage = >::group_storage(db); let interned_storage = IQ::query_storage(group_storage); @@ -534,7 +545,7 @@ where key: &Q::Key, _database_key: &DB::DatabaseKey, ) -> bool { - let index: u32 = key.as_u32(); + let index = InternIndex::from(key); // FIXME -- This seems maybe not quite right, as it will panic // if `key` has been removed from the map since, but it should From 7dcdad88e3bdff1cadbf2219fbb178423d6b5fd3 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 13 Mar 2019 05:18:05 -0400 Subject: [PATCH 07/17] convert to `<=` when comparing against `std::u32::MAX` --- src/interned.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/interned.rs b/src/interned.rs index 7141006c..83a5f36f 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -84,7 +84,7 @@ impl InternKey for usize { } fn as_u32(&self) -> u32 { - assert!(*self < (std::u32::MAX as usize)); + assert!(*self <= (std::u32::MAX as usize)); *self as u32 } } From 7d5d01104c5b4cef63bdaaac5bd406774f59eb7d Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 13 Mar 2019 05:27:14 -0400 Subject: [PATCH 08/17] document the logic from a FIXME and improve panics --- src/interned.rs | 40 +++++++++++++++++++++++++++++++++++----- 1 file changed, 35 insertions(+), 5 deletions(-) diff --git a/src/interned.rs b/src/interned.rs index 83a5f36f..8a0d8dfa 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -364,6 +364,12 @@ where { let tables = self.tables.read(); + debug_assert!( + index < tables.values.len(), + "interned key ``{:?}({})` is out of bounds", + Q::default(), + index, + ); match &tables.values[index] { InternValue::Present { accessed_at, @@ -381,7 +387,11 @@ where } } - InternValue::Free { .. } => panic!("lookup of index {:?} found a free slot", index), + InternValue::Free { .. } => panic!( + "interned key `{:?}({})` has been garbage collected", + Q::default(), + index, + ), } } @@ -403,7 +413,11 @@ where }; } - InternValue::Free { .. } => panic!("lookup of index {:?} found a free slot", index), + InternValue::Free { .. } => panic!( + "interned key `{:?}({})` has been garbage collected", + Q::default(), + index, + ), } } } @@ -547,9 +561,25 @@ where ) -> bool { let index = InternIndex::from(key); - // FIXME -- This seems maybe not quite right, as it will panic - // if `key` has been removed from the map since, but it should - // return true in that event. + // NB. This will **panic** if `key` has been removed from the + // map, whereas you might expect it to return true in that + // event. But I think this is ok. You have to ask yourself, + // where did this (invalid) key K come from? There are two + // options: + // + // ## Some query Q1 obtained the key K by interning a value: + // + // In that case, Q1 has a prior input that computes K. This + // input must be invalid and hence Q1 must be considered to + // have changed, so it shouldn't be asking if we have changed. + // + // ## Some query Q1 was given K as an input: + // + // In that case, the query Q1 must be invoked (ultimately) by + // some query Q2 that computed K. This implies that K must be + // the result of *some* valid interning call, and therefore + // that it should be a valid key now (and not pointing at a + // free slot or out of bounds). let group_storage = >::group_storage(db); let interned_storage = IQ::query_storage(group_storage); From c8b30c52e187d2e8e68c4811c08c38a6a154862f Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 13 Mar 2019 05:30:03 -0400 Subject: [PATCH 09/17] nit: change `filter` to `take_while` to make clear we stop early i.e., we never proceed after we find *something* that is dirty. --- src/derived.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/derived.rs b/src/derived.rs index cd3eabc4..4b716a89 100644 --- a/src/derived.rs +++ b/src/derived.rs @@ -846,7 +846,7 @@ where let maybe_changed = inputs .iter() .flat_map(|inputs| inputs.iter()) - .filter(|input| input.maybe_changed_since(db, revision)) + .take_while(|input| input.maybe_changed_since(db, revision)) .inspect(|input| { debug!( "{:?}({:?}): input `{:?}` may have changed", @@ -947,7 +947,7 @@ where debug!("sweep({:?}): clearing the table", Q::default()); map_write.clear(); return; - }, + } (DiscardIf::Never, _) | (_, DiscardWhat::Nothing) => return, _ => {} } @@ -1042,7 +1042,7 @@ where MemoInputs::Tracked { inputs } => { let changed_input = inputs .iter() - .filter(|input| input.maybe_changed_since(db, verified_at)) + .take_while(|input| input.maybe_changed_since(db, verified_at)) .next(); if let Some(input) = changed_input { From f9fe9e4f072d1394e1630cd30e8ef439d2d26cf9 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Wed, 13 Mar 2019 05:34:56 -0400 Subject: [PATCH 10/17] add missing `HasQueryGroup` --- components/salsa-macros/src/query_group.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/components/salsa-macros/src/query_group.rs b/components/salsa-macros/src/query_group.rs index 57bdfc79..ebd300b8 100644 --- a/components/salsa-macros/src/query_group.rs +++ b/components/salsa-macros/src/query_group.rs @@ -379,6 +379,7 @@ pub(crate) fn query_group(args: TokenStream, input: TokenStream) -> TokenStream impl salsa::plumbing::QueryFunction for #qt where DB: #trait_name, + DB: salsa::plumbing::HasQueryGroup<#group_struct>, DB: salsa::Database, { fn execute(db: &DB, #key_pattern: >::Key) From 9689d4471b2671e25272f4e70e53183b7b15dfa4 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 22 Mar 2019 04:58:47 -0400 Subject: [PATCH 11/17] revert take_while changes, which were just .. wrong --- src/derived.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/derived.rs b/src/derived.rs index 4b716a89..a18235fb 100644 --- a/src/derived.rs +++ b/src/derived.rs @@ -846,7 +846,7 @@ where let maybe_changed = inputs .iter() .flat_map(|inputs| inputs.iter()) - .take_while(|input| input.maybe_changed_since(db, revision)) + .filter(|input| input.maybe_changed_since(db, revision)) .inspect(|input| { debug!( "{:?}({:?}): input `{:?}` may have changed", @@ -1042,7 +1042,7 @@ where MemoInputs::Tracked { inputs } => { let changed_input = inputs .iter() - .take_while(|input| input.maybe_changed_since(db, verified_at)) + .filter(|input| input.maybe_changed_since(db, verified_at)) .next(); if let Some(input) = changed_input { From c5795a3e5cd775bc1c6819803f122064e02c8876 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 22 Mar 2019 05:13:07 -0400 Subject: [PATCH 12/17] only GC outdated intern keys --- src/interned.rs | 16 +++++++++++-- tests/gc/db.rs | 3 ++- tests/gc/interned.rs | 57 ++++++++++++++++++++++++++++++++++++++++++++ tests/gc/main.rs | 1 + 4 files changed, 74 insertions(+), 3 deletions(-) create mode 100644 tests/gc/interned.rs diff --git a/src/interned.rs b/src/interned.rs index 8a0d8dfa..d7a70deb 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -494,7 +494,20 @@ where map.retain(|key, intern_index| { let discard = match strategy.discard_if { DiscardIf::Never => false, - DiscardIf::Outdated => match values[intern_index.index()] { + + // NB: Interned keys *never* discard keys unless they + // are outdated, regardless of the sweep strategy. This is + // because interned queries are not deterministic; + // if we were to remove a value from the current revision, + // and the query were later executed again, it would not necessarily + // produce the same intern key the second time. This would wreak + // havoc. See the test `discard_during_same_revision` for an example. + // + // Keys that have not (yet) been accessed during this + // revision don't have this problem. Anything + // dependent on them would regard itself as dirty if + // they are removed and also be forced to re-execute. + DiscardIf::Always | DiscardIf::Outdated => match values[intern_index.index()] { InternValue::Present { accessed_at, .. } => accessed_at < revision_now, InternValue::Free { .. } => { @@ -504,7 +517,6 @@ where ); } }, - DiscardIf::Always => true, }; if discard { diff --git a/tests/gc/db.rs b/tests/gc/db.rs index 777e9f09..b81dae8c 100644 --- a/tests/gc/db.rs +++ b/tests/gc/db.rs @@ -1,7 +1,8 @@ use crate::group; +use crate::interned; use crate::log::{HasLog, Log}; -#[salsa::database(group::Gc)] +#[salsa::database(group::Gc, interned::Intern)] #[derive(Default)] pub(crate) struct DatabaseImpl { runtime: salsa::Runtime, diff --git a/tests/gc/interned.rs b/tests/gc/interned.rs new file mode 100644 index 00000000..ed9a3e6c --- /dev/null +++ b/tests/gc/interned.rs @@ -0,0 +1,57 @@ +use crate::db; +use salsa::{Database, SweepStrategy}; + +#[salsa::query_group(Intern)] +pub(crate) trait InternDatabase { + #[salsa::interned] + fn intern_str(&self, x: &'static str) -> u32; + + fn repeat_intern1(&self, x: &'static str) -> u32; + + fn repeat_intern2(&self, x: &'static str) -> u32; +} + +fn repeat_intern1(db: &impl InternDatabase, x: &'static str) -> u32 { + db.intern_str(x) +} + +fn repeat_intern2(db: &impl InternDatabase, x: &'static str) -> u32 { + db.intern_str(x) +} + +/// This test highlights the difference between *interned queries* and +/// other non-input queries -- in particular, their results are not +/// *deterministic*. Therefore, we cannot GC values that were created +/// in the current revision; that might cause us to re-execute the +/// query twice on the same key during the same revision, which could +/// yield different results each time, wreaking havoc. This test +/// exercises precisely that scenario. +#[test] +fn discard_during_same_revision() { + let db = db::DatabaseImpl::default(); + + // This will assign index 0 for "foo". + let foo1a = db.repeat_intern1("foo"); + + // If we are not careful, this would remove the interned key for + // "foo". + db.query(InternStrQuery).sweep( + SweepStrategy::default() + .discard_everything() + .sweep_all_revisions(), + ); + + // This would then reuse index 0 for "bar". + let bar1 = db.intern_str("bar"); + + // And here we would assign index *1* to "foo". + let foo2 = db.repeat_intern2("foo"); + + // But we would still have a cached result, *from the same + // revision*, with the value 0. So that's inconsistent. + let foo1b = db.repeat_intern1("foo"); + + assert_ne!(foo2, bar1); + assert_eq!(foo1a, foo1b); + assert_eq!(foo1b, foo2); +} diff --git a/tests/gc/main.rs b/tests/gc/main.rs index 1cc88349..ff9dedfe 100644 --- a/tests/gc/main.rs +++ b/tests/gc/main.rs @@ -13,5 +13,6 @@ mod db; mod derived_tests; mod discard_values; mod group; +mod interned; mod log; mod shallow_constant_tests; From 791ec3065c1c94d076087888f0a653c6054ae990 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 22 Mar 2019 05:18:32 -0400 Subject: [PATCH 13/17] elaborate a bit more on the GC tests --- tests/gc/interned.rs | 51 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/gc/interned.rs b/tests/gc/interned.rs index ed9a3e6c..5b79c94a 100644 --- a/tests/gc/interned.rs +++ b/tests/gc/interned.rs @@ -1,13 +1,21 @@ use crate::db; use salsa::{Database, SweepStrategy}; +/// Query group for tests for how interned keys interact with GC. #[salsa::query_group(Intern)] pub(crate) trait InternDatabase { + /// A dummy input that can be used to trigger a new revision. + #[salsa::input] + fn dummy(&self) -> (); + + /// Underlying interning query. #[salsa::interned] fn intern_str(&self, x: &'static str) -> u32; + /// This just executes the intern query and returns the result. fn repeat_intern1(&self, x: &'static str) -> u32; + /// Same as `repeat_intern1`. =) fn repeat_intern2(&self, x: &'static str) -> u32; } @@ -55,3 +63,46 @@ fn discard_during_same_revision() { assert_eq!(foo1a, foo1b); assert_eq!(foo1b, foo2); } + +/// This test highlights the difference between *interned queries* and +/// other non-input queries -- in particular, their results are not +/// *deterministic*. Therefore, we cannot GC values that were created +/// in the current revision; that might cause us to re-execute the +/// query twice on the same key during the same revision, which could +/// yield different results each time, wreaking havoc. This test +/// exercises precisely that scenario. +#[test] +fn discard_outdated() { + let mut db = db::DatabaseImpl::default(); + + let foo_from_rev0 = db.repeat_intern1("foo"); + let bar_from_rev0 = db.repeat_intern1("bar"); + + // Trigger a new revision. + db.set_dummy(()); + + // In this revision, we use "bar". + let bar_from_rev1 = db.repeat_intern1("bar"); + + // This should collect "foo". + db.sweep_all(SweepStrategy::discard_outdated()); + + // This should be the same as before the GC, as bar + // is not outdated. + let bar2_from_rev1 = db.repeat_intern1("bar"); + + // This should re-use the index of "foo". + let baz_from_rev1 = db.repeat_intern1("baz"); + + // This should assign the next index to "foo". + let foo_from_rev1 = db.repeat_intern1("foo"); + + assert_eq!(bar_from_rev0, bar_from_rev1); + assert_eq!(bar_from_rev0, bar2_from_rev1); + + assert_eq!(foo_from_rev0, baz_from_rev1); + + assert_ne!(foo_from_rev0, foo_from_rev1); + assert_ne!(foo_from_rev1, bar_from_rev1); + assert_ne!(foo_from_rev1, baz_from_rev1); +} From 6cf1ffd24ab3a85d94cd397b4a78957db56f6c73 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 22 Mar 2019 05:19:46 -0400 Subject: [PATCH 14/17] test reverse lookup after we have reused slots and the like --- tests/gc/interned.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tests/gc/interned.rs b/tests/gc/interned.rs index 5b79c94a..b47d5878 100644 --- a/tests/gc/interned.rs +++ b/tests/gc/interned.rs @@ -105,4 +105,8 @@ fn discard_outdated() { assert_ne!(foo_from_rev0, foo_from_rev1); assert_ne!(foo_from_rev1, bar_from_rev1); assert_ne!(foo_from_rev1, baz_from_rev1); + + assert_eq!(db.lookup_intern_str(foo_from_rev1), "foo"); + assert_eq!(db.lookup_intern_str(bar_from_rev1), "bar"); + assert_eq!(db.lookup_intern_str(baz_from_rev1), "baz"); } From c040b0c6737006deeae82979e95512be6bfb7cee Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 22 Mar 2019 16:24:37 -0400 Subject: [PATCH 15/17] fix gc and volatile tests --- src/derived.rs | 34 +++++++++++++----- tests/gc/db.rs | 3 +- tests/gc/main.rs | 1 + tests/gc/volatile_tests.rs | 72 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 10 deletions(-) create mode 100644 tests/gc/volatile_tests.rs diff --git a/src/derived.rs b/src/derived.rs index a18235fb..3419d3e1 100644 --- a/src/derived.rs +++ b/src/derived.rs @@ -942,15 +942,6 @@ where fn sweep(&self, db: &DB, strategy: SweepStrategy) { let mut map_write = self.map.write(); let revision_now = db.salsa_runtime().current_revision(); - match (strategy.discard_if, strategy.discard_what) { - (DiscardIf::Always, DiscardWhat::Everything) => { - debug!("sweep({:?}): clearing the table", Q::default()); - map_write.clear(); - return; - } - (DiscardIf::Never, _) | (_, DiscardWhat::Nothing) => return, - _ => {} - } map_write.retain(|key, query_state| { match query_state { // Leave stuff that is currently being computed -- the @@ -972,6 +963,19 @@ where revision_now ); + // Check if this memo read something "untracked" + // -- meaning non-deterministic. In this case, we + // can only collect "outdated" data that wasn't + // used in the current revision. This is because + // if we collected something from the current + // revision, we might wind up re-executing the + // query later in the revision and getting a + // distinct result. + let is_volatile = match memo.inputs { + MemoInputs::Untracked => true, + _ => false, + }; + // Since we don't acquire a query lock in this // method, it *is* possible for the revision to // change while we are executing. However, it is @@ -982,7 +986,19 @@ where assert!(memo.verified_at <= revision_now); match strategy.discard_if { DiscardIf::Never => unreachable!(), + + // If we are only discarding outdated things, + // and this is not outdated, keep it. DiscardIf::Outdated if memo.verified_at == revision_now => true, + + // As explained on the `is_volatile` variable + // definition, if this is a volatile entry, we + // can't discard it unless it is outdated. + DiscardIf::Always if is_volatile && memo.verified_at == revision_now => { + true + } + + // Otherwise, we can discard -- discard whatever the user requested. DiscardIf::Outdated | DiscardIf::Always => match strategy.discard_what { DiscardWhat::Nothing => unreachable!(), DiscardWhat::Values => { diff --git a/tests/gc/db.rs b/tests/gc/db.rs index b81dae8c..b6aaba57 100644 --- a/tests/gc/db.rs +++ b/tests/gc/db.rs @@ -1,8 +1,9 @@ use crate::group; use crate::interned; use crate::log::{HasLog, Log}; +use crate::volatile_tests; -#[salsa::database(group::Gc, interned::Intern)] +#[salsa::database(group::Gc, interned::Intern, volatile_tests::Volatile)] #[derive(Default)] pub(crate) struct DatabaseImpl { runtime: salsa::Runtime, diff --git a/tests/gc/main.rs b/tests/gc/main.rs index ff9dedfe..9bfa6b30 100644 --- a/tests/gc/main.rs +++ b/tests/gc/main.rs @@ -16,3 +16,4 @@ mod group; mod interned; mod log; mod shallow_constant_tests; +mod volatile_tests; diff --git a/tests/gc/volatile_tests.rs b/tests/gc/volatile_tests.rs new file mode 100644 index 00000000..9953fdbf --- /dev/null +++ b/tests/gc/volatile_tests.rs @@ -0,0 +1,72 @@ +use crate::db; +use salsa::{Database, SweepStrategy}; +use std::sync::atomic::{AtomicU32, Ordering}; +use std::sync::Arc; + +/// Query group for tests for how interned keys interact with GC. +#[salsa::query_group(Volatile)] +pub(crate) trait VolatileDatabase { + #[salsa::input] + fn atomic_cell(&self) -> Arc; + + /// Underlying volatile query. + #[salsa::volatile] + fn volatile(&self) -> u32; + + /// This just executes the intern query and returns the result. + fn repeat1(&self) -> u32; + + /// Same as `repeat_intern1`. =) + fn repeat2(&self) -> u32; +} + +fn volatile(db: &impl VolatileDatabase) -> u32 { + db.atomic_cell().load(Ordering::SeqCst) +} + +fn repeat1(db: &impl VolatileDatabase) -> u32 { + db.volatile() +} + +fn repeat2(db: &impl VolatileDatabase) -> u32 { + db.volatile() +} + +#[test] +fn consistency_no_gc() { + let mut db = db::DatabaseImpl::default(); + + let cell = Arc::new(AtomicU32::new(22)); + + db.set_atomic_cell(cell.clone()); + + let v1 = db.repeat1(); + + cell.store(23, Ordering::SeqCst); + + let v2 = db.repeat2(); + + assert_eq!(v1, v2); +} + +#[test] +fn consistency_with_gc() { + let mut db = db::DatabaseImpl::default(); + + let cell = Arc::new(AtomicU32::new(22)); + + db.set_atomic_cell(cell.clone()); + + let v1 = db.repeat1(); + + cell.store(23, Ordering::SeqCst); + db.query(VolatileQuery).sweep( + SweepStrategy::default() + .discard_everything() + .sweep_all_revisions(), + ); + + let v2 = db.repeat2(); + + assert_eq!(v1, v2); +} From f0d2b964e274c55960bd373909b9d5b99cec03b3 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 22 Mar 2019 18:40:35 -0400 Subject: [PATCH 16/17] bump syn to 0.15.29 (older versions didn't have the `syn::Result` type, it seems?) --- components/salsa-macros/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/components/salsa-macros/Cargo.toml b/components/salsa-macros/Cargo.toml index 6cb7f3af..90f248a2 100644 --- a/components/salsa-macros/Cargo.toml +++ b/components/salsa-macros/Cargo.toml @@ -15,5 +15,5 @@ proc-macro = true heck = "0.3" proc-macro2 = "0.4" quote = "0.6" -syn = { version = "0.15", features = ["full", "extra-traits"] } +syn = { version = "0.15.29", features = ["full", "extra-traits"] } From 5c7e2fee0910d0a6501b14eb6fd2d9add2ae1702 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 25 Mar 2019 14:40:32 -0400 Subject: [PATCH 17/17] s/AtomicU32/AtomicUsize/ in tests --- tests/gc/volatile_tests.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/tests/gc/volatile_tests.rs b/tests/gc/volatile_tests.rs index 9953fdbf..69c47954 100644 --- a/tests/gc/volatile_tests.rs +++ b/tests/gc/volatile_tests.rs @@ -1,34 +1,34 @@ use crate::db; use salsa::{Database, SweepStrategy}; -use std::sync::atomic::{AtomicU32, Ordering}; +use std::sync::atomic::{AtomicUsize, Ordering}; use std::sync::Arc; /// Query group for tests for how interned keys interact with GC. #[salsa::query_group(Volatile)] pub(crate) trait VolatileDatabase { #[salsa::input] - fn atomic_cell(&self) -> Arc; + fn atomic_cell(&self) -> Arc; /// Underlying volatile query. #[salsa::volatile] - fn volatile(&self) -> u32; + fn volatile(&self) -> usize; /// This just executes the intern query and returns the result. - fn repeat1(&self) -> u32; + fn repeat1(&self) -> usize; /// Same as `repeat_intern1`. =) - fn repeat2(&self) -> u32; + fn repeat2(&self) -> usize; } -fn volatile(db: &impl VolatileDatabase) -> u32 { +fn volatile(db: &impl VolatileDatabase) -> usize { db.atomic_cell().load(Ordering::SeqCst) } -fn repeat1(db: &impl VolatileDatabase) -> u32 { +fn repeat1(db: &impl VolatileDatabase) -> usize { db.volatile() } -fn repeat2(db: &impl VolatileDatabase) -> u32 { +fn repeat2(db: &impl VolatileDatabase) -> usize { db.volatile() } @@ -36,7 +36,7 @@ fn repeat2(db: &impl VolatileDatabase) -> u32 { fn consistency_no_gc() { let mut db = db::DatabaseImpl::default(); - let cell = Arc::new(AtomicU32::new(22)); + let cell = Arc::new(AtomicUsize::new(22)); db.set_atomic_cell(cell.clone()); @@ -53,7 +53,7 @@ fn consistency_no_gc() { fn consistency_with_gc() { let mut db = db::DatabaseImpl::default(); - let cell = Arc::new(AtomicU32::new(22)); + let cell = Arc::new(AtomicUsize::new(22)); db.set_atomic_cell(cell.clone());