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

Use Result over catch_unwind for cancellation and cycle handling #572

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ description = "A generic framework for on-demand, incrementalized computation (e
arc-swap = "1"
crossbeam = "0.8"
dashmap = "6"
drop_bomb = "0.1.5"
hashlink = "0.9"
indexmap = "2"
append-only-vec = "0.1.5"
Expand Down
31 changes: 17 additions & 14 deletions benches/compare.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ pub struct Input {
}

#[salsa::tracked]
pub fn length(db: &dyn salsa::Database, input: Input) -> usize {
input.text(db).len()
pub fn length(db: &dyn salsa::Database, input: Input) -> salsa::Result<usize> {
Ok(input.text(db)?.len())
}

#[salsa::interned]
Expand All @@ -17,8 +17,11 @@ pub struct InternedInput<'db> {
}

#[salsa::tracked]
pub fn interned_length<'db>(db: &'db dyn salsa::Database, input: InternedInput<'db>) -> usize {
input.text(db).len()
pub fn interned_length<'db>(
db: &'db dyn salsa::Database,
input: InternedInput<'db>,
) -> salsa::Result<usize> {
Ok(input.text(db).len())
}

fn mutating_inputs(c: &mut Criterion) {
Expand All @@ -38,11 +41,11 @@ fn mutating_inputs(c: &mut Criterion) {
group.bench_function(BenchmarkId::new("mutating", n), |b| {
b.iter(|| {
let input = Input::new(&db, base_string.clone());
let actual_len = length(&db, input);
let actual_len = length(&db, input).unwrap();
assert_eq!(base_len, actual_len);

input.set_text(&mut db).to(string.clone());
let actual_len = length(&db, input);
let actual_len = length(&db, input).unwrap();
assert_eq!(new_len, actual_len);
})
});
Expand All @@ -60,30 +63,30 @@ fn inputs(c: &mut Criterion) {

group.bench_function(BenchmarkId::new("new", "InternedInput"), |b| {
b.iter(|| {
let input: InternedInput = InternedInput::new(&db, "hello, world!".to_owned());
interned_length(&db, input);
let input: InternedInput = InternedInput::new(&db, "hello, world!".to_owned()).unwrap();
interned_length(&db, input).unwrap();
})
});

group.bench_function(BenchmarkId::new("amortized", "InternedInput"), |b| {
let input = InternedInput::new(&db, "hello, world!".to_owned());
let _ = interned_length(&db, input);
let input = InternedInput::new(&db, "hello, world!".to_owned()).unwrap();
let _ = interned_length(&db, input).unwrap();

b.iter(|| interned_length(&db, input));
b.iter(|| interned_length(&db, input).unwrap());
});

group.bench_function(BenchmarkId::new("new", "Input"), |b| {
b.iter(|| {
let input = Input::new(&db, "hello, world!".to_owned());
length(&db, input);
length(&db, input).unwrap();
})
});

group.bench_function(BenchmarkId::new("amortized", "Input"), |b| {
let input = Input::new(&db, "hello, world!".to_owned());
let _ = length(&db, input);
let _ = length(&db, input).unwrap();

b.iter(|| length(&db, input));
b.iter(|| length(&db, input).unwrap());
});

group.finish();
Expand Down
12 changes: 6 additions & 6 deletions benches/incremental.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,14 +12,14 @@ struct Tracked<'db> {
}

#[salsa::tracked(return_ref)]
fn index<'db>(db: &'db dyn salsa::Database, input: Input) -> Vec<Tracked<'db>> {
(0..input.field(db)).map(|i| Tracked::new(db, i)).collect()
fn index<'db>(db: &'db dyn salsa::Database, input: Input) -> salsa::Result<Vec<Tracked<'db>>> {
(0..input.field(db)?).map(|i| Tracked::new(db, i)).collect()
}

#[salsa::tracked]
fn root(db: &dyn salsa::Database, input: Input) -> usize {
let index = index(db, input);
index.len()
fn root(db: &dyn salsa::Database, input: Input) -> salsa::Result<usize> {
let index = index(db, input)?;
Ok(index.len())
}

fn many_tracked_structs(criterion: &mut Criterion) {
Expand All @@ -41,7 +41,7 @@ fn many_tracked_structs(criterion: &mut Criterion) {
// Make a change, but fetch the result for the other input
input2.set_field(db).to(2);

let result = root(db, *input);
let result = root(db, *input).unwrap();

assert_eq!(result, 1_000);
},
Expand Down
8 changes: 4 additions & 4 deletions components/salsa-macro-rules/src/setup_input_struct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ macro_rules! setup_input_struct {
}

$(
$field_getter_vis fn $field_getter_id<'db, $Db>(self, db: &'db $Db) -> $zalsa::maybe_cloned_ty!($field_option, 'db, $field_ty)
$field_getter_vis fn $field_getter_id<'db, $Db>(self, db: &'db $Db) -> salsa::Result<$zalsa::maybe_cloned_ty!($field_option, 'db, $field_ty)>
where
// FIXME(rust-lang/rust#65991): The `db` argument *should* have the type `dyn Database`
$Db: ?Sized + $zalsa::Database,
Expand All @@ -151,12 +151,12 @@ macro_rules! setup_input_struct {
db.as_dyn_database(),
self,
$field_index,
);
$zalsa::maybe_clone!(
)?;
Ok($zalsa::maybe_clone!(
$field_option,
$field_ty,
&fields.$field_index,
)
))
}
)*

