Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] fixpoint iteration support #603

Open
wants to merge 34 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
3d00ae3
add example test for fixpoint iteration
carljm Aug 28, 2024
31979b9
add a multi-symbol test
carljm Aug 28, 2024
35e236e
simplify test case
carljm Sep 21, 2024
90ea6e9
WIP: remove existing cycle handling tests for now
carljm Oct 8, 2024
f218e58
WIP: remove all existing cycle handling, add fixpoint options
carljm Oct 9, 2024
6ce61c6
WIP: added provisional value and cycle fields
carljm Oct 12, 2024
29d110e
rename to CycleRecoveryStrategy::Fixpoint
carljm Oct 17, 2024
adb6c77
WIP: rip out ProvisionalValue
carljm Oct 23, 2024
7f82b6d
WIP: working single-iteration with provisional memo
carljm Oct 23, 2024
a7be3d9
WIP: add count arg to cycle_recovery_fn
carljm Oct 23, 2024
6c6dd55
WIP: move insert-initial out into fetch_cold
carljm Oct 23, 2024
8974361
WIP: cycle-head iteration
carljm Oct 23, 2024
797655e
WIP: move loop into execute
carljm Oct 23, 2024
27742a2
WIP: delay storing memo
carljm Oct 23, 2024
4f4df72
WIP: remove ourself from cycle heads when done iterating
carljm Oct 23, 2024
315944e
WIP: working convergence and fallback
carljm Oct 23, 2024
28242a4
WIP: clippy and cleanup
carljm Oct 23, 2024
0be825a
WIP: improve comments and add a type annotation
carljm Oct 24, 2024
c3c84c4
WIP: don't allow cycle_fn with no_eq
carljm Oct 24, 2024
72dff5f
WIP: add tracing for cycle iteration
carljm Oct 24, 2024
6b44c92
WIP: fail fast if we get an evicted provisional value
carljm Oct 24, 2024
a029ef4
WIP: use FxHashSet::from_iter
carljm Oct 24, 2024
492ae1b
add tests, fix multiple-revision, lazy provisional value
carljm Oct 30, 2024
0f7d940
review feedback, more tracing
carljm Oct 31, 2024
5ef7a5f
fix multi-revision bug
carljm Oct 31, 2024
3093460
better fix for multi-revision bug
carljm Oct 31, 2024
7d9ec1c
test fixes
carljm Oct 31, 2024
aa4a731
pass inputs to cycle recovery functions
carljm Nov 1, 2024
67376f1
fixed cycle-unchanged test
carljm Nov 15, 2024
286b5fb
add TODO comments for some outstanding questions
carljm Nov 15, 2024
b2d4d92
add a test for the "AB peeping C" scenario
Nov 15, 2024
670f88b
another parallel test scenario
Nov 15, 2024
00acc56
WIP: removed cycle_ignore; nested cycles broken
carljm Dec 14, 2024
5202579
fixed all single-thread cycles; multi-thread still not working
carljm Dec 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions components/salsa-macro-rules/src/setup_tracked_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,9 @@ macro_rules! setup_tracked_fn {
// Path to the cycle recovery function to use.
cycle_recovery_fn: ($($cycle_recovery_fn:tt)*),

// Path to function to get the initial value to use for cycle recovery.
cycle_recovery_initial: ($($cycle_recovery_initial:tt)*),

// Name of cycle recovery strategy variant to use.
cycle_recovery_strategy: $cycle_recovery_strategy:ident,

Expand Down Expand Up @@ -155,15 +158,15 @@ macro_rules! setup_tracked_fn {

const CYCLE_STRATEGY: $zalsa::CycleRecoveryStrategy = $zalsa::CycleRecoveryStrategy::$cycle_recovery_strategy;

fn should_backdate_value(
fn values_equal(
old_value: &Self::Output<'_>,
new_value: &Self::Output<'_>,
) -> bool {
$zalsa::macro_if! {
if $no_eq {
false
} else {
$zalsa::should_backdate_value(old_value, new_value)
$zalsa::values_equal(old_value, new_value)
}
}
}
Expand All @@ -174,12 +177,17 @@ macro_rules! setup_tracked_fn {
$inner($db, $($input_id),*)
}

fn cycle_initial<$db_lt>(db: &$db_lt dyn $Db, ($($input_id),*): ($($input_ty),*)) -> Self::Output<$db_lt> {
$($cycle_recovery_initial)*(db, $($input_id),*)
}

fn recover_from_cycle<$db_lt>(
db: &$db_lt dyn $Db,
cycle: &$zalsa::Cycle,
value: &Self::Output<$db_lt>,
count: u32,
($($input_id),*): ($($input_ty),*)
) -> Self::Output<$db_lt> {
$($cycle_recovery_fn)*(db, cycle, $($input_id),*)
) -> $zalsa::CycleRecoveryAction<Self::Output<$db_lt>> {
$($cycle_recovery_fn)*(db, value, count, $($input_id),*)
}

fn id_to_input<$db_lt>(db: &$db_lt Self::DbView, key: salsa::Id) -> Self::Input<$db_lt> {
Expand Down
21 changes: 14 additions & 7 deletions components/salsa-macro-rules/src/unexpected_cycle_recovery.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,18 @@
// a macro because it can take a variadic number of arguments.
#[macro_export]
macro_rules! unexpected_cycle_recovery {
($db:ident, $cycle:ident, $($other_inputs:ident),*) => {
{
std::mem::drop($db);
std::mem::drop(($($other_inputs),*));
panic!("cannot recover from cycle `{:?}`", $cycle)
}
}
($db:ident, $value:ident, $count:ident, $($other_inputs:ident),*) => {{
std::mem::drop($db);
std::mem::drop(($($other_inputs),*));
panic!("cannot recover from cycle")
}};
}

#[macro_export]
macro_rules! unexpected_cycle_initial {
($db:ident, $($other_inputs:ident),*) => {{
std::mem::drop($db);
std::mem::drop(($($other_inputs),*));
panic!("no cycle initial value")
}};
}
3 changes: 2 additions & 1 deletion components/salsa-macros/src/accumulator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ impl AllowedOptions for Accumulator {
const SINGLETON: bool = false;
const DATA: bool = false;
const DB: bool = false;
const RECOVERY_FN: bool = false;
const CYCLE_FN: bool = false;
const CYCLE_INITIAL: bool = false;
const LRU: bool = false;
const CONSTRUCTOR_NAME: bool = false;
}
Expand Down
4 changes: 3 additions & 1 deletion components/salsa-macros/src/input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ impl crate::options::AllowedOptions for InputStruct {

const DB: bool = false;

const RECOVERY_FN: bool = false;
const CYCLE_FN: bool = false;

const CYCLE_INITIAL: bool = false;

const LRU: bool = false;

Expand Down
4 changes: 3 additions & 1 deletion components/salsa-macros/src/interned.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ impl crate::options::AllowedOptions for InternedStruct {

const DB: bool = false;

const RECOVERY_FN: bool = false;
const CYCLE_FN: bool = false;

const CYCLE_INITIAL: bool = false;

const LRU: bool = false;

Expand Down
44 changes: 35 additions & 9 deletions components/salsa-macros/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,15 @@ pub(crate) struct Options<A: AllowedOptions> {
/// If this is `Some`, the value is the `<path>`.
pub db_path: Option<syn::Path>,

/// The `recovery_fn = <path>` option is used to indicate the recovery function.
/// The `cycle_fn = <path>` option is used to indicate the cycle recovery function.
///
/// If this is `Some`, the value is the `<path>`.
pub recovery_fn: Option<syn::Path>,
pub cycle_fn: Option<syn::Path>,

/// The `cycle_initial = <path>` option is the initial value for cycle iteration.
///
/// If this is `Some`, the value is the `<path>`.
pub cycle_initial: Option<syn::Path>,

/// The `data = <ident>` option is used to define the name of the data type for an interned
/// struct.
Expand Down Expand Up @@ -79,7 +84,8 @@ impl<A: AllowedOptions> Default for Options<A> {
no_debug: Default::default(),
no_clone: Default::default(),
db_path: Default::default(),
recovery_fn: Default::default(),
cycle_fn: Default::default(),
cycle_initial: Default::default(),
data: Default::default(),
constructor_name: Default::default(),
phantom: Default::default(),
Expand All @@ -99,7 +105,8 @@ pub(crate) trait AllowedOptions {
const SINGLETON: bool;
const DATA: bool;
const DB: bool;
const RECOVERY_FN: bool;
const CYCLE_FN: bool;
const CYCLE_INITIAL: bool;
const LRU: bool;
const CONSTRUCTOR_NAME: bool;
}
Expand Down Expand Up @@ -207,20 +214,39 @@ impl<A: AllowedOptions> syn::parse::Parse for Options<A> {
"`db` option not allowed here",
));
}
} else if ident == "recovery_fn" {
if A::RECOVERY_FN {
} else if ident == "cycle_fn" {
if A::CYCLE_FN {
let _eq = Equals::parse(input)?;
let path = syn::Path::parse(input)?;
if let Some(old) = std::mem::replace(&mut options.cycle_fn, Some(path)) {
return Err(syn::Error::new(
old.span(),
"option `cycle_fn` provided twice",
));
}
} else {
return Err(syn::Error::new(
ident.span(),
"`cycle_fn` option not allowed here",
));
}
} else if ident == "cycle_initial" {
if A::CYCLE_INITIAL {
// TODO(carljm) should it be an error to give cycle_initial without cycle_fn,
// or should we just allow this to fall into potentially infinite iteration, if
// iteration never converges?
let _eq = Equals::parse(input)?;
let path = syn::Path::parse(input)?;
if let Some(old) = std::mem::replace(&mut options.recovery_fn, Some(path)) {
if let Some(old) = std::mem::replace(&mut options.cycle_initial, Some(path)) {
return Err(syn::Error::new(
old.span(),
"option `recovery_fn` provided twice",
"option `cycle_initial` provided twice",
));
}
} else {
return Err(syn::Error::new(
ident.span(),
"`recovery_fn` option not allowed here",
"`cycle_initial` option not allowed here",
));
}
} else if ident == "data" {
Expand Down
46 changes: 37 additions & 9 deletions components/salsa-macros/src/tracked_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ impl crate::options::AllowedOptions for TrackedFn {

const DB: bool = false;

const RECOVERY_FN: bool = true;
const CYCLE_FN: bool = true;

const CYCLE_INITIAL: bool = true;

const LRU: bool = true;

Expand Down Expand Up @@ -68,9 +70,20 @@ impl Macro {
let input_ids = self.input_ids(&item);
let input_tys = self.input_tys(&item)?;
let output_ty = self.output_ty(&db_lt, &item)?;
let (cycle_recovery_fn, cycle_recovery_strategy) = self.cycle_recovery();
let (cycle_recovery_fn, cycle_recovery_initial, cycle_recovery_strategy) =
self.cycle_recovery()?;
let is_specifiable = self.args.specify.is_some();
let no_eq = self.args.no_eq.is_some();
let no_eq = if let Some(token) = &self.args.no_eq {
if self.args.cycle_fn.is_some() {
return Err(syn::Error::new_spanned(
token,
"the `no_eq` option cannot be used with `cycle_fn`",
));
}
true
} else {
false
};

let mut inner_fn = item.clone();
inner_fn.vis = syn::Visibility::Inherited;
Expand Down Expand Up @@ -127,6 +140,7 @@ impl Macro {
output_ty: #output_ty,
inner_fn: #inner_fn,
cycle_recovery_fn: #cycle_recovery_fn,
cycle_recovery_initial: #cycle_recovery_initial,
cycle_recovery_strategy: #cycle_recovery_strategy,
is_specifiable: #is_specifiable,
no_eq: #no_eq,
Expand Down Expand Up @@ -160,14 +174,28 @@ impl Macro {

Ok(ValidFn { db_ident, db_path })
}
fn cycle_recovery(&self) -> (TokenStream, TokenStream) {
if let Some(recovery_fn) = &self.args.recovery_fn {
(quote!((#recovery_fn)), quote!(Fallback))
} else {
(
fn cycle_recovery(&self) -> syn::Result<(TokenStream, TokenStream, TokenStream)> {
// TODO should we ask the user to specify a struct that impls a trait with two methods,
// rather than asking for two methods separately?
match (&self.args.cycle_fn, &self.args.cycle_initial) {
(Some(cycle_fn), Some(cycle_initial)) => Ok((
quote!((#cycle_fn)),
quote!((#cycle_initial)),
quote!(Fixpoint),
)),
(None, None) => Ok((
quote!((salsa::plumbing::unexpected_cycle_recovery!)),
quote!((salsa::plumbing::unexpected_cycle_initial!)),
quote!(Panic),
)
)),
(Some(_), None) => Err(syn::Error::new_spanned(
self.args.cycle_fn.as_ref().unwrap(),
"must provide `cycle_initial` along with `cycle_fn`",
)),
(None, Some(_)) => Err(syn::Error::new_spanned(
self.args.cycle_initial.as_ref().unwrap(),
"must provide `cycle_fn` along with `cycle_initial`",
)),
}
}

Expand Down
4 changes: 3 additions & 1 deletion components/salsa-macros/src/tracked_struct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,9 @@ impl crate::options::AllowedOptions for TrackedStruct {

const DB: bool = false;

const RECOVERY_FN: bool = false;
const CYCLE_FN: bool = false;

const CYCLE_INITIAL: bool = false;

const LRU: bool = false;

Expand Down
7 changes: 6 additions & 1 deletion src/accumulator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use accumulated_map::AccumulatedMap;

use crate::{
cycle::CycleRecoveryStrategy,
function::VerifyResult,
ingredient::{fmt_index, Ingredient, Jar},
plumbing::JarAux,
zalsa::IngredientIndex,
Expand Down Expand Up @@ -102,10 +103,14 @@ impl<A: Accumulator> Ingredient for IngredientImpl<A> {
_db: &dyn Database,
_input: Option<Id>,
_revision: Revision,
) -> bool {
) -> VerifyResult {
panic!("nothing should ever depend on an accumulator directly")
}

fn is_verified_final<'db>(&'db self, _db: &'db dyn Database, _input: Id) -> bool {
false
}

fn cycle_recovery_strategy(&self) -> CycleRecoveryStrategy {
CycleRecoveryStrategy::Panic
}
Expand Down
Loading
Loading