remove created_at field from tracked structs

Author: ibraheemdev

src/tracked_struct.rs

@@ -271,15 +271,6 @@ where
     /// with different values.
     durability: Durability,
 
-    /// The revision in which the tracked struct was first created.
-    ///
-    /// Unlike `updated_at`, which gets bumped on every read,
-    /// `created_at` is updated whenever an untracked field is updated.
-    /// This is necessary to detect reused tracked struct ids _after_
-    /// they've been freed in a prior revision or tracked structs that have been updated
-    /// in-place because of a bad `Hash` implementation.
-    created_at: Revision,
-
     /// The revision when this tracked struct was last updated.
     /// This field also acts as a kind of "lock". Once it is equal
     /// to `Some(current_revision)`, the fields are locked and
@@ -400,7 +391,7 @@ where
                 tracing::trace!("Reuse tracked struct {id:?}", id = index);
                 // The struct already exists in the intern map.
                 zalsa_local.add_output(index);
-                self.update(zalsa, current_revision, id, &current_deps, fields);
+                let id = self.update(zalsa, current_revision, id, &current_deps, fields);
                 FromId::from_id(id)
             }
 
@@ -425,7 +416,6 @@ where
         fields: C::Fields<'db>,
     ) -> Id {
         let value = |_| Value {
-            created_at: current_revision,
             updated_at: OptionalAtomicRevision::new(Some(current_revision)),
             durability: current_deps.durability,
             // lifetime erase for storage
@@ -434,17 +424,11 @@ where
             memos: Default::default(),
         };
 
-        while let Some(id) = self.free_list.pop() {
+        if let Some(id) = self.free_list.pop() {
             // Increment the ID generation before reusing it, as if we have allocated a new
             // slot in the table.
-            //
-            // If the generation will wrap, we are forced to leak the slot. We reserve enough
-            // bits for the generation that this should not be a problem in practice.
-            let Some(generation) = id.generation().checked_add(1) else {
-                continue;
-            };
-
-            let id = id.with_generation(generation);
+            let id = id.with_generation(id.generation() + 1);
+
             return Self::data_raw(zalsa.table(), id).with_mut(|data_raw| {
                 let data_raw = data_raw.cast::<Value<C>>();
 
@@ -477,10 +461,10 @@ where
         &'db self,
         zalsa: &'db Zalsa,
         current_revision: Revision,
-        id: Id,
+        mut id: Id,
         current_deps: &StampedValue<()>,
         fields: C::Fields<'db>,
-    ) {
+    ) -> Id {
         let data_raw = Self::data_raw(zalsa.table(), id);
 
         // The protocol is:
@@ -547,7 +531,7 @@ where
         });
 
         if !locked {
-            return;
+            return id;
         }
 
         data_raw.with_mut(|data| {
@@ -570,22 +554,27 @@ where
                     ),
                     fields,
                 ) {
-                    // Consider this a new tracked-struct (even though it still uses the same id)
-                    // when any non-tracked field got updated.
-                    // This should be rare and only ever happen if there's a hash collision
-                    // which makes Salsa consider two tracked structs to still be the same
-                    // even though the fields are different.
-                    // See `tracked-struct-id-field-bad-hash` for more details.
-                    data.created_at = current_revision;
+                    // Consider this a new tracked-struct when any non-tracked field got updated.
+                    // This should be rare and only ever happen if there's a hash collision.
+                    //
+                    // Note that we hold the lock and have exclusive access to the tracked struct data,
+                    // so there should be no live instances of IDs from the previous generation. We clear
+                    // the memos and return a new ID here as if we have allocated a new slot.
+
+                    // SAFETY: We already set `updated_at` to `None` to acquire the lock.
+                    self.delete_entity(zalsa, id, true);
+
+                    id = id.with_generation(id.generation() + 1);
                 }
             }
             if current_deps.durability < data.durability {
                 data.revisions = C::new_revisions(current_deps.changed_at);
-                data.created_at = current_revision;
             }
             data.durability = current_deps.durability;
             let swapped_out = data.updated_at.swap(Some(current_revision));
             assert!(swapped_out.is_none());
+
+            id
         })
     }
 
