From 296d33aae75bee91037277064e01692596a37a5d Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 08:53:13 -0400 Subject: [PATCH 1/9] factor out `ChangedAt` to record when something changed --- src/input.rs | 11 ++++----- src/memoized.rs | 60 +++++++++++++++++++++++++++++++------------------ src/runtime.rs | 29 +++++++++++++++++++----- src/volatile.rs | 6 ++--- 4 files changed, 71 insertions(+), 35 deletions(-) diff --git a/src/input.rs b/src/input.rs index b957d98a..a3d95323 100644 --- a/src/input.rs +++ b/src/input.rs @@ -1,3 +1,4 @@ +use crate::runtime::ChangedAt; use crate::runtime::QueryDescriptorSet; use crate::runtime::Revision; use crate::runtime::StampedValue; @@ -66,7 +67,7 @@ where Ok(StampedValue { value: ::default(), - changed_at: Revision::ZERO, + changed_at: ChangedAt::Revision(Revision::ZERO), }) } } @@ -109,10 +110,10 @@ where map_read .get(key) .map(|v| v.changed_at) - .unwrap_or(Revision::ZERO) + .unwrap_or(ChangedAt::Revision(Revision::ZERO)) }; - changed_at > revision + changed_at.changed_since(revision) } } @@ -131,7 +132,7 @@ where // racing with somebody else to modify this same cell. // (Otherwise, someone else might write a *newer* revision // into the same cell while we block on the lock.) - let changed_at = db.salsa_runtime().increment_revision(); + let changed_at = ChangedAt::Revision(db.salsa_runtime().increment_revision()); map_write.insert(key, StampedValue { value, changed_at }); } @@ -150,7 +151,7 @@ where // Unlike with `set`, here we use the **current revision** and // do not create a new one. - let changed_at = db.salsa_runtime().current_revision(); + let changed_at = ChangedAt::Revision(db.salsa_runtime().current_revision()); map_write.insert(key, StampedValue { value, changed_at }); } diff --git a/src/memoized.rs b/src/memoized.rs index fbcb15fb..fb8339b6 100644 --- a/src/memoized.rs +++ b/src/memoized.rs @@ -1,3 +1,4 @@ +use crate::runtime::ChangedAt; use crate::runtime::QueryDescriptorSet; use crate::runtime::Revision; use crate::runtime::StampedValue; @@ -91,7 +92,7 @@ where { /// Last time the value has actually changed. /// changed_at can be less than verified_at. - changed_at: Revision, + changed_at: ChangedAt, /// The result of the query, if we decide to memoize it. value: Option, @@ -184,24 +185,18 @@ where // first things first, let's walk over each of our previous // inputs and check whether they are out of date. if let Some(QueryState::Memoized(old_memo)) = &mut old_value { - if old_memo.value.is_some() { - if old_memo - .inputs - .iter() - .all(|old_input| !old_input.maybe_changed_since(db, old_memo.changed_at)) - { - debug!("{:?}({:?}): inputs still valid", Q::default(), key); - // If none of out inputs have changed since the last time we refreshed - // our value, then our value must still be good. We'll just patch - // the verified-at date and re-use it. - old_memo.verified_at = revision_now; - let value = old_memo.value.clone().unwrap(); - let changed_at = old_memo.changed_at; + if old_memo.validate_memoized_value(db) { + debug!("{:?}({:?}): inputs still valid", Q::default(), key); + // If none of out inputs have changed since the last time we refreshed + // our value, then our value must still be good. We'll just patch + // the verified-at date and re-use it. + old_memo.verified_at = revision_now; + let value = old_memo.value.clone().unwrap(); + let changed_at = old_memo.changed_at; - let mut map_write = self.map.write(); - self.overwrite_placeholder(&mut map_write, key, old_value.unwrap()); - return Ok(StampedValue { value, changed_at }); - } + let mut map_write = self.map.write(); + self.overwrite_placeholder(&mut map_write, key, old_value.unwrap()); + return Ok(StampedValue { value, changed_at }); } } @@ -318,14 +313,14 @@ where // If our memo is still up to date, then check if we've // changed since the revision. if memo.verified_at == revision_now { - return memo.changed_at > revision; + return memo.changed_at.changed_since(revision); } if memo.value.is_some() { // Otherwise, if we cache values, fall back to the full read to compute the result. drop(memo); drop(map_read); return match self.read(db, key, descriptor) { - Ok(v) => v.changed_at > revision, + Ok(v) => v.changed_at.changed_since(revision), Err(CycleDetected) => true, }; } @@ -370,7 +365,8 @@ where let mut map_write = self.map.write(); - let changed_at = db.salsa_runtime().current_revision(); + let current_revision = db.salsa_runtime().current_revision(); + let changed_at = ChangedAt::Revision(current_revision); map_write.insert( key, @@ -378,8 +374,28 @@ where value: Some(value), changed_at, inputs: QueryDescriptorSet::new(), - verified_at: changed_at, + verified_at: current_revision, }), ); } } + +impl Memo +where + Q: QueryFunction, + DB: Database, +{ + fn validate_memoized_value(&self, db: &DB) -> bool { + // If we don't have a memoized value, nothing to validate. + if !self.value.is_some() { + return false; + } + + match self.changed_at { + ChangedAt::Revision(revision) => self + .inputs + .iter() + .all(|old_input| !old_input.maybe_changed_since(db, revision)), + } + } +} diff --git a/src/runtime.rs b/src/runtime.rs index f92456ea..4423872e 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -136,7 +136,7 @@ where /// - `descriptor`: the query whose result was read /// - `changed_revision`: the last revision in which the result of that /// query had changed - crate fn report_query_read(&self, descriptor: &DB::QueryDescriptor, changed_at: Revision) { + crate fn report_query_read(&self, descriptor: &DB::QueryDescriptor, changed_at: ChangedAt) { if let Some(top_query) = self.local_state.borrow_mut().query_stack.last_mut() { top_query.add_read(descriptor, changed_at); } @@ -178,7 +178,7 @@ struct ActiveQuery { descriptor: DB::QueryDescriptor, /// Records the maximum revision where any subquery changed - changed_at: Revision, + changed_at: ChangedAt, /// Each subquery subqueries: QueryDescriptorSet, @@ -188,12 +188,12 @@ impl ActiveQuery { fn new(descriptor: DB::QueryDescriptor) -> Self { ActiveQuery { descriptor, - changed_at: Revision::ZERO, + changed_at: ChangedAt::Revision(Revision::ZERO), subqueries: QueryDescriptorSet::new(), } } - fn add_read(&mut self, subquery: &DB::QueryDescriptor, changed_at: Revision) { + fn add_read(&mut self, subquery: &DB::QueryDescriptor, changed_at: ChangedAt) { self.subqueries.insert(subquery.clone()); self.changed_at = self.changed_at.max(changed_at); } @@ -214,6 +214,25 @@ impl std::fmt::Debug for Revision { } } +/// Records when a stamped value changed. +/// +/// Note: the order of variants is significant. We sometimes use `max` +/// for example to find the "most recent revision" when something +/// changed. +#[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] +pub enum ChangedAt { + Revision(Revision), +} + +impl ChangedAt { + /// True if this value has changed after `revision`. + pub fn changed_since(self, revision: Revision) -> bool { + match self { + ChangedAt::Revision(r) => r > revision, + } + } +} + /// An insertion-order-preserving set of queries. Used to track the /// inputs accessed during query execution. crate struct QueryDescriptorSet { @@ -249,5 +268,5 @@ impl QueryDescriptorSet { #[derive(Clone, Debug)] crate struct StampedValue { crate value: V, - crate changed_at: Revision, + crate changed_at: ChangedAt, } diff --git a/src/volatile.rs b/src/volatile.rs index 6fa61729..be70e38d 100644 --- a/src/volatile.rs +++ b/src/volatile.rs @@ -1,3 +1,4 @@ +use crate::runtime::ChangedAt; use crate::runtime::Revision; use crate::runtime::StampedValue; use crate::CycleDetected; @@ -68,10 +69,9 @@ where let was_in_progress = self.in_progress.lock().remove(key); assert!(was_in_progress); - let revision_now = db.salsa_runtime().current_revision(); + let changed_at = ChangedAt::Revision(db.salsa_runtime().current_revision()); - db.salsa_runtime() - .report_query_read(descriptor, revision_now); + db.salsa_runtime().report_query_read(descriptor, changed_at); Ok(value) } From 30236cc11000712b7b40ac9915630e79cb037b33 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 09:27:59 -0400 Subject: [PATCH 2/9] introduce a new helper, `verify_inputs` --- src/memoized.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/memoized.rs b/src/memoized.rs index fb8339b6..e5216c7e 100644 --- a/src/memoized.rs +++ b/src/memoized.rs @@ -185,7 +185,7 @@ where // first things first, let's walk over each of our previous // inputs and check whether they are out of date. if let Some(QueryState::Memoized(old_memo)) = &mut old_value { - if old_memo.validate_memoized_value(db) { + if old_memo.verify_memoized_value(db) { debug!("{:?}({:?}): inputs still valid", Q::default(), key); // If none of out inputs have changed since the last time we refreshed // our value, then our value must still be good. We'll just patch @@ -337,11 +337,7 @@ where _ => unreachable!(), }; - if memo - .inputs - .iter() - .all(|old_input| !old_input.maybe_changed_since(db, memo.verified_at)) - { + if memo.verify_inputs(db) { memo.verified_at = revision_now; self.overwrite_placeholder(&mut self.map.write(), key, QueryState::Memoized(memo)); return false; @@ -385,12 +381,16 @@ where Q: QueryFunction, DB: Database, { - fn validate_memoized_value(&self, db: &DB) -> bool { + fn verify_memoized_value(&self, db: &DB) -> bool { // If we don't have a memoized value, nothing to validate. if !self.value.is_some() { return false; } + self.verify_inputs(db) + } + + fn verify_inputs(&self, db: &DB) -> bool { match self.changed_at { ChangedAt::Revision(revision) => self .inputs From 1e6bfc7fdd908937372e8093a87c33f2c4d95844 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 10:26:33 -0400 Subject: [PATCH 3/9] introduce a callback into `execute_query_implementation` --- src/memoized.rs | 9 ++++++--- src/runtime.rs | 13 +++++-------- src/volatile.rs | 5 ++++- 3 files changed, 15 insertions(+), 12 deletions(-) diff --git a/src/memoized.rs b/src/memoized.rs index e5216c7e..10294d8c 100644 --- a/src/memoized.rs +++ b/src/memoized.rs @@ -202,9 +202,12 @@ where // Query was not previously executed, or value is potentially // stale, or value is absent. Let's execute! - let (mut stamped_value, inputs) = db - .salsa_runtime() - .execute_query_implementation::(db, descriptor, key); + let (mut stamped_value, inputs) = + db.salsa_runtime() + .execute_query_implementation(db, descriptor, || { + debug!("{:?}({:?}): executing query", Q::default(), key); + Q::execute(db, key.clone()) + }); // We assume that query is side-effect free -- that is, does // not mutate the "inputs" to the query system. Sanity check diff --git a/src/runtime.rs b/src/runtime.rs index 4423872e..a763896b 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -88,16 +88,13 @@ where result } - crate fn execute_query_implementation( + crate fn execute_query_implementation( &self, db: &DB, descriptor: &DB::QueryDescriptor, - key: &Q::Key, - ) -> (StampedValue, QueryDescriptorSet) - where - Q: QueryFunction, - { - debug!("{:?}({:?}): executing query", Q::default(), key); + execute: impl FnOnce() -> V, + ) -> (StampedValue, QueryDescriptorSet) { + debug!("{:?}: execute_query_implementation invoked", descriptor); // Push the active query onto the stack. let push_len = { @@ -109,7 +106,7 @@ where }; // Execute user's code, accumulating inputs etc. - let value = Q::execute(db, key.clone()); + let value = execute(); // Extract accumulated inputs. let ActiveQuery { diff --git a/src/volatile.rs b/src/volatile.rs index be70e38d..2364a663 100644 --- a/src/volatile.rs +++ b/src/volatile.rs @@ -64,7 +64,10 @@ where _inputs, ) = db .salsa_runtime() - .execute_query_implementation::(db, descriptor, key); + .execute_query_implementation(db, descriptor, || { + debug!("{:?}({:?}): executing query", Q::default(), key); + Q::execute(db, key.clone()) + }); let was_in_progress = self.in_progress.lock().remove(key); assert!(was_in_progress); From 2d6e454638a5eb4739bafd695ee8cdf23ddbe513 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 11:04:25 -0400 Subject: [PATCH 4/9] add the idea of "untracked reads" and use it to optimize volatile Now we won't be tracking the inputs to a volatile query, since we don't care about them anyway. --- src/memoized.rs | 15 ++++++++----- src/runtime.rs | 58 ++++++++++++++++++++++++++++++++++--------------- src/volatile.rs | 23 ++++++++++---------- 3 files changed, 61 insertions(+), 35 deletions(-) diff --git a/src/memoized.rs b/src/memoized.rs index 10294d8c..9b408f14 100644 --- a/src/memoized.rs +++ b/src/memoized.rs @@ -204,7 +204,7 @@ where // stale, or value is absent. Let's execute! let (mut stamped_value, inputs) = db.salsa_runtime() - .execute_query_implementation(db, descriptor, || { + .execute_query_implementation(descriptor, || { debug!("{:?}({:?}): executing query", Q::default(), key); Q::execute(db, key.clone()) }); @@ -372,7 +372,7 @@ where QueryState::Memoized(Memo { value: Some(value), changed_at, - inputs: QueryDescriptorSet::new(), + inputs: QueryDescriptorSet::default(), verified_at: current_revision, }), ); @@ -395,10 +395,13 @@ where fn verify_inputs(&self, db: &DB) -> bool { match self.changed_at { - ChangedAt::Revision(revision) => self - .inputs - .iter() - .all(|old_input| !old_input.maybe_changed_since(db, revision)), + ChangedAt::Revision(revision) => match &self.inputs { + QueryDescriptorSet::Tracked(inputs) => inputs + .iter() + .all(|old_input| !old_input.maybe_changed_since(db, revision)), + + QueryDescriptorSet::Untracked => false, + }, } } } diff --git a/src/runtime.rs b/src/runtime.rs index a763896b..604cec31 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -90,7 +90,6 @@ where crate fn execute_query_implementation( &self, - db: &DB, descriptor: &DB::QueryDescriptor, execute: impl FnOnce() -> V, ) -> (StampedValue, QueryDescriptorSet) { @@ -139,6 +138,13 @@ where } } + crate fn report_untracked_read(&self) { + if let Some(top_query) = self.local_state.borrow_mut().query_stack.last_mut() { + let changed_at = ChangedAt::Revision(self.current_revision()); + top_query.add_untracked_read(changed_at); + } + } + /// Obviously, this should be user configurable at some point. crate fn report_unexpected_cycle(&self, descriptor: DB::QueryDescriptor) -> ! { let local_state = self.local_state.borrow(); @@ -186,7 +192,7 @@ impl ActiveQuery { ActiveQuery { descriptor, changed_at: ChangedAt::Revision(Revision::ZERO), - subqueries: QueryDescriptorSet::new(), + subqueries: QueryDescriptorSet::default(), } } @@ -194,6 +200,11 @@ impl ActiveQuery { self.subqueries.insert(subquery.clone()); self.changed_at = self.changed_at.max(changed_at); } + + fn add_untracked_read(&mut self, changed_at: ChangedAt) { + self.subqueries.insert_untracked(); + self.changed_at = self.changed_at.max(changed_at); + } } #[derive(Copy, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] @@ -232,33 +243,44 @@ impl ChangedAt { /// An insertion-order-preserving set of queries. Used to track the /// inputs accessed during query execution. -crate struct QueryDescriptorSet { - set: FxIndexSet, +crate enum QueryDescriptorSet { + /// All reads were to tracked things: + Tracked(FxIndexSet), + + /// Some reads to an untracked thing: + Untracked, } impl std::fmt::Debug for QueryDescriptorSet { fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - std::fmt::Debug::fmt(&self.set, fmt) + match self { + QueryDescriptorSet::Tracked(set) => std::fmt::Debug::fmt(set, fmt), + QueryDescriptorSet::Untracked => write!(fmt, "Untracked"), + } + } +} + +impl Default for QueryDescriptorSet { + fn default() -> Self { + QueryDescriptorSet::Tracked(FxIndexSet::default()) } } impl QueryDescriptorSet { - crate fn new() -> Self { - QueryDescriptorSet { - set: FxIndexSet::default(), + /// Add `descriptor` to the set. Returns true if `descriptor` is + /// newly added and false if `descriptor` was already a member. + fn insert(&mut self, descriptor: DB::QueryDescriptor) { + match self { + QueryDescriptorSet::Tracked(set) => { + set.insert(descriptor); + } + + QueryDescriptorSet::Untracked => {} } } - /// Add `descriptor` to the set. Returns true if `descriptor` is - /// newly added and false if `descriptor` was already a member. - fn insert(&mut self, descriptor: DB::QueryDescriptor) -> bool { - self.set.insert(descriptor) - } - - /// Iterate over all queries in the set, in the order of their - /// first insertion. - pub fn iter(&self) -> impl Iterator { - self.set.iter() + fn insert_untracked(&mut self) { + *self = QueryDescriptorSet::Untracked; } } diff --git a/src/volatile.rs b/src/volatile.rs index 2364a663..091048a6 100644 --- a/src/volatile.rs +++ b/src/volatile.rs @@ -1,4 +1,5 @@ use crate::runtime::ChangedAt; +use crate::runtime::QueryDescriptorSet; use crate::runtime::Revision; use crate::runtime::StampedValue; use crate::CycleDetected; @@ -56,24 +57,24 @@ where return Err(CycleDetected); } - let ( - StampedValue { - value, - changed_at: _, - }, - _inputs, - ) = db - .salsa_runtime() - .execute_query_implementation(db, descriptor, || { + let runtime = db.salsa_runtime(); + + let (StampedValue { value, changed_at }, inputs) = + runtime.execute_query_implementation(descriptor, || { debug!("{:?}({:?}): executing query", Q::default(), key); + runtime.report_untracked_read(); Q::execute(db, key.clone()) }); + assert!(changed_at == ChangedAt::Revision(db.salsa_runtime().current_revision())); + assert!(match inputs { + QueryDescriptorSet::Untracked => true, + _ => false, + }); + let was_in_progress = self.in_progress.lock().remove(key); assert!(was_in_progress); - let changed_at = ChangedAt::Revision(db.salsa_runtime().current_revision()); - db.salsa_runtime().report_query_read(descriptor, changed_at); Ok(value) From 5ad0049b9fab6dac30e96cf6221db570767c4bb3 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 12:14:09 -0400 Subject: [PATCH 5/9] merge volatile and memoized queries --- src/lib.rs | 3 +- src/memoized.rs | 51 ++++++++++++++++++++++--- src/volatile.rs | 99 ------------------------------------------------- 3 files changed, 46 insertions(+), 107 deletions(-) delete mode 100644 src/volatile.rs diff --git a/src/lib.rs b/src/lib.rs index 4a5b5130..5a759640 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -19,7 +19,6 @@ use std::hash::Hash; pub mod input; pub mod memoized; pub mod runtime; -pub mod volatile; /// The base trait which your "query context" must implement. Gives /// access to the salsa runtime, which you must embed into your query @@ -428,7 +427,7 @@ macro_rules! query_group { ( @storage_ty[$DB:ident, $Self:ident, volatile] ) => { - $crate::volatile::VolatileStorage<$DB, $Self> + $crate::memoized::VolatileStorage<$DB, $Self> }; ( diff --git a/src/memoized.rs b/src/memoized.rs index 9b408f14..294a133e 100644 --- a/src/memoized.rs +++ b/src/memoized.rs @@ -31,6 +31,11 @@ pub type MemoizedStorage = WeakMemoizedStorage /// storage requirements. pub type DependencyStorage = WeakMemoizedStorage; +/// "Dependency" queries just track their dependencies and not the +/// actual value (which they produce on demand). This lessens the +/// storage requirements. +pub type VolatileStorage = WeakMemoizedStorage; + pub struct WeakMemoizedStorage where Q: QueryFunction, @@ -47,6 +52,8 @@ where DB: Database, { fn should_memoize_value(key: &Q::Key) -> bool; + + fn is_volatile(key: &Q::Key) -> bool; } pub enum AlwaysMemoizeValue {} @@ -58,6 +65,10 @@ where fn should_memoize_value(_key: &Q::Key) -> bool { true } + + fn is_volatile(_key: &Q::Key) -> bool { + false + } } pub enum NeverMemoizeValue {} @@ -69,6 +80,25 @@ where fn should_memoize_value(_key: &Q::Key) -> bool { false } + + fn is_volatile(_key: &Q::Key) -> bool { + false + } +} + +pub enum VolatileValue {} +impl MemoizationPolicy for VolatileValue +where + Q: QueryFunction, + DB: Database, +{ + fn should_memoize_value(_key: &Q::Key) -> bool { + false + } + + fn is_volatile(_key: &Q::Key) -> bool { + true + } } /// Defines the "current state" of query's memoized results. @@ -97,6 +127,7 @@ where /// The result of the query, if we decide to memoize it. value: Option, + /// The inputs that went into our query, if we are tracking them. inputs: QueryDescriptorSet, /// Last time that we checked our inputs to see if they have @@ -202,12 +233,16 @@ where // Query was not previously executed, or value is potentially // stale, or value is absent. Let's execute! - let (mut stamped_value, inputs) = - db.salsa_runtime() - .execute_query_implementation(descriptor, || { - debug!("{:?}({:?}): executing query", Q::default(), key); - Q::execute(db, key.clone()) - }); + let runtime = db.salsa_runtime(); + let (mut stamped_value, inputs) = runtime.execute_query_implementation(descriptor, || { + debug!("{:?}({:?}): executing query", Q::default(), key); + + if self.is_volatile(key) { + runtime.report_untracked_read(); + } + + Q::execute(db, key.clone()) + }); // We assume that query is side-effect free -- that is, does // not mutate the "inputs" to the query system. Sanity check @@ -270,6 +305,10 @@ where fn should_memoize_value(&self, key: &Q::Key) -> bool { MP::should_memoize_value(key) } + + fn is_volatile(&self, key: &Q::Key) -> bool { + MP::is_volatile(key) + } } impl QueryStorageOps for WeakMemoizedStorage diff --git a/src/volatile.rs b/src/volatile.rs deleted file mode 100644 index 091048a6..00000000 --- a/src/volatile.rs +++ /dev/null @@ -1,99 +0,0 @@ -use crate::runtime::ChangedAt; -use crate::runtime::QueryDescriptorSet; -use crate::runtime::Revision; -use crate::runtime::StampedValue; -use crate::CycleDetected; -use crate::Database; -use crate::QueryFunction; -use crate::QueryStorageOps; -use crate::QueryTable; -use log::debug; -use parking_lot::Mutex; -use rustc_hash::FxHashSet; -use std::any::Any; -use std::cell::RefCell; -use std::collections::hash_map::Entry; -use std::fmt::Debug; -use std::fmt::Display; -use std::fmt::Write; -use std::hash::Hash; - -/// Volatile Storage is just **always** considered dirty. Any time you -/// ask for the result of such a query, it is recomputed. -pub struct VolatileStorage -where - Q: QueryFunction, - DB: Database, -{ - /// We don't store the results of volatile queries, - /// but we track in-progress set to detect cycles. - in_progress: Mutex>, -} - -impl Default for VolatileStorage -where - Q: QueryFunction, - DB: Database, -{ - fn default() -> Self { - VolatileStorage { - in_progress: Mutex::new(FxHashSet::default()), - } - } -} - -impl QueryStorageOps for VolatileStorage -where - Q: QueryFunction, - DB: Database, -{ - fn try_fetch<'q>( - &self, - db: &'q DB, - key: &Q::Key, - descriptor: &DB::QueryDescriptor, - ) -> Result { - if !self.in_progress.lock().insert(key.clone()) { - return Err(CycleDetected); - } - - let runtime = db.salsa_runtime(); - - let (StampedValue { value, changed_at }, inputs) = - runtime.execute_query_implementation(descriptor, || { - debug!("{:?}({:?}): executing query", Q::default(), key); - runtime.report_untracked_read(); - Q::execute(db, key.clone()) - }); - - assert!(changed_at == ChangedAt::Revision(db.salsa_runtime().current_revision())); - assert!(match inputs { - QueryDescriptorSet::Untracked => true, - _ => false, - }); - - let was_in_progress = self.in_progress.lock().remove(key); - assert!(was_in_progress); - - db.salsa_runtime().report_query_read(descriptor, changed_at); - - Ok(value) - } - - fn maybe_changed_since( - &self, - _db: &'q DB, - revision: Revision, - key: &Q::Key, - _descriptor: &DB::QueryDescriptor, - ) -> bool { - debug!( - "{:?}({:?})::maybe_changed_since(revision={:?}) ==> true (volatile)", - Q::default(), - key, - revision, - ); - - true - } -} From 7c65d07ea6e77d8397bba0362e21da612f79be93 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 12:15:33 -0400 Subject: [PATCH 6/9] rename from WeakMemoizedStorage to DerivedStorage --- src/{memoized.rs => derived.rs} | 20 +++++++++++--------- src/lib.rs | 8 ++++---- 2 files changed, 15 insertions(+), 13 deletions(-) rename src/{memoized.rs => derived.rs} (95%) diff --git a/src/memoized.rs b/src/derived.rs similarity index 95% rename from src/memoized.rs rename to src/derived.rs index 294a133e..ed070a44 100644 --- a/src/memoized.rs +++ b/src/derived.rs @@ -24,19 +24,21 @@ use std::marker::PhantomData; /// Memoized queries store the result plus a list of the other queries /// that they invoked. This means we can avoid recomputing them when /// none of those inputs have changed. -pub type MemoizedStorage = WeakMemoizedStorage; +pub type MemoizedStorage = DerivedStorage; /// "Dependency" queries just track their dependencies and not the /// actual value (which they produce on demand). This lessens the /// storage requirements. -pub type DependencyStorage = WeakMemoizedStorage; +pub type DependencyStorage = DerivedStorage; /// "Dependency" queries just track their dependencies and not the /// actual value (which they produce on demand). This lessens the /// storage requirements. -pub type VolatileStorage = WeakMemoizedStorage; +pub type VolatileStorage = DerivedStorage; -pub struct WeakMemoizedStorage +/// Handles storage where the value is 'derived' by executing a +/// function (in contrast to "inputs"). +pub struct DerivedStorage where Q: QueryFunction, DB: Database, @@ -138,21 +140,21 @@ where verified_at: Revision, } -impl Default for WeakMemoizedStorage +impl Default for DerivedStorage where Q: QueryFunction, DB: Database, MP: MemoizationPolicy, { fn default() -> Self { - WeakMemoizedStorage { + DerivedStorage { map: RwLock::new(FxHashMap::default()), policy: PhantomData, } } } -impl WeakMemoizedStorage +impl DerivedStorage where Q: QueryFunction, DB: Database, @@ -311,7 +313,7 @@ where } } -impl QueryStorageOps for WeakMemoizedStorage +impl QueryStorageOps for DerivedStorage where Q: QueryFunction, DB: Database, @@ -392,7 +394,7 @@ where } } -impl UncheckedMutQueryStorageOps for WeakMemoizedStorage +impl UncheckedMutQueryStorageOps for DerivedStorage where Q: QueryFunction, DB: Database, diff --git a/src/lib.rs b/src/lib.rs index 5a759640..ec136f41 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -16,8 +16,8 @@ use std::fmt::Display; use std::fmt::Write; use std::hash::Hash; +pub mod derived; pub mod input; -pub mod memoized; pub mod runtime; /// The base trait which your "query context" must implement. Gives @@ -421,19 +421,19 @@ macro_rules! query_group { ( @storage_ty[$DB:ident, $Self:ident, memoized] ) => { - $crate::memoized::MemoizedStorage<$DB, $Self> + $crate::derived::MemoizedStorage<$DB, $Self> }; ( @storage_ty[$DB:ident, $Self:ident, volatile] ) => { - $crate::memoized::VolatileStorage<$DB, $Self> + $crate::derived::VolatileStorage<$DB, $Self> }; ( @storage_ty[$DB:ident, $Self:ident, dependencies] ) => { - $crate::memoized::DependencyStorage<$DB, $Self> + $crate::derived::DependencyStorage<$DB, $Self> }; ( From c93868c9dcb6f97e9602c1c2953e26b3f119cfda Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 12:18:16 -0400 Subject: [PATCH 7/9] make volatile queries memoize This ensures consistency of results. --- src/derived.rs | 7 ++++- tests/incremental/memoized_volatile.rs | 9 +++--- tests/storage_varieties/main.rs | 1 + tests/storage_varieties/queries.rs | 2 +- tests/storage_varieties/tests.rs | 43 +++++++++++++++++--------- 5 files changed, 42 insertions(+), 20 deletions(-) diff --git a/src/derived.rs b/src/derived.rs index ed070a44..9c15ed19 100644 --- a/src/derived.rs +++ b/src/derived.rs @@ -95,7 +95,12 @@ where DB: Database, { fn should_memoize_value(_key: &Q::Key) -> bool { - false + // Why memoize? Well, if the "volatile" value really is + // constantly changing, we still want to capture its value + // until the next revision is triggered and ensure it doesn't + // change -- otherwise the system gets into an inconsistent + // state where the same query reports back different values. + true } fn is_volatile(_key: &Q::Key) -> bool { diff --git a/tests/incremental/memoized_volatile.rs b/tests/incremental/memoized_volatile.rs index 2864ddf3..36bd3593 100644 --- a/tests/incremental/memoized_volatile.rs +++ b/tests/incremental/memoized_volatile.rs @@ -38,10 +38,11 @@ fn volatile(db: &impl MemoizedVolatileContext, (): ()) -> usize { fn volatile_x2() { let query = TestContextImpl::default(); - // Invoking volatile twice will simply execute twice. + // Invoking volatile twice doesn't execute twice, because volatile + // queries are memoized by default. query.volatile(()); query.volatile(()); - query.assert_log(&["Volatile invoked", "Volatile invoked"]); + query.assert_log(&["Volatile invoked"]); } /// Test that: @@ -67,7 +68,7 @@ fn revalidate() { query.salsa_runtime().next_revision(); query.memoized2(()); - query.assert_log(&["Memoized1 invoked", "Volatile invoked"]); + query.assert_log(&["Volatile invoked", "Memoized1 invoked"]); query.memoized2(()); query.assert_log(&[]); @@ -78,7 +79,7 @@ fn revalidate() { query.salsa_runtime().next_revision(); query.memoized2(()); - query.assert_log(&["Memoized1 invoked", "Volatile invoked", "Memoized2 invoked"]); + query.assert_log(&["Volatile invoked", "Memoized1 invoked", "Memoized2 invoked"]); query.memoized2(()); query.assert_log(&[]); diff --git a/tests/storage_varieties/main.rs b/tests/storage_varieties/main.rs index c10de188..0e90f20e 100644 --- a/tests/storage_varieties/main.rs +++ b/tests/storage_varieties/main.rs @@ -1,4 +1,5 @@ #![feature(crate_visibility_modifier)] +#![feature(underscore_imports)] mod implementation; mod queries; diff --git a/tests/storage_varieties/queries.rs b/tests/storage_varieties/queries.rs index d0af5e2b..b66dc53e 100644 --- a/tests/storage_varieties/queries.rs +++ b/tests/storage_varieties/queries.rs @@ -17,7 +17,7 @@ salsa::query_group! { /// Because this query is memoized, we only increment the counter /// the first time it is invoked. fn memoized(db: &impl Database, (): ()) -> usize { - db.increment() + db.volatile(()) } /// Because this query is volatile, each time it is invoked, diff --git a/tests/storage_varieties/tests.rs b/tests/storage_varieties/tests.rs index 5ad5ffc5..bfd82143 100644 --- a/tests/storage_varieties/tests.rs +++ b/tests/storage_varieties/tests.rs @@ -2,32 +2,47 @@ use crate::implementation::DatabaseImpl; use crate::queries::Database; +use salsa::Database as _; #[test] fn memoized_twice() { - let query = DatabaseImpl::default(); - let v1 = query.memoized(()); - let v2 = query.memoized(()); + let db = DatabaseImpl::default(); + let v1 = db.memoized(()); + let v2 = db.memoized(()); assert_eq!(v1, v2); } #[test] fn volatile_twice() { - let query = DatabaseImpl::default(); - let v1 = query.volatile(()); - let v2 = query.volatile(()); - assert_eq!(v1 + 1, v2); + let db = DatabaseImpl::default(); + let v1 = db.volatile(()); + let v2 = db.volatile(()); // volatiles are cached, so 2nd read returns the same + assert_eq!(v1, v2); + + db.salsa_runtime().next_revision(); // clears volatile caches + + let v3 = db.volatile(()); // will re-increment the counter + let v4 = db.volatile(()); // second call will be cached + assert_eq!(v1 + 1, v3); + assert_eq!(v3, v4); } #[test] fn intermingled() { - let query = DatabaseImpl::default(); - let v1 = query.volatile(()); - let v2 = query.memoized(()); - let v3 = query.volatile(()); - let v4 = query.memoized(()); + let db = DatabaseImpl::default(); + let v1 = db.volatile(()); + let v2 = db.memoized(()); + let v3 = db.volatile(()); // cached + let v4 = db.memoized(()); // cached - assert_eq!(v1 + 1, v2); - assert_eq!(v2 + 1, v3); + assert_eq!(v1, v2); + assert_eq!(v1, v3); assert_eq!(v2, v4); + + db.salsa_runtime().next_revision(); // clears volatile caches + + let v5 = db.memoized(()); // re-executes volatile, caches new result + let v6 = db.memoized(()); // re-use cached result + assert_eq!(v4 + 1, v5); + assert_eq!(v5, v6); } From 6658a47a3657657f6332e0084cbdbc4ffd630a71 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 13:28:33 -0400 Subject: [PATCH 8/9] rename `is_volatile` to `should_track_inputs` --- src/derived.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/derived.rs b/src/derived.rs index 9c15ed19..f8b1ad04 100644 --- a/src/derived.rs +++ b/src/derived.rs @@ -55,7 +55,7 @@ where { fn should_memoize_value(key: &Q::Key) -> bool; - fn is_volatile(key: &Q::Key) -> bool; + fn should_track_inputs(key: &Q::Key) -> bool; } pub enum AlwaysMemoizeValue {} @@ -68,8 +68,8 @@ where true } - fn is_volatile(_key: &Q::Key) -> bool { - false + fn should_track_inputs(_key: &Q::Key) -> bool { + true } } @@ -83,8 +83,8 @@ where false } - fn is_volatile(_key: &Q::Key) -> bool { - false + fn should_track_inputs(_key: &Q::Key) -> bool { + true } } @@ -103,8 +103,8 @@ where true } - fn is_volatile(_key: &Q::Key) -> bool { - true + fn should_track_inputs(_key: &Q::Key) -> bool { + false } } @@ -244,7 +244,7 @@ where let (mut stamped_value, inputs) = runtime.execute_query_implementation(descriptor, || { debug!("{:?}({:?}): executing query", Q::default(), key); - if self.is_volatile(key) { + if !self.should_track_inputs(key) { runtime.report_untracked_read(); } @@ -313,8 +313,8 @@ where MP::should_memoize_value(key) } - fn is_volatile(&self, key: &Q::Key) -> bool { - MP::is_volatile(key) + fn should_track_inputs(&self, key: &Q::Key) -> bool { + MP::should_track_inputs(key) } } From 0dd96865c78d7f126fb2a5fbc8e86de78e2fd187 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 15:11:57 -0400 Subject: [PATCH 9/9] refactor to unwrap less --- src/derived.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/derived.rs b/src/derived.rs index f8b1ad04..83090539 100644 --- a/src/derived.rs +++ b/src/derived.rs @@ -223,13 +223,12 @@ where // first things first, let's walk over each of our previous // inputs and check whether they are out of date. if let Some(QueryState::Memoized(old_memo)) = &mut old_value { - if old_memo.verify_memoized_value(db) { + if let Some(value) = old_memo.verify_memoized_value(db) { debug!("{:?}({:?}): inputs still valid", Q::default(), key); // If none of out inputs have changed since the last time we refreshed // our value, then our value must still be good. We'll just patch // the verified-at date and re-use it. old_memo.verified_at = revision_now; - let value = old_memo.value.clone().unwrap(); let changed_at = old_memo.changed_at; let mut map_write = self.map.write(); @@ -430,13 +429,16 @@ where Q: QueryFunction, DB: Database, { - fn verify_memoized_value(&self, db: &DB) -> bool { + fn verify_memoized_value(&self, db: &DB) -> Option { // If we don't have a memoized value, nothing to validate. - if !self.value.is_some() { - return false; + if let Some(v) = &self.value { + // If inputs are still valid. + if self.verify_inputs(db) { + return Some(v.clone()); + } } - self.verify_inputs(db) + None } fn verify_inputs(&self, db: &DB) -> bool {