From c58ff26d8778dba69475a1cbacfd2db677911082 Mon Sep 17 00:00:00 2001 From: Jack Rickard Date: Sun, 25 Sep 2022 22:33:37 +0100 Subject: [PATCH] Simplify interning by restricting deletes --- components/salsa-2022/src/interned.rs | 82 +++++++++++---------------- 1 file changed, 34 insertions(+), 48 deletions(-) diff --git a/components/salsa-2022/src/interned.rs b/components/salsa-2022/src/interned.rs index a6d08509..8fd00349 100644 --- a/components/salsa-2022/src/interned.rs +++ b/components/salsa-2022/src/interned.rs @@ -32,12 +32,12 @@ pub struct InternedIngredient { /// Maps from data to the existing interned id for that data. /// - /// Deadlock requirement: We access `key_map` while holding lock on `value_map`, but not vice versa. + /// Deadlock requirement: We access `value_map` while holding lock on `key_map`, but not vice versa. key_map: FxDashMap, /// Maps from an interned id to its data. /// - /// Deadlock requirement: We access `key_map` while holding lock on `value_map`, but not vice versa. + /// Deadlock requirement: We access `value_map` while holding lock on `key_map`, but not vice versa. value_map: FxDashMap>, /// counter for the next id. @@ -89,31 +89,18 @@ where return *id; } - let next_id = self.counter.fetch_add(1); - let next_id = Id::from_id(crate::id::Id::from_u32(next_id)); - // Insert the data into the value map. This temporarily breaks the - // invariant that value_map and key_map are opposites, this is fine - // because bno-one has the value next_id so it's not observable. - let old_value = self.value_map.insert(next_id, Box::new(data.clone())); - assert!( - old_value.is_none(), - "next_id is guaranteed to be unique, bar overflow" - ); - match self.key_map.entry(data) { + match self.key_map.entry(data.clone()) { // Data has been interned by a racing call, use that ID instead - dashmap::mapref::entry::Entry::Occupied(entry) => { - // Return the interned ID set by the thread that beat us here. - let id = *entry.get(); - // Release the lock on key_map so we don't deadlock. - drop(entry); - // Fixup the broken invariant by removing the vale we just - // optimistically added. - self.value_map.remove(&next_id); - id - } - // We won any races so can complete the interning + dashmap::mapref::entry::Entry::Occupied(entry) => *entry.get(), + // We won any races so should intern the data dashmap::mapref::entry::Entry::Vacant(entry) => { - // Fixup the broken invariant by adding next_id to the key_map. + let next_id = self.counter.fetch_add(1); + let next_id = Id::from_id(crate::id::Id::from_u32(next_id)); + let old_value = self.value_map.insert(next_id, Box::new(data)); + assert!( + old_value.is_none(), + "next_id is guaranteed to be unique, bar overflow" + ); entry.insert(next_id); next_id } @@ -161,33 +148,32 @@ where /// /// # Warning /// - /// This should only be used when you are certain that the given `id` has not (and will not) - /// be used in the current revision. More specifically, this is used when a query `Q` executes - /// and we can compare the entities `E_now` that it produced in this revision vs the entities - /// `E_prev` it produced in the last revision. Any missing entities `E_prev - E_new` can be - /// deleted. + /// This should only be used when you are certain that: + /// 1. The given `id` has not (and will not) be used in the current revision. + /// 2. The interned data corresponding to `id` will not be interned in this revision. + /// + /// More specifically, this is used when a query `Q` executes and we can compare the + /// entities `E_now` that it produced in this revision vs the entities `E_prev` it + /// produced in the last revision. Any missing entities `E_prev - E_new` can be deleted. /// /// If you are wrong about this, it should not be unsafe, but unpredictable results may occur. pub(crate) fn delete_index(&self, id: Id) { - match self.value_map.entry(id) { - dashmap::mapref::entry::Entry::Vacant(_) => { - panic!("No entry for id `{:?}`", id); - } - dashmap::mapref::entry::Entry::Occupied(entry) => { - self.key_map.remove(entry.get()); + let (_, key) = self + .value_map + .remove(&id) + .unwrap_or_else(|| panic!("No entry for id `{:?}`", id)); - // Careful: even though `id` ought not to have been used in this revision, - // we don't know that for sure since users could have leaked things. If they did, - // they may have stray references into `data`. So push the box onto the - // "to be deleted" queue. - // - // To avoid this, we could include some kind of atomic counter in the `Box` that - // gets set whenever `data` executes, so we can track if the data was accessed since - // the last time an `&mut self` method was called. But that'd take extra storage - // and doesn't obviously seem worth it. - self.deleted_entries.push(entry.remove()); - } - } + self.key_map.remove(&key); + // Careful: even though `id` ought not to have been used in this revision, + // we don't know that for sure since users could have leaked things. If they did, + // they may have stray references into `data`. So push the box onto the + // "to be deleted" queue. + // + // To avoid this, we could include some kind of atomic counter in the `Box` that + // gets set whenever `data` executes, so we can track if the data was accessed since + // the last time an `&mut self` method was called. But that'd take extra storage + // and doesn't obviously seem worth it. + self.deleted_entries.push(key); } pub(crate) fn clear_deleted_indices(&mut self) {