diff --git a/asyncband/Cargo.toml b/asyncband/Cargo.toml index 9b831d7..9f8d199 100644 --- a/asyncband/Cargo.toml +++ b/asyncband/Cargo.toml @@ -54,7 +54,7 @@ mpsc = [] mutex = [] once = ["semaphore"] once-cell = ["semaphore"] -once-map = ["dep:hashbrown", "once-cell"] +once-map = ["dep:hashbrown"] oneshot = [] pool = ["semaphore"] rwlock = [] diff --git a/asyncband/src/internal/mod.rs b/asyncband/src/internal/mod.rs index 9cabd55..bf798da 100644 --- a/asyncband/src/internal/mod.rs +++ b/asyncband/src/internal/mod.rs @@ -24,6 +24,7 @@ pub(crate) mod atomic_waker; feature = "latch", feature = "mpsc", feature = "mutex", + feature = "once-map", feature = "rwlock", feature = "semaphore", feature = "waitgroup", @@ -52,6 +53,7 @@ pub(crate) mod value_cell; feature = "latch", feature = "mpsc", feature = "mutex", + feature = "once-map", feature = "rwlock", feature = "semaphore", feature = "waitgroup", @@ -83,11 +85,12 @@ pub(crate) mod waitlist; feature = "barrier", feature = "broadcast", feature = "latch", + feature = "once-map", feature = "once", feature = "waitgroup", feature = "watch", ))] -// `barrier` constructs a wait set with `with_capacity`, while countdown-based primitives use -// `new`. One constructor is therefore unused in every single-primitive build. +// `barrier` constructs a wait set with `with_capacity`, while the other primitives use `new`. One +// constructor is therefore unused in every single-primitive build. #[allow(dead_code)] pub(crate) mod waitset; diff --git a/asyncband/src/once/once_map/mod.rs b/asyncband/src/once/once_map/mod.rs index 30f66a2..00148cf 100644 --- a/asyncband/src/once/once_map/mod.rs +++ b/asyncband/src/once/once_map/mod.rs @@ -16,31 +16,134 @@ // under the License. use std::borrow::Borrow; +use std::convert::Infallible; use std::fmt; +use std::future::Future; use std::hash::BuildHasher; use std::hash::Hash; use std::hash::RandomState; +use std::mem; +use std::panic; +use std::panic::AssertUnwindSafe; +use std::pin::Pin; use std::sync::Arc; +use std::task::Context; +use std::task::Poll; use hashbrown::HashTable; use crate::internal::mutex::Mutex; -use crate::once::OnceCell; +use crate::internal::waitset::WaitSet; +use crate::internal::waitset::WakerToken; +use crate::internal::waitset::wake_all; #[cfg(test)] mod tests; -type Entries = HashTable>>; - +// The table is authoritative for each key. Initialization runs outside its lock, then changes the +// same entry from Pending to Ready; pointer identity prevents a detached generation from publishing +// over a replacement created by remove or discard. struct Entry { hash: u64, key: K, - cell: OnceCell, + state: EntryState, +} + +enum EntryState { + Ready(Arc), + Pending(Arc>), +} + +// Coordination exists only while a value is being initialized. A ready entry retains just its key +// and value; callers already joined to a detached initialization keep it alive through this Arc. +struct Initialization { + hash: u64, + state: Mutex>, +} + +enum InitializationState { + Running(WaitSet), + Complete(Completion), +} + +enum Completion { + Value(Arc), + Retry, } enum Lookup { - Ready(V), - Pending(Arc>), + Ready(Arc), + Wait(K, Arc>), + Start(Arc>), +} + +impl Initialization { + fn new(hash: u64) -> Self { + Self { + hash, + state: Mutex::new(InitializationState::Running(WaitSet::new())), + } + } + + fn complete(&self, completion: Completion) -> WaitSet { + let mut state = self.state.lock(); + let InitializationState::Running(waiters) = + mem::replace(&mut *state, InitializationState::Complete(completion)) + else { + unreachable!("pending entry completed more than once") + }; + waiters + } + + fn wait(&self) -> InitializationWait<'_, V> { + InitializationWait { + initialization: self, + token: None, + } + } +} + +struct InitializationWait<'a, V> { + initialization: &'a Initialization, + token: Option, +} + +impl Future for InitializationWait<'_, V> { + type Output = Completion; + + fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { + let Self { + initialization, + token, + } = self.get_mut(); + let replaced = { + let mut state = initialization.state.lock(); + match &mut *state { + InitializationState::Complete(Completion::Value(value)) => { + return Poll::Ready(Completion::Value(Arc::clone(value))); + } + InitializationState::Complete(Completion::Retry) => { + return Poll::Ready(Completion::Retry); + } + InitializationState::Running(waiters) => waiters.register_waker(token, cx), + } + }; + drop(replaced); + Poll::Pending + } +} + +impl Drop for InitializationWait<'_, V> { + fn drop(&mut self) { + let removed = { + let mut state = self.initialization.state.lock(); + match &mut *state { + InitializationState::Running(waiters) => waiters.unregister_waker(&mut self.token), + InitializationState::Complete(_) => None, + } + }; + drop(removed); + } } /// A hash map that runs computation only once for each key and stores the result. @@ -48,22 +151,20 @@ enum Lookup { /// Note that this always clones the value out of the underlying map. Because of this, it's common /// to wrap the `V` in an `Arc` to make cloning cheap. pub struct OnceMap { - // Hashbrown allocates the table lazily, and computation always runs after releasing this lock. - entries: Mutex>, + entries: Mutex>>, hasher: S, } -impl fmt::Debug for OnceMap -where - K: fmt::Debug, - V: fmt::Debug, -{ +impl fmt::Debug for OnceMap { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - // Formatting user types can run arbitrary code, so release the table lock first. - let entries: Vec<_> = self.entries.lock().iter().cloned().collect(); - fmt::Write::write_str(f, "OnceMap ")?; - f.debug_map() - .entries(entries.iter().map(|entry| (&entry.key, &entry.cell))) + let entries = self.entries.lock(); + let pending = entries + .iter() + .filter(|entry| matches!(entry.state, EntryState::Pending(_))) + .count(); + f.debug_struct("OnceMap") + .field("len", &entries.len()) + .field("pending", &pending) .finish() } } @@ -80,51 +181,63 @@ where K: Eq + Hash, S: BuildHasher, { - fn get_or_insert(&self, key: K) -> Lookup - where - V: Clone, - { + fn lookup(&self, key: K) -> Lookup { let hash = self.hasher.hash_one(&key); - let entry = { - let mut entries = self.entries.lock(); - if let Some(entry) = entries - .find(hash, |entry| entry.key.eq(&key)) - .map(Arc::clone) - { - entry - } else { - let entry = Arc::new(Entry { - hash, - key, - cell: OnceCell::new(), - }); - entries.insert_unique(hash, Arc::clone(&entry), |entry| entry.hash); - entry - } - }; + let mut entries = self.entries.lock(); + if let Some(entry) = entries.find(hash, |entry| entry.key.eq(&key)) { + return match &entry.state { + EntryState::Ready(value) => Lookup::Ready(Arc::clone(value)), + EntryState::Pending(initialization) => { + Lookup::Wait(key, Arc::clone(initialization)) + } + }; + } - Self::classify(entry) + let initialization = Arc::new(Initialization::new(hash)); + entries.insert_unique( + hash, + Entry { + hash, + key, + state: EntryState::Pending(Arc::clone(&initialization)), + }, + |entry| entry.hash, + ); + Lookup::Start(initialization) } - fn classify(entry: Arc>) -> Lookup - where - V: Clone, - { - match entry.cell.get().cloned() { - Some(value) => Lookup::Ready(value), - None => Lookup::Pending(entry), + fn finish_initialization( + &self, + initialization: &Arc>, + completion: Completion, + ) -> (WaitSet, Option>) { + let mut stored_initialization = None; + let mut detached_entry = None; + { + let mut entries = self.entries.lock(); + let current = |entry: &Entry| { + matches!( + &entry.state, + EntryState::Pending(stored) if Arc::ptr_eq(stored, initialization) + ) + }; + if let Completion::Value(value) = &completion { + if let Some(entry) = entries.find_mut(initialization.hash, current) { + let EntryState::Pending(pending) = + mem::replace(&mut entry.state, EntryState::Ready(Arc::clone(value))) + else { + unreachable!() + }; + stored_initialization = Some(pending); + } + } else if let Ok(occupied) = entries.find_entry(initialization.hash, current) { + detached_entry = Some(occupied.remove().0); + } } - } + // Never destroy table-owned state while holding the table lock. + drop(stored_initialization); - fn find_entry( - &self, - hash: u64, - matches: impl Fn(&Entry) -> bool, - ) -> Option>> { - self.entries - .lock() - .find(hash, |entry| matches(entry)) - .cloned() + (initialization.complete(completion), detached_entry) } fn get_value(&self, key: &Q) -> Option @@ -134,129 +247,143 @@ where V: Clone, { let hash = self.hasher.hash_one(key); - let entry = self.find_entry(hash, |entry| entry.key.borrow() == key)?; - entry.cell.get().cloned() + let value = { + let entries = self.entries.lock(); + let entry = entries.find(hash, |entry| entry.key.borrow() == key)?; + match &entry.state { + EntryState::Ready(value) => Some(Arc::clone(value)), + EntryState::Pending(_) => None, + } + }?; + Some(value.as_ref().clone()) } - fn remove_entry(&self, key: &Q) -> Option>> + fn detach(&self, key: &Q) -> Option> where K: Borrow, Q: Eq + Hash + ?Sized, { let hash = self.hasher.hash_one(key); - let mut entries = self.entries.lock(); - let occupied = entries - .find_entry(hash, |entry| entry.key.borrow() == key) - .ok()?; - let (entry, _) = occupied.remove(); - drop(entries); - Some(entry) - } - - fn cleanup_abandoned_entry(&self, entry: Arc>) { let removed = { let mut entries = self.entries.lock(); - let Ok(occupied) = entries.find_entry(entry.hash, |stored| Arc::ptr_eq(stored, &entry)) - else { - drop(entries); - drop(entry); - return; - }; - - // With table ownership confirmed and new callers excluded, two owners means the table - // and this cleanup guard are the only remaining references. - if Arc::strong_count(&entry) == 2 && !entry.cell.initialized() { - Some(occupied.remove().0) - } else { - // A waiting cleanup must observe this call's reference being released before it - // can inspect the count while holding the write lock. - drop(entry); - None - } + let occupied = entries + .find_entry(hash, |entry| entry.key.borrow() == key) + .ok()?; + occupied.remove().0 }; - // Key and value destructors must not run while the table is locked. - drop(removed); - } - fn insert(&mut self, key: K, value: V) { - let hash = self.hasher.hash_one(&key); - let entry = Arc::new(Entry { - hash, - key, - cell: OnceCell::from_value(value), - }); - - let mut entries = self.entries.lock(); - let replaced = entries - .find_entry(hash, |stored| stored.key.eq(&entry.key)) - .ok() - .map(|occupied| occupied.remove().0); - entries.insert_unique(hash, entry, |entry| entry.hash); - drop(entries); - drop(replaced); + let Entry { key, state, .. } = removed; + drop(key); + match state { + EntryState::Ready(value) => Some(value), + EntryState::Pending(_) => None, + } } -} -impl FromIterator<(K, V)> for OnceMap -where - K: Eq + Hash, - V: Clone, - S: BuildHasher + Default, -{ - fn from_iter>(iter: T) -> Self { - let iter = iter.into_iter(); - let mut map = Self { - entries: Mutex::new(HashTable::with_capacity(iter.size_hint().0)), - hasher: S::default(), - }; - for (key, value) in iter { - map.insert(key, value); + async fn resolve(&self, mut lookup: Lookup, func: F) -> Result + where + V: Clone, + F: AsyncFnOnce() -> Result, + { + loop { + match lookup { + Lookup::Ready(value) => return Ok(value.as_ref().clone()), + Lookup::Wait(key, initialization) => match initialization.wait().await { + Completion::Value(value) => return Ok(value.as_ref().clone()), + Completion::Retry => lookup = self.lookup(key), + }, + Lookup::Start(initialization) => { + let guard = InitializationGuard::new(self, initialization); + let value = Arc::new(func().await?); + guard.publish(Arc::clone(&value)); + return Ok(value.as_ref().clone()); + } + } } - map } } -// Holds one call's entry so Drop can clean it up if the computation is abandoned. -struct ComputeCleanupGuard<'a, K, V, S> +struct InitializationGuard<'a, K, V, S> where K: Eq + Hash, S: BuildHasher, { - once_map: &'a OnceMap, - entry: Option>>, + map: &'a OnceMap, + initialization: Option>>, } -impl<'a, K, V, S> ComputeCleanupGuard<'a, K, V, S> +impl<'a, K, V, S> InitializationGuard<'a, K, V, S> where K: Eq + Hash, S: BuildHasher, { - fn new(once_map: &'a OnceMap, entry: Arc>) -> Self { + fn new(map: &'a OnceMap, initialization: Arc>) -> Self { Self { - once_map, - entry: Some(entry), + map, + initialization: Some(initialization), } } - fn entry(&self) -> &Arc> { - self.entry.as_ref().unwrap() - } - - fn dismiss(mut self) { - drop(self.entry.take()); + fn publish(mut self, value: Arc) { + let (mut wakers, detached_entry) = self.map.finish_initialization( + self.initialization.as_ref().unwrap(), + Completion::Value(value), + ); + self.initialization.take(); + wake_all(wakers.take_wakers()); + drop(detached_entry); } } -impl Drop for ComputeCleanupGuard<'_, K, V, S> +impl Drop for InitializationGuard<'_, K, V, S> where K: Eq + Hash, S: BuildHasher, { fn drop(&mut self) { - let Some(entry) = self.entry.take() else { + let Some(initialization) = self.initialization.take() else { return; }; - self.once_map.cleanup_abandoned_entry(entry); + let (mut waiters, detached_entry) = self + .map + .finish_initialization(&initialization, Completion::Retry); + // Cleanup runs during cancellation and may already be unwinding from user code. + let _ = panic::catch_unwind(AssertUnwindSafe(|| wake_all(waiters.take_wakers()))); + drop(detached_entry); + } +} + +impl FromIterator<(K, V)> for OnceMap +where + K: Eq + Hash, + V: Clone, + S: BuildHasher + Default, +{ + fn from_iter>(iter: T) -> Self { + let iter = iter.into_iter(); + let hasher = S::default(); + let mut entries: HashTable> = HashTable::with_capacity(iter.size_hint().0); + for (key, value) in iter { + let hash = hasher.hash_one(&key); + let replaced = entries + .find_entry(hash, |entry| entry.key.eq(&key)) + .ok() + .map(|occupied| occupied.remove().0); + entries.insert_unique( + hash, + Entry { + hash, + key, + state: EntryState::Ready(Arc::new(value)), + }, + |entry| entry.hash, + ); + drop(replaced); + } + Self { + entries: Mutex::new(entries), + hasher, + } } } @@ -307,15 +434,15 @@ where where F: AsyncFnOnce() -> V, { - let entry = match self.get_or_insert(key) { - Lookup::Ready(value) => return value, - Lookup::Pending(entry) => entry, - }; - - let guard = ComputeCleanupGuard::new(self, entry); - let result = guard.entry().cell.get_or_init(func).await.clone(); - guard.dismiss(); - result + match self.lookup(key) { + Lookup::Ready(value) => value.as_ref().clone(), + pending => match self + .resolve(pending, async || Ok::(func().await)) + .await + { + Ok(value) => value, + }, + } } /// Compute the value for the given key if absent. @@ -329,15 +456,10 @@ where where F: AsyncFnOnce() -> Result, { - let entry = match self.get_or_insert(key) { - Lookup::Ready(value) => return Ok(value), - Lookup::Pending(entry) => entry, - }; - - let guard = ComputeCleanupGuard::new(self, entry); - let result = guard.entry().cell.get_or_try_init(func).await?.clone(); - guard.dismiss(); - Ok(result) + match self.lookup(key) { + Lookup::Ready(value) => Ok(value.as_ref().clone()), + pending => self.resolve(pending, func).await, + } } /// Get a clone of the value for the given key if exists. @@ -362,7 +484,7 @@ where K: Borrow, Q: Hash + Eq + ?Sized, { - drop(self.remove_entry(key)); + drop(self.detach(key)); } /// Remove the given key from the map and return a *clone* of the value if exists. @@ -379,7 +501,7 @@ where K: Borrow, Q: Hash + Eq + ?Sized, { - let entry = self.remove_entry(key)?; - entry.cell.get().cloned() + let value = self.detach(key)?; + Some(value.as_ref().clone()) } } diff --git a/asyncband/src/once/once_map/tests.rs b/asyncband/src/once/once_map/tests.rs index 66aaba3..1f7ad6a 100644 --- a/asyncband/src/once/once_map/tests.rs +++ b/asyncband/src/once/once_map/tests.rs @@ -19,6 +19,7 @@ use std::hash::BuildHasherDefault; use std::hash::Hasher; use std::sync::Arc; +use super::InitializationGuard; use super::Lookup; use super::OnceMap; use crate::test_support::poll_once; @@ -103,7 +104,7 @@ fn pending_computation_does_not_block_another_key() { } #[tokio::test] -async fn failed_compute_preserves_entry_for_waiter_retry() { +async fn failed_compute_wakes_waiter_to_retry() { let map = OnceMap::new(); let (release_tx, release_rx) = tokio::sync::oneshot::channel(); @@ -121,7 +122,7 @@ async fn failed_compute_preserves_entry_for_waiter_retry() { release_tx.send(()).unwrap(); assert_eq!(first.await, Err("fail")); - assert_eq!(map.len(), 1); + assert_eq!(map.len(), 0); assert_eq!(retry.await, Ok(1)); assert_eq!(map.get("key"), Some(1)); } @@ -129,11 +130,11 @@ async fn failed_compute_preserves_entry_for_waiter_retry() { #[test] fn abandoned_pending_entry_is_removed_when_last_caller_leaves() { let map = OnceMap::<&str, i32>::new(); - let Lookup::Pending(entry) = map.get_or_insert("key") else { + let Lookup::Start(entry) = map.lookup("key") else { unreachable!() }; - map.cleanup_abandoned_entry(entry); + drop(InitializationGuard::new(&map, entry)); assert_eq!(map.len(), 0); } @@ -155,13 +156,13 @@ fn colliding_ready_entries_can_be_unlinked_independently() { #[test] fn colliding_pending_entries_are_tracked_independently() { let map: OnceMap> = OnceMap::default(); - let Lookup::Pending(first) = map.get_or_insert(1) else { + let Lookup::Start(first) = map.lookup(1) else { unreachable!() }; - let Lookup::Pending(first_waiter) = map.get_or_insert(1) else { + let Lookup::Wait(_, first_waiter) = map.lookup(1) else { unreachable!() }; - let Lookup::Pending(second) = map.get_or_insert(2) else { + let Lookup::Start(second) = map.lookup(2) else { unreachable!() }; @@ -169,7 +170,7 @@ fn colliding_pending_entries_are_tracked_independently() { assert!(!Arc::ptr_eq(&first, &second)); drop(first_waiter); - map.cleanup_abandoned_entry(first); - map.cleanup_abandoned_entry(second); + drop(InitializationGuard::new(&map, first)); + drop(InitializationGuard::new(&map, second)); assert_eq!(map.len(), 0); } diff --git a/tests-integration/tests/once_map_test.rs b/tests-integration/tests/once_map_test.rs index aa150d3..58cb356 100644 --- a/tests-integration/tests/once_map_test.rs +++ b/tests-integration/tests/once_map_test.rs @@ -16,6 +16,7 @@ // under the License. use std::borrow::Borrow; +use std::cell::Cell; use std::collections::hash_map::RandomState; use std::hash::BuildHasherDefault; use std::hash::Hash; @@ -25,6 +26,7 @@ use std::sync::atomic::AtomicUsize; use std::sync::atomic::Ordering; use asyncband::once::OnceMap; +use tests_integration::poll_once; #[test] fn constructors_and_default() { @@ -238,6 +240,37 @@ async fn remove_while_computing_allows_a_new_generation() { assert_eq!(map.get("key"), Some(2)); } +#[test] +fn remove_detaches_waiters_from_a_new_generation() { + let map = OnceMap::new(); + let release = Cell::new(false); + + let mut leader = std::pin::pin!(map.compute("key", async || { + std::future::poll_fn(|_| { + if release.get() { + std::task::Poll::Ready(()) + } else { + std::task::Poll::Pending + } + }) + .await; + 1 + })); + assert!(poll_once(leader.as_mut()).is_pending()); + + let mut waiter = std::pin::pin!(map.compute("key", async || 2)); + assert!(poll_once(waiter.as_mut()).is_pending()); + + assert_eq!(map.remove("key"), None); + let mut replacement = std::pin::pin!(map.compute("key", async || 3)); + assert_eq!(poll_once(replacement.as_mut()), std::task::Poll::Ready(3)); + + release.set(true); + assert_eq!(poll_once(leader.as_mut()), std::task::Poll::Ready(1)); + assert_eq!(poll_once(waiter.as_mut()), std::task::Poll::Ready(1)); + assert_eq!(map.get("key"), Some(3)); +} + #[tokio::test] async fn get_returns_none_while_computing() { let map = Arc::new(OnceMap::new());