From ac93c950be36a06259c052ee114557a28b662414 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Tue, 25 Jun 2019 21:50:00 -0400 Subject: [PATCH] make interned keys have durability high --- src/interned.rs | 23 +++++++----- tests/gc/interned.rs | 88 ++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 99 insertions(+), 12 deletions(-) diff --git a/src/interned.rs b/src/interned.rs index f39e381c..26c76497 100644 --- a/src/interned.rs +++ b/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 @@ -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(::from_intern_id(index)) } fn durability(&self, _db: &DB, _key: &Q::Key) -> Durability { - Durability::LOW + INTERN_DURABILITY } fn entries(&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(&self, db: &DB) -> C @@ -497,12 +500,14 @@ impl Slot { } /// 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) => { diff --git a/tests/gc/interned.rs b/tests/gc/interned.rs index 3c69080f..5772890b 100644 --- a/tests/gc/interned.rs +++ b/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); +}