diff --git a/src/accumulator.rs b/src/accumulator.rs index dad727ca..bb961b99 100644 --- a/src/accumulator.rs +++ b/src/accumulator.rs @@ -8,7 +8,7 @@ use std::{ use crate::{ cycle::CycleRecoveryStrategy, hash::FxDashMap, - ingredient::{fmt_index, Ingredient, IngredientRequiresReset, Jar}, + ingredient::{fmt_index, Ingredient, Jar}, key::DependencyIndex, runtime::local_state::QueryOrigin, storage::IngredientIndex, @@ -183,6 +183,10 @@ impl Ingredient for IngredientImpl { } } + fn requires_reset_for_new_revision(&self) -> bool { + false + } + fn reset_for_new_revision(&mut self) { panic!("unexpected reset on accumulator") } @@ -196,10 +200,6 @@ impl Ingredient for IngredientImpl { } } -impl IngredientRequiresReset for IngredientImpl { - const RESET_ON_NEW_REVISION: bool = false; -} - impl std::fmt::Debug for IngredientImpl where A: Accumulator, diff --git a/src/function.rs b/src/function.rs index 8098e451..4633bcd0 100644 --- a/src/function.rs +++ b/src/function.rs @@ -4,7 +4,7 @@ use crossbeam::atomic::AtomicCell; use crate::{ cycle::CycleRecoveryStrategy, - ingredient::{fmt_index, IngredientRequiresReset}, + ingredient::fmt_index, key::DatabaseKeyIndex, runtime::local_state::QueryOrigin, salsa_struct::SalsaStructInDb, @@ -254,6 +254,10 @@ where // Since its `verified_at` field has not changed, it will be considered dirty if it is invoked. } + fn requires_reset_for_new_revision(&self) -> bool { + true + } + fn reset_for_new_revision(&mut self) { std::mem::take(&mut self.deleted_entries); } @@ -293,10 +297,3 @@ where .finish() } } - -impl IngredientRequiresReset for IngredientImpl -where - C: Configuration, -{ - const RESET_ON_NEW_REVISION: bool = true; -} diff --git a/src/ingredient.rs b/src/ingredient.rs index e793b77d..4751816a 100644 --- a/src/ingredient.rs +++ b/src/ingredient.rs @@ -65,6 +65,10 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync { /// since only function ingredients push themselves onto the active query stack.) fn cycle_recovery_strategy(&self) -> CycleRecoveryStrategy; + /// Returns true if `reset_for_new_revision` should be called when new revisions start. + /// Invoked once when ingredient is added and not after that. + fn requires_reset_for_new_revision(&self) -> bool; + /// Invoked when a new revision is about to start. /// This moment is important because it means that we have an `&mut`-reference to the /// database, and hence any pre-existing `&`-references must have expired. @@ -128,11 +132,3 @@ pub(crate) fn fmt_index( write!(fmt, "{}()", debug_name) } } - -/// Defines a const indicating if an ingredient needs to be reset each round. -/// This const probably *should* be a member of `Ingredient` trait but then `Ingredient` would -/// not be dyn-safe. -pub trait IngredientRequiresReset { - /// If this is true, then `reset_for_new_revision` will be called every new revision. - const RESET_ON_NEW_REVISION: bool; -} diff --git a/src/input.rs b/src/input.rs index d1769c61..2fd24814 100644 --- a/src/input.rs +++ b/src/input.rs @@ -15,7 +15,7 @@ use struct_map::StructMap; use crate::{ cycle::CycleRecoveryStrategy, id::{AsId, FromId}, - ingredient::{fmt_index, Ingredient, IngredientRequiresReset}, + ingredient::{fmt_index, Ingredient}, key::{DatabaseKeyIndex, DependencyIndex}, plumbing::{Jar, Stamp}, runtime::{local_state::QueryOrigin, Runtime}, @@ -225,6 +225,10 @@ impl Ingredient for IngredientImpl { ); } + fn requires_reset_for_new_revision(&self) -> bool { + false + } + fn reset_for_new_revision(&mut self) { panic!("unexpected call to `reset_for_new_revision`") } @@ -240,10 +244,6 @@ impl Ingredient for IngredientImpl { } } -impl IngredientRequiresReset for IngredientImpl { - const RESET_ON_NEW_REVISION: bool = false; -} - impl std::fmt::Debug for IngredientImpl { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct(std::any::type_name::()) diff --git a/src/input/input_field.rs b/src/input/input_field.rs index 0c0c1b5b..fccd1ca3 100644 --- a/src/input/input_field.rs +++ b/src/input/input_field.rs @@ -1,5 +1,5 @@ use crate::cycle::CycleRecoveryStrategy; -use crate::ingredient::{fmt_index, Ingredient, IngredientRequiresReset}; +use crate::ingredient::{fmt_index, Ingredient}; use crate::input::Configuration; use crate::runtime::local_state::QueryOrigin; use crate::storage::IngredientIndex; @@ -86,6 +86,10 @@ where panic!("unexpected call: input fields are never deleted"); } + fn requires_reset_for_new_revision(&self) -> bool { + false + } + fn reset_for_new_revision(&mut self) { panic!("unexpected call: input fields don't register for resets"); } @@ -95,13 +99,6 @@ where } } -impl IngredientRequiresReset for FieldIngredientImpl -where - C: Configuration, -{ - const RESET_ON_NEW_REVISION: bool = false; -} - impl std::fmt::Debug for FieldIngredientImpl where C: Configuration, diff --git a/src/interned.rs b/src/interned.rs index eee48f59..b9a6912b 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -7,7 +7,7 @@ use std::ptr::NonNull; use crate::alloc::Alloc; use crate::durability::Durability; use crate::id::AsId; -use crate::ingredient::{fmt_index, IngredientRequiresReset}; +use crate::ingredient::fmt_index; use crate::key::DependencyIndex; use crate::plumbing::Jar; use crate::runtime::local_state::QueryOrigin; @@ -253,6 +253,10 @@ where ); } + fn requires_reset_for_new_revision(&self) -> bool { + false + } + fn reset_for_new_revision(&mut self) { // Interned ingredients do not, normally, get deleted except when they are "reset" en masse. // There ARE methods (e.g., `clear_deleted_entries` and `remove`) for deleting individual @@ -269,13 +273,6 @@ where } } -impl IngredientRequiresReset for IngredientImpl -where - C: Configuration, -{ - const RESET_ON_NEW_REVISION: bool = false; -} - impl std::fmt::Debug for IngredientImpl where C: Configuration, diff --git a/src/storage.rs b/src/storage.rs index 0038b348..4dc298c6 100644 --- a/src/storage.rs +++ b/src/storage.rs @@ -236,6 +236,9 @@ struct Shared { /// /// Immutable unless the mutex on `ingredients_map` is held. ingredients_vec: ConcurrentVec>, + + /// Indices of ingredients that require reset when a new revision starts. + ingredients_requiring_reset: ConcurrentVec, } // ANCHOR: default @@ -247,6 +250,7 @@ impl Default for Storage { nonce: NONCE.nonce(), jar_map: Default::default(), ingredients_vec: Default::default(), + ingredients_requiring_reset: Default::default(), }, runtime: Runtime::default(), } @@ -276,6 +280,11 @@ impl Storage { let ingredients = jar.create_ingredients(index); for ingredient in ingredients { let expected_index = ingredient.ingredient_index(); + + if ingredient.requires_reset_for_new_revision() { + self.shared.ingredients_requiring_reset.push(expected_index); + } + let actual_index = self .shared .ingredients_vec @@ -288,6 +297,7 @@ impl Storage { expected_index, actual_index, ); + } index }) @@ -308,6 +318,10 @@ impl Storage { ) -> (&mut dyn Ingredient, &mut Runtime) { self.runtime.new_revision(); + for index in self.shared.ingredients_requiring_reset.iter() { + self.shared.ingredients_vec.get_mut(index.as_usize()).unwrap().reset_for_new_revision(); + } + ( &mut **self .shared @@ -337,7 +351,7 @@ unsafe impl Sync for IngredientCache where I: Ingredient + Sync {} impl Default for IngredientCache where I: Ingredient, - { +{ fn default() -> Self { Self::new() } diff --git a/src/tracked_struct.rs b/src/tracked_struct.rs index 746f9fa4..46423a88 100644 --- a/src/tracked_struct.rs +++ b/src/tracked_struct.rs @@ -8,7 +8,7 @@ use crate::{ cycle::CycleRecoveryStrategy, hash::FxDashMap, id::AsId, - ingredient::{fmt_index, Ingredient, IngredientRequiresReset, Jar}, + ingredient::{fmt_index, Ingredient, Jar}, ingredient_list::IngredientList, key::{DatabaseKeyIndex, DependencyIndex}, runtime::{local_state::QueryOrigin, Runtime}, @@ -470,6 +470,10 @@ where self.delete_entity(db.as_salsa_database(), stale_output_key.unwrap()); } + fn requires_reset_for_new_revision(&self) -> bool { + true + } + fn reset_for_new_revision(&mut self) { self.struct_map.drop_deleted_entries(); } @@ -494,13 +498,6 @@ where } } -impl IngredientRequiresReset for IngredientImpl -where - C: Configuration, -{ - const RESET_ON_NEW_REVISION: bool = true; -} - impl Value where C: Configuration, diff --git a/src/tracked_struct/tracked_field.rs b/src/tracked_struct/tracked_field.rs index 99aeb6fe..86bf94d9 100644 --- a/src/tracked_struct/tracked_field.rs +++ b/src/tracked_struct/tracked_field.rs @@ -1,9 +1,6 @@ use crate::{ - id::AsId, - ingredient::{Ingredient, IngredientRequiresReset}, - key::DependencyIndex, - storage::IngredientIndex, - Database, Id, Runtime, + id::AsId, ingredient::Ingredient, key::DependencyIndex, storage::IngredientIndex, Database, Id, + Runtime, }; use super::{struct_map::StructMapView, Configuration}; @@ -119,6 +116,10 @@ where panic!("tracked field ingredients are not registered as dependent") } + fn requires_reset_for_new_revision(&self) -> bool { + false + } + fn reset_for_new_revision(&mut self) { panic!("tracked field ingredients do not require reset") } @@ -149,10 +150,3 @@ where .finish() } } - -impl IngredientRequiresReset for FieldIngredientImpl -where - C: Configuration, -{ - const RESET_ON_NEW_REVISION: bool = false; -} diff --git a/tests/deletion-drops.rs b/tests/deletion-drops.rs new file mode 100644 index 00000000..cd8bf2d6 --- /dev/null +++ b/tests/deletion-drops.rs @@ -0,0 +1,99 @@ +//! Basic deletion test: +//! +//! * entities not created in a revision are deleted, as is any memoized data keyed on them. + +mod common; + +use salsa::{Database, Setter}; +use test_log::test; + +#[salsa::input] +struct MyInput { + identity: u32, +} + +#[salsa::tracked] +struct MyTracked<'db> { + #[id] + identifier: u32, + + #[return_ref] + field: Bomb, +} + +thread_local! { + static DROPPED: std::cell::RefCell> = std::cell::RefCell::new(vec![]); +} + +fn dropped() -> Vec { + DROPPED.with(|d| d.borrow().clone()) +} + +#[derive(Clone, Debug, PartialEq, Eq)] +struct Bomb { + identity: u32, +} + +impl Drop for Bomb { + fn drop(&mut self) { + DROPPED.with(|d| d.borrow_mut().push(self.identity)); + } +} + +#[salsa::tracked] +impl MyInput { + #[salsa::tracked] + fn create_tracked_struct(self, db: &dyn Database) -> MyTracked<'_> { + MyTracked::new( + db, + self.identity(db), + Bomb { + identity: self.identity(db), + }, + ) + } +} + +#[test] +fn deletion_drops() { + let mut db = salsa::default_database(); + + let input = MyInput::new(&db, 22); + + expect_test::expect![[r#" + [] + "#]] + .assert_debug_eq(&dropped()); + + let tracked_struct = input.create_tracked_struct(&db); + assert_eq!(tracked_struct.field(&db).identity, 22); + + expect_test::expect![[r#" + [] + "#]] + .assert_debug_eq(&dropped()); + + input.set_identity(&mut db).to(44); + + expect_test::expect![[r#" + [] + "#]] + .assert_debug_eq(&dropped()); + + let tracked_struct = input.create_tracked_struct(&db); + assert_eq!(tracked_struct.field(&db).identity, 44); + + expect_test::expect![[r#" + [] + "#]] + .assert_debug_eq(&dropped()); + + input.set_identity(&mut db).to(66); + + expect_test::expect![[r#" + [ + 22, + ] + "#]] + .assert_debug_eq(&dropped()); +}