From 500a60d40de1278c53ac1472baf5961fd230e1e7 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 18 Jan 2019 05:38:44 -0500 Subject: [PATCH 1/5] make `LocalState` encapsulate its state --- src/runtime.rs | 112 ++++++++++++++++++++++++++++++++----------------- 1 file changed, 73 insertions(+), 39 deletions(-) diff --git a/src/runtime.rs b/src/runtime.rs index ab909d6b..effb66eb 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -4,6 +4,7 @@ use log::debug; use parking_lot::{Mutex, RwLock}; use rustc_hash::{FxHashMap, FxHasher}; use smallvec::SmallVec; +use std::cell::Ref; use std::cell::RefCell; use std::fmt::Write; use std::hash::BuildHasherDefault; @@ -29,7 +30,7 @@ pub struct Runtime { revision_guard: Option>, /// Local state that is specific to this runtime (thread). - local_state: RefCell>, + local_state: LocalState, /// Shared state that is accessible via all runtimes. shared_state: Arc>, @@ -99,7 +100,7 @@ where "invoked `snapshot` with a non-matching database" ); - if self.local_state.borrow().query_in_progress() { + if self.local_state.query_in_progress() { panic!("it is not legal to `snapshot` during a query (see salsa-rs/salsa#80)"); } @@ -151,11 +152,7 @@ where /// Returns the descriptor for the query that this thread is /// actively executing (if any). pub fn active_query(&self) -> Option { - self.local_state - .borrow() - .query_stack - .last() - .map(|active_query| active_query.descriptor.clone()) + self.local_state.active_query() } /// Read current value of the revision counter. @@ -303,7 +300,7 @@ where } pub(crate) fn permits_increment(&self) -> bool { - self.revision_guard.is_none() && !self.local_state.borrow().query_in_progress() + self.revision_guard.is_none() && !self.local_state.query_in_progress() } pub(crate) fn execute_query_implementation( @@ -322,13 +319,7 @@ where }); // Push the active query onto the stack. - let push_len = { - let mut local_state = self.local_state.borrow_mut(); - local_state - .query_stack - .push(ActiveQuery::new(descriptor.clone())); - local_state.query_stack.len() - }; + let push_len = self.local_state.push_query(descriptor); // Execute user's code, accumulating inputs etc. let value = execute(); @@ -338,14 +329,7 @@ where subqueries, changed_at, .. - } = { - let mut local_state = self.local_state.borrow_mut(); - - // Sanity check: pushes and pops should be balanced. - assert_eq!(local_state.query_stack.len(), push_len); - - local_state.query_stack.pop().unwrap() - }; + } = self.local_state.pop_query(push_len); ComputedQueryResult { value, @@ -367,15 +351,12 @@ where descriptor: &DB::QueryDescriptor, changed_at: ChangedAt, ) { - if let Some(top_query) = self.local_state.borrow_mut().query_stack.last_mut() { - top_query.add_read(descriptor, changed_at); - } + self.local_state.report_query_read(descriptor, changed_at); } pub(crate) fn report_untracked_read(&self) { - if let Some(top_query) = self.local_state.borrow_mut().query_stack.last_mut() { - top_query.add_untracked_read(self.current_revision()); - } + self.local_state + .report_untracked_read(self.current_revision()); } /// An "anonymous" read is a read that doesn't come from executing @@ -387,18 +368,14 @@ where /// /// This is used when queries check if they have been canceled. fn report_anon_read(&self, revision: Revision) { - if let Some(top_query) = self.local_state.borrow_mut().query_stack.last_mut() { - top_query.add_anon_read(revision); - } + self.local_state.report_anon_read(revision) } /// Obviously, this should be user configurable at some point. pub(crate) fn report_unexpected_cycle(&self, descriptor: DB::QueryDescriptor) -> ! { debug!("report_unexpected_cycle(descriptor={:?})", descriptor); - let local_state = self.local_state.borrow(); - let LocalState { query_stack, .. } = &*local_state; - + let query_stack = self.local_state.borrow_query_stack(); let start_index = (0..query_stack.len()) .rev() .filter(|&i| query_stack[i].descriptor == descriptor) @@ -501,10 +478,18 @@ where } } -/// State that will be specific to a single execution threads (when we -/// support multiple threads) +/// State that is specific to a single execution thread. +/// +/// Internally, this type uses ref-cells. +/// +/// **Note also that all mutations to the database handle (and hence +/// to the local-state) must be undone during unwinding.** struct LocalState { - query_stack: Vec>, + /// Vector of active queries. + /// + /// Unwinding note: pushes onto this vector must be popped -- even + /// during unwinding. + query_stack: RefCell>>, } impl Default for LocalState { @@ -516,8 +501,57 @@ impl Default for LocalState { } impl LocalState { + fn push_query(&self, descriptor: &DB::QueryDescriptor) -> usize { + let mut query_stack = self.query_stack.borrow_mut(); + query_stack.push(ActiveQuery::new(descriptor.clone())); + query_stack.len() + } + + fn pop_query(&self, push_len: usize) -> ActiveQuery { + let mut query_stack = self.query_stack.borrow_mut(); + + // Sanity check: pushes and pops should be balanced. + assert_eq!(query_stack.len(), push_len); + + query_stack.pop().unwrap() + } + + /// Returns a reference to the active query stack. + /// + /// **Warning:** Because this reference holds the ref-cell lock, + /// you should not use any mutating methods of `LocalState` while + /// reading from it. + fn borrow_query_stack(&self) -> Ref<'_, Vec>> { + self.query_stack.borrow() + } + fn query_in_progress(&self) -> bool { - !self.query_stack.is_empty() + !self.query_stack.borrow().is_empty() + } + + fn active_query(&self) -> Option { + self.query_stack + .borrow() + .last() + .map(|active_query| active_query.descriptor.clone()) + } + + fn report_query_read(&self, descriptor: &DB::QueryDescriptor, changed_at: ChangedAt) { + if let Some(top_query) = self.query_stack.borrow_mut().last_mut() { + top_query.add_read(descriptor, changed_at); + } + } + + fn report_untracked_read(&self, current_revision: Revision) { + if let Some(top_query) = self.query_stack.borrow_mut().last_mut() { + top_query.add_untracked_read(current_revision); + } + } + + fn report_anon_read(&self, revision: Revision) { + if let Some(top_query) = self.query_stack.borrow_mut().last_mut() { + top_query.add_anon_read(revision); + } } } From 15e1366d81359896f2dfea5b7867f7075639652a Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 18 Jan 2019 05:43:03 -0500 Subject: [PATCH 2/5] move local-state into its own module --- src/runtime.rs | 82 ++--------------------------------- src/runtime/local_state.rs | 87 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 79 deletions(-) create mode 100644 src/runtime/local_state.rs diff --git a/src/runtime.rs b/src/runtime.rs index effb66eb..5c954511 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -4,8 +4,6 @@ use log::debug; use parking_lot::{Mutex, RwLock}; use rustc_hash::{FxHashMap, FxHasher}; use smallvec::SmallVec; -use std::cell::Ref; -use std::cell::RefCell; use std::fmt::Write; use std::hash::BuildHasherDefault; use std::sync::atomic::{AtomicUsize, Ordering}; @@ -13,6 +11,9 @@ use std::sync::Arc; pub(crate) type FxIndexSet = indexmap::IndexSet>; +mod local_state; +use local_state::LocalState; + /// The salsa runtime stores the storage for all queries as well as /// tracking the query stack and dependencies between cycles. /// @@ -478,83 +479,6 @@ where } } -/// State that is specific to a single execution thread. -/// -/// Internally, this type uses ref-cells. -/// -/// **Note also that all mutations to the database handle (and hence -/// to the local-state) must be undone during unwinding.** -struct LocalState { - /// Vector of active queries. - /// - /// Unwinding note: pushes onto this vector must be popped -- even - /// during unwinding. - query_stack: RefCell>>, -} - -impl Default for LocalState { - fn default() -> Self { - LocalState { - query_stack: Default::default(), - } - } -} - -impl LocalState { - fn push_query(&self, descriptor: &DB::QueryDescriptor) -> usize { - let mut query_stack = self.query_stack.borrow_mut(); - query_stack.push(ActiveQuery::new(descriptor.clone())); - query_stack.len() - } - - fn pop_query(&self, push_len: usize) -> ActiveQuery { - let mut query_stack = self.query_stack.borrow_mut(); - - // Sanity check: pushes and pops should be balanced. - assert_eq!(query_stack.len(), push_len); - - query_stack.pop().unwrap() - } - - /// Returns a reference to the active query stack. - /// - /// **Warning:** Because this reference holds the ref-cell lock, - /// you should not use any mutating methods of `LocalState` while - /// reading from it. - fn borrow_query_stack(&self) -> Ref<'_, Vec>> { - self.query_stack.borrow() - } - - fn query_in_progress(&self) -> bool { - !self.query_stack.borrow().is_empty() - } - - fn active_query(&self) -> Option { - self.query_stack - .borrow() - .last() - .map(|active_query| active_query.descriptor.clone()) - } - - fn report_query_read(&self, descriptor: &DB::QueryDescriptor, changed_at: ChangedAt) { - if let Some(top_query) = self.query_stack.borrow_mut().last_mut() { - top_query.add_read(descriptor, changed_at); - } - } - - fn report_untracked_read(&self, current_revision: Revision) { - if let Some(top_query) = self.query_stack.borrow_mut().last_mut() { - top_query.add_untracked_read(current_revision); - } - } - - fn report_anon_read(&self, revision: Revision) { - if let Some(top_query) = self.query_stack.borrow_mut().last_mut() { - top_query.add_anon_read(revision); - } - } -} - struct ActiveQuery { /// What query is executing descriptor: DB::QueryDescriptor, diff --git a/src/runtime/local_state.rs b/src/runtime/local_state.rs new file mode 100644 index 00000000..1739f651 --- /dev/null +++ b/src/runtime/local_state.rs @@ -0,0 +1,87 @@ +use crate::runtime::ActiveQuery; +use crate::runtime::ChangedAt; +use crate::runtime::Revision; +use crate::Database; +use std::cell::Ref; +use std::cell::RefCell; + +/// State that is specific to a single execution thread. +/// +/// Internally, this type uses ref-cells. +/// +/// **Note also that all mutations to the database handle (and hence +/// to the local-state) must be undone during unwinding.** +pub(super) struct LocalState { + /// Vector of active queries. + /// + /// Unwinding note: pushes onto this vector must be popped -- even + /// during unwinding. + query_stack: RefCell>>, +} + +impl Default for LocalState { + fn default() -> Self { + LocalState { + query_stack: Default::default(), + } + } +} + +impl LocalState { + pub(super) fn push_query(&self, descriptor: &DB::QueryDescriptor) -> usize { + let mut query_stack = self.query_stack.borrow_mut(); + query_stack.push(ActiveQuery::new(descriptor.clone())); + query_stack.len() + } + + pub(super) fn pop_query(&self, push_len: usize) -> ActiveQuery { + let mut query_stack = self.query_stack.borrow_mut(); + + // Sanity check: pushes and pops should be balanced. + assert_eq!(query_stack.len(), push_len); + + query_stack.pop().unwrap() + } + + /// Returns a reference to the active query stack. + /// + /// **Warning:** Because this reference holds the ref-cell lock, + /// you should not use any mutating methods of `LocalState` while + /// reading from it. + pub(super) fn borrow_query_stack(&self) -> Ref<'_, Vec>> { + self.query_stack.borrow() + } + + pub(super) fn query_in_progress(&self) -> bool { + !self.query_stack.borrow().is_empty() + } + + pub(super) fn active_query(&self) -> Option { + self.query_stack + .borrow() + .last() + .map(|active_query| active_query.descriptor.clone()) + } + + pub(super) fn report_query_read( + &self, + descriptor: &DB::QueryDescriptor, + changed_at: ChangedAt, + ) { + if let Some(top_query) = self.query_stack.borrow_mut().last_mut() { + top_query.add_read(descriptor, changed_at); + } + } + + pub(super) fn report_untracked_read(&self, current_revision: Revision) { + if let Some(top_query) = self.query_stack.borrow_mut().last_mut() { + top_query.add_untracked_read(current_revision); + } + } + + pub(super) fn report_anon_read(&self, revision: Revision) { + if let Some(top_query) = self.query_stack.borrow_mut().last_mut() { + top_query.add_anon_read(revision); + } + } +} From 21519e6ff7ff5621c98356a0b822076107ce117a Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 18 Jan 2019 05:49:25 -0500 Subject: [PATCH 3/5] introduce a `ActiveQueryGuard` type --- src/runtime.rs | 4 ++-- src/runtime/local_state.rs | 46 +++++++++++++++++++++++++++++--------- 2 files changed, 37 insertions(+), 13 deletions(-) diff --git a/src/runtime.rs b/src/runtime.rs index 5c954511..d63ba8c9 100644 --- a/src/runtime.rs +++ b/src/runtime.rs @@ -320,7 +320,7 @@ where }); // Push the active query onto the stack. - let push_len = self.local_state.push_query(descriptor); + let active_query = self.local_state.push_query(descriptor); // Execute user's code, accumulating inputs etc. let value = execute(); @@ -330,7 +330,7 @@ where subqueries, changed_at, .. - } = self.local_state.pop_query(push_len); + } = active_query.complete(); ComputedQueryResult { value, diff --git a/src/runtime/local_state.rs b/src/runtime/local_state.rs index 1739f651..d7075f72 100644 --- a/src/runtime/local_state.rs +++ b/src/runtime/local_state.rs @@ -28,19 +28,13 @@ impl Default for LocalState { } impl LocalState { - pub(super) fn push_query(&self, descriptor: &DB::QueryDescriptor) -> usize { + pub(super) fn push_query(&self, descriptor: &DB::QueryDescriptor) -> ActiveQueryGuard<'_, DB> { let mut query_stack = self.query_stack.borrow_mut(); query_stack.push(ActiveQuery::new(descriptor.clone())); - query_stack.len() - } - - pub(super) fn pop_query(&self, push_len: usize) -> ActiveQuery { - let mut query_stack = self.query_stack.borrow_mut(); - - // Sanity check: pushes and pops should be balanced. - assert_eq!(query_stack.len(), push_len); - - query_stack.pop().unwrap() + ActiveQueryGuard { + local_state: self, + push_len: query_stack.len(), + } } /// Returns a reference to the active query stack. @@ -85,3 +79,33 @@ impl LocalState { } } } + +/// When a query is pushed onto the `active_query` stack, this guard +/// is returned to represent its slot. The guard can be used to pop +/// the query from the stack -- in the case of unwinding, the guard's +/// destructor will also remove the query. +pub(super) struct ActiveQueryGuard<'me, DB: Database> { + local_state: &'me LocalState, + push_len: usize, +} + +impl<'me, DB> ActiveQueryGuard<'me, DB> +where + DB: Database, +{ + fn pop_helper(&self) -> ActiveQuery { + let mut query_stack = self.local_state.query_stack.borrow_mut(); + + // Sanity check: pushes and pops should be balanced. + assert_eq!(query_stack.len(), self.push_len); + + query_stack.pop().unwrap() + } + + /// Invoked when the query has successfully completed execution. + pub(super) fn complete(self) -> ActiveQuery { + let query = self.pop_helper(); + std::mem::forget(self); + query + } +} From 27af8ca820dbaa33b9e8c3ac8095582511847540 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 18 Jan 2019 05:52:02 -0500 Subject: [PATCH 4/5] add (failing) test that checks that we clear query stack --- tests/panic_safely.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/panic_safely.rs b/tests/panic_safely.rs index 01d00d47..8f256674 100644 --- a/tests/panic_safely.rs +++ b/tests/panic_safely.rs @@ -64,3 +64,17 @@ fn storages_are_unwind_safe() { fn check_unwind_safe() {} check_unwind_safe::<&DatabaseStruct>(); } + +#[test] +fn panics_clear_query_stack() { + let db = DatabaseStruct::default(); + + // Invoke `db.panic_if_not_one() without having set `db.input`. `db.input` + // will default to 0 and we should catch the panic. + let result = panic::catch_unwind(AssertUnwindSafe(|| db.panic_safely())); + assert!(result.is_err()); + + // The database has been poisoned and any attempt to increment the + // revision should panic. + assert_eq!(db.salsa_runtime().active_query(), None); +} From f0c5cffd8922e493dc17cc30b040780e94c6cc63 Mon Sep 17 00:00:00 2001 From: Niko Matsakis Date: Fri, 18 Jan 2019 05:52:47 -0500 Subject: [PATCH 5/5] add Drop to recover from panic gracefully --- src/runtime/local_state.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/runtime/local_state.rs b/src/runtime/local_state.rs index d7075f72..496d3afc 100644 --- a/src/runtime/local_state.rs +++ b/src/runtime/local_state.rs @@ -109,3 +109,12 @@ where query } } + +impl<'me, DB> Drop for ActiveQueryGuard<'me, DB> +where + DB: Database, +{ + fn drop(&mut self) { + self.pop_helper(); + } +}