From 30714022fd65e1eb461180c0c25f5068e3f96474 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 21 Jun 2019 20:12:51 -0700 Subject: [PATCH] kill the `ChangedAt` struct It didn't seem like it was buying us much. --- src/derived.rs | 9 ++++- src/derived/slot.rs | 30 +++++++-------- src/input.rs | 23 ++++++----- src/interned.rs | 18 +++------ src/runtime.rs | 78 ++++++++++++++------------------------ src/runtime/local_state.rs | 10 +++-- 6 files changed, 73 insertions(+), 95 deletions(-) diff --git a/src/derived.rs b/src/derived.rs index 2bf47eb..4ce2b41 100644 --- a/src/derived.rs +++ b/src/derived.rs @@ -134,13 +134,18 @@ where { fn try_fetch(&self, db: &DB, key: &Q::Key) -> Result { let slot = self.slot(key); - let StampedValue { value, changed_at } = slot.read(db)?; + let StampedValue { + value, + is_constant, + changed_at, + } = slot.read(db)?; if let Some(evicted) = self.lru_list.record_use(&slot) { evicted.evict(); } - db.salsa_runtime().report_query_read(slot, changed_at); + db.salsa_runtime() + .report_query_read(slot, is_constant, changed_at); Ok(value) } diff --git a/src/derived/slot.rs b/src/derived/slot.rs index d19d793..4c030f9 100644 --- a/src/derived/slot.rs +++ b/src/derived/slot.rs @@ -8,7 +8,6 @@ use crate::plumbing::CycleDetected; use crate::plumbing::GetQueryTable; use crate::plumbing::HasQueryGroup; use crate::plumbing::QueryFunction; -use crate::runtime::ChangedAt; use crate::runtime::FxIndexSet; use crate::runtime::Revision; use crate::runtime::Runtime; @@ -223,7 +222,7 @@ where // Careful: the "constant-ness" must also not have // changed, see the test `constant_to_non_constant`. let memo_was_constant = old_memo.constant_in_revision.is_some(); - if memo_was_constant == result.changed_at.is_constant + if memo_was_constant == result.is_constant && MP::memoized_value_eq(&old_value, &result.value) { debug!( @@ -231,14 +230,15 @@ where self, old_memo.changed_at, ); - assert!(old_memo.changed_at <= result.changed_at.revision); - result.changed_at.revision = old_memo.changed_at; + assert!(old_memo.changed_at <= result.changed_at); + result.changed_at = old_memo.changed_at; } } } let new_value = StampedValue { value: result.value, + is_constant: result.is_constant, changed_at: result.changed_at, }; @@ -253,7 +253,7 @@ where self, result.changed_at, result.dependencies, ); - let constant_in_revision = if result.changed_at.is_constant { + let constant_in_revision = if result.is_constant { Some(revision_now) } else { None @@ -280,7 +280,7 @@ where panic_guard.memo = Some(Memo { value, - changed_at: result.changed_at.revision, + changed_at: result.changed_at, verified_at: revision_now, inputs, constant_in_revision, @@ -717,10 +717,8 @@ where } StampedValue { - changed_at: ChangedAt { - is_constant, - revision: self.changed_at, - }, + is_constant, + changed_at: self.changed_at, value, } } @@ -738,10 +736,8 @@ where let is_constant = self.constant_in_revision.is_some(); return Some(StampedValue { - changed_at: ChangedAt { - is_constant, - revision: self.changed_at, - }, + is_constant, + changed_at: self.changed_at, value: value.clone(), }); } @@ -840,7 +836,7 @@ where std::mem::drop(state); let value = rx.recv().unwrap_or_else(|_| db.on_propagated_panic()); - return value.changed_at.changed_since(revision); + return value.changed_at > revision; } // Consider a cycle to have changed. @@ -903,10 +899,10 @@ where debug!( "maybe_changed_since({:?}: {:?} since (recomputed) value changed at {:?}", self, - v.changed_at.changed_since(revision), + v.changed_at > revision, v.changed_at, ); - v.changed_at.changed_since(revision) + v.changed_at > revision } Err(CycleDetected) => true, }; diff --git a/src/input.rs b/src/input.rs index 042c863..e3c7091 100644 --- a/src/input.rs +++ b/src/input.rs @@ -4,7 +4,6 @@ use crate::plumbing::CycleDetected; use crate::plumbing::InputQueryStorageOps; use crate::plumbing::QueryStorageMassOps; use crate::plumbing::QueryStorageOps; -use crate::runtime::ChangedAt; use crate::runtime::Revision; use crate::runtime::StampedValue; use crate::Database; @@ -100,18 +99,17 @@ where // racing with somebody else to modify this same cell. // (Otherwise, someone else might write a *newer* revision // into the same cell while we block on the lock.) - let changed_at = ChangedAt { + let stamped_value = StampedValue { + value, is_constant: is_constant.0, - revision: guard.new_revision(), + changed_at: guard.new_revision(), }; - let stamped_value = StampedValue { value, changed_at }; - match slots.entry(key.clone()) { Entry::Occupied(entry) => { let mut slot_stamped_value = entry.get().stamped_value.write(); - if slot_stamped_value.changed_at.is_constant { + if slot_stamped_value.is_constant { guard.mark_constants_as_changed(); } @@ -140,16 +138,21 @@ where None => panic!("no value set for {:?}({:?})", Q::default(), key), }; - let StampedValue { value, changed_at } = slot.stamped_value.read().clone(); + let StampedValue { + value, + is_constant, + changed_at, + } = slot.stamped_value.read().clone(); - db.salsa_runtime().report_query_read(slot, changed_at); + db.salsa_runtime() + .report_query_read(slot, is_constant, changed_at); Ok(value) } fn is_constant(&self, _db: &DB, key: &Q::Key) -> bool { self.slot(key) - .map(|slot| slot.stamped_value.read().changed_at.is_constant) + .map(|slot| slot.stamped_value.read().is_constant) .unwrap_or(false) } @@ -215,7 +218,7 @@ where debug!("maybe_changed_since: changed_at = {:?}", changed_at); - changed_at.changed_since(revision) + changed_at > revision } } diff --git a/src/interned.rs b/src/interned.rs index d558046..8c3914e 100644 --- a/src/interned.rs +++ b/src/interned.rs @@ -5,7 +5,6 @@ use crate::plumbing::CycleDetected; use crate::plumbing::HasQueryGroup; use crate::plumbing::QueryStorageMassOps; use crate::plumbing::QueryStorageOps; -use crate::runtime::ChangedAt; use crate::runtime::Revision; use crate::Query; use crate::{Database, DiscardIf, SweepStrategy}; @@ -323,13 +322,8 @@ where let slot = self.intern_index(db, key); let changed_at = slot.interned_at; let index = slot.index; - db.salsa_runtime().report_query_read( - slot, - ChangedAt { - is_constant: false, - revision: changed_at, - }, - ); + db.salsa_runtime() + .report_query_read(slot, false, changed_at); Ok(::from_intern_id(index)) } @@ -427,12 +421,10 @@ where let group_storage = >::group_storage(db); let interned_storage = IQ::query_storage(group_storage); let slot = interned_storage.lookup_value(db, index); - let changed_at = ChangedAt { - is_constant: false, - revision: slot.interned_at, - }; let value = slot.value.clone(); - db.salsa_runtime().report_query_read(slot, changed_at); + let interned_at = slot.interned_at; + db.salsa_runtime() + .report_query_read(slot, false, interned_at); Ok(value) } diff --git a/src/runtime.rs b/src/runtime.rs index d4e553e..4d10d72 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -332,11 +332,13 @@ where let ActiveQuery { dependencies, changed_at, + is_constant, .. } = active_query.complete(); ComputedQueryResult { value, + is_constant, changed_at, dependencies, } @@ -353,10 +355,12 @@ where pub(crate) fn report_query_read<'hack>( &self, database_slot: Arc + 'hack>, - changed_at: ChangedAt, + is_constant: bool, + changed_at: Revision, ) { let dependency = Dependency::new(database_slot); - self.local_state.report_query_read(dependency, changed_at); + self.local_state + .report_query_read(dependency, is_constant, changed_at); } /// Reports that the query depends on some state unknown to salsa. @@ -532,11 +536,12 @@ struct ActiveQuery { /// What query is executing database_key: DB::DatabaseKey, - /// Maximum revision of all inputs thus far; - /// we also track if all inputs have been constant. - /// - /// If we see an untracked input, this is not terribly relevant. - changed_at: ChangedAt, + /// True if all inputs were constant (and no untracked inputs). + is_constant: bool, + + /// Maximum revision of all inputs observed. If we observe an + /// untracked read, this will be set to the most recent revision. + changed_at: Revision, /// Set of subqueries that were accessed thus far, or `None` if /// there was an untracked the read. @@ -547,12 +552,12 @@ pub(crate) struct ComputedQueryResult { /// Final value produced pub(crate) value: V, - /// Maximum revision of all inputs observed; `is_constant` is true - /// if all inputs were constants. - /// - /// If we observe an untracked read, this will be set to a - /// non-constant value that changed in the most recent revision. - pub(crate) changed_at: ChangedAt, + /// True if all inputs were constant (and no untracked inputs). + pub(crate) is_constant: bool, + + /// Maximum revision of all inputs observed. If we observe an + /// untracked read, this will be set to the most recent revision. + pub(crate) changed_at: Revision, /// Complete set of subqueries that were accessed, or `None` if /// there was an untracked the read. @@ -563,36 +568,29 @@ impl ActiveQuery { fn new(database_key: DB::DatabaseKey) -> Self { ActiveQuery { database_key, - changed_at: ChangedAt { - is_constant: true, - revision: Revision::start(), - }, + is_constant: true, + changed_at: Revision::start(), dependencies: Some(FxIndexSet::default()), } } - fn add_read(&mut self, dependency: Dependency, changed_at: ChangedAt) { - let ChangedAt { - is_constant, - revision, - } = changed_at; - + fn add_read(&mut self, dependency: Dependency, is_constant: bool, revision: Revision) { if let Some(set) = &mut self.dependencies { set.insert(dependency); } - self.changed_at.is_constant &= is_constant; - self.changed_at.revision = self.changed_at.revision.max(revision); + self.is_constant &= is_constant; + self.changed_at = self.changed_at.max(revision); } fn add_untracked_read(&mut self, changed_at: Revision) { self.dependencies = None; - self.changed_at.is_constant = false; - self.changed_at.revision = changed_at; + self.is_constant = false; + self.changed_at = changed_at; } fn add_anon_read(&mut self, changed_at: Revision) { - self.changed_at.revision = self.changed_at.revision.max(changed_at); + self.changed_at = self.changed_at.max(changed_at); } } @@ -640,31 +638,11 @@ impl std::fmt::Debug for Revision { } } -/// Records when a stamped value changed. -#[derive(Copy, Clone, Debug, PartialEq, Eq)] -pub struct ChangedAt { - // Will this value ever change again? - pub(crate) is_constant: bool, - - // At which revision did this value last change? (If this value is - // the value of a constant input, this indicates when it became - // constant.) - pub(crate) revision: Revision, -} - -impl ChangedAt { - /// True if a value is stored with this `ChangedAt` value has - /// changed after `revision`. This is invoked by query storage - /// when their dependents are asking them if they have changed. - pub(crate) fn changed_since(self, revision: Revision) -> bool { - self.revision > revision - } -} - #[derive(Clone, Debug)] pub(crate) struct StampedValue { pub(crate) value: V, - pub(crate) changed_at: ChangedAt, + pub(crate) is_constant: bool, + pub(crate) changed_at: Revision, } struct DependencyGraph { diff --git a/src/runtime/local_state.rs b/src/runtime/local_state.rs index d98cdc2..fe769a7 100644 --- a/src/runtime/local_state.rs +++ b/src/runtime/local_state.rs @@ -1,6 +1,5 @@ use crate::dependency::Dependency; use crate::runtime::ActiveQuery; -use crate::runtime::ChangedAt; use crate::runtime::Revision; use crate::Database; use std::cell::Ref; @@ -58,9 +57,14 @@ impl LocalState { .map(|active_query| active_query.database_key.clone()) } - pub(super) fn report_query_read(&self, dependency: Dependency, changed_at: ChangedAt) { + pub(super) fn report_query_read( + &self, + dependency: Dependency, + is_constant: bool, + changed_at: Revision, + ) { if let Some(top_query) = self.query_stack.borrow_mut().last_mut() { - top_query.add_read(dependency, changed_at); + top_query.add_read(dependency, is_constant, changed_at); } }