fix some bugs in deep_verify_memo

Author: carljm

src/function/maybe_changed_after.rs

@@ -110,7 +110,7 @@ where
                     return Some(VerifyResult::Unchanged(
                         InputAccumulatedValues::Empty,
                         FxHashSet::from_iter([database_key_index]),
-                    ))
+                    ));
                 }
             },
             ClaimResult::Claimed(guard) => guard,
@@ -257,9 +257,6 @@ where
         if self.shallow_verify_memo(db, zalsa, database_key_index, old_memo, false) {
             return VerifyResult::Unchanged(InputAccumulatedValues::Empty, Default::default());
         }
-        if old_memo.may_be_provisional() {
-            return VerifyResult::Changed;
-        }
 
         let mut cycle_heads = FxHashSet::default();
         loop {
@@ -338,6 +335,42 @@ where
                 }
             };
 
+            // If this was a provisional memo from an older revision, we should have now validated
+            // the latest memos of all dependencies, so try one more time to validate ourselves;
+            // otherwise return changed. (This is necessary because a provisional memo may have a
+            // cycle head which itself is provisional, and in the previous revision it's possible
+            // that neither one was ever finalized; `validate_provisional` is not recursive, so we
+            // need to validate them in the right order.)
+            if old_memo.may_be_provisional() && !self.validate_provisional(db, zalsa, &old_memo) {
+                return VerifyResult::Changed;
+            }
+
+            // Possible scenarios here:
+            //
+            // 1. Cycle heads is empty. We traversed our full dependency graph and neither hit any
+            //    cycles, nor found any changed dependencies. We can mark our memo verified and
+            //    return Unchanged with empty cycle heads.
+            //
+            // 2. Cycle heads is non-empty, and does not contain our own key index. We are part of
+            //    a cycle, and since we don't know if some other cycle participant that hasn't been
+            //    traversed yet (that is, some other dependency of the cycle head, which is only a
+            //    dependency of ours via the cycle) might still have changed, we can't yet mark our
+            //    memo verified. We can return a provisional Unchanged, with cycle heads.
+            //
+            // 3. Cycle heads is non-empty, and contains only our own key index. We are the head of
+            //    a cycle, and we've now traversed the entire cycle and found no changes, but no
+            //    other cycle participants were verified (they would have all hit case 2 above). We
+            //    can now safely mark our own memo as verified. Then we have to traverse the entire
+            //    cycle again. This time, since our own memo is verified, there will be no cycle
+            //    encountered, and the rest of the cycle will be able to verify itself.
+            //
+            // 4. Cycle heads is non-empty, and contains our own key index as well as other key
+            //    indices. We are the head of a cycle nested within another cycle. We can't mark
+            //    our own memo verified (for the same reason as in case 2: the full outer cycle
+            //    hasn't been validated unchanged yet). We return Unchanged, with ourself removed
+            //    from cycle heads. We will handle our own memo (and the rest of our cycle) on a
+            //    future iteration; first the outer cycle head needs to verify itself.
+
             let in_heads = cycle_heads.remove(&database_key_index);
 
             if cycle_heads.is_empty() {
@@ -347,10 +380,15 @@ where
                     database_key_index,
                     inputs,
                 );
-            }
-            if in_heads {
-                cycle_heads.clear();
-                continue;
+
+                if in_heads {
+                    // Iterate our dependency graph again, starting from the top. We clear the
+                    // cycle heads here because we are starting a fresh traversal. (It might be
+                    // logically clearer to create a new HashSet each time, but clearing the
+                    // existing one is more efficient.)
+                    cycle_heads.clear();
+                    continue;
+                }
             }
             return VerifyResult::Unchanged(InputAccumulatedValues::Empty, cycle_heads);
         }

tests/cycle.rs

@@ -802,7 +802,13 @@ fn nested_double_multiple_revisions() {
     a.clone().assert_bounds(&db);
 
     // and next revision, we converge
-    c_in.set_inputs(&mut db).to(vec![value(240), a.clone(), b]);
+    c_in.set_inputs(&mut db)
+        .to(vec![value(240), a.clone(), b.clone()]);
+
+    a.clone().assert_value(&db, 240);
+
+    // one more revision, without relevant changes
+    a_in.set_inputs(&mut db).to(vec![b]);
 
     a.assert_value(&db, 240);
 }
@@ -879,3 +885,120 @@ fn cycle_unchanged() {
 
     a.assert_value(&db, 45);
 }
