From 157e1e47f1595ff71affca8261ed3d8cb568f92a Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Mon, 1 Oct 2018 08:23:18 -0400 Subject: [PATCH] add support for "dependency only" tracking --- src/dependencies.rs | 247 +++++++++++++++++++++++ src/lib.rs | 23 +++ tests/incremental/implementation.rs | 17 +- tests/incremental/main.rs | 1 + tests/incremental/memoized_dep_inputs.rs | 71 +++++++ 5 files changed, 355 insertions(+), 4 deletions(-) create mode 100644 src/dependencies.rs create mode 100644 tests/incremental/memoized_dep_inputs.rs diff --git a/src/dependencies.rs b/src/dependencies.rs new file mode 100644 index 00000000..d5f185fe --- /dev/null +++ b/src/dependencies.rs @@ -0,0 +1,247 @@ +use crate::runtime::QueryDescriptorSet; +use crate::runtime::Revision; +use crate::runtime::StampedValue; +use crate::CycleDetected; +use crate::Query; +use crate::QueryContext; +use crate::QueryDescriptor; +use crate::QueryStorageOps; +use crate::QueryTable; +use log::debug; +use parking_lot::{RwLock, RwLockUpgradableReadGuard}; +use rustc_hash::FxHashMap; +use std::any::Any; +use std::cell::RefCell; +use std::collections::hash_map::Entry; +use std::fmt::Debug; +use std::fmt::Display; +use std::fmt::Write; +use std::hash::Hash; + +/// "Dependency" queries just track their dependencies and not the +/// actual value (which they produce on demand). This lessens the +/// storage requirements. +pub struct DependencyStorage +where + Q: Query, + QC: QueryContext, +{ + map: RwLock>>, +} + +/// Defines the "current state" of query's memoized results. +enum QueryState +where + QC: QueryContext, +{ + /// We are currently computing the result of this query; if we see + /// this value in the table, it indeeds a cycle. + InProgress, + + /// We have computed the query already, and here is the result. + Memoized(Memo), +} + +struct Memo +where + QC: QueryContext, +{ + inputs: QueryDescriptorSet, + + /// Last time that we checked our inputs to see if they have + /// changed. If this is equal to the current revision, then the + /// value is up to date. If not, we need to check our inputs and + /// see if any of them have changed since our last check -- if so, + /// we'll need to re-execute. + verified_at: Revision, + + /// Last time that our value changed. + changed_at: Revision, +} + +impl Default for DependencyStorage +where + Q: Query, + QC: QueryContext, +{ + fn default() -> Self { + DependencyStorage { + map: RwLock::new(FxHashMap::default()), + } + } +} + +impl DependencyStorage +where + Q: Query, + QC: QueryContext, +{ + fn read( + &self, + query: &QC, + key: &Q::Key, + descriptor: &QC::QueryDescriptor, + ) -> Result, CycleDetected> { + let revision_now = query.salsa_runtime().current_revision(); + + debug!( + "{:?}({:?}): invoked at {:?}", + Q::default(), + key, + revision_now, + ); + + { + let map_read = self.map.upgradable_read(); + if let Some(value) = map_read.get(key) { + match value { + QueryState::InProgress => return Err(CycleDetected), + QueryState::Memoized(_) => {} + } + } + + let mut map_write = RwLockUpgradableReadGuard::upgrade(map_read); + map_write.insert(key.clone(), QueryState::InProgress); + } + + // Note that, unlike with a memoized query, we must always + // re-execute. + let (stamped_value, inputs) = query + .salsa_runtime() + .execute_query_implementation::(query, descriptor, key); + + // We assume that query is side-effect free -- that is, does + // not mutate the "inputs" to the query system. Sanity check + // that assumption here, at least to the best of our ability. + assert_eq!( + query.salsa_runtime().current_revision(), + revision_now, + "revision altered during query execution", + ); + + { + let mut map_write = self.map.write(); + + let old_value = map_write.insert( + key.clone(), + QueryState::Memoized(Memo { + inputs, + verified_at: revision_now, + changed_at: stamped_value.changed_at, + }), + ); + assert!( + match old_value { + Some(QueryState::InProgress) => true, + _ => false, + }, + "expected in-progress state", + ); + } + + Ok(stamped_value) + } + + fn overwrite_placeholder( + &self, + map_write: &mut FxHashMap>, + key: &Q::Key, + value: Option>, + ) { + let old_value = if let Some(v) = value { + map_write.insert(key.clone(), v) + } else { + map_write.remove(key) + }; + + assert!( + match old_value { + Some(QueryState::InProgress) => true, + _ => false, + }, + "expected in-progress state", + ); + } +} + +impl QueryStorageOps for DependencyStorage +where + Q: Query, + QC: QueryContext, +{ + fn try_fetch<'q>( + &self, + query: &'q QC, + key: &Q::Key, + descriptor: &QC::QueryDescriptor, + ) -> Result { + let StampedValue { value, changed_at } = self.read(query, key, &descriptor)?; + + query + .salsa_runtime() + .report_query_read(descriptor, changed_at); + + Ok(value) + } + + fn maybe_changed_since( + &self, + query: &'q QC, + revision: Revision, + key: &Q::Key, + _descriptor: &QC::QueryDescriptor, + ) -> bool { + let revision_now = query.salsa_runtime().current_revision(); + + debug!( + "{:?}({:?})::maybe_changed_since(revision={:?}, revision_now={:?})", + Q::default(), + key, + revision, + revision_now, + ); + + let value = { + let map_read = self.map.upgradable_read(); + match map_read.get(key) { + None | Some(QueryState::InProgress) => return true, + Some(QueryState::Memoized(memo)) => { + // If our memo is still up to date, then check if we've + // changed since the revision. + if memo.verified_at == revision_now { + return memo.changed_at > revision; + } + } + } + + let mut map_write = RwLockUpgradableReadGuard::upgrade(map_read); + map_write.insert(key.clone(), QueryState::InProgress) + }; + + // Otherwise, walk the inputs we had and check them. Note that + // we don't want to hold the lock while we do this. + let mut memo = match value { + Some(QueryState::Memoized(memo)) => memo, + _ => unreachable!(), + }; + + if memo + .inputs + .iter() + .all(|old_input| !old_input.maybe_changed_since(query, memo.verified_at)) + { + memo.verified_at = revision_now; + self.overwrite_placeholder( + &mut self.map.write(), + key, + Some(QueryState::Memoized(memo)), + ); + return false; + } + + // Just remove the existing entry. It's out of date. + self.overwrite_placeholder(&mut self.map.write(), key, None); + + true + } +} diff --git a/src/lib.rs b/src/lib.rs index b3d226cc..f1f345d3 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -16,6 +16,7 @@ use std::fmt::Display; use std::fmt::Write; use std::hash::Hash; +pub mod dependencies; pub mod input; pub mod memoized; pub mod runtime; @@ -304,6 +305,22 @@ macro_rules! query_definition { } }; + ( + @filter_attrs { + input { #[storage(dependencies)] $($input:tt)* }; + storage { $storage:tt }; + other_attrs { $($other_attrs:tt)* }; + } + ) => { + $crate::query_definition! { + @filter_attrs { + input { $($input)* }; + storage { dependencies }; + other_attrs { $($other_attrs)* }; + } + } + }; + ( @filter_attrs { input { #[$attr:meta] $($input:tt)* }; @@ -371,6 +388,12 @@ macro_rules! query_definition { $crate::volatile::VolatileStorage<$QC, $Self> }; + ( + @storage_ty[$QC:ident, $Self:ident, dependencies] + ) => { + $crate::dependencies::DependencyStorage<$QC, $Self> + }; + // Accept a "field-like" query definition (input) ( @filter_attrs { diff --git a/tests/incremental/implementation.rs b/tests/incremental/implementation.rs index 10cbc0bd..16a8e35e 100644 --- a/tests/incremental/implementation.rs +++ b/tests/incremental/implementation.rs @@ -1,5 +1,6 @@ use crate::counter::Counter; use crate::log::Log; +use crate::memoized_dep_inputs; use crate::memoized_inputs; use crate::memoized_volatile; @@ -42,10 +43,12 @@ impl TestContextImpl { salsa::query_context_storage! { crate struct TestContextImplStorage for TestContextImpl { - impl memoized_volatile::MemoizedVolatileContext { - fn memoized2() for memoized_volatile::Memoized2; - fn memoized1() for memoized_volatile::Memoized1; - fn volatile() for memoized_volatile::Volatile; + impl memoized_dep_inputs::MemoizedDepInputsContext { + fn dep_memoized2() for memoized_dep_inputs::Memoized2; + fn dep_memoized1() for memoized_dep_inputs::Memoized1; + fn dep_derived1() for memoized_dep_inputs::Derived1; + fn dep_input1() for memoized_dep_inputs::Input1; + fn dep_input2() for memoized_dep_inputs::Input2; } impl memoized_inputs::MemoizedInputsContext { @@ -53,6 +56,12 @@ salsa::query_context_storage! { fn input1() for memoized_inputs::Input1; fn input2() for memoized_inputs::Input2; } + + impl memoized_volatile::MemoizedVolatileContext { + fn memoized2() for memoized_volatile::Memoized2; + fn memoized1() for memoized_volatile::Memoized1; + fn volatile() for memoized_volatile::Volatile; + } } } diff --git a/tests/incremental/main.rs b/tests/incremental/main.rs index 25b9a967..8fe306be 100644 --- a/tests/incremental/main.rs +++ b/tests/incremental/main.rs @@ -4,6 +4,7 @@ mod counter; mod implementation; mod log; +mod memoized_dep_inputs; mod memoized_inputs; mod memoized_volatile; diff --git a/tests/incremental/memoized_dep_inputs.rs b/tests/incremental/memoized_dep_inputs.rs new file mode 100644 index 00000000..256c9c37 --- /dev/null +++ b/tests/incremental/memoized_dep_inputs.rs @@ -0,0 +1,71 @@ +use crate::implementation::{TestContext, TestContextImpl}; + +crate trait MemoizedDepInputsContext: TestContext { + salsa::query_prototype! { + fn dep_memoized2() for Memoized2; + fn dep_memoized1() for Memoized1; + fn dep_derived1() for Derived1; + fn dep_input1() for Input1; + fn dep_input2() for Input2; + } +} + +salsa::query_definition! { + crate Memoized2(query: &impl MemoizedDepInputsContext, (): ()) -> usize { + query.log().add("Memoized2 invoked"); + query.dep_memoized1().read() + } +} + +salsa::query_definition! { + crate Memoized1(query: &impl MemoizedDepInputsContext, (): ()) -> usize { + query.log().add("Memoized1 invoked"); + query.dep_derived1().read() * 2 + } +} + +salsa::query_definition! { + #[storage(dependencies)] + crate Derived1(query: &impl MemoizedDepInputsContext, (): ()) -> usize { + query.log().add("Derived1 invoked"); + query.dep_input1().read() / 2 + } +} + +salsa::query_definition! { + crate Input1: Map<(), usize>; +} + +salsa::query_definition! { + crate Input2: Map<(), usize>; +} + +#[test] +fn revalidate() { + let query = TestContextImpl::default(); + + // Initial run starts from Memoized2: + let v = query.dep_memoized2().read(); + assert_eq!(v, 0); + query.assert_log(&["Memoized2 invoked", "Memoized1 invoked", "Derived1 invoked"]); + + // After that, we first try to validate Memoized1 but wind up + // running Memoized2. Note that we don't try to validate + // Derived1, so it is invoked by Memoized1. + query.dep_input1().set((), 44); + let v = query.dep_memoized2().read(); + assert_eq!(v, 44); + query.assert_log(&["Memoized1 invoked", "Derived1 invoked", "Memoized2 invoked"]); + + // Here validation of Memoized1 succeeds so Memoized2 never runs. + query.dep_input1().set((), 45); + let v = query.dep_memoized2().read(); + assert_eq!(v, 44); + query.assert_log(&["Memoized1 invoked", "Derived1 invoked"]); + + // Here, a change to input2 doesn't affect us, so nothing runs. + query.dep_input2().set((), 45); + let v = query.dep_memoized2().read(); + assert_eq!(v, 44); + query.assert_log(&[]); +}