make interned keys have durability high

src/interned.rs

@@ -18,6 +18,8 @@ use std::fmt::Debug;
 use std::hash::Hash;
 use std::sync::Arc;
 
+const INTERN_DURABILITY: Durability = Durability::HIGH;
+
 /// Handles storage where the value is 'derived' by executing a
 /// function (in contrast to "inputs").
 pub struct InternedStorage<DB, Q>
@@ -324,12 +326,12 @@ where
         let changed_at = slot.interned_at;
         let index = slot.index;
         db.salsa_runtime()
-            .report_query_read(slot, Durability::LOW, changed_at);
+            .report_query_read(slot, INTERN_DURABILITY, changed_at);
         Ok(<Q::Value>::from_intern_id(index))
     }
 
     fn durability(&self, _db: &DB, _key: &Q::Key) -> Durability {
-        Durability::LOW
+        INTERN_DURABILITY
     }
 
     fn entries<C>(&self, _db: &DB) -> C
@@ -355,6 +357,7 @@ where
 {
     fn sweep(&self, db: &DB, strategy: SweepStrategy) {
         let mut tables = self.tables.write();
+        let last_changed = db.salsa_runtime().last_changed_revision(INTERN_DURABILITY);
         let revision_now = db.salsa_runtime().current_revision();
         let InternTables {
             map,
@@ -379,7 +382,7 @@ where
                 // they are removed and also be forced to re-execute.
                 DiscardIf::Always | DiscardIf::Outdated => match &values[intern_index.as_usize()] {
                     InternValue::Present { slot, .. } => {
-                        if slot.try_collect(revision_now) {
+                        if slot.try_collect(last_changed, revision_now) {
                             values[intern_index.as_usize()] =
                                 InternValue::Free { next: *first_free };
                             *first_free = Some(*intern_index);
@@ -425,12 +428,12 @@ where
         let value = slot.value.clone();
         let interned_at = slot.interned_at;
         db.salsa_runtime()
-            .report_query_read(slot, Durability::LOW, interned_at);
+            .report_query_read(slot, INTERN_DURABILITY, interned_at);
         Ok(value)
     }
 
     fn durability(&self, _db: &DB, _key: &Q::Key) -> Durability {
-        Durability::LOW
+        INTERN_DURABILITY
     }
 
     fn entries<C>(&self, db: &DB) -> C
@@ -497,12 +500,14 @@ impl<K> Slot<K> {
     }
 
     /// Invoked during sweeping to try and collect this slot. Fails if
-    /// the slot has been accessed in the current revision. Note that
-    /// this access could be racing with the attempt to collect (in
+    /// the slot has been accessed since the intern durability last
+    /// changed, because in that case there may be outstanding
+    /// references that are still considered valid. Note that this
+    /// access could be racing with the attempt to collect (in
     /// particular, when verifying dependencies).
-    fn try_collect(&self, revision_now: Revision) -> bool {
+    fn try_collect(&self, last_changed: Revision, revision_now: Revision) -> bool {
         let accessed_at = self.accessed_at.load().unwrap();
-        if accessed_at < revision_now {
+        if accessed_at < last_changed {
             match self.accessed_at.compare_exchange(Some(accessed_at), None) {
                 Ok(_) => true,
                 Err(r) => {

tests/gc/interned.rs

@@ -1,5 +1,6 @@
 use crate::db;
-use salsa::{Database, InternId, SweepStrategy};
+use salsa::debug::DebugQueryTable;
+use salsa::{Database, Durability, InternId, SweepStrategy};
 
 /// Query group for tests for how interned keys interact with GC.
 #[salsa::query_group(Intern)]
@@ -73,13 +74,13 @@ fn discard_during_same_revision() {
 /// exercises precisely that scenario.
 #[test]
 fn discard_outdated() {
-    let mut db = db::DatabaseImpl::default();
+    let db = db::DatabaseImpl::default();
 
     let foo_from_rev0 = db.repeat_intern1("foo");
     let bar_from_rev0 = db.repeat_intern1("bar");
 
     // Trigger a new revision.
-    db.set_dummy(());
+    db.salsa_runtime().synthetic_write(Durability::HIGH);
 
     // In this revision, we use "bar".
     let bar_from_rev1 = db.repeat_intern1("bar");
@@ -110,3 +111,84 @@ fn discard_outdated() {
     assert_eq!(db.lookup_intern_str(bar_from_rev1), "bar");
     assert_eq!(db.lookup_intern_str(baz_from_rev1), "baz");
 }
+
+/// Variation on `discard_during_same_revision` --- here we show that
+/// a synthetic write of level LOW isn't enough to collect interned
+/// keys (which are considered durability HIGH).
+#[test]
+fn discard_durability_after_synthetic_write_low() {
+    let db = db::DatabaseImpl::default();
+
+    // This will assign index 0 for "foo".
+    let foo1a = db.repeat_intern1("foo");
+    assert_eq!(
+        Durability::HIGH,
+        db.query(RepeatIntern1Query).durability("foo")
+    );
+
+    // Trigger a new revision.
+    db.salsa_runtime().synthetic_write(Durability::LOW);
+
+    // If we are not careful, this would remove the interned key for
+    // "foo".
+    db.query(InternStrQuery).sweep(
+        SweepStrategy::default()
+            .discard_everything()
+            .sweep_all_revisions(),
+    );
+
+    // This would then reuse index 0 for "bar".
+    let bar1 = db.intern_str("bar");
+
+    // And here we would assign index *1* to "foo".
+    let foo2 = db.repeat_intern2("foo");
+
+    // But we would still have a cached result with the value 0 and
+    // with high durability, so we can reuse it. That gives an
+    // inconsistent result.
+    let foo1b = db.repeat_intern1("foo");
+
+    assert_ne!(foo2, bar1);
+    assert_eq!(foo1a, foo1b);
+    assert_eq!(foo1b, foo2);
+}
+
+/// Variation on previous test in which we do a synthetic write to
+/// `Durability::HIGH`.
+#[test]
+fn discard_durability_after_synthetic_write_high() {
+    let db = db::DatabaseImpl::default();
+
+    // This will assign index 0 for "foo".
+    let foo1a = db.repeat_intern1("foo");
+    assert_eq!(
+        Durability::HIGH,
+        db.query(RepeatIntern1Query).durability("foo")
+    );
+
+    // Trigger a new revision -- marking even high things as having changed.
+    db.salsa_runtime().synthetic_write(Durability::HIGH);
+
+    // We are now able to collect "collect".
+    db.query(InternStrQuery).sweep(
+        SweepStrategy::default()
+            .discard_everything()
+            .sweep_all_revisions(),
+    );
+
+    // So we can reuse index 0 for "bar".
+    let bar1 = db.intern_str("bar");
+
+    // And here we assign index *1* to "foo".
+    let foo2 = db.repeat_intern2("foo");
+    let foo1b = db.repeat_intern1("foo");
+
+    // Thus foo1a (from before the synthetic write) and foo1b (from
+    // after) are different.
+    assert_ne!(foo1a, foo1b);
+
+    // But the things that come after the synthetic write are
+    // consistent.
+    assert_ne!(foo2, bar1);
+    assert_eq!(foo1b, foo2);
+}