From eabb55632eb495323b096ecb4b8542fb19f227c6 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 12 Aug 2022 14:28:20 -0400 Subject: [PATCH 1/7] Revert "create `remove_stale_output` method on ingredients" This reverts commit 8b7324dca886001df7e7bf7c45567520954fec6b. --- components/salsa-2022-macros/src/db.rs | 6 ------ components/salsa-2022/src/accumulator.rs | 4 ---- components/salsa-2022/src/function.rs | 5 ----- components/salsa-2022/src/function/diff_outputs.rs | 6 ++---- components/salsa-2022/src/ingredient.rs | 9 +-------- components/salsa-2022/src/input.rs | 4 ---- components/salsa-2022/src/interned.rs | 5 ----- components/salsa-2022/src/storage.rs | 2 -- components/salsa-2022/src/tracked_struct.rs | 5 ----- 9 files changed, 3 insertions(+), 43 deletions(-) diff --git a/components/salsa-2022-macros/src/db.rs b/components/salsa-2022-macros/src/db.rs index 74573df0..0d1151fc 100644 --- a/components/salsa-2022-macros/src/db.rs +++ b/components/salsa-2022-macros/src/db.rs @@ -140,12 +140,6 @@ fn has_jars_dyn_impl(input: &syn::ItemStruct, storage: &syn::Ident) -> syn::Item let ingredient = self.#storage.ingredient(index.ingredient_index()); ingredient.inputs(index.key_index()) } - - fn remove_stale_output(&self, executor: salsa::DatabaseKeyIndex, stale_output: salsa::key::DependencyIndex) { - let ingredient = self.#storage.ingredient(stale_output.ingredient_index()); - ingredient.remove_stale_output(executor, stale_output.key_index()); - } - } } } diff --git a/components/salsa-2022/src/accumulator.rs b/components/salsa-2022/src/accumulator.rs index 27f89a25..a685ef67 100644 --- a/components/salsa-2022/src/accumulator.rs +++ b/components/salsa-2022/src/accumulator.rs @@ -81,10 +81,6 @@ where fn inputs(&self, _key_index: crate::Id) -> Option { None } - - fn remove_stale_output(&self, executor: DatabaseKeyIndex, stale_output_key: Option) { - // FIXME - } } impl MutIngredient for AccumulatorIngredient diff --git a/components/salsa-2022/src/function.rs b/components/salsa-2022/src/function.rs index 4c27987c..8d543fbb 100644 --- a/components/salsa-2022/src/function.rs +++ b/components/salsa-2022/src/function.rs @@ -203,11 +203,6 @@ where let key = C::key_from_id(key_index); self.inputs(key) } - - fn remove_stale_output(&self, executor: DatabaseKeyIndex, stale_output_key: Option) { - let stale_output_key = C::key_from_id(stale_output_key.unwrap()); - // FIXME - } } impl MutIngredient for FunctionIngredient diff --git a/components/salsa-2022/src/function/diff_outputs.rs b/components/salsa-2022/src/function/diff_outputs.rs index 81a67e67..24398746 100644 --- a/components/salsa-2022/src/function/diff_outputs.rs +++ b/components/salsa-2022/src/function/diff_outputs.rs @@ -1,6 +1,6 @@ use crate::{ - key::DependencyIndex, runtime::local_state::QueryRevisions, storage::HasJarsDyn, Database, - DatabaseKeyIndex, Event, EventKind, + key::DependencyIndex, runtime::local_state::QueryRevisions, Database, DatabaseKeyIndex, Event, + EventKind, }; use super::{memo::Memo, Configuration, DynDb, FunctionIngredient}; @@ -59,7 +59,5 @@ where output_key: output, }, }); - - db.remove_stale_output(key, output); } } diff --git a/components/salsa-2022/src/ingredient.rs b/components/salsa-2022/src/ingredient.rs index 058a3fbe..d2aef7b4 100644 --- a/components/salsa-2022/src/ingredient.rs +++ b/components/salsa-2022/src/ingredient.rs @@ -1,6 +1,5 @@ use crate::{ - cycle::CycleRecoveryStrategy, key::DependencyIndex, runtime::local_state::QueryEdges, - DatabaseKeyIndex, Id, + cycle::CycleRecoveryStrategy, key::DependencyIndex, runtime::local_state::QueryEdges, Id, }; use super::Revision; @@ -23,12 +22,6 @@ pub trait Ingredient { /// What were the inputs (if any) that were used to create the value at `key_index`. fn inputs(&self, key_index: Id) -> Option; - - /// Invoked when the value `stale_output` was output by `executor` in a previous - /// revision, but was NOT output in the current revision. - /// - /// This hook is used to clear out the stale value so others cannot read it. - fn remove_stale_output(&self, executor: DatabaseKeyIndex, stale_output_key: Option); } /// Optional trait for ingredients that wish to be notified when new revisions are diff --git a/components/salsa-2022/src/input.rs b/components/salsa-2022/src/input.rs index 6b5737c6..3fb7afc8 100644 --- a/components/salsa-2022/src/input.rs +++ b/components/salsa-2022/src/input.rs @@ -61,8 +61,4 @@ where fn inputs(&self, _key_index: crate::Id) -> Option { None } - - fn remove_stale_output(&self, executor: DatabaseKeyIndex, stale_output_key: Option) { - unreachable!("input cannot be the output of a tracked function"); - } } diff --git a/components/salsa-2022/src/interned.rs b/components/salsa-2022/src/interned.rs index 3e867a06..cf6c88fd 100644 --- a/components/salsa-2022/src/interned.rs +++ b/components/salsa-2022/src/interned.rs @@ -8,7 +8,6 @@ use crate::id::AsId; use crate::key::DependencyIndex; use crate::runtime::local_state::QueryEdges; use crate::runtime::Runtime; -use crate::DatabaseKeyIndex; use super::hash::FxDashMap; use super::ingredient::Ingredient; @@ -198,10 +197,6 @@ where fn inputs(&self, _key_index: crate::Id) -> Option { None } - - fn remove_stale_output(&self, executor: DatabaseKeyIndex, stale_output_key: Option) { - unreachable!("interned ids are not outputs"); - } } pub struct IdentityInterner { diff --git a/components/salsa-2022/src/storage.rs b/components/salsa-2022/src/storage.rs index 8fab71e2..5384a76a 100644 --- a/components/salsa-2022/src/storage.rs +++ b/components/salsa-2022/src/storage.rs @@ -179,8 +179,6 @@ pub trait HasJarsDyn { fn cycle_recovery_strategy(&self, input: IngredientIndex) -> CycleRecoveryStrategy; fn inputs(&self, input: DatabaseKeyIndex) -> Option; - - fn remove_stale_output(&self, executor: DatabaseKeyIndex, stale_output: DependencyIndex); } pub trait HasIngredientsFor diff --git a/components/salsa-2022/src/tracked_struct.rs b/components/salsa-2022/src/tracked_struct.rs index 59550370..c57819cf 100644 --- a/components/salsa-2022/src/tracked_struct.rs +++ b/components/salsa-2022/src/tracked_struct.rs @@ -117,11 +117,6 @@ where fn inputs(&self, _key_index: crate::Id) -> Option { None } - - fn remove_stale_output(&self, executor: DatabaseKeyIndex, stale_output_key: Option) { - let key: Id = Id::from_id(stale_output_key.unwrap()); - // FIXME -- we can delete this entity - } } impl MutIngredient for TrackedStructIngredient From afefcdf33563416041dcf0beaf6f43295c155a98 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 12 Aug 2022 14:28:25 -0400 Subject: [PATCH 2/7] Revert "diff outputs when replacing a memoized value" This reverts commit 1e3272bc6117cf6dde37d7a4493403112478e730. --- components/salsa-2022/src/database.rs | 4 +- components/salsa-2022/src/event.rs | 29 +--- components/salsa-2022/src/function.rs | 1 - .../salsa-2022/src/function/diff_outputs.rs | 63 -------- components/salsa-2022/src/function/execute.rs | 1 - components/salsa-2022/src/function/specify.rs | 5 +- .../specify_tracked_fn_in_rev_1_but_not_2.rs | 147 ++---------------- 7 files changed, 19 insertions(+), 231 deletions(-) delete mode 100644 components/salsa-2022/src/function/diff_outputs.rs diff --git a/components/salsa-2022/src/database.rs b/components/salsa-2022/src/database.rs index 15104844..c6077f45 100644 --- a/components/salsa-2022/src/database.rs +++ b/components/salsa-2022/src/database.rs @@ -7,8 +7,8 @@ pub trait Database: HasJarsDyn + AsSalsaDatabase { /// /// By default, the event is logged at level debug using /// the standard `log` facade. - fn salsa_event(&self, event: Event) { - log::debug!("salsa_event: {:?}", event.debug(self)); + fn salsa_event(&self, event_fn: Event) { + log::debug!("salsa_event: {:?}", event_fn.debug(self)); } fn salsa_runtime(&self) -> &Runtime; diff --git a/components/salsa-2022/src/event.rs b/components/salsa-2022/src/event.rs index e35bfd43..c328df97 100644 --- a/components/salsa-2022/src/event.rs +++ b/components/salsa-2022/src/event.rs @@ -1,6 +1,4 @@ -use crate::{ - debug::DebugWithDb, key::DependencyIndex, runtime::RuntimeId, Database, DatabaseKeyIndex, -}; +use crate::{debug::DebugWithDb, runtime::RuntimeId, Database, DatabaseKeyIndex}; use std::fmt; /// The `Event` struct identifies various notable things that can @@ -76,15 +74,6 @@ pub enum EventKind { /// Indicates that `unwind_if_cancelled` was called and salsa will check if /// the current revision has been cancelled. WillCheckCancellation, - - /// Discovered that a query used to output a given output but no longer does. - WillDiscardStaleOutput { - /// Key for the query that is executing and which no longer outputs the given value. - execute_key: DatabaseKeyIndex, - - /// Key for the query that is no longer output - output_key: DependencyIndex, - }, } impl fmt::Debug for EventKind { @@ -107,14 +96,6 @@ impl fmt::Debug for EventKind { .field("database_key", database_key) .finish(), EventKind::WillCheckCancellation => fmt.debug_struct("WillCheckCancellation").finish(), - EventKind::WillDiscardStaleOutput { - execute_key, - output_key, - } => fmt - .debug_struct("WillDiscardStaleOutput") - .field("execute_key", &execute_key) - .field("output_key", &output_key) - .finish(), } } } @@ -142,14 +123,6 @@ where .field("database_key", &database_key.debug(db)) .finish(), EventKind::WillCheckCancellation => fmt.debug_struct("WillCheckCancellation").finish(), - EventKind::WillDiscardStaleOutput { - execute_key, - output_key, - } => fmt - .debug_struct("WillDiscardStaleOutput") - .field("execute_key", &execute_key.debug(db)) - .field("output_key", &output_key.debug(db)) - .finish(), } } } diff --git a/components/salsa-2022/src/function.rs b/components/salsa-2022/src/function.rs index 8d543fbb..1ac8793a 100644 --- a/components/salsa-2022/src/function.rs +++ b/components/salsa-2022/src/function.rs @@ -17,7 +17,6 @@ use super::{ingredient::Ingredient, routes::IngredientIndex, AsId}; mod accumulated; mod backdate; -mod diff_outputs; mod execute; mod fetch; mod inputs; diff --git a/components/salsa-2022/src/function/diff_outputs.rs b/components/salsa-2022/src/function/diff_outputs.rs deleted file mode 100644 index 24398746..00000000 --- a/components/salsa-2022/src/function/diff_outputs.rs +++ /dev/null @@ -1,63 +0,0 @@ -use crate::{ - key::DependencyIndex, runtime::local_state::QueryRevisions, Database, DatabaseKeyIndex, Event, - EventKind, -}; - -use super::{memo::Memo, Configuration, DynDb, FunctionIngredient}; - -impl FunctionIngredient -where - C: Configuration, -{ - /// Compute the old and new outputs and invoke the `clear_stale_output` callback - /// for each output that was generated before but is not generated now. - pub(super) fn diff_outputs( - &self, - db: &DynDb<'_, C>, - key: DatabaseKeyIndex, - old_memo: &Memo, - revisions: &QueryRevisions, - ) { - let mut old_outputs = old_memo - .revisions - .edges - .outputs() - .iter() - .copied() - .peekable(); - let mut new_outputs = revisions.edges.outputs().iter().copied().peekable(); - - // two list are in sorted order, we can merge them in linear time. - while let (Some(&old_output), Some(&new_output)) = (old_outputs.peek(), new_outputs.peek()) - { - if old_output < new_output { - // Output that was generated but is no longer. - Self::report_stale_output(db, key, old_output); - old_outputs.next(); - } else if new_output < old_output { - // This is a new output that was not generated before. - // No action needed. - new_outputs.next(); - } else { - // Output generated both times. - old_outputs.next(); - new_outputs.next(); - } - } - - for old_output in old_outputs { - Self::report_stale_output(db, key, old_output); - } - } - - fn report_stale_output(db: &DynDb<'_, C>, key: DatabaseKeyIndex, output: DependencyIndex) { - let runtime_id = db.salsa_runtime().id(); - db.salsa_event(Event { - runtime_id, - kind: EventKind::WillDiscardStaleOutput { - execute_key: key, - output_key: output, - }, - }); - } -} diff --git a/components/salsa-2022/src/function/execute.rs b/components/salsa-2022/src/function/execute.rs index 51e3867e..12a45ecb 100644 --- a/components/salsa-2022/src/function/execute.rs +++ b/components/salsa-2022/src/function/execute.rs @@ -87,7 +87,6 @@ where // old value. if let Some(old_memo) = &opt_old_memo { self.backdate_if_appropriate(old_memo, &mut revisions, &value); - self.diff_outputs(db, database_key_index, &old_memo, &revisions); } let value = self diff --git a/components/salsa-2022/src/function/specify.rs b/components/salsa-2022/src/function/specify.rs index 49e9fd1b..6c879707 100644 --- a/components/salsa-2022/src/function/specify.rs +++ b/components/salsa-2022/src/function/specify.rs @@ -26,8 +26,8 @@ where None => panic!("can only use `set` with an active query"), }; - let database_key_index = key.database_key_index(db); - if !runtime.is_output_of_active_query(database_key_index) { + let entity_index = key.database_key_index(db); + if !runtime.is_output_of_active_query(entity_index) { panic!("can only use `set` on entities created during current query"); } @@ -64,7 +64,6 @@ where if let Some(old_memo) = self.memo_map.get(key) { self.backdate_if_appropriate(&old_memo, &mut revisions, &value); - self.diff_outputs(db, database_key_index, &old_memo, &revisions); } let memo = Memo { diff --git a/salsa-2022-tests/tests/specify_tracked_fn_in_rev_1_but_not_2.rs b/salsa-2022-tests/tests/specify_tracked_fn_in_rev_1_but_not_2.rs index c955b9d8..0a8e9138 100644 --- a/salsa-2022-tests/tests/specify_tracked_fn_in_rev_1_but_not_2.rs +++ b/salsa-2022-tests/tests/specify_tracked_fn_in_rev_1_but_not_2.rs @@ -2,7 +2,6 @@ //! compiles and executes successfully. use expect_test::expect; -use salsa::DebugWithDb; use salsa_2022_tests::{HasLogger, Logger}; use test_log::test; @@ -75,10 +74,6 @@ impl salsa::Database for Database { fn salsa_runtime(&self) -> &salsa::Runtime { self.storage.runtime() } - - fn salsa_event(&self, event: salsa::Event) { - self.push_log(format!("{:?}", event.debug(self))); - } } impl Db for Database {} @@ -97,17 +92,9 @@ fn test_run_0() { assert_eq!(final_result(&db, input), 100); db.assert_logs(expect![[r#" [ - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(7), key_index: Some(Id { value: 1 }) } } }", "final_result(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 1 }) } } }", "create_tracked(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(5), key_index: Some(Id { value: 1 }) } } }", "read_maybe_specified(MyTracked(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", ]"#]]); } @@ -119,17 +106,9 @@ fn test_run_5() { assert_eq!(final_result(&db, input), 100); db.assert_logs(expect![[r#" [ - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(7), key_index: Some(Id { value: 1 }) } } }", "final_result(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 1 }) } } }", "create_tracked(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(5), key_index: Some(Id { value: 1 }) } } }", "read_maybe_specified(MyTracked(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", ]"#]]); } @@ -141,21 +120,10 @@ fn test_run_10() { assert_eq!(final_result(&db, input), 100); db.assert_logs(expect![[r#" [ - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(7), key_index: Some(Id { value: 1 }) } } }", "final_result(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 1 }) } } }", "create_tracked(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(5), key_index: Some(Id { value: 1 }) } } }", "read_maybe_specified(MyTracked(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(4), key_index: Some(Id { value: 1 }) } } }", "maybe_specified(MyTracked(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", ]"#]]); } @@ -167,21 +135,10 @@ fn test_run_20() { assert_eq!(final_result(&db, input), 200); db.assert_logs(expect![[r#" [ - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(7), key_index: Some(Id { value: 1 }) } } }", "final_result(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 1 }) } } }", "create_tracked(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(5), key_index: Some(Id { value: 1 }) } } }", "read_maybe_specified(MyTracked(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(4), key_index: Some(Id { value: 1 }) } } }", "maybe_specified(MyTracked(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", ]"#]]); } @@ -193,17 +150,9 @@ fn test_run_0_then_5_then_20() { assert_eq!(final_result(&db, input), 100); db.assert_logs(expect![[r#" [ - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(7), key_index: Some(Id { value: 1 }) } } }", "final_result(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 1 }) } } }", "create_tracked(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(5), key_index: Some(Id { value: 1 }) } } }", "read_maybe_specified(MyTracked(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", ]"#]]); // FIXME: read_maybe_specified should not re-execute @@ -211,35 +160,16 @@ fn test_run_0_then_5_then_20() { assert_eq!(final_result(&db, input), 100); db.assert_logs(expect![[r#" [ - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(7), key_index: Some(Id { value: 2 }) } } }", "final_result(MyInput(Id { value: 2 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 2 }) } } }", "create_tracked(MyInput(Id { value: 2 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(5), key_index: Some(Id { value: 2 }) } } }", "read_maybe_specified(MyTracked(Id { value: 2 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", ]"#]]); input.set_field(&mut db, 20); assert_eq!(final_result(&db, input), 100); // FIXME: Should be 20. db.assert_logs(expect![[r#" [ - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 2 }) } } }", "create_tracked(MyInput(Id { value: 2 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillDiscardStaleOutput { execute_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 2 }) }, output_key: DependencyIndex { ingredient_index: IngredientIndex(4), key_index: Some(Id { value: 2 }) } } }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: DidValidateMemoizedValue { database_key: DependencyIndex { ingredient_index: IngredientIndex(4), key_index: Some(Id { value: 2 }) } } }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: DidValidateMemoizedValue { database_key: DependencyIndex { ingredient_index: IngredientIndex(5), key_index: Some(Id { value: 2 }) } } }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: DidValidateMemoizedValue { database_key: DependencyIndex { ingredient_index: IngredientIndex(7), key_index: Some(Id { value: 2 }) } } }", ]"#]]); // FIXME: should invoke maybe_specified } @@ -251,71 +181,41 @@ fn test_run_0_then_5_then_10_then_20() { assert_eq!(final_result(&db, input), 100); db.assert_logs(expect![[r#" [ - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(7), key_index: Some(Id { value: 1 }) } } }", "final_result(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 1 }) } } }", "create_tracked(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(5), key_index: Some(Id { value: 1 }) } } }", "read_maybe_specified(MyTracked(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", ]"#]]); // FIXME: `read_maybe_specified` should not re-execute - input.set_field(&mut db, 5); + let input = MyInput::new(&mut db, 5); assert_eq!(final_result(&db, input), 100); db.assert_logs(expect![[r#" [ - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 1 }) } } }", - "create_tracked(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: DidValidateMemoizedValue { database_key: DependencyIndex { ingredient_index: IngredientIndex(5), key_index: Some(Id { value: 1 }) } } }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: DidValidateMemoizedValue { database_key: DependencyIndex { ingredient_index: IngredientIndex(7), key_index: Some(Id { value: 1 }) } } }", + "final_result(MyInput(Id { value: 2 }))", + "create_tracked(MyInput(Id { value: 2 }))", + "read_maybe_specified(MyTracked(Id { value: 2 }))", ]"#]]); // FIXME: should execute `maybe_specified` but not `read_maybe_specified` - input.set_field(&mut db, 10); + let input = MyInput::new(&mut db, 10); assert_eq!(final_result(&db, input), 100); db.assert_logs(expect![[r#" [ - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 1 }) } } }", - "create_tracked(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillDiscardStaleOutput { execute_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 1 }) }, output_key: DependencyIndex { ingredient_index: IngredientIndex(4), key_index: Some(Id { value: 1 }) } } }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: DidValidateMemoizedValue { database_key: DependencyIndex { ingredient_index: IngredientIndex(4), key_index: Some(Id { value: 1 }) } } }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: DidValidateMemoizedValue { database_key: DependencyIndex { ingredient_index: IngredientIndex(5), key_index: Some(Id { value: 1 }) } } }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: DidValidateMemoizedValue { database_key: DependencyIndex { ingredient_index: IngredientIndex(7), key_index: Some(Id { value: 1 }) } } }", + "final_result(MyInput(Id { value: 3 }))", + "create_tracked(MyInput(Id { value: 3 }))", + "read_maybe_specified(MyTracked(Id { value: 3 }))", + "maybe_specified(MyTracked(Id { value: 3 }))", ]"#]]); // FIXME: should execute `maybe_specified` but not `read_maybe_specified` input.set_field(&mut db, 20); - assert_eq!(final_result(&db, input), 100); + assert_eq!(final_result(&db, input), 200); db.assert_logs(expect![[r#" [ - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 1 }) } } }", - "create_tracked(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: DidValidateMemoizedValue { database_key: DependencyIndex { ingredient_index: IngredientIndex(4), key_index: Some(Id { value: 1 }) } } }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: DidValidateMemoizedValue { database_key: DependencyIndex { ingredient_index: IngredientIndex(5), key_index: Some(Id { value: 1 }) } } }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: DidValidateMemoizedValue { database_key: DependencyIndex { ingredient_index: IngredientIndex(7), key_index: Some(Id { value: 1 }) } } }", + "create_tracked(MyInput(Id { value: 3 }))", + "maybe_specified(MyTracked(Id { value: 3 }))", + "read_maybe_specified(MyTracked(Id { value: 3 }))", + "final_result(MyInput(Id { value: 3 }))", ]"#]]); } @@ -327,34 +227,15 @@ fn test_run_5_then_20() { assert_eq!(final_result(&db, input), 100); db.assert_logs(expect![[r#" [ - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(7), key_index: Some(Id { value: 1 }) } } }", "final_result(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 1 }) } } }", "create_tracked(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(5), key_index: Some(Id { value: 1 }) } } }", "read_maybe_specified(MyTracked(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", ]"#]]); input.set_field(&mut db, 20); assert_eq!(final_result(&db, input), 100); // FIXME: Should be 20. db.assert_logs(expect![[r#" [ - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillExecute { database_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 1 }) } } }", "create_tracked(MyInput(Id { value: 1 }))", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillDiscardStaleOutput { execute_key: DependencyIndex { ingredient_index: IngredientIndex(6), key_index: Some(Id { value: 1 }) }, output_key: DependencyIndex { ingredient_index: IngredientIndex(4), key_index: Some(Id { value: 1 }) } } }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: WillCheckCancellation }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: DidValidateMemoizedValue { database_key: DependencyIndex { ingredient_index: IngredientIndex(4), key_index: Some(Id { value: 1 }) } } }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: DidValidateMemoizedValue { database_key: DependencyIndex { ingredient_index: IngredientIndex(5), key_index: Some(Id { value: 1 }) } } }", - "Event { runtime_id: RuntimeId { counter: 0 }, kind: DidValidateMemoizedValue { database_key: DependencyIndex { ingredient_index: IngredientIndex(7), key_index: Some(Id { value: 1 }) } } }", ]"#]]); // FIXME: should invoke maybe_specified } From cb0b53caa70e660ceb9438bb1b60af2d025dcafe Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 12 Aug 2022 14:28:26 -0400 Subject: [PATCH 3/7] Revert "track both inputs/outputs for each query" This reverts commit 49ccac5d3df8e0af5f9f68931497c6f4feb17147. --- components/salsa-2022-macros/src/db.rs | 2 +- components/salsa-2022/src/accumulator.rs | 4 +- components/salsa-2022/src/function.rs | 4 +- .../salsa-2022/src/function/accumulated.rs | 6 +-- components/salsa-2022/src/function/fetch.rs | 2 +- components/salsa-2022/src/function/inputs.rs | 6 +-- .../src/function/maybe_changed_after.rs | 4 +- components/salsa-2022/src/function/specify.rs | 9 ++-- components/salsa-2022/src/function/store.rs | 7 ++- components/salsa-2022/src/ingredient.rs | 4 +- components/salsa-2022/src/input.rs | 4 +- components/salsa-2022/src/interned.rs | 4 +- .../salsa-2022/src/runtime/active_query.rs | 25 +++------- .../salsa-2022/src/runtime/local_state.rs | 49 +++---------------- components/salsa-2022/src/storage.rs | 4 +- components/salsa-2022/src/tracked_struct.rs | 4 +- 16 files changed, 46 insertions(+), 92 deletions(-) diff --git a/components/salsa-2022-macros/src/db.rs b/components/salsa-2022-macros/src/db.rs index 0d1151fc..94c729ef 100644 --- a/components/salsa-2022-macros/src/db.rs +++ b/components/salsa-2022-macros/src/db.rs @@ -136,7 +136,7 @@ fn has_jars_dyn_impl(input: &syn::ItemStruct, storage: &syn::Ident) -> syn::Item fn inputs( &self, index: salsa::DatabaseKeyIndex, - ) -> Option { + ) -> Option { let ingredient = self.#storage.ingredient(index.ingredient_index()); ingredient.inputs(index.key_index()) } diff --git a/components/salsa-2022/src/accumulator.rs b/components/salsa-2022/src/accumulator.rs index a685ef67..07e12b47 100644 --- a/components/salsa-2022/src/accumulator.rs +++ b/components/salsa-2022/src/accumulator.rs @@ -3,7 +3,7 @@ use crate::{ hash::FxDashMap, ingredient::{Ingredient, MutIngredient}, key::DependencyIndex, - runtime::{local_state::QueryEdges, StampedValue}, + runtime::{local_state::QueryInputs, StampedValue}, storage::HasJar, DatabaseKeyIndex, Durability, IngredientIndex, Revision, Runtime, }; @@ -78,7 +78,7 @@ where CycleRecoveryStrategy::Panic } - fn inputs(&self, _key_index: crate::Id) -> Option { + fn inputs(&self, _key_index: crate::Id) -> Option { None } } diff --git a/components/salsa-2022/src/function.rs b/components/salsa-2022/src/function.rs index 1ac8793a..fddd3a88 100644 --- a/components/salsa-2022/src/function.rs +++ b/components/salsa-2022/src/function.rs @@ -8,7 +8,7 @@ use crate::{ ingredient::MutIngredient, jar::Jar, key::{DatabaseKeyIndex, DependencyIndex}, - runtime::local_state::QueryEdges, + runtime::local_state::QueryInputs, salsa_struct::SalsaStructInDb, Cycle, DbWithJar, Id, Revision, }; @@ -198,7 +198,7 @@ where C::CYCLE_STRATEGY } - fn inputs(&self, key_index: Id) -> Option { + fn inputs(&self, key_index: Id) -> Option { let key = C::key_from_id(key_index); self.inputs(key) } diff --git a/components/salsa-2022/src/function/accumulated.rs b/components/salsa-2022/src/function/accumulated.rs index ee80a823..e4243cab 100644 --- a/components/salsa-2022/src/function/accumulated.rs +++ b/components/salsa-2022/src/function/accumulated.rs @@ -1,7 +1,7 @@ use crate::{ hash::FxHashSet, key::DependencyIndex, - runtime::local_state::QueryEdges, + runtime::local_state::QueryInputs, storage::{HasJar, HasJarsDyn}, Database, DatabaseKeyIndex, }; @@ -55,7 +55,7 @@ impl Stack { self.v.pop() } - fn extend(&mut self, inputs: Option) { + fn extend(&mut self, inputs: Option) { let inputs = match inputs { None => return, Some(v) => v, @@ -64,7 +64,7 @@ impl Stack { for DependencyIndex { ingredient_index, key_index, - } in inputs.inputs().iter().copied() + } in inputs.tracked.iter().copied() { if let Some(key_index) = key_index { let i = DatabaseKeyIndex { diff --git a/components/salsa-2022/src/function/fetch.rs b/components/salsa-2022/src/function/fetch.rs index ed20b0cf..d8b82732 100644 --- a/components/salsa-2022/src/function/fetch.rs +++ b/components/salsa-2022/src/function/fetch.rs @@ -93,7 +93,7 @@ where if let Some(memo) = self.memo_map.get(key) { // Careful: we can't evict memos with untracked inputs // as their values cannot be reconstructed. - if memo.revisions.edges.untracked { + if memo.revisions.inputs.untracked { return; } diff --git a/components/salsa-2022/src/function/inputs.rs b/components/salsa-2022/src/function/inputs.rs index 4cfc4c80..2ad9d0b2 100644 --- a/components/salsa-2022/src/function/inputs.rs +++ b/components/salsa-2022/src/function/inputs.rs @@ -1,4 +1,4 @@ -use crate::runtime::local_state::QueryEdges; +use crate::runtime::local_state::QueryInputs; use super::{Configuration, FunctionIngredient}; @@ -6,7 +6,7 @@ impl FunctionIngredient where C: Configuration, { - pub(super) fn inputs(&self, key: C::Key) -> Option { - self.memo_map.get(key).map(|m| m.revisions.edges.clone()) + pub(super) fn inputs(&self, key: C::Key) -> Option { + self.memo_map.get(key).map(|m| m.revisions.inputs.clone()) } } diff --git a/components/salsa-2022/src/function/maybe_changed_after.rs b/components/salsa-2022/src/function/maybe_changed_after.rs index abcde4b9..b576b770 100644 --- a/components/salsa-2022/src/function/maybe_changed_after.rs +++ b/components/salsa-2022/src/function/maybe_changed_after.rs @@ -158,13 +158,13 @@ where return true; } - if old_memo.revisions.edges.untracked { + if old_memo.revisions.inputs.untracked { // Untracked inputs? Have to assume that it changed. return false; } let last_verified_at = old_memo.verified_at.load(); - for &input in old_memo.revisions.edges.inputs().iter() { + for &input in old_memo.revisions.inputs.tracked.iter() { if db.maybe_changed_after(input, last_verified_at) { return false; } diff --git a/components/salsa-2022/src/function/specify.rs b/components/salsa-2022/src/function/specify.rs index 6c879707..baa9e476 100644 --- a/components/salsa-2022/src/function/specify.rs +++ b/components/salsa-2022/src/function/specify.rs @@ -1,7 +1,7 @@ use crossbeam::atomic::AtomicCell; use crate::{ - runtime::local_state::{QueryEdges, QueryRevisions}, + runtime::local_state::{QueryInputs, QueryRevisions}, tracked_struct::TrackedStructInDb, Database, }; @@ -49,17 +49,16 @@ where // // - a result that is verified in the current revision, because it was set, which will use the set value // - a result that is NOT verified and has untracked inputs, which will re-execute (and likely panic) - let edges = QueryEdges { + let inputs = QueryInputs { untracked: false, - separator: 0, - input_outputs: runtime.empty_dependencies(), + tracked: runtime.empty_dependencies(), }; let revision = runtime.current_revision(); let mut revisions = QueryRevisions { changed_at: current_deps.changed_at, durability: current_deps.durability, - edges, + inputs, }; if let Some(old_memo) = self.memo_map.get(key) { diff --git a/components/salsa-2022/src/function/store.rs b/components/salsa-2022/src/function/store.rs index ece3f424..843afead 100644 --- a/components/salsa-2022/src/function/store.rs +++ b/components/salsa-2022/src/function/store.rs @@ -4,7 +4,7 @@ use crossbeam::atomic::AtomicCell; use crate::{ durability::Durability, - runtime::local_state::{QueryEdges, QueryRevisions}, + runtime::local_state::{QueryInputs, QueryRevisions}, Runtime, }; @@ -28,10 +28,9 @@ where revisions: QueryRevisions { changed_at: revision, durability, - edges: QueryEdges { + inputs: QueryInputs { untracked: false, - separator: 0, - input_outputs: runtime.empty_dependencies(), + tracked: runtime.empty_dependencies(), }, }, }; diff --git a/components/salsa-2022/src/ingredient.rs b/components/salsa-2022/src/ingredient.rs index d2aef7b4..0b321c26 100644 --- a/components/salsa-2022/src/ingredient.rs +++ b/components/salsa-2022/src/ingredient.rs @@ -1,5 +1,5 @@ use crate::{ - cycle::CycleRecoveryStrategy, key::DependencyIndex, runtime::local_state::QueryEdges, Id, + cycle::CycleRecoveryStrategy, key::DependencyIndex, runtime::local_state::QueryInputs, Id, }; use super::Revision; @@ -21,7 +21,7 @@ pub trait Ingredient { fn maybe_changed_after(&self, db: &DB, input: DependencyIndex, revision: Revision) -> bool; /// What were the inputs (if any) that were used to create the value at `key_index`. - fn inputs(&self, key_index: Id) -> Option; + fn inputs(&self, key_index: Id) -> Option; } /// Optional trait for ingredients that wish to be notified when new revisions are diff --git a/components/salsa-2022/src/input.rs b/components/salsa-2022/src/input.rs index 3fb7afc8..b61dd3ba 100644 --- a/components/salsa-2022/src/input.rs +++ b/components/salsa-2022/src/input.rs @@ -2,7 +2,7 @@ use crate::{ cycle::CycleRecoveryStrategy, ingredient::Ingredient, key::{DatabaseKeyIndex, DependencyIndex}, - runtime::{local_state::QueryEdges, Runtime}, + runtime::{local_state::QueryInputs, Runtime}, AsId, IngredientIndex, Revision, }; @@ -58,7 +58,7 @@ where CycleRecoveryStrategy::Panic } - fn inputs(&self, _key_index: crate::Id) -> Option { + fn inputs(&self, _key_index: crate::Id) -> Option { None } } diff --git a/components/salsa-2022/src/interned.rs b/components/salsa-2022/src/interned.rs index cf6c88fd..26fc14c7 100644 --- a/components/salsa-2022/src/interned.rs +++ b/components/salsa-2022/src/interned.rs @@ -6,7 +6,7 @@ use std::marker::PhantomData; use crate::durability::Durability; use crate::id::AsId; use crate::key::DependencyIndex; -use crate::runtime::local_state::QueryEdges; +use crate::runtime::local_state::QueryInputs; use crate::runtime::Runtime; use super::hash::FxDashMap; @@ -194,7 +194,7 @@ where crate::cycle::CycleRecoveryStrategy::Panic } - fn inputs(&self, _key_index: crate::Id) -> Option { + fn inputs(&self, _key_index: crate::Id) -> Option { None } } diff --git a/components/salsa-2022/src/runtime/active_query.rs b/components/salsa-2022/src/runtime/active_query.rs index 7652ed27..04337858 100644 --- a/components/salsa-2022/src/runtime/active_query.rs +++ b/components/salsa-2022/src/runtime/active_query.rs @@ -8,7 +8,7 @@ use crate::{ Cycle, Revision, Runtime, }; -use super::local_state::{QueryEdges, QueryRevisions}; +use super::local_state::{QueryInputs, QueryRevisions}; #[derive(Debug)] pub(super) struct ActiveQuery { @@ -95,27 +95,18 @@ impl ActiveQuery { } pub(crate) fn revisions(&self, runtime: &Runtime) -> QueryRevisions { - let separator = u32::try_from(self.dependencies.len()).unwrap(); - - let input_outputs = if self.dependencies.is_empty() && self.outputs.is_empty() { - runtime.empty_dependencies() - } else { - self.dependencies - .iter() - .copied() - .chain(self.outputs.iter().map(|&o| o.into())) - .collect() - }; - - let edges = QueryEdges { + let inputs = QueryInputs { untracked: self.untracked_read, - separator, - input_outputs, + tracked: if self.dependencies.is_empty() { + runtime.empty_dependencies() + } else { + self.dependencies.iter().copied().collect() + }, }; QueryRevisions { changed_at: self.changed_at, - edges, + inputs, durability: self.durability, } } diff --git a/components/salsa-2022/src/runtime/local_state.rs b/components/salsa-2022/src/runtime/local_state.rs index 4a56e901..6c5b7730 100644 --- a/components/salsa-2022/src/runtime/local_state.rs +++ b/components/salsa-2022/src/runtime/local_state.rs @@ -40,7 +40,7 @@ pub(crate) struct QueryRevisions { pub(crate) durability: Durability, /// The inputs that went into our query, if we are tracking them. - pub(crate) edges: QueryEdges, + pub(crate) inputs: QueryInputs, } impl QueryRevisions { @@ -53,53 +53,18 @@ impl QueryRevisions { } } -/// The edges between a memoized value and other queries in the dependency graph. -/// These edges include both dependency edges -/// e.g., when creating the memoized value for Q0 executed another function Q1) -/// and output edges -/// (e.g., when Q0 specified the value for another query Q2). +/// Every input. #[derive(Debug, Clone)] -pub struct QueryEdges { - /// The list of outgoing edges from this node. - /// This list combines *both* inputs and outputs. - /// The inputs are defined from the indices `0..S` where - /// `S` is the value of the `separator` field. - /// - /// Note that we always track input dependencies even when there are untracked reads. - /// Untracked reads mean that we can't verify values, so we don't use the list of inputs for that, - /// but we still use it for finding the transitive inputs to an accumulator. - /// - /// You can access the input/output list via the methods [`inputs`] and [`outputs`] respectively. - /// - /// Important: - /// - /// * The inputs must be in **execution order** for the red-green algorithm to work. - /// * The outputs must be in **sorted order** so that we can easily "diff" them between revisions. - pub(crate) input_outputs: Arc<[DependencyIndex]>, - - /// The index that separates inputs from outputs in the `tracked` field. - pub(crate) separator: u32, +pub struct QueryInputs { + /// Inputs that are fully known. + /// We track these even if there are unknown inputs so that the accumulator code + /// can walk all the inputs even for tracked functions that read untracked values. + pub(crate) tracked: Arc<[DependencyIndex]>, /// Where there any *unknown* inputs? pub(crate) untracked: bool, } -impl QueryEdges { - /// Returns the (tracked) inputs that were executed in computing this memoized value. - /// - /// These will always be in execution order. - pub(crate) fn inputs(&self) -> &[DependencyIndex] { - &self.input_outputs[0..self.separator as usize] - } - - /// Returns the queries whose values were assigned while computing this memoized value. - /// - /// These will always be in sorted order. - pub(crate) fn outputs(&self) -> &[DependencyIndex] { - &self.input_outputs[self.separator as usize..] - } -} - impl Default for LocalState { fn default() -> Self { LocalState { diff --git a/components/salsa-2022/src/storage.rs b/components/salsa-2022/src/storage.rs index 5384a76a..29926d37 100644 --- a/components/salsa-2022/src/storage.rs +++ b/components/salsa-2022/src/storage.rs @@ -6,7 +6,7 @@ use crate::cycle::CycleRecoveryStrategy; use crate::ingredient::Ingredient; use crate::jar::Jar; use crate::key::DependencyIndex; -use crate::runtime::local_state::QueryEdges; +use crate::runtime::local_state::QueryInputs; use crate::runtime::Runtime; use crate::{Database, DatabaseKeyIndex, IngredientIndex}; @@ -178,7 +178,7 @@ pub trait HasJarsDyn { fn cycle_recovery_strategy(&self, input: IngredientIndex) -> CycleRecoveryStrategy; - fn inputs(&self, input: DatabaseKeyIndex) -> Option; + fn inputs(&self, input: DatabaseKeyIndex) -> Option; } pub trait HasIngredientsFor diff --git a/components/salsa-2022/src/tracked_struct.rs b/components/salsa-2022/src/tracked_struct.rs index c57819cf..0b326775 100644 --- a/components/salsa-2022/src/tracked_struct.rs +++ b/components/salsa-2022/src/tracked_struct.rs @@ -3,7 +3,7 @@ use crate::{ ingredient::{Ingredient, MutIngredient}, interned::{InternedData, InternedId, InternedIngredient}, key::{DatabaseKeyIndex, DependencyIndex}, - runtime::{local_state::QueryEdges, Runtime}, + runtime::{local_state::QueryInputs, Runtime}, salsa_struct::SalsaStructInDb, Database, IngredientIndex, Revision, }; @@ -114,7 +114,7 @@ where <_ as Ingredient>::cycle_recovery_strategy(&self.interned) } - fn inputs(&self, _key_index: crate::Id) -> Option { + fn inputs(&self, _key_index: crate::Id) -> Option { None } } From 5046ba1e6dad9763fabfc9cb4ea69d8d0042905b Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 12 Aug 2022 14:28:27 -0400 Subject: [PATCH 4/7] Revert "record when specify is called by a user" This reverts commit 787480ffab9d5c531c8f02635d10690b9b9c395b. --- components/salsa-2022-macros/src/tracked_fn.rs | 2 +- components/salsa-2022/src/function/specify.rs | 14 -------------- 2 files changed, 1 insertion(+), 15 deletions(-) diff --git a/components/salsa-2022-macros/src/tracked_fn.rs b/components/salsa-2022-macros/src/tracked_fn.rs index fb031cb2..feb362d0 100644 --- a/components/salsa-2022-macros/src/tracked_fn.rs +++ b/components/salsa-2022-macros/src/tracked_fn.rs @@ -415,7 +415,7 @@ fn specify_fn( let (__jar, __runtime) = <_ as salsa::storage::HasJar<#jar_ty>>::jar(#db_var); let __ingredients = <_ as salsa::storage::HasIngredientsFor<#config_ty>>::ingredient(__jar); - __ingredients.function.specify_and_record(#db_var, #(#arg_names,)* #value_arg) + __ingredients.function.specify(#db_var, #(#arg_names,)* #value_arg) } }, })) diff --git a/components/salsa-2022/src/function/specify.rs b/components/salsa-2022/src/function/specify.rs index baa9e476..047e2849 100644 --- a/components/salsa-2022/src/function/specify.rs +++ b/components/salsa-2022/src/function/specify.rs @@ -71,20 +71,6 @@ where revisions, }; - log::debug!("specify: about to add memo {:#?} for key {:?}", memo, key); self.insert_memo(key, memo); } - - /// Specify the value for `key` *and* record that we did so. - /// Used for explicit calls to `specify`, but not needed for pre-declared tracked struct fields. - pub fn specify_and_record<'db>(&self, db: &'db DynDb<'db, C>, key: C::Key, value: C::Value) - where - C::Key: TrackedStructInDb>, - { - self.specify(db, key, value); - - // Record that the current query *specified* a value for this cell. - let database_key_index = self.database_key_index(key); - db.salsa_runtime().add_output(database_key_index); - } } From 8fc38fed48e1e292f3e85b3e45aa8cbb23c2ccb7 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 12 Aug 2022 14:28:28 -0400 Subject: [PATCH 5/7] Revert "generalize list of "entities created" to "outputs"" This reverts commit cbe7d371c9b6c9581a4a16c842e1f108f67871b8. --- components/salsa-2022/src/function/specify.rs | 2 +- components/salsa-2022/src/runtime.rs | 14 +++++++------- components/salsa-2022/src/runtime/active_query.rs | 3 ++- components/salsa-2022/src/runtime/local_state.rs | 4 ++-- components/salsa-2022/src/tracked_struct.rs | 2 +- 5 files changed, 13 insertions(+), 12 deletions(-) diff --git a/components/salsa-2022/src/function/specify.rs b/components/salsa-2022/src/function/specify.rs index 047e2849..9195805e 100644 --- a/components/salsa-2022/src/function/specify.rs +++ b/components/salsa-2022/src/function/specify.rs @@ -27,7 +27,7 @@ where }; let entity_index = key.database_key_index(db); - if !runtime.is_output_of_active_query(entity_index) { + if !runtime.was_entity_created(entity_index) { panic!("can only use `set` on entities created during current query"); } diff --git a/components/salsa-2022/src/runtime.rs b/components/salsa-2022/src/runtime.rs index e83a54b8..3c1500a3 100644 --- a/components/salsa-2022/src/runtime.rs +++ b/components/salsa-2022/src/runtime.rs @@ -144,15 +144,15 @@ impl Runtime { } } - /// Adds `key` to the list of output created by the current query - /// (if not already present). - pub(crate) fn add_output(&self, key: DatabaseKeyIndex) { - self.local_state.add_output(key); + /// Adds `entity` to the lits of entities created by the current query. + /// Panics if `entity` was already added. + pub(crate) fn add_entity_created(&self, entity: DatabaseKeyIndex) { + self.local_state.add_entity_created(entity); } - /// Check whether `entity` is contained the list of outputs written by the current query. - pub(super) fn is_output_of_active_query(&self, entity: DatabaseKeyIndex) -> bool { - self.local_state.is_output(entity) + /// Check whether `entity` is contained the list of entities created by the current query. + pub(super) fn was_entity_created(&self, entity: DatabaseKeyIndex) -> bool { + self.local_state.was_entity_created(entity) } /// Called when the active queries creates an index from the diff --git a/components/salsa-2022/src/runtime/active_query.rs b/components/salsa-2022/src/runtime/active_query.rs index 04337858..4ef9c889 100644 --- a/components/salsa-2022/src/runtime/active_query.rs +++ b/components/salsa-2022/src/runtime/active_query.rs @@ -86,7 +86,8 @@ impl ActiveQuery { /// Adds a key to our list of outputs. pub(super) fn add_output(&mut self, key: DatabaseKeyIndex) { - self.outputs.insert(key); + let is_new = self.outputs.insert(key); + assert!(is_new); } /// True if the given key was output by this query. diff --git a/components/salsa-2022/src/runtime/local_state.rs b/components/salsa-2022/src/runtime/local_state.rs index 6c5b7730..ab47a2b7 100644 --- a/components/salsa-2022/src/runtime/local_state.rs +++ b/components/salsa-2022/src/runtime/local_state.rs @@ -115,7 +115,7 @@ impl LocalState { }) } - pub(super) fn add_output(&self, entity: DatabaseKeyIndex) { + pub(super) fn add_entity_created(&self, entity: DatabaseKeyIndex) { self.with_query_stack(|stack| { if let Some(top_query) = stack.last_mut() { top_query.add_output(entity) @@ -123,7 +123,7 @@ impl LocalState { }) } - pub(super) fn is_output(&self, entity: DatabaseKeyIndex) -> bool { + pub(super) fn was_entity_created(&self, entity: DatabaseKeyIndex) -> bool { self.with_query_stack(|stack| { if let Some(top_query) = stack.last_mut() { top_query.is_output(entity) diff --git a/components/salsa-2022/src/tracked_struct.rs b/components/salsa-2022/src/tracked_struct.rs index 0b326775..39d19548 100644 --- a/components/salsa-2022/src/tracked_struct.rs +++ b/components/salsa-2022/src/tracked_struct.rs @@ -76,7 +76,7 @@ where data, }; let result = self.interned.intern(runtime, entity_key); - runtime.add_output(self.database_key_index(result)); + runtime.add_entity_created(self.database_key_index(result)); result } From 15ec60613ca9fecdf9ae0da3c4eeaf0fe22d637b Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 12 Aug 2022 14:28:30 -0400 Subject: [PATCH 6/7] Revert "track outputs for the active record" This reverts commit de2fb22a1cd79711050394a9361905b1ae3b9661. --- .../salsa-2022/src/runtime/active_query.rs | 25 ++++++------------- .../salsa-2022/src/runtime/local_state.rs | 4 +-- 2 files changed, 9 insertions(+), 20 deletions(-) diff --git a/components/salsa-2022/src/runtime/active_query.rs b/components/salsa-2022/src/runtime/active_query.rs index 4ef9c889..02fa5c6f 100644 --- a/components/salsa-2022/src/runtime/active_query.rs +++ b/components/salsa-2022/src/runtime/active_query.rs @@ -1,5 +1,3 @@ -use std::collections::BTreeSet; - use crate::{ durability::Durability, hash::{FxHashSet, FxIndexMap, FxIndexSet}, @@ -36,15 +34,8 @@ pub(super) struct ActiveQuery { /// Otherwise it is 1 more than the current value (which is incremented). pub(super) disambiguator_map: FxIndexMap, - /// Tracks values written by this query. Could be... - /// - /// * tracked structs created - /// * invocations of `specify` - /// * accumulators pushed to - /// - /// We use a btree-set because we want to be able to - /// extract the keys in sorted order. - pub(super) outputs: BTreeSet, + /// Tracks entities created by this query. + pub(super) entities_created: FxHashSet, } impl ActiveQuery { @@ -57,7 +48,7 @@ impl ActiveQuery { untracked_read: false, cycle: None, disambiguator_map: Default::default(), - outputs: Default::default(), + entities_created: Default::default(), } } @@ -84,15 +75,13 @@ impl ActiveQuery { self.changed_at = self.changed_at.max(revision); } - /// Adds a key to our list of outputs. - pub(super) fn add_output(&mut self, key: DatabaseKeyIndex) { - let is_new = self.outputs.insert(key); + pub(super) fn add_entity_created(&mut self, entity: DatabaseKeyIndex) { + let is_new = self.entities_created.insert(entity); assert!(is_new); } - /// True if the given key was output by this query. - pub(super) fn is_output(&self, key: DatabaseKeyIndex) -> bool { - self.outputs.contains(&key) + pub(super) fn was_entity_created(&self, entity: DatabaseKeyIndex) -> bool { + self.entities_created.contains(&entity) } pub(crate) fn revisions(&self, runtime: &Runtime) -> QueryRevisions { diff --git a/components/salsa-2022/src/runtime/local_state.rs b/components/salsa-2022/src/runtime/local_state.rs index ab47a2b7..785da66c 100644 --- a/components/salsa-2022/src/runtime/local_state.rs +++ b/components/salsa-2022/src/runtime/local_state.rs @@ -118,7 +118,7 @@ impl LocalState { pub(super) fn add_entity_created(&self, entity: DatabaseKeyIndex) { self.with_query_stack(|stack| { if let Some(top_query) = stack.last_mut() { - top_query.add_output(entity) + top_query.add_entity_created(entity) } }) } @@ -126,7 +126,7 @@ impl LocalState { pub(super) fn was_entity_created(&self, entity: DatabaseKeyIndex) -> bool { self.with_query_stack(|stack| { if let Some(top_query) = stack.last_mut() { - top_query.is_output(entity) + top_query.was_entity_created(entity) } else { false } From 5722b333edb636fb33578b882f7e1c309aaa0000 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 12 Aug 2022 14:28:31 -0400 Subject: [PATCH 7/7] Revert "add broken test for specify" This reverts commit 87ff990774ddc0c3386ea754251dfdf248fb1b1f. --- .../specify_tracked_fn_in_rev_1_but_not_2.rs | 241 ------------------ 1 file changed, 241 deletions(-) delete mode 100644 salsa-2022-tests/tests/specify_tracked_fn_in_rev_1_but_not_2.rs diff --git a/salsa-2022-tests/tests/specify_tracked_fn_in_rev_1_but_not_2.rs b/salsa-2022-tests/tests/specify_tracked_fn_in_rev_1_but_not_2.rs deleted file mode 100644 index 0a8e9138..00000000 --- a/salsa-2022-tests/tests/specify_tracked_fn_in_rev_1_but_not_2.rs +++ /dev/null @@ -1,241 +0,0 @@ -//! Test that a `tracked` fn on a `salsa::input` -//! compiles and executes successfully. - -use expect_test::expect; -use salsa_2022_tests::{HasLogger, Logger}; -use test_log::test; - -#[salsa::jar(db = Db)] -struct Jar( - MyInput, - MyTracked, - maybe_specified, - read_maybe_specified, - create_tracked, - final_result, -); - -trait Db: salsa::DbWithJar + HasLogger {} - -#[salsa::input] -struct MyInput { - field: u32, -} - -#[salsa::tracked] -struct MyTracked { - input: MyInput, -} - -/// If the input is in the range 0..10, this is specified to return 10. -/// Otherwise, the default occurs, and it returns the input. -#[salsa::tracked(specify)] -fn maybe_specified(db: &dyn Db, tracked: MyTracked) -> u32 { - db.push_log(format!("maybe_specified({:?})", tracked)); - tracked.input(db).field(db) -} - -/// Reads maybe-specified and multiplies it by 10. -/// This is here to show whether we can detect when `maybe_specified` has changed -/// and control down-stream work accordingly. -#[salsa::tracked] -fn read_maybe_specified(db: &dyn Db, tracked: MyTracked) -> u32 { - db.push_log(format!("read_maybe_specified({:?})", tracked)); - maybe_specified(db, tracked) * 10 -} - -/// Create a tracked value and *maybe* specify a value for -/// `maybe_specified` -#[salsa::tracked(jar = Jar)] -fn create_tracked(db: &dyn Db, input: MyInput) -> MyTracked { - db.push_log(format!("create_tracked({:?})", input)); - let tracked = MyTracked::new(db, input); - if input.field(db) < 10 { - maybe_specified::specify(db, tracked, 10); - } - tracked -} - -#[salsa::tracked] -fn final_result(db: &dyn Db, input: MyInput) -> u32 { - db.push_log(format!("final_result({:?})", input)); - let tracked = create_tracked(db, input); - read_maybe_specified(db, tracked) -} - -#[salsa::db(Jar)] -#[derive(Default)] -struct Database { - storage: salsa::Storage, - logger: Logger, -} - -impl salsa::Database for Database { - fn salsa_runtime(&self) -> &salsa::Runtime { - self.storage.runtime() - } -} - -impl Db for Database {} - -impl HasLogger for Database { - fn logger(&self) -> &Logger { - &self.logger - } -} - -#[test] -fn test_run_0() { - let mut db = Database::default(); - - let input = MyInput::new(&mut db, 0); - assert_eq!(final_result(&db, input), 100); - db.assert_logs(expect![[r#" - [ - "final_result(MyInput(Id { value: 1 }))", - "create_tracked(MyInput(Id { value: 1 }))", - "read_maybe_specified(MyTracked(Id { value: 1 }))", - ]"#]]); -} - -#[test] -fn test_run_5() { - let mut db = Database::default(); - - let input = MyInput::new(&mut db, 5); - assert_eq!(final_result(&db, input), 100); - db.assert_logs(expect![[r#" - [ - "final_result(MyInput(Id { value: 1 }))", - "create_tracked(MyInput(Id { value: 1 }))", - "read_maybe_specified(MyTracked(Id { value: 1 }))", - ]"#]]); -} - -#[test] -fn test_run_10() { - let mut db = Database::default(); - - let input = MyInput::new(&mut db, 10); - assert_eq!(final_result(&db, input), 100); - db.assert_logs(expect![[r#" - [ - "final_result(MyInput(Id { value: 1 }))", - "create_tracked(MyInput(Id { value: 1 }))", - "read_maybe_specified(MyTracked(Id { value: 1 }))", - "maybe_specified(MyTracked(Id { value: 1 }))", - ]"#]]); -} - -#[test] -fn test_run_20() { - let mut db = Database::default(); - - let input = MyInput::new(&mut db, 20); - assert_eq!(final_result(&db, input), 200); - db.assert_logs(expect![[r#" - [ - "final_result(MyInput(Id { value: 1 }))", - "create_tracked(MyInput(Id { value: 1 }))", - "read_maybe_specified(MyTracked(Id { value: 1 }))", - "maybe_specified(MyTracked(Id { value: 1 }))", - ]"#]]); -} - -#[test] -fn test_run_0_then_5_then_20() { - let mut db = Database::default(); - - let input = MyInput::new(&mut db, 0); - assert_eq!(final_result(&db, input), 100); - db.assert_logs(expect![[r#" - [ - "final_result(MyInput(Id { value: 1 }))", - "create_tracked(MyInput(Id { value: 1 }))", - "read_maybe_specified(MyTracked(Id { value: 1 }))", - ]"#]]); - - // FIXME: read_maybe_specified should not re-execute - let input = MyInput::new(&mut db, 5); - assert_eq!(final_result(&db, input), 100); - db.assert_logs(expect![[r#" - [ - "final_result(MyInput(Id { value: 2 }))", - "create_tracked(MyInput(Id { value: 2 }))", - "read_maybe_specified(MyTracked(Id { value: 2 }))", - ]"#]]); - - input.set_field(&mut db, 20); - assert_eq!(final_result(&db, input), 100); // FIXME: Should be 20. - db.assert_logs(expect![[r#" - [ - "create_tracked(MyInput(Id { value: 2 }))", - ]"#]]); // FIXME: should invoke maybe_specified -} - -#[test] -fn test_run_0_then_5_then_10_then_20() { - let mut db = Database::default(); - - let input = MyInput::new(&mut db, 0); - assert_eq!(final_result(&db, input), 100); - db.assert_logs(expect![[r#" - [ - "final_result(MyInput(Id { value: 1 }))", - "create_tracked(MyInput(Id { value: 1 }))", - "read_maybe_specified(MyTracked(Id { value: 1 }))", - ]"#]]); - - // FIXME: `read_maybe_specified` should not re-execute - let input = MyInput::new(&mut db, 5); - assert_eq!(final_result(&db, input), 100); - db.assert_logs(expect![[r#" - [ - "final_result(MyInput(Id { value: 2 }))", - "create_tracked(MyInput(Id { value: 2 }))", - "read_maybe_specified(MyTracked(Id { value: 2 }))", - ]"#]]); - - // FIXME: should execute `maybe_specified` but not `read_maybe_specified` - let input = MyInput::new(&mut db, 10); - assert_eq!(final_result(&db, input), 100); - db.assert_logs(expect![[r#" - [ - "final_result(MyInput(Id { value: 3 }))", - "create_tracked(MyInput(Id { value: 3 }))", - "read_maybe_specified(MyTracked(Id { value: 3 }))", - "maybe_specified(MyTracked(Id { value: 3 }))", - ]"#]]); - - // FIXME: should execute `maybe_specified` but not `read_maybe_specified` - input.set_field(&mut db, 20); - assert_eq!(final_result(&db, input), 200); - db.assert_logs(expect![[r#" - [ - "create_tracked(MyInput(Id { value: 3 }))", - "maybe_specified(MyTracked(Id { value: 3 }))", - "read_maybe_specified(MyTracked(Id { value: 3 }))", - "final_result(MyInput(Id { value: 3 }))", - ]"#]]); -} - -#[test] -fn test_run_5_then_20() { - let mut db = Database::default(); - - let input = MyInput::new(&mut db, 5); - assert_eq!(final_result(&db, input), 100); - db.assert_logs(expect![[r#" - [ - "final_result(MyInput(Id { value: 1 }))", - "create_tracked(MyInput(Id { value: 1 }))", - "read_maybe_specified(MyTracked(Id { value: 1 }))", - ]"#]]); - - input.set_field(&mut db, 20); - assert_eq!(final_result(&db, input), 100); // FIXME: Should be 20. - db.assert_logs(expect![[r#" - [ - "create_tracked(MyInput(Id { value: 1 }))", - ]"#]]); // FIXME: should invoke maybe_specified -}