Skip to content

Commit f0c7881

Browse files
VeykrilVeykril
committed
Allow immediately resolving cycles with a fallback instead of fixpoint
1 parent ab7ecb4 commit f0c7881

13 files changed

+155
-7
lines changed

Diff for: ‎components/salsa-macros/src/accumulator.rs

+1
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ impl AllowedOptions for Accumulator {
4040
const DB: bool = false;
4141
const CYCLE_FN: bool = false;
4242
const CYCLE_INITIAL: bool = false;
43+
const CYCLE_RESULT: bool = false;
4344
const LRU: bool = false;
4445
const CONSTRUCTOR_NAME: bool = false;
4546
const ID: bool = false;

Diff for: ‎components/salsa-macros/src/input.rs

+2
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ impl crate::options::AllowedOptions for InputStruct {
5555

5656
const CYCLE_INITIAL: bool = false;
5757

58+
const CYCLE_RESULT: bool = false;
59+
5860
const LRU: bool = false;
5961

6062
const CONSTRUCTOR_NAME: bool = true;

Diff for: ‎components/salsa-macros/src/interned.rs

+2
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,8 @@ impl crate::options::AllowedOptions for InternedStruct {
5555

5656
const CYCLE_INITIAL: bool = false;
5757

58+
const CYCLE_RESULT: bool = false;
59+
5860
const LRU: bool = false;
5961

6062
const CONSTRUCTOR_NAME: bool = true;

Diff for: ‎components/salsa-macros/src/options.rs

+23
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,11 @@ pub(crate) struct Options<A: AllowedOptions> {
6161
/// If this is `Some`, the value is the `<path>`.
6262
pub cycle_initial: Option<syn::Path>,
6363

64+
/// The `cycle_result = <path>` option is the result for non-fixpoint cycle.
65+
///
66+
/// If this is `Some`, the value is the `<path>`.
67+
pub cycle_result: Option<syn::Expr>,
68+
6469
/// The `data = <ident>` option is used to define the name of the data type for an interned
6570
/// struct.
6671
///
@@ -100,6 +105,7 @@ impl<A: AllowedOptions> Default for Options<A> {
100105
db_path: Default::default(),
101106
cycle_fn: Default::default(),
102107
cycle_initial: Default::default(),
108+
cycle_result: Default::default(),
103109
data: Default::default(),
104110
constructor_name: Default::default(),
105111
phantom: Default::default(),
@@ -123,6 +129,7 @@ pub(crate) trait AllowedOptions {
123129
const DB: bool;
124130
const CYCLE_FN: bool;
125131
const CYCLE_INITIAL: bool;
132+
const CYCLE_RESULT: bool;
126133
const LRU: bool;
127134
const CONSTRUCTOR_NAME: bool;
128135
const ID: bool;
@@ -274,6 +281,22 @@ impl<A: AllowedOptions> syn::parse::Parse for Options<A> {
274281
"`cycle_initial` option not allowed here",
275282
));
276283
}
284+
} else if ident == "cycle_result" {
285+
if A::CYCLE_RESULT {
286+
let _eq = Equals::parse(input)?;
287+
let expr = syn::Expr::parse(input)?;
288+
if let Some(old) = options.cycle_result.replace(expr) {
289+
return Err(syn::Error::new(
290+
old.span(),
291+
"option `cycle_result` provided twice",
292+
));
293+
}
294+
} else {
295+
return Err(syn::Error::new(
296+
ident.span(),
297+
"`cycle_result` option not allowed here",
298+
));
299+
}
277300
} else if ident == "data" {
278301
if A::DATA {
279302
let _eq = Equals::parse(input)?;

Diff for: ‎components/salsa-macros/src/tracked_fn.rs

+20-5
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ impl crate::options::AllowedOptions for TrackedFn {
4848

4949
const CYCLE_INITIAL: bool = true;
5050

51+
const CYCLE_RESULT: bool = true;
52+
5153
const LRU: bool = true;
5254

5355
const CONSTRUCTOR_NAME: bool = false;
@@ -201,25 +203,38 @@ impl Macro {
201203
fn cycle_recovery(&self) -> syn::Result<(TokenStream, TokenStream, TokenStream)> {
202204
// TODO should we ask the user to specify a struct that impls a trait with two methods,
203205
// rather than asking for two methods separately?
204-
match (&self.args.cycle_fn, &self.args.cycle_initial) {
205-
(Some(cycle_fn), Some(cycle_initial)) => Ok((
206+
match (
207+
&self.args.cycle_fn,
208+
&self.args.cycle_initial,
209+
&self.args.cycle_result,
210+
) {
211+
(Some(cycle_fn), Some(cycle_initial), None) => Ok((
206212
quote!((#cycle_fn)),
207213
quote!((#cycle_initial)),
208214
quote!(Fixpoint),
209215
)),
210-
(None, None) => Ok((
216+
(None, None, None) => Ok((
211217
quote!((salsa::plumbing::unexpected_cycle_recovery!)),
212218
quote!((salsa::plumbing::unexpected_cycle_initial!)),
213219
quote!(Panic),
214220
)),
215-
(Some(_), None) => Err(syn::Error::new_spanned(
221+
(Some(_), None, None) => Err(syn::Error::new_spanned(
216222
self.args.cycle_fn.as_ref().unwrap(),
217223
"must provide `cycle_initial` along with `cycle_fn`",
218224
)),
219-
(None, Some(_)) => Err(syn::Error::new_spanned(
225+
(None, Some(_), None) => Err(syn::Error::new_spanned(
220226
self.args.cycle_initial.as_ref().unwrap(),
221227
"must provide `cycle_fn` along with `cycle_initial`",
222228
)),
229+
(None, None, Some(cycle_result)) => Ok((
230+
quote!((salsa::plumbing::unexpected_cycle_recovery!)),
231+
quote!((#cycle_result)),
232+
quote!(FallbackImmediate),
233+
)),
234+
(_, _, Some(_)) => Err(syn::Error::new_spanned(
235+
self.args.cycle_initial.as_ref().unwrap(),
236+
"must provide either `cycle_result` or `cycle_fn` along with `cycle_initial`, not both",
237+
))
223238
}
224239
}
225240

Diff for: ‎components/salsa-macros/src/tracked_struct.rs

+2
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,8 @@ impl crate::options::AllowedOptions for TrackedStruct {
5050

5151
const CYCLE_INITIAL: bool = false;
5252

53+
const CYCLE_RESULT: bool = false;
54+
5355
const LRU: bool = false;
5456

5557
const CONSTRUCTOR_NAME: bool = true;

Diff for: ‎src/cycle.rs

+2
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,8 @@ pub enum CycleRecoveryStrategy {
8383
/// This choice is computed by the query's `cycle_recovery`
8484
/// function and initial value.
8585
Fixpoint,
86+
87+
FallbackImmediate,
8688
}
8789

8890
/// A "cycle head" is the query at which we encounter a cycle; that is, if A -> B -> C -> A, then A

Diff for: ‎src/function/execute.rs

+1-1
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ where
7272
// initial provisional value from there.
7373
let memo =
7474
self.get_memo_from_table_for(zalsa, id, memo_ingredient_index)
75-
.unwrap_or_else(|| panic!("{database_key_index:#?} is a cycle head, but no provisional memo found"));
75+
.unwrap_or_else(|| unreachable!("{database_key_index:#?} is a cycle head, but no provisional memo found"));
7676
debug_assert!(memo.may_be_provisional());
7777
memo.value.as_ref()
7878
};

Diff for: ‎src/function/maybe_changed_after.rs

+3
Original file line numberDiff line numberDiff line change
@@ -119,6 +119,9 @@ where
119119
stack,
120120
);
121121
}),
122+
CycleRecoveryStrategy::FallbackImmediate => {
123+
return Some(VerifyResult::unchanged());
124+
}
122125
CycleRecoveryStrategy::Fixpoint => {
123126
return Some(VerifyResult::Unchanged(
124127
InputAccumulatedValues::Empty,

Diff for: ‎src/function/memo.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,9 @@ impl<C: Configuration> IngredientImpl<C> {
113113
key: Id,
114114
) -> Option<C::Output<'db>> {
115115
match C::CYCLE_STRATEGY {
116-
CycleRecoveryStrategy::Fixpoint => Some(C::cycle_initial(db, C::id_to_input(db, key))),
116+
CycleRecoveryStrategy::Fixpoint | CycleRecoveryStrategy::FallbackImmediate => {
117+
Some(C::cycle_initial(db, C::id_to_input(db, key)))
118+
}
117119
CycleRecoveryStrategy::Panic => None,
118120
}
119121
}

Diff for: ‎tests/cycle_fallback_immediate.rs

+23
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
//! It is possible to omit the `cycle_fn`, only specifying `cycle_result` in which case
2+
//! an immediate fallback value is used as the cycle handling opposed to doing a fixpoint resolution.
3+
#[salsa::tracked]
4+
fn zero(_db: &dyn salsa::Database) -> u32 {
5+
0
6+
}
7+
8+
#[salsa::tracked(cycle_result=cycle_result)]
9+
fn one_o_one(db: &dyn salsa::Database) -> u32 {
10+
let val = one_o_one(db);
11+
val + 1
12+
}
13+
14+
fn cycle_result(_db: &dyn salsa::Database) -> u32 {
15+
100
16+
}
17+
18+
#[test_log::test]
19+
fn the_test() {
20+
let db = salsa::DatabaseImpl::default();
21+
22+
assert_eq!(one_o_one(&db), 101);
23+
}

Diff for: ‎tests/parallel/cycle_a_t1_b_t2_fallback.rs

+72
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
//! Test a specific cycle scenario:
2+
//!
3+
//! ```text
4+
//! Thread T1 Thread T2
5+
//! --------- ---------
6+
//! | |
7+
//! v |
8+
//! query_a() |
9+
//! ^ | v
10+
//! | +------------> query_b()
11+
//! | |
12+
//! +--------------------+
13+
//! ```
14+
15+
use crate::setup::{Knobs, KnobsDatabase};
16+
17+
const FALLBACK_A: u32 = 0b01;
18+
const FALLBACK_B: u32 = 0b10;
19+
const OFFSET_A: u32 = 0b0100;
20+
const OFFSET_B: u32 = 0b1000;
21+
22+
// Signal 1: T1 has entered `query_a`
23+
// Signal 2: T2 has entered `query_b`
24+
25+
#[salsa::tracked(cycle_result=cycle_result_a)]
26+
fn query_a(db: &dyn KnobsDatabase) -> u32 {
27+
db.signal(1);
28+
29+
// Wait for Thread T2 to enter `query_b` before we continue.
30+
db.wait_for(2);
31+
32+
query_b(db) | OFFSET_A
33+
}
34+
35+
#[salsa::tracked(cycle_result=cycle_result_b)]
36+
fn query_b(db: &dyn KnobsDatabase) -> u32 {
37+
// Wait for Thread T1 to enter `query_a` before we continue.
38+
db.wait_for(1);
39+
40+
db.signal(2);
41+
42+
query_a(db) | OFFSET_B
43+
}
44+
45+
fn cycle_result_a(_db: &dyn KnobsDatabase) -> u32 {
46+
FALLBACK_A
47+
}
48+
49+
fn cycle_result_b(_db: &dyn KnobsDatabase) -> u32 {
50+
FALLBACK_B
51+
}
52+
53+
#[test_log::test]
54+
fn the_test() {
55+
std::thread::scope(|scope| {
56+
let db_t1 = Knobs::default();
57+
let db_t2 = db_t1.clone();
58+
59+
let t1 = scope.spawn(move || query_a(&db_t1));
60+
let t2 = scope.spawn(move || query_b(&db_t2));
61+
62+
let (r_t1, r_t2) = (t1.join().unwrap(), t2.join().unwrap());
63+
64+
assert_eq!(
65+
(r_t1, r_t2),
66+
(
67+
FALLBACK_A | OFFSET_A | OFFSET_B,
68+
FALLBACK_B | OFFSET_A | OFFSET_B,
69+
)
70+
);
71+
});
72+
}

Diff for: ‎tests/parallel/main.rs

+1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
mod setup;
22

33
mod cycle_a_t1_b_t2;
4+
mod cycle_a_t1_b_t2_fallback;
45
mod cycle_ab_peeping_c;
56
mod cycle_nested_three_threads;
67
mod cycle_panic;

0 commit comments

Comments
 (0)