From a6da747412677ef5e3815d866fba1f319460dd19 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 18 Aug 2025 16:28:47 +0200 Subject: [PATCH 01/13] fix: Runaway for unchanged queries participating in cycle --- src/function/fetch.rs | 2 +- src/function/maybe_changed_after.rs | 10 ++- tests/common/mod.rs | 1 + tests/cycle.rs | 124 ++++++++++++++++++++++++++++ 4 files changed, 133 insertions(+), 4 deletions(-) diff --git a/src/function/fetch.rs b/src/function/fetch.rs index 87ff22db3..5dee42bc0 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -23,7 +23,7 @@ where let database_key_index = self.database_key_index(id); #[cfg(debug_assertions)] - let _span = crate::tracing::debug_span!("fetch", query = ?database_key_index).entered(); + let _span = tracing::warn_span!("fetch", query = ?database_key_index).entered(); let memo = self.refresh_memo(db, zalsa, zalsa_local, id); diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index 54fce885d..778083271 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -81,9 +81,8 @@ where loop { let database_key_index = self.database_key_index(id); - crate::tracing::debug!( - "{database_key_index:?}: maybe_changed_after(revision = {revision:?})" - ); + let _entered = + tracing::warn_span!("maybe_changed_after", query=?database_key_index).entered(); // Check if we have a verified version: this is the hot path. let memo_guard = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index); @@ -609,6 +608,11 @@ where cycle_heads.remove(database_key_index); + tracing::warn!( + "Cycle heads after maybe_changed_after: {:?} for {database_key_index:?}", + cycle_heads + ); + // 1 and 3 if !cycle_heads.has_own() { old_memo.mark_as_verified(zalsa, database_key_index); 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..08e50dc39 100644 --- a/tests/cycle.rs +++ b/tests/cycle.rs @@ -1137,3 +1137,127 @@ 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. +/// To some degree, this isn't specific to cycles, because `maybe_changed_after` does have that "back-off" behavior +/// where it slowely narrows in to find the query that needs re-executing. However, what's different +/// for queries participating in a cycle is that this not only happens for queries that have changed, +/// it also happens for unchanged queries that participate in cycles, making it a much bigger problem. +#[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(cycle_fn=cycle_recover, cycle_initial=initial)] + 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(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_b(Id(0)) })", + "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", + "salsa_event(WillExecute { database_key: query_c(Id(0)) })", + "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", + "salsa_event(WillExecute { database_key: query_d(Id(0)) })", + "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", + "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)) })", + ]"#]]); +} From 6d31c9d7c5816e02eaf2d14af4bb63de3b0a06d1 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 18 Aug 2025 18:23:06 +0200 Subject: [PATCH 02/13] Another regression test --- tests/cycle.rs | 143 +++++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 133 insertions(+), 10 deletions(-) diff --git a/tests/cycle.rs b/tests/cycle.rs index 08e50dc39..05c688e06 100644 --- a/tests/cycle.rs +++ b/tests/cycle.rs @@ -1141,10 +1141,16 @@ fn repeat_provisional_query_incremental() { /// 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. -/// To some degree, this isn't specific to cycles, because `maybe_changed_after` does have that "back-off" behavior -/// where it slowely narrows in to find the query that needs re-executing. However, what's different -/// for queries participating in a cycle is that this not only happens for queries that have changed, -/// it also happens for unchanged queries that participate in cycles, making it a much bigger problem. +/// 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] @@ -1177,7 +1183,7 @@ fn repeat_query_participating_in_cycle() { CycleRecoveryAction::Iterate } - #[salsa::tracked(cycle_fn=cycle_recover, cycle_initial=initial)] + #[salsa::tracked] fn query_a(db: &dyn Db, input: Input) -> u32 { let _ = query_b(db, input); @@ -1237,15 +1243,10 @@ fn repeat_query_participating_in_cycle() { [ "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_b(Id(0)) })", - "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", "salsa_event(WillExecute { database_key: query_c(Id(0)) })", - "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", "salsa_event(WillExecute { database_key: query_d(Id(0)) })", - "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", "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)) })", @@ -1261,3 +1262,125 @@ fn repeat_query_participating_in_cycle() { "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 value = 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); + + // 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(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", + "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", + "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_hot(Id(0)) })", + "salsa_event(WillExecute { database_key: query_b(Id(0)) })", + "salsa_event(WillExecute { database_key: query_hot(Id(0)) })", + "salsa_event(WillExecute { database_key: query_c(Id(0)) })", + "salsa_event(WillExecute { database_key: query_hot(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)) })", + ]"#]]); +} From 4dda9c69db86ae742fba2f5a161f8364b748cb95 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 19 Aug 2025 09:00:15 +0200 Subject: [PATCH 03/13] Fix runaway situation --- src/function/maybe_changed_after.rs | 68 +++++++++++++++++++++-------- tests/cycle.rs | 22 +++++----- 2 files changed, 61 insertions(+), 29 deletions(-) diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index 778083271..6cf955ef8 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, @@ -179,6 +182,10 @@ where return Some(VerifyResult::unchanged()); } + if let Some(cached) = cycle_heads.get_result(database_key_index) { + return Some(*cached); + } + let deep_verify = self.deep_verify_memo( db, zalsa, @@ -260,7 +267,7 @@ where crate::tracing::debug!( "hit cycle at {database_key_index:?} in `maybe_changed_after`, returning fixpoint initial value", ); - cycle_heads.insert(database_key_index); + cycle_heads.insert_head(database_key_index); VerifyResult::unchanged() } } @@ -505,7 +512,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,6 +541,9 @@ where // 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), + participating_queries: std::mem::take( + &mut cycle_heads.participating_queries, + ), has_outer_cycles: cycle_heads.has_any(), }; @@ -546,8 +556,10 @@ where // Reuse the cycle head allocation. child_cycle_heads = inner_cycle_heads.heads; + cycle_heads.participating_queries = + inner_cycle_heads.participating_queries; // 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(), @@ -606,13 +618,18 @@ 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); tracing::warn!( "Cycle heads after maybe_changed_after: {:?} for {database_key_index:?}", cycle_heads ); + let result = VerifyResult::unchanged_with_accumulated( + #[cfg(feature = "accumulator")] + inputs, + ); + // 1 and 3 if !cycle_heads.has_own() { old_memo.mark_as_verified(zalsa, database_key_index); @@ -622,12 +639,11 @@ where .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(_) => { @@ -649,7 +665,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(_) => { @@ -696,27 +712,35 @@ impl ShallowUpdate { /// [`maybe_changed_after`]: crate::ingredient::Ingredient::maybe_changed_after #[derive(Debug, Default)] pub struct VerifyCycleHeads { + /// The cycle heads encountered while verifying this ingredient and its subtree. heads: 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: 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 { + /// 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 { + fn remove_head(&mut self, key: DatabaseKeyIndex) -> bool { let found = self.heads.iter().position(|&head| head == key); let Some(found) = found else { return false }; @@ -725,20 +749,28 @@ 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); + 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); + } + + 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/tests/cycle.rs b/tests/cycle.rs index 05c688e06..fc0078f8a 100644 --- a/tests/cycle.rs +++ b/tests/cycle.rs @@ -1243,10 +1243,15 @@ fn repeat_query_participating_in_cycle() { [ "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_b(Id(0)) })", + "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", "salsa_event(WillExecute { database_key: query_c(Id(0)) })", + "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", "salsa_event(WillExecute { database_key: query_d(Id(0)) })", + "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", "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)) })", @@ -1329,7 +1334,7 @@ fn repeat_query_participating_in_cycle2() { fn query_hot(db: &dyn Db, input: Input) -> u32 { let _ = Interned::new(db, 2); - let value = head(db, input); + let _ = head(db, input); 1 } @@ -1346,18 +1351,13 @@ fn repeat_query_participating_in_cycle2() { 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. + // `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(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", - "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", "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 })", From 4bf96b0a6a23c1b535a1e6109a597c0b149e95fb Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 19 Aug 2025 09:07:40 +0200 Subject: [PATCH 04/13] Discard changes to src/function/fetch.rs --- src/function/fetch.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/function/fetch.rs b/src/function/fetch.rs index 5dee42bc0..87ff22db3 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -23,7 +23,7 @@ where let database_key_index = self.database_key_index(id); #[cfg(debug_assertions)] - let _span = tracing::warn_span!("fetch", query = ?database_key_index).entered(); + let _span = crate::tracing::debug_span!("fetch", query = ?database_key_index).entered(); let memo = self.refresh_memo(db, zalsa, zalsa_local, id); From 76360630d9c65e1951aa196a3e2a29365ffb5e19 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 19 Aug 2025 09:08:45 +0200 Subject: [PATCH 05/13] Undo tracing changes --- src/function/maybe_changed_after.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index 6cf955ef8..a4a699d88 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -84,8 +84,9 @@ where loop { let database_key_index = self.database_key_index(id); - let _entered = - tracing::warn_span!("maybe_changed_after", query=?database_key_index).entered(); + crate::tracing::debug!( + "{database_key_index:?}: maybe_changed_after(revision = {revision:?})" + ); // Check if we have a verified version: this is the hot path. let memo_guard = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index); @@ -620,11 +621,6 @@ where cycle_heads.remove_head(database_key_index); - tracing::warn!( - "Cycle heads after maybe_changed_after: {:?} for {database_key_index:?}", - cycle_heads - ); - let result = VerifyResult::unchanged_with_accumulated( #[cfg(feature = "accumulator")] inputs, From 759f1152bdf712b4c9582238d568b8c5f067ed22 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 19 Aug 2025 09:12:42 +0200 Subject: [PATCH 06/13] Move accumulated write outside of non-cycle branch --- src/function/maybe_changed_after.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index a4a699d88..e81c5211d 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -626,11 +626,14 @@ where 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 From 7882634ef00a1a134da7fbe3cb284cf47c8568f4 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 19 Aug 2025 09:18:23 +0200 Subject: [PATCH 07/13] Short circuit if cycle head is executing --- src/function/maybe_changed_after.rs | 19 +++++++++++++++++-- tests/cycle.rs | 5 ----- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index e81c5211d..d82283ea8 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -10,7 +10,7 @@ use crate::function::{Configuration, IngredientImpl}; use crate::key::DatabaseKeyIndex; use crate::sync::atomic::Ordering; use crate::zalsa::{MemoIngredientIndex, Zalsa, ZalsaDatabase}; -use crate::zalsa_local::{QueryEdgeKind, QueryOriginRef, ZalsaLocal}; +use crate::zalsa_local::{self, QueryEdgeKind, QueryOriginRef, ZalsaLocal}; use crate::{Id, Revision}; /// Result of memo validation. @@ -269,7 +269,22 @@ where "hit cycle at {database_key_index:?} in `maybe_changed_after`, returning fixpoint initial value", ); cycle_heads.insert_head(database_key_index); - VerifyResult::unchanged() + + // SAFETY: We don't access the query stack reentrantly. + let running = unsafe { + db.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() + } } } } diff --git a/tests/cycle.rs b/tests/cycle.rs index fc0078f8a..a385de035 100644 --- a/tests/cycle.rs +++ b/tests/cycle.rs @@ -1243,15 +1243,10 @@ fn repeat_query_participating_in_cycle() { [ "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_b(Id(0)) })", - "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", "salsa_event(WillExecute { database_key: query_c(Id(0)) })", - "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", "salsa_event(WillExecute { database_key: query_d(Id(0)) })", - "salsa_event(DidValidateInternedValue { key: Interned(Id(400)), revision: R2 })", "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)) })", From 6e43ecd8e8222d671e29327147b316dddb61cec7 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 19 Aug 2025 09:19:03 +0200 Subject: [PATCH 08/13] Inline --- src/function/maybe_changed_after.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index d82283ea8..0f207d455 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -10,7 +10,7 @@ use crate::function::{Configuration, IngredientImpl}; use crate::key::DatabaseKeyIndex; use crate::sync::atomic::Ordering; use crate::zalsa::{MemoIngredientIndex, Zalsa, ZalsaDatabase}; -use crate::zalsa_local::{self, QueryEdgeKind, QueryOriginRef, ZalsaLocal}; +use crate::zalsa_local::{QueryEdgeKind, QueryOriginRef, ZalsaLocal}; use crate::{Id, Revision}; /// Result of memo validation. @@ -781,6 +781,7 @@ impl VerifyCycleHeads { self.participating_queries.insert(key, result); } + #[inline] fn get_result(&self, key: DatabaseKeyIndex) -> Option<&VerifyResult> { self.participating_queries.get(&key) } From 5e7fecf18bfe84e5126bdf1c2e1c25c5c2b845e0 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 19 Aug 2025 09:26:40 +0200 Subject: [PATCH 09/13] Update expected test output --- tests/cycle_output.rs | 1 - 1 file changed, 1 deletion(-) 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)) })", From 246daf0a2d408fe851003d019d6d6b18cbd1fcd7 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 19 Aug 2025 12:04:25 +0200 Subject: [PATCH 10/13] Fix double execution --- src/cycle.rs | 23 +++- src/function.rs | 10 +- src/function/fetch.rs | 1 - src/function/maybe_changed_after.rs | 182 ++++++++++++++++------------ src/function/memo.rs | 1 + src/ingredient.rs | 11 +- tests/cycle.rs | 3 - 7 files changed, 137 insertions(+), 94 deletions(-) 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..98e9fb2fd 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -172,7 +172,6 @@ where zalsa_local, database_key_index, old_memo, - true, ) { self.update_shallow(zalsa, database_key_index, old_memo, can_shallow_update); diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index 0f207d455..5e634a7c6 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -113,6 +113,7 @@ where if let Some(mcs) = self.maybe_changed_after_cold( zalsa, + zalsa_local, db, id, revision, @@ -127,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, @@ -146,7 +149,7 @@ where } ClaimResult::Cycle { .. } => { return Some(self.maybe_changed_after_cold_cycle( - db, + zalsa_local, database_key_index, cycle_heads, )) @@ -166,21 +169,28 @@ 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) { @@ -217,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; @@ -246,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\ @@ -272,7 +281,7 @@ where // SAFETY: We don't access the query stack reentrantly. let running = unsafe { - db.zalsa_local().with_query_stack_unchecked(|stack| { + zalsa_local.with_query_stack_unchecked(|stack| { stack .iter() .any(|query| query.database_key_index == database_key_index) @@ -350,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, @@ -358,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 @@ -379,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 @@ -387,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; @@ -446,62 +464,74 @@ where return true; } + let verified_at = memo.verified_at.load(); + let current_revision = zalsa.current_revision(); + // SAFETY: We do not access the query stack reentrantly. unsafe { zalsa_local.with_query_stack_unchecked(|stack| { cycle_heads.iter().all(|cycle_head| { - stack - .iter() - .rev() - .find(|query| query.database_key_index == cycle_head.database_key_index) - .map(|query| query.iteration_count()) - .or_else(|| { - // If the cycle head isn't on our stack because: - // - // * another thread holds the lock on the cycle head (but it waits for the current query to complete) - // * we're in `maybe_changed_after` because `maybe_changed_after` doesn't modify the cycle stack - // - // check if the latest memo has the same iteration count. + // If the memo was verified in the current revision, check if it has the same iteration count as any query on the stack. + // We skip this check for memo's from past revisions because we want to re-execute them if the cycle heads re-execute. + if current_revision == verified_at { + if let Some(query) = stack + .iter() + .rev() + .find(|query| query.database_key_index == cycle_head.database_key_index) + { + return query.iteration_count() == cycle_head.iteration_count; + } + } - // However, we've to be careful to skip over fixpoint initial values: - // If the head is the memo we're trying to validate, always return `None` - // to force a re-execution of the query. This is necessary because the query - // has obviously not completed its iteration yet. - // - // This should be rare but the `cycle_panic` test fails on some platforms (mainly GitHub actions) - // without this check. What happens there is that: - // - // * query a blocks on query b - // * query b tries to claim a, fails to do so and inserts the fixpoint initial value - // * query b completes and has `a` as head. It returns its query result Salsa blocks query b from - // exiting inside `block_on` (or the thread would complete before the cycle iteration is complete) - // * query a resumes but panics because of the fixpoint iteration function - // * query b resumes. It rexecutes its own query which then tries to fetch a (which depends on itself because it's a fixpoint initial value). - // Without this check, `validate_same_iteration` would return `true` because the latest memo for `a` is the fixpoint initial value. - // But it should return `false` so that query b's thread re-executes `a` (which then also causes the panic). - // - // That's why we always return `None` if the cycle head is the same as the current database key index. - if cycle_head.database_key_index == database_key_index { - return None; - } + // If the cycle head isn't on our stack because: + // + // * another thread holds the lock on the cycle head (but it waits for the current query to complete) + // * we're in `maybe_changed_after` because `maybe_changed_after` doesn't modify the cycle stack + // + // check if the latest memo has the same iteration count. - let ingredient = zalsa.lookup_ingredient( - cycle_head.database_key_index.ingredient_index(), - ); - let wait_result = ingredient - .wait_for(zalsa, cycle_head.database_key_index.key_index()); + // However, we've to be careful to skip over fixpoint initial values: + // If the head is the memo we're trying to validate, always return `None` + // to force a re-execution of the query. This is necessary because the query + // has obviously not completed its iteration yet. + // + // This should be rare but the `cycle_panic` test fails on some platforms (mainly GitHub actions) + // without this check. What happens there is that: + // + // * query a blocks on query b + // * query b tries to claim a, fails to do so and inserts the fixpoint initial value + // * query b completes and has `a` as head. It returns its query result Salsa blocks query b from + // exiting inside `block_on` (or the thread would complete before the cycle iteration is complete) + // * query a resumes but panics because of the fixpoint iteration function + // * query b resumes. It rexecutes its own query which then tries to fetch a (which depends on itself because it's a fixpoint initial value). + // Without this check, `validate_same_iteration` would return `true` because the latest memo for `a` is the fixpoint initial value. + // But it should return `false` so that query b's thread re-executes `a` (which then also causes the panic). + // + // That's why we always return `None` if the cycle head is the same as the current database key index. + if cycle_head.database_key_index == database_key_index { + return false; + } - if !wait_result.is_cycle() { - return None; - } + let ingredient = + zalsa.lookup_ingredient(cycle_head.database_key_index.ingredient_index()); + let wait_result = + ingredient.wait_for(zalsa, cycle_head.database_key_index.key_index()); - let provisional_status = ingredient.provisional_status( - zalsa, - cycle_head.database_key_index.key_index(), - )?; - provisional_status.iteration() - }) - == Some(cycle_head.iteration_count) + if !wait_result.is_cycle() { + return false; + } + + let Some(provisional_status) = ingredient + .provisional_status(zalsa, cycle_head.database_key_index.key_index()) + else { + return false; + }; + + if provisional_status.verified_at() != Some(verified_at) { + false + } else { + provisional_status.iteration() == Some(cycle_head.iteration_count) + } }) }) } 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/cycle.rs b/tests/cycle.rs index a385de035..d226a9eb7 100644 --- a/tests/cycle.rs +++ b/tests/cycle.rs @@ -1359,11 +1359,8 @@ fn repeat_query_participating_in_cycle2() { "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_hot(Id(0)) })", "salsa_event(WillExecute { database_key: query_b(Id(0)) })", - "salsa_event(WillExecute { database_key: query_hot(Id(0)) })", "salsa_event(WillExecute { database_key: query_c(Id(0)) })", - "salsa_event(WillExecute { database_key: query_hot(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)) })", From c1b538430c35cc053ae1fd2ff5ebb52085fcd1bb Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 19 Aug 2025 12:16:51 +0200 Subject: [PATCH 11/13] Simplify check in `validate_same_iteration` --- src/function/maybe_changed_after.rs | 116 ++++++++++++++-------------- 1 file changed, 59 insertions(+), 57 deletions(-) diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index 5e634a7c6..883ea0b76 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -465,73 +465,75 @@ where } let verified_at = memo.verified_at.load(); - let current_revision = zalsa.current_revision(); + + // 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| { cycle_heads.iter().all(|cycle_head| { - // If the memo was verified in the current revision, check if it has the same iteration count as any query on the stack. - // We skip this check for memo's from past revisions because we want to re-execute them if the cycle heads re-execute. - if current_revision == verified_at { - if let Some(query) = stack - .iter() - .rev() - .find(|query| query.database_key_index == cycle_head.database_key_index) - { - return query.iteration_count() == cycle_head.iteration_count; - } - } - - // If the cycle head isn't on our stack because: - // - // * another thread holds the lock on the cycle head (but it waits for the current query to complete) - // * we're in `maybe_changed_after` because `maybe_changed_after` doesn't modify the cycle stack - // - // check if the latest memo has the same iteration count. + stack + .iter() + .rev() + .find(|query| query.database_key_index == cycle_head.database_key_index) + .map(|query| query.iteration_count()) + .or_else(|| { + // If the cycle head isn't on our stack because: + // + // * another thread holds the lock on the cycle head (but it waits for the current query to complete) + // * we're in `maybe_changed_after` because `maybe_changed_after` doesn't modify the cycle stack + // + // check if the latest memo has the same iteration count. - // However, we've to be careful to skip over fixpoint initial values: - // If the head is the memo we're trying to validate, always return `None` - // to force a re-execution of the query. This is necessary because the query - // has obviously not completed its iteration yet. - // - // This should be rare but the `cycle_panic` test fails on some platforms (mainly GitHub actions) - // without this check. What happens there is that: - // - // * query a blocks on query b - // * query b tries to claim a, fails to do so and inserts the fixpoint initial value - // * query b completes and has `a` as head. It returns its query result Salsa blocks query b from - // exiting inside `block_on` (or the thread would complete before the cycle iteration is complete) - // * query a resumes but panics because of the fixpoint iteration function - // * query b resumes. It rexecutes its own query which then tries to fetch a (which depends on itself because it's a fixpoint initial value). - // Without this check, `validate_same_iteration` would return `true` because the latest memo for `a` is the fixpoint initial value. - // But it should return `false` so that query b's thread re-executes `a` (which then also causes the panic). - // - // That's why we always return `None` if the cycle head is the same as the current database key index. - if cycle_head.database_key_index == database_key_index { - return false; - } + // However, we've to be careful to skip over fixpoint initial values: + // If the head is the memo we're trying to validate, always return `None` + // to force a re-execution of the query. This is necessary because the query + // has obviously not completed its iteration yet. + // + // This should be rare but the `cycle_panic` test fails on some platforms (mainly GitHub actions) + // without this check. What happens there is that: + // + // * query a blocks on query b + // * query b tries to claim a, fails to do so and inserts the fixpoint initial value + // * query b completes and has `a` as head. It returns its query result Salsa blocks query b from + // exiting inside `block_on` (or the thread would complete before the cycle iteration is complete) + // * query a resumes but panics because of the fixpoint iteration function + // * query b resumes. It rexecutes its own query which then tries to fetch a (which depends on itself because it's a fixpoint initial value). + // Without this check, `validate_same_iteration` would return `true` because the latest memo for `a` is the fixpoint initial value. + // But it should return `false` so that query b's thread re-executes `a` (which then also causes the panic). + // + // That's why we always return `None` if the cycle head is the same as the current database key index. + if cycle_head.database_key_index == database_key_index { + return None; + } - let ingredient = - zalsa.lookup_ingredient(cycle_head.database_key_index.ingredient_index()); - let wait_result = - ingredient.wait_for(zalsa, cycle_head.database_key_index.key_index()); + let ingredient = zalsa.lookup_ingredient( + cycle_head.database_key_index.ingredient_index(), + ); + let wait_result = ingredient + .wait_for(zalsa, cycle_head.database_key_index.key_index()); - if !wait_result.is_cycle() { - return false; - } + if !wait_result.is_cycle() { + return None; + } - let Some(provisional_status) = ingredient - .provisional_status(zalsa, cycle_head.database_key_index.key_index()) - else { - return false; - }; + let provisional_status = ingredient.provisional_status( + zalsa, + cycle_head.database_key_index.key_index(), + )?; - if provisional_status.verified_at() != Some(verified_at) { - false - } else { - provisional_status.iteration() == Some(cycle_head.iteration_count) - } + if provisional_status.verified_at() == Some(verified_at) { + provisional_status.iteration() + } else { + None + } + }) + == Some(cycle_head.iteration_count) }) }) } From 545b0e3087b237e95dbbe506606abd559349b66d Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 19 Aug 2025 19:37:43 +0200 Subject: [PATCH 12/13] Some more inline --- src/function/maybe_changed_after.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index 883ea0b76..b2564e0b1 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -786,7 +786,17 @@ impl VerifyCycleHeads { } } + #[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 }; @@ -803,6 +813,7 @@ impl VerifyCycleHeads { self.append_heads_slow(heads); } + #[cold] fn append_heads_slow(&mut self, other: &mut Vec) { for key in other.drain(..) { self.insert_head(key); From f39e1947af0b00f63afac32e27449029c1b1499c Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Wed, 20 Aug 2025 08:12:53 +0200 Subject: [PATCH 13/13] Pass references --- src/function/fetch.rs | 10 +++++++--- src/function/maybe_changed_after.rs | 31 +++++++++++++++++------------ 2 files changed, 25 insertions(+), 16 deletions(-) diff --git a/src/function/fetch.rs b/src/function/fetch.rs index 98e9fb2fd..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; @@ -181,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 b2564e0b1..2ab1d18ee 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -588,11 +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), - participating_queries: std::mem::take( - &mut cycle_heads.participating_queries, - ), 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( @@ -602,10 +600,6 @@ where &mut inner_cycle_heads, ); - // Reuse the cycle head allocation. - child_cycle_heads = inner_cycle_heads.heads; - cycle_heads.participating_queries = - inner_cycle_heads.participating_queries; // Aggregate the cycle heads into the parent cycle heads cycle_heads.append_heads(&mut child_cycle_heads); @@ -756,23 +750,34 @@ impl ShallowUpdate { /// aren't included. /// /// [`maybe_changed_after`]: crate::ingredient::Ingredient::maybe_changed_after -#[derive(Debug, Default)] -pub struct VerifyCycleHeads { +#[derive(Debug)] +pub struct VerifyCycleHeads<'a> { /// The cycle heads encountered while verifying this ingredient and its subtree. - heads: Vec, + 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: FxHashMap, + 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_head(&self, key: DatabaseKeyIndex) -> bool {