diff --git a/src/cycle.rs b/src/cycle.rs index 0558bda04..4dca7bcc0 100644 --- a/src/cycle.rs +++ b/src/cycle.rs @@ -53,6 +53,7 @@ use thin_vec::{thin_vec, ThinVec}; use crate::key::DatabaseKeyIndex; use crate::sync::OnceLock; +use crate::Revision; /// The maximum number of times we'll fixpoint-iterate before panicking. /// @@ -236,16 +237,30 @@ pub(crate) fn empty_cycle_heads() -> &'static CycleHeads { #[derive(Debug, PartialEq, Eq)] pub enum ProvisionalStatus { - Provisional { iteration: IterationCount }, - Final { iteration: IterationCount }, + Provisional { + iteration: IterationCount, + verified_at: Revision, + }, + Final { + iteration: IterationCount, + verified_at: Revision, + }, FallbackImmediate, } impl ProvisionalStatus { pub(crate) const fn iteration(&self) -> Option { match self { - ProvisionalStatus::Provisional { iteration } => Some(*iteration), - ProvisionalStatus::Final { iteration } => Some(*iteration), + ProvisionalStatus::Provisional { iteration, .. } => Some(*iteration), + ProvisionalStatus::Final { iteration, .. } => Some(*iteration), + ProvisionalStatus::FallbackImmediate => None, + } + } + + pub(crate) const fn verified_at(&self) -> Option { + match self { + ProvisionalStatus::Provisional { verified_at, .. } => Some(*verified_at), + ProvisionalStatus::Final { verified_at, .. } => Some(*verified_at), ProvisionalStatus::FallbackImmediate => None, } } diff --git a/src/function.rs b/src/function.rs index 28b76a0cc..0ff90d70f 100644 --- a/src/function.rs +++ b/src/function.rs @@ -303,10 +303,16 @@ where if C::CYCLE_STRATEGY == CycleRecoveryStrategy::FallbackImmediate { ProvisionalStatus::FallbackImmediate } else { - ProvisionalStatus::Final { iteration } + ProvisionalStatus::Final { + iteration, + verified_at: memo.verified_at.load(), + } } } else { - ProvisionalStatus::Provisional { iteration } + ProvisionalStatus::Provisional { + iteration, + verified_at: memo.verified_at.load(), + } }) } diff --git a/src/function/fetch.rs b/src/function/fetch.rs index 87ff22db3..57b79a52a 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -1,3 +1,5 @@ +use rustc_hash::FxHashMap; + use crate::cycle::{CycleHeads, CycleRecoveryStrategy, IterationCount}; use crate::function::maybe_changed_after::VerifyCycleHeads; use crate::function::memo::Memo; @@ -172,7 +174,6 @@ where zalsa_local, database_key_index, old_memo, - true, ) { self.update_shallow(zalsa, database_key_index, old_memo, can_shallow_update); @@ -182,17 +183,19 @@ where return unsafe { Some(self.extend_memo_lifetime(old_memo)) }; } - let mut cycle_heads = VerifyCycleHeads::default(); + let mut cycle_heads = Vec::new(); + let mut participating_queries = FxHashMap::default(); + let verify_result = self.deep_verify_memo( db, zalsa, old_memo, database_key_index, - &mut cycle_heads, + &mut VerifyCycleHeads::new(&mut cycle_heads, &mut participating_queries), can_shallow_update, ); - if verify_result.is_unchanged() && !cycle_heads.has_any() { + if verify_result.is_unchanged() && cycle_heads.is_empty() { // SAFETY: memo is present in memo_map and we have verified that it is // still valid for the current revision. return unsafe { Some(self.extend_memo_lifetime(old_memo)) }; diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index 54fce885d..2ab1d18ee 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -1,9 +1,12 @@ +use rustc_hash::FxHashMap; + #[cfg(feature = "accumulator")] use crate::accumulator::accumulated_map::InputAccumulatedValues; use crate::cycle::{CycleRecoveryStrategy, IterationCount, ProvisionalStatus}; use crate::function::memo::Memo; use crate::function::sync::ClaimResult; use crate::function::{Configuration, IngredientImpl}; + use crate::key::DatabaseKeyIndex; use crate::sync::atomic::Ordering; use crate::zalsa::{MemoIngredientIndex, Zalsa, ZalsaDatabase}; @@ -11,7 +14,7 @@ use crate::zalsa_local::{QueryEdgeKind, QueryOriginRef, ZalsaLocal}; use crate::{Id, Revision}; /// Result of memo validation. -#[derive(Debug)] +#[derive(Debug, Copy, Clone)] pub enum VerifyResult { /// Memo has changed and needs to be recomputed. Changed, @@ -110,6 +113,7 @@ where if let Some(mcs) = self.maybe_changed_after_cold( zalsa, + zalsa_local, db, id, revision, @@ -124,9 +128,11 @@ where } #[inline(never)] + #[expect(clippy::too_many_arguments)] fn maybe_changed_after_cold<'db>( &'db self, zalsa: &Zalsa, + zalsa_local: &ZalsaLocal, db: &'db C::DbView, key_index: Id, revision: Revision, @@ -143,7 +149,7 @@ where } ClaimResult::Cycle { .. } => { return Some(self.maybe_changed_after_cold_cycle( - db, + zalsa_local, database_key_index, cycle_heads, )) @@ -163,21 +169,32 @@ where let can_shallow_update = self.shallow_verify_memo(zalsa, database_key_index, old_memo); if can_shallow_update.yes() - && self.validate_may_be_provisional( - zalsa, - db.zalsa_local(), - database_key_index, - old_memo, - // Don't conclude that the query is unchanged if the memo itself is still - // provisional (because all its cycle heads have the same iteration count - // as the cycle head memos in the database). - // See https://github.com/salsa-rs/salsa/pull/961 - false, - ) + && self.validate_may_be_provisional(zalsa, zalsa_local, database_key_index, old_memo) { self.update_shallow(zalsa, database_key_index, old_memo, can_shallow_update); - return Some(VerifyResult::unchanged()); + // If `validate_maybe_provisional` returns `true`, but only because all cycle heads are from the same iteration, + // carry over the cycle heads so that the caller verifies them. + if old_memo.may_be_provisional() { + for head in old_memo.cycle_heads() { + cycle_heads.insert_head(head.database_key_index); + } + } + + return Some(if old_memo.revisions.changed_at > revision { + VerifyResult::changed() + } else { + VerifyResult::unchanged_with_accumulated( + #[cfg(feature = "accumulator")] + { + old_memo.revisions.accumulated_inputs.load() + }, + ) + }); + } + + if let Some(cached) = cycle_heads.get_result(database_key_index) { + return Some(*cached); } let deep_verify = self.deep_verify_memo( @@ -210,9 +227,8 @@ where // `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() && !cycle_heads.has_any() { - let active_query = db - .zalsa_local() - .push_query(database_key_index, IterationCount::initial()); + let active_query = + zalsa_local.push_query(database_key_index, IterationCount::initial()); let memo = self.execute(db, active_query, Some(old_memo)); let changed_at = memo.revisions.changed_at; @@ -239,16 +255,16 @@ where #[cold] #[inline(never)] - fn maybe_changed_after_cold_cycle<'db>( - &'db self, - db: &'db C::DbView, + fn maybe_changed_after_cold_cycle( + &self, + zalsa_local: &ZalsaLocal, database_key_index: DatabaseKeyIndex, cycle_heads: &mut VerifyCycleHeads, ) -> VerifyResult { match C::CYCLE_STRATEGY { // SAFETY: We do not access the query stack reentrantly. CycleRecoveryStrategy::Panic => unsafe { - db.zalsa_local().with_query_stack_unchecked(|stack| { + zalsa_local.with_query_stack_unchecked(|stack| { panic!( "dependency graph cycle when validating {database_key_index:#?}, \ set cycle_fn/cycle_initial to fixpoint iterate.\n\ @@ -261,8 +277,23 @@ where crate::tracing::debug!( "hit cycle at {database_key_index:?} in `maybe_changed_after`, returning fixpoint initial value", ); - cycle_heads.insert(database_key_index); - VerifyResult::unchanged() + cycle_heads.insert_head(database_key_index); + + // SAFETY: We don't access the query stack reentrantly. + let running = unsafe { + zalsa_local.with_query_stack_unchecked(|stack| { + stack + .iter() + .any(|query| query.database_key_index == database_key_index) + }) + }; + + // If the cycle head is being executed, consider this query as changed. + if running { + VerifyResult::changed() + } else { + VerifyResult::unchanged() + } } } } @@ -328,7 +359,6 @@ where /// * provisional memos that have been successfully marked as verified final, that is, its /// cycle heads have all been finalized. /// * provisional memos that have been created in the same revision and iteration and are part of the same cycle. - /// This check is skipped if `allow_non_finalized` is `false` as the memo itself is still not finalized. It's a provisional value. #[inline] pub(super) fn validate_may_be_provisional( &self, @@ -336,12 +366,10 @@ where zalsa_local: &ZalsaLocal, database_key_index: DatabaseKeyIndex, memo: &Memo<'_, C>, - allow_non_finalized: bool, ) -> bool { !memo.may_be_provisional() || self.validate_provisional(zalsa, database_key_index, memo) - || (allow_non_finalized - && self.validate_same_iteration(zalsa, zalsa_local, database_key_index, memo)) + || self.validate_same_iteration(zalsa, zalsa_local, database_key_index, memo) } /// Check if this memo's cycle heads have all been finalized. If so, mark it verified final and @@ -357,6 +385,9 @@ where "{database_key_index:?}: validate_provisional(memo = {memo:#?})", memo = memo.tracing_debug() ); + + let memo_verified_at = memo.verified_at.load(); + for cycle_head in memo.revisions.cycle_heads() { // Test if our cycle heads (with the same revision) are now finalized. let Some(kind) = zalsa @@ -365,15 +396,24 @@ where else { return false; }; + match kind { ProvisionalStatus::Provisional { .. } => return false, - ProvisionalStatus::Final { iteration } => { - // It's important to also account for the revision for the case where: + ProvisionalStatus::Final { + iteration, + verified_at, + } => { + // Only consider the cycle head if it is from the same revision as the memo + if verified_at != memo_verified_at { + return false; + } + + // It's important to also account for the iteration for the case where: // thread 1: `b` -> `a` (but only in the first iteration) // -> `c` -> `b` // thread 2: `a` -> `b` // - // If we don't account for the revision, then `a` (from iteration 0) will be finalized + // If we don't account for the iteration, then `a` (from iteration 0) will be finalized // because its cycle head `b` is now finalized, but `b` never pulled `a` in the last iteration. if iteration != cycle_head.iteration_count { return false; @@ -424,6 +464,15 @@ where return true; } + let verified_at = memo.verified_at.load(); + + // This is an optimization to avoid unnecessary re-execution within the same revision. + // Don't apply it when verifying memos from past revisions. We want them to re-execute + // to verify their cycle heads and all participating queries. + if verified_at != zalsa.current_revision() { + return false; + } + // SAFETY: We do not access the query stack reentrantly. unsafe { zalsa_local.with_query_stack_unchecked(|stack| { @@ -477,7 +526,12 @@ where zalsa, cycle_head.database_key_index.key_index(), )?; - provisional_status.iteration() + + if provisional_status.verified_at() == Some(verified_at) { + provisional_status.iteration() + } else { + None + } }) == Some(cycle_head.iteration_count) }) @@ -506,7 +560,7 @@ where old_memo = old_memo.tracing_debug() ); - debug_assert!(!cycle_heads.contains(database_key_index)); + debug_assert!(!cycle_heads.contains_head(database_key_index)); match old_memo.revisions.origin.as_ref() { QueryOriginRef::Derived(edges) => { @@ -534,8 +588,9 @@ where // The `MaybeChangeAfterCycleHeads` is used as an out parameter and it's // the caller's responsibility to pass an empty `heads`, which is what we do here. let mut inner_cycle_heads = VerifyCycleHeads { - heads: std::mem::take(&mut child_cycle_heads), has_outer_cycles: cycle_heads.has_any(), + heads: &mut child_cycle_heads, + participating_queries: cycle_heads.participating_queries, }; let input_result = dependency_index.maybe_changed_after( @@ -545,10 +600,8 @@ where &mut inner_cycle_heads, ); - // Reuse the cycle head allocation. - child_cycle_heads = inner_cycle_heads.heads; // Aggregate the cycle heads into the parent cycle heads - cycle_heads.append(&mut child_cycle_heads); + cycle_heads.append_heads(&mut child_cycle_heads); match input_result { VerifyResult::Changed => return VerifyResult::changed(), @@ -607,23 +660,30 @@ where // from cycle heads. We will handle our own memo (and the rest of our cycle) on a // future iteration; first the outer cycle head needs to verify itself. - cycle_heads.remove(database_key_index); + cycle_heads.remove_head(database_key_index); + + let result = VerifyResult::unchanged_with_accumulated( + #[cfg(feature = "accumulator")] + inputs, + ); + + // This value is only read once the memo is verified. It's therefore safe + // to write a non-final value here. + #[cfg(feature = "accumulator")] + old_memo.revisions.accumulated_inputs.store(inputs); // 1 and 3 if !cycle_heads.has_own() { old_memo.mark_as_verified(zalsa, database_key_index); - #[cfg(feature = "accumulator")] - old_memo.revisions.accumulated_inputs.store(inputs); old_memo .revisions .verified_final .store(true, Ordering::Relaxed); + } else { + cycle_heads.insert_participating_query(database_key_index, result); } - VerifyResult::unchanged_with_accumulated( - #[cfg(feature = "accumulator")] - inputs, - ) + result } QueryOriginRef::Assigned(_) => { @@ -645,7 +705,7 @@ where // are tracked by the outer query. Nothing should have changed assuming that the // fixpoint initial function is deterministic. QueryOriginRef::FixpointInitial => { - cycle_heads.insert(database_key_index); + cycle_heads.insert_head(database_key_index); VerifyResult::unchanged() } QueryOriginRef::DerivedUntracked(_) => { @@ -690,29 +750,58 @@ impl ShallowUpdate { /// aren't included. /// /// [`maybe_changed_after`]: crate::ingredient::Ingredient::maybe_changed_after -#[derive(Debug, Default)] -pub struct VerifyCycleHeads { - heads: Vec, +#[derive(Debug)] +pub struct VerifyCycleHeads<'a> { + /// The cycle heads encountered while verifying this ingredient and its subtree. + heads: &'a mut Vec, + + /// The cached `maybe_changed_after` results for queries that participate in cycles but aren't a cycle head + /// themselves. We need to cache the results here to avoid calling `deep_verify_memo` repeatedly + /// for queries that have cyclic dependencies (b depends on a (iteration 0) and a depends on b(iteration 1)) + /// as well as to avoid a run-away situation if a query is dependet on a lot inside a single cycle. + participating_queries: &'a mut FxHashMap, /// Whether the outer query (e.g. the parent query running `maybe_changed_after`) has encountered /// any cycles to this point. has_outer_cycles: bool, } -impl VerifyCycleHeads { +impl<'a> VerifyCycleHeads<'a> { + pub(crate) fn new( + heads: &'a mut Vec, + participating_queries: &'a mut FxHashMap, + ) -> Self { + Self { + heads, + participating_queries, + has_outer_cycles: false, + } + } + + /// Returns `true` if this query or any of its dependencies depend on this cycle. #[inline] - fn contains(&self, key: DatabaseKeyIndex) -> bool { + fn contains_head(&self, key: DatabaseKeyIndex) -> bool { self.heads.contains(&key) } #[inline] - fn insert(&mut self, key: DatabaseKeyIndex) { + fn insert_head(&mut self, key: DatabaseKeyIndex) { if !self.heads.contains(&key) { self.heads.push(key); } } - fn remove(&mut self, key: DatabaseKeyIndex) -> bool { + #[inline] + fn remove_head(&mut self, key: DatabaseKeyIndex) -> bool { + if self.heads.is_empty() { + return false; + } + + self.remove_head_slow(key) + } + + #[cold] + fn remove_head_slow(&mut self, key: DatabaseKeyIndex) -> bool { let found = self.heads.iter().position(|&head| head == key); let Some(found) = found else { return false }; @@ -721,20 +810,30 @@ impl VerifyCycleHeads { } #[inline] - fn append(&mut self, heads: &mut Vec) { + fn append_heads(&mut self, heads: &mut Vec) { if heads.is_empty() { return; } - self.append_slow(heads); + self.append_heads_slow(heads); } - fn append_slow(&mut self, heads: &mut Vec) { - for key in heads.drain(..) { - self.insert(key); + #[cold] + fn append_heads_slow(&mut self, other: &mut Vec) { + for key in other.drain(..) { + self.insert_head(key); } } + fn insert_participating_query(&mut self, key: DatabaseKeyIndex, result: VerifyResult) { + self.participating_queries.insert(key, result); + } + + #[inline] + fn get_result(&self, key: DatabaseKeyIndex) -> Option<&VerifyResult> { + self.participating_queries.get(&key) + } + /// Returns `true` if this query or any of its dependencies has encountered a cycle or /// if the outer query has encountered a cycle. pub fn has_any(&self) -> bool { diff --git a/src/function/memo.rs b/src/function/memo.rs index 54df61420..dd1615db6 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -526,6 +526,7 @@ impl<'me> Iterator for TryClaimCycleHeadsIter<'me> { .provisional_status(self.zalsa, head_key_index) .unwrap_or(ProvisionalStatus::Provisional { iteration: IterationCount::initial(), + verified_at: Revision::start(), }); match cycle_head_kind { diff --git a/src/ingredient.rs b/src/ingredient.rs index 4cd857962..5509ace8b 100644 --- a/src/ingredient.rs +++ b/src/ingredient.rs @@ -1,9 +1,7 @@ use std::any::{Any, TypeId}; use std::fmt; -use crate::cycle::{ - empty_cycle_heads, CycleHeads, CycleRecoveryStrategy, IterationCount, ProvisionalStatus, -}; +use crate::cycle::{empty_cycle_heads, CycleHeads, CycleRecoveryStrategy, ProvisionalStatus}; use crate::database::RawDatabase; use crate::function::{VerifyCycleHeads, VerifyResult}; use crate::runtime::Running; @@ -60,11 +58,8 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync { /// Is it a provisional value or has it been finalized and in which iteration. /// /// Returns `None` if `input` doesn't exist. - fn provisional_status(&self, zalsa: &Zalsa, input: Id) -> Option { - _ = (zalsa, input); - Some(ProvisionalStatus::Final { - iteration: IterationCount::initial(), - }) + fn provisional_status(&self, _zalsa: &Zalsa, _input: Id) -> Option { + unreachable!("provisional_status should only be called on cycle heads and only functions can be cycle heads"); } /// Returns the cycle heads for this ingredient. diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 3b9e7434f..46f1de86c 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -158,6 +158,7 @@ impl Default for ExecuteValidateLoggerDatabase { move |event| match event.kind { salsa::EventKind::WillExecute { .. } | salsa::EventKind::WillIterateCycle { .. } + | salsa::EventKind::DidValidateInternedValue { .. } | salsa::EventKind::DidValidateMemoizedValue { .. } => { logger.push_log(format!("salsa_event({:?})", event.kind)); } diff --git a/tests/cycle.rs b/tests/cycle.rs index 2eb9bac23..d226a9eb7 100644 --- a/tests/cycle.rs +++ b/tests/cycle.rs @@ -1137,3 +1137,242 @@ fn repeat_provisional_query_incremental() { "salsa_event(WillExecute { database_key: min_panic(Id(2)) })", ]"#]]); } + +/// Tests a situation where a query participating in a cycle gets called many times (think thousands of times). +/// +/// We want to avoid calling `deep_verify_memo` for that query over and over again. +/// This isn't an issue for regular queries because a non-cyclic query is guaranteed to be verified +/// after `maybe_changed_after` because: +/// * It can be shallow verified +/// * `deep_verify_memo` returns `unchanged` and it updates the `verified_at` revision. +/// * `deep_verify_memo` returns `changed` and Salsa re-executes the query. The query is verified once `execute` completes. +/// +/// The same guarantee doesn't exist for queries participating in cycles because: +/// +/// * Salsa update `verified_at` because it depends on the cycle head if the query didn't change. +/// * Salsa doesn't run `execute` because some inputs may not have been verified yet (which can lead to all sort of pancis). +#[test] +fn repeat_query_participating_in_cycle() { + #[salsa::input] + struct Input { + value: u32, + } + + #[salsa::interned] + struct Interned { + value: u32, + } + + #[salsa::tracked(cycle_fn=cycle_recover, cycle_initial=initial)] + fn head(db: &dyn Db, input: Input) -> u32 { + let a = query_a(db, input); + + a.min(2) + } + + fn initial(_db: &dyn Db, _input: Input) -> u32 { + 0 + } + + fn cycle_recover( + _db: &dyn Db, + _value: &u32, + _count: u32, + _input: Input, + ) -> CycleRecoveryAction { + CycleRecoveryAction::Iterate + } + + #[salsa::tracked] + fn query_a(db: &dyn Db, input: Input) -> u32 { + let _ = query_b(db, input); + + query_hot(db, input) + } + + #[salsa::tracked] + fn query_b(db: &dyn Db, input: Input) -> u32 { + let _ = query_c(db, input); + + query_hot(db, input) + } + + #[salsa::tracked] + fn query_c(db: &dyn Db, input: Input) -> u32 { + let _ = query_d(db, input); + + query_hot(db, input) + } + + #[salsa::tracked] + fn query_d(db: &dyn Db, input: Input) -> u32 { + query_hot(db, input) + } + + #[salsa::tracked] + fn query_hot(db: &dyn Db, input: Input) -> u32 { + let value = head(db, input); + + let _ = Interned::new(db, 2); + + let _ = input.value(db); + + value + 1 + } + + let mut db = ExecuteValidateLoggerDatabase::default(); + + let input = Input::new(&db, 1); + + assert_eq!(head(&db, input), 2); + + db.clear_logs(); + + input.set_value(&mut db).to(10); + + assert_eq!(head(&db, input), 2); + + // The interned value should only be validate once. We otherwise have a + // run-away situation where `deep_verify_memo` of `query_hot` is called over and over again. + // * First: when checking if `head` has changed + // * Second: when checking if `query_a` has changed + // * Third: when checking if `query_b` has changed + // * ... + // Ultimately, this can easily be more expensive than running the cycle head again. + db.assert_logs(expect![[r#" + [ + "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", + "salsa_event(WillExecute { database_key: head(Id(0)) })", + "salsa_event(WillExecute { database_key: query_a(Id(0)) })", + "salsa_event(WillExecute { database_key: query_b(Id(0)) })", + "salsa_event(WillExecute { database_key: query_c(Id(0)) })", + "salsa_event(WillExecute { database_key: query_d(Id(0)) })", + "salsa_event(WillExecute { database_key: query_hot(Id(0)) })", + "salsa_event(WillIterateCycle { database_key: head(Id(0)), iteration_count: IterationCount(1), fell_back: false })", + "salsa_event(WillExecute { database_key: query_a(Id(0)) })", + "salsa_event(WillExecute { database_key: query_b(Id(0)) })", + "salsa_event(WillExecute { database_key: query_c(Id(0)) })", + "salsa_event(WillExecute { database_key: query_d(Id(0)) })", + "salsa_event(WillExecute { database_key: query_hot(Id(0)) })", + "salsa_event(WillIterateCycle { database_key: head(Id(0)), iteration_count: IterationCount(2), fell_back: false })", + "salsa_event(WillExecute { database_key: query_a(Id(0)) })", + "salsa_event(WillExecute { database_key: query_b(Id(0)) })", + "salsa_event(WillExecute { database_key: query_c(Id(0)) })", + "salsa_event(WillExecute { database_key: query_d(Id(0)) })", + "salsa_event(WillExecute { database_key: query_hot(Id(0)) })", + ]"#]]); +} + +/// Tests a similar scenario as `repeat_query_participating_in_cycle` with the main difference +/// that `query_hot` is called before calling the next `query_xxx`. +#[test] +fn repeat_query_participating_in_cycle2() { + #[salsa::input] + struct Input { + value: u32, + } + + #[salsa::interned] + struct Interned { + value: u32, + } + + #[salsa::tracked(cycle_fn=cycle_recover, cycle_initial=initial)] + fn head(db: &dyn Db, input: Input) -> u32 { + let a = query_a(db, input); + + a.min(2) + } + + fn initial(_db: &dyn Db, _input: Input) -> u32 { + 0 + } + + fn cycle_recover( + _db: &dyn Db, + _value: &u32, + _count: u32, + _input: Input, + ) -> CycleRecoveryAction { + CycleRecoveryAction::Iterate + } + + #[salsa::tracked(cycle_fn=cycle_recover, cycle_initial=initial)] + fn query_a(db: &dyn Db, input: Input) -> u32 { + let _ = query_hot(db, input); + query_b(db, input) + } + + #[salsa::tracked] + fn query_b(db: &dyn Db, input: Input) -> u32 { + let _ = query_hot(db, input); + query_c(db, input) + } + + #[salsa::tracked] + fn query_c(db: &dyn Db, input: Input) -> u32 { + let _ = query_hot(db, input); + query_d(db, input) + } + + #[salsa::tracked] + fn query_d(db: &dyn Db, input: Input) -> u32 { + let _ = query_hot(db, input); + + let value = head(db, input); + let _ = input.value(db); + + value + 1 + } + + #[salsa::tracked] + fn query_hot(db: &dyn Db, input: Input) -> u32 { + let _ = Interned::new(db, 2); + + let _ = head(db, input); + + 1 + } + + let mut db = ExecuteValidateLoggerDatabase::default(); + + let input = Input::new(&db, 1); + + assert_eq!(head(&db, input), 2); + + db.clear_logs(); + + input.set_value(&mut db).to(10); + + assert_eq!(head(&db, input), 2); + + // `DidValidateInternedValue { key: Interned(Id(400)), revision: R2 }` should only be logged + // once per `maybe_changed_after` root-call (e.g. validating `head` shouldn't validate `query_hot` multiple times). + // + // This is important to avoid a run-away situation where a query is called many times within a cycle and + // Salsa would end up recusively validating the hot query over and over again. + db.assert_logs(expect![[r#" + [ + "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", + "salsa_event(WillExecute { database_key: head(Id(0)) })", + "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", + "salsa_event(WillExecute { database_key: query_a(Id(0)) })", + "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", + "salsa_event(WillExecute { database_key: query_hot(Id(0)) })", + "salsa_event(WillExecute { database_key: query_b(Id(0)) })", + "salsa_event(WillExecute { database_key: query_c(Id(0)) })", + "salsa_event(WillExecute { database_key: query_d(Id(0)) })", + "salsa_event(WillIterateCycle { database_key: head(Id(0)), iteration_count: IterationCount(1), fell_back: false })", + "salsa_event(WillExecute { database_key: query_a(Id(0)) })", + "salsa_event(WillExecute { database_key: query_hot(Id(0)) })", + "salsa_event(WillExecute { database_key: query_b(Id(0)) })", + "salsa_event(WillExecute { database_key: query_c(Id(0)) })", + "salsa_event(WillExecute { database_key: query_d(Id(0)) })", + "salsa_event(WillIterateCycle { database_key: head(Id(0)), iteration_count: IterationCount(2), fell_back: false })", + "salsa_event(WillExecute { database_key: query_a(Id(0)) })", + "salsa_event(WillExecute { database_key: query_hot(Id(0)) })", + "salsa_event(WillExecute { database_key: query_b(Id(0)) })", + "salsa_event(WillExecute { database_key: query_c(Id(0)) })", + "salsa_event(WillExecute { database_key: query_d(Id(0)) })", + ]"#]]); +} diff --git a/tests/cycle_output.rs b/tests/cycle_output.rs index c8e17ba6b..27ba304e3 100644 --- a/tests/cycle_output.rs +++ b/tests/cycle_output.rs @@ -184,7 +184,6 @@ fn revalidate_with_change_after_output_read() { "salsa_event(DidValidateMemoizedValue { database_key: read_value(Id(400)) })", "salsa_event(DidValidateInternedValue { key: query_d::interned_arguments(Id(800)), revision: R2 })", "salsa_event(WillExecute { database_key: query_b(Id(0)) })", - "salsa_event(DidValidateInternedValue { key: query_d::interned_arguments(Id(800)), revision: R2 })", "salsa_event(WillExecute { database_key: query_a(Id(0)) })", "salsa_event(WillExecute { database_key: query_d(Id(800)) })", "salsa_event(WillDiscardStaleOutput { execute_key: query_a(Id(0)), output_key: Output(Id(401)) })",