Merge pull request #682 from Veykril/veykril/push-vntqlkpyzmqm

Fix Disambiguator- and IdentityMap hashing

Author: MichaReiser

src/tracked_struct.rs

@@ -187,7 +187,7 @@ impl Identity {
 }
 
 /// Stores the data that (almost) uniquely identifies a tracked struct.
-/// This includes the ingredient index of that struct type plus the hash of its id fields.
+/// This includes the ingredient index of that struct type plus the hash of its untracked fields.
 /// This is mapped to a disambiguator -- a value that starts as 0 but increments each round,
 /// allowing for multiple tracked structs with the same hash and ingredient_index
 /// created within the query to each have a unique id.
@@ -222,10 +222,7 @@ impl IdentityMap {
     pub(crate) fn insert(&mut self, key: Identity, id: Id) -> Option<Id> {
         use hashbrown::hash_map::RawEntryMut;
 
-        let eq_modulo_hash = |k: &Identity| {
-            k.ingredient_index == key.ingredient_index && k.disambiguator == key.disambiguator
-        };
-        let entry = self.map.raw_entry_mut().from_hash(key.hash, eq_modulo_hash);
+        let entry = self.map.raw_entry_mut().from_hash(key.hash, |k| *k == key);
         match entry {
             RawEntryMut::Occupied(mut occupied) => Some(occupied.insert(id)),
             RawEntryMut::Vacant(vacant) => {
@@ -236,12 +233,9 @@ impl IdentityMap {
     }
 
     pub(crate) fn get(&self, key: &Identity) -> Option<Id> {
-        let eq_modulo_hash = |k: &Identity| {
-            k.ingredient_index == key.ingredient_index && k.disambiguator == key.disambiguator
-        };
         self.map
             .raw_entry()
-            .from_hash(key.hash, eq_modulo_hash)
+            .from_hash(key.hash, |k| *k == *key)
             .map(|(_, &v)| v)
     }
 
@@ -318,8 +312,7 @@ impl DisambiguatorMap {
     pub(crate) fn disambiguate(&mut self, key: IdentityHash) -> Disambiguator {
         use hashbrown::hash_map::RawEntryMut;
 
-        let eq_modulo_hash = |k: &IdentityHash| k.ingredient_index == key.ingredient_index;
-        let entry = self.map.raw_entry_mut().from_hash(key.hash, eq_modulo_hash);
+        let entry = self.map.raw_entry_mut().from_hash(key.hash, |k| *k == key);
         let disambiguator = match entry {
             RawEntryMut::Occupied(occupied) => occupied.into_mut(),
             RawEntryMut::Vacant(vacant) => {
@@ -388,7 +381,7 @@ where
 
         let identity = Identity {
             hash: identity_hash.hash,
-            ingredient_index: self.ingredient_index,
+            ingredient_index: identity_hash.ingredient_index,
             disambiguator,
         };
 
@@ -845,3 +838,101 @@ where
         &self.syncs
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn disambiguate_map_works() {
+        let mut d = DisambiguatorMap::default();
+        // set up all 4 permutations of differing field values
+        let h1 = IdentityHash {
+            ingredient_index: IngredientIndex::from(0),
+            hash: 0,
+        };
+        let h2 = IdentityHash {
+            ingredient_index: IngredientIndex::from(1),
+            hash: 0,
+        };
+        let h3 = IdentityHash {
+            ingredient_index: IngredientIndex::from(0),
+            hash: 1,
+        };
+        let h4 = IdentityHash {
+            ingredient_index: IngredientIndex::from(1),
+            hash: 1,
+        };
+        assert_eq!(d.disambiguate(h1), Disambiguator(0));
+        assert_eq!(d.disambiguate(h1), Disambiguator(1));
+        assert_eq!(d.disambiguate(h2), Disambiguator(0));
+        assert_eq!(d.disambiguate(h2), Disambiguator(1));
+        assert_eq!(d.disambiguate(h3), Disambiguator(0));
+        assert_eq!(d.disambiguate(h3), Disambiguator(1));
+        assert_eq!(d.disambiguate(h4), Disambiguator(0));
+        assert_eq!(d.disambiguate(h4), Disambiguator(1));
+    }
+
+    #[test]
+    fn identity_map_works() {
+        let mut d = IdentityMap::default();
+        // set up all 8 permutations of differing field values
+        let i1 = Identity {
+            ingredient_index: IngredientIndex::from(0),
+            hash: 0,
+            disambiguator: Disambiguator(0),
+        };
+        let i2 = Identity {
+            ingredient_index: IngredientIndex::from(1),
+            hash: 0,
+            disambiguator: Disambiguator(0),
+        };
+        let i3 = Identity {
+            ingredient_index: IngredientIndex::from(0),
+            hash: 1,
+            disambiguator: Disambiguator(0),
+        };
+        let i4 = Identity {
+            ingredient_index: IngredientIndex::from(1),
+            hash: 1,
+            disambiguator: Disambiguator(0),
+        };
+        let i5 = Identity {
+            ingredient_index: IngredientIndex::from(0),
+            hash: 0,
+            disambiguator: Disambiguator(1),
+        };
+        let i6 = Identity {
+            ingredient_index: IngredientIndex::from(1),
+            hash: 0,
+            disambiguator: Disambiguator(1),
+        };
+        let i7 = Identity {
+            ingredient_index: IngredientIndex::from(0),
+            hash: 1,
+            disambiguator: Disambiguator(1),
+        };
+        let i8 = Identity {
+            ingredient_index: IngredientIndex::from(1),
+            hash: 1,
+            disambiguator: Disambiguator(1),
+        };
+        assert_eq!(d.insert(i1, Id::from_u32(0)), None);
+        assert_eq!(d.insert(i2, Id::from_u32(1)), None);
+        assert_eq!(d.insert(i3, Id::from_u32(2)), None);
+        assert_eq!(d.insert(i4, Id::from_u32(3)), None);
+        assert_eq!(d.insert(i5, Id::from_u32(4)), None);
+        assert_eq!(d.insert(i6, Id::from_u32(5)), None);
+        assert_eq!(d.insert(i7, Id::from_u32(6)), None);
+        assert_eq!(d.insert(i8, Id::from_u32(7)), None);
+
+        assert_eq!(d.get(&i1), Some(Id::from_u32(0)));
+        assert_eq!(d.get(&i2), Some(Id::from_u32(1)));
+        assert_eq!(d.get(&i3), Some(Id::from_u32(2)));
+        assert_eq!(d.get(&i4), Some(Id::from_u32(3)));
+        assert_eq!(d.get(&i5), Some(Id::from_u32(4)));
+        assert_eq!(d.get(&i6), Some(Id::from_u32(5)));
+        assert_eq!(d.get(&i7), Some(Id::from_u32(6)));
+        assert_eq!(d.get(&i8), Some(Id::from_u32(7)));
+    }
+}

src/update.rs

@@ -5,6 +5,8 @@ use std::{
     sync::Arc,
 };
 
+use rayon::iter::Either;
+
 use crate::Revision;
 
 /// This is used by the macro generated code.
@@ -348,6 +350,24 @@ where
     }
 }
 
+unsafe impl<L, R> Update for Either<L, R>
+where
+    L: Update,
+    R: Update,
+{
+    unsafe fn maybe_update(old_pointer: *mut Self, new_value: Self) -> bool {
+        let old_value = unsafe { &mut *old_pointer };
+        match (old_value, new_value) {
+            (Either::Left(old), Either::Left(new)) => L::maybe_update(old, new),
+            (Either::Right(old), Either::Right(new)) => R::maybe_update(old, new),
+            (old_value, new_value) => {
+                *old_value = new_value;
+                true
+            }
+        }
+    }
+}
+
 macro_rules! fallback_impl {
     ($($t:ty,)*) => {
         $(

tests/tracked_struct_disambiguates.rs

@@ -0,0 +1,107 @@
+//! Test that disambiguation works, that is when we have a revision where we track multiple structs
+//! that have the same hash, we can still differentiate between them.
+#![allow(warnings)]
+
+use std::hash::Hash;
+
+use rayon::iter::Either;
+use salsa::Setter;
+
+#[salsa::input]
+struct MyInput {
+    field: u32,
+}
+
+#[salsa::input]
+struct MyInputs {
+    field: Vec<MyInput>,
+}
+
+#[salsa::tracked]
+struct TrackedStruct<'db> {
+    field: DumbHashable,
+}
+
+#[salsa::tracked]
+struct TrackedStruct2<'db> {
+    field: DumbHashable,
+}
+
+#[derive(Debug, Clone)]
+pub struct DumbHashable {
+    field: u32,
+}
+
+impl Eq for DumbHashable {}
+impl PartialEq for DumbHashable {
+    fn eq(&self, other: &Self) -> bool {
+        self.field == other.field
+    }
+}
+
+// Force collisions, note that this is still a correct implementation wrt. PartialEq / Eq above
+// as keep the property that k1 == k2 -> hash(k1) == hash(k2)
+impl Hash for DumbHashable {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        (self.field % 3).hash(state);
+    }
+}
+
+fn alternate(
+    db: &dyn salsa::Database,
+    input: MyInput,
+) -> Either<TrackedStruct<'_>, TrackedStruct2<'_>> {
+    if input.field(db) % 2 == 0 {
+        Either::Left(TrackedStruct::new(
+            db,
+            DumbHashable {
+                field: input.field(db),
+            },
+        ))
+    } else {
+        Either::Right(TrackedStruct2::new(
+            db,
+            DumbHashable {
+                field: input.field(db),
+            },
+        ))
+    }
+}
+
+#[salsa::tracked]
+fn batch(
+    db: &dyn salsa::Database,
+    inputs: MyInputs,
+) -> Vec<Either<TrackedStruct<'_>, TrackedStruct2<'_>>> {
+    inputs
+        .field(db)
+        .iter()
+        .map(|input| alternate(db, input.clone()))
+        .collect()
+}
+
+#[test]
+fn execute() {
+    let mut db = salsa::DatabaseImpl::new();
+    let inputs = MyInputs::new(
+        &db,
+        (0..1024)
+            .into_iter()
+            .map(|i| MyInput::new(&db, i))
+            .collect(),
+    );
+    let trackeds = batch(&db, inputs);
+    for (id, tracked) in trackeds.into_iter().enumerate() {
+        assert_eq!(id % 2 == 0, tracked.is_left());
+        assert_eq!(id % 2 != 0, tracked.is_right());
+    }
+    for input in inputs.field(&db) {
+        let prev = input.field(&db);
+        input.set_field(&mut db).to(prev);
+    }
+    let trackeds = batch(&db, inputs);
+    for (id, tracked) in trackeds.into_iter().enumerate() {
+        assert_eq!(id % 2 == 0, tracked.is_left());
+        assert_eq!(id % 2 != 0, tracked.is_right());
+    }
+}