From 13ae45d441a7a48cadf535cd7196b8d30cf5b792 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 30 Oct 2018 12:59:33 -0400 Subject: [PATCH] remove input policies --- src/input.rs | 162 ++--------------------- src/lib.rs | 45 +------ src/plumbing.rs | 4 - tests/gc/derived_tests.rs | 18 +-- tests/gc/group.rs | 2 +- tests/incremental/constants.rs | 33 +---- tests/incremental/memoized_dep_inputs.rs | 4 +- tests/incremental/memoized_inputs.rs | 15 ++- 8 files changed, 43 insertions(+), 240 deletions(-) diff --git a/src/input.rs b/src/input.rs index b4a081d..7653af3 100644 --- a/src/input.rs +++ b/src/input.rs @@ -13,141 +13,36 @@ use log::debug; use parking_lot::{RwLock, RwLockUpgradableReadGuard}; use rustc_hash::FxHashMap; use std::collections::hash_map::Entry; -use std::marker::PhantomData; /// Input 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 struct InputStorage +pub struct InputStorage where Q: Query, DB: Database, - IP: InputPolicy, { map: RwLock>>, - input_policy: PhantomData, } -pub trait InputPolicy +impl Default for InputStorage where Q: Query, DB: Database, -{ - fn compare_values() -> bool { - false - } - - fn compare_value(_old_value: &Q::Value, _new_value: &Q::Value) -> bool { - panic!("should never be asked to compare values") - } - - fn missing_value(key: &Q::Key) -> Q::Value { - panic!("no value set for {:?}({:?})", Q::default(), key) - } -} - -/// The default policy for inputs: -/// -/// - Each time a new value is set, trigger a new revision. -/// - On an attempt access a value that is not yet set, panic. -pub enum ExplicitInputPolicy {} -impl InputPolicy for ExplicitInputPolicy -where - Q: Query, - DB: Database, -{ -} - -/// Alternative policy for inputs: -/// -/// - Each time a new value is set, trigger a new revision. -/// - On an attempt access a value that is not yet set, use `Default::default` to find -/// the value. -/// -/// Requires that `Q::Value` implements the `Default` trait. -pub enum DefaultValueInputPolicy {} -impl InputPolicy for DefaultValueInputPolicy -where - Q: Query, - DB: Database, - Q::Value: Default, -{ - fn missing_value(_key: &Q::Key) -> Q::Value { - ::default() - } -} - -/// Alternative policy for inputs: -/// -/// - Each time a new value is set, trigger a new revision -/// only if it is not equal to the old value. -/// - On an attempt access a value that is not yet set, panic. -/// -/// Requires that `Q::Value` implements the `Eq` trait. -pub enum EqValueInputPolicy {} -impl InputPolicy for EqValueInputPolicy -where - Q: Query, - DB: Database, - Q::Value: Eq, -{ - fn compare_values() -> bool { - true - } - - fn compare_value(old_value: &Q::Value, new_value: &Q::Value) -> bool { - old_value == new_value - } -} - -/// Alternative policy for inputs: -/// -/// - Each time a new value is set, trigger a new revision -/// only if it is not equal to the old value. -/// - On an attempt access a value that is not yet set, use `Default::default`. -/// -/// Requires that `Q::Value` implements the `Eq` trait. -pub enum DefaultEqValueInputPolicy {} -impl InputPolicy for DefaultEqValueInputPolicy -where - Q: Query, - DB: Database, - Q::Value: Default + Eq, -{ - fn compare_values() -> bool { - true - } - - fn compare_value(old_value: &Q::Value, new_value: &Q::Value) -> bool { - old_value == new_value - } - - fn missing_value(_key: &Q::Key) -> Q::Value { - ::default() - } -} - -impl Default for InputStorage -where - Q: Query, - DB: Database, - IP: InputPolicy, { fn default() -> Self { InputStorage { map: RwLock::new(FxHashMap::default()), - input_policy: PhantomData, } } } struct IsConstant(bool); -impl InputStorage +impl InputStorage where Q: Query, DB: Database, - IP: InputPolicy, { fn read<'q>( &self, @@ -162,43 +57,18 @@ where } } - let value = IP::missing_value(key); - - Ok(StampedValue { - value: value, - changed_at: ChangedAt { - is_constant: false, - revision: Revision::ZERO, - }, - }) + panic!("no value set for {:?}({:?})", Q::default(), key) } fn set_common(&self, db: &DB, key: &Q::Key, value: Q::Value, is_constant: IsConstant) { - let map = self.map.upgradable_read(); - - if IP::compare_values() { - if let Some(old_value) = map.get(key) { - if IP::compare_value(&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 { - let mut map = RwLockUpgradableReadGuard::upgrade(map); - let old_value = map.get_mut(key).unwrap(); - old_value.changed_at.is_constant = true; - } - - return; - } - } - } - let key = key.clone(); + // This upgradable read just serves to gate against other + // people invoking `set`; we should probably remove it and + // instead acquire the query-write-lock, but that would + // require refactoring the `increment_revision` API a bit. + let map = self.map.upgradable_read(); + // The value is changing, so even if we are setting this to a // constant, we still need a new revision. // @@ -243,11 +113,10 @@ where } } -impl QueryStorageOps for InputStorage +impl QueryStorageOps for InputStorage where Q: Query, DB: Database, - IP: InputPolicy, { fn try_fetch( &self, @@ -314,20 +183,18 @@ where } } -impl QueryStorageMassOps for InputStorage +impl QueryStorageMassOps for InputStorage where Q: Query, DB: Database, - IP: InputPolicy, { fn sweep(&self, _db: &DB, _strategy: SweepStrategy) {} } -impl InputQueryStorageOps for InputStorage +impl InputQueryStorageOps for InputStorage where Q: Query, DB: Database, - IP: InputPolicy, { fn set(&self, db: &DB, key: &Q::Key, value: Q::Value) { log::debug!("{:?}({:?}) = {:?}", Q::default(), key, value); @@ -342,11 +209,10 @@ where } } -impl UncheckedMutQueryStorageOps for InputStorage +impl UncheckedMutQueryStorageOps for InputStorage where Q: Query, DB: Database, - IP: InputPolicy, { fn set_unchecked(&self, db: &DB, key: &Q::Key, value: Q::Value) { let key = key.clone(); diff --git a/src/lib.rs b/src/lib.rs index 1566571..3a47d3f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -355,17 +355,6 @@ macro_rules! query_group { // do nothing for `storage input`, presuming they did not write an explicit `use fn` }; - ( - @query_fn[ - storage((input $($flags:tt)*)); - method_name($method_name:ident); - fn_path(); - $($rest:tt)* - ] - ) => { - // do nothing for `storage input`, presuming they did not write an explicit `use fn` - }; - ( @query_fn[ storage(input); @@ -380,20 +369,6 @@ macro_rules! query_group { } }; - ( - @query_fn[ - storage((input $($flags:tt)*)); - method_name($method_name:ident); - fn_path($fn_path:path); - $($rest:tt)* - ] - ) => { - // error for `storage input` with an explicit `use fn` - compile_error! { - "cannot have `storage input` combined with `use fn`" - } - }; - ( @query_fn[ storage($($storage:ident)*); @@ -502,25 +477,7 @@ macro_rules! query_group { ( @storage_ty[$DB:ident, $Self:ident, input] ) => { - $crate::plumbing::InputStorage - }; - - ( - @storage_ty[$DB:ident, $Self:ident, (input default)] - ) => { - $crate::plumbing::InputStorage - }; - - ( - @storage_ty[$DB:ident, $Self:ident, (input eq)] - ) => { - $crate::plumbing::InputStorage - }; - - ( - @storage_ty[$DB:ident, $Self:ident, (input default eq)] - ) => { - $crate::plumbing::InputStorage + $crate::plumbing::InputStorage }; ( diff --git a/src/plumbing.rs b/src/plumbing.rs index 4d04dbe..66482ed 100644 --- a/src/plumbing.rs +++ b/src/plumbing.rs @@ -8,10 +8,6 @@ use std::hash::Hash; pub use crate::derived::DependencyStorage; pub use crate::derived::MemoizedStorage; pub use crate::derived::VolatileStorage; -pub use crate::input::DefaultEqValueInputPolicy; -pub use crate::input::DefaultValueInputPolicy; -pub use crate::input::EqValueInputPolicy; -pub use crate::input::ExplicitInputPolicy; pub use crate::input::InputStorage; pub use crate::runtime::Revision; diff --git a/tests/gc/derived_tests.rs b/tests/gc/derived_tests.rs index 53e345c..8822337 100644 --- a/tests/gc/derived_tests.rs +++ b/tests/gc/derived_tests.rs @@ -18,6 +18,7 @@ fn compute_one() { let db = db::DatabaseImpl::default(); // Will compute fibonacci(5) + db.query(UseTriangular).set(5, false); db.compute(5); db.salsa_runtime().next_revision(); @@ -27,7 +28,7 @@ fn compute_one() { Triangular => (), Fibonacci => (0, 1, 2, 3, 4, 5), Compute => (5), - UseTriangular => (), + UseTriangular => (5), Min => (), Max => (), } @@ -41,7 +42,7 @@ fn compute_one() { Triangular => (), Fibonacci => (5), Compute => (5), - UseTriangular => (), + UseTriangular => (5), Min => (), Max => (), } @@ -52,6 +53,7 @@ fn compute_switch() { let db = db::DatabaseImpl::default(); // Will compute fibonacci(5) + db.query(UseTriangular).set(5, false); assert_eq!(db.compute(5), 5); // Change to triangular mode @@ -107,9 +109,9 @@ fn compute_switch() { fn compute_all() { let db = db::DatabaseImpl::default(); - db.query(UseTriangular).set(1, true); - db.query(UseTriangular).set(3, true); - db.query(UseTriangular).set(5, true); + for i in 0..6 { + db.query(UseTriangular).set(i, (i % 2) != 0); + } db.query(Min).set((), 0); db.query(Max).set((), 6); @@ -125,7 +127,7 @@ fn compute_all() { Fibonacci => (0, 2, 4), Compute => (0, 1, 2, 3, 4, 5), ComputeAll => (()), - UseTriangular => (1, 3, 5), + UseTriangular => (0, 1, 2, 3, 4, 5), Min => (()), Max => (()), } @@ -140,7 +142,7 @@ fn compute_all() { Fibonacci => (0, 2, 4), Compute => (0, 1, 2, 3, 4, 5), ComputeAll => (()), - UseTriangular => (1, 3, 5), + UseTriangular => (0, 1, 2, 3, 4, 5), Min => (()), Max => (()), } @@ -155,7 +157,7 @@ fn compute_all() { Fibonacci => (0, 2, 4), Compute => (0, 1, 2, 3, 4), ComputeAll => (()), - UseTriangular => (1, 3, 5), + UseTriangular => (0, 1, 2, 3, 4, 5), Min => (()), Max => (()), } diff --git a/tests/gc/group.rs b/tests/gc/group.rs index 269ed11..8c4aa41 100644 --- a/tests/gc/group.rs +++ b/tests/gc/group.rs @@ -14,7 +14,7 @@ salsa::query_group! { fn use_triangular(key: usize) -> bool { type UseTriangular; - storage (input default); + storage input; } fn fibonacci(key: usize) -> usize { diff --git a/tests/incremental/constants.rs b/tests/incremental/constants.rs index 856859f..1d7ebc4 100644 --- a/tests/incremental/constants.rs +++ b/tests/incremental/constants.rs @@ -6,7 +6,7 @@ salsa::query_group! { pub(crate) trait ConstantsDatabase: TestContext { fn constants_input(key: char) -> usize { type ConstantsInput; - storage (input default eq); + storage input; } fn constants_add(keys: (char, char)) -> usize { @@ -43,9 +43,10 @@ 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 that invoking `set` on a constant is an error, even if you +/// don't change the value. #[test] +#[should_panic] fn set_after_constant_same_value() { let db = &TestContextImpl::default(); db.query(ConstantsInput).set_constant('a', 44); @@ -99,29 +100,3 @@ fn becomes_constant_with_change() { 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)"]); -} diff --git a/tests/incremental/memoized_dep_inputs.rs b/tests/incremental/memoized_dep_inputs.rs index f71fcf1..fd30599 100644 --- a/tests/incremental/memoized_dep_inputs.rs +++ b/tests/incremental/memoized_dep_inputs.rs @@ -15,7 +15,7 @@ salsa::query_group! { } fn dep_input1() -> usize { type Input1; - storage (input default); + storage input; } fn dep_input2() -> usize { type Input2; @@ -43,6 +43,8 @@ fn dep_derived1(db: &impl MemoizedDepInputsContext) -> usize { fn revalidate() { let db = &TestContextImpl::default(); + db.query(Input1).set((), 0); + // Initial run starts from Memoized2: let v = db.dep_memoized2(); assert_eq!(v, 0); diff --git a/tests/incremental/memoized_inputs.rs b/tests/incremental/memoized_inputs.rs index de9c2e1..5ff409b 100644 --- a/tests/incremental/memoized_inputs.rs +++ b/tests/incremental/memoized_inputs.rs @@ -8,11 +8,11 @@ salsa::query_group! { } fn input1() -> usize { type Input1; - storage (input default eq); + storage input; } fn input2() -> usize { type Input2; - storage (input default eq); + storage input; } } } @@ -26,6 +26,9 @@ fn max(db: &impl MemoizedInputsContext) -> usize { fn revalidate() { let db = &TestContextImpl::default(); + db.query(Input1).set((), 0); + db.query(Input2).set((), 0); + let v = db.max(); assert_eq!(v, 0); db.assert_log(&["Max invoked"]); @@ -61,12 +64,14 @@ fn revalidate() { db.assert_log(&[]); } -/// Test that invoking `set` on an input with the same value does not -/// trigger a new revision. +/// Test that invoking `set` on an input with the same value still +/// triggers a new revision. #[test] fn set_after_no_change() { let db = &TestContextImpl::default(); + db.query(Input2).set((), 0); + db.query(Input1).set((), 44); let v = db.max(); assert_eq!(v, 44); @@ -75,5 +80,5 @@ fn set_after_no_change() { db.query(Input1).set((), 44); let v = db.max(); assert_eq!(v, 44); - db.assert_log(&[]); + db.assert_log(&["Max invoked"]); }