From 3275c5b92c75d5a757fcad2095a9a16d435e5c58 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Sun, 30 Jun 2024 21:52:43 +0000 Subject: [PATCH] types: drop BoundMutex and instead use references into the type context slab This completes the transition to using type contexts to keep track of (and allocate/mass-deallocate) type bounds :). There are three major improvements in this changeset: * We no longer leak memory when infinite type bounds are constructed. * It is no longer possible to create distinct programs where the variables are mixed up. (Ok, you can do this still, but you have to explicitly use the same type context for both programs, which is an obvious bug.) * Unification and binding happen atomically, so if you are doing type inference across multiple threads, crosstalk won't happen between them. --- src/types/context.rs | 65 ++++++++++++++++++++++++++------------------ src/types/mod.rs | 39 -------------------------- 2 files changed, 39 insertions(+), 65 deletions(-) diff --git a/src/types/context.rs b/src/types/context.rs index 981ca355..04775a7a 100644 --- a/src/types/context.rs +++ b/src/types/context.rs @@ -19,7 +19,6 @@ use std::sync::{Arc, Mutex, MutexGuard}; use crate::dag::{Dag, DagLike}; -use super::bound_mutex::BoundMutex; use super::{Bound, CompleteBound, Error, Final, Type}; /// Type inference context, or handle to a context. @@ -60,9 +59,13 @@ impl Context { /// Helper function to allocate a bound and return a reference to it. fn alloc_bound(&self, bound: Bound) -> BoundRef { + let mut lock = self.lock(); + lock.slab.push(bound); + let index = lock.slab.len() - 1; + BoundRef { context: Arc::as_ptr(&self.slab), - index: Arc::new(BoundMutex::new(bound)), + index, } } @@ -132,7 +135,8 @@ impl Context { /// Panics if passed a `BoundRef` that was not allocated by this context. pub fn get(&self, bound: &BoundRef) -> Bound { bound.assert_matches_context(self); - bound.index.get().shallow_clone() + let lock = self.lock(); + lock.slab[bound.index].shallow_clone() } /// Reassigns a bound to a different bound. @@ -147,8 +151,8 @@ impl Context { /// /// Also panics if passed a `BoundRef` that was not allocated by this context. pub fn reassign_non_complete(&self, bound: BoundRef, new: Bound) { - bound.assert_matches_context(self); - bound.index.set(new) + let mut lock = self.lock(); + lock.reassign_non_complete(bound, new); } /// Binds the type to a given bound. If this fails, attach the provided @@ -157,7 +161,7 @@ impl Context { /// Fails if the type has an existing incompatible bound. pub fn bind(&self, existing: &Type, new: Bound, hint: &'static str) -> Result<(), Error> { let existing_root = existing.bound.root(); - let lock = self.lock(); + let mut lock = self.lock(); lock.bind(existing_root, new, hint) } @@ -165,7 +169,7 @@ impl Context { /// /// Fails if the bounds on the two types are incompatible pub fn unify(&self, ty1: &Type, ty2: &Type, hint: &'static str) -> Result<(), Error> { - let lock = self.lock(); + let mut lock = self.lock(); lock.unify(ty1, ty2, hint) } @@ -180,9 +184,7 @@ impl Context { #[derive(Debug, Clone)] pub struct BoundRef { context: *const Mutex>, - // Will become an index into the context in a latter commit, but for - // now we set it to an Arc to preserve semantics. - index: Arc, + index: usize, } impl BoundRef { @@ -200,7 +202,7 @@ impl BoundRef { pub fn occurs_check_id(&self) -> OccursCheckId { OccursCheckId { context: self.context, - index: Arc::as_ptr(&self.index), + index: self.index, } } } @@ -211,13 +213,13 @@ impl super::PointerLike for BoundRef { self.context, other.context, "tried to compare two bounds from different inference contexts" ); - Arc::ptr_eq(&self.index, &other.index) + self.index == other.index } fn shallow_clone(&self) -> Self { BoundRef { context: self.context, - index: Arc::clone(&self.index), + index: self.index, } } } @@ -243,7 +245,7 @@ pub struct OccursCheckId { context: *const Mutex>, // Will become an index into the context in a latter commit, but for // now we set it to an Arc to preserve semantics. - index: *const BoundMutex, + index: usize, } /// Structure representing an inference context with its slab allocator mutex locked. @@ -255,17 +257,25 @@ struct LockedContext<'ctx> { } impl<'ctx> LockedContext<'ctx> { + fn reassign_non_complete(&mut self, bound: BoundRef, new: Bound) { + assert!( + !matches!(self.slab[bound.index], Bound::Complete(..)), + "tried to modify finalized type", + ); + self.slab[bound.index] = new; + } + /// Unify the type with another one. /// /// Fails if the bounds on the two types are incompatible - fn unify(&self, existing: &Type, other: &Type, hint: &'static str) -> Result<(), Error> { + fn unify(&mut self, existing: &Type, other: &Type, hint: &'static str) -> Result<(), Error> { existing.bound.unify(&other.bound, |x_bound, y_bound| { - self.bind(x_bound, y_bound.index.get(), hint) + self.bind(x_bound, self.slab[y_bound.index].shallow_clone(), hint) }) } - fn bind(&self, existing: BoundRef, new: Bound, hint: &'static str) -> Result<(), Error> { - let existing_bound = existing.index.get(); + fn bind(&mut self, existing: BoundRef, new: Bound, hint: &'static str) -> Result<(), Error> { + let existing_bound = self.slab[existing.index].shallow_clone(); let bind_error = || Error::Bind { existing_bound: existing_bound.shallow_clone(), new_bound: new.shallow_clone(), @@ -278,7 +288,7 @@ impl<'ctx> LockedContext<'ctx> { // Free types are simply dropped and replaced by the new bound (Bound::Free(_), _) => { // Free means non-finalized, so set() is ok. - existing.index.set(new); + self.reassign_non_complete(existing, new); Ok(()) } // Binding complete->complete shouldn't ever happen, but if so, we just @@ -320,14 +330,17 @@ impl<'ctx> LockedContext<'ctx> { // // It also gives the user access to more information about the type, // prior to finalization. - if let (Some(data1), Some(data2)) = (y1.final_data(), y2.final_data()) { - existing - .index - .set(Bound::Complete(if let Bound::Sum(..) = existing_bound { - Final::sum(data1, data2) + let y1_bound = &self.slab[y1.bound.root().index]; + let y2_bound = &self.slab[y2.bound.root().index]; + if let (Bound::Complete(data1), Bound::Complete(data2)) = (y1_bound, y2_bound) { + self.reassign_non_complete( + existing, + Bound::Complete(if let Bound::Sum(..) = existing_bound { + Final::sum(Arc::clone(data1), Arc::clone(data2)) } else { - Final::product(data1, data2) - })); + Final::product(Arc::clone(data1), Arc::clone(data2)) + }), + ); } Ok(()) } diff --git a/src/types/mod.rs b/src/types/mod.rs index fd5c650d..90ae2b54 100644 --- a/src/types/mod.rs +++ b/src/types/mod.rs @@ -148,45 +148,6 @@ impl fmt::Display for Error { impl std::error::Error for Error {} -mod bound_mutex { - use super::Bound; - use std::fmt; - use std::sync::Mutex; - - /// Source or target type of a Simplicity expression - pub struct BoundMutex { - /// The type's status according to the union-bound algorithm. - inner: Mutex, - } - - impl fmt::Debug for BoundMutex { - fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { - self.get().fmt(f) - } - } - - impl BoundMutex { - pub fn new(bound: Bound) -> Self { - BoundMutex { - inner: Mutex::new(bound), - } - } - - pub fn get(&self) -> Bound { - self.inner.lock().unwrap().shallow_clone() - } - - pub fn set(&self, new: Bound) { - let mut lock = self.inner.lock().unwrap(); - assert!( - !matches!(*lock, Bound::Complete(..)), - "tried to modify finalized type", - ); - *lock = new; - } - } -} - /// The state of a [`Type`] based on all constraints currently imposed on it. #[derive(Clone, Debug)] pub enum Bound {