Merge branch 'master' into fixpoint

* master: (32 commits) fix `HashEqLike` for `Box<T>` bless ui tests expose `Database::unwind_if_revision_cancelled` Describe deadlock risk for assemble closure in interning Simplify Event construction Remove panicking paths from `maybe_changed_after` Remove PartialOrd, Ord implementations from keys Remove unnecessary `Option` around `Storage::zalsa_impl` Remove unreachable unwraps via typing Make `QueryEdge` an enum Type `QueryEdge` Encapsulate `DependencyIndex` Fix mutating benchmark benching more than just mutation Use proper setup for compare benches Fix benchmarks accidentally re-using the same database Cleanup Add self-ref test Split Lookup into two traits Implement self-referential interning Improve safety comments on function/fetch ...
salsa-rs · Jan 14, 2025 · 1dfa978 · 1dfa978
2 parents 5202579 + 2e7e28b
commit 1dfa978
Show file tree

Hide file tree

Showing 40 changed files with 931 additions and 487 deletions.
diff --git a/Cargo.toml b/Cargo.toml
@@ -21,7 +21,6 @@ rustc-hash = "2"
 salsa-macro-rules = { version = "0.1.0", path = "components/salsa-macro-rules" }
 salsa-macros = { path = "components/salsa-macros" }
 smallvec = "1"
-lazy_static = "1"
 rayon = "1.10.0"
 
 [dev-dependencies]

diff --git a/benches/compare.rs b/benches/compare.rs
@@ -1,4 +1,8 @@
-use codspeed_criterion_compat::{criterion_group, criterion_main, BenchmarkId, Criterion};
+use std::mem::transmute;
+
+use codspeed_criterion_compat::{
+    criterion_group, criterion_main, BatchSize, BenchmarkId, Criterion,
+};
 use salsa::Setter;
 
 #[salsa::input]
