Drop unnecessary locking in Lru::for_each_evicted

Author: Veykril

src/function.rs

@@ -231,8 +231,13 @@ where
     }
 
     fn reset_for_new_revision(&mut self, table: &mut Table) {
-        self.lru
-            .for_each_evicted(|evict| self.evict_value_from_memo_for(table.memos_mut(evict)));
+        self.lru.for_each_evicted(|evict| {
+            Self::evict_value_from_memo_for(
+                table.memos_mut(evict),
+                self.memo_ingredient_index,
+                &self.delete,
+            )
+        });
     }
 
     fn fmt_index(&self, index: Option<crate::Id>, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {

src/function/lru.rs

@@ -26,8 +26,8 @@ impl Lru {
         self.capacity.store(capacity);
     }
 
-    pub(super) fn for_each_evicted(&self, mut cb: impl FnMut(Id)) {
-        let mut set = self.set.lock();
+    pub(super) fn for_each_evicted(&mut self, mut cb: impl FnMut(Id)) {
+        let set = self.set.get_mut();
         let cap = self.capacity.load();
         if set.len() <= cap || cap == 0 {
             return;

src/function/memo.rs

@@ -7,7 +7,9 @@ use std::sync::Arc;
 use crossbeam::atomic::AtomicCell;
 
 use crate::accumulator::accumulated_map::InputAccumulatedValues;
+use crate::plumbing::MemoDropSender;
 use crate::table::memo::MemoTable;
+use crate::zalsa::MemoIngredientIndex;
 use crate::zalsa_local::QueryOrigin;
 use crate::{
     key::DatabaseKeyIndex, zalsa::Zalsa, zalsa_local::QueryRevisions, Event, EventKind, Id,
@@ -68,7 +70,11 @@ impl<C: Configuration> IngredientImpl<C> {
     /// Evicts the existing memo for the given key, replacing it
     /// with an equivalent memo that has no value. If the memo is untracked, BaseInput,
     /// or has values assigned as output of another query, this has no effect.
-    pub(super) fn evict_value_from_memo_for(&self, table: &mut MemoTable) {
+    pub(super) fn evict_value_from_memo_for(
+        table: &mut MemoTable,
+        memo_ingredient_index: MemoIngredientIndex,
+        memo_drop: &MemoDropSender,
+    ) {
         let map = |memo: ArcMemo<'static, C>| -> ArcMemo<'static, C> {
             match &memo.revisions.origin {
                 QueryOrigin::Assigned(_)
@@ -82,37 +88,17 @@ impl<C: Configuration> IngredientImpl<C> {
                 }
                 QueryOrigin::Derived(_) => {
                     // Note that we cannot use `Arc::get_mut` here as the use of `ArcSwap` makes it
-                    // impossible to get unique access to the interior Arc
-                    // QueryRevisions: !Clone to discourage cloning, we need it here though
-                    let &QueryRevisions {
-                        changed_at,
-                        durability,
-                        ref origin,
-                        ref tracked_struct_ids,
-                        ref accumulated,
-                        ref accumulated_inputs,
-                    } = &memo.revisions;
-                    // Re-assemble the memo but with the value set to `None`
-                    Arc::new(Memo::new(
-                        None,
-                        memo.verified_at.load(),
-                        QueryRevisions {
-                            changed_at,
-                            durability,
-                            origin: origin.clone(),
-                            tracked_struct_ids: tracked_struct_ids.clone(),
-                            accumulated: accumulated.clone(),
-                            accumulated_inputs: AtomicCell::new(accumulated_inputs.load()),
-                        },
-                    ))
+                    // impossible to get unique access to the interior Arc so we need to construct
+                    // a new allocation
+                    Arc::new(memo.without_value())
                 }
             }
         };
         // SAFETY: We queue the old value for deletion, delaying its drop until the next revision
         // bump.
-        let old_memo = unsafe { table.map_memo(self.memo_ingredient_index, map) };
+        let old_memo = unsafe { table.map_memo(memo_ingredient_index, map) };
         if let Some(old_memo) = old_memo {
-            self.delete.delay(ManuallyDrop::into_inner(old_memo));
+            memo_drop.delay(ManuallyDrop::into_inner(old_memo));
         }
     }
 }
@@ -143,6 +129,31 @@ impl<V> Memo<V> {
             revisions,
         }
     }
+
+    pub(super) fn without_value(&self) -> Self {
+        let &QueryRevisions {
+            changed_at,
+            durability,
+            ref origin,
+            ref tracked_struct_ids,
+            ref accumulated,
+            ref accumulated_inputs,
+        } = &self.revisions;
+        // Re-assemble the memo but with the value set to `None`
+        Memo::new(
+            None,
+            self.verified_at.load(),
+            QueryRevisions {
+                changed_at,
+                durability,
+                origin: origin.clone(),
+                tracked_struct_ids: tracked_struct_ids.clone(),
+                accumulated: accumulated.clone(),
+                accumulated_inputs: AtomicCell::new(accumulated_inputs.load()),
+            },
+        )
+    }
+
     /// True if this memo is known not to have changed based on its durability.
     pub(super) fn check_durability(&self, zalsa: &Zalsa) -> bool {
         let last_changed = zalsa.last_changed_revision(self.revisions.durability);