From 662fcf9784b9f54af9b2b904bba1f3ad675cc79d Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Wed, 18 May 2022 18:46:17 +0100 Subject: [PATCH 1/4] replace parking_lot with parking_lot_core --- Cargo.toml | 4 ++- src/imp_pl.rs | 82 ++++++++++++++++++++++++++++++++------------------- 2 files changed, 54 insertions(+), 32 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 92f1a1c..fadaef7 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -22,7 +22,7 @@ members = ["xtask"] # Uses parking_lot to implement once_cell::sync::OnceCell. # This makes not speed difference, but makes each OnceCell # for up to 16 bytes smaller, depending on the size of the T. -parking_lot = { version = "0.12", optional = true, default_features = false } +parking_lot_core = { version = "0.9.3", optional = true, default_features = false } # To be used in order to enable the race feature on targets # that do not have atomics @@ -48,6 +48,8 @@ race = [] # At the moment, this feature is unused. unstable = [] +parking_lot = ["parking_lot_core"] + [[example]] name = "bench" required-features = ["std"] diff --git a/src/imp_pl.rs b/src/imp_pl.rs index a9bae85..684fd9b 100644 --- a/src/imp_pl.rs +++ b/src/imp_pl.rs @@ -2,19 +2,18 @@ use std::{ cell::UnsafeCell, hint, panic::{RefUnwindSafe, UnwindSafe}, - sync::atomic::{AtomicBool, Ordering}, + sync::atomic::{AtomicU8, Ordering}, }; -use parking_lot::Mutex; - -use crate::take_unchecked; - pub(crate) struct OnceCell { - mutex: Mutex<()>, - is_initialized: AtomicBool, + state: AtomicU8, value: UnsafeCell>, } +const INCOMPLETE: u8 = 0x0; +const RUNNING: u8 = 0x1; +const COMPLETE: u8 = 0x2; + // Why do we need `T: Send`? // Thread A creates a `OnceCell` and shares it with // scoped thread B, which fills the cell, which is @@ -28,25 +27,17 @@ impl UnwindSafe for OnceCell {} impl OnceCell { pub(crate) const fn new() -> OnceCell { - OnceCell { - mutex: parking_lot::const_mutex(()), - is_initialized: AtomicBool::new(false), - value: UnsafeCell::new(None), - } + OnceCell { state: AtomicU8::new(INCOMPLETE), value: UnsafeCell::new(None) } } pub(crate) const fn with_value(value: T) -> OnceCell { - OnceCell { - mutex: parking_lot::const_mutex(()), - is_initialized: AtomicBool::new(true), - value: UnsafeCell::new(Some(value)), - } + OnceCell { state: AtomicU8::new(COMPLETE), value: UnsafeCell::new(Some(value)) } } /// Safety: synchronizes with store to value via Release/Acquire. #[inline] pub(crate) fn is_initialized(&self) -> bool { - self.is_initialized.load(Ordering::Acquire) + self.state.load(Ordering::Acquire) == COMPLETE } /// Safety: synchronizes with store to value via `is_initialized` or mutex @@ -59,7 +50,7 @@ impl OnceCell { let mut f = Some(f); let mut res: Result<(), E> = Ok(()); let slot: *mut Option = self.value.get(); - initialize_inner(&self.mutex, &self.is_initialized, &mut || { + initialize_inner(&self.state, &mut || { // We are calling user-supplied function and need to be careful. // - if it returns Err, we unlock mutex and return without touching anything // - if it panics, we unlock mutex and propagate panic without touching anything @@ -68,7 +59,7 @@ impl OnceCell { // but that is more complicated // - finally, if it returns Ok, we store the value and store the flag with // `Release`, which synchronizes with `Acquire`s. - let f = unsafe { take_unchecked(&mut f) }; + let f = unsafe { crate::take_unchecked(&mut f) }; match f() { Ok(value) => unsafe { // Safe b/c we have a unique access and no panic may happen @@ -121,18 +112,47 @@ impl OnceCell { } } +struct Guard<'a> { + state: &'a AtomicU8, + new_state: u8, +} + +impl<'a> Drop for Guard<'a> { + fn drop(&mut self) { + self.state.store(self.new_state, Ordering::Release); + } +} + // Note: this is intentionally monomorphic #[inline(never)] -fn initialize_inner( - mutex: &Mutex<()>, - is_initialized: &AtomicBool, - init: &mut dyn FnMut() -> bool, -) { - let _guard = mutex.lock(); - - if !is_initialized.load(Ordering::Acquire) { - if init() { - is_initialized.store(true, Ordering::Release); +fn initialize_inner(state: &AtomicU8, init: &mut dyn FnMut() -> bool) { + let key = state as *const _ as usize; + loop { + let exchange = + state.compare_exchange(INCOMPLETE, RUNNING, Ordering::Acquire, Ordering::Acquire); + match exchange { + Ok(_) => { + let mut guard = Guard { state, new_state: INCOMPLETE }; + if init() { + guard.new_state = COMPLETE; + } + unsafe { + parking_lot_core::unpark_all(key, parking_lot_core::DEFAULT_UNPARK_TOKEN); + } + return; + } + Err(COMPLETE) => return, + Err(RUNNING) => unsafe { + parking_lot_core::park( + key, + || state.load(Ordering::Relaxed) == RUNNING, + || (), + |_, _| (), + parking_lot_core::DEFAULT_PARK_TOKEN, + None, + ); + }, + Err(_) => debug_assert!(false), } } } @@ -141,5 +161,5 @@ fn initialize_inner( fn test_size() { use std::mem::size_of; - assert_eq!(size_of::>(), 2 * size_of::() + size_of::()); + assert_eq!(size_of::>(), 1 * size_of::() + size_of::()); } From 0067580881b0bedf860dd27f5e087c273198b0eb Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Thu, 19 May 2022 14:02:26 +0100 Subject: [PATCH 2/4] use exchange_weak --- src/imp_pl.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/imp_pl.rs b/src/imp_pl.rs index 684fd9b..9d56ae5 100644 --- a/src/imp_pl.rs +++ b/src/imp_pl.rs @@ -129,7 +129,7 @@ fn initialize_inner(state: &AtomicU8, init: &mut dyn FnMut() -> bool) { let key = state as *const _ as usize; loop { let exchange = - state.compare_exchange(INCOMPLETE, RUNNING, Ordering::Acquire, Ordering::Acquire); + state.compare_exchange_weak(INCOMPLETE, RUNNING, Ordering::Acquire, Ordering::Acquire); match exchange { Ok(_) => { let mut guard = Guard { state, new_state: INCOMPLETE }; From 5386606ec4a8f9dcfa1124f7a7b9c23a049529be Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Thu, 19 May 2022 16:15:10 +0100 Subject: [PATCH 3/4] fix deadlock Kudos to @ekleog for finding it! --- src/imp_pl.rs | 7 ++++--- tests/it.rs | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/src/imp_pl.rs b/src/imp_pl.rs index 9d56ae5..cd6f856 100644 --- a/src/imp_pl.rs +++ b/src/imp_pl.rs @@ -120,6 +120,10 @@ struct Guard<'a> { impl<'a> Drop for Guard<'a> { fn drop(&mut self) { self.state.store(self.new_state, Ordering::Release); + unsafe { + let key = self.state as *const _ as usize; + parking_lot_core::unpark_all(key, parking_lot_core::DEFAULT_UNPARK_TOKEN); + } } } @@ -136,9 +140,6 @@ fn initialize_inner(state: &AtomicU8, init: &mut dyn FnMut() -> bool) { if init() { guard.new_state = COMPLETE; } - unsafe { - parking_lot_core::unpark_all(key, parking_lot_core::DEFAULT_UNPARK_TOKEN); - } return; } Err(COMPLETE) => return, diff --git a/tests/it.rs b/tests/it.rs index 4d6efdc..18a8094 100644 --- a/tests/it.rs +++ b/tests/it.rs @@ -319,6 +319,28 @@ mod sync { assert_eq!(cell.get(), Some(&"hello".to_string())); } + #[test] + #[cfg_attr(miri, ignore)] // miri doesn't support Barrier + fn get_or_init_stress() { + use std::sync::Barrier; + let n_threads = 1_000; + let n_cells = 1_000; + let cells: Vec<_> = std::iter::repeat_with(|| (Barrier::new(n_threads), OnceCell::new())) + .take(n_cells) + .collect(); + scope(|s| { + for _ in 0..n_threads { + s.spawn(|_| { + for (i, (b, s)) in cells.iter().enumerate() { + b.wait(); + let j = s.get_or_init(|| i); + assert_eq!(*j, i); + } + }); + } + }).unwrap(); + } + #[test] fn from_impl() { assert_eq!(OnceCell::from("value").get(), Some(&"value")); From 39a47e32d3ce22c42392e9fedf9b0646255a74d4 Mon Sep 17 00:00:00 2001 From: Aleksey Kladov Date: Thu, 19 May 2022 16:22:40 +0100 Subject: [PATCH 4/4] release 1.11.0 --- CHANGELOG.md | 7 ++++++- Cargo.toml | 2 +- src/lib.rs | 5 +++++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 497fbbf..36cc79a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,9 +1,14 @@ # Changelog +## Unreleased + +- + ## 1.11 -- Add `OnceCell::with_value` to create initialized `OnceCell` at compile time. +- Add `OnceCell::with_value` to create initialized `OnceCell` in `const` context. - Improve `Clone` implementation for `OnceCell`. +- Rewrite `parking_lot` version on top of `parking_lot_core`, for even smaller cells! ## 1.10 diff --git a/Cargo.toml b/Cargo.toml index fadaef7..34a75c2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "once_cell" -version = "1.10.0" +version = "1.11.0" authors = ["Aleksey Kladov "] license = "MIT OR Apache-2.0" edition = "2018" diff --git a/src/lib.rs b/src/lib.rs index b344251..75d1c06 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -311,6 +311,10 @@ //! At the moment, `unsync` has an additional benefit that reentrant initialization causes a panic, which //! might be easier to debug than a deadlock. //! +//! **Does this crate support async?** +//! +//! No, but you can use [`async_once_cell`](https://crates.io/crates/async_once_cell) instead. +//! //! # Related crates //! //! * [double-checked-cell](https://github.com/niklasf/double-checked-cell) @@ -318,6 +322,7 @@ //! * [lazycell](https://crates.io/crates/lazycell) //! * [mitochondria](https://crates.io/crates/mitochondria) //! * [lazy_static](https://crates.io/crates/lazy_static) +//! * [async_once_cell](https://crates.io/crates/async_once_cell) //! //! Most of this crate's functionality is available in `std` in nightly Rust. //! See the [tracking issue](https://github.com/rust-lang/rust/issues/74465).