@@ -26,25 +30,30 @@ fn mutating_inputs(c: &mut Criterion) {
         codspeed_criterion_compat::measurement::WallTime,
     > = c.benchmark_group("Mutating Inputs");
 
-    let mut db = salsa::DatabaseImpl::default();
-
     for n in &[10, 20, 30] {
-        let base_string = "hello, world!".to_owned();
-        let base_len = base_string.len();
-
-        let string = base_string.clone().repeat(*n);
-        let new_len = string.len();
-
         group.bench_function(BenchmarkId::new("mutating", n), |b| {
-            b.iter(|| {
-                let input = Input::new(&db, base_string.clone());
-                let actual_len = length(&db, input);
-                assert_eq!(base_len, actual_len);
-
-                input.set_text(&mut db).to(string.clone());
-                let actual_len = length(&db, input);
-                assert_eq!(new_len, actual_len);
-            })
+            b.iter_batched_ref(
+                || {
+                    let db = salsa::DatabaseImpl::default();
+                    let base_string = "hello, world!".to_owned();
+                    let base_len = base_string.len();
+
+                    let string = base_string.clone().repeat(*n);
+                    let new_len = string.len();
+
+                    let input = Input::new(&db, base_string.clone());
+                    let actual_len = length(&db, input);
+                    assert_eq!(base_len, actual_len);
+
+                    (db, input, string, new_len)
+                },
+                |&mut (ref mut db, input, ref string, new_len)| {
+                    input.set_text(db).to(string.clone());
+                    let actual_len = length(db, input);
+                    assert_eq!(new_len, actual_len);
+                },
+                BatchSize::SmallInput,
+            )
         });
     }
 
@@ -56,34 +65,58 @@ fn inputs(c: &mut Criterion) {
         codspeed_criterion_compat::measurement::WallTime,
     > = c.benchmark_group("Mutating Inputs");
 
-    let db = salsa::DatabaseImpl::default();
-
     group.bench_function(BenchmarkId::new("new", "InternedInput"), |b| {
-        b.iter(|| {
-            let input: InternedInput = InternedInput::new(&db, "hello, world!".to_owned());
-            interned_length(&db, input);
-        })
+        b.iter_batched_ref(
+            salsa::DatabaseImpl::default,
+            |db| {
+                let input: InternedInput = InternedInput::new(db, "hello, world!".to_owned());
+                interned_length(db, input);
+            },
+            BatchSize::SmallInput,
+        )
     });
 
     group.bench_function(BenchmarkId::new("amortized", "InternedInput"), |b| {
-        let input = InternedInput::new(&db, "hello, world!".to_owned());
-        let _ = interned_length(&db, input);
-
-        b.iter(|| interned_length(&db, input));
+        b.iter_batched_ref(
+            || {
+                let db = salsa::DatabaseImpl::default();
+                // we can't pass this along otherwise, and the lifetime is generally informational
+                let input: InternedInput<'static> =
+                    unsafe { transmute(InternedInput::new(&db, "hello, world!".to_owned())) };
+                let _ = interned_length(&db, input);
+                (db, input)
+            },
+            |&mut (ref db, input)| {
+                interned_length(db, input);
+            },
+            BatchSize::SmallInput,
+        )
     });
 
     group.bench_function(BenchmarkId::new("new", "Input"), |b| {
-        b.iter(|| {
-            let input = Input::new(&db, "hello, world!".to_owned());
-            length(&db, input);
-        })
+        b.iter_batched_ref(
+            salsa::DatabaseImpl::default,
+            |db| {
+                let input = Input::new(db, "hello, world!".to_owned());
+                length(db, input);
+            },
+            BatchSize::SmallInput,
+        )
     });
 
     group.bench_function(BenchmarkId::new("amortized", "Input"), |b| {
-        let input = Input::new(&db, "hello, world!".to_owned());
-        let _ = length(&db, input);
-
-        b.iter(|| length(&db, input));
+        b.iter_batched_ref(
+            || {
+                let db = salsa::DatabaseImpl::default();
+                let input = Input::new(&db, "hello, world!".to_owned());
+                let _ = length(&db, input);
+                (db, input)
+            },
+            |&mut (ref db, input)| {
+                length(db, input);
+            },
+            BatchSize::SmallInput,
+        )
     });
 
     group.finish();

diff --git a/components/salsa-macro-rules/src/setup_input_struct.rs b/components/salsa-macro-rules/src/setup_input_struct.rs
@@ -124,6 +124,9 @@ macro_rules! setup_input_struct {
             }
 
             impl $zalsa::SalsaStructInDb for $Struct {
+                fn lookup_ingredient_index(aux: &dyn $zalsa::JarAux) -> core::option::Option<$zalsa::IngredientIndex> {
+                    aux.lookup_jar_by_type(&<$zalsa_struct::JarImpl<$Configuration>>::default())
+                }
             }
 
             impl $Struct {

diff --git a/components/salsa-macro-rules/src/setup_interned_struct.rs b/components/salsa-macro-rules/src/setup_interned_struct.rs
@@ -69,21 +69,29 @@ macro_rules! setup_interned_struct {
 
             /// Key to use during hash lookups. Each field is some type that implements `Lookup<T>`
             /// for the owned type. This permits interning with an `&str` when a `String` is required and so forth.
-            struct StructKey<$db_lt, $($indexed_ty: $zalsa::interned::Lookup<$field_ty>),*>(
+            #[derive(Hash)]
+            struct StructKey<$db_lt, $($indexed_ty),*>(
                 $($indexed_ty,)*
                 std::marker::PhantomData<&$db_lt ()>,
             );
 
-            impl<$db_lt, $($indexed_ty: $zalsa::interned::Lookup<$field_ty>),*> $zalsa::interned::Lookup<StructData<$db_lt>>
-                for StructKey<$db_lt, $($indexed_ty),*> {
+            impl<$db_lt, $($indexed_ty,)*> $zalsa::interned::HashEqLike<StructKey<$db_lt, $($indexed_ty),*>>
+                for StructData<$db_lt>
+                where
+                $($field_ty: $zalsa::interned::HashEqLike<$indexed_ty>),*
+                {
 
                 fn hash<H: std::hash::Hasher>(&self, h: &mut H) {
-                    $($zalsa::interned::Lookup::hash(&self.$field_index, &mut *h);)*
+                    $($zalsa::interned::HashEqLike::<$indexed_ty>::hash(&self.$field_index, &mut *h);)*
                 }
 
-                fn eq(&self, data: &StructData<$db_lt>) -> bool {
-                    ($($zalsa::interned::Lookup::eq(&self.$field_index, &data.$field_index) && )* true)
+                fn eq(&self, data: &StructKey<$db_lt, $($indexed_ty),*>) -> bool {
+                    ($($zalsa::interned::HashEqLike::<$indexed_ty>::eq(&self.$field_index, &data.$field_index) && )* true)
                 }
+            }
+
+            impl<$db_lt, $($indexed_ty: $zalsa::interned::Lookup<$field_ty>),*> $zalsa::interned::Lookup<StructData<$db_lt>>
+                for StructKey<$db_lt, $($indexed_ty),*> {
 
                 #[allow(unused_unit)]
                 fn into_owned(self) -> StructData<$db_lt> {
@@ -141,6 +149,9 @@ macro_rules! setup_interned_struct {
             }
 
             impl $zalsa::SalsaStructInDb for $Struct<'_> {
+                fn lookup_ingredient_index(aux: &dyn $zalsa::JarAux) -> core::option::Option<$zalsa::IngredientIndex> {
+                    aux.lookup_jar_by_type(&<$zalsa_struct::JarImpl<$Configuration>>::default())
+                }
             }
 
             unsafe impl $zalsa::Update for $Struct<'_> {
@@ -155,14 +166,17 @@ macro_rules! setup_interned_struct {
             }
 
             impl<$db_lt> $Struct<$db_lt> {
-                pub fn $new_fn<$Db>(db: &$db_lt $Db,  $($field_id: impl $zalsa::interned::Lookup<$field_ty>),*) -> Self
+                pub fn $new_fn<$Db, $($indexed_ty: $zalsa::interned::Lookup<$field_ty> + std::hash::Hash,)*>(db: &$db_lt $Db,  $($field_id: $indexed_ty),*) -> Self
                 where
                     // FIXME(rust-lang/rust#65991): The `db` argument *should* have the type `dyn Database`
                     $Db: ?Sized + salsa::Database,
+                    $(
+                        $field_ty: $zalsa::interned::HashEqLike<$indexed_ty>,
+                    )*
                 {
                     let current_revision = $zalsa::current_revision(db);
                     $Configuration::ingredient(db).intern(db.as_dyn_database(),
-                        StructKey::<$db_lt>($($field_id,)* std::marker::PhantomData::default()))
+                        StructKey::<$db_lt>($($field_id,)* std::marker::PhantomData::default()), |_, data| ($($zalsa::interned::Lookup::into_owned(data.$field_index),)*))
                 }
 
                 $(

diff --git a/components/salsa-macro-rules/src/setup_tracked_fn.rs b/components/salsa-macro-rules/src/setup_tracked_fn.rs
@@ -102,6 +102,9 @@ macro_rules! setup_tracked_fn {
                         $zalsa::IngredientCache::new();
 
                     impl $zalsa::SalsaStructInDb for $InternedData<'_> {
+                        fn lookup_ingredient_index(_aux: &dyn $zalsa::JarAux) -> core::option::Option<$zalsa::IngredientIndex> {
+                            None
+                        }
                     }
 
                     impl $zalsa::interned::Configuration for $Configuration {
@@ -207,7 +210,19 @@ macro_rules! setup_tracked_fn {
                     aux: &dyn $zalsa::JarAux,
                     first_index: $zalsa::IngredientIndex,
                 ) -> Vec<Box<dyn $zalsa::Ingredient>> {
+                    let struct_index = $zalsa::macro_if! {
+                        if $needs_interner {
+                            first_index.successor(0)
+                        } else {
+                            <$InternedData as $zalsa::SalsaStructInDb>::lookup_ingredient_index(aux)
+                                .expect(
+                                    "Salsa struct is passed as an argument of a tracked function, but its ingredient hasn't been added!"
+                                )
+                        }
+                    };
+
                     let fn_ingredient = <$zalsa::function::IngredientImpl<$Configuration>>::new(
+                        struct_index,
                         first_index,
                         aux,
                     );
@@ -227,6 +242,10 @@ macro_rules! setup_tracked_fn {
                         }
                     }
                 }
+
+                fn salsa_struct_type_id(&self) -> Option<core::any::TypeId> {
+                    None
+                }
             }
 
             #[allow(non_local_definitions)]
@@ -238,7 +257,7 @@ macro_rules! setup_tracked_fn {
                     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),*), |_, data| data)
                         } else {
                             $zalsa::AsId::as_id(&($($input_id),*))
                         }
@@ -274,7 +293,7 @@ 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),*));
+                            let key = $Configuration::intern_ingredient($db).intern_id($db.as_dyn_database(), ($($input_id),*), |_, data| data);
                             $Configuration::fn_ingredient($db).fetch($db, key)
                         }
                     } else {

diff --git a/components/salsa-macro-rules/src/setup_tracked_struct.rs b/components/salsa-macro-rules/src/setup_tracked_struct.rs
@@ -152,6 +152,9 @@ macro_rules! setup_tracked_struct {
             }
 
             impl $zalsa::SalsaStructInDb for $Struct<'_> {
+                fn lookup_ingredient_index(aux: &dyn $zalsa::JarAux) -> core::option::Option<$zalsa::IngredientIndex> {
+                    aux.lookup_jar_by_type(&<$zalsa_struct::JarImpl<$Configuration>>::default())
+                }
             }
 
             impl $zalsa::TrackedStructInDb for $Struct<'_> {

