From e51ca704f5cde1f98ad129a1250f42265ff80fa7 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 30 May 2025 11:20:07 +0200 Subject: [PATCH 01/26] Set `validate_final` in `execute` after removing the last cycle head --- src/function/execute.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/function/execute.rs b/src/function/execute.rs index 7b4dae966..952aa2613 100644 --- a/src/function/execute.rs +++ b/src/function/execute.rs @@ -235,6 +235,11 @@ where "{database_key_index:?}: execute: fixpoint iteration has a final value" ); revisions.cycle_heads.remove(&database_key_index); + + if revisions.cycle_heads.is_empty() { + // If there are no more cycle heads, we can mark this as verified. + revisions.verified_final.store(true, Ordering::Relaxed); + } } tracing::debug!("{database_key_index:?}: execute: result.revisions = {revisions:#?}"); From f573178ab83944db76de867df3e1423da7c26623 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sun, 25 May 2025 14:14:45 +0200 Subject: [PATCH 02/26] Add runaway query repro --- tests/parallel/cycle_nested_deep.rs | 120 ++++++++++++++++++++++++++++ tests/parallel/main.rs | 1 + 2 files changed, 121 insertions(+) create mode 100644 tests/parallel/cycle_nested_deep.rs diff --git a/tests/parallel/cycle_nested_deep.rs b/tests/parallel/cycle_nested_deep.rs new file mode 100644 index 000000000..2422e1aca --- /dev/null +++ b/tests/parallel/cycle_nested_deep.rs @@ -0,0 +1,120 @@ +//! Test a nested-cycle scenario across three threads: +//! +//! ```text +//! Thread T1 Thread T2 Thread T3 +//! --------- --------- --------- +//! | | | +//! v | | +//! query_a() | | +//! ^ | v | +//! | +------------> query_b() | +//! | ^ | v +//! | | +------------> query_c() +//! | | | +//! +------------------+--------------------+ +//! +//! ``` +use crate::sync::thread; +use crate::{Knobs, KnobsDatabase}; + +use salsa::CycleRecoveryAction; + +#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy, salsa::Update)] +struct CycleValue(u32); + +const MIN: CycleValue = CycleValue(0); +const MAX: CycleValue = CycleValue(3); + +// Signal 1: T1 has entered `query_a` +// Signal 2: T2 has entered `query_b` +// Signal 3: T3 has entered `query_c` + +#[salsa::tracked(cycle_fn=cycle_fn, cycle_initial=initial)] +fn query_a(db: &dyn KnobsDatabase) -> CycleValue { + query_b(db) +} + +#[salsa::tracked(cycle_fn=cycle_fn, cycle_initial=initial)] +fn query_b(db: &dyn KnobsDatabase) -> CycleValue { + let c_value = query_c(db); + CycleValue(c_value.0 + 1).min(MAX) +} + +#[salsa::tracked(cycle_fn=cycle_fn, cycle_initial=initial)] +fn query_c(db: &dyn KnobsDatabase) -> CycleValue { + let d_value = query_d(db); + let e_value = query_e(db); + let b_value = query_b(db); + let a_value = query_a(db); + + CycleValue(d_value.0.max(e_value.0).max(b_value.0).max(a_value.0)) +} + +#[salsa::tracked(cycle_fn=cycle_fn, cycle_initial=initial)] +fn query_d(db: &dyn KnobsDatabase) -> CycleValue { + query_c(db) +} + +#[salsa::tracked] +fn query_e(db: &dyn KnobsDatabase) -> CycleValue { + query_c(db) +} + +fn cycle_fn( + _db: &dyn KnobsDatabase, + _value: &CycleValue, + _count: u32, +) -> CycleRecoveryAction { + CycleRecoveryAction::Iterate +} + +fn initial(_db: &dyn KnobsDatabase) -> CycleValue { + MIN +} + +#[test_log::test] +fn the_test() { + shuttle::replay( + || { + let db_t1 = Knobs::default(); + let db_t2 = db_t1.clone(); + let db_t3 = db_t1.clone(); + + let t1 = thread::spawn(move || query_a(&db_t1)); + let t2 = thread::spawn(move || query_d(&db_t2)); + let t3 = thread::spawn(move || query_e(&db_t3)); + + let r_t1 = t1.join().unwrap(); + let r_t2 = t2.join().unwrap(); + let r_t3 = t3.join().unwrap(); + + assert_eq!((r_t1, r_t2, r_t3), (MAX, MAX, MAX)); + }, + " + 9102b312f9d1f2f7a3aeeef5ee01000000000000802449922449922449922449922449922449 + 9224499224499224499224499224499224499224499224499224499224499224499224499224 + 4992244992244992244992244992244992244992244992244992244992244992244992244992 + 2449922449922449922449922449922449922449922449922449922449922449922449922449 + 9224499224499224499224499224499224499224499224499224499224499224499224499224 + 4992244992244992244992244992244992244992244992244992244992244992244992244992 + 2449922449922449922449922449922449922449922449922449922449922449922449922449 + 9224499224499224499224499224499224499224499224499224499224499224499224499224 + 4992244992244992244992244992244992244992244992244992244992244992244992244992 + 2449922449922449922449922449922449922449922449922449922449922449922449922449 + 9224499224499224499224499224499224499224499224499224499224499224499224499224 + 4992244992244992244992244992244992244992244992244992244992244992244992244992 + 2449922449922449922449922449922449922449922449922449922449922449922449922449 + 9224499224499224499224499224499224499224499224499224499224499224499224499224 + 4992244992244992244992244992244992244992244992244992244992244992244992244992 + 2449922449922449922449922449922449922449922449922449922449922449922449922449 + 9224499224499224499224499224499224499224499224499224499224499224499224499224 + 4992244992244992244992244992244992244992244992244992244992244992244992244992 + 2449922449922449922449922449922449922449922449922449922449922449922449922449 + 9224499224499224499224499224499224499224499224499224499224499224499224499224 + 4992244992244992244992244992244992244992244992244992244992244992244992244992 + 2449922449922449922449922449922449922449922449922449922449922449922449922449 + 9224499224499224499224499224499224499224499224499224499224812449922449922449 + 922449922461dbb66ddbb66ddbb66ddbb66ddbb66ddb3600 + ", + ); +} diff --git a/tests/parallel/main.rs b/tests/parallel/main.rs index bd9d56580..0ab2e52b7 100644 --- a/tests/parallel/main.rs +++ b/tests/parallel/main.rs @@ -4,6 +4,7 @@ mod signal; mod cycle_a_t1_b_t2; mod cycle_a_t1_b_t2_fallback; mod cycle_ab_peeping_c; +mod cycle_nested_deep; mod cycle_nested_three_threads; mod cycle_panic; mod cycle_provisional_depending_on_itself; From d7ef838a656ee3a75917a2a08ba304cc16e715b9 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Mon, 26 May 2025 18:46:30 +0200 Subject: [PATCH 03/26] Add tracing --- src/function/memo.rs | 1 + src/runtime.rs | 5 ++ tests/parallel/cycle_nested_deep.rs | 71 +++++++++++++++++------------ 3 files changed, 49 insertions(+), 28 deletions(-) diff --git a/src/function/memo.rs b/src/function/memo.rs index 367baddf9..304d3bf5a 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -171,6 +171,7 @@ impl Memo { // There's a new memo available for the cycle head; fetch our own // updated memo and see if it's still provisional or if the cycle // has resolved. + tracing::trace!("Dependent cycle head {head_index:?} has been released (there's a new memo)"); retry = true; } else { // We hit a cycle blocking on the cycle head; this means it's in diff --git a/src/runtime.rs b/src/runtime.rs index 9d76119e6..294bfd162 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -181,6 +181,7 @@ impl Runtime { let dg = self.dependency_graph.lock(); if dg.depends_on(other_id, thread_id) { + tracing::debug!("block_on: cycle detected for {database_key:?} in thread {thread_id:?} on {other_id:?}"); return BlockResult::Cycle; } @@ -191,6 +192,10 @@ impl Runtime { }) }); + tracing::debug!( + "block_on: thread {thread_id:?} is blocking on {database_key:?} in thread {other_id:?}" + ); + let result = DependencyGraph::block_on(dg, thread_id, database_key, other_id, query_mutex_guard); diff --git a/tests/parallel/cycle_nested_deep.rs b/tests/parallel/cycle_nested_deep.rs index 2422e1aca..cab5526cc 100644 --- a/tests/parallel/cycle_nested_deep.rs +++ b/tests/parallel/cycle_nested_deep.rs @@ -80,9 +80,22 @@ fn the_test() { let db_t2 = db_t1.clone(); let db_t3 = db_t1.clone(); - let t1 = thread::spawn(move || query_a(&db_t1)); - let t2 = thread::spawn(move || query_d(&db_t2)); - let t3 = thread::spawn(move || query_e(&db_t3)); + let t1 = thread::spawn(move || + { + let _span = + tracing::debug_span!("t1", thread_id = ?thread::current().id()).entered(); + query_a(&db_t1) + }); + let t2 = thread::spawn(move || { + let _span = + tracing::debug_span!("t2", thread_id = ?thread::current().id()).entered(); + query_d(&db_t2) + }); + let t3 = thread::spawn(move || { + let _span = + tracing::debug_span!("t3", thread_id = ?thread::current().id()).entered(); + query_e(&db_t3) + }); let r_t1 = t1.join().unwrap(); let r_t2 = t2.join().unwrap(); @@ -90,31 +103,33 @@ fn the_test() { assert_eq!((r_t1, r_t2, r_t3), (MAX, MAX, MAX)); }, + " - 9102b312f9d1f2f7a3aeeef5ee01000000000000802449922449922449922449922449922449 - 9224499224499224499224499224499224499224499224499224499224499224499224499224 - 4992244992244992244992244992244992244992244992244992244992244992244992244992 - 2449922449922449922449922449922449922449922449922449922449922449922449922449 - 9224499224499224499224499224499224499224499224499224499224499224499224499224 - 4992244992244992244992244992244992244992244992244992244992244992244992244992 - 2449922449922449922449922449922449922449922449922449922449922449922449922449 - 9224499224499224499224499224499224499224499224499224499224499224499224499224 - 4992244992244992244992244992244992244992244992244992244992244992244992244992 - 2449922449922449922449922449922449922449922449922449922449922449922449922449 - 9224499224499224499224499224499224499224499224499224499224499224499224499224 - 4992244992244992244992244992244992244992244992244992244992244992244992244992 - 2449922449922449922449922449922449922449922449922449922449922449922449922449 - 9224499224499224499224499224499224499224499224499224499224499224499224499224 - 4992244992244992244992244992244992244992244992244992244992244992244992244992 - 2449922449922449922449922449922449922449922449922449922449922449922449922449 - 9224499224499224499224499224499224499224499224499224499224499224499224499224 - 4992244992244992244992244992244992244992244992244992244992244992244992244992 - 2449922449922449922449922449922449922449922449922449922449922449922449922449 - 9224499224499224499224499224499224499224499224499224499224499224499224499224 - 4992244992244992244992244992244992244992244992244992244992244992244992244992 - 2449922449922449922449922449922449922449922449922449922449922449922449922449 - 9224499224499224499224499224499224499224499224499224499224812449922449922449 - 922449922461dbb66ddbb66ddbb66ddbb66ddbb66ddb3600 - ", +9102fb12d0e0f3bef2bbecafd901000000000000802449922449922449922449922449922449 +9224499224499224499224499224499224499224499224499224499224499224499224499224 +4992244992244992244992244992244992244992244992244992244992244992244992244992 +2449922449922449922449922449922449922449922449922449922449922449922449922449 +9224499224499224499224499224499224499224499224499224499224499224499224499224 +4992244992244992244992244992244992244992244992244992244992244992244992244992 +2449922449922449922449922449922449922449922449922449922449922449922449922449 +9224499224499224499224499224499224499224499224499224499224499224499224499224 +4992244992244992244992244992244992244992244992244992244992244992244992244992 +2449922449922449922449922449922449922449922449922449922449922449922449922449 +9224499224499224499224499224499224499224499224499224499224499224499224499224 +4992244992244992244992244992244992244992244992244992244992244992244992244992 +2449922449922449922449922449922449922449922449922449922449922449922449922449 +9224499224499224499224499224499224499224499224499224499224499224499224499224 +4992244992244992244992244992244992244992244992244992244992244992244992244992 +2449922449922449922449922449922449922449922449922449922449922449922449922449 +9224499224499224499224499224499224499224499224499224499224499224499224499224 +4992244992244992244992244992244992244992244992244992244992244992244992244992 +2449922449922449922449922449922449922449922449922449922449922449922449922449 +9224499224499224499224499224499224499224499224499224499224499224499224499224 +4992244992244992244992244992244992244992244992244992244992244992244992244992 +2449922449922449922449922449922449922449922449922449922449922449922449922449 +9224499224499224499224499224499224499224499224499224499224499224499224499224 +499224499224499224499224499224499224812449922449922449922449922461dbb66ddbb6 +6ddbb66ddbb66ddbb66ddb3600 +" ); } From 60d9617c3d05ee7d69c1e911aa73a3809550e789 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 27 May 2025 09:59:47 +0200 Subject: [PATCH 04/26] Fix part 1 --- src/function.rs | 11 ++++++++--- tests/parallel/cycle_nested_deep.rs | 16 ++++++++++------ 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/function.rs b/src/function.rs index a577d33ba..8c73cc737 100644 --- a/src/function.rs +++ b/src/function.rs @@ -247,9 +247,14 @@ where let is_provisional = self .get_memo_from_table_for(zalsa, input, self.memo_ingredient_index(zalsa, input)) .is_some_and(|memo| { - memo.cycle_heads() - .into_iter() - .any(|head| head.database_key_index == self.database_key_index(input)) + match memo.cycle_heads().iter().as_slice() { + [] => false, + [head] => head.database_key_index == self.database_key_index(input), + // If there are multiple cycle heads, we assume that the memo is still provisional. + // This indicates that this memo is part of an outer cycle that should be awaited first. + [..] => true, + } + }); if is_provisional { CycleHeadKind::Provisional diff --git a/tests/parallel/cycle_nested_deep.rs b/tests/parallel/cycle_nested_deep.rs index cab5526cc..3dfabe9ad 100644 --- a/tests/parallel/cycle_nested_deep.rs +++ b/tests/parallel/cycle_nested_deep.rs @@ -55,7 +55,7 @@ fn query_d(db: &dyn KnobsDatabase) -> CycleValue { query_c(db) } -#[salsa::tracked] +#[salsa::tracked(cycle_fn=cycle_fn, cycle_initial=initial)] fn query_e(db: &dyn KnobsDatabase) -> CycleValue { query_c(db) } @@ -81,19 +81,23 @@ fn the_test() { let db_t3 = db_t1.clone(); let t1 = thread::spawn(move || - { - let _span = - tracing::debug_span!("t1", thread_id = ?thread::current().id()).entered(); - query_a(&db_t1) - }); + { + let _span = + tracing::debug_span!("t1", thread_id = ?thread::current().id()).entered(); + let result = query_a(&db_t1); + db_t1.signal(1); + result + }); let t2 = thread::spawn(move || { let _span = tracing::debug_span!("t2", thread_id = ?thread::current().id()).entered(); + db_t2.wait_for(1); query_d(&db_t2) }); let t3 = thread::spawn(move || { let _span = tracing::debug_span!("t3", thread_id = ?thread::current().id()).entered(); + db_t3.wait_for(1); query_e(&db_t3) }); From 6a1181dc2264b92c2073bf795d31cdcfbd3c8ec9 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 27 May 2025 13:56:54 +0200 Subject: [PATCH 05/26] Fix `cycle_head_kinds` to always return provisional for memos that aren't verified final (They should be validated by `validate_same_iteration` or wait for the cycle head --- src/function.rs | 15 +---- tests/parallel/cycle_nested_deep.rs | 96 +++++++++++++++++------------ 2 files changed, 61 insertions(+), 50 deletions(-) diff --git a/src/function.rs b/src/function.rs index 8c73cc737..ebc22b0f6 100644 --- a/src/function.rs +++ b/src/function.rs @@ -1,8 +1,8 @@ +pub(crate) use maybe_changed_after::VerifyResult; use std::any::Any; use std::fmt; use std::ptr::NonNull; - -pub(crate) use maybe_changed_after::VerifyResult; +use std::sync::atomic::Ordering; use crate::accumulator::accumulated_map::{AccumulatedMap, InputAccumulatedValues}; use crate::cycle::{CycleHeadKind, CycleHeads, CycleRecoveryAction, CycleRecoveryStrategy}; @@ -246,16 +246,7 @@ where fn cycle_head_kind(&self, zalsa: &Zalsa, input: Id) -> CycleHeadKind { let is_provisional = self .get_memo_from_table_for(zalsa, input, self.memo_ingredient_index(zalsa, input)) - .is_some_and(|memo| { - match memo.cycle_heads().iter().as_slice() { - [] => false, - [head] => head.database_key_index == self.database_key_index(input), - // If there are multiple cycle heads, we assume that the memo is still provisional. - // This indicates that this memo is part of an outer cycle that should be awaited first. - [..] => true, - } - - }); + .is_some_and(|memo| !memo.revisions.verified_final.load(Ordering::Relaxed)); if is_provisional { CycleHeadKind::Provisional } else if C::CYCLE_STRATEGY == CycleRecoveryStrategy::FallbackImmediate { diff --git a/tests/parallel/cycle_nested_deep.rs b/tests/parallel/cycle_nested_deep.rs index 3dfabe9ad..4a6832feb 100644 --- a/tests/parallel/cycle_nested_deep.rs +++ b/tests/parallel/cycle_nested_deep.rs @@ -74,20 +74,21 @@ fn initial(_db: &dyn KnobsDatabase) -> CycleValue { #[test_log::test] fn the_test() { - shuttle::replay( + // shuttle::replay( + crate::sync::check( || { + tracing::info!("New run"); let db_t1 = Knobs::default(); let db_t2 = db_t1.clone(); let db_t3 = db_t1.clone(); - let t1 = thread::spawn(move || - { - let _span = - tracing::debug_span!("t1", thread_id = ?thread::current().id()).entered(); - let result = query_a(&db_t1); - db_t1.signal(1); - result - }); + let t1 = thread::spawn(move || { + let _span = + tracing::debug_span!("t1", thread_id = ?thread::current().id()).entered(); + let result = query_a(&db_t1); + db_t1.signal(1); + result + }); let t2 = thread::spawn(move || { let _span = tracing::debug_span!("t2", thread_id = ?thread::current().id()).entered(); @@ -106,34 +107,53 @@ fn the_test() { let r_t3 = t3.join().unwrap(); assert_eq!((r_t1, r_t2, r_t3), (MAX, MAX, MAX)); - }, - - " -9102fb12d0e0f3bef2bbecafd901000000000000802449922449922449922449922449922449 -9224499224499224499224499224499224499224499224499224499224499224499224499224 -4992244992244992244992244992244992244992244992244992244992244992244992244992 -2449922449922449922449922449922449922449922449922449922449922449922449922449 -9224499224499224499224499224499224499224499224499224499224499224499224499224 -4992244992244992244992244992244992244992244992244992244992244992244992244992 -2449922449922449922449922449922449922449922449922449922449922449922449922449 -9224499224499224499224499224499224499224499224499224499224499224499224499224 -4992244992244992244992244992244992244992244992244992244992244992244992244992 -2449922449922449922449922449922449922449922449922449922449922449922449922449 -9224499224499224499224499224499224499224499224499224499224499224499224499224 -4992244992244992244992244992244992244992244992244992244992244992244992244992 -2449922449922449922449922449922449922449922449922449922449922449922449922449 -9224499224499224499224499224499224499224499224499224499224499224499224499224 -4992244992244992244992244992244992244992244992244992244992244992244992244992 -2449922449922449922449922449922449922449922449922449922449922449922449922449 -9224499224499224499224499224499224499224499224499224499224499224499224499224 -4992244992244992244992244992244992244992244992244992244992244992244992244992 -2449922449922449922449922449922449922449922449922449922449922449922449922449 -9224499224499224499224499224499224499224499224499224499224499224499224499224 -4992244992244992244992244992244992244992244992244992244992244992244992244992 -2449922449922449922449922449922449922449922449922449922449922449922449922449 -9224499224499224499224499224499224499224499224499224499224499224499224499224 -499224499224499224499224499224499224812449922449922449922449922461dbb66ddbb6 -6ddbb66ddbb66ddbb66ddb3600 -" + + tracing::info!("Complete"); + }, // , +// " +// 9102ac21f5a392c8f88cc0a27300000000004092244992244992244992244992244992244992 +// 2449922449922449922449922449922449922449922449922449922449922449922449922449 +// 9224499224499224491248c23664dbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66d +// dbb66ddbb66d9324499224499224499224499224499224499224499224499224494a92244992 +// 2449922449922449922449922449922449922449922449922449922449922449922449922449 +// 9224499224499224499224499224499224499224499224499224499224499224499224499224 +// 499224499224499224499224499224499224499224c91629dbb66ddbb66d922449922449d9b6 +// 6ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddb364992942449922449924892 +// 244992244992244992244992244992244992b429dab66ddbb66d4b922449922449b46ddbb66d +// dbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb6 +// 6ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddb +// b66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddb2449922449922449922449922449922449 +// 922449922449922449d2b66ddbb66ddbb66ddbb66ddbb66ddbb66ddb962c4992244992244992 +// 2449922449922449b66ddbb64d49922449922449922449922449922449922449922449922449 +// 922449922449924892244992244992244992246ddbb624499224499224499224499224499224 +// 4992244992244992244992244992244992244992244992244992244992244992244992244992 +// 2449922449922449922449922449922449922449922449922449922449922449922449922449 +// 9224499224499224499224499224499224499224499224499224499224499224499224499224 +// 4992244992244992244992244992244992244992244992244992244992244992244992244992 +// 2449922449922449922449922449922449922449922449922449922449922449922449922449 +// 9224499224499224499224499224499224499224499224499224499224499224499224499224 +// 4992244992244992244992244992244992244992244992244992244992244992244992244992 +// 2449922449922449922449922449922449922449922449922449922449922449922449922449 +// 9224499224499224499224499224499224499224499224499224499224499224499224499224 +// 4992244992244992244992244992244992244992244992244992244992244992244992244992 +// 244992244992244992244992244992244992489124499224498a244992244992244992244992 +// 2449922449922449922449922449922449922449922449922449529224499224492249922449 +// 9224499224499224499224499224499224499224499224499224499224499224499224499224 +// 4992244992244992244992244992244992244992244992244992244992244992244992244992 +// 2449922449922449922449922449922449922449922449922449922449922449922449922449 +// 9224499224499224499224499224499224499224499224499224499224499224499224499224 +// 4992244992244992244992244992244992244992244992244992244992244992244992244992 +// 2449922449922449922449922449922449922449922449922449922449922449922449922449 +// 9224499224499224499224499224499224499224499224499224499224499224499224499224 +// 4992244992244992244992244992244992244992244992244992244992244992244992244992 +// 2449922449922449922449922449922449922449922449922449922449922449922449922449 +// 9224499224499224499224499224499224499224499224499224499224499224499224499224 +// 4992244992244992244992244992244992244992244992244992244992244992244992244992 +// 2449922449922449922449922449922449922449922449922449922449922449922449922449 +// 9224499224499224499224499224499224499224499224499224499224499224499224499224 +// 4992244992244992244992244992244992244992244992244992244992244992244992244992 +// 2449922449922449922449922449922449922449922449922449922449922449922449922449 +// 922449922449922449922449922449922409 +// " ); } From 88b7350efb05009f55a4cfd66c6ce1723567ab84 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Tue, 27 May 2025 18:38:02 +0200 Subject: [PATCH 06/26] Fix cycle error --- src/function/fetch.rs | 29 +++++++++- src/function/memo.rs | 29 ++++++++++ tests/parallel/cycle_nested_deep.rs | 90 ++++++++++++++--------------- 3 files changed, 102 insertions(+), 46 deletions(-) diff --git a/src/function/fetch.rs b/src/function/fetch.rs index b855a4128..8da911eeb 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -180,7 +180,34 @@ where }; // Now that we've claimed the item, check again to see if there's a "hot" value. - let opt_old_memo = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index); + let mut opt_old_memo = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index); + + // If this is a provisional memo from the same revision. Await all cycle heads because they could be + // running on a different thread. + if let Some(mut old_memo) = opt_old_memo { + if old_memo.value.is_some() + && old_memo.may_be_provisional() + && old_memo.verified_at.load() == zalsa.current_revision() + { + opt_old_memo = loop { + old_memo.await_heads(zalsa, self.database_key_index(id)); + + let new_old = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index); + match new_old { + None => unreachable!("Expected memo to be present"), + // If the new memo is the same as the old, then this means that this is still the "latest" memo for this + Some(new_old) if std::ptr::eq(new_old, old_memo) => { + break Some(new_old); + } + Some(new_old) => { + tracing::debug!("Provisional memo has been updated by another thread while waiting for its cycle heads"); + old_memo = new_old; + } + } + }; + } + } + if let Some(old_memo) = opt_old_memo { if old_memo.value.is_some() { let mut cycle_heads = CycleHeads::default(); diff --git a/src/function/memo.rs b/src/function/memo.rs index 304d3bf5a..421fe74bb 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -201,6 +201,35 @@ impl Memo { } } + #[inline(always)] + pub(super) fn await_heads(&self, zalsa: &Zalsa, database_key_index: DatabaseKeyIndex) { + for head in &self.revisions.cycle_heads { + let head_index = head.database_key_index; + + if database_key_index == head_index { + continue; + } + + let ingredient = zalsa.lookup_ingredient(head_index.ingredient_index()); + let cycle_head_kind = ingredient.cycle_head_kind(zalsa, head_index.key_index()); + + if matches!( + cycle_head_kind, + CycleHeadKind::NotProvisional | CycleHeadKind::FallbackImmediate + ) { + // This cycle is already finalized, so we don't need to wait on it; + // keep looping through cycle heads. + tracing::trace!("Dependent cycle head {head_index:?} has been finalized."); + } else if ingredient.wait_for(zalsa, head_index.key_index()) { + tracing::trace!("Dependent cycle head {head_index:?} has been released"); + } else { + // We hit a cycle blocking on the cycle head; this means it's in + // our own active query stack and we are responsible to resolve the + // cycle + } + } + } + /// Cycle heads that should be propagated to dependent queries. #[inline(always)] pub(super) fn cycle_heads(&self) -> &CycleHeads { diff --git a/tests/parallel/cycle_nested_deep.rs b/tests/parallel/cycle_nested_deep.rs index 4a6832feb..499865e4d 100644 --- a/tests/parallel/cycle_nested_deep.rs +++ b/tests/parallel/cycle_nested_deep.rs @@ -110,50 +110,50 @@ fn the_test() { tracing::info!("Complete"); }, // , -// " -// 9102ac21f5a392c8f88cc0a27300000000004092244992244992244992244992244992244992 -// 2449922449922449922449922449922449922449922449922449922449922449922449922449 -// 9224499224499224491248c23664dbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66d -// dbb66ddbb66d9324499224499224499224499224499224499224499224499224494a92244992 -// 2449922449922449922449922449922449922449922449922449922449922449922449922449 -// 9224499224499224499224499224499224499224499224499224499224499224499224499224 -// 499224499224499224499224499224499224499224c91629dbb66ddbb66d922449922449d9b6 -// 6ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddb364992942449922449924892 -// 244992244992244992244992244992244992b429dab66ddbb66d4b922449922449b46ddbb66d -// dbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb6 -// 6ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddb -// b66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddb2449922449922449922449922449922449 -// 922449922449922449d2b66ddbb66ddbb66ddbb66ddbb66ddbb66ddb962c4992244992244992 -// 2449922449922449b66ddbb64d49922449922449922449922449922449922449922449922449 -// 922449922449924892244992244992244992246ddbb624499224499224499224499224499224 -// 4992244992244992244992244992244992244992244992244992244992244992244992244992 -// 2449922449922449922449922449922449922449922449922449922449922449922449922449 -// 9224499224499224499224499224499224499224499224499224499224499224499224499224 -// 4992244992244992244992244992244992244992244992244992244992244992244992244992 -// 2449922449922449922449922449922449922449922449922449922449922449922449922449 -// 9224499224499224499224499224499224499224499224499224499224499224499224499224 -// 4992244992244992244992244992244992244992244992244992244992244992244992244992 -// 2449922449922449922449922449922449922449922449922449922449922449922449922449 -// 9224499224499224499224499224499224499224499224499224499224499224499224499224 -// 4992244992244992244992244992244992244992244992244992244992244992244992244992 -// 244992244992244992244992244992244992489124499224498a244992244992244992244992 -// 2449922449922449922449922449922449922449922449922449529224499224492249922449 -// 9224499224499224499224499224499224499224499224499224499224499224499224499224 -// 4992244992244992244992244992244992244992244992244992244992244992244992244992 -// 2449922449922449922449922449922449922449922449922449922449922449922449922449 -// 9224499224499224499224499224499224499224499224499224499224499224499224499224 -// 4992244992244992244992244992244992244992244992244992244992244992244992244992 -// 2449922449922449922449922449922449922449922449922449922449922449922449922449 -// 9224499224499224499224499224499224499224499224499224499224499224499224499224 -// 4992244992244992244992244992244992244992244992244992244992244992244992244992 -// 2449922449922449922449922449922449922449922449922449922449922449922449922449 -// 9224499224499224499224499224499224499224499224499224499224499224499224499224 -// 4992244992244992244992244992244992244992244992244992244992244992244992244992 -// 2449922449922449922449922449922449922449922449922449922449922449922449922449 -// 9224499224499224499224499224499224499224499224499224499224499224499224499224 -// 4992244992244992244992244992244992244992244992244992244992244992244992244992 -// 2449922449922449922449922449922449922449922449922449922449922449922449922449 -// 922449922449922449922449922449922409 -// " + // " + // 9102ac21f5a392c8f88cc0a27300000000004092244992244992244992244992244992244992 + // 2449922449922449922449922449922449922449922449922449922449922449922449922449 + // 9224499224499224491248c23664dbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66d + // dbb66ddbb66d9324499224499224499224499224499224499224499224499224494a92244992 + // 2449922449922449922449922449922449922449922449922449922449922449922449922449 + // 9224499224499224499224499224499224499224499224499224499224499224499224499224 + // 499224499224499224499224499224499224499224c91629dbb66ddbb66d922449922449d9b6 + // 6ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddb364992942449922449924892 + // 244992244992244992244992244992244992b429dab66ddbb66d4b922449922449b46ddbb66d + // dbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb6 + // 6ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddb + // b66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddb2449922449922449922449922449922449 + // 922449922449922449d2b66ddbb66ddbb66ddbb66ddbb66ddbb66ddb962c4992244992244992 + // 2449922449922449b66ddbb64d49922449922449922449922449922449922449922449922449 + // 922449922449924892244992244992244992246ddbb624499224499224499224499224499224 + // 4992244992244992244992244992244992244992244992244992244992244992244992244992 + // 2449922449922449922449922449922449922449922449922449922449922449922449922449 + // 9224499224499224499224499224499224499224499224499224499224499224499224499224 + // 4992244992244992244992244992244992244992244992244992244992244992244992244992 + // 2449922449922449922449922449922449922449922449922449922449922449922449922449 + // 9224499224499224499224499224499224499224499224499224499224499224499224499224 + // 4992244992244992244992244992244992244992244992244992244992244992244992244992 + // 2449922449922449922449922449922449922449922449922449922449922449922449922449 + // 9224499224499224499224499224499224499224499224499224499224499224499224499224 + // 4992244992244992244992244992244992244992244992244992244992244992244992244992 + // 244992244992244992244992244992244992489124499224498a244992244992244992244992 + // 2449922449922449922449922449922449922449922449922449529224499224492249922449 + // 9224499224499224499224499224499224499224499224499224499224499224499224499224 + // 4992244992244992244992244992244992244992244992244992244992244992244992244992 + // 2449922449922449922449922449922449922449922449922449922449922449922449922449 + // 9224499224499224499224499224499224499224499224499224499224499224499224499224 + // 4992244992244992244992244992244992244992244992244992244992244992244992244992 + // 2449922449922449922449922449922449922449922449922449922449922449922449922449 + // 9224499224499224499224499224499224499224499224499224499224499224499224499224 + // 4992244992244992244992244992244992244992244992244992244992244992244992244992 + // 2449922449922449922449922449922449922449922449922449922449922449922449922449 + // 9224499224499224499224499224499224499224499224499224499224499224499224499224 + // 4992244992244992244992244992244992244992244992244992244992244992244992244992 + // 2449922449922449922449922449922449922449922449922449922449922449922449922449 + // 9224499224499224499224499224499224499224499224499224499224499224499224499224 + // 4992244992244992244992244992244992244992244992244992244992244992244992244992 + // 2449922449922449922449922449922449922449922449922449922449922449922449922449 + // 922449922449922449922449922449922409 + // " ); } From 358a76fa31a23f459523b1560fe11e8cedf1d6d3 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 29 May 2025 08:27:40 +0200 Subject: [PATCH 07/26] Documentation --- src/function/fetch.rs | 50 ++++++----- tests/parallel/cycle_nested_deep.rs | 133 +++++++--------------------- 2 files changed, 59 insertions(+), 124 deletions(-) diff --git a/src/function/fetch.rs b/src/function/fetch.rs index 8da911eeb..483b77287 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -113,11 +113,11 @@ where id: Id, memo_ingredient_index: MemoIngredientIndex, ) -> Option<&'db Memo>> { + let database_key_index = self.database_key_index(id); // Try to claim this query: if someone else has claimed it already, go back and start again. let _claim_guard = match self.sync_table.try_claim(zalsa, id) { ClaimResult::Retry => return None, ClaimResult::Cycle => { - let database_key_index = self.database_key_index(id); // check if there's a provisional value for this query // Note we don't `validate_may_be_provisional` the memo here as we want to reuse an // existing provisional memo if it exists @@ -180,31 +180,33 @@ where }; // Now that we've claimed the item, check again to see if there's a "hot" value. - let mut opt_old_memo = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index); - - // If this is a provisional memo from the same revision. Await all cycle heads because they could be - // running on a different thread. - if let Some(mut old_memo) = opt_old_memo { + let opt_old_memo = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index); + + // If this is a provisional memo from the same revision, await all its cycle heads because + // we need to ensure that only one thread is iterating on a cycle at a given time. + // For example, if we have a nested cycle like so: + // ``` + // a -> b -> c -> b + // -> a + // + // d -> b + // ``` + // thread 1 calls `a` and `a` completes the inner cycle `b -> c` but hasn't finished the outer cycle `a` yet. + // thread 2 now calls `b`. We don't want that thread 2 iterates `b` while thread 1 is iterating `a` at the same time + // because it can result in thread b overriding provisional memos that thread a has accessed already and still relies upon. + // + // By waiting, we ensure that thread 1 completes a (based on a provisional value for `b`) and `b` + // becomes the new outer cycle, which thread 2 drives to completion. + if let Some(old_memo) = opt_old_memo { if old_memo.value.is_some() && old_memo.may_be_provisional() && old_memo.verified_at.load() == zalsa.current_revision() { - opt_old_memo = loop { - old_memo.await_heads(zalsa, self.database_key_index(id)); - - let new_old = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index); - match new_old { - None => unreachable!("Expected memo to be present"), - // If the new memo is the same as the old, then this means that this is still the "latest" memo for this - Some(new_old) if std::ptr::eq(new_old, old_memo) => { - break Some(new_old); - } - Some(new_old) => { - tracing::debug!("Provisional memo has been updated by another thread while waiting for its cycle heads"); - old_memo = new_old; - } - } - }; + old_memo.await_heads(zalsa, database_key_index); + + // It's possible that one of the cycle heads replaced the memo for this ingredient + // with fixpoint initial. We ignore that memo because we know it's only a temporary memo + // and instead continue with the memo we already have (acquired). } } @@ -215,7 +217,7 @@ where db, zalsa, old_memo, - self.database_key_index(id), + database_key_index, &mut cycle_heads, ) { if cycle_heads.is_empty() { @@ -229,7 +231,7 @@ where let memo = self.execute( db, - db.zalsa_local().push_query(self.database_key_index(id), 0), + db.zalsa_local().push_query(database_key_index, 0), opt_old_memo, ); diff --git a/tests/parallel/cycle_nested_deep.rs b/tests/parallel/cycle_nested_deep.rs index 499865e4d..e1e3b2531 100644 --- a/tests/parallel/cycle_nested_deep.rs +++ b/tests/parallel/cycle_nested_deep.rs @@ -1,19 +1,10 @@ -//! Test a nested-cycle scenario across three threads: +//! Test a deeply nested-cycle scenario across multiple threads. //! -//! ```text -//! Thread T1 Thread T2 Thread T3 -//! --------- --------- --------- -//! | | | -//! v | | -//! query_a() | | -//! ^ | v | -//! | +------------> query_b() | -//! | ^ | v -//! | | +------------> query_c() -//! | | | -//! +------------------+--------------------+ +//! The trick is that different threads call into the same cycle from different entry queries. //! -//! ``` +//! * Thread 1: `a` -> b -> c (which calls back into d, e, b, a) +//! * Thread 2: `d` -> `c` +//! * Thread 3: `e` -> `c` use crate::sync::thread; use crate::{Knobs, KnobsDatabase}; @@ -25,10 +16,6 @@ struct CycleValue(u32); const MIN: CycleValue = CycleValue(0); const MAX: CycleValue = CycleValue(3); -// Signal 1: T1 has entered `query_a` -// Signal 2: T2 has entered `query_b` -// Signal 3: T3 has entered `query_c` - #[salsa::tracked(cycle_fn=cycle_fn, cycle_initial=initial)] fn query_a(db: &dyn KnobsDatabase) -> CycleValue { query_b(db) @@ -74,86 +61,32 @@ fn initial(_db: &dyn KnobsDatabase) -> CycleValue { #[test_log::test] fn the_test() { - // shuttle::replay( - crate::sync::check( - || { - tracing::info!("New run"); - let db_t1 = Knobs::default(); - let db_t2 = db_t1.clone(); - let db_t3 = db_t1.clone(); - - let t1 = thread::spawn(move || { - let _span = - tracing::debug_span!("t1", thread_id = ?thread::current().id()).entered(); - let result = query_a(&db_t1); - db_t1.signal(1); - result - }); - let t2 = thread::spawn(move || { - let _span = - tracing::debug_span!("t2", thread_id = ?thread::current().id()).entered(); - db_t2.wait_for(1); - query_d(&db_t2) - }); - let t3 = thread::spawn(move || { - let _span = - tracing::debug_span!("t3", thread_id = ?thread::current().id()).entered(); - db_t3.wait_for(1); - query_e(&db_t3) - }); - - let r_t1 = t1.join().unwrap(); - let r_t2 = t2.join().unwrap(); - let r_t3 = t3.join().unwrap(); - - assert_eq!((r_t1, r_t2, r_t3), (MAX, MAX, MAX)); - - tracing::info!("Complete"); - }, // , - // " - // 9102ac21f5a392c8f88cc0a27300000000004092244992244992244992244992244992244992 - // 2449922449922449922449922449922449922449922449922449922449922449922449922449 - // 9224499224499224491248c23664dbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66d - // dbb66ddbb66d9324499224499224499224499224499224499224499224499224494a92244992 - // 2449922449922449922449922449922449922449922449922449922449922449922449922449 - // 9224499224499224499224499224499224499224499224499224499224499224499224499224 - // 499224499224499224499224499224499224499224c91629dbb66ddbb66d922449922449d9b6 - // 6ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddb364992942449922449924892 - // 244992244992244992244992244992244992b429dab66ddbb66d4b922449922449b46ddbb66d - // dbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb6 - // 6ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddb - // b66ddbb66ddbb66ddbb66ddbb66ddbb66ddbb66ddb2449922449922449922449922449922449 - // 922449922449922449d2b66ddbb66ddbb66ddbb66ddbb66ddbb66ddb962c4992244992244992 - // 2449922449922449b66ddbb64d49922449922449922449922449922449922449922449922449 - // 922449922449924892244992244992244992246ddbb624499224499224499224499224499224 - // 4992244992244992244992244992244992244992244992244992244992244992244992244992 - // 2449922449922449922449922449922449922449922449922449922449922449922449922449 - // 9224499224499224499224499224499224499224499224499224499224499224499224499224 - // 4992244992244992244992244992244992244992244992244992244992244992244992244992 - // 2449922449922449922449922449922449922449922449922449922449922449922449922449 - // 9224499224499224499224499224499224499224499224499224499224499224499224499224 - // 4992244992244992244992244992244992244992244992244992244992244992244992244992 - // 2449922449922449922449922449922449922449922449922449922449922449922449922449 - // 9224499224499224499224499224499224499224499224499224499224499224499224499224 - // 4992244992244992244992244992244992244992244992244992244992244992244992244992 - // 244992244992244992244992244992244992489124499224498a244992244992244992244992 - // 2449922449922449922449922449922449922449922449922449529224499224492249922449 - // 9224499224499224499224499224499224499224499224499224499224499224499224499224 - // 4992244992244992244992244992244992244992244992244992244992244992244992244992 - // 2449922449922449922449922449922449922449922449922449922449922449922449922449 - // 9224499224499224499224499224499224499224499224499224499224499224499224499224 - // 4992244992244992244992244992244992244992244992244992244992244992244992244992 - // 2449922449922449922449922449922449922449922449922449922449922449922449922449 - // 9224499224499224499224499224499224499224499224499224499224499224499224499224 - // 4992244992244992244992244992244992244992244992244992244992244992244992244992 - // 2449922449922449922449922449922449922449922449922449922449922449922449922449 - // 9224499224499224499224499224499224499224499224499224499224499224499224499224 - // 4992244992244992244992244992244992244992244992244992244992244992244992244992 - // 2449922449922449922449922449922449922449922449922449922449922449922449922449 - // 9224499224499224499224499224499224499224499224499224499224499224499224499224 - // 4992244992244992244992244992244992244992244992244992244992244992244992244992 - // 2449922449922449922449922449922449922449922449922449922449922449922449922449 - // 922449922449922449922449922449922409 - // " - ); + crate::sync::check(|| { + let db_t1 = Knobs::default(); + let db_t2 = db_t1.clone(); + let db_t3 = db_t1.clone(); + + let t1 = thread::spawn(move || { + let _span = tracing::debug_span!("t1", thread_id = ?thread::current().id()).entered(); + let result = query_a(&db_t1); + db_t1.signal(1); + result + }); + let t2 = thread::spawn(move || { + let _span = tracing::debug_span!("t2", thread_id = ?thread::current().id()).entered(); + db_t2.wait_for(1); + query_d(&db_t2) + }); + let t3 = thread::spawn(move || { + let _span = tracing::debug_span!("t3", thread_id = ?thread::current().id()).entered(); + db_t3.wait_for(1); + query_e(&db_t3) + }); + + let r_t1 = t1.join().unwrap(); + let r_t2 = t2.join().unwrap(); + let r_t3 = t3.join().unwrap(); + + assert_eq!((r_t1, r_t2, r_t3), (MAX, MAX, MAX)); + }); } From ae3c7a8cf2b8b5e49a18c12d336da644d6010f73 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 29 May 2025 10:19:16 +0200 Subject: [PATCH 08/26] Fix await for queries depending on initial value --- src/function.rs | 10 +- src/function/fetch.rs | 12 +-- src/function/memo.rs | 107 ++++++++------------- src/ingredient.rs | 11 ++- tests/parallel/cycle_a_t1_b_t2_fallback.rs | 5 +- tests/parallel/cycle_nested_deep.rs | 16 ++- 6 files changed, 78 insertions(+), 83 deletions(-) diff --git a/src/function.rs b/src/function.rs index ebc22b0f6..7d9e7993e 100644 --- a/src/function.rs +++ b/src/function.rs @@ -5,7 +5,9 @@ use std::ptr::NonNull; use std::sync::atomic::Ordering; use crate::accumulator::accumulated_map::{AccumulatedMap, InputAccumulatedValues}; -use crate::cycle::{CycleHeadKind, CycleHeads, CycleRecoveryAction, CycleRecoveryStrategy}; +use crate::cycle::{ + empty_cycle_heads, CycleHeadKind, CycleHeads, CycleRecoveryAction, CycleRecoveryStrategy, +}; use crate::function::delete::DeletedEntries; use crate::function::sync::{ClaimResult, SyncTable}; use crate::ingredient::Ingredient; @@ -256,6 +258,12 @@ where } } + fn cycle_heads<'db>(&self, zalsa: &'db Zalsa, input: Id) -> &'db CycleHeads { + self.get_memo_from_table_for(zalsa, input, self.memo_ingredient_index(zalsa, input)) + .map(|memo| memo.cycle_heads()) + .unwrap_or(empty_cycle_heads()) + } + /// Attempts to claim `key_index`, returning `false` if a cycle occurs. fn wait_for(&self, zalsa: &Zalsa, key_index: Id) -> bool { match self.sync_table.try_claim(zalsa, key_index) { diff --git a/src/function/fetch.rs b/src/function/fetch.rs index 483b77287..b86abf0d2 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -202,7 +202,7 @@ where && old_memo.may_be_provisional() && old_memo.verified_at.load() == zalsa.current_revision() { - old_memo.await_heads(zalsa, database_key_index); + old_memo.await_heads(zalsa); // It's possible that one of the cycle heads replaced the memo for this ingredient // with fixpoint initial. We ignore that memo because we know it's only a temporary memo @@ -213,13 +213,9 @@ where if let Some(old_memo) = opt_old_memo { if old_memo.value.is_some() { let mut cycle_heads = CycleHeads::default(); - if let VerifyResult::Unchanged(_) = self.deep_verify_memo( - db, - zalsa, - old_memo, - database_key_index, - &mut cycle_heads, - ) { + if let VerifyResult::Unchanged(_) = + self.deep_verify_memo(db, zalsa, old_memo, database_key_index, &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/memo.rs b/src/function/memo.rs index 421fe74bb..3c5607f3b 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -5,6 +5,7 @@ use std::ptr::NonNull; use crate::cycle::{empty_cycle_heads, CycleHeadKind, CycleHeads}; use crate::function::{Configuration, IngredientImpl}; +use crate::hash::FxHashSet; use crate::key::DatabaseKeyIndex; use crate::revision::AtomicRevision; use crate::sync::atomic::Ordering; @@ -142,73 +143,27 @@ impl Memo { return false; }; - return provisional_retry_cold(zalsa, database_key_index, &self.revisions.cycle_heads); - - #[inline(never)] - fn provisional_retry_cold( - zalsa: &Zalsa, - database_key_index: DatabaseKeyIndex, - cycle_heads: &CycleHeads, - ) -> bool { - let mut retry = false; - let mut hit_cycle = false; - - for head in cycle_heads { - let head_index = head.database_key_index; - - let ingredient = zalsa.lookup_ingredient(head_index.ingredient_index()); - let cycle_head_kind = ingredient.cycle_head_kind(zalsa, head_index.key_index()); - if matches!( - cycle_head_kind, - CycleHeadKind::NotProvisional | CycleHeadKind::FallbackImmediate - ) { - // This cycle is already finalized, so we don't need to wait on it; - // keep looping through cycle heads. - retry = true; - tracing::trace!("Dependent cycle head {head_index:?} has been finalized."); - } else if ingredient.wait_for(zalsa, head_index.key_index()) { - tracing::trace!("Dependent cycle head {head_index:?} has been released (there's a new memo)"); - // There's a new memo available for the cycle head; fetch our own - // updated memo and see if it's still provisional or if the cycle - // has resolved. - tracing::trace!("Dependent cycle head {head_index:?} has been released (there's a new memo)"); - retry = true; - } else { - // We hit a cycle blocking on the cycle head; this means it's in - // our own active query stack and we are responsible to resolve the - // cycle, so go ahead and return the provisional memo. - tracing::debug!( - "Waiting for {head_index:?} results in a cycle, return {database_key_index:?} once all other cycle heads completed to allow the outer cycle to make progress." - ); - hit_cycle = true; - } - } - - // If `retry` is `true`, all our cycle heads (barring ourself) are complete; re-fetch - // and we should get a non-provisional memo. If we get here and `retry` is still - // `false`, we have no cycle heads other than ourself, so we are a provisional value of - // the cycle head (either initial value, or from a later iteration) and should be - // returned to caller to allow fixpoint iteration to proceed. (All cases in the loop - // above other than "cycle head is self" are either terminal or set `retry`.) - if hit_cycle { - false - } else if retry { - tracing::debug!("Retrying {database_key_index:?}"); - true - } else { - false - } + if self.await_heads(zalsa) { + false + } else { + tracing::debug!( + "Retrying provisional memo {database_key_index:?} after awaiting cycle heads." + ); + true } } - #[inline(always)] - pub(super) fn await_heads(&self, zalsa: &Zalsa, database_key_index: DatabaseKeyIndex) { - for head in &self.revisions.cycle_heads { - let head_index = head.database_key_index; + /// Awaits all cycle heads (recursively) that this memo depends on. + /// + /// Returns `true` if awaiting the cycle heads resulted in a cycle. + pub(super) fn await_heads(&self, zalsa: &Zalsa) -> bool { + let mut hit_cycle = false; - if database_key_index == head_index { - continue; - } + let mut visited = FxHashSet::default(); + let mut queue: Vec<_> = self.revisions.cycle_heads.iter().collect(); + + while let Some(head) = queue.pop() { + let head_index = head.database_key_index; let ingredient = zalsa.lookup_ingredient(head_index.ingredient_index()); let cycle_head_kind = ingredient.cycle_head_kind(zalsa, head_index.key_index()); @@ -221,13 +176,35 @@ impl Memo { // keep looping through cycle heads. tracing::trace!("Dependent cycle head {head_index:?} has been finalized."); } else if ingredient.wait_for(zalsa, head_index.key_index()) { - tracing::trace!("Dependent cycle head {head_index:?} has been released"); + // There's a new memo available for the cycle head; fetch our own + // updated memo and see if it's still provisional or if the cycle + // has resolved. + tracing::trace!( + "Dependent cycle head {head_index:?} has been released (there's a new memo)" + ); + // Recursively wait for all cycle heads that this head depends on. + // This is normally not necessary, because cycle heads are transitively added + // as query dependencies (they aggregate). The exception to this are queries + // that depend on a fixpoint initial value. We don't know all the dependencies of + // the query yet, so they can't be carried over. We only know them once the cycle + // completes but the cycle heads of the queries don't get updated. + // Because of that, recurse here to collect all cycle heads. + queue.extend( + ingredient + .cycle_heads(zalsa, head_index.key_index()) + .iter() + .filter(|head| visited.insert(head.database_key_index)), + ); } else { // We hit a cycle blocking on the cycle head; this means it's in // our own active query stack and we are responsible to resolve the - // cycle + // cycle, so go ahead and return the provisional memo. + tracing::debug!("Waiting for {head_index:?} results in a cycle"); + hit_cycle = true; } } + + hit_cycle } /// Cycle heads that should be propagated to dependent queries. diff --git a/src/ingredient.rs b/src/ingredient.rs index dcddebd9c..abc3f0c16 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, CycleHeads, CycleRecoveryStrategy}; +use crate::cycle::{empty_cycle_heads, CycleHeadKind, CycleHeads, CycleRecoveryStrategy}; use crate::function::VerifyResult; use crate::plumbing::IngredientIndices; use crate::sync::Arc; @@ -67,14 +67,17 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync { ) -> VerifyResult; /// Is the value for `input` in this ingredient a cycle head that is still provisional? - /// - /// In the case of nested cycles, we are not asking here whether the value is provisional due - /// to the outer cycle being unresolved, only whether its own cycle remains provisional. fn cycle_head_kind(&self, zalsa: &Zalsa, input: Id) -> CycleHeadKind { _ = (zalsa, input); CycleHeadKind::NotProvisional } + /// Returns the cycle heads for this ingredient. + fn cycle_heads<'db>(&self, zalsa: &'db Zalsa, input: Id) -> &'db CycleHeads { + _ = (zalsa, input); + empty_cycle_heads() + } + /// Invoked when the current thread needs to wait for a result for the given `key_index`. /// /// A return value of `true` indicates that a result is now available. A return value of diff --git a/tests/parallel/cycle_a_t1_b_t2_fallback.rs b/tests/parallel/cycle_a_t1_b_t2_fallback.rs index a052e6db1..3ade92f50 100644 --- a/tests/parallel/cycle_a_t1_b_t2_fallback.rs +++ b/tests/parallel/cycle_a_t1_b_t2_fallback.rs @@ -12,7 +12,7 @@ //! +--------------------+ //! ``` use crate::sync::thread; -use crate::{Knobs, KnobsDatabase}; +use crate::KnobsDatabase; const FALLBACK_A: u32 = 0b01; const FALLBACK_B: u32 = 0b10; @@ -53,6 +53,9 @@ fn cycle_result_b(_db: &dyn KnobsDatabase) -> u32 { #[test_log::test] #[cfg(not(feature = "shuttle"))] // This test is currently failing. fn the_test() { + use crate::sync::thread; + use crate::Knobs; + crate::sync::check(|| { let db_t1 = Knobs::default(); let db_t2 = db_t1.clone(); diff --git a/tests/parallel/cycle_nested_deep.rs b/tests/parallel/cycle_nested_deep.rs index e1e3b2531..7b7c2f42a 100644 --- a/tests/parallel/cycle_nested_deep.rs +++ b/tests/parallel/cycle_nested_deep.rs @@ -3,8 +3,9 @@ //! The trick is that different threads call into the same cycle from different entry queries. //! //! * Thread 1: `a` -> b -> c (which calls back into d, e, b, a) -//! * Thread 2: `d` -> `c` -//! * Thread 3: `e` -> `c` +//! * Thread 2: `b` +//! * Thread 3: `d` -> `c` +//! * Thread 4: `e` -> `c` use crate::sync::thread; use crate::{Knobs, KnobsDatabase}; @@ -65,6 +66,7 @@ fn the_test() { let db_t1 = Knobs::default(); let db_t2 = db_t1.clone(); let db_t3 = db_t1.clone(); + let db_t4 = db_t1.clone(); let t1 = thread::spawn(move || { let _span = tracing::debug_span!("t1", thread_id = ?thread::current().id()).entered(); @@ -73,11 +75,16 @@ fn the_test() { result }); let t2 = thread::spawn(move || { + let _span = tracing::debug_span!("t4", thread_id = ?thread::current().id()).entered(); + db_t4.wait_for(1); + query_b(&db_t4) + }); + let t3 = thread::spawn(move || { let _span = tracing::debug_span!("t2", thread_id = ?thread::current().id()).entered(); db_t2.wait_for(1); query_d(&db_t2) }); - let t3 = thread::spawn(move || { + let t4 = thread::spawn(move || { let _span = tracing::debug_span!("t3", thread_id = ?thread::current().id()).entered(); db_t3.wait_for(1); query_e(&db_t3) @@ -86,7 +93,8 @@ fn the_test() { let r_t1 = t1.join().unwrap(); let r_t2 = t2.join().unwrap(); let r_t3 = t3.join().unwrap(); + let r_t4 = t4.join().unwrap(); - assert_eq!((r_t1, r_t2, r_t3), (MAX, MAX, MAX)); + assert_eq!((r_t1, r_t2, r_t3, r_t4), (MAX, MAX, MAX, MAX)); }); } From ffd437cbf16f9886b887b185e303af4907c5602f Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 29 May 2025 11:55:45 +0200 Subject: [PATCH 09/26] correctly initialize queued --- src/cycle.rs | 2 +- src/function.rs | 2 +- src/function/maybe_changed_after.rs | 2 +- src/function/memo.rs | 53 +++++++++++++--------- src/ingredient.rs | 2 +- tests/parallel/cycle_a_t1_b_t2_fallback.rs | 1 - 6 files changed, 36 insertions(+), 26 deletions(-) diff --git a/src/cycle.rs b/src/cycle.rs index 6de6c4180..2b56abfd3 100644 --- a/src/cycle.rs +++ b/src/cycle.rs @@ -268,6 +268,6 @@ pub(crate) fn empty_cycle_heads() -> &'static CycleHeads { #[derive(Debug, PartialEq, Eq)] pub enum CycleHeadKind { Provisional, - NotProvisional, + Final, FallbackImmediate, } diff --git a/src/function.rs b/src/function.rs index 7d9e7993e..7d5162232 100644 --- a/src/function.rs +++ b/src/function.rs @@ -254,7 +254,7 @@ where } else if C::CYCLE_STRATEGY == CycleRecoveryStrategy::FallbackImmediate { CycleHeadKind::FallbackImmediate } else { - CycleHeadKind::NotProvisional + CycleHeadKind::Final } } diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index 9986f6852..28f6486fc 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -263,7 +263,7 @@ where .cycle_head_kind(zalsa, cycle_head.database_key_index.key_index()); match kind { CycleHeadKind::Provisional => return false, - CycleHeadKind::NotProvisional => { + CycleHeadKind::Final => { // FIXME: We can ignore this, I just don't have a use-case for this. if C::CYCLE_STRATEGY == CycleRecoveryStrategy::FallbackImmediate { panic!("cannot mix `cycle_fn` and `cycle_result` in cycles") diff --git a/src/function/memo.rs b/src/function/memo.rs index 3c5607f3b..cbd3534f8 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -144,8 +144,13 @@ impl Memo { }; if self.await_heads(zalsa) { + // If we get here, we are a provisional value of + // the cycle head (either initial value, or from a later iteration) and should be + // returned to caller to allow fixpoint iteration to proceed. false } else { + // all our cycle heads are complete; re-fetch + // and we should get a non-provisional memo. tracing::debug!( "Retrying provisional memo {database_key_index:?} after awaiting cycle heads." ); @@ -156,10 +161,16 @@ impl Memo { /// Awaits all cycle heads (recursively) that this memo depends on. /// /// Returns `true` if awaiting the cycle heads resulted in a cycle. + #[inline(never)] pub(super) fn await_heads(&self, zalsa: &Zalsa) -> bool { let mut hit_cycle = false; - let mut visited = FxHashSet::default(); + let mut queued: FxHashSet<_> = self + .revisions + .cycle_heads + .iter() + .map(|head| head.database_key_index) + .collect(); let mut queue: Vec<_> = self.revisions.cycle_heads.iter().collect(); while let Some(head) = queue.pop() { @@ -170,38 +181,38 @@ impl Memo { if matches!( cycle_head_kind, - CycleHeadKind::NotProvisional | CycleHeadKind::FallbackImmediate + CycleHeadKind::Final | CycleHeadKind::FallbackImmediate ) { // This cycle is already finalized, so we don't need to wait on it; // keep looping through cycle heads. tracing::trace!("Dependent cycle head {head_index:?} has been finalized."); + continue; } else if ingredient.wait_for(zalsa, head_index.key_index()) { - // There's a new memo available for the cycle head; fetch our own - // updated memo and see if it's still provisional or if the cycle - // has resolved. + // There's a new memo available for the cycle head (may still be provisional). tracing::trace!( "Dependent cycle head {head_index:?} has been released (there's a new memo)" ); - // Recursively wait for all cycle heads that this head depends on. - // This is normally not necessary, because cycle heads are transitively added - // as query dependencies (they aggregate). The exception to this are queries - // that depend on a fixpoint initial value. We don't know all the dependencies of - // the query yet, so they can't be carried over. We only know them once the cycle - // completes but the cycle heads of the queries don't get updated. - // Because of that, recurse here to collect all cycle heads. - queue.extend( - ingredient - .cycle_heads(zalsa, head_index.key_index()) - .iter() - .filter(|head| visited.insert(head.database_key_index)), - ); } else { - // We hit a cycle blocking on the cycle head; this means it's in - // our own active query stack and we are responsible to resolve the - // cycle, so go ahead and return the provisional memo. + // We hit a cycle blocking on the cycle head; this means this query actively + // participates in the cycle and some other query is blocked on this thread. tracing::debug!("Waiting for {head_index:?} results in a cycle"); hit_cycle = true; } + + // Recursively wait for all cycle heads that this head depends on. + // This is normally not necessary, because cycle heads are transitively added + // as query dependencies (they aggregate). The exception to this are queries + // that depend on a fixpoint initial value. They only depend on the fixpoint initial + // value but not on its dependencies because they aren't known yet. They're only known + // once the cycle completes but the cycle heads of the queries don't get updated. + // Because of that, recurse here to collect all cycle heads. + // This also ensures that if a query added new cycle heads, that they are awaited too. + queue.extend( + ingredient + .cycle_heads(zalsa, head_index.key_index()) + .iter() + .filter(|head| queued.insert(head.database_key_index)), + ); } hit_cycle diff --git a/src/ingredient.rs b/src/ingredient.rs index abc3f0c16..ef61c9d1d 100644 --- a/src/ingredient.rs +++ b/src/ingredient.rs @@ -69,7 +69,7 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync { /// Is the value for `input` in this ingredient a cycle head that is still provisional? fn cycle_head_kind(&self, zalsa: &Zalsa, input: Id) -> CycleHeadKind { _ = (zalsa, input); - CycleHeadKind::NotProvisional + CycleHeadKind::Final } /// Returns the cycle heads for this ingredient. diff --git a/tests/parallel/cycle_a_t1_b_t2_fallback.rs b/tests/parallel/cycle_a_t1_b_t2_fallback.rs index 3ade92f50..917eace4f 100644 --- a/tests/parallel/cycle_a_t1_b_t2_fallback.rs +++ b/tests/parallel/cycle_a_t1_b_t2_fallback.rs @@ -11,7 +11,6 @@ //! | | //! +--------------------+ //! ``` -use crate::sync::thread; use crate::KnobsDatabase; const FALLBACK_A: u32 = 0b01; From 873a9fb6e7992a6ab421c3b8c554648a786a048c Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 29 May 2025 12:26:43 +0200 Subject: [PATCH 10/26] Cleanup --- src/function/memo.rs | 82 +++++++++++++++++++++++--------------------- 1 file changed, 42 insertions(+), 40 deletions(-) diff --git a/src/function/memo.rs b/src/function/memo.rs index cbd3534f8..7871627cc 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -163,56 +163,58 @@ impl Memo { /// Returns `true` if awaiting the cycle heads resulted in a cycle. #[inline(never)] pub(super) fn await_heads(&self, zalsa: &Zalsa) -> bool { - let mut hit_cycle = false; - - let mut queued: FxHashSet<_> = self + let mut queue: Vec<_> = self .revisions .cycle_heads .iter() .map(|head| head.database_key_index) .collect(); - let mut queue: Vec<_> = self.revisions.cycle_heads.iter().collect(); + let mut queued: FxHashSet<_> = queue.iter().copied().collect(); + let mut hit_cycle = false; while let Some(head) = queue.pop() { - let head_index = head.database_key_index; + let ingredient = zalsa.lookup_ingredient(head.ingredient_index()); + let cycle_head_kind = ingredient.cycle_head_kind(zalsa, head.key_index()); - let ingredient = zalsa.lookup_ingredient(head_index.ingredient_index()); - let cycle_head_kind = ingredient.cycle_head_kind(zalsa, head_index.key_index()); + match cycle_head_kind { + CycleHeadKind::Final | CycleHeadKind::FallbackImmediate => { + // This cycle is already finalized, so we don't need to wait on it; + // keep looping through cycle heads. + tracing::trace!("Dependent cycle head {head:?} has been finalized."); + } + CycleHeadKind::Provisional => { + if ingredient.wait_for(zalsa, head.key_index()) { + // There's a new memo available for the cycle head (may still be provisional). + tracing::trace!( + "Dependent cycle head {head:?} has been released (there's a new memo)" + ); - if matches!( - cycle_head_kind, - CycleHeadKind::Final | CycleHeadKind::FallbackImmediate - ) { - // This cycle is already finalized, so we don't need to wait on it; - // keep looping through cycle heads. - tracing::trace!("Dependent cycle head {head_index:?} has been finalized."); - continue; - } else if ingredient.wait_for(zalsa, head_index.key_index()) { - // There's a new memo available for the cycle head (may still be provisional). - tracing::trace!( - "Dependent cycle head {head_index:?} has been released (there's a new memo)" - ); - } else { - // We hit a cycle blocking on the cycle head; this means this query actively - // participates in the cycle and some other query is blocked on this thread. - tracing::debug!("Waiting for {head_index:?} results in a cycle"); - hit_cycle = true; + // Recursively wait for all cycle heads that this head depends on. + // This is normally not necessary, because cycle heads are transitively added + // as query dependencies (they aggregate). The exception to this are queries + // that depend on a fixpoint initial value. They only depend on the fixpoint initial + // value but not on its dependencies because they aren't known yet. They're only known + // once the cycle completes but the cycle heads of the queries don't get updated. + // Because of that, recurse here to collect all cycle heads. + // This also ensures that if a query added new cycle heads, that they are awaited too. + // IMPORTANT: It's critical that we get the cycle head from the latest memo + // here, in case the memo has become part of another cycle (we need to add that too!) + // I recommend running `cycle_nested_deep` with 1000 iterations if you make any changes here. + queue.extend( + ingredient + .cycle_heads(zalsa, head.key_index()) + .iter() + .map(|head| head.database_key_index) + .filter(|head| queued.insert(*head)), + ); + } else { + // We hit a cycle blocking on the cycle head; this means this query actively + // participates in the cycle and some other query is blocked on this thread. + tracing::debug!("Waiting for {head:?} results in a cycle"); + hit_cycle = true; + } + } } - - // Recursively wait for all cycle heads that this head depends on. - // This is normally not necessary, because cycle heads are transitively added - // as query dependencies (they aggregate). The exception to this are queries - // that depend on a fixpoint initial value. They only depend on the fixpoint initial - // value but not on its dependencies because they aren't known yet. They're only known - // once the cycle completes but the cycle heads of the queries don't get updated. - // Because of that, recurse here to collect all cycle heads. - // This also ensures that if a query added new cycle heads, that they are awaited too. - queue.extend( - ingredient - .cycle_heads(zalsa, head_index.key_index()) - .iter() - .filter(|head| queued.insert(head.database_key_index)), - ); } hit_cycle From 558c65bf5f0aa292a8840bd7dcf11e8d63b0236d Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 29 May 2025 12:42:07 +0200 Subject: [PATCH 11/26] Short circuit if entire query runs on single thread --- src/function/accumulated.rs | 3 ++- src/function/fetch.rs | 21 +++++++++++------- src/function/maybe_changed_after.rs | 18 ++------------- src/function/memo.rs | 34 ++++++++++++++++++++++++++--- 4 files changed, 48 insertions(+), 28 deletions(-) diff --git a/src/function/accumulated.rs b/src/function/accumulated.rs index bd7ccd72a..2ef77a403 100644 --- a/src/function/accumulated.rs +++ b/src/function/accumulated.rs @@ -96,8 +96,9 @@ where db: &'db C::DbView, key: Id, ) -> (Option<&'db AccumulatedMap>, InputAccumulatedValues) { + let (zalsa, zalsa_local) = db.zalsas(); // NEXT STEP: stash and refactor `fetch` to return an `&Memo` so we can make this work - let memo = self.refresh_memo(db, db.zalsa(), key); + let memo = self.refresh_memo(db, zalsa, zalsa_local, key); ( memo.revisions.accumulated.as_deref(), memo.revisions.accumulated_inputs.load(), diff --git a/src/function/fetch.rs b/src/function/fetch.rs index b86abf0d2..58c294a8f 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -3,7 +3,7 @@ use crate::function::memo::Memo; use crate::function::sync::ClaimResult; use crate::function::{Configuration, IngredientImpl, VerifyResult}; use crate::zalsa::{MemoIngredientIndex, Zalsa, ZalsaDatabase}; -use crate::zalsa_local::QueryRevisions; +use crate::zalsa_local::{QueryRevisions, ZalsaLocal}; use crate::Id; impl IngredientImpl @@ -18,7 +18,7 @@ where #[cfg(debug_assertions)] let _span = tracing::debug_span!("fetch", query = ?database_key_index).entered(); - let memo = self.refresh_memo(db, zalsa, id); + let memo = self.refresh_memo(db, zalsa, zalsa_local, id); // SAFETY: We just refreshed the memo so it is guaranteed to contain a value now. let memo_value = unsafe { memo.value.as_ref().unwrap_unchecked() }; @@ -41,13 +41,16 @@ where &'db self, db: &'db C::DbView, zalsa: &'db Zalsa, + zalsa_local: &'db ZalsaLocal, id: Id, ) -> &'db Memo> { let memo_ingredient_index = self.memo_ingredient_index(zalsa, id); loop { if let Some(memo) = self .fetch_hot(zalsa, id, memo_ingredient_index) - .or_else(|| self.fetch_cold_with_retry(zalsa, db, id, memo_ingredient_index)) + .or_else(|| { + self.fetch_cold_with_retry(zalsa, zalsa_local, db, id, memo_ingredient_index) + }) { return memo; } @@ -84,11 +87,12 @@ where fn fetch_cold_with_retry<'db>( &'db self, zalsa: &'db Zalsa, + zalsa_local: &'db ZalsaLocal, db: &'db C::DbView, id: Id, memo_ingredient_index: MemoIngredientIndex, ) -> Option<&'db Memo>> { - let memo = self.fetch_cold(zalsa, db, id, memo_ingredient_index)?; + let memo = self.fetch_cold(zalsa, zalsa_local, db, id, memo_ingredient_index)?; // If we get back a provisional cycle memo, and it's provisional on any cycle heads // that are claimed by a different thread, we can't propagate the provisional memo @@ -98,7 +102,7 @@ where // That is only correct for fixpoint cycles, though: `FallbackImmediate` cycles // never have provisional entries. if C::CYCLE_STRATEGY == CycleRecoveryStrategy::FallbackImmediate - || !memo.provisional_retry(zalsa, self.database_key_index(id)) + || !memo.provisional_retry(zalsa, zalsa_local, self.database_key_index(id)) { Some(memo) } else { @@ -109,6 +113,7 @@ where fn fetch_cold<'db>( &'db self, zalsa: &'db Zalsa, + zalsa_local: &'db ZalsaLocal, db: &'db C::DbView, id: Id, memo_ingredient_index: MemoIngredientIndex, @@ -161,7 +166,7 @@ where tracing::debug!( "hit a `FallbackImmediate` cycle at {database_key_index:#?}" ); - let active_query = db.zalsa_local().push_query(database_key_index, 0); + let active_query = zalsa_local.push_query(database_key_index, 0); let fallback_value = C::cycle_initial(db, C::id_to_input(db, id)); let mut revisions = active_query.pop(); revisions.cycle_heads = CycleHeads::initial(database_key_index); @@ -202,7 +207,7 @@ where && old_memo.may_be_provisional() && old_memo.verified_at.load() == zalsa.current_revision() { - old_memo.await_heads(zalsa); + old_memo.await_heads(zalsa, zalsa_local); // It's possible that one of the cycle heads replaced the memo for this ingredient // with fixpoint initial. We ignore that memo because we know it's only a temporary memo @@ -227,7 +232,7 @@ where let memo = self.execute( db, - db.zalsa_local().push_query(database_key_index, 0), + zalsa_local.push_query(database_key_index, 0), opt_old_memo, ); diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index 28f6486fc..ee2e20f22 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -4,10 +4,9 @@ use crate::function::memo::Memo; use crate::function::sync::ClaimResult; use crate::function::{Configuration, IngredientImpl}; use crate::key::DatabaseKeyIndex; -use crate::plumbing::ZalsaLocal; use crate::sync::atomic::Ordering; use crate::zalsa::{MemoIngredientIndex, Zalsa, ZalsaDatabase}; -use crate::zalsa_local::{QueryEdge, QueryOriginRef}; +use crate::zalsa_local::{QueryEdge, QueryOriginRef, ZalsaLocal}; use crate::{AsDynDatabase as _, Id, Revision}; /// Result of memo validation. @@ -303,20 +302,7 @@ where memo = memo.tracing_debug() ); - let cycle_heads = &memo.revisions.cycle_heads; - if cycle_heads.is_empty() { - return true; - } - - zalsa_local.with_query_stack(|stack| { - cycle_heads.iter().all(|cycle_head| { - stack - .iter() - .rev() - .find(|query| query.database_key_index == cycle_head.database_key_index) - .is_some_and(|query| query.iteration_count() == cycle_head.iteration_count) - }) - }) + memo.validate_same_iteration(zalsa_local) } /// VerifyResult::Unchanged if the memo's value and `changed_at` time is up-to-date in the diff --git a/src/function/memo.rs b/src/function/memo.rs index 7871627cc..c413bc51d 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -11,7 +11,7 @@ use crate::revision::AtomicRevision; use crate::sync::atomic::Ordering; use crate::table::memo::MemoTableWithTypesMut; use crate::zalsa::{MemoIngredientIndex, Zalsa}; -use crate::zalsa_local::{QueryOriginRef, QueryRevisions}; +use crate::zalsa_local::{QueryOriginRef, QueryRevisions, ZalsaLocal}; use crate::{Event, EventKind, Id, Revision}; impl IngredientImpl { @@ -133,6 +133,7 @@ impl Memo { pub(super) fn provisional_retry( &self, zalsa: &Zalsa, + zalsa_local: &ZalsaLocal, database_key_index: DatabaseKeyIndex, ) -> bool { if self.revisions.cycle_heads.is_empty() { @@ -143,7 +144,7 @@ impl Memo { return false; }; - if self.await_heads(zalsa) { + if self.await_heads(zalsa, zalsa_local) { // If we get here, we are a provisional value of // the cycle head (either initial value, or from a later iteration) and should be // returned to caller to allow fixpoint iteration to proceed. @@ -162,7 +163,14 @@ impl Memo { /// /// Returns `true` if awaiting the cycle heads resulted in a cycle. #[inline(never)] - pub(super) fn await_heads(&self, zalsa: &Zalsa) -> bool { + pub(super) fn await_heads(&self, zalsa: &Zalsa, zalsa_local: &ZalsaLocal) -> bool { + // The most common case is that the entire cycle is running in the same thread. + // If that's the case, short circuit and return `true` immediately. + if self.validate_same_iteration(zalsa_local) { + return true; + } + + // Otherwise, await all cycle heads, recursively. let mut queue: Vec<_> = self .revisions .cycle_heads @@ -220,6 +228,26 @@ impl Memo { hit_cycle } + /// If this is a provisional memo, validate that it was cached in the same iteration of the + /// same cycle(s) that we are still executing. If so, it is valid for reuse. This avoids + /// runaway re-execution of the same queries within a fixpoint iteration. + pub(super) fn validate_same_iteration(&self, zalsa_local: &ZalsaLocal) -> bool { + let cycle_heads = &self.revisions.cycle_heads; + if cycle_heads.is_empty() { + return true; + } + + zalsa_local.with_query_stack(|stack| { + cycle_heads.iter().all(|cycle_head| { + stack + .iter() + .rev() + .find(|query| query.database_key_index == cycle_head.database_key_index) + .is_some_and(|query| query.iteration_count() == cycle_head.iteration_count) + }) + }) + } + /// Cycle heads that should be propagated to dependent queries. #[inline(always)] pub(super) fn cycle_heads(&self) -> &CycleHeads { From b6a83baa5e23e247e5ea0584c8ab569634c7f647 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 29 May 2025 12:51:06 +0200 Subject: [PATCH 12/26] Move parallel code into its own method --- src/function/memo.rs | 93 ++++++++++++++++++++++---------------------- 1 file changed, 46 insertions(+), 47 deletions(-) diff --git a/src/function/memo.rs b/src/function/memo.rs index c413bc51d..2ae43563e 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -162,7 +162,6 @@ impl Memo { /// Awaits all cycle heads (recursively) that this memo depends on. /// /// Returns `true` if awaiting the cycle heads resulted in a cycle. - #[inline(never)] pub(super) fn await_heads(&self, zalsa: &Zalsa, zalsa_local: &ZalsaLocal) -> bool { // The most common case is that the entire cycle is running in the same thread. // If that's the case, short circuit and return `true` immediately. @@ -171,61 +170,61 @@ impl Memo { } // Otherwise, await all cycle heads, recursively. - let mut queue: Vec<_> = self - .revisions - .cycle_heads - .iter() - .map(|head| head.database_key_index) - .collect(); - let mut queued: FxHashSet<_> = queue.iter().copied().collect(); - let mut hit_cycle = false; + return await_heads_cold(zalsa, self.cycle_heads()); - while let Some(head) = queue.pop() { - let ingredient = zalsa.lookup_ingredient(head.ingredient_index()); - let cycle_head_kind = ingredient.cycle_head_kind(zalsa, head.key_index()); + #[inline(never)] + fn await_heads_cold(zalsa: &Zalsa, heads: &CycleHeads) -> bool { + let mut queue: Vec<_> = heads.iter().map(|head| head.database_key_index).collect(); + let mut queued: FxHashSet<_> = queue.iter().copied().collect(); + let mut hit_cycle = false; - match cycle_head_kind { - CycleHeadKind::Final | CycleHeadKind::FallbackImmediate => { - // This cycle is already finalized, so we don't need to wait on it; - // keep looping through cycle heads. - tracing::trace!("Dependent cycle head {head:?} has been finalized."); - } - CycleHeadKind::Provisional => { - if ingredient.wait_for(zalsa, head.key_index()) { - // There's a new memo available for the cycle head (may still be provisional). - tracing::trace!( + while let Some(head) = queue.pop() { + let ingredient = zalsa.lookup_ingredient(head.ingredient_index()); + let cycle_head_kind = ingredient.cycle_head_kind(zalsa, head.key_index()); + + match cycle_head_kind { + CycleHeadKind::Final | CycleHeadKind::FallbackImmediate => { + // This cycle is already finalized, so we don't need to wait on it; + // keep looping through cycle heads. + tracing::trace!("Dependent cycle head {head:?} has been finalized."); + } + CycleHeadKind::Provisional => { + if ingredient.wait_for(zalsa, head.key_index()) { + // There's a new memo available for the cycle head (may still be provisional). + tracing::trace!( "Dependent cycle head {head:?} has been released (there's a new memo)" ); - // Recursively wait for all cycle heads that this head depends on. - // This is normally not necessary, because cycle heads are transitively added - // as query dependencies (they aggregate). The exception to this are queries - // that depend on a fixpoint initial value. They only depend on the fixpoint initial - // value but not on its dependencies because they aren't known yet. They're only known - // once the cycle completes but the cycle heads of the queries don't get updated. - // Because of that, recurse here to collect all cycle heads. - // This also ensures that if a query added new cycle heads, that they are awaited too. - // IMPORTANT: It's critical that we get the cycle head from the latest memo - // here, in case the memo has become part of another cycle (we need to add that too!) - // I recommend running `cycle_nested_deep` with 1000 iterations if you make any changes here. - queue.extend( - ingredient - .cycle_heads(zalsa, head.key_index()) - .iter() - .map(|head| head.database_key_index) - .filter(|head| queued.insert(*head)), - ); - } else { - // We hit a cycle blocking on the cycle head; this means this query actively - // participates in the cycle and some other query is blocked on this thread. - tracing::debug!("Waiting for {head:?} results in a cycle"); - hit_cycle = true; + // Recursively wait for all cycle heads that this head depends on. + // This is normally not necessary, because cycle heads are transitively added + // as query dependencies (they aggregate). The exception to this are queries + // that depend on a fixpoint initial value. They only depend on the fixpoint initial + // value but not on its dependencies because they aren't known yet. They're only known + // once the cycle completes but the cycle heads of the queries don't get updated. + // Because of that, recurse here to collect all cycle heads. + // This also ensures that if a query added new cycle heads, that they are awaited too. + // IMPORTANT: It's critical that we get the cycle head from the latest memo + // here, in case the memo has become part of another cycle (we need to add that too!) + // I recommend running `cycle_nested_deep` with 1000 iterations if you make any changes here. + queue.extend( + ingredient + .cycle_heads(zalsa, head.key_index()) + .iter() + .map(|head| head.database_key_index) + .filter(|head| queued.insert(*head)), + ); + } else { + // We hit a cycle blocking on the cycle head; this means this query actively + // participates in the cycle and some other query is blocked on this thread. + tracing::debug!("Waiting for {head:?} results in a cycle"); + hit_cycle = true; + } } } } - } - hit_cycle + hit_cycle + } } /// If this is a provisional memo, validate that it was cached in the same iteration of the From 979f57a87c5209d994dd102b4ec0a23f3523f828 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 29 May 2025 14:06:15 +0200 Subject: [PATCH 13/26] Rename method, add self_key to queued --- src/function/fetch.rs | 2 +- src/function/memo.rs | 19 +++++++++++++------ 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/function/fetch.rs b/src/function/fetch.rs index 58c294a8f..a1dc515fb 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -207,7 +207,7 @@ where && old_memo.may_be_provisional() && old_memo.verified_at.load() == zalsa.current_revision() { - old_memo.await_heads(zalsa, zalsa_local); + old_memo.block_on_heads(zalsa, zalsa_local, database_key_index); // It's possible that one of the cycle heads replaced the memo for this ingredient // with fixpoint initial. We ignore that memo because we know it's only a temporary memo diff --git a/src/function/memo.rs b/src/function/memo.rs index 2ae43563e..fe52cbc2c 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -144,7 +144,7 @@ impl Memo { return false; }; - if self.await_heads(zalsa, zalsa_local) { + if self.block_on_heads(zalsa, zalsa_local, database_key_index) { // If we get here, we are a provisional value of // the cycle head (either initial value, or from a later iteration) and should be // returned to caller to allow fixpoint iteration to proceed. @@ -159,10 +159,15 @@ impl Memo { } } - /// Awaits all cycle heads (recursively) that this memo depends on. + /// Blocks on all cycle heads (recursively) that this memo depends on. /// /// Returns `true` if awaiting the cycle heads resulted in a cycle. - pub(super) fn await_heads(&self, zalsa: &Zalsa, zalsa_local: &ZalsaLocal) -> bool { + pub(super) fn block_on_heads( + &self, + zalsa: &Zalsa, + zalsa_local: &ZalsaLocal, + self_key: DatabaseKeyIndex, + ) -> bool { // The most common case is that the entire cycle is running in the same thread. // If that's the case, short circuit and return `true` immediately. if self.validate_same_iteration(zalsa_local) { @@ -170,12 +175,14 @@ impl Memo { } // Otherwise, await all cycle heads, recursively. - return await_heads_cold(zalsa, self.cycle_heads()); + return await_heads_cold(zalsa, self.cycle_heads(), self_key); #[inline(never)] - fn await_heads_cold(zalsa: &Zalsa, heads: &CycleHeads) -> bool { + fn await_heads_cold(zalsa: &Zalsa, heads: &CycleHeads, self_key: DatabaseKeyIndex) -> bool { let mut queue: Vec<_> = heads.iter().map(|head| head.database_key_index).collect(); - let mut queued: FxHashSet<_> = queue.iter().copied().collect(); + let mut queued: FxHashSet<_> = std::iter::once(self_key) + .chain(queue.iter().copied()) + .collect(); let mut hit_cycle = false; while let Some(head) = queue.pop() { From 8b4d69c6d0dc32e53ec00895f4c77381e7813b57 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 29 May 2025 14:34:51 +0200 Subject: [PATCH 14/26] Revert self-key changes --- src/function/fetch.rs | 5 ++++- src/function/memo.rs | 24 ++++++++++-------------- 2 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/function/fetch.rs b/src/function/fetch.rs index a1dc515fb..444594e25 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -207,7 +207,10 @@ where && old_memo.may_be_provisional() && old_memo.verified_at.load() == zalsa.current_revision() { - old_memo.block_on_heads(zalsa, zalsa_local, database_key_index); + // TODO(micha) 29th May 2025: It would be nice if `block_on_heads` would release + // and re-acquire the lock if it blocks on any other thread. That would allow any other thread + // to claim this query (e.g. thread a in the example above) and make progress on it. + old_memo.block_on_heads(zalsa, zalsa_local); // It's possible that one of the cycle heads replaced the memo for this ingredient // with fixpoint initial. We ignore that memo because we know it's only a temporary memo diff --git a/src/function/memo.rs b/src/function/memo.rs index fe52cbc2c..0e545d010 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -144,7 +144,7 @@ impl Memo { return false; }; - if self.block_on_heads(zalsa, zalsa_local, database_key_index) { + if self.block_on_heads(zalsa, zalsa_local) { // If we get here, we are a provisional value of // the cycle head (either initial value, or from a later iteration) and should be // returned to caller to allow fixpoint iteration to proceed. @@ -162,12 +162,11 @@ impl Memo { /// Blocks on all cycle heads (recursively) that this memo depends on. /// /// Returns `true` if awaiting the cycle heads resulted in a cycle. - pub(super) fn block_on_heads( - &self, - zalsa: &Zalsa, - zalsa_local: &ZalsaLocal, - self_key: DatabaseKeyIndex, - ) -> bool { + #[inline(always)] + pub(super) fn block_on_heads(&self, zalsa: &Zalsa, zalsa_local: &ZalsaLocal) -> bool { + // IMPORTANT: If you make changes to this function, make sure to run `cycle_nested_deep` with + // shuttle with at least 10k iterations. + // The most common case is that the entire cycle is running in the same thread. // If that's the case, short circuit and return `true` immediately. if self.validate_same_iteration(zalsa_local) { @@ -175,14 +174,12 @@ impl Memo { } // Otherwise, await all cycle heads, recursively. - return await_heads_cold(zalsa, self.cycle_heads(), self_key); + return block_on_heads_cold(zalsa, self.cycle_heads()); #[inline(never)] - fn await_heads_cold(zalsa: &Zalsa, heads: &CycleHeads, self_key: DatabaseKeyIndex) -> bool { + fn block_on_heads_cold(zalsa: &Zalsa, heads: &CycleHeads) -> bool { let mut queue: Vec<_> = heads.iter().map(|head| head.database_key_index).collect(); - let mut queued: FxHashSet<_> = std::iter::once(self_key) - .chain(queue.iter().copied()) - .collect(); + let mut queued: FxHashSet<_> = queue.iter().copied().collect(); let mut hit_cycle = false; while let Some(head) = queue.pop() { @@ -211,8 +208,7 @@ impl Memo { // Because of that, recurse here to collect all cycle heads. // This also ensures that if a query added new cycle heads, that they are awaited too. // IMPORTANT: It's critical that we get the cycle head from the latest memo - // here, in case the memo has become part of another cycle (we need to add that too!) - // I recommend running `cycle_nested_deep` with 1000 iterations if you make any changes here. + // here, in case the memo has become part of another cycle (we need to block on that too!). queue.extend( ingredient .cycle_heads(zalsa, head.key_index()) From 3d9c30700662644d32b8184157b014c120b5cc17 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Thu, 29 May 2025 18:12:55 +0200 Subject: [PATCH 15/26] Move check *after* `deep_verify_memo` --- src/function/fetch.rs | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/function/fetch.rs b/src/function/fetch.rs index 444594e25..8ac3493a1 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -187,6 +187,21 @@ where // Now that we've claimed the item, check again to see if there's a "hot" value. 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() { + let mut cycle_heads = CycleHeads::default(); + if let VerifyResult::Unchanged(_) = + self.deep_verify_memo(db, zalsa, old_memo, database_key_index, &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. + return unsafe { Some(self.extend_memo_lifetime(old_memo)) }; + } + } + } + } + // If this is a provisional memo from the same revision, await all its cycle heads because // we need to ensure that only one thread is iterating on a cycle at a given time. // For example, if we have a nested cycle like so: @@ -218,21 +233,6 @@ where } } - if let Some(old_memo) = opt_old_memo { - if old_memo.value.is_some() { - let mut cycle_heads = CycleHeads::default(); - if let VerifyResult::Unchanged(_) = - self.deep_verify_memo(db, zalsa, old_memo, database_key_index, &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. - return unsafe { Some(self.extend_memo_lifetime(old_memo)) }; - } - } - } - } - let memo = self.execute( db, zalsa_local.push_query(database_key_index, 0), From ebb9c52e38fffb38accd226742ad2c42c554ebd6 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 30 May 2025 09:06:37 +0200 Subject: [PATCH 16/26] Add a test for a cycle with changing cycle heads --- .../parallel/cycle_nested_deep_conditional.rs | 110 ++++++++++++++++++ tests/parallel/main.rs | 1 + 2 files changed, 111 insertions(+) create mode 100644 tests/parallel/cycle_nested_deep_conditional.rs diff --git a/tests/parallel/cycle_nested_deep_conditional.rs b/tests/parallel/cycle_nested_deep_conditional.rs new file mode 100644 index 000000000..316612845 --- /dev/null +++ b/tests/parallel/cycle_nested_deep_conditional.rs @@ -0,0 +1,110 @@ +//! Test a deeply nested-cycle scenario where cycles have changing query dependencies. +//! +//! The trick is that different threads call into the same cycle from different entry queries and +//! the cycle heads change over different iterations +//! +//! * Thread 1: `a` -> b -> c +//! * Thread 2: `b` +//! * Thread 3: `d` -> `c` +//! * Thread 4: `e` -> `c` +//! +//! `c` calls: +//! * `d` and `a` in the first few iterations +//! * `d`, `b` and `e` in the last iterations +use crate::sync::thread; +use crate::{Knobs, KnobsDatabase}; + +use salsa::CycleRecoveryAction; + +#[derive(Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, Copy, salsa::Update)] +struct CycleValue(u32); + +const MIN: CycleValue = CycleValue(0); +const MAX: CycleValue = CycleValue(3); + +#[salsa::tracked(cycle_fn=cycle_fn, cycle_initial=initial)] +fn query_a(db: &dyn KnobsDatabase) -> CycleValue { + query_b(db) +} + +#[salsa::tracked(cycle_fn=cycle_fn, cycle_initial=initial)] +fn query_b(db: &dyn KnobsDatabase) -> CycleValue { + let c_value = query_c(db); + CycleValue(c_value.0 + 1).min(MAX) +} + +#[salsa::tracked(cycle_fn=cycle_fn, cycle_initial=initial)] +fn query_c(db: &dyn KnobsDatabase) -> CycleValue { + let d_value = query_d(db); + + if d_value > CycleValue(0) { + let e_value = query_e(db); + let b_value = query_b(db); + CycleValue(d_value.0.max(e_value.0).max(b_value.0)) + } else { + let a_value = query_a(db); + CycleValue(d_value.0.max(a_value.0)) + } +} + +#[salsa::tracked(cycle_fn=cycle_fn, cycle_initial=initial)] +fn query_d(db: &dyn KnobsDatabase) -> CycleValue { + query_c(db) +} + +#[salsa::tracked(cycle_fn=cycle_fn, cycle_initial=initial)] +fn query_e(db: &dyn KnobsDatabase) -> CycleValue { + query_c(db) +} + +fn cycle_fn( + _db: &dyn KnobsDatabase, + _value: &CycleValue, + _count: u32, +) -> CycleRecoveryAction { + CycleRecoveryAction::Iterate +} + +fn initial(_db: &dyn KnobsDatabase) -> CycleValue { + MIN +} + +#[test_log::test] +fn the_test() { + crate::sync::check(|| { + tracing::debug!("New run"); + let db_t1 = Knobs::default(); + let db_t2 = db_t1.clone(); + let db_t3 = db_t1.clone(); + let db_t4 = db_t1.clone(); + + let t1 = thread::spawn(move || { + let _span = tracing::debug_span!("t1", thread_id = ?thread::current().id()).entered(); + let result = query_a(&db_t1); + db_t1.signal(1); + result + }); + let t2 = thread::spawn(move || { + let _span = tracing::debug_span!("t4", thread_id = ?thread::current().id()).entered(); + db_t4.wait_for(1); + query_b(&db_t4) + }); + let t3 = thread::spawn(move || { + let _span = tracing::debug_span!("t2", thread_id = ?thread::current().id()).entered(); + db_t2.wait_for(1); + query_d(&db_t2) + }); + let t4 = thread::spawn(move || { + let _span = tracing::debug_span!("t3", thread_id = ?thread::current().id()).entered(); + db_t3.wait_for(1); + query_e(&db_t3) + }); + + let r_t1 = t1.join().unwrap(); + let r_t2 = t2.join().unwrap(); + let r_t3 = t3.join().unwrap(); + let r_t4 = t4.join().unwrap(); + + assert_eq!((r_t1, r_t2, r_t3, r_t4), (MAX, MAX, MAX, MAX)); + }); +} diff --git a/tests/parallel/main.rs b/tests/parallel/main.rs index 0ab2e52b7..da5e5e4a1 100644 --- a/tests/parallel/main.rs +++ b/tests/parallel/main.rs @@ -5,6 +5,7 @@ mod cycle_a_t1_b_t2; mod cycle_a_t1_b_t2_fallback; mod cycle_ab_peeping_c; mod cycle_nested_deep; +mod cycle_nested_deep_conditional; mod cycle_nested_three_threads; mod cycle_panic; mod cycle_provisional_depending_on_itself; From e888eb50af380ebf02982be5ec35d11f454be06f Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 30 May 2025 11:37:38 +0200 Subject: [PATCH 17/26] Short circuit more often --- src/function/memo.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/src/function/memo.rs b/src/function/memo.rs index 0e545d010..c68ca7df1 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -169,7 +169,7 @@ impl Memo { // The most common case is that the entire cycle is running in the same thread. // If that's the case, short circuit and return `true` immediately. - if self.validate_same_iteration(zalsa_local) { + if self.all_cycles_on_stack(zalsa_local) { return true; } @@ -250,6 +250,22 @@ impl Memo { }) } + fn all_cycles_on_stack(&self, zalsa_local: &ZalsaLocal) -> bool { + let cycle_heads = &self.revisions.cycle_heads; + if cycle_heads.is_empty() { + return true; + } + + zalsa_local.with_query_stack(|stack| { + cycle_heads.iter().all(|cycle_head| { + stack + .iter() + .rev() + .any(|query| query.database_key_index == cycle_head.database_key_index) + }) + }) + } + /// Cycle heads that should be propagated to dependent queries. #[inline(always)] pub(super) fn cycle_heads(&self) -> &CycleHeads { From b91a0025c845d0eef8d8b1ff0d8fc4cea343d84e Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 30 May 2025 13:35:45 +0200 Subject: [PATCH 18/26] Consider iteration in `validate_provisional` --- src/active_query.rs | 3 ++- src/function.rs | 7 +++++-- src/function/execute.rs | 2 +- src/function/fetch.rs | 17 ++++++++++++++++- src/function/maybe_changed_after.rs | 14 +++++++++++++- src/function/memo.rs | 20 ++++++++++++-------- src/function/specify.rs | 1 + src/ingredient.rs | 4 ++-- src/zalsa_local.rs | 4 +++- tests/parallel/cycle_a_t1_b_t2.rs | 11 +++++++++-- tests/parallel/cycle_ab_peeping_c.rs | 13 ++++++++++--- 11 files changed, 74 insertions(+), 22 deletions(-) diff --git a/src/active_query.rs b/src/active_query.rs index 095171210..4ac5069e9 100644 --- a/src/active_query.rs +++ b/src/active_query.rs @@ -190,7 +190,7 @@ impl ActiveQuery { ref mut accumulated, accumulated_inputs, ref mut cycle_heads, - iteration_count: _, + iteration_count, } = self; let origin = if untracked_read { @@ -213,6 +213,7 @@ impl ActiveQuery { tracked_struct_ids, accumulated_inputs, accumulated, + iteration: u8::try_from(iteration_count).expect("iteration count to be less than 250"), verified_final: AtomicBool::new(cycle_heads.is_empty()), cycle_heads, } diff --git a/src/function.rs b/src/function.rs index 7d5162232..b0564be9a 100644 --- a/src/function.rs +++ b/src/function.rs @@ -245,10 +245,13 @@ where /// True if the input `input` contains a memo that cites itself as a cycle head. /// This indicates an intermediate value for a cycle that has not yet reached a fixed point. - fn cycle_head_kind(&self, zalsa: &Zalsa, input: Id) -> CycleHeadKind { + fn cycle_head_kind(&self, zalsa: &Zalsa, input: Id, iteration: Option) -> CycleHeadKind { let is_provisional = self .get_memo_from_table_for(zalsa, input, self.memo_ingredient_index(zalsa, input)) - .is_some_and(|memo| !memo.revisions.verified_final.load(Ordering::Relaxed)); + .is_some_and(|memo| { + Some(memo.revisions.iteration as u32) != iteration + || !memo.revisions.verified_final.load(Ordering::Relaxed) + }); if is_provisional { CycleHeadKind::Provisional } else if C::CYCLE_STRATEGY == CycleRecoveryStrategy::FallbackImmediate { diff --git a/src/function/execute.rs b/src/function/execute.rs index 952aa2613..596be28ee 100644 --- a/src/function/execute.rs +++ b/src/function/execute.rs @@ -189,7 +189,7 @@ where C::id_to_input(db, id), ) { crate::CycleRecoveryAction::Iterate => { - tracing::debug!("{database_key_index:?}: execute: iterate again"); + tracing::debug!("{database_key_index:?}: execute: iterate again, revisions: {revisions:#?}"); } crate::CycleRecoveryAction::Fallback(fallback_value) => { tracing::debug!( diff --git a/src/function/fetch.rs b/src/function/fetch.rs index 8ac3493a1..a4522b8df 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -45,11 +45,19 @@ where id: Id, ) -> &'db Memo> { let memo_ingredient_index = self.memo_ingredient_index(zalsa, id); + let mut retries = 0; loop { if let Some(memo) = self .fetch_hot(zalsa, id, memo_ingredient_index) .or_else(|| { - self.fetch_cold_with_retry(zalsa, zalsa_local, db, id, memo_ingredient_index) + self.fetch_cold_with_retry( + zalsa, + zalsa_local, + db, + id, + memo_ingredient_index, + &mut retries, + ) }) { return memo; @@ -91,6 +99,7 @@ where db: &'db C::DbView, id: Id, memo_ingredient_index: MemoIngredientIndex, + retries: &mut u32, ) -> Option<&'db Memo>> { let memo = self.fetch_cold(zalsa, zalsa_local, db, id, memo_ingredient_index)?; @@ -106,6 +115,12 @@ where { Some(memo) } else { + assert!( + *retries < 250, + "Maximum cold fetch retries exceeded for {id:?}", + ); + + *retries += 1; None } } diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index ee2e20f22..961f785e0 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -257,9 +257,21 @@ where memo = memo.tracing_debug() ); for cycle_head in &memo.revisions.cycle_heads { + // Test if our cycle heads (with the same revision) are now finalized. + // It's important to also account for the revision 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 + // because its cycle head `b` is now finalized, but `b` never pulled `a` in the last iteration. let kind = zalsa .lookup_ingredient(cycle_head.database_key_index.ingredient_index()) - .cycle_head_kind(zalsa, cycle_head.database_key_index.key_index()); + .cycle_head_kind( + zalsa, + cycle_head.database_key_index.key_index(), + Some(cycle_head.iteration_count), + ); match kind { CycleHeadKind::Provisional => return false, CycleHeadKind::Final => { diff --git a/src/function/memo.rs b/src/function/memo.rs index c68ca7df1..d533a4df4 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -102,7 +102,7 @@ pub struct Memo { #[cfg(not(feature = "shuttle"))] #[cfg(target_pointer_width = "64")] const _: [(); std::mem::size_of::>()] = - [(); std::mem::size_of::<[usize; 11]>()]; + [(); std::mem::size_of::<[usize; 12]>()]; impl Memo { pub(super) fn new(value: Option, revision_now: Revision, revisions: QueryRevisions) -> Self { @@ -178,13 +178,17 @@ impl Memo { #[inline(never)] fn block_on_heads_cold(zalsa: &Zalsa, heads: &CycleHeads) -> bool { - let mut queue: Vec<_> = heads.iter().map(|head| head.database_key_index).collect(); - let mut queued: FxHashSet<_> = queue.iter().copied().collect(); + // TODO: Test if we can change `queued` to only track the `DatabaseKey` or if the iteration is important + let mut queue: Vec<_> = heads.iter().copied().collect(); + let mut queued: FxHashSet<_> = heads.iter().copied().collect(); let mut hit_cycle = false; while let Some(head) = queue.pop() { - let ingredient = zalsa.lookup_ingredient(head.ingredient_index()); - let cycle_head_kind = ingredient.cycle_head_kind(zalsa, head.key_index()); + let head_database_key = head.database_key_index; + let head_key_index = head_database_key.key_index(); + let ingredient = zalsa.lookup_ingredient(head_database_key.ingredient_index()); + // We don't care about the iteration. If it's final, we can go. + let cycle_head_kind = ingredient.cycle_head_kind(zalsa, head_key_index, None); match cycle_head_kind { CycleHeadKind::Final | CycleHeadKind::FallbackImmediate => { @@ -193,7 +197,7 @@ impl Memo { tracing::trace!("Dependent cycle head {head:?} has been finalized."); } CycleHeadKind::Provisional => { - if ingredient.wait_for(zalsa, head.key_index()) { + if ingredient.wait_for(zalsa, head_key_index) { // There's a new memo available for the cycle head (may still be provisional). tracing::trace!( "Dependent cycle head {head:?} has been released (there's a new memo)" @@ -211,9 +215,9 @@ impl Memo { // here, in case the memo has become part of another cycle (we need to block on that too!). queue.extend( ingredient - .cycle_heads(zalsa, head.key_index()) + .cycle_heads(zalsa, head_key_index) .iter() - .map(|head| head.database_key_index) + .copied() .filter(|head| queued.insert(*head)), ); } else { diff --git a/src/function/specify.rs b/src/function/specify.rs index 299f09c09..034991200 100644 --- a/src/function/specify.rs +++ b/src/function/specify.rs @@ -69,6 +69,7 @@ where tracked_struct_ids: Default::default(), accumulated: Default::default(), accumulated_inputs: Default::default(), + iteration: 0, verified_final: AtomicBool::new(true), cycle_heads: Default::default(), }; diff --git a/src/ingredient.rs b/src/ingredient.rs index ef61c9d1d..f4d4d9fb1 100644 --- a/src/ingredient.rs +++ b/src/ingredient.rs @@ -67,8 +67,8 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync { ) -> VerifyResult; /// Is the value for `input` in this ingredient a cycle head that is still provisional? - fn cycle_head_kind(&self, zalsa: &Zalsa, input: Id) -> CycleHeadKind { - _ = (zalsa, input); + fn cycle_head_kind(&self, zalsa: &Zalsa, input: Id, iteration: Option) -> CycleHeadKind { + _ = (zalsa, input, iteration); CycleHeadKind::Final } diff --git a/src/zalsa_local.rs b/src/zalsa_local.rs index bf6e94c43..a20dca6c1 100644 --- a/src/zalsa_local.rs +++ b/src/zalsa_local.rs @@ -355,6 +355,7 @@ pub(crate) struct QueryRevisions { /// Are the `cycle_heads` verified to not be provisional anymore? pub(super) verified_final: AtomicBool, + pub(super) iteration: u8, /// This result was computed based on provisional values from /// these cycle heads. The "cycle head" is the query responsible @@ -369,7 +370,7 @@ pub(crate) struct QueryRevisions { #[cfg(not(feature = "shuttle"))] #[cfg(target_pointer_width = "64")] -const _: [(); std::mem::size_of::()] = [(); std::mem::size_of::<[usize; 9]>()]; +const _: [(); std::mem::size_of::()] = [(); std::mem::size_of::<[usize; 10]>()]; impl QueryRevisions { pub(crate) fn fixpoint_initial(query: DatabaseKeyIndex) -> Self { @@ -380,6 +381,7 @@ impl QueryRevisions { tracked_struct_ids: Default::default(), accumulated: Default::default(), accumulated_inputs: Default::default(), + iteration: 0, verified_final: AtomicBool::new(false), cycle_heads: CycleHeads::initial(query), } diff --git a/tests/parallel/cycle_a_t1_b_t2.rs b/tests/parallel/cycle_a_t1_b_t2.rs index 061549ee7..d9d5ca365 100644 --- a/tests/parallel/cycle_a_t1_b_t2.rs +++ b/tests/parallel/cycle_a_t1_b_t2.rs @@ -62,11 +62,18 @@ fn initial(_db: &dyn KnobsDatabase) -> CycleValue { #[test_log::test] fn the_test() { crate::sync::check(|| { + tracing::debug!("New run"); let db_t1 = Knobs::default(); let db_t2 = db_t1.clone(); - let t1 = thread::spawn(move || query_a(&db_t1)); - let t2 = thread::spawn(move || query_b(&db_t2)); + let t1 = thread::spawn(move || { + let _span = tracing::debug_span!("t1", thread_id = ?thread::current().id()).entered(); + query_a(&db_t1) + }); + let t2 = thread::spawn(move || { + let _span = tracing::debug_span!("t2", thread_id = ?thread::current().id()).entered(); + query_b(&db_t2) + }); let (r_t1, r_t2) = (t1.join().unwrap(), t2.join().unwrap()); diff --git a/tests/parallel/cycle_ab_peeping_c.rs b/tests/parallel/cycle_ab_peeping_c.rs index 52fc2deea..134fe7429 100644 --- a/tests/parallel/cycle_ab_peeping_c.rs +++ b/tests/parallel/cycle_ab_peeping_c.rs @@ -60,7 +60,7 @@ fn query_c(db: &dyn KnobsDatabase) -> CycleValue { query_b(db) } -#[test] +#[test_log::test] fn the_test() { crate::sync::check(|| { let db_t1 = Knobs::default(); @@ -68,8 +68,15 @@ fn the_test() { let db_t2 = db_t1.clone(); db_t2.signal_on_will_block(2); - let t1 = thread::spawn(move || query_a(&db_t1)); - let t2 = thread::spawn(move || query_c(&db_t2)); + let t1 = thread::spawn(move || { + let _span = tracing::debug_span!("t1", thread_id = ?thread::current().id()).entered(); + query_a(&db_t1) + }); + let t2 = thread::spawn(move || { + let _span = tracing::debug_span!("t2", thread_id = ?thread::current().id()).entered(); + + query_c(&db_t2) + }); let (r_t1, r_t2) = (t1.join().unwrap(), t2.join().unwrap()); From 0c60657f6f8d9111b397ee16bad8eab21225b5de Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 30 May 2025 15:19:55 +0200 Subject: [PATCH 19/26] Only yield if all heads result in a cycle. Retry if even just one inner cycle made progress (in which case there's a probably a new memo) --- src/function/memo.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/function/memo.rs b/src/function/memo.rs index d533a4df4..264d451b7 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -161,7 +161,8 @@ impl Memo { /// Blocks on all cycle heads (recursively) that this memo depends on. /// - /// Returns `true` if awaiting the cycle heads resulted in a cycle. + /// Returns `true` if awaiting all cycle heads results in a cycle. This means, they're all waiting + /// for us to make progress. #[inline(always)] pub(super) fn block_on_heads(&self, zalsa: &Zalsa, zalsa_local: &ZalsaLocal) -> bool { // IMPORTANT: If you make changes to this function, make sure to run `cycle_nested_deep` with @@ -181,7 +182,7 @@ impl Memo { // TODO: Test if we can change `queued` to only track the `DatabaseKey` or if the iteration is important let mut queue: Vec<_> = heads.iter().copied().collect(); let mut queued: FxHashSet<_> = heads.iter().copied().collect(); - let mut hit_cycle = false; + let mut all_cycles = true; while let Some(head) = queue.pop() { let head_database_key = head.database_key_index; @@ -195,14 +196,16 @@ impl Memo { // This cycle is already finalized, so we don't need to wait on it; // keep looping through cycle heads. tracing::trace!("Dependent cycle head {head:?} has been finalized."); + all_cycles = false; } CycleHeadKind::Provisional => { if ingredient.wait_for(zalsa, head_key_index) { // There's a new memo available for the cycle head (may still be provisional). tracing::trace!( - "Dependent cycle head {head:?} has been released (there's a new memo)" - ); + "Dependent cycle head {head:?} has been released (there's a new memo)" + ); + all_cycles = false; // Recursively wait for all cycle heads that this head depends on. // This is normally not necessary, because cycle heads are transitively added // as query dependencies (they aggregate). The exception to this are queries @@ -224,13 +227,12 @@ impl Memo { // We hit a cycle blocking on the cycle head; this means this query actively // participates in the cycle and some other query is blocked on this thread. tracing::debug!("Waiting for {head:?} results in a cycle"); - hit_cycle = true; } } } } - hit_cycle + all_cycles } } From 02a9053daa64f987c3a8e5adb9e7db9a81459810 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 31 May 2025 15:29:44 +0200 Subject: [PATCH 20/26] Fix hangs --- src/active_query.rs | 24 ++-- src/cycle.rs | 38 ++++- src/event.rs | 3 +- src/function.rs | 25 +++- src/function/execute.rs | 26 ++-- src/function/fetch.rs | 100 +++++++------ src/function/maybe_changed_after.rs | 46 +++++- src/function/memo.rs | 216 ++++++++++++++++++++-------- src/function/specify.rs | 3 +- src/function/sync.rs | 20 +-- src/ingredient.rs | 42 +++++- src/runtime.rs | 119 ++++++++++----- src/runtime/dependency_graph.rs | 2 +- src/zalsa_local.rs | 8 +- tests/backtrace.rs | 6 +- tests/cycle.rs | 2 +- tests/cycle_output.rs | 6 +- tests/cycle_tracked.rs | 2 +- tests/cycle_tracked_own_input.rs | 2 +- 19 files changed, 477 insertions(+), 213 deletions(-) diff --git a/src/active_query.rs b/src/active_query.rs index 4ac5069e9..c39480133 100644 --- a/src/active_query.rs +++ b/src/active_query.rs @@ -4,7 +4,7 @@ use std::{fmt, mem, ops}; use crate::accumulator::accumulated_map::{ AccumulatedMap, AtomicInputAccumulatedValues, InputAccumulatedValues, }; -use crate::cycle::CycleHeads; +use crate::cycle::{CycleHeads, IterationCount}; use crate::durability::Durability; use crate::hash::FxIndexSet; use crate::key::DatabaseKeyIndex; @@ -62,7 +62,7 @@ pub(crate) struct ActiveQuery { cycle_heads: CycleHeads, /// If this query is a cycle head, iteration count of that cycle. - iteration_count: u32, + iteration_count: IterationCount, } impl ActiveQuery { @@ -148,7 +148,7 @@ impl ActiveQuery { } } - pub(super) fn iteration_count(&self) -> u32 { + pub(super) fn iteration_count(&self) -> IterationCount { self.iteration_count } @@ -162,7 +162,7 @@ impl ActiveQuery { } impl ActiveQuery { - fn new(database_key_index: DatabaseKeyIndex, iteration_count: u32) -> Self { + fn new(database_key_index: DatabaseKeyIndex, iteration_count: IterationCount) -> Self { ActiveQuery { database_key_index, durability: Durability::MAX, @@ -213,7 +213,7 @@ impl ActiveQuery { tracked_struct_ids, accumulated_inputs, accumulated, - iteration: u8::try_from(iteration_count).expect("iteration count to be less than 250"), + iteration: iteration_count, verified_final: AtomicBool::new(cycle_heads.is_empty()), cycle_heads, } @@ -238,10 +238,14 @@ impl ActiveQuery { tracked_struct_ids.clear(); accumulated.clear(); *cycle_heads = Default::default(); - *iteration_count = 0; + *iteration_count = IterationCount::initial(); } - fn reset_for(&mut self, new_database_key_index: DatabaseKeyIndex, new_iteration_count: u32) { + fn reset_for( + &mut self, + new_database_key_index: DatabaseKeyIndex, + new_iteration_count: IterationCount, + ) { let Self { database_key_index, durability, @@ -325,7 +329,7 @@ impl QueryStack { pub(crate) fn push_new_query( &mut self, database_key_index: DatabaseKeyIndex, - iteration_count: u32, + iteration_count: IterationCount, ) { if self.len < self.stack.len() { self.stack[self.len].reset_for(database_key_index, iteration_count); @@ -375,7 +379,7 @@ struct CapturedQuery { durability: Durability, changed_at: Revision, cycle_heads: CycleHeads, - iteration_count: u32, + iteration_count: IterationCount, } impl fmt::Debug for CapturedQuery { @@ -451,7 +455,7 @@ impl fmt::Display for Backtrace { write!(fmt, "{idx:>4}: {database_key_index:?}")?; if full { write!(fmt, " -> ({changed_at:?}, {durability:#?}")?; - if !cycle_heads.is_empty() || iteration_count > 0 { + if !cycle_heads.is_empty() || !iteration_count.is_initial() { write!(fmt, ", iteration = {iteration_count:?}")?; } write!(fmt, ")")?; diff --git a/src/cycle.rs b/src/cycle.rs index 2b56abfd3..9d9747c39 100644 --- a/src/cycle.rs +++ b/src/cycle.rs @@ -60,7 +60,7 @@ use crate::sync::OnceLock; /// The maximum number of times we'll fixpoint-iterate before panicking. /// /// Should only be relevant in case of a badly configured cycle recovery. -pub const MAX_ITERATIONS: u32 = 200; +pub const MAX_ITERATIONS: IterationCount = IterationCount(200); pub struct UnexpectedCycle(Option); @@ -147,7 +147,33 @@ pub enum CycleRecoveryStrategy { #[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)] pub struct CycleHead { pub(crate) database_key_index: DatabaseKeyIndex, - pub(crate) iteration_count: u32, + pub(crate) iteration_count: IterationCount, +} + +#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, Default)] +pub struct IterationCount(u8); + +impl IterationCount { + pub(crate) const fn initial() -> Self { + Self(0) + } + + pub(crate) const fn is_initial(self) -> bool { + self.0 == 0 + } + + pub(crate) const fn increment(self) -> Option { + let next = Self(self.0 + 1); + if next.0 <= MAX_ITERATIONS.0 { + Some(next) + } else { + None + } + } + + pub(crate) const fn as_u32(self) -> u32 { + self.0 as u32 + } } /// Any provisional value generated by any query in a cycle will track the cycle head(s) (can be @@ -164,7 +190,7 @@ impl CycleHeads { pub(crate) fn initial(database_key_index: DatabaseKeyIndex) -> Self { Self(thin_vec![CycleHead { database_key_index, - iteration_count: 0, + iteration_count: IterationCount::initial(), }]) } @@ -190,7 +216,7 @@ impl CycleHeads { pub(crate) fn update_iteration_count( &mut self, cycle_head_index: DatabaseKeyIndex, - new_iteration_count: u32, + new_iteration_count: IterationCount, ) { if let Some(cycle_head) = self .0 @@ -208,11 +234,11 @@ impl CycleHeads { .iter() .find(|candidate| candidate.database_key_index == database_key_index) { - assert_eq!(existing.iteration_count, 0); + assert_eq!(existing.iteration_count, IterationCount::initial()); } else { self.0.push(CycleHead { database_key_index, - iteration_count: 0, + iteration_count: IterationCount::initial(), }); } } diff --git a/src/event.rs b/src/event.rs index 284164d11..ddbcc07e5 100644 --- a/src/event.rs +++ b/src/event.rs @@ -1,3 +1,4 @@ +use crate::cycle::IterationCount; use crate::key::DatabaseKeyIndex; use crate::sync::thread::{self, ThreadId}; use crate::Revision; @@ -61,7 +62,7 @@ pub enum EventKind { WillIterateCycle { /// The database-key for the cycle head. Implements `Debug`. database_key: DatabaseKeyIndex, - iteration_count: u32, + iteration_count: IterationCount, fell_back: bool, }, diff --git a/src/function.rs b/src/function.rs index b0564be9a..948936bfe 100644 --- a/src/function.rs +++ b/src/function.rs @@ -3,14 +3,16 @@ use std::any::Any; use std::fmt; use std::ptr::NonNull; use std::sync::atomic::Ordering; +pub(crate) use sync::SyncGuard; use crate::accumulator::accumulated_map::{AccumulatedMap, InputAccumulatedValues}; use crate::cycle::{ empty_cycle_heads, CycleHeadKind, CycleHeads, CycleRecoveryAction, CycleRecoveryStrategy, + IterationCount, }; use crate::function::delete::DeletedEntries; use crate::function::sync::{ClaimResult, SyncTable}; -use crate::ingredient::Ingredient; +use crate::ingredient::{Ingredient, WaitForResult}; use crate::key::DatabaseKeyIndex; use crate::plumbing::MemoIngredientMap; use crate::salsa_struct::SalsaStructInDb; @@ -245,11 +247,16 @@ where /// True if the input `input` contains a memo that cites itself as a cycle head. /// This indicates an intermediate value for a cycle that has not yet reached a fixed point. - fn cycle_head_kind(&self, zalsa: &Zalsa, input: Id, iteration: Option) -> CycleHeadKind { + fn cycle_head_kind( + &self, + zalsa: &Zalsa, + input: Id, + iteration: Option, + ) -> CycleHeadKind { let is_provisional = self .get_memo_from_table_for(zalsa, input, self.memo_ingredient_index(zalsa, input)) .is_some_and(|memo| { - Some(memo.revisions.iteration as u32) != iteration + Some(memo.revisions.iteration) != iteration || !memo.revisions.verified_final.load(Ordering::Relaxed) }); if is_provisional { @@ -261,6 +268,11 @@ where } } + fn iteration(&self, zalsa: &Zalsa, input: Id) -> Option { + self.get_memo_from_table_for(zalsa, input, self.memo_ingredient_index(zalsa, input)) + .map(|memo| memo.revisions.iteration) + } + fn cycle_heads<'db>(&self, zalsa: &'db Zalsa, input: Id) -> &'db CycleHeads { self.get_memo_from_table_for(zalsa, input, self.memo_ingredient_index(zalsa, input)) .map(|memo| memo.cycle_heads()) @@ -268,10 +280,11 @@ where } /// Attempts to claim `key_index`, returning `false` if a cycle occurs. - fn wait_for(&self, zalsa: &Zalsa, key_index: Id) -> bool { + fn wait_for<'me>(&'me self, zalsa: &'me Zalsa, key_index: Id) -> WaitForResult<'me> { match self.sync_table.try_claim(zalsa, key_index) { - ClaimResult::Retry | ClaimResult::Claimed(_) => true, - ClaimResult::Cycle => false, + ClaimResult::BlockedOn(blocked_on) => WaitForResult::running(blocked_on), + ClaimResult::Cycle => WaitForResult::Cycle, + ClaimResult::Claimed(_) => WaitForResult::Available, } } diff --git a/src/function/execute.rs b/src/function/execute.rs index 596be28ee..4b3252b2f 100644 --- a/src/function/execute.rs +++ b/src/function/execute.rs @@ -1,4 +1,4 @@ -use crate::cycle::{CycleRecoveryStrategy, MAX_ITERATIONS}; +use crate::cycle::{CycleRecoveryStrategy, IterationCount}; use crate::function::memo::Memo; use crate::function::{Configuration, IngredientImpl}; use crate::sync::atomic::{AtomicBool, Ordering}; @@ -74,7 +74,9 @@ where // Cycle participants that don't have a fallback will be discarded in // `validate_provisional()`. let cycle_heads = revisions.cycle_heads; - let active_query = db.zalsa_local().push_query(database_key_index, 0); + let active_query = db + .zalsa_local() + .push_query(database_key_index, IterationCount::initial()); new_value = C::cycle_initial(db, C::id_to_input(db, id)); revisions = active_query.pop(); // We need to set `cycle_heads` and `verified_final` because it needs to propagate to the callers. @@ -124,7 +126,7 @@ where memo_ingredient_index: MemoIngredientIndex, ) -> (C::Output<'db>, QueryRevisions) { let database_key_index = active_query.database_key_index; - let mut iteration_count: u32 = 0; + let mut iteration_count = IterationCount::initial(); let mut fell_back = false; // Our provisional value from the previous iteration, when doing fixpoint iteration. @@ -185,12 +187,10 @@ where match C::recover_from_cycle( db, &new_value, - iteration_count, + iteration_count.as_u32(), C::id_to_input(db, id), ) { - crate::CycleRecoveryAction::Iterate => { - tracing::debug!("{database_key_index:?}: execute: iterate again, revisions: {revisions:#?}"); - } + crate::CycleRecoveryAction::Iterate => {} crate::CycleRecoveryAction::Fallback(fallback_value) => { tracing::debug!( "{database_key_index:?}: execute: user cycle_fn says to fall back" @@ -204,10 +204,9 @@ where } // `iteration_count` can't overflow as we check it against `MAX_ITERATIONS` // which is less than `u32::MAX`. - iteration_count += 1; - if iteration_count > MAX_ITERATIONS { - panic!("{database_key_index:?}: execute: too many cycle iterations"); - } + iteration_count = iteration_count.increment().unwrap_or_else(|| { + panic!("{database_key_index:?}: execute: too many cycle iterations") + }); zalsa.event(&|| { Event::new(EventKind::WillIterateCycle { database_key: database_key_index, @@ -218,6 +217,11 @@ where revisions .cycle_heads .update_iteration_count(database_key_index, iteration_count); + revisions.iteration = iteration_count; + tracing::debug!( + "{database_key_index:?}: execute: iterate again, revisions: {revisions:#?}" + ); + opt_last_provisional = Some(self.insert_memo( zalsa, id, diff --git a/src/function/fetch.rs b/src/function/fetch.rs index a4522b8df..39ae51a87 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -1,4 +1,4 @@ -use crate::cycle::{CycleHeads, CycleRecoveryStrategy, UnexpectedCycle}; +use crate::cycle::{CycleHeads, CycleRecoveryStrategy, IterationCount, UnexpectedCycle}; use crate::function::memo::Memo; use crate::function::sync::ClaimResult; use crate::function::{Configuration, IngredientImpl, VerifyResult}; @@ -45,19 +45,11 @@ where id: Id, ) -> &'db Memo> { let memo_ingredient_index = self.memo_ingredient_index(zalsa, id); - let mut retries = 0; loop { if let Some(memo) = self .fetch_hot(zalsa, id, memo_ingredient_index) .or_else(|| { - self.fetch_cold_with_retry( - zalsa, - zalsa_local, - db, - id, - memo_ingredient_index, - &mut retries, - ) + self.fetch_cold_with_retry(zalsa, zalsa_local, db, id, memo_ingredient_index) }) { return memo; @@ -99,7 +91,6 @@ where db: &'db C::DbView, id: Id, memo_ingredient_index: MemoIngredientIndex, - retries: &mut u32, ) -> Option<&'db Memo>> { let memo = self.fetch_cold(zalsa, zalsa_local, db, id, memo_ingredient_index)?; @@ -115,12 +106,6 @@ where { Some(memo) } else { - assert!( - *retries < 250, - "Maximum cold fetch retries exceeded for {id:?}", - ); - - *retries += 1; None } } @@ -135,8 +120,22 @@ where ) -> Option<&'db Memo>> { let database_key_index = self.database_key_index(id); // Try to claim this query: if someone else has claimed it already, go back and start again. - let _claim_guard = match self.sync_table.try_claim(zalsa, id) { - ClaimResult::Retry => return None, + let claim_guard = match self.sync_table.try_claim(zalsa, id) { + ClaimResult::BlockedOn(blocked_on) => { + blocked_on.wait_for(zalsa); + + let memo = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index); + + if let Some(memo) = memo { + // This isn't strictly necessary, but if this is a provisional memo for an inner cycle, + // await all outer cycle heads to give the thread driving it a chance to complete + // (we don't want multiple threads compute for the queries participating in the same cycle). + if memo.value.is_some() && memo.may_be_provisional() { + memo.block_on_heads(zalsa, zalsa_local); + } + } + return None; + } ClaimResult::Cycle => { // check if there's a provisional value for this query // Note we don't `validate_may_be_provisional` the memo here as we want to reuse an @@ -181,7 +180,8 @@ where tracing::debug!( "hit a `FallbackImmediate` cycle at {database_key_index:#?}" ); - let active_query = zalsa_local.push_query(database_key_index, 0); + let active_query = + zalsa_local.push_query(database_key_index, IterationCount::initial()); let fallback_value = C::cycle_initial(db, C::id_to_input(db, id)); let mut revisions = active_query.pop(); revisions.cycle_heads = CycleHeads::initial(database_key_index); @@ -214,43 +214,41 @@ where return unsafe { Some(self.extend_memo_lifetime(old_memo)) }; } } - } - } - - // If this is a provisional memo from the same revision, await all its cycle heads because - // we need to ensure that only one thread is iterating on a cycle at a given time. - // For example, if we have a nested cycle like so: - // ``` - // a -> b -> c -> b - // -> a - // - // d -> b - // ``` - // thread 1 calls `a` and `a` completes the inner cycle `b -> c` but hasn't finished the outer cycle `a` yet. - // thread 2 now calls `b`. We don't want that thread 2 iterates `b` while thread 1 is iterating `a` at the same time - // because it can result in thread b overriding provisional memos that thread a has accessed already and still relies upon. - // - // By waiting, we ensure that thread 1 completes a (based on a provisional value for `b`) and `b` - // becomes the new outer cycle, which thread 2 drives to completion. - if let Some(old_memo) = opt_old_memo { - if old_memo.value.is_some() - && old_memo.may_be_provisional() - && old_memo.verified_at.load() == zalsa.current_revision() - { - // TODO(micha) 29th May 2025: It would be nice if `block_on_heads` would release - // and re-acquire the lock if it blocks on any other thread. That would allow any other thread - // to claim this query (e.g. thread a in the example above) and make progress on it. - old_memo.block_on_heads(zalsa, zalsa_local); - // It's possible that one of the cycle heads replaced the memo for this ingredient - // with fixpoint initial. We ignore that memo because we know it's only a temporary memo - // and instead continue with the memo we already have (acquired). + // If this is a provisional memo from the same revision, await all its cycle heads because + // we need to ensure that only one thread is iterating on a cycle at a given time. + // For example, if we have a nested cycle like so: + // ``` + // a -> b -> c -> b + // -> a + // + // d -> b + // ``` + // thread 1 calls `a` and `a` completes the inner cycle `b -> c` but hasn't finished the outer cycle `a` yet. + // thread 2 now calls `b`. We don't want that thread 2 iterates `b` while thread 1 is iterating `a` at the same time + // because it can result in thread b overriding provisional memos that thread a has accessed already and still relies upon. + // + // By waiting, we ensure that thread 1 completes a (based on a provisional value for `b`) and `b` + // becomes the new outer cycle, which thread 2 drives to completion. + if old_memo.may_be_provisional() + && old_memo.verified_at.load() == zalsa.current_revision() + { + // Try to claim all cycle heads of the provisional memo. If we can't because + // some head is running on another thread, drop our claim guard to give that thread + // a chance to take ownership of this query and complete it as part of its fixpoint iteration. + // We will then block on the cycle head and retry once all cycle heads completed. + if !old_memo.try_claim_heads(zalsa, zalsa_local) { + drop(claim_guard); + old_memo.block_on_heads(zalsa, zalsa_local); + return None; + } + } } } let memo = self.execute( db, - zalsa_local.push_query(database_key_index, 0), + zalsa_local.push_query(database_key_index, IterationCount::initial()), opt_old_memo, ); diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index 961f785e0..efb04394d 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -1,8 +1,11 @@ use crate::accumulator::accumulated_map::InputAccumulatedValues; -use crate::cycle::{CycleHeadKind, CycleHeads, CycleRecoveryStrategy, UnexpectedCycle}; +use crate::cycle::{ + CycleHeadKind, CycleHeads, CycleRecoveryStrategy, IterationCount, UnexpectedCycle, +}; use crate::function::memo::Memo; use crate::function::sync::ClaimResult; use crate::function::{Configuration, IngredientImpl}; +use crate::ingredient::WaitForResult; use crate::key::DatabaseKeyIndex; use crate::sync::atomic::Ordering; use crate::zalsa::{MemoIngredientIndex, Zalsa, ZalsaDatabase}; @@ -101,7 +104,10 @@ where let database_key_index = self.database_key_index(key_index); let _claim_guard = match self.sync_table.try_claim(zalsa, key_index) { - ClaimResult::Retry => return None, + ClaimResult::BlockedOn(blocked_on) => { + blocked_on.wait_for(zalsa); + return None; + } ClaimResult::Cycle => match C::CYCLE_STRATEGY { CycleRecoveryStrategy::Panic => UnexpectedCycle::throw(), CycleRecoveryStrategy::FallbackImmediate => { @@ -151,7 +157,9 @@ 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.is_empty() { - let active_query = db.zalsa_local().push_query(database_key_index, 0); + let active_query = db + .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; @@ -240,7 +248,7 @@ where ) -> bool { !memo.may_be_provisional() || self.validate_provisional(zalsa, database_key_index, memo) - || self.validate_same_iteration(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 @@ -305,6 +313,7 @@ where /// runaway re-execution of the same queries within a fixpoint iteration. pub(super) fn validate_same_iteration( &self, + zalsa: &Zalsa, zalsa_local: &ZalsaLocal, database_key_index: DatabaseKeyIndex, memo: &Memo>, @@ -314,7 +323,34 @@ where memo = memo.tracing_debug() ); - memo.validate_same_iteration(zalsa_local) + let cycle_heads = &memo.revisions.cycle_heads; + if cycle_heads.is_empty() { + return true; + } + + zalsa_local.with_query_stack(|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 this is a cycle head is owned by another thread that is blocked by this ingredient, + // check if it has the same iteration count. + let ingredient = zalsa + .lookup_ingredient(cycle_head.database_key_index.ingredient_index()); + + match ingredient.wait_for(zalsa, cycle_head.database_key_index.key_index()) + { + WaitForResult::Running(_) | WaitForResult::Available => None, + WaitForResult::Cycle => ingredient + .iteration(zalsa, cycle_head.database_key_index.key_index()), + } + }) + == Some(cycle_head.iteration_count) + }) + }) } /// VerifyResult::Unchanged if the memo's value and `changed_at` time is up-to-date in the diff --git a/src/function/memo.rs b/src/function/memo.rs index 264d451b7..cfc2b7897 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -3,11 +3,13 @@ use std::fmt::{Debug, Formatter}; use std::mem::transmute; use std::ptr::NonNull; -use crate::cycle::{empty_cycle_heads, CycleHeadKind, CycleHeads}; +use crate::cycle::{empty_cycle_heads, CycleHead, CycleHeadKind, CycleHeads}; use crate::function::{Configuration, IngredientImpl}; use crate::hash::FxHashSet; +use crate::ingredient::{Ingredient, WaitForResult}; use crate::key::DatabaseKeyIndex; use crate::revision::AtomicRevision; +use crate::runtime::BlockedOn; use crate::sync::atomic::Ordering; use crate::table::memo::MemoTableWithTypesMut; use crate::zalsa::{MemoIngredientIndex, Zalsa}; @@ -106,6 +108,9 @@ const _: [(); std::mem::size_of::>()] = impl Memo { pub(super) fn new(value: Option, revision_now: Revision, revisions: QueryRevisions) -> Self { + debug_assert!( + !revisions.verified_final.load(Ordering::Relaxed) || revisions.cycle_heads.is_empty() + ); Memo { value, verified_at: AtomicRevision::from(revision_now), @@ -179,55 +184,21 @@ impl Memo { #[inline(never)] fn block_on_heads_cold(zalsa: &Zalsa, heads: &CycleHeads) -> bool { - // TODO: Test if we can change `queued` to only track the `DatabaseKey` or if the iteration is important - let mut queue: Vec<_> = heads.iter().copied().collect(); - let mut queued: FxHashSet<_> = heads.iter().copied().collect(); + let mut cycle_heads = TryClaimCycleHeadsIter::new(zalsa, heads); let mut all_cycles = true; - while let Some(head) = queue.pop() { - let head_database_key = head.database_key_index; - let head_key_index = head_database_key.key_index(); - let ingredient = zalsa.lookup_ingredient(head_database_key.ingredient_index()); - // We don't care about the iteration. If it's final, we can go. - let cycle_head_kind = ingredient.cycle_head_kind(zalsa, head_key_index, None); - - match cycle_head_kind { - CycleHeadKind::Final | CycleHeadKind::FallbackImmediate => { - // This cycle is already finalized, so we don't need to wait on it; - // keep looping through cycle heads. - tracing::trace!("Dependent cycle head {head:?} has been finalized."); + while let Some(claim_result) = cycle_heads.next() { + match claim_result { + TryClaimHeadsResult::Cycle => {} + TryClaimHeadsResult::Finalized => { all_cycles = false; } - CycleHeadKind::Provisional => { - if ingredient.wait_for(zalsa, head_key_index) { - // There's a new memo available for the cycle head (may still be provisional). - tracing::trace!( - "Dependent cycle head {head:?} has been released (there's a new memo)" - ); - - all_cycles = false; - // Recursively wait for all cycle heads that this head depends on. - // This is normally not necessary, because cycle heads are transitively added - // as query dependencies (they aggregate). The exception to this are queries - // that depend on a fixpoint initial value. They only depend on the fixpoint initial - // value but not on its dependencies because they aren't known yet. They're only known - // once the cycle completes but the cycle heads of the queries don't get updated. - // Because of that, recurse here to collect all cycle heads. - // This also ensures that if a query added new cycle heads, that they are awaited too. - // IMPORTANT: It's critical that we get the cycle head from the latest memo - // here, in case the memo has become part of another cycle (we need to block on that too!). - queue.extend( - ingredient - .cycle_heads(zalsa, head_key_index) - .iter() - .copied() - .filter(|head| queued.insert(*head)), - ); - } else { - // We hit a cycle blocking on the cycle head; this means this query actively - // participates in the cycle and some other query is blocked on this thread. - tracing::debug!("Waiting for {head:?} results in a cycle"); - } + TryClaimHeadsResult::Available => { + all_cycles = false; + } + TryClaimHeadsResult::Running(running) => { + all_cycles = false; + running.block_on(&mut cycle_heads); } } } @@ -236,24 +207,28 @@ impl Memo { } } - /// If this is a provisional memo, validate that it was cached in the same iteration of the - /// same cycle(s) that we are still executing. If so, it is valid for reuse. This avoids - /// runaway re-execution of the same queries within a fixpoint iteration. - pub(super) fn validate_same_iteration(&self, zalsa_local: &ZalsaLocal) -> bool { - let cycle_heads = &self.revisions.cycle_heads; - if cycle_heads.is_empty() { + /// Tries to claim all cycle heads to see if they're finalized or available. + /// + /// Unlike `block_on_heads`, this code does not block on any cycle head. Instead it returns `false` if + /// claiming all cycle heads failed because one of them is running on another thread. + pub(super) fn try_claim_heads(&self, zalsa: &Zalsa, zalsa_local: &ZalsaLocal) -> bool { + if self.all_cycles_on_stack(zalsa_local) { return true; } + let cycle_heads = TryClaimCycleHeadsIter::new(zalsa, &self.revisions.cycle_heads); + + for claim_result in cycle_heads { + match claim_result { + TryClaimHeadsResult::Cycle + | TryClaimHeadsResult::Finalized + | TryClaimHeadsResult::Available => {} + TryClaimHeadsResult::Running(_) => { + return false; + } + } + } - zalsa_local.with_query_stack(|stack| { - cycle_heads.iter().all(|cycle_head| { - stack - .iter() - .rev() - .find(|query| query.database_key_index == cycle_head.database_key_index) - .is_some_and(|query| query.iteration_count() == cycle_head.iteration_count) - }) - }) + true } fn all_cycles_on_stack(&self, zalsa_local: &ZalsaLocal) -> bool { @@ -324,7 +299,7 @@ impl Memo { }, ) .field("verified_at", &self.memo.verified_at) - // .field("revisions", &self.memo.revisions) + .field("revisions", &self.memo.revisions) .finish() } } @@ -338,3 +313,120 @@ impl crate::table::memo::Memo for Memo { self.revisions.origin.as_ref() } } + +pub(super) enum TryClaimHeadsResult<'me> { + /// Claiming every cycle head results in a cycle head. + Cycle, + + /// The cycle head has been finalized. + Finalized, + + /// The cycle head is not finalized, but it can be claimed. + Available, + + /// The cycle head is currently executed on another thread. + Running(RunningCycleHead<'me>), +} + +pub(super) struct RunningCycleHead<'me> { + blocked_on: BlockedOn<'me>, + ingredient: &'me dyn Ingredient, +} + +impl<'a> RunningCycleHead<'a> { + fn block_on(self, cycle_heads: &mut TryClaimCycleHeadsIter<'a>) { + let key_index = self.blocked_on.database_key().key_index(); + self.blocked_on.wait_for(cycle_heads.zalsa); + + cycle_heads.queue_ingredient_heads(self.ingredient, key_index); + } +} + +/// Iterator to try claiming the transitive cycle heads of a memo. +struct TryClaimCycleHeadsIter<'a> { + zalsa: &'a Zalsa, + queue: Vec, + queued: FxHashSet, +} + +impl<'a> TryClaimCycleHeadsIter<'a> { + fn new(zalsa: &'a Zalsa, heads: &CycleHeads) -> Self { + let queue: Vec<_> = heads.iter().copied().collect(); + let queued: FxHashSet<_> = queue.iter().copied().collect(); + + Self { + zalsa, + queue, + queued, + } + } + + fn queue_ingredient_heads(&mut self, ingredient: &dyn Ingredient, key: Id) { + // Recursively wait for all cycle heads that this head depends on. + // This is normally not necessary, because cycle heads are transitively added + // as query dependencies (they aggregate). The exception to this are queries + // that depend on a fixpoint initial value. They only depend on the fixpoint initial + // value but not on its dependencies because they aren't known yet. They're only known + // once the cycle completes but the cycle heads of the queries don't get updated. + // Because of that, recurse here to collect all cycle heads. + // This also ensures that if a query added new cycle heads, that they are awaited too. + // IMPORTANT: It's critical that we get the cycle head from the latest memo + // here, in case the memo has become part of another cycle (we need to block on that too!). + self.queue.extend( + ingredient + .cycle_heads(self.zalsa, key) + .iter() + .copied() + .filter(|head| self.queued.insert(*head)), + ) + } +} + +impl<'me> Iterator for TryClaimCycleHeadsIter<'me> { + type Item = TryClaimHeadsResult<'me>; + + fn next(&mut self) -> Option { + let head = self.queue.pop()?; + + let head_database_key = head.database_key_index; + let head_key_index = head_database_key.key_index(); + let ingredient = self + .zalsa + .lookup_ingredient(head_database_key.ingredient_index()); + // We don't care about the iteration. If it's final, we can go. + let cycle_head_kind = ingredient.cycle_head_kind(self.zalsa, head_key_index, None); + + match cycle_head_kind { + CycleHeadKind::Final | CycleHeadKind::FallbackImmediate => { + // This cycle is already finalized, so we don't need to wait on it; + // keep looping through cycle heads. + tracing::trace!("Dependent cycle head {head:?} has been finalized."); + Some(TryClaimHeadsResult::Finalized) + } + CycleHeadKind::Provisional => { + match ingredient.wait_for(self.zalsa, head_key_index) { + WaitForResult::Cycle => { + // We hit a cycle blocking on the cycle head; this means this query actively + // participates in the cycle and some other query is blocked on this thread. + tracing::debug!("Waiting for {head:?} results in a cycle"); + Some(TryClaimHeadsResult::Cycle) + } + WaitForResult::Running(running) => { + tracing::debug!( + "Ingredient {head:?} is running: {running:?}, blocking on it" + ); + + Some(TryClaimHeadsResult::Running(RunningCycleHead { + blocked_on: running.into_blocked_on(), + ingredient, + })) + } + WaitForResult::Available => { + self.queue_ingredient_heads(ingredient, head_key_index); + Some(TryClaimHeadsResult::Available) + } + } + } + } + } +} diff --git a/src/function/specify.rs b/src/function/specify.rs index 034991200..4ba6ebbd7 100644 --- a/src/function/specify.rs +++ b/src/function/specify.rs @@ -1,4 +1,5 @@ use crate::accumulator::accumulated_map::InputAccumulatedValues; +use crate::cycle::IterationCount; use crate::function::memo::Memo; use crate::function::{Configuration, IngredientImpl}; use crate::revision::AtomicRevision; @@ -69,7 +70,7 @@ where tracked_struct_ids: Default::default(), accumulated: Default::default(), accumulated_inputs: Default::default(), - iteration: 0, + iteration: IterationCount::initial(), verified_final: AtomicBool::new(true), cycle_heads: Default::default(), }; diff --git a/src/function/sync.rs b/src/function/sync.rs index cc5018c60..e94c40fba 100644 --- a/src/function/sync.rs +++ b/src/function/sync.rs @@ -1,12 +1,14 @@ use rustc_hash::FxHashMap; use crate::key::DatabaseKeyIndex; -use crate::runtime::{BlockResult, WaitResult}; +use crate::runtime::{BlockResult, BlockedOn, WaitResult}; use crate::sync::thread::{self, ThreadId}; use crate::sync::Mutex; use crate::zalsa::Zalsa; use crate::{Id, IngredientIndex}; +pub(crate) type SyncGuard<'me> = crate::sync::MutexGuard<'me, FxHashMap>; + /// Tracks the keys that are currently being processed; used to coordinate between /// worker threads. pub(crate) struct SyncTable { @@ -15,12 +17,15 @@ pub(crate) struct SyncTable { } pub(crate) enum ClaimResult<'a> { - Retry, + /// Can't claim the query because it is running on an other thread. + BlockedOn(BlockedOn<'a>), + /// Claiming the query results in a cycle. Cycle, + /// Successfully claimed the query. Claimed(ClaimGuard<'a>), } -struct SyncState { +pub(crate) struct SyncState { id: ThreadId, /// Set to true if any other queries are blocked, @@ -51,13 +56,12 @@ impl SyncTable { // boolean is to decide *whether* to acquire the lock, // not to gate future atomic reads. *anyone_waiting = true; - match zalsa.runtime().block_on( - zalsa, + match zalsa.runtime().block( DatabaseKeyIndex::new(self.ingredient, key_index), id, write, ) { - BlockResult::Completed => ClaimResult::Retry, + BlockResult::BlockedOn(blocked_on) => ClaimResult::BlockedOn(blocked_on), BlockResult::Cycle => ClaimResult::Cycle, } } @@ -70,7 +74,6 @@ impl SyncTable { key_index, zalsa, sync_table: self, - _padding: false, }) } } @@ -84,9 +87,6 @@ pub(crate) struct ClaimGuard<'me> { key_index: Id, zalsa: &'me Zalsa, sync_table: &'me SyncTable, - // Reduce the size of ClaimResult by making more niches available in ClaimGuard; this fits into - // the padding of ClaimGuard so doesn't increase its size. - _padding: bool, } impl ClaimGuard<'_> { diff --git a/src/ingredient.rs b/src/ingredient.rs index f4d4d9fb1..488289173 100644 --- a/src/ingredient.rs +++ b/src/ingredient.rs @@ -2,9 +2,12 @@ use std::any::{Any, TypeId}; use std::fmt; use crate::accumulator::accumulated_map::{AccumulatedMap, InputAccumulatedValues}; -use crate::cycle::{empty_cycle_heads, CycleHeadKind, CycleHeads, CycleRecoveryStrategy}; +use crate::cycle::{ + empty_cycle_heads, CycleHeadKind, CycleHeads, CycleRecoveryStrategy, IterationCount, +}; use crate::function::VerifyResult; use crate::plumbing::IngredientIndices; +use crate::runtime::BlockedOn; use crate::sync::Arc; use crate::table::memo::MemoTableTypes; use crate::table::Table; @@ -67,11 +70,21 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync { ) -> VerifyResult; /// Is the value for `input` in this ingredient a cycle head that is still provisional? - fn cycle_head_kind(&self, zalsa: &Zalsa, input: Id, iteration: Option) -> CycleHeadKind { + fn cycle_head_kind( + &self, + zalsa: &Zalsa, + input: Id, + iteration: Option, + ) -> CycleHeadKind { _ = (zalsa, input, iteration); CycleHeadKind::Final } + fn iteration(&self, zalsa: &Zalsa, input: Id) -> Option { + _ = (zalsa, input); + None + } + /// Returns the cycle heads for this ingredient. fn cycle_heads<'db>(&self, zalsa: &'db Zalsa, input: Id) -> &'db CycleHeads { _ = (zalsa, input); @@ -83,9 +96,9 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync { /// A return value of `true` indicates that a result is now available. A return value of /// `false` means that a cycle was encountered; the waited-on query is either already claimed /// by the current thread, or by a thread waiting on the current thread. - fn wait_for(&self, zalsa: &Zalsa, key_index: Id) -> bool { + fn wait_for<'me>(&'me self, zalsa: &'me Zalsa, key_index: Id) -> WaitForResult<'me> { _ = (zalsa, key_index); - true + WaitForResult::Available } /// Invoked when the value `output_key` should be marked as valid in the current revision. @@ -206,3 +219,24 @@ impl dyn Ingredient { pub(crate) fn fmt_index(debug_name: &str, id: Id, fmt: &mut fmt::Formatter<'_>) -> fmt::Result { write!(fmt, "{debug_name}({id:?})") } + +pub enum WaitForResult<'me> { + Running(Running<'me>), + Available, + Cycle, +} + +impl<'a> WaitForResult<'a> { + pub(crate) const fn running(blocked_on: BlockedOn<'a>) -> Self { + WaitForResult::Running(Running(blocked_on)) + } +} + +#[derive(Debug)] +pub struct Running<'me>(BlockedOn<'me>); + +impl<'a> Running<'a> { + pub(crate) fn into_blocked_on(self) -> BlockedOn<'a> { + self.0 + } +} diff --git a/src/runtime.rs b/src/runtime.rs index 294bfd162..09aaab59e 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -1,10 +1,12 @@ use self::dependency_graph::DependencyGraph; use crate::durability::Durability; +use crate::function::SyncGuard; use crate::key::DatabaseKeyIndex; use crate::sync::atomic::{AtomicBool, Ordering}; use crate::sync::thread::{self, ThreadId}; use crate::sync::Mutex; use crate::table::Table; +use crate::zalsa::Zalsa; use crate::{Cancelled, Event, EventKind, Revision}; mod dependency_graph; @@ -34,18 +36,87 @@ pub struct Runtime { table: Table, } -#[derive(Clone, Debug)] -pub(crate) enum WaitResult { +#[derive(Copy, Clone, Debug)] +pub(super) enum WaitResult { Completed, Panicked, } -#[derive(Clone, Debug)] -pub(crate) enum BlockResult { - Completed, +impl WaitResult { + pub(crate) const fn is_panicked(self) -> bool { + matches!(self, WaitResult::Panicked) + } +} + +#[derive(Debug)] +pub(crate) enum BlockResult<'me> { + /// The query is running on another thread. + BlockedOn(BlockedOn<'me>), + + /// Blocking resulted in a cycle. + /// + /// There's another thread that is waiting on the current thread, + /// and blocking this thread on the other thread would result in a deadlock/cycle. Cycle, } +pub(crate) struct BlockedOn<'me>(Box>); + +struct BlockedOnInner<'me> { + dg: crate::sync::MutexGuard<'me, DependencyGraph>, + query_mutex_guard: SyncGuard<'me>, + database_key: DatabaseKeyIndex, + other_id: ThreadId, + thread_id: ThreadId, +} + +impl BlockedOn<'_> { + pub(crate) fn database_key(&self) -> DatabaseKeyIndex { + self.0.database_key + } + + pub(crate) fn wait_for(self, zalsa: &Zalsa) { + let BlockedOnInner { + dg, + query_mutex_guard, + database_key, + other_id, + thread_id, + } = *self.0; + + zalsa.event(&|| { + Event::new(EventKind::WillBlockOn { + other_thread_id: other_id, + database_key, + }) + }); + + tracing::debug!( + "block_on: thread {thread_id:?} is blocking on {database_key:?} in thread {other_id:?}", + ); + + let result = + DependencyGraph::block_on(dg, thread_id, database_key, other_id, query_mutex_guard); + + if result.is_panicked() { + // If the other thread panicked, then we consider this thread + // cancelled. The assumption is that the panic will be detected + // by the other thread and responded to appropriately. + Cancelled::PropagatedPanic.throw() + } + } +} + +impl std::fmt::Debug for BlockedOn<'_> { + fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + fmt.debug_struct("BlockedOn") + .field("database_key", &self.0.database_key) + .field("other_id", &self.0.other_id) + .field("thread_id", &self.0.thread_id) + .finish() + } +} + #[derive(Copy, Clone, Debug)] pub struct StampedValue { pub value: V, @@ -165,13 +236,12 @@ impl Runtime { /// /// If the thread `other_id` panics, then our thread is considered /// cancelled, so this function will panic with a `Cancelled` value. - pub(crate) fn block_on( - &self, - zalsa: &crate::zalsa::Zalsa, + pub(crate) fn block<'a>( + &'a self, database_key: DatabaseKeyIndex, other_id: ThreadId, - query_mutex_guard: QueryMutexGuard, - ) -> BlockResult { + query_mutex_guard: SyncGuard<'a>, + ) -> BlockResult<'a> { let thread_id = thread::current().id(); // Cycle in the same thread. if thread_id == other_id { @@ -185,28 +255,13 @@ impl Runtime { return BlockResult::Cycle; } - zalsa.event(&|| { - Event::new(EventKind::WillBlockOn { - other_thread_id: other_id, - database_key, - }) - }); - - tracing::debug!( - "block_on: thread {thread_id:?} is blocking on {database_key:?} in thread {other_id:?}" - ); - - let result = - DependencyGraph::block_on(dg, thread_id, database_key, other_id, query_mutex_guard); - - match result { - WaitResult::Completed => BlockResult::Completed, - - // If the other thread panicked, then we consider this thread - // cancelled. The assumption is that the panic will be detected - // by the other thread and responded to appropriately. - WaitResult::Panicked => Cancelled::PropagatedPanic.throw(), - } + BlockResult::BlockedOn(BlockedOn(Box::new(BlockedOnInner { + dg, + query_mutex_guard, + database_key, + other_id, + thread_id, + }))) } /// Invoked when this runtime completed computing `database_key` with diff --git a/src/runtime/dependency_graph.rs b/src/runtime/dependency_graph.rs index ad5f06b74..fd26c04fa 100644 --- a/src/runtime/dependency_graph.rs +++ b/src/runtime/dependency_graph.rs @@ -123,7 +123,7 @@ impl DependencyGraph { .unwrap_or_default(); for from_id in dependents { - self.unblock_runtime(from_id, wait_result.clone()); + self.unblock_runtime(from_id, wait_result); } } diff --git a/src/zalsa_local.rs b/src/zalsa_local.rs index a20dca6c1..95b022d68 100644 --- a/src/zalsa_local.rs +++ b/src/zalsa_local.rs @@ -7,7 +7,7 @@ use tracing::debug; use crate::accumulator::accumulated_map::{AccumulatedMap, AtomicInputAccumulatedValues}; use crate::active_query::QueryStack; -use crate::cycle::CycleHeads; +use crate::cycle::{CycleHeads, IterationCount}; use crate::durability::Durability; use crate::key::DatabaseKeyIndex; use crate::runtime::Stamp; @@ -99,7 +99,7 @@ impl ZalsaLocal { pub(crate) fn push_query( &self, database_key_index: DatabaseKeyIndex, - iteration_count: u32, + iteration_count: IterationCount, ) -> ActiveQueryGuard<'_> { let mut query_stack = self.query_stack.borrow_mut(); query_stack.push_new_query(database_key_index, iteration_count); @@ -355,7 +355,7 @@ pub(crate) struct QueryRevisions { /// Are the `cycle_heads` verified to not be provisional anymore? pub(super) verified_final: AtomicBool, - pub(super) iteration: u8, + pub(super) iteration: IterationCount, /// This result was computed based on provisional values from /// these cycle heads. The "cycle head" is the query responsible @@ -381,7 +381,7 @@ impl QueryRevisions { tracked_struct_ids: Default::default(), accumulated: Default::default(), accumulated_inputs: Default::default(), - iteration: 0, + iteration: IterationCount::initial(), verified_final: AtomicBool::new(false), cycle_heads: CycleHeads::initial(query), } diff --git a/tests/backtrace.rs b/tests/backtrace.rs index 74990663e..c64fd8ae3 100644 --- a/tests/backtrace.rs +++ b/tests/backtrace.rs @@ -106,7 +106,7 @@ fn backtrace_works() { at tests/backtrace.rs:30 1: query_cycle(Id(2)) at tests/backtrace.rs:43 - cycle heads: query_cycle(Id(2)) -> 0 + cycle heads: query_cycle(Id(2)) -> IterationCount(0) 2: query_f(Id(2)) at tests/backtrace.rs:38 "#]] @@ -117,9 +117,9 @@ fn backtrace_works() { query stacktrace: 0: query_e(Id(3)) -> (R1, Durability::LOW) at tests/backtrace.rs:30 - 1: query_cycle(Id(3)) -> (R1, Durability::HIGH, iteration = 0) + 1: query_cycle(Id(3)) -> (R1, Durability::HIGH, iteration = IterationCount(0)) at tests/backtrace.rs:43 - cycle heads: query_cycle(Id(3)) -> 0 + cycle heads: query_cycle(Id(3)) -> IterationCount(0) 2: query_f(Id(3)) -> (R1, Durability::HIGH) at tests/backtrace.rs:38 "#]] diff --git a/tests/cycle.rs b/tests/cycle.rs index 24dd6e707..b906b690b 100644 --- a/tests/cycle.rs +++ b/tests/cycle.rs @@ -1017,7 +1017,7 @@ fn repeat_provisional_query() { "salsa_event(WillExecute { database_key: min_iterate(Id(0)) })", "salsa_event(WillExecute { database_key: min_panic(Id(1)) })", "salsa_event(WillExecute { database_key: min_panic(Id(2)) })", - "salsa_event(WillIterateCycle { database_key: min_iterate(Id(0)), iteration_count: 1, fell_back: false })", + "salsa_event(WillIterateCycle { database_key: min_iterate(Id(0)), iteration_count: IterationCount(1), fell_back: false })", "salsa_event(WillExecute { database_key: min_panic(Id(1)) })", "salsa_event(WillExecute { database_key: min_panic(Id(2)) })", ]"#]]); diff --git a/tests/cycle_output.rs b/tests/cycle_output.rs index 49340ea46..672ce807e 100644 --- a/tests/cycle_output.rs +++ b/tests/cycle_output.rs @@ -194,13 +194,13 @@ fn revalidate_with_change_after_output_read() { "salsa_event(WillDiscardStaleOutput { execute_key: query_a(Id(0)), output_key: Output(Id(402)) })", "salsa_event(DidDiscard { key: Output(Id(402)) })", "salsa_event(DidDiscard { key: read_value(Id(402)) })", - "salsa_event(WillIterateCycle { database_key: query_b(Id(0)), iteration_count: 1, fell_back: false })", + "salsa_event(WillIterateCycle { database_key: query_b(Id(0)), iteration_count: IterationCount(1), fell_back: false })", "salsa_event(WillExecute { database_key: query_a(Id(0)) })", "salsa_event(WillExecute { database_key: read_value(Id(403g1)) })", - "salsa_event(WillIterateCycle { database_key: query_b(Id(0)), iteration_count: 2, fell_back: false })", + "salsa_event(WillIterateCycle { database_key: query_b(Id(0)), iteration_count: IterationCount(2), fell_back: false })", "salsa_event(WillExecute { database_key: query_a(Id(0)) })", "salsa_event(WillExecute { database_key: read_value(Id(401g1)) })", - "salsa_event(WillIterateCycle { database_key: query_b(Id(0)), iteration_count: 3, fell_back: false })", + "salsa_event(WillIterateCycle { database_key: query_b(Id(0)), iteration_count: IterationCount(3), fell_back: false })", "salsa_event(WillExecute { database_key: query_a(Id(0)) })", "salsa_event(WillExecute { database_key: read_value(Id(402g1)) })", ]"#]]); diff --git a/tests/cycle_tracked.rs b/tests/cycle_tracked.rs index 7a653f81e..30bf513f1 100644 --- a/tests/cycle_tracked.rs +++ b/tests/cycle_tracked.rs @@ -163,7 +163,7 @@ fn main() { "WillExecute { database_key: cost_to_start(Id(401)) }", "WillCheckCancellation", "WillCheckCancellation", - "WillIterateCycle { database_key: cost_to_start(Id(403)), iteration_count: 1, fell_back: false }", + "WillIterateCycle { database_key: cost_to_start(Id(403)), iteration_count: IterationCount(1), fell_back: false }", "WillCheckCancellation", "WillCheckCancellation", "WillCheckCancellation", diff --git a/tests/cycle_tracked_own_input.rs b/tests/cycle_tracked_own_input.rs index cdfa43aa6..6bfbc97de 100644 --- a/tests/cycle_tracked_own_input.rs +++ b/tests/cycle_tracked_own_input.rs @@ -115,7 +115,7 @@ fn main() { "WillExecute { database_key: infer_type_param(Id(400)) }", "WillCheckCancellation", "DidInternValue { key: Class(Id(c00)), revision: R2 }", - "WillIterateCycle { database_key: infer_class(Id(0)), iteration_count: 1, fell_back: false }", + "WillIterateCycle { database_key: infer_class(Id(0)), iteration_count: IterationCount(1), fell_back: false }", "WillCheckCancellation", "WillExecute { database_key: infer_type_param(Id(400)) }", "WillCheckCancellation", From 7764e21b0e00409095306c6f15b292d1e088b94d Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 31 May 2025 15:52:02 +0200 Subject: [PATCH 21/26] Cargo fmt --- src/active_query.rs | 2 +- src/function/execute.rs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/src/active_query.rs b/src/active_query.rs index 6ee54606d..0d4b74c81 100644 --- a/src/active_query.rs +++ b/src/active_query.rs @@ -204,7 +204,7 @@ impl ActiveQuery { mem::take(accumulated), mem::take(tracked_struct_ids), mem::take(cycle_heads), - iteration_count + iteration_count, ); let accumulated_inputs = AtomicInputAccumulatedValues::new(accumulated_inputs); diff --git a/src/function/execute.rs b/src/function/execute.rs index 0c1cfc009..b2166d8cb 100644 --- a/src/function/execute.rs +++ b/src/function/execute.rs @@ -74,7 +74,9 @@ where // Cycle participants that don't have a fallback will be discarded in // `validate_provisional()`. let cycle_heads = std::mem::take(cycle_heads); - let active_query = db.zalsa_local().push_query(database_key_index, IterationCount::initial()); + let active_query = db + .zalsa_local() + .push_query(database_key_index, IterationCount::initial()); new_value = C::cycle_initial(db, C::id_to_input(db, id)); revisions = active_query.pop(); // We need to set `cycle_heads` and `verified_final` because it needs to propagate to the callers. From b7105f8abfb70bc7000b595cda93a42f6997dcf8 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 31 May 2025 16:05:18 +0200 Subject: [PATCH 22/26] clippy --- src/zalsa_local.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/zalsa_local.rs b/src/zalsa_local.rs index b46890d43..ccb82e79b 100644 --- a/src/zalsa_local.rs +++ b/src/zalsa_local.rs @@ -491,7 +491,7 @@ impl QueryRevisions { } /// Updates the iteration count if this query has any cycle heads. Otherwise it's a no-op. - pub(crate) const fn update_iteration_count(&mut self, iteration_count: IterationCount) { + pub(crate) fn update_iteration_count(&mut self, iteration_count: IterationCount) { if let Some(extra) = &mut self.extra.0 { extra.iteration = iteration_count } From 30f6eb4b62fa0491cff41eaff2d5a07aadcb27b2 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sat, 31 May 2025 21:59:20 +0200 Subject: [PATCH 23/26] Fix hang if cycle initial panics --- src/function.rs | 4 +-- src/function/fetch.rs | 6 ++--- src/function/maybe_changed_after.rs | 10 +++---- src/function/memo.rs | 18 ++++++------- src/function/sync.rs | 10 +++---- src/ingredient.rs | 19 ++----------- src/runtime.rs | 42 ++++++++++++++--------------- 7 files changed, 46 insertions(+), 63 deletions(-) diff --git a/src/function.rs b/src/function.rs index 42090d7ff..34de5e8df 100644 --- a/src/function.rs +++ b/src/function.rs @@ -285,8 +285,8 @@ where /// Attempts to claim `key_index`, returning `false` if a cycle occurs. fn wait_for<'me>(&'me self, zalsa: &'me Zalsa, key_index: Id) -> WaitForResult<'me> { match self.sync_table.try_claim(zalsa, key_index) { - ClaimResult::BlockedOn(blocked_on) => WaitForResult::running(blocked_on), - ClaimResult::Cycle => WaitForResult::Cycle, + ClaimResult::Running(blocked_on) => WaitForResult::Running(blocked_on), + ClaimResult::Cycle { same_thread } => WaitForResult::Cycle { same_thread }, ClaimResult::Claimed(_) => WaitForResult::Available, } } diff --git a/src/function/fetch.rs b/src/function/fetch.rs index 065fa3432..d392c23d4 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -121,8 +121,8 @@ where let database_key_index = self.database_key_index(id); // Try to claim this query: if someone else has claimed it already, go back and start again. let claim_guard = match self.sync_table.try_claim(zalsa, id) { - ClaimResult::BlockedOn(blocked_on) => { - blocked_on.wait_for(zalsa); + ClaimResult::Running(blocked_on) => { + blocked_on.block_on(zalsa); let memo = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index); @@ -136,7 +136,7 @@ where } return None; } - ClaimResult::Cycle => { + ClaimResult::Cycle { .. } => { // check if there's a provisional value for this query // Note we don't `validate_may_be_provisional` the memo here as we want to reuse an // existing provisional memo if it exists diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index ca28eaee9..1121ca172 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -104,11 +104,11 @@ where let database_key_index = self.database_key_index(key_index); let _claim_guard = match self.sync_table.try_claim(zalsa, key_index) { - ClaimResult::BlockedOn(blocked_on) => { - blocked_on.wait_for(zalsa); + ClaimResult::Running(blocked_on) => { + blocked_on.block_on(zalsa); return None; } - ClaimResult::Cycle => match C::CYCLE_STRATEGY { + ClaimResult::Cycle { .. } => match C::CYCLE_STRATEGY { CycleRecoveryStrategy::Panic => UnexpectedCycle::throw(), CycleRecoveryStrategy::FallbackImmediate => { return Some(VerifyResult::unchanged()); @@ -343,9 +343,9 @@ where match ingredient.wait_for(zalsa, cycle_head.database_key_index.key_index()) { - WaitForResult::Running(_) | WaitForResult::Available => None, - WaitForResult::Cycle => ingredient + WaitForResult::Cycle { same_thread: false } => ingredient .iteration(zalsa, cycle_head.database_key_index.key_index()), + _ => None, } }) == Some(cycle_head.iteration_count) diff --git a/src/function/memo.rs b/src/function/memo.rs index 3386d9c71..0e349524a 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -9,7 +9,7 @@ use crate::hash::FxHashSet; use crate::ingredient::{Ingredient, WaitForResult}; use crate::key::DatabaseKeyIndex; use crate::revision::AtomicRevision; -use crate::runtime::BlockedOn; +use crate::runtime::Running; use crate::sync::atomic::Ordering; use crate::table::memo::MemoTableWithTypesMut; use crate::zalsa::{MemoIngredientIndex, Zalsa}; @@ -185,6 +185,7 @@ impl Memo { #[inline(never)] fn block_on_heads_cold(zalsa: &Zalsa, heads: &CycleHeads) -> bool { + let _entered = tracing::debug_span!("block_on_heads").entered(); let mut cycle_heads = TryClaimCycleHeadsIter::new(zalsa, heads); let mut all_cycles = true; @@ -213,6 +214,7 @@ impl Memo { /// Unlike `block_on_heads`, this code does not block on any cycle head. Instead it returns `false` if /// claiming all cycle heads failed because one of them is running on another thread. pub(super) fn try_claim_heads(&self, zalsa: &Zalsa, zalsa_local: &ZalsaLocal) -> bool { + let _entered = tracing::debug_span!("try_claim_heads").entered(); if self.all_cycles_on_stack(zalsa_local) { return true; } @@ -331,14 +333,14 @@ pub(super) enum TryClaimHeadsResult<'me> { } pub(super) struct RunningCycleHead<'me> { - blocked_on: BlockedOn<'me>, + inner: Running<'me>, ingredient: &'me dyn Ingredient, } impl<'a> RunningCycleHead<'a> { fn block_on(self, cycle_heads: &mut TryClaimCycleHeadsIter<'a>) { - let key_index = self.blocked_on.database_key().key_index(); - self.blocked_on.wait_for(cycle_heads.zalsa); + let key_index = self.inner.database_key().key_index(); + self.inner.block_on(cycle_heads.zalsa); cycle_heads.queue_ingredient_heads(self.ingredient, key_index); } @@ -407,19 +409,17 @@ impl<'me> Iterator for TryClaimCycleHeadsIter<'me> { } CycleHeadKind::Provisional => { match ingredient.wait_for(self.zalsa, head_key_index) { - WaitForResult::Cycle => { + WaitForResult::Cycle { .. } => { // We hit a cycle blocking on the cycle head; this means this query actively // participates in the cycle and some other query is blocked on this thread. tracing::debug!("Waiting for {head:?} results in a cycle"); Some(TryClaimHeadsResult::Cycle) } WaitForResult::Running(running) => { - tracing::debug!( - "Ingredient {head:?} is running: {running:?}, blocking on it" - ); + tracing::debug!("Ingredient {head:?} is running: {running:?}"); Some(TryClaimHeadsResult::Running(RunningCycleHead { - blocked_on: running.into_blocked_on(), + inner: running, ingredient, })) } diff --git a/src/function/sync.rs b/src/function/sync.rs index e94c40fba..28f088af4 100644 --- a/src/function/sync.rs +++ b/src/function/sync.rs @@ -1,7 +1,7 @@ use rustc_hash::FxHashMap; use crate::key::DatabaseKeyIndex; -use crate::runtime::{BlockResult, BlockedOn, WaitResult}; +use crate::runtime::{BlockResult, Running, WaitResult}; use crate::sync::thread::{self, ThreadId}; use crate::sync::Mutex; use crate::zalsa::Zalsa; @@ -18,9 +18,9 @@ pub(crate) struct SyncTable { pub(crate) enum ClaimResult<'a> { /// Can't claim the query because it is running on an other thread. - BlockedOn(BlockedOn<'a>), + Running(Running<'a>), /// Claiming the query results in a cycle. - Cycle, + Cycle { same_thread: bool }, /// Successfully claimed the query. Claimed(ClaimGuard<'a>), } @@ -61,8 +61,8 @@ impl SyncTable { id, write, ) { - BlockResult::BlockedOn(blocked_on) => ClaimResult::BlockedOn(blocked_on), - BlockResult::Cycle => ClaimResult::Cycle, + BlockResult::Running(blocked_on) => ClaimResult::Running(blocked_on), + BlockResult::Cycle { same_thread } => ClaimResult::Cycle { same_thread }, } } std::collections::hash_map::Entry::Vacant(vacant_entry) => { diff --git a/src/ingredient.rs b/src/ingredient.rs index 488289173..fdd0b5508 100644 --- a/src/ingredient.rs +++ b/src/ingredient.rs @@ -7,7 +7,7 @@ use crate::cycle::{ }; use crate::function::VerifyResult; use crate::plumbing::IngredientIndices; -use crate::runtime::BlockedOn; +use crate::runtime::Running; use crate::sync::Arc; use crate::table::memo::MemoTableTypes; use crate::table::Table; @@ -223,20 +223,5 @@ pub(crate) fn fmt_index(debug_name: &str, id: Id, fmt: &mut fmt::Formatter<'_>) pub enum WaitForResult<'me> { Running(Running<'me>), Available, - Cycle, -} - -impl<'a> WaitForResult<'a> { - pub(crate) const fn running(blocked_on: BlockedOn<'a>) -> Self { - WaitForResult::Running(Running(blocked_on)) - } -} - -#[derive(Debug)] -pub struct Running<'me>(BlockedOn<'me>); - -impl<'a> Running<'a> { - pub(crate) fn into_blocked_on(self) -> BlockedOn<'a> { - self.0 - } + Cycle { same_thread: bool }, } diff --git a/src/runtime.rs b/src/runtime.rs index 09aaab59e..5ed65bb63 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -42,25 +42,19 @@ pub(super) enum WaitResult { Panicked, } -impl WaitResult { - pub(crate) const fn is_panicked(self) -> bool { - matches!(self, WaitResult::Panicked) - } -} - #[derive(Debug)] pub(crate) enum BlockResult<'me> { /// The query is running on another thread. - BlockedOn(BlockedOn<'me>), + Running(Running<'me>), /// Blocking resulted in a cycle. /// - /// There's another thread that is waiting on the current thread, + /// The lock is hold by the current thread or there's another thread that is waiting on the current thread, /// and blocking this thread on the other thread would result in a deadlock/cycle. - Cycle, + Cycle { same_thread: bool }, } -pub(crate) struct BlockedOn<'me>(Box>); +pub struct Running<'me>(Box>); struct BlockedOnInner<'me> { dg: crate::sync::MutexGuard<'me, DependencyGraph>, @@ -70,12 +64,13 @@ struct BlockedOnInner<'me> { thread_id: ThreadId, } -impl BlockedOn<'_> { +impl Running<'_> { pub(crate) fn database_key(&self) -> DatabaseKeyIndex { self.0.database_key } - pub(crate) fn wait_for(self, zalsa: &Zalsa) { + /// Blocks on the other thread to complete the computation. + pub(crate) fn block_on(self, zalsa: &Zalsa) { let BlockedOnInner { dg, query_mutex_guard, @@ -98,18 +93,21 @@ impl BlockedOn<'_> { let result = DependencyGraph::block_on(dg, thread_id, database_key, other_id, query_mutex_guard); - if result.is_panicked() { - // If the other thread panicked, then we consider this thread - // cancelled. The assumption is that the panic will be detected - // by the other thread and responded to appropriately. - Cancelled::PropagatedPanic.throw() + match result { + WaitResult::Panicked => { + // If the other thread panicked, then we consider this thread + // cancelled. The assumption is that the panic will be detected + // by the other thread and responded to appropriately. + Cancelled::PropagatedPanic.throw() + } + WaitResult::Completed => {} } } } -impl std::fmt::Debug for BlockedOn<'_> { +impl std::fmt::Debug for Running<'_> { fn fmt(&self, fmt: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - fmt.debug_struct("BlockedOn") + fmt.debug_struct("Running") .field("database_key", &self.0.database_key) .field("other_id", &self.0.other_id) .field("thread_id", &self.0.thread_id) @@ -245,17 +243,17 @@ impl Runtime { let thread_id = thread::current().id(); // Cycle in the same thread. if thread_id == other_id { - return BlockResult::Cycle; + return BlockResult::Cycle { same_thread: true }; } let dg = self.dependency_graph.lock(); if dg.depends_on(other_id, thread_id) { tracing::debug!("block_on: cycle detected for {database_key:?} in thread {thread_id:?} on {other_id:?}"); - return BlockResult::Cycle; + return BlockResult::Cycle { same_thread: false }; } - BlockResult::BlockedOn(BlockedOn(Box::new(BlockedOnInner { + BlockResult::Running(Running(Box::new(BlockedOnInner { dg, query_mutex_guard, database_key, From 08b0fa590f67366a86663806123c37b5689f3339 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sun, 1 Jun 2025 10:01:05 +0200 Subject: [PATCH 24/26] Rename `cycle_head_kind` enable `cycle_a_t1_b_t2_fallback` shuttle test --- src/cycle.rs | 16 +++++-- src/function.rs | 47 +++++++++----------- src/function/maybe_changed_after.rs | 51 ++++++++++++---------- src/function/memo.rs | 14 +++--- src/ingredient.rs | 31 +++++++------ tests/parallel/cycle_a_t1_b_t2_fallback.rs | 2 +- 6 files changed, 88 insertions(+), 73 deletions(-) diff --git a/src/cycle.rs b/src/cycle.rs index 9d9747c39..2e73748d8 100644 --- a/src/cycle.rs +++ b/src/cycle.rs @@ -292,8 +292,18 @@ pub(crate) fn empty_cycle_heads() -> &'static CycleHeads { } #[derive(Debug, PartialEq, Eq)] -pub enum CycleHeadKind { - Provisional, - Final, +pub enum ProvisionalStatus { + Provisional { iteration: IterationCount }, + Final { iteration: IterationCount }, FallbackImmediate, } + +impl ProvisionalStatus { + pub(crate) const fn iteration(&self) -> Option { + match self { + ProvisionalStatus::Provisional { iteration } => Some(*iteration), + ProvisionalStatus::Final { iteration } => Some(*iteration), + ProvisionalStatus::FallbackImmediate => None, + } + } +} diff --git a/src/function.rs b/src/function.rs index 34de5e8df..f57b55221 100644 --- a/src/function.rs +++ b/src/function.rs @@ -7,8 +7,7 @@ pub(crate) use sync::SyncGuard; use crate::accumulator::accumulated_map::{AccumulatedMap, InputAccumulatedValues}; use crate::cycle::{ - empty_cycle_heads, CycleHeadKind, CycleHeads, CycleRecoveryAction, CycleRecoveryStrategy, - IterationCount, + empty_cycle_heads, CycleHeads, CycleRecoveryAction, CycleRecoveryStrategy, ProvisionalStatus, }; use crate::function::delete::DeletedEntries; use crate::function::sync::{ClaimResult, SyncTable}; @@ -248,32 +247,26 @@ where self.maybe_changed_after(db, input, revision, cycle_heads) } - /// True if the input `input` contains a memo that cites itself as a cycle head. - /// This indicates an intermediate value for a cycle that has not yet reached a fixed point. - fn cycle_head_kind( - &self, - zalsa: &Zalsa, - input: Id, - iteration: Option, - ) -> CycleHeadKind { - let is_provisional = self - .get_memo_from_table_for(zalsa, input, self.memo_ingredient_index(zalsa, input)) - .is_some_and(|memo| { - Some(memo.revisions.iteration()) != iteration - || !memo.revisions.verified_final.load(Ordering::Relaxed) - }); - if is_provisional { - CycleHeadKind::Provisional - } else if C::CYCLE_STRATEGY == CycleRecoveryStrategy::FallbackImmediate { - CycleHeadKind::FallbackImmediate + /// Returns `final` only if the memo has the `verified_final` flag set and the cycle recovery strategy is not `FallbackImmediate`. + /// + /// Otherwise, the value is still provisional. For both final and provisional, it also + /// returns the iteration in which this memo was created (always 0 except for cycle heads). + fn provisional_status(&self, zalsa: &Zalsa, input: Id) -> Option { + let memo = + self.get_memo_from_table_for(zalsa, input, self.memo_ingredient_index(zalsa, input))?; + + let iteration = memo.revisions.iteration(); + let verified_final = memo.revisions.verified_final.load(Ordering::Relaxed); + + Some(if verified_final { + if C::CYCLE_STRATEGY == CycleRecoveryStrategy::FallbackImmediate { + ProvisionalStatus::FallbackImmediate + } else { + ProvisionalStatus::Final { iteration } + } } else { - CycleHeadKind::Final - } - } - - fn iteration(&self, zalsa: &Zalsa, input: Id) -> Option { - self.get_memo_from_table_for(zalsa, input, self.memo_ingredient_index(zalsa, input)) - .map(|memo| memo.revisions.iteration()) + ProvisionalStatus::Provisional { iteration } + }) } fn cycle_heads<'db>(&self, zalsa: &'db Zalsa, input: Id) -> &'db CycleHeads { diff --git a/src/function/maybe_changed_after.rs b/src/function/maybe_changed_after.rs index 1121ca172..b3aac9e55 100644 --- a/src/function/maybe_changed_after.rs +++ b/src/function/maybe_changed_after.rs @@ -1,11 +1,10 @@ use crate::accumulator::accumulated_map::InputAccumulatedValues; use crate::cycle::{ - CycleHeadKind, CycleHeads, CycleRecoveryStrategy, IterationCount, UnexpectedCycle, + CycleHeads, CycleRecoveryStrategy, IterationCount, ProvisionalStatus, UnexpectedCycle, }; use crate::function::memo::Memo; use crate::function::sync::ClaimResult; use crate::function::{Configuration, IngredientImpl}; -use crate::ingredient::WaitForResult; use crate::key::DatabaseKeyIndex; use crate::sync::atomic::Ordering; use crate::zalsa::{MemoIngredientIndex, Zalsa, ZalsaDatabase}; @@ -266,29 +265,32 @@ where ); for cycle_head in memo.revisions.cycle_heads() { // Test if our cycle heads (with the same revision) are now finalized. - // It's important to also account for the revision 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 - // because its cycle head `b` is now finalized, but `b` never pulled `a` in the last iteration. - let kind = zalsa + let Some(kind) = zalsa .lookup_ingredient(cycle_head.database_key_index.ingredient_index()) - .cycle_head_kind( - zalsa, - cycle_head.database_key_index.key_index(), - Some(cycle_head.iteration_count), - ); + .provisional_status(zalsa, cycle_head.database_key_index.key_index()) + else { + return false; + }; match kind { - CycleHeadKind::Provisional => return false, - CycleHeadKind::Final => { + ProvisionalStatus::Provisional { .. } => return false, + ProvisionalStatus::Final { iteration } => { + // It's important to also account for the revision 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 + // 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; + } + // FIXME: We can ignore this, I just don't have a use-case for this. if C::CYCLE_STRATEGY == CycleRecoveryStrategy::FallbackImmediate { panic!("cannot mix `cycle_fn` and `cycle_result` in cycles") } } - CycleHeadKind::FallbackImmediate => match C::CYCLE_STRATEGY { + ProvisionalStatus::FallbackImmediate => match C::CYCLE_STRATEGY { CycleRecoveryStrategy::Panic => { // Queries without fallback are not considered when inside a cycle. return false; @@ -340,13 +342,16 @@ where // check if it 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()); - match ingredient.wait_for(zalsa, cycle_head.database_key_index.key_index()) - { - WaitForResult::Cycle { same_thread: false } => ingredient - .iteration(zalsa, cycle_head.database_key_index.key_index()), - _ => None, + if !wait_result.is_cycle_with_other_thread() { + return None; } + + let provisional_status = ingredient + .provisional_status(zalsa, cycle_head.database_key_index.key_index())?; + provisional_status.iteration() }) == Some(cycle_head.iteration_count) }) diff --git a/src/function/memo.rs b/src/function/memo.rs index 0e349524a..f21d00662 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -3,7 +3,7 @@ use std::fmt::{Debug, Formatter}; use std::mem::transmute; use std::ptr::NonNull; -use crate::cycle::{empty_cycle_heads, CycleHead, CycleHeadKind, CycleHeads}; +use crate::cycle::{empty_cycle_heads, CycleHead, CycleHeads, IterationCount, ProvisionalStatus}; use crate::function::{Configuration, IngredientImpl}; use crate::hash::FxHashSet; use crate::ingredient::{Ingredient, WaitForResult}; @@ -397,17 +397,21 @@ impl<'me> Iterator for TryClaimCycleHeadsIter<'me> { let ingredient = self .zalsa .lookup_ingredient(head_database_key.ingredient_index()); - // We don't care about the iteration. If it's final, we can go. - let cycle_head_kind = ingredient.cycle_head_kind(self.zalsa, head_key_index, None); + + let cycle_head_kind = ingredient + .provisional_status(self.zalsa, head_key_index) + .unwrap_or(ProvisionalStatus::Provisional { + iteration: IterationCount::initial(), + }); match cycle_head_kind { - CycleHeadKind::Final | CycleHeadKind::FallbackImmediate => { + ProvisionalStatus::Final { .. } | ProvisionalStatus::FallbackImmediate => { // This cycle is already finalized, so we don't need to wait on it; // keep looping through cycle heads. tracing::trace!("Dependent cycle head {head:?} has been finalized."); Some(TryClaimHeadsResult::Finalized) } - CycleHeadKind::Provisional => { + ProvisionalStatus::Provisional { .. } => { match ingredient.wait_for(self.zalsa, head_key_index) { WaitForResult::Cycle { .. } => { // We hit a cycle blocking on the cycle head; this means this query actively diff --git a/src/ingredient.rs b/src/ingredient.rs index fdd0b5508..c5c9515d2 100644 --- a/src/ingredient.rs +++ b/src/ingredient.rs @@ -3,7 +3,7 @@ use std::fmt; use crate::accumulator::accumulated_map::{AccumulatedMap, InputAccumulatedValues}; use crate::cycle::{ - empty_cycle_heads, CycleHeadKind, CycleHeads, CycleRecoveryStrategy, IterationCount, + empty_cycle_heads, CycleHeads, CycleRecoveryStrategy, IterationCount, ProvisionalStatus, }; use crate::function::VerifyResult; use crate::plumbing::IngredientIndices; @@ -69,20 +69,16 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync { cycle_heads: &mut CycleHeads, ) -> VerifyResult; - /// Is the value for `input` in this ingredient a cycle head that is still provisional? - fn cycle_head_kind( - &self, - zalsa: &Zalsa, - input: Id, - iteration: Option, - ) -> CycleHeadKind { - _ = (zalsa, input, iteration); - CycleHeadKind::Final - } - - fn iteration(&self, zalsa: &Zalsa, input: Id) -> Option { + /// Returns information about the current provisional status of `input`. + /// + /// 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); - None + Some(ProvisionalStatus::Final { + iteration: IterationCount::initial(), + }) } /// Returns the cycle heads for this ingredient. @@ -225,3 +221,10 @@ pub enum WaitForResult<'me> { Available, Cycle { same_thread: bool }, } + +impl WaitForResult<'_> { + /// Returns `true` if waiting for this input results in a cycle with another thread. + pub const fn is_cycle_with_other_thread(&self) -> bool { + matches!(self, WaitForResult::Cycle { same_thread: false }) + } +} diff --git a/tests/parallel/cycle_a_t1_b_t2_fallback.rs b/tests/parallel/cycle_a_t1_b_t2_fallback.rs index 917eace4f..92dcbeb4d 100644 --- a/tests/parallel/cycle_a_t1_b_t2_fallback.rs +++ b/tests/parallel/cycle_a_t1_b_t2_fallback.rs @@ -50,7 +50,7 @@ fn cycle_result_b(_db: &dyn KnobsDatabase) -> u32 { } #[test_log::test] -#[cfg(not(feature = "shuttle"))] // This test is currently failing. +// #[cfg(not(feature = "shuttle"))] // This test is currently failing. fn the_test() { use crate::sync::thread; use crate::Knobs; From 2979a54c72980f795ef822ee633510be4e1667a0 Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sun, 1 Jun 2025 10:12:13 +0200 Subject: [PATCH 25/26] Cleanup --- src/function/fetch.rs | 2 +- tests/parallel/cycle_a_t1_b_t2_fallback.rs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/function/fetch.rs b/src/function/fetch.rs index d392c23d4..8067ce4cb 100644 --- a/src/function/fetch.rs +++ b/src/function/fetch.rs @@ -129,7 +129,7 @@ where if let Some(memo) = memo { // This isn't strictly necessary, but if this is a provisional memo for an inner cycle, // await all outer cycle heads to give the thread driving it a chance to complete - // (we don't want multiple threads compute for the queries participating in the same cycle). + // (we don't want multiple threads competing for the queries participating in the same cycle). if memo.value.is_some() && memo.may_be_provisional() { memo.block_on_heads(zalsa, zalsa_local); } diff --git a/tests/parallel/cycle_a_t1_b_t2_fallback.rs b/tests/parallel/cycle_a_t1_b_t2_fallback.rs index 92dcbeb4d..8005a9c23 100644 --- a/tests/parallel/cycle_a_t1_b_t2_fallback.rs +++ b/tests/parallel/cycle_a_t1_b_t2_fallback.rs @@ -50,7 +50,6 @@ fn cycle_result_b(_db: &dyn KnobsDatabase) -> u32 { } #[test_log::test] -// #[cfg(not(feature = "shuttle"))] // This test is currently failing. fn the_test() { use crate::sync::thread; use crate::Knobs; From 0c8420eaa6309229e7f1a58d3c7c49e92f08079c Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Sun, 1 Jun 2025 10:29:54 +0200 Subject: [PATCH 26/26] Docs --- src/function.rs | 8 +++++++- src/function/memo.rs | 13 +++++-------- src/ingredient.rs | 8 ++++++-- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/src/function.rs b/src/function.rs index f57b55221..be88f7e39 100644 --- a/src/function.rs +++ b/src/function.rs @@ -275,7 +275,13 @@ where .unwrap_or(empty_cycle_heads()) } - /// Attempts to claim `key_index`, returning `false` if a cycle occurs. + /// Attempts to claim `key_index` without blocking. + /// + /// * [`WaitForResult::Running`] if the `key_index` is running on another thread. It's up to the caller to block on the other thread + /// to wait until the result becomes available. + /// * [`WaitForResult::Available`] It is (or at least was) possible to claim the `key_index` + /// * [`WaitResult::Cycle`] Claiming the `key_index` results in a cycle because it's on the current's thread query stack or + /// running on another thread that is blocked on this thread. fn wait_for<'me>(&'me self, zalsa: &'me Zalsa, key_index: Id) -> WaitForResult<'me> { match self.sync_table.try_claim(zalsa, key_index) { ClaimResult::Running(blocked_on) => WaitForResult::Running(blocked_on), diff --git a/src/function/memo.rs b/src/function/memo.rs index f21d00662..fcc85b885 100644 --- a/src/function/memo.rs +++ b/src/function/memo.rs @@ -366,14 +366,11 @@ impl<'a> TryClaimCycleHeadsIter<'a> { } fn queue_ingredient_heads(&mut self, ingredient: &dyn Ingredient, key: Id) { - // Recursively wait for all cycle heads that this head depends on. - // This is normally not necessary, because cycle heads are transitively added - // as query dependencies (they aggregate). The exception to this are queries - // that depend on a fixpoint initial value. They only depend on the fixpoint initial - // value but not on its dependencies because they aren't known yet. They're only known - // once the cycle completes but the cycle heads of the queries don't get updated. - // Because of that, recurse here to collect all cycle heads. - // This also ensures that if a query added new cycle heads, that they are awaited too. + // Recursively wait for all cycle heads that this head depends on. It's important + // that we fetch those from the updated memo because the cycle heads can change + // between iterations and new cycle heads can be added if a query depeonds on + // some cycle heads depending on a specific condition being met + // (`a` calls `b` and `c` in iteration 0 but `c` and `d` in iteration 1 or later). // IMPORTANT: It's critical that we get the cycle head from the latest memo // here, in case the memo has become part of another cycle (we need to block on that too!). self.queue.extend( diff --git a/src/ingredient.rs b/src/ingredient.rs index c5c9515d2..bfbcc2d30 100644 --- a/src/ingredient.rs +++ b/src/ingredient.rs @@ -88,9 +88,13 @@ pub trait Ingredient: Any + std::fmt::Debug + Send + Sync { } /// Invoked when the current thread needs to wait for a result for the given `key_index`. + /// This call doesn't block the current thread. Instead, it's up to the caller to block + /// in case `key_index` is [running](`WaitForResult::Running`) on another thread. /// - /// A return value of `true` indicates that a result is now available. A return value of - /// `false` means that a cycle was encountered; the waited-on query is either already claimed + /// A return value of [`WaitForResult::Available`] indicates that a result is now available. + /// A return value of [`WaitForResult::Running`] indicates that `key_index` is currently running + /// on an other thread, it's up to caller to block until the result becomes available if desired. + /// A return value of [`WaitForResult::Cycle`] means that a cycle was encountered; the waited-on query is either already claimed /// by the current thread, or by a thread waiting on the current thread. fn wait_for<'me>(&'me self, zalsa: &'me Zalsa, key_index: Id) -> WaitForResult<'me> { _ = (zalsa, key_index);