Smaller clean-ups

Author: MichaReiser

src/active_query.rs

@@ -225,7 +225,6 @@ impl ActiveQuery {
             active_tracked_structs,
             mem::take(cycle_heads),
             iteration_count,
-            false,
         );
 
         let revisions = QueryRevisions {
@@ -518,7 +517,7 @@ impl fmt::Display for Backtrace {
                         }
                         write!(
                             fmt,
-                            "{:?} -> {}",
+                            "{:?} -> iteration = {}",
                             head.database_key_index, head.iteration_count
                         )?;
                     }

src/cycle.rs

@@ -102,6 +102,14 @@ pub enum CycleRecoveryStrategy {
 pub struct CycleHead {
     pub(crate) database_key_index: DatabaseKeyIndex,
     pub(crate) iteration_count: AtomicIterationCount,
+
+    /// Marks a cycle head as removed within its `CycleHeads` container.
+    ///
+    /// Cycle heads are marked as removed when the memo from the last iteration (a provisional memo)
+    /// is used as the initial value for the next iteration. It's necessary to remove all but its own
+    /// head from the `CycleHeads` container, because the query might now depend on fewer cycles
+    /// (in case of conditional dependencies). However, we can't actually remove the cycle head
+    /// within `fetch_cold_cycle` because we only have a readonly memo. That's what `removed` is used for.
     #[cfg_attr(feature = "persistence", serde(skip))]
     removed: AtomicBool,
 }
@@ -130,6 +138,11 @@ impl IterationCount {
         self.0 == 0
     }
 
+    /// Iteration count reserved for panicked cycles.
+    ///
+    /// Using a special iteration count ensures that `validate_same_iteration` and `validate_provisional`
+    /// return `false` for queries depending on this panicked cycle, because the iteration count is guaranteed
+    /// to be different (which isn't guaranteed if the panicked memo uses [`Self::initial`]).
     pub(crate) const fn panicked() -> Self {
         Self(u8::MAX)
     }
@@ -150,7 +163,7 @@ impl IterationCount {
 
 impl std::fmt::Display for IterationCount {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        write!(f, "iteration={}", self.0)
+        self.0.fmt(f)
     }
 }
 
@@ -162,6 +175,10 @@ impl AtomicIterationCount {
         IterationCount(self.0.load(Ordering::Relaxed))
     }
 
+    pub(crate) fn load_mut(&mut self) -> IterationCount {
+        IterationCount(*self.0.get_mut())
+    }
+
     pub(crate) fn store(&self, value: IterationCount) {
         self.0.store(value.0, Ordering::Release);
     }
@@ -214,10 +231,13 @@ impl CycleHeads {
         self.0.is_empty()
     }
 
-    pub(crate) fn initial(database_key_index: DatabaseKeyIndex) -> Self {
+    pub(crate) fn initial(
+        database_key_index: DatabaseKeyIndex,
+        iteration_count: IterationCount,
+    ) -> Self {
         Self(thin_vec![CycleHead {
             database_key_index,
-            iteration_count: IterationCount::initial().into(),
+            iteration_count: iteration_count.into(),
             removed: false.into()
         }])
     }
@@ -233,17 +253,24 @@ impl CycleHeads {
             .any(|head| head.database_key_index == *value && !head.removed.load(Ordering::Relaxed))
     }
 
-    pub(crate) fn clear_except(&self, except: DatabaseKeyIndex) {
+    /// Removes all cycle heads except `except` by marking them as removed.
+    ///
+    /// Note that the heads aren't actually removed. They're only marked as removed and will be
+    /// skipped when iterating. This is because we might not have a mutable reference.
+    pub(crate) fn remove_all_except(&self, except: DatabaseKeyIndex) {
         for head in self.0.iter() {
             if head.database_key_index == except {
                 continue;
             }
 
-            // TODO: verify ordering
             head.removed.store(true, Ordering::Release);
         }
     }
 
+    /// Updates the iteration count for the head `cycle_head_index` to `new_iteration_count`.
+    ///
+    /// Unlike [`update_iteration_count`], this method takes a `&mut self` reference. It should
+    /// be preferred if possible, as it avoids atomic operations.
     pub(crate) fn update_iteration_count_mut(
         &mut self,
         cycle_head_index: DatabaseKeyIndex,
@@ -258,6 +285,9 @@ impl CycleHeads {
         }
     }
 
+    /// Updates the iteration count for the head `cycle_head_index` to `new_iteration_count`.
+    ///
+    /// Unlike [`update_iteration_count_mut`], this method takes a `&self` reference.
     pub(crate) fn update_iteration_count(
         &self,
         cycle_head_index: DatabaseKeyIndex,
@@ -282,6 +312,8 @@ impl CycleHeads {
     }
 
     pub(crate) fn insert(&mut self, head: &CycleHead) -> bool {
+        debug_assert!(!head.removed.load(Ordering::Relaxed));
+
         if let Some(existing) = self
             .0
             .iter_mut()
@@ -294,12 +326,9 @@ impl CycleHeads {
 
                 true
             } else {
-                let existing_count = existing.iteration_count.load();
+                let existing_count = existing.iteration_count.load_mut();
                 let head_count = head.iteration_count.load();
 
-                // It's now possible that a query can depend on different iteration counts of the same query
-                // This because some queries (inner) read the provisional value of the last iteration
-                // while outer queries read the value from the last iteration (which is i+1 if the head didn't converge).
                 assert_eq!(
                     existing_count, head_count,
                     "Can't merge cycle heads {:?} with different iteration counts ({existing_count:?}, {head_count:?})",
@@ -309,7 +338,6 @@ impl CycleHeads {
                 false
             }
         } else {
-            debug_assert!(!head.removed.load(Ordering::Relaxed));
             self.0.push(head.clone());
             true
         }

src/function.rs

@@ -410,7 +410,7 @@ where
     fn wait_for<'me>(&'me self, zalsa: &'me Zalsa, key_index: Id) -> WaitForResult<'me> {
         match self.sync_table.try_claim::<false>(zalsa, key_index) {
             ClaimResult::Running(blocked_on) => WaitForResult::Running(blocked_on),
-            ClaimResult::Cycle { inner: nested } => WaitForResult::Cycle { inner: nested },
+            ClaimResult::Cycle { inner } => WaitForResult::Cycle { inner },
             ClaimResult::Claimed(_) => WaitForResult::Available,
         }
     }

src/function/execute.rs

@@ -67,8 +67,6 @@ where
                 if let Some(cycle_heads) = completed_query.revisions.cycle_heads_mut() {
                     // Did the new result we got depend on our own provisional value, in a cycle?
                     if cycle_heads.contains(&database_key_index) {
-                        let id = database_key_index.key_index();
-
                         // Ignore the computed value, leave the fallback value there.
                         let memo = self
                             .get_memo_from_table_for(zalsa, id, memo_ingredient_index)
@@ -126,18 +124,16 @@ where
             self.diff_outputs(zalsa, database_key_index, old_memo, &completed_query);
         }
 
-        let new_memo = self.insert_memo(
+        self.insert_memo(
             zalsa,
-            database_key_index.key_index(),
+            id,
             Memo::new(
                 Some(new_value),
                 zalsa.current_revision(),
                 completed_query.revisions,
             ),
             memo_ingredient_index,
-        );
-
-        new_memo
+        )
     }
 
     fn execute_maybe_iterate<'db>(
@@ -512,9 +508,7 @@ impl<'a, C: Configuration> ClearCycleHeadIfPanicking<'a, C> {
 impl<C: Configuration> Drop for ClearCycleHeadIfPanicking<'_, C> {
     fn drop(&mut self) {
         if std::thread::panicking() {
-            let mut revisions =
-                QueryRevisions::fixpoint_initial(self.ingredient.database_key_index(self.id));
-            revisions.update_iteration_count_mut(
+            let revisions = QueryRevisions::fixpoint_initial(
                 self.ingredient.database_key_index(self.id),
                 IterationCount::panicked(),
             );

src/function/fetch.rs

@@ -141,7 +141,7 @@ where
                     let memo = self.get_memo_from_table_for(zalsa, id, memo_ingredient_index);
 
                     if let Some(memo) = memo {
-                        if memo.value.is_some() && memo.may_be_provisional() {
+                        if memo.value.is_some() {
                             memo.block_on_heads(zalsa, zalsa_local);
                         }
                     }
@@ -230,12 +230,9 @@ where
                     self.update_shallow(zalsa, database_key_index, memo, can_shallow_update);
 
                     if C::CYCLE_STRATEGY == CycleRecoveryStrategy::Fixpoint {
-                        // This feels strange. I feel like we need to preserve the cycle heads. Let's say a cycle head only sometimes participates in the cycle.
-                        // This doesn't mean that the value becomes final because of it. The query might as well be cyclic in the next iteration but
-                        // we then never re-executed that query because it was marked as `verified_final`.
                         memo.revisions
                             .cycle_heads()
-                            .clear_except(database_key_index);
+                            .remove_all_except(database_key_index);
                     }
 
                     crate::tracing::debug!(
@@ -267,7 +264,8 @@ where
                     "hit cycle at {database_key_index:#?}, \
                     inserting and returning fixpoint initial value"
                 );
-                let revisions = QueryRevisions::fixpoint_initial(database_key_index);
+                let revisions =
+                    QueryRevisions::fixpoint_initial(database_key_index, IterationCount::initial());
                 let initial_value = C::cycle_initial(db, C::id_to_input(zalsa, id));
                 self.insert_memo(
                     zalsa,
@@ -286,7 +284,10 @@ where
                 let mut completed_query = active_query.pop();
                 completed_query
                     .revisions
-                    .set_cycle_heads(CycleHeads::initial(database_key_index));
+                    .set_cycle_heads(CycleHeads::initial(
+                        database_key_index,
+                        IterationCount::initial(),
+                    ));
                 // We need this for `cycle_heads()` to work. We will unset this in the outer `execute()`.
                 *completed_query.revisions.verified_final.get_mut() = false;
                 self.insert_memo(