diff --git a/src/accumulator.rs b/src/accumulator.rs
@@ -54,6 +54,10 @@ impl<A: Accumulator> Jar for JarImpl<A> {
     ) -> Vec<Box<dyn Ingredient>> {
         vec![Box::new(<IngredientImpl<A>>::new(first_index))]
     }
+
+    fn salsa_struct_type_id(&self) -> Option<std::any::TypeId> {
+        None
+    }
 }
 
 pub struct IngredientImpl<A: Accumulator> {
@@ -62,7 +66,7 @@ pub struct IngredientImpl<A: Accumulator> {
 }
 
 impl<A: Accumulator> IngredientImpl<A> {
-    /// Find the accumulator ingrediate for `A` in the database, if any.
+    /// Find the accumulator ingredient for `A` in the database, if any.
     pub fn from_db<Db>(db: &Db) -> Option<&Self>
     where
         Db: ?Sized + Database,
@@ -101,7 +105,7 @@ impl<A: Accumulator> Ingredient for IngredientImpl<A> {
     fn maybe_changed_after(
         &self,
         _db: &dyn Database,
-        _input: Option<Id>,
+        _input: Id,
         _revision: Revision,
     ) -> VerifyResult {
         panic!("nothing should ever depend on an accumulator directly")
@@ -123,15 +127,15 @@ impl<A: Accumulator> Ingredient for IngredientImpl<A> {
         &self,
         _db: &dyn Database,
         _executor: DatabaseKeyIndex,
-        _output_key: Option<crate::Id>,
+        _output_key: crate::Id,
     ) {
     }
 
     fn remove_stale_output(
         &self,
         _db: &dyn Database,
         _executor: DatabaseKeyIndex,
-        _stale_output_key: Option<crate::Id>,
+        _stale_output_key: crate::Id,
     ) {
     }