diff --git a/src/accumulator.rs b/src/accumulator.rs index 1e0c88d79..35808d6d2 100644 --- a/src/accumulator.rs +++ b/src/accumulator.rs @@ -7,6 +7,7 @@ use std::panic::UnwindSafe; use accumulated::{Accumulated, AnyAccumulated}; +use crate::cycle::CycleHeads; use crate::function::VerifyResult; use crate::ingredient::{Ingredient, Jar}; use crate::loom::sync::Arc; @@ -105,7 +106,7 @@ impl Ingredient for IngredientImpl { _db: &dyn Database, _input: Id, _revision: Revision, - _in_cycle: bool, + _cycle_heads: &mut CycleHeads, ) -> VerifyResult { panic!("nothing should ever depend on an accumulator directly") } diff --git a/src/cycle.rs b/src/cycle.rs index 43250b53a..c0e2bd2ea 100644 --- a/src/cycle.rs +++ b/src/cycle.rs @@ -151,6 +151,22 @@ impl CycleHeads { } } + #[inline] + pub(crate) fn push_initial(&mut self, database_key_index: DatabaseKeyIndex) { + if let Some(existing) = self + .0 + .iter() + .find(|candidate| candidate.database_key_index == database_key_index) + { + assert_eq!(existing.iteration_count, 0); + } else { + self.0.push(CycleHead { + database_key_index, + iteration_count: 0, + }); + } + } + #[inline] pub(crate) fn extend(&mut self, other: &Self) { self.0.reserve(other.0.len()); diff --git a/src/function.rs b/src/function.rs index 0fe78d167..f7e659915 100644 --- a/src/function.rs +++ b/src/function.rs @@ -5,7 +5,7 @@ use std::ptr::NonNull; pub(crate) use maybe_changed_after::VerifyResult; use crate::accumulator::accumulated_map::{AccumulatedMap, InputAccumulatedValues}; -use crate::cycle::{CycleHeadKind, CycleRecoveryAction, CycleRecoveryStrategy}; +use crate::cycle::{CycleHeadKind, CycleHeads, CycleRecoveryAction, CycleRecoveryStrategy}; use crate::function::delete::DeletedEntries; use crate::function::sync::{ClaimResult, SyncTable}; use crate::ingredient::Ingredient; @@ -233,11 +233,11 @@ where db: &dyn Database, input: Id, revision: Revision, - in_cycle: bool, + cycle_heads: &mut CycleHeads, ) -> VerifyResult { // SAFETY: The `db` belongs to the ingredient as per caller invariant let db = unsafe { self.view_caster.downcast_unchecked(db) }; - self.maybe_changed_after(db, input, revision, in_cycle) + self.maybe_changed_after(db, input, revision, cycle_heads) } /// True if the input `input` contains a memo that cites itself as a cycle head. diff --git a/src/function/fetch.rs b/src/function/fetch.rs index 3519e6abb..d77ba401c 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -176,9 +176,14 @@ where let opt_old_memo = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index); if let Some(old_memo) = opt_old_memo { if old_memo.value.is_some() { - if let VerifyResult::Unchanged(_, cycle_heads) = - self.deep_verify_memo(db, zalsa, old_memo, self.database_key_index(id)) - { + let mut cycle_heads = CycleHeads::default(); + if let VerifyResult::Unchanged(_) = self.deep_verify_memo( + db, + zalsa, + old_memo, + self.database_key_index(id), + &mut cycle_heads, + ) { if cycle_heads.is_empty() { // SAFETY: memo is present in memo_map and we have verified that it is // still valid for the current revision. diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index de222adc5..47921f033 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -13,53 +13,33 @@ use crate::{AsDynDatabase as _, Id, Revision}; /// Result of memo validation. pub enum VerifyResult { /// Memo has changed and needs to be recomputed. - /// - /// The cycle heads encountered when validating the memo. - Changed(CycleHeads), + Changed, /// Memo remains valid. /// - /// The first inner value tracks whether the memo or any of its dependencies have an + /// The inner value tracks whether the memo or any of its dependencies have an /// accumulated value. /// - /// The second is the cycle heads encountered in validation; don't mark - /// memos verified until we've iterated the full cycle to ensure no inputs changed. - Unchanged(InputAccumulatedValues, CycleHeads), + /// Don't mark memos verified until we've iterated the full cycle to ensure no inputs changed + /// when encountering this variant. + Unchanged(InputAccumulatedValues), } impl VerifyResult { pub(crate) fn changed_if(changed: bool) -> Self { if changed { - Self::changed() + Self::Changed } else { Self::unchanged() } } - pub(crate) fn changed() -> Self { - Self::Changed(CycleHeads::default()) - } - pub(crate) fn unchanged() -> Self { - Self::Unchanged(InputAccumulatedValues::Empty, CycleHeads::default()) - } - - pub(crate) fn cycle_heads(&self) -> &CycleHeads { - match self { - Self::Changed(cycle_heads) => cycle_heads, - Self::Unchanged(_, cycle_heads) => cycle_heads, - } - } - - pub(crate) fn into_cycle_heads(self) -> CycleHeads { - match self { - Self::Changed(cycle_heads) => cycle_heads, - Self::Unchanged(_, cycle_heads) => cycle_heads, - } + Self::Unchanged(InputAccumulatedValues::Empty) } pub(crate) const fn is_unchanged(&self) -> bool { - matches!(self, Self::Unchanged(_, _)) + matches!(self, Self::Unchanged(_)) } } @@ -72,7 +52,7 @@ where db: &'db C::DbView, id: Id, revision: Revision, - in_cycle: bool, + cycle_heads: &mut CycleHeads, ) -> VerifyResult { let (zalsa, zalsa_local) = db.zalsas(); let memo_ingredient_index = self.memo_ingredient_index(zalsa, id); @@ -87,7 +67,7 @@ where let memo_guard = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index); let Some(memo) = memo_guard else { // No memo? Assume has changed. - return VerifyResult::changed(); + return VerifyResult::Changed; }; let can_shallow_update = self.shallow_verify_memo(zalsa, database_key_index, memo); @@ -95,12 +75,9 @@ where self.update_shallow(zalsa, database_key_index, memo, can_shallow_update); return if memo.revisions.changed_at > revision { - VerifyResult::changed() + VerifyResult::Changed } else { - VerifyResult::Unchanged( - memo.revisions.accumulated_inputs.load(), - CycleHeads::default(), - ) + VerifyResult::Unchanged(memo.revisions.accumulated_inputs.load()) }; } @@ -110,7 +87,7 @@ where id, revision, memo_ingredient_index, - in_cycle, + cycle_heads, ) { return mcs; } else { @@ -127,7 +104,7 @@ where key_index: Id, revision: Revision, memo_ingredient_index: MemoIngredientIndex, - in_cycle: bool, + cycle_heads: &mut CycleHeads, ) -> Option { let database_key_index = self.database_key_index(key_index); @@ -148,10 +125,8 @@ where tracing::debug!( "hit cycle at {database_key_index:?} in `maybe_changed_after`, returning fixpoint initial value", ); - return Some(VerifyResult::Unchanged( - InputAccumulatedValues::Empty, - CycleHeads::initial(database_key_index), - )); + cycle_heads.push_initial(database_key_index); + return Some(VerifyResult::unchanged()); } }, ClaimResult::Claimed(guard) => guard, @@ -159,7 +134,7 @@ where // Load the current memo, if any. let Some(old_memo) = self.get_memo_from_table_for(zalsa, key_index, memo_ingredient_index) else { - return Some(VerifyResult::changed()); + return Some(VerifyResult::Changed); }; tracing::debug!( @@ -169,15 +144,13 @@ where ); // Check if the inputs are still valid. We can just compare `changed_at`. - let deep_verify = self.deep_verify_memo(db, zalsa, old_memo, database_key_index); + let deep_verify = + self.deep_verify_memo(db, zalsa, old_memo, database_key_index, cycle_heads); if deep_verify.is_unchanged() { return Some(if old_memo.revisions.changed_at > revision { - VerifyResult::Changed(deep_verify.into_cycle_heads()) + VerifyResult::Changed } else { - VerifyResult::Unchanged( - old_memo.revisions.accumulated_inputs.load(), - deep_verify.into_cycle_heads(), - ) + VerifyResult::Unchanged(old_memo.revisions.accumulated_inputs.load()) }); } @@ -191,26 +164,23 @@ where // the cycle head returned *fixpoint initial* without validating its dependencies. // `in_cycle` tracks if the enclosing query is in a cycle. `deep_verify.cycle_heads` tracks // if **this query** encountered a cycle (which means there's some provisional value somewhere floating around). - if old_memo.value.is_some() && !in_cycle && deep_verify.cycle_heads().is_empty() { + if old_memo.value.is_some() && cycle_heads.is_empty() { let active_query = db.zalsa_local().push_query(database_key_index, 0); let memo = self.execute(db, active_query, Some(old_memo)); let changed_at = memo.revisions.changed_at; return Some(if changed_at > revision { - VerifyResult::changed() + VerifyResult::Changed } else { - VerifyResult::Unchanged( - match &memo.revisions.accumulated { - Some(_) => InputAccumulatedValues::Any, - None => memo.revisions.accumulated_inputs.load(), - }, - CycleHeads::default(), - ) + VerifyResult::Unchanged(match &memo.revisions.accumulated { + Some(_) => InputAccumulatedValues::Any, + None => memo.revisions.accumulated_inputs.load(), + }) }); } // Otherwise, nothing for it: have to consider the value to have changed. - Some(VerifyResult::Changed(deep_verify.into_cycle_heads())) + Some(VerifyResult::Changed) } /// `Some` if the memo's value and `changed_at` time is still valid in this revision. @@ -249,7 +219,7 @@ where ); if last_changed <= verified_at { // No input of the suitable durability has changed since last verified. - ShallowUpdate::HigherDurability(revision_now) + ShallowUpdate::HigherDurability } else { ShallowUpdate::No } @@ -263,8 +233,8 @@ where memo: &Memo>, update: ShallowUpdate, ) { - if let ShallowUpdate::HigherDurability(revision_now) = update { - memo.mark_as_verified(zalsa, revision_now, database_key_index); + if let ShallowUpdate::HigherDurability = update { + memo.mark_as_verified(zalsa, database_key_index); memo.mark_outputs_as_verified(zalsa, database_key_index); } } @@ -375,6 +345,7 @@ where zalsa: &Zalsa, old_memo: &Memo>, database_key_index: DatabaseKeyIndex, + cycle_heads: &mut CycleHeads, ) -> VerifyResult { tracing::debug!( "{database_key_index:?}: deep_verify_memo(old_memo = {old_memo:#?})", @@ -408,30 +379,29 @@ where // Conditionally specified queries // where the value is specified // in rev 1 but not in rev 2. - VerifyResult::changed() + VerifyResult::Changed } // Return `Unchanged` similar to the initial value that we insert // when we hit the cycle. Any dependencies accessed when creating the fixpoint initial // are tracked by the outer query. Nothing should have changed assuming that the // fixpoint initial function is deterministic. - QueryOrigin::FixpointInitial => VerifyResult::Unchanged( - InputAccumulatedValues::Empty, - CycleHeads::initial(database_key_index), - ), + QueryOrigin::FixpointInitial => { + cycle_heads.push_initial(database_key_index); + VerifyResult::unchanged() + } QueryOrigin::DerivedUntracked(_) => { // Untracked inputs? Have to assume that it changed. - VerifyResult::changed() + VerifyResult::Changed } QueryOrigin::Derived(edges) => { let is_provisional = old_memo.may_be_provisional(); // If the value is from the same revision but is still provisional, consider it changed // because we're now in a new iteration. - if can_shallow_update.yes() && is_provisional { - return VerifyResult::changed(); + if can_shallow_update == ShallowUpdate::Verified && is_provisional { + return VerifyResult::Changed; } - let mut cycle_heads = CycleHeads::default(); 'cycle: loop { // Fully tracked inputs? Iterate over the inputs and check them, one by one. // @@ -449,16 +419,12 @@ where dyn_db, zalsa, last_verified_at, - !cycle_heads.is_empty(), + cycle_heads, ) { - VerifyResult::Changed(heads) => { - // Carry over the heads from the inner query to avoid - // backdating the outer query. - cycle_heads.extend(&heads); - break 'cycle VerifyResult::Changed(cycle_heads); + VerifyResult::Changed => { + break 'cycle VerifyResult::Changed; } - VerifyResult::Unchanged(input_accumulated, cycles) => { - cycle_heads.extend(&cycles); + VerifyResult::Unchanged(input_accumulated) => { inputs |= input_accumulated; } } @@ -514,11 +480,7 @@ where let in_heads = cycle_heads.remove(&database_key_index); if cycle_heads.is_empty() { - old_memo.mark_as_verified( - zalsa, - zalsa.current_revision(), - database_key_index, - ); + old_memo.mark_as_verified(zalsa, database_key_index); old_memo.revisions.accumulated_inputs.store(inputs); if is_provisional { @@ -532,7 +494,7 @@ where continue 'cycle; } } - break 'cycle VerifyResult::Unchanged(inputs, cycle_heads); + break 'cycle VerifyResult::Unchanged(inputs); } } } @@ -546,7 +508,7 @@ pub(super) enum ShallowUpdate { /// The revision for the memo's durability hasn't changed. It can be marked as verified /// in this revision. - HigherDurability(Revision), + HigherDurability, /// The memo requires a deep verification. No, @@ -556,7 +518,7 @@ impl ShallowUpdate { pub(super) fn yes(&self) -> bool { matches!( self, - ShallowUpdate::Verified | ShallowUpdate::HigherDurability(_) + ShallowUpdate::Verified | ShallowUpdate::HigherDurability ) } } diff --git a/src/function/memo.rs b/src/function/memo.rs index bde07d9d9..3dbe1ffde 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -219,19 +219,14 @@ impl Memo { /// The caller is responsible to update the memo's `accumulated` state if their accumulated /// values have changed since. #[inline] - pub(super) fn mark_as_verified( - &self, - zalsa: &Zalsa, - revision_now: Revision, - database_key_index: DatabaseKeyIndex, - ) { + pub(super) fn mark_as_verified(&self, zalsa: &Zalsa, database_key_index: DatabaseKeyIndex) { zalsa.event(&|| { Event::new(EventKind::DidValidateMemoizedValue { database_key: database_key_index, }) }); - self.verified_at.store(revision_now); + self.verified_at.store(zalsa.current_revision()); } pub(super) fn mark_outputs_as_verified( diff --git a/src/function/specify.rs b/src/function/specify.rs index e341efd05..cde9fe424 100644 --- a/src/function/specify.rs +++ b/src/function/specify.rs @@ -125,7 +125,7 @@ where } let database_key_index = self.database_key_index(key); - memo.mark_as_verified(zalsa, zalsa.current_revision(), database_key_index); + memo.mark_as_verified(zalsa, database_key_index); memo.revisions .accumulated_inputs .store(InputAccumulatedValues::Empty); diff --git a/src/ingredient.rs b/src/ingredient.rs index 33917a6f6..60d2c1f40 100644 --- a/src/ingredient.rs +++ b/src/ingredient.rs @@ -2,7 +2,7 @@ use std::any::{Any, TypeId}; use std::fmt; use crate::accumulator::accumulated_map::{AccumulatedMap, InputAccumulatedValues}; -use crate::cycle::{CycleHeadKind, CycleRecoveryStrategy}; +use crate::cycle::{CycleHeadKind, CycleHeads, CycleRecoveryStrategy}; use crate::function::VerifyResult; use crate::loom::sync::Arc; use crate::plumbing::IngredientIndices; @@ -63,7 +63,7 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync { db: &'db dyn Database, input: Id, revision: Revision, - in_cycle: bool, + cycle_heads: &mut CycleHeads, ) -> VerifyResult; /// Is the value for `input` in this ingredient a cycle head that is still provisional? diff --git a/src/input.rs b/src/input.rs index be7447b86..db86c4c65 100644 --- a/src/input.rs +++ b/src/input.rs @@ -9,6 +9,7 @@ pub mod singleton; use input_field::FieldIngredientImpl; +use crate::cycle::CycleHeads; use crate::function::VerifyResult; use crate::id::{AsId, FromId, FromIdWithDb}; use crate::ingredient::Ingredient; @@ -217,7 +218,7 @@ impl Ingredient for IngredientImpl { _db: &dyn Database, _input: Id, _revision: Revision, - _in_cycle: bool, + _cycle_heads: &mut CycleHeads, ) -> VerifyResult { // Input ingredients are just a counter, they store no data, they are immortal. // Their *fields* are stored in function ingredients elsewhere. diff --git a/src/input/input_field.rs b/src/input/input_field.rs index cafab7a50..d5a4899d8 100644 --- a/src/input/input_field.rs +++ b/src/input/input_field.rs @@ -1,6 +1,7 @@ use std::fmt; use std::marker::PhantomData; +use crate::cycle::CycleHeads; use crate::function::VerifyResult; use crate::ingredient::Ingredient; use crate::input::{Configuration, IngredientImpl, Value}; @@ -54,7 +55,7 @@ where db: &dyn Database, input: Id, revision: Revision, - _in_cycle: bool, + _cycle_heads: &mut CycleHeads, ) -> VerifyResult { let zalsa = db.zalsa(); let value = >::data(zalsa, input); diff --git a/src/interned.rs b/src/interned.rs index 6795c11ba..542436549 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -8,6 +8,7 @@ use std::path::{Path, PathBuf}; use dashmap::SharedValue; +use crate::cycle::CycleHeads; use crate::durability::Durability; use crate::function::VerifyResult; use crate::hash::FxDashMap; @@ -396,13 +397,13 @@ where db: &dyn Database, input: Id, revision: Revision, - _in_cycle: bool, + _cycle_heads: &mut CycleHeads, ) -> VerifyResult { let zalsa = db.zalsa(); let value = zalsa.table().get::>(input); if value.first_interned_at > revision { // The slot was reused. - return VerifyResult::changed(); + return VerifyResult::Changed; } // The slot is valid in this revision but we have to sync the value's revision. diff --git a/src/key.rs b/src/key.rs index 524c926ce..92372b8c7 100644 --- a/src/key.rs +++ b/src/key.rs @@ -1,5 +1,6 @@ use core::fmt; +use crate::cycle::CycleHeads; use crate::function::VerifyResult; use crate::zalsa::{IngredientIndex, Zalsa}; use crate::{Database, Id}; @@ -38,13 +39,13 @@ impl DatabaseKeyIndex { db: &dyn Database, zalsa: &Zalsa, last_verified_at: crate::Revision, - in_cycle: bool, + cycle_heads: &mut CycleHeads, ) -> VerifyResult { // SAFETY: The `db` belongs to the ingredient unsafe { zalsa .lookup_ingredient(self.ingredient_index) - .maybe_changed_after(db, self.key_index, last_verified_at, in_cycle) + .maybe_changed_after(db, self.key_index, last_verified_at, cycle_heads) } } diff --git a/src/tracked_struct.rs b/src/tracked_struct.rs index 377021339..cb2959b51 100644 --- a/src/tracked_struct.rs +++ b/src/tracked_struct.rs @@ -10,6 +10,7 @@ use std::{fmt, mem}; use crossbeam_queue::SegQueue; use tracked_field::FieldIngredientImpl; +use crate::cycle::CycleHeads; use crate::function::VerifyResult; use crate::id::{AsId, FromId}; use crate::ingredient::{Ingredient, Jar}; @@ -761,7 +762,7 @@ where db: &dyn Database, input: Id, revision: Revision, - _in_cycle: bool, + _cycle_heads: &mut CycleHeads, ) -> VerifyResult { let zalsa = db.zalsa(); let data = Self::data(zalsa.table(), input); diff --git a/src/tracked_struct/tracked_field.rs b/src/tracked_struct/tracked_field.rs index 154168727..048a02b2f 100644 --- a/src/tracked_struct/tracked_field.rs +++ b/src/tracked_struct/tracked_field.rs @@ -1,5 +1,6 @@ use std::marker::PhantomData; +use crate::cycle::CycleHeads; use crate::function::VerifyResult; use crate::ingredient::Ingredient; use crate::loom::sync::Arc; @@ -59,7 +60,7 @@ where db: &'db dyn Database, input: Id, revision: crate::Revision, - _in_cycle: bool, + _cycle_heads: &mut CycleHeads, ) -> VerifyResult { let zalsa = db.zalsa(); let data = >::data(zalsa.table(), input); diff --git a/tests/cycle_output.rs b/tests/cycle_output.rs index 11a0c0399..a82aa4dfd 100644 --- a/tests/cycle_output.rs +++ b/tests/cycle_output.rs @@ -152,11 +152,11 @@ fn revalidate_no_changes() { "salsa_event(DidValidateMemoizedValue { database_key: read_value(Id(400)) })", "salsa_event(DidReinternValue { key: query_d::interned_arguments(Id(800)), revision: R2 })", "salsa_event(DidValidateMemoizedValue { database_key: query_d(Id(800)) })", + "salsa_event(DidValidateMemoizedValue { database_key: query_b(Id(0)) })", + "salsa_event(DidReinternValue { key: query_d::interned_arguments(Id(800)), revision: R2 })", "salsa_event(DidValidateMemoizedValue { database_key: read_value(Id(401)) })", "salsa_event(DidValidateMemoizedValue { database_key: read_value(Id(402)) })", "salsa_event(DidValidateMemoizedValue { database_key: read_value(Id(403)) })", - "salsa_event(DidValidateMemoizedValue { database_key: query_b(Id(0)) })", - "salsa_event(DidReinternValue { key: query_d::interned_arguments(Id(800)), revision: R2 })", "salsa_event(DidValidateMemoizedValue { database_key: query_a(Id(0)) })", "salsa_event(DidValidateMemoizedValue { database_key: query_b(Id(0)) })", ]"#]]);