From 6a0ed30d73cbbcb2138774320cb6dc0ba1327a3e Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 15:40:15 -0400 Subject: [PATCH 01/13] rename `MutQueryStorageOps` to `InputQueryStorageOps` --- src/input.rs | 4 ++-- src/lib.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/input.rs b/src/input.rs index 80678bfa..aa0f8938 100644 --- a/src/input.rs +++ b/src/input.rs @@ -4,7 +4,7 @@ use crate::runtime::Revision; use crate::runtime::StampedValue; use crate::CycleDetected; use crate::Database; -use crate::MutQueryStorageOps; +use crate::InputQueryStorageOps; use crate::Query; use crate::QueryDescriptor; use crate::QueryStorageOps; @@ -117,7 +117,7 @@ where } } -impl MutQueryStorageOps for InputStorage +impl InputQueryStorageOps for InputStorage where Q: Query, DB: Database, diff --git a/src/lib.rs b/src/lib.rs index 56b6faca..c38879ae 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -124,7 +124,7 @@ where /// 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. -pub trait MutQueryStorageOps: Default +pub trait InputQueryStorageOps: Default where DB: Database, Q: Query, @@ -174,7 +174,7 @@ where /// an active query computation. pub fn set(&self, key: Q::Key, value: Q::Value) where - Q::Storage: MutQueryStorageOps, + Q::Storage: InputQueryStorageOps, { self.storage.set(self.db, &key, value); } From 3ffd166f2c917d78407629f611ef18f916aa4f01 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 15:56:05 -0400 Subject: [PATCH 02/13] check if input has changed before incrementing revision WIP -- needs test --- src/input.rs | 14 ++++++++++++-- src/lib.rs | 2 +- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/input.rs b/src/input.rs index aa0f8938..1fdcce66 100644 --- a/src/input.rs +++ b/src/input.rs @@ -124,9 +124,19 @@ where Q::Value: Default, { fn set(&self, db: &DB, key: &Q::Key, value: Q::Value) { + let map = self.map.upgradable_read(); + + // If this value was previously stored, check if this is an + // *actual change* before we do anything. + if let Some(old_value) = map.get(key) { + if old_value.value == value { + return; + } + } + let key = key.clone(); - let mut map_write = self.map.write(); + let mut map = RwLockUpgradableReadGuard::upgrade(map); // Do this *after* we acquire the lock, so that we are not // racing with somebody else to modify this same cell. @@ -134,7 +144,7 @@ where // into the same cell while we block on the lock.) let changed_at = ChangedAt::Revision(db.salsa_runtime().increment_revision()); - map_write.insert(key, StampedValue { value, changed_at }); + map.insert(key, StampedValue { value, changed_at }); } } diff --git a/src/lib.rs b/src/lib.rs index c38879ae..c6f21324 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -170,7 +170,7 @@ where }) } - /// Assign a value to an "input queries". Must be used outside of + /// Assign a value to an "input query". Must be used outside of /// an active query computation. pub fn set(&self, key: Q::Key, value: Q::Value) where From 1afca5d5053ce06c935b14accac55e1bd5f6ffe5 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 16:20:57 -0400 Subject: [PATCH 03/13] add a `Constant` for `ChangedAt` --- src/derived.rs | 13 +++++++++++++ src/runtime.rs | 19 ++++++++++++++++--- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/src/derived.rs b/src/derived.rs index 49ca1e5d..5a370970 100644 --- a/src/derived.rs +++ b/src/derived.rs @@ -443,6 +443,19 @@ where fn verify_inputs(&self, db: &DB) -> bool { match self.changed_at { + ChangedAt::Constant => { + // If we know that the value is constant, it had + // better not change, but in that case, we ought not + // to have any inputs. Using `debug_assert` because + // this is on the fast path. + debug_assert!(match &self.inputs { + QueryDescriptorSet::Tracked(inputs) => inputs.is_empty(), + QueryDescriptorSet::Untracked => false, + }); + + true + } + ChangedAt::Revision(revision) => match &self.inputs { QueryDescriptorSet::Tracked(inputs) => inputs .iter() diff --git a/src/runtime.rs b/src/runtime.rs index e032453d..82fb2668 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -194,14 +194,22 @@ impl ActiveQuery { fn new(descriptor: DB::QueryDescriptor) -> Self { ActiveQuery { descriptor, - changed_at: ChangedAt::Revision(Revision::ZERO), + changed_at: ChangedAt::Constant, subqueries: QueryDescriptorSet::default(), } } 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); + match changed_at { + ChangedAt::Constant => { + // When we read constant values, we don't need to + // track the source of the value. + } + ChangedAt::Revision(_) => { + self.subqueries.insert(subquery.clone()); + self.changed_at = self.changed_at.max(changed_at); + } + } } fn add_untracked_read(&mut self, changed_at: ChangedAt) { @@ -232,6 +240,10 @@ impl std::fmt::Debug for Revision { /// changed. #[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] pub enum ChangedAt { + /// Will never change again. + Constant, + + /// Last changed in the given revision. May change in the future. Revision(Revision), } @@ -239,6 +251,7 @@ impl ChangedAt { /// True if this value has changed after `revision`. pub fn changed_since(self, revision: Revision) -> bool { match self { + ChangedAt::Constant => false, ChangedAt::Revision(r) => r > revision, } } From 032b2691133ef250c2896e20d5030d9763ef7202 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 16:25:58 -0400 Subject: [PATCH 04/13] extract a `set_common` helper --- src/input.rs | 48 +++++++++++++++++++++++++++--------------------- 1 file changed, 27 insertions(+), 21 deletions(-) diff --git a/src/input.rs b/src/input.rs index 1fdcce66..ac92cbae 100644 --- a/src/input.rs +++ b/src/input.rs @@ -70,6 +70,30 @@ where changed_at: ChangedAt::Revision(Revision::ZERO), }) } + + fn set_common(&self, key: &Q::Key, value: Q::Value, changed_at: impl FnOnce() -> ChangedAt) { + let map = self.map.upgradable_read(); + + // If this value was previously stored, check if this is an + // *actual change* before we do anything. + if let Some(old_value) = map.get(key) { + if old_value.value == value { + return; + } + } + + let key = key.clone(); + + let mut map = RwLockUpgradableReadGuard::upgrade(map); + + // Do this *after* we acquire the lock, so that we are not + // 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 = changed_at(); + + map.insert(key, StampedValue { value, changed_at }); + } } impl QueryStorageOps for InputStorage @@ -124,27 +148,9 @@ where Q::Value: Default, { fn set(&self, db: &DB, key: &Q::Key, value: Q::Value) { - let map = self.map.upgradable_read(); - - // If this value was previously stored, check if this is an - // *actual change* before we do anything. - if let Some(old_value) = map.get(key) { - if old_value.value == value { - return; - } - } - - let key = key.clone(); - - let mut map = RwLockUpgradableReadGuard::upgrade(map); - - // Do this *after* we acquire the lock, so that we are not - // 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 = ChangedAt::Revision(db.salsa_runtime().increment_revision()); - - map.insert(key, StampedValue { value, changed_at }); + self.set_common(key, value, || { + ChangedAt::Revision(db.salsa_runtime().increment_revision()) + }); } } From a7317084dca59e3281b0d33e7ea5728024d460bf Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 16:32:45 -0400 Subject: [PATCH 05/13] support `set_constant` in inputs FIXME: Need test for the `panic!` case etc --- src/input.rs | 27 ++++++++++++++++++++++++++- src/lib.rs | 12 ++++++++++++ src/runtime.rs | 7 +++++++ 3 files changed, 45 insertions(+), 1 deletion(-) diff --git a/src/input.rs b/src/input.rs index ac92cbae..c70c2f6b 100644 --- a/src/input.rs +++ b/src/input.rs @@ -91,8 +91,26 @@ where // (Otherwise, someone else might write a *newer* revision // into the same cell while we block on the lock.) let changed_at = changed_at(); + let stamped_value = StampedValue { value, changed_at }; - map.insert(key, StampedValue { value, changed_at }); + match map.entry(key) { + Entry::Occupied(mut entry) => { + assert!( + !entry.get().changed_at.is_constant(), + "modifying `{:?}({:?})`, which was previously marked as constant (old value `{:?}`, new value `{:?}`)", + Q::default(), + entry.key(), + entry.get().value, + stamped_value.value, + ); + + entry.insert(stamped_value); + } + + Entry::Vacant(entry) => { + entry.insert(stamped_value); + } + } } } @@ -152,6 +170,13 @@ where ChangedAt::Revision(db.salsa_runtime().increment_revision()) }); } + + fn set_constant(&self, _db: &DB, key: &Q::Key, value: Q::Value) { + // FIXME. One weirdness: if this previously had a value, but + // was not marked as constant, we will not "lower" the + // `ChangedAt` rating with the code as it is. + self.set_common(key, value, || ChangedAt::Constant); + } } impl UncheckedMutQueryStorageOps for InputStorage diff --git a/src/lib.rs b/src/lib.rs index c6f21324..cb98a170 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -130,6 +130,8 @@ where Q: Query, { fn set(&self, db: &DB, key: &Q::Key, new_value: Q::Value); + + fn set_constant(&self, db: &DB, key: &Q::Key, new_value: Q::Value); } /// An optional trait that is implemented for "user mutable" storage: @@ -179,6 +181,16 @@ where self.storage.set(self.db, &key, value); } + /// Assign a value to an "input query", with the additional + /// promise that this value will **never change**. Must be used + /// outside of an active query computation. + pub fn set_constant(&self, key: Q::Key, value: Q::Value) + where + Q::Storage: InputQueryStorageOps, + { + self.storage.set_constant(self.db, &key, value); + } + /// Assigns a value to the query **bypassing the normal /// incremental checking** -- this value becomes the value for the /// query in the current revision. This can even be used on diff --git a/src/runtime.rs b/src/runtime.rs index 82fb2668..6f134643 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -248,6 +248,13 @@ pub enum ChangedAt { } impl ChangedAt { + pub fn is_constant(self) -> bool { + match self { + ChangedAt::Constant => true, + ChangedAt::Revision(_) => false, + } + } + /// True if this value has changed after `revision`. pub fn changed_since(self, revision: Revision) -> bool { match self { From 42b88fe7e6812d9d4955a8e3d9add96b2a2e6594 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 17:40:56 -0400 Subject: [PATCH 06/13] rewrite map to just grab a write lock, and fix some latent bugs --- src/input.rs | 41 ++++++++++++++++++++++++++++------------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/src/input.rs b/src/input.rs index c70c2f6b..c81523af 100644 --- a/src/input.rs +++ b/src/input.rs @@ -46,6 +46,8 @@ where } } +struct IsConstant(bool); + impl InputStorage where Q: Query, @@ -71,26 +73,44 @@ where }) } - fn set_common(&self, key: &Q::Key, value: Q::Value, changed_at: impl FnOnce() -> ChangedAt) { - let map = self.map.upgradable_read(); + fn set_common(&self, db: &DB, key: &Q::Key, value: Q::Value, is_constant: IsConstant) { + let mut map = self.map.write(); // If this value was previously stored, check if this is an // *actual change* before we do anything. - if let Some(old_value) = map.get(key) { + if let Some(old_value) = map.get_mut(key) { if old_value.value == value { + // If the value did not change, but it is now + // considered constant, we can just update + // `changed_at`. We don't have to trigger a new + // revision for this case: all the derived values are + // still intact, they just have conservative + // dependencies. The next revision, they may wind up + // with something more precise. + if is_constant.0 && !old_value.changed_at.is_constant() { + old_value.changed_at = ChangedAt::Constant; + } + return; } } let key = key.clone(); - let mut map = RwLockUpgradableReadGuard::upgrade(map); + // The value is changing, so even if we are setting this to a + // constant, we still need a new revision. + let next_revision = db.salsa_runtime().increment_revision(); // Do this *after* we acquire the lock, so that we are not // 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 = changed_at(); + let changed_at = if is_constant.0 { + ChangedAt::Constant + } else { + ChangedAt::Revision(next_revision) + }; + let stamped_value = StampedValue { value, changed_at }; match map.entry(key) { @@ -166,16 +186,11 @@ where Q::Value: Default, { fn set(&self, db: &DB, key: &Q::Key, value: Q::Value) { - self.set_common(key, value, || { - ChangedAt::Revision(db.salsa_runtime().increment_revision()) - }); + self.set_common(db, key, value, IsConstant(false)) } - fn set_constant(&self, _db: &DB, key: &Q::Key, value: Q::Value) { - // FIXME. One weirdness: if this previously had a value, but - // was not marked as constant, we will not "lower" the - // `ChangedAt` rating with the code as it is. - self.set_common(key, value, || ChangedAt::Constant); + fn set_constant(&self, db: &DB, key: &Q::Key, value: Q::Value) { + self.set_common(db, key, value, IsConstant(true)) } } From 15faf43071c7d987108583fdb1ae0e2cd6d0bc2a Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 9 Oct 2018 20:35:57 -0400 Subject: [PATCH 07/13] add some tests for constants (check for invalidation) --- tests/incremental/constants.rs | 43 +++++++++++++++++++++++++++++ tests/incremental/implementation.rs | 6 ++++ tests/incremental/main.rs | 1 + 3 files changed, 50 insertions(+) create mode 100644 tests/incremental/constants.rs diff --git a/tests/incremental/constants.rs b/tests/incremental/constants.rs new file mode 100644 index 00000000..fa6ecae5 --- /dev/null +++ b/tests/incremental/constants.rs @@ -0,0 +1,43 @@ +use crate::implementation::{TestContext, TestContextImpl}; +use salsa::Database; + +salsa::query_group! { + pub(crate) trait ConstantsDatabase: TestContext { + fn constants_input(key: usize) -> usize { + type ConstantsInput; + storage input; + } + + fn constants_derived(key: usize) -> usize { + type ConstantsDerived; + } + } +} + +fn constants_derived(db: &impl ConstantsDatabase, key: usize) -> usize { + db.log().add(format!("constants_derived({}) invoked", key)); + db.constants_input(key) * 2 +} + +#[test] +#[should_panic] +fn invalidate_constant() { + let db = &TestContextImpl::default(); + db.query(ConstantsInput).set_constant(22, 44); + db.query(ConstantsInput).set_constant(22, 66); +} + +#[test] +#[should_panic] +fn invalidate_constant_1() { + let db = &TestContextImpl::default(); + + // Not constant: + db.query(ConstantsInput).set(22, 44); + + // Becomes constant: + db.query(ConstantsInput).set_constant(22, 44); + + // Invalidates: + db.query(ConstantsInput).set_constant(22, 66); +} diff --git a/tests/incremental/implementation.rs b/tests/incremental/implementation.rs index 868e49c9..8fba2ce7 100644 --- a/tests/incremental/implementation.rs +++ b/tests/incremental/implementation.rs @@ -1,3 +1,4 @@ +use crate::constants; use crate::counter::Counter; use crate::log::Log; use crate::memoized_dep_inputs; @@ -43,6 +44,11 @@ impl TestContextImpl { salsa::database_storage! { pub(crate) struct TestContextImplStorage for TestContextImpl { + impl constants::ConstantsDatabase { + fn constants_input() for constants::ConstantsInput; + fn constants_derived() for constants::ConstantsDerived; + } + impl memoized_dep_inputs::MemoizedDepInputsContext { fn dep_memoized2() for memoized_dep_inputs::Memoized2; fn dep_memoized1() for memoized_dep_inputs::Memoized1; diff --git a/tests/incremental/main.rs b/tests/incremental/main.rs index 33a623cd..bcd13c75 100644 --- a/tests/incremental/main.rs +++ b/tests/incremental/main.rs @@ -1,3 +1,4 @@ +mod constants; mod counter; mod implementation; mod log; From 16d151e4c8abb25625ed708083e6efcbb46fe5da Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 11 Oct 2018 04:37:29 -0400 Subject: [PATCH 08/13] add debugging APIs -- just `is_constant` for now --- src/debug.rs | 28 ++++++++++++++++++++++++++++ src/derived.rs | 9 +++++++++ src/input.rs | 8 ++++++++ src/lib.rs | 4 ++++ 4 files changed, 49 insertions(+) create mode 100644 src/debug.rs diff --git a/src/debug.rs b/src/debug.rs new file mode 100644 index 00000000..491ffdf1 --- /dev/null +++ b/src/debug.rs @@ -0,0 +1,28 @@ +//! Debugging APIs: these are meant for use when unit-testing or +//! debugging your application but aren't ordinarily needed. + +use crate::Database; +use crate::Query; +use crate::QueryStorageOps; +use crate::QueryTable; + +pub trait DebugQueryTable { + type Key; + + /// True if salsa thinks that the value for `key` is a + /// **constant**, meaning that it can never change, no matter what + /// values the inputs take on from this point. + fn is_constant(&self, key: Self::Key) -> bool; +} + +impl DebugQueryTable for QueryTable<'_, DB, Q> +where + DB: Database, + Q: Query, +{ + type Key = Q::Key; + + fn is_constant(&self, key: Q::Key) -> bool { + self.storage.is_constant(self.db, &key) + } +} diff --git a/src/derived.rs b/src/derived.rs index 5a370970..c1042f36 100644 --- a/src/derived.rs +++ b/src/derived.rs @@ -396,6 +396,15 @@ where true } + + fn is_constant(&self, _db: &DB, key: &Q::Key) -> bool { + let map_read = self.map.read(); + match map_read.get(key) { + None => false, + Some(QueryState::InProgress) => panic!("query in progress"), + Some(QueryState::Memoized(memo)) => memo.changed_at.is_constant(), + } + } } impl UncheckedMutQueryStorageOps for DerivedStorage diff --git a/src/input.rs b/src/input.rs index c81523af..face5243 100644 --- a/src/input.rs +++ b/src/input.rs @@ -177,6 +177,14 @@ where changed_at.changed_since(revision) } + + fn is_constant(&self, _db: &DB, key: &Q::Key) -> bool { + let map_read = self.map.read(); + map_read + .get(key) + .map(|v| v.changed_at.is_constant()) + .unwrap_or(false) + } } impl InputQueryStorageOps for InputStorage diff --git a/src/lib.rs b/src/lib.rs index cb98a170..af20b33b 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -12,6 +12,7 @@ use std::fmt::Display; use std::fmt::Write; use std::hash::Hash; +pub mod debug; pub mod derived; pub mod input; pub mod runtime; @@ -119,6 +120,9 @@ where key: &Q::Key, descriptor: &DB::QueryDescriptor, ) -> bool; + + /// Check if `key` is (currently) believed to be a constant. + fn is_constant(&self, db: &DB, key: &Q::Key) -> bool; } /// An optional trait that is implemented for "user mutable" storage: From efa8b0f307207fc1a8060f2a37ff03406fb93cc9 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 11 Oct 2018 04:53:24 -0400 Subject: [PATCH 09/13] add some simple tests using `is_constant` --- tests/incremental/constants.rs | 55 +++++++++++++++++++++++------ tests/incremental/implementation.rs | 2 +- 2 files changed, 45 insertions(+), 12 deletions(-) diff --git a/tests/incremental/constants.rs b/tests/incremental/constants.rs index fa6ecae5..ea054b9b 100644 --- a/tests/incremental/constants.rs +++ b/tests/incremental/constants.rs @@ -1,30 +1,32 @@ use crate::implementation::{TestContext, TestContextImpl}; +use salsa::debug::DebugQueryTable; use salsa::Database; salsa::query_group! { pub(crate) trait ConstantsDatabase: TestContext { - fn constants_input(key: usize) -> usize { + fn constants_input(key: char) -> usize { type ConstantsInput; storage input; } - fn constants_derived(key: usize) -> usize { - type ConstantsDerived; + fn constants_add(keys: (char, char)) -> usize { + type ConstantsAdd; } } } -fn constants_derived(db: &impl ConstantsDatabase, key: usize) -> usize { - db.log().add(format!("constants_derived({}) invoked", key)); - db.constants_input(key) * 2 +fn constants_add(db: &impl ConstantsDatabase, (key1, key2): (char, char)) -> usize { + db.log() + .add(format!("constants_derived({}, {}) invoked", key1, key2)); + db.constants_input(key1) + db.constants_input(key2) } #[test] #[should_panic] fn invalidate_constant() { let db = &TestContextImpl::default(); - db.query(ConstantsInput).set_constant(22, 44); - db.query(ConstantsInput).set_constant(22, 66); + db.query(ConstantsInput).set_constant('a', 44); + db.query(ConstantsInput).set_constant('a', 66); } #[test] @@ -33,11 +35,42 @@ fn invalidate_constant_1() { let db = &TestContextImpl::default(); // Not constant: - db.query(ConstantsInput).set(22, 44); + db.query(ConstantsInput).set('a', 44); // Becomes constant: - db.query(ConstantsInput).set_constant(22, 44); + db.query(ConstantsInput).set_constant('a', 44); // Invalidates: - db.query(ConstantsInput).set_constant(22, 66); + db.query(ConstantsInput).set_constant('a', 66); +} + +#[test] +fn not_constant() { + let db = &TestContextImpl::default(); + + db.query(ConstantsInput).set('a', 22); + db.query(ConstantsInput).set('b', 44); + assert_eq!(db.constants_add(('a', 'b')), 66); + assert!(!db.query(ConstantsAdd).is_constant(('a', 'b'))); +} + +#[test] +fn is_constant() { + let db = &TestContextImpl::default(); + + db.query(ConstantsInput).set_constant('a', 22); + db.query(ConstantsInput).set_constant('b', 44); + assert_eq!(db.constants_add(('a', 'b')), 66); + assert!(db.query(ConstantsAdd).is_constant(('a', 'b'))); +} + +#[test] +fn mixed_constant() { + let db = &TestContextImpl::default(); + + db.query(ConstantsInput).set_constant('a', 22); + db.query(ConstantsInput).set('b', 44); + assert_eq!(db.constants_add(('a', 'b')), 66); + assert!(!db.query(ConstantsAdd).is_constant(('a', 'b'))); +} } diff --git a/tests/incremental/implementation.rs b/tests/incremental/implementation.rs index 8fba2ce7..36809bef 100644 --- a/tests/incremental/implementation.rs +++ b/tests/incremental/implementation.rs @@ -46,7 +46,7 @@ salsa::database_storage! { pub(crate) struct TestContextImplStorage for TestContextImpl { impl constants::ConstantsDatabase { fn constants_input() for constants::ConstantsInput; - fn constants_derived() for constants::ConstantsDerived; + fn constants_derived() for constants::ConstantsAdd; } impl memoized_dep_inputs::MemoizedDepInputsContext { From 6778898a34de18197017051cbf4dd2bb261a7be3 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 11 Oct 2018 04:53:33 -0400 Subject: [PATCH 10/13] track when a value *became* constant Turns out we need this, as demonstrated by the included test =) --- src/derived.rs | 2 +- src/input.rs | 5 +++-- src/runtime.rs | 24 +++++++++++++++--------- tests/incremental/constants.rs | 17 +++++++++++++++++ 4 files changed, 36 insertions(+), 12 deletions(-) diff --git a/src/derived.rs b/src/derived.rs index c1042f36..e827f71b 100644 --- a/src/derived.rs +++ b/src/derived.rs @@ -452,7 +452,7 @@ where fn verify_inputs(&self, db: &DB) -> bool { match self.changed_at { - ChangedAt::Constant => { + ChangedAt::Constant(_) => { // If we know that the value is constant, it had // better not change, but in that case, we ought not // to have any inputs. Using `debug_assert` because diff --git a/src/input.rs b/src/input.rs index face5243..a90c92ea 100644 --- a/src/input.rs +++ b/src/input.rs @@ -88,7 +88,8 @@ where // dependencies. The next revision, they may wind up // with something more precise. if is_constant.0 && !old_value.changed_at.is_constant() { - old_value.changed_at = ChangedAt::Constant; + old_value.changed_at = + ChangedAt::Constant(db.salsa_runtime().current_revision()); } return; @@ -106,7 +107,7 @@ where // (Otherwise, someone else might write a *newer* revision // into the same cell while we block on the lock.) let changed_at = if is_constant.0 { - ChangedAt::Constant + ChangedAt::Constant(next_revision) } else { ChangedAt::Revision(next_revision) }; diff --git a/src/runtime.rs b/src/runtime.rs index 6f134643..41c6cba7 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -134,7 +134,11 @@ where /// - `descriptor`: the query whose result was read /// - `changed_revision`: the last revision in which the result of that /// query had changed - pub(crate) fn report_query_read(&self, descriptor: &DB::QueryDescriptor, changed_at: ChangedAt) { + pub(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); } @@ -194,14 +198,14 @@ impl ActiveQuery { fn new(descriptor: DB::QueryDescriptor) -> Self { ActiveQuery { descriptor, - changed_at: ChangedAt::Constant, + changed_at: ChangedAt::Constant(Revision::ZERO), subqueries: QueryDescriptorSet::default(), } } fn add_read(&mut self, subquery: &DB::QueryDescriptor, changed_at: ChangedAt) { match changed_at { - ChangedAt::Constant => { + ChangedAt::Constant(_) => { // When we read constant values, we don't need to // track the source of the value. } @@ -240,8 +244,9 @@ impl std::fmt::Debug for Revision { /// changed. #[derive(Copy, Clone, Debug, PartialEq, Eq, PartialOrd, Ord)] pub enum ChangedAt { - /// Will never change again. - Constant, + /// Will never change again (and the revision in which we became a + /// constant). + Constant(Revision), /// Last changed in the given revision. May change in the future. Revision(Revision), @@ -250,16 +255,17 @@ pub enum ChangedAt { impl ChangedAt { pub fn is_constant(self) -> bool { match self { - ChangedAt::Constant => true, + ChangedAt::Constant(_) => true, ChangedAt::Revision(_) => false, } } - /// True if this value has changed after `revision`. + /// True if a value is stored with this `ChangedAt` value has + /// changed after `revision`. This is invoked by query storage + /// when their dependents are asking them if they have changed. pub fn changed_since(self, revision: Revision) -> bool { match self { - ChangedAt::Constant => false, - ChangedAt::Revision(r) => r > revision, + ChangedAt::Constant(r) | ChangedAt::Revision(r) => r > revision, } } } diff --git a/tests/incremental/constants.rs b/tests/incremental/constants.rs index ea054b9b..07278a96 100644 --- a/tests/incremental/constants.rs +++ b/tests/incremental/constants.rs @@ -73,4 +73,21 @@ fn mixed_constant() { assert_eq!(db.constants_add(('a', 'b')), 66); assert!(!db.query(ConstantsAdd).is_constant(('a', 'b'))); } + +#[test] +fn becomes_constant() { + let db = &TestContextImpl::default(); + + db.query(ConstantsInput).set('a', 22); + db.query(ConstantsInput).set('b', 44); + assert_eq!(db.constants_add(('a', 'b')), 66); + assert!(!db.query(ConstantsAdd).is_constant(('a', 'b'))); + + db.query(ConstantsInput).set_constant('a', 23); + assert_eq!(db.constants_add(('a', 'b')), 67); + assert!(!db.query(ConstantsAdd).is_constant(('a', 'b'))); + + db.query(ConstantsInput).set_constant('b', 45); + assert_eq!(db.constants_add(('a', 'b')), 68); + assert!(db.query(ConstantsAdd).is_constant(('a', 'b'))); } From 5e381f314bd67009c9ec7879b54ee61e2ea6af90 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 11 Oct 2018 04:56:34 -0400 Subject: [PATCH 11/13] add test where we become constant but do not change value --- tests/incremental/constants.rs | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/tests/incremental/constants.rs b/tests/incremental/constants.rs index 07278a96..d7d3acd2 100644 --- a/tests/incremental/constants.rs +++ b/tests/incremental/constants.rs @@ -16,8 +16,7 @@ salsa::query_group! { } fn constants_add(db: &impl ConstantsDatabase, (key1, key2): (char, char)) -> usize { - db.log() - .add(format!("constants_derived({}, {}) invoked", key1, key2)); + db.log().add(format!("add({}, {})", key1, key2)); db.constants_input(key1) + db.constants_input(key2) } @@ -91,3 +90,29 @@ fn becomes_constant() { assert_eq!(db.constants_add(('a', 'b')), 68); assert!(db.query(ConstantsAdd).is_constant(('a', 'b'))); } + +#[test] +fn becomes_constant_no_change() { + let db = &TestContextImpl::default(); + + db.query(ConstantsInput).set('a', 22); + db.query(ConstantsInput).set('b', 44); + assert_eq!(db.constants_add(('a', 'b')), 66); + assert!(!db.query(ConstantsAdd).is_constant(('a', 'b'))); + db.assert_log(&["add(a, b)"]); + + // 'a' is now constant, but the value did not change; this + // should not in and of itself trigger a new revision. + db.query(ConstantsInput).set_constant('a', 22); + assert_eq!(db.constants_add(('a', 'b')), 66); + assert!(!db.query(ConstantsAdd).is_constant(('a', 'b'))); + db.assert_log(&[]); // no new revision, no new log entries + + // 'b' is now constant, and its value DID change. This triggers a + // new revision, and at that point we figure out that we are + // constant. + db.query(ConstantsInput).set_constant('b', 45); + assert_eq!(db.constants_add(('a', 'b')), 67); + assert!(db.query(ConstantsAdd).is_constant(('a', 'b'))); + db.assert_log(&["add(a, b)"]); +} From a0c983403d8df16e4423a3cae0af8d665d8914af Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 11 Oct 2018 04:59:07 -0400 Subject: [PATCH 12/13] add some tests about setting with same value --- tests/incremental/constants.rs | 9 +++++++++ tests/incremental/memoized_inputs.rs | 17 +++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/tests/incremental/constants.rs b/tests/incremental/constants.rs index d7d3acd2..a7391bda 100644 --- a/tests/incremental/constants.rs +++ b/tests/incremental/constants.rs @@ -43,6 +43,15 @@ fn invalidate_constant_1() { db.query(ConstantsInput).set_constant('a', 66); } +/// Test that use can still `set` an input that is constant, so long +/// as you don't change the value. +#[test] +fn set_after_constant_same_value() { + let db = &TestContextImpl::default(); + db.query(ConstantsInput).set_constant('a', 44); + db.query(ConstantsInput).set('a', 44); +} + #[test] fn not_constant() { let db = &TestContextImpl::default(); diff --git a/tests/incremental/memoized_inputs.rs b/tests/incremental/memoized_inputs.rs index 0fdc9b79..23e690f3 100644 --- a/tests/incremental/memoized_inputs.rs +++ b/tests/incremental/memoized_inputs.rs @@ -60,3 +60,20 @@ fn revalidate() { assert_eq!(v, 66); db.assert_log(&[]); } + +/// Test that invoking `set` on an input with the same value does not +/// trigger a new revision. +#[test] +fn set_after_no_change() { + let db = &TestContextImpl::default(); + + db.query(Input1).set((), 44); + let v = db.max(()); + assert_eq!(v, 44); + db.assert_log(&["Max invoked"]); + + db.query(Input1).set((), 44); + let v = db.max(()); + assert_eq!(v, 44); + db.assert_log(&[]); +} From a353ffb13cc9bca2015e79c1ac907bf6e11ee09b Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Thu, 11 Oct 2018 05:14:40 -0400 Subject: [PATCH 13/13] add back implied outlives bounds Those were reverted on beta. --- src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index af20b33b..80653de8 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -152,8 +152,8 @@ where #[derive(new)] pub struct QueryTable<'me, DB, Q> where - DB: Database, - Q: Query, + DB: Database + 'me, + Q: Query + 'me, { db: &'me DB, storage: &'me Q::Storage,