@@ -609,7 +598,11 @@ where
     /// Using this method on an entity id that MAY be used in the current revision will lead to
     /// unspecified results (but not UB). See [`InternedIngredient::delete_index`] for more
     /// discussion and important considerations.
-    pub(crate) fn delete_entity(&self, zalsa: &Zalsa, id: Id) {
+    ///
+    /// # Safety
+    ///
+    /// If `locked == true` is passed, the `updated_at` field must already be set to `None`.
+    pub(crate) unsafe fn delete_entity(&self, zalsa: &Zalsa, id: Id, locked: bool) {
         zalsa.event(&|| {
             Event::new(crate::EventKind::DidDiscard {
                 key: self.database_key_index(id),
@@ -619,7 +612,8 @@ where
         let current_revision = zalsa.current_revision();
         let data_raw = Self::data_raw(zalsa.table(), id);
 
-        data_raw.with(|data| {
+        if !locked {
+            data_raw.with(|data| {
             let data = data.cast::<Value<C>>();
 
             // We want to set `updated_at` to `None`, signalling that other field values
@@ -639,6 +633,7 @@ where
                 }
             }
         });
+        }
 
         let mut memo_table = data_raw.with_mut(|data| {
             // SAFETY: We have acquired the write lock
@@ -767,15 +762,13 @@ where
 
     unsafe fn maybe_changed_after(
         &self,
-        db: &dyn Database,
-        input: Id,
-        revision: Revision,
+        _db: &dyn Database,
+        _input: Id,
+        _revision: Revision,
         _cycle_heads: &mut CycleHeads,
     ) -> VerifyResult {
-        let zalsa = db.zalsa();
-        let data = Self::data(zalsa.table(), input);
-
-        VerifyResult::changed_if(data.created_at > revision)
+        // Any change to a tracked struct results in a new ID generation.
+        VerifyResult::unchanged()
     }
 
     fn mark_validated_output(
@@ -799,7 +792,9 @@ where
         // `executor` creates a tracked struct `salsa_output_key`,
         // but it did not in the current revision.
         // In that case, we can delete `stale_output_key` and any data associated with it.
-        self.delete_entity(zalsa, stale_output_key);
+        //
+        // SAFETY: `delete_entity` is always sound to call with `locked == false`.
+        unsafe { self.delete_entity(zalsa, stale_output_key, false) };
     }
 
     fn debug_name(&self) -> &'static str {

tests/tracked-struct-id-field-bad-hash.rs

@@ -1,9 +1,10 @@
 //! Test for a tracked struct where an untracked field has a
 //! very poorly chosen hash impl (always returns 0).
-//! This demonstrates that the untracked fields on a struct
-//! can change values and yet the struct can have the same
-//! id (because struct ids are based on the *hash* of the
-//! untracked fields).
+//!
+//! This demonstrates that tracked struct ids will always change if
+//! untracked fields on a struct change values, because although struct
+//! ids are based on the *hash* of the untracked fields, ids are generational
+//! based on the field values.
 
 use salsa::{Database as Db, Setter};
 use test_log::test;
@@ -62,10 +63,6 @@ fn with_tracked<'db>(db: &'db dyn Db, tracked: MyTracked<'db>) -> bool {
 }
 
 #[test]
-// Because tracked struct IDs are generational, the ID of the second tracked struct ends
-// up being different than the ID of the first. However, this test still has precedence
-// and represents valid behavior for IDs that get leaked.
-#[ignore]
 fn dependent_query() {
     let mut db = salsa::DatabaseImpl::new();
 
@@ -76,10 +73,9 @@ fn dependent_query() {
     input.set_field(&mut db).to(false);
 
     // We now re-run the query that creates the tracked struct.
-    // Salsa will re-use the `MyTracked` struct from the previous revision
-    // because it thinks it is unchanged because of `BadHash`'s bad hash function.
-    // That's why Salsa updates the `MyTracked` struct in-place and the struct
-    // should be considered re-created even though it still has the same id.
+    //
+    // Salsa will re-use the `MyTracked` struct from the previous revision,
+    // but practically it has been re-created due to generational ids.
     let tracked = create_tracked(&db, input);
     assert!(!with_tracked(&db, tracked));
 }