Expand Down
2 changes: 1 addition & 1 deletion components/salsa-macro-rules/src/setup_interned_struct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ macro_rules! setup_interned_struct {
}

impl<$db_lt> $Struct<$db_lt> {
pub fn $new_fn<$Db>(db: &$db_lt $Db, $($field_id: $field_ty),*) -> Self
pub fn $new_fn<$Db>(db: &$db_lt $Db, $($field_id: $field_ty),*) -> salsa::Result<Self>
where
// FIXME(rust-lang/rust#65991): The `db` argument *should* have the type `dyn Database`
$Db: ?Sized + salsa::Database,
Expand Down
26 changes: 13 additions & 13 deletions components/salsa-macro-rules/src/setup_tracked_fn.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ macro_rules! setup_tracked_fn {
$($input_id: $input_ty,)*
) -> salsa::plumbing::macro_if! {
if $return_ref {
&$db_lt $output_ty
salsa::Result<&$db_lt <$output_ty as salsa::plumbing::HasOutput>::Output>
} else {
$output_ty
}
Expand Down Expand Up @@ -156,7 +156,7 @@ macro_rules! setup_tracked_fn {

type Input<$db_lt> = ($($input_ty),*);

type Output<$db_lt> = $output_ty;
type Output<$db_lt> = <$output_ty as $zalsa::HasOutput>::Output;

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

Expand All @@ -173,7 +173,7 @@ macro_rules! setup_tracked_fn {
}
}

fn execute<$db_lt>($db: &$db_lt Self::DbView, ($($input_id),*): ($($input_ty),*)) -> Self::Output<$db_lt> {
fn execute<$db_lt>($db: &$db_lt Self::DbView, ($($input_id),*): ($($input_ty),*)) -> salsa::Result<Self::Output<$db_lt>> {
$inner_fn

$inner($db, $($input_id),*)
Expand All @@ -183,7 +183,7 @@ macro_rules! setup_tracked_fn {
db: &$db_lt dyn $Db,
cycle: &$zalsa::Cycle,
($($input_id),*): ($($input_ty),*)
) -> Self::Output<$db_lt> {
) -> salsa::Result<Self::Output<$db_lt>> {
$($cycle_recovery_fn)*(db, cycle, $($input_id),*)
}

Expand Down Expand Up @@ -231,11 +231,11 @@ macro_rules! setup_tracked_fn {
pub fn accumulated<$db_lt, A: salsa::Accumulator>(
$db: &$db_lt dyn $Db,
$($input_id: $input_ty,)*
) -> Vec<A> {
) -> salsa::Result<Vec<A>> {
use salsa::plumbing as $zalsa;
let key = $zalsa::macro_if! {
if $needs_interner {
$Configuration::intern_ingredient($db).intern_id($db.as_dyn_database(), ($($input_id),*))
$Configuration::intern_ingredient($db).intern_id($db.as_dyn_database(), ($($input_id),*))?
} else {
$zalsa::AsId::as_id(&($($input_id),*))
}
Expand All @@ -248,7 +248,7 @@ macro_rules! setup_tracked_fn {
pub fn specify<$db_lt>(
$db: &$db_lt dyn $Db,
$($input_id: $input_ty,)*
value: $output_ty,
value: <$Configuration as $zalsa::function::Configuration>::Output<$db_lt>,
) {
let key = $zalsa::AsId::as_id(&($($input_id),*));
$Configuration::fn_ingredient($db).specify_and_record(
Expand All @@ -271,21 +271,21 @@ macro_rules! setup_tracked_fn {
let result = $zalsa::macro_if! {
if $needs_interner {
{
let key = $Configuration::intern_ingredient($db).intern_id($db.as_dyn_database(), ($($input_id),*));
$Configuration::fn_ingredient($db).fetch($db, key)
let key = $Configuration::intern_ingredient($db).intern_id($db.as_dyn_database(), ($($input_id),*))?;
$Configuration::fn_ingredient($db).fetch($db, key)?
}
} else {
$Configuration::fn_ingredient($db).fetch($db, $zalsa::AsId::as_id(&($($input_id),*)))
$Configuration::fn_ingredient($db).fetch($db, $zalsa::AsId::as_id(&($($input_id),*)))?
}
};

$zalsa::macro_if! {
Ok($zalsa::macro_if! {
if $return_ref {
result
} else {
<$output_ty as std::clone::Clone>::clone(result)
<<$Configuration as $zalsa::function::Configuration>::Output<$db_lt> as std::clone::Clone>::clone(result)
}
}
})
})
}
};
Expand Down
10 changes: 5 additions & 5 deletions components/salsa-macro-rules/src/setup_tracked_struct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ macro_rules! setup_tracked_struct {
}

impl<$db_lt> $Struct<$db_lt> {
pub fn $new_fn<$Db>(db: &$db_lt $Db, $($field_id: $field_ty),*) -> Self
pub fn $new_fn<$Db>(db: &$db_lt $Db, $($field_id: $field_ty),*) -> salsa::Result<Self>
where
// FIXME(rust-lang/rust#65991): The `db` argument *should* have the type `dyn Database`
$Db: ?Sized + $zalsa::Database,
Expand All @@ -196,18 +196,18 @@ macro_rules! setup_tracked_struct {
}

$(
$field_getter_vis fn $field_getter_id<$Db>(self, db: &$db_lt $Db) -> $crate::maybe_cloned_ty!($field_option, $db_lt, $field_ty)
$field_getter_vis fn $field_getter_id<$Db>(self, db: &$db_lt $Db) -> salsa::Result<$crate::maybe_cloned_ty!($field_option, $db_lt, $field_ty)>
where
// FIXME(rust-lang/rust#65991): The `db` argument *should* have the type `dyn Database`
$Db: ?Sized + $zalsa::Database,
{
let db = db.as_dyn_database();
let fields = $Configuration::ingredient(db).field(db, self, $field_index);
$crate::maybe_clone!(
let fields = $Configuration::ingredient(db).field(db, self, $field_index)?;
Ok($crate::maybe_clone!(
$field_option,
$field_ty,
&fields.$field_index,
)
))
}
)*

Expand Down
3 changes: 2 additions & 1 deletion components/salsa-macros/src/tracked_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -274,7 +274,8 @@ impl Macro {
) -> syn::Result<()> {
if let Some(return_ref) = &args.return_ref {
if let syn::ReturnType::Type(_, t) = &mut sig.output {
**t = parse_quote!(& #db_lt #t)
**t =
parse_quote!(salsa::Result<&#db_lt <#t as salsa::plumbing::HasOutput>::Output>)
} else {
return Err(syn::Error::new_spanned(
return_ref,
Expand Down
8 changes: 5 additions & 3 deletions examples/calc/compile.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
use crate::{ir::SourceProgram, parser::parse_statements, type_check::type_check_program};

#[salsa::tracked]
pub fn compile(db: &dyn crate::Db, source_program: SourceProgram) {
let program = parse_statements(db, source_program);
type_check_program(db, program);
pub fn compile(db: &dyn crate::Db, source_program: SourceProgram) -> salsa::Result<()> {
let program = parse_statements(db, source_program)?;
type_check_program(db, program)?;

Ok(())
}
10 changes: 5 additions & 5 deletions examples/calc/ir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,18 +108,18 @@ pub struct Diagnostic {

impl Diagnostic {
#[cfg(test)]
pub fn render(&self, db: &dyn crate::Db, src: SourceProgram) -> String {
pub fn render(&self, db: &dyn crate::Db, src: SourceProgram) -> salsa::Result<String> {
use annotate_snippets::*;
let line_start = src.text(db)[..self.start].lines().count() + 1;
Renderer::plain()
let line_start = src.text(db)?[..self.start].lines().count() + 1;
Ok(Renderer::plain()
.render(
Level::Error.title(&self.message).snippet(
Snippet::source(src.text(db))
Snippet::source(src.text(db)?)
.line_start(line_start)
.origin("input")
.annotation(Level::Error.span(self.start..self.end).label("here")),
),
)
.to_string()
.to_string())
}
}
4 changes: 2 additions & 2 deletions examples/calc/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ mod type_check;
pub fn main() {
let db: CalcDatabaseImpl = Default::default();
let source_program = SourceProgram::new(&db, String::new());
compile::compile(&db, source_program);
let diagnostics = compile::compile::accumulated::<Diagnostic>(&db, source_program);
compile::compile(&db, source_program).unwrap();
let diagnostics = compile::compile::accumulated::<Diagnostic>(&db, source_program).unwrap();
eprintln!("{diagnostics:?}");
}
Loading
Loading