+
+/// a:Np(v59, b) -> b:Ni(v60, c) -> c:Np(d) -> d:Ni(v61, b, e) -> e:Np(d)
+///                 ^                          |   ^              |
+///                 +--------------------------+   +--------------+
+///
+/// If nothing in a nested cycle changed in the new revision, no part of the cycle should
+/// re-execute.
+#[test]
+fn cycle_unchanged_nested() {
+    let mut db = ExecuteValidateLoggerDatabase::default();
+    let a_in = Inputs::new(&db, vec![]);
+    let b_in = Inputs::new(&db, vec![]);
+    let c_in = Inputs::new(&db, vec![]);
+    let d_in = Inputs::new(&db, vec![]);
+    let e_in = Inputs::new(&db, vec![]);
+    let a = Input::MinPanic(a_in);
+    let b = Input::MinIterate(b_in);
+    let c = Input::MinPanic(c_in);
+    let d = Input::MinIterate(d_in);
+    let e = Input::MinPanic(e_in);
+    a_in.set_inputs(&mut db).to(vec![value(59), b.clone()]);
+    b_in.set_inputs(&mut db).to(vec![value(60), c.clone()]);
+    c_in.set_inputs(&mut db).to(vec![d.clone()]);
+    d_in.set_inputs(&mut db)
+        .to(vec![value(61), b.clone(), e.clone()]);
+    e_in.set_inputs(&mut db).to(vec![d.clone()]);
+
+    a.clone().assert_value(&db, 59);
+    b.clone().assert_value(&db, 60);
+
+    db.assert_logs_len(15);
+
+    // next revision, we change only A, which is not part of the cycle and the cycle does not
+    // depend on.
+    a_in.set_inputs(&mut db).to(vec![value(45), b.clone()]);
+    b.assert_value(&db, 60);
+
+    db.assert_logs(expect![[r#"
+        [
+            "salsa_event(DidValidateMemoizedValue { database_key: min_iterate(Id(1)) })",
+            "salsa_event(DidValidateMemoizedValue { database_key: min_iterate(Id(3)) })",
+            "salsa_event(DidValidateMemoizedValue { database_key: min_panic(Id(4)) })",
+            "salsa_event(DidValidateMemoizedValue { database_key: min_iterate(Id(3)) })",
+            "salsa_event(DidValidateMemoizedValue { database_key: min_panic(Id(2)) })",
+            "salsa_event(DidValidateMemoizedValue { database_key: min_iterate(Id(1)) })",
+        ]"#]]);
+
+    a.assert_value(&db, 45);
+}
+
+///                                 +--------------------------------+
+///                                 |                                v
+/// a:Np(v59, b) -> b:Ni(v60, c) -> c:Np(d, e) -> d:Ni(v61, b, e) -> e:Ni(d)
+///                 ^                             |   ^              |
+///                 +-----------------------------+   +--------------+
+///
+/// If nothing in a nested cycle changed in the new revision, no part of the cycle should
+/// re-execute.
+#[test_log::test]
+fn cycle_unchanged_nested_intertwined() {
+    // We run this test twice in order to catch some subtly different cases; see below.
+    for i in 0..1 {
+        let mut db = ExecuteValidateLoggerDatabase::default();
+        let a_in = Inputs::new(&db, vec![]);
+        let b_in = Inputs::new(&db, vec![]);
+        let c_in = Inputs::new(&db, vec![]);
+        let d_in = Inputs::new(&db, vec![]);
+        let e_in = Inputs::new(&db, vec![]);
+        let a = Input::MinPanic(a_in);
+        let b = Input::MinIterate(b_in);
+        let c = Input::MinPanic(c_in);
+        let d = Input::MinIterate(d_in);
+        let e = Input::MinIterate(e_in);
+        a_in.set_inputs(&mut db).to(vec![value(59), b.clone()]);
+        b_in.set_inputs(&mut db).to(vec![value(60), c.clone()]);
+        c_in.set_inputs(&mut db).to(vec![d.clone(), e.clone()]);
+        d_in.set_inputs(&mut db)
+            .to(vec![value(61), b.clone(), e.clone()]);
+        e_in.set_inputs(&mut db).to(vec![d.clone()]);
+
+        a.clone().assert_value(&db, 59);
+        b.clone().assert_value(&db, 60);
+
+        // First time we run this test, don't fetch c/d/e here; this means they won't get marked
+        // `verified_final` in R6 (this revision), which will leave us in the next revision (R7)
+        // with a chain of could-be-provisional memos from the previous revision which should be
+        // final but were never confirmed as such; this triggers the case in `deep_verify_memo`
+        // where we need to double-check `validate_provisional` after traversing dependencies.
+        //
+        // Second time we run this test, fetch everything in R6, to check the behavior of
+        // `maybe_changed_after` with all validated-final memos.
+        if i == 1 {
+            c.clone().assert_value(&db, 60);
+            d.clone().assert_value(&db, 60);
+            e.clone().assert_value(&db, 60);
+        }
+
+        db.assert_logs_len(27 + i);
+
+        // next revision, we change only A, which is not part of the cycle and the cycle does not
+        // depend on.
+        a_in.set_inputs(&mut db).to(vec![value(45), b.clone()]);
+        b.assert_value(&db, 60);
+
+        db.assert_logs(expect![[r#"
+        [
+            "salsa_event(DidValidateMemoizedValue { database_key: min_iterate(Id(1)) })",
+            "salsa_event(DidValidateMemoizedValue { database_key: min_iterate(Id(3)) })",
+            "salsa_event(DidValidateMemoizedValue { database_key: min_iterate(Id(4)) })",
+            "salsa_event(DidValidateMemoizedValue { database_key: min_iterate(Id(3)) })",
+            "salsa_event(DidValidateMemoizedValue { database_key: min_panic(Id(2)) })",
+            "salsa_event(DidValidateMemoizedValue { database_key: min_iterate(Id(1)) })",
+        ]"#]]);
+
+        a.assert_value(&db, 45);
+    }
+}