From 51e61f1029b96dd890f737d6b3546d7fa6e229ae Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Tue, 6 Sep 2022 08:52:11 +0200 Subject: [PATCH 01/14] Add `no_std` implementation based on `critical-section`. --- Cargo.toml | 10 +++- src/imp_cs.rs | 131 +++++++++++++++++++++++++++++++++++++++++++++++++ src/imp_pl.rs | 12 +---- src/imp_std.rs | 16 ++---- src/lib.rs | 74 ++++++++++++++++++---------- src/race.rs | 1 + 6 files changed, 194 insertions(+), 50 deletions(-) create mode 100644 src/imp_cs.rs diff --git a/Cargo.toml b/Cargo.toml index 004aa77..6cba648 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -32,15 +32,19 @@ parking_lot_core = { version = "0.9.3", optional = true, default_features = fals # and make sure you understand all the implications atomic-polyfill = { version = "1", optional = true } +# Uses `critical-section` to implement `once_cell::sync::OnceCell`. +critical-section = { version = "1.1", optional = true } + [dev-dependencies] lazy_static = "1.0.0" crossbeam-utils = "0.8.7" regex = "1.2.0" +critical-section = { version = "1.1", features = ["std"] } [features] default = ["std"] # Enables `once_cell::sync` module. -std = ["alloc"] +std = ["alloc", "sync"] # Enables `once_cell::race::OnceBox` type. alloc = ["race"] # Enables `once_cell::race` module. @@ -49,8 +53,12 @@ race = [] # At the moment, this feature is unused. unstable = [] +sync = [] + parking_lot = ["parking_lot_core"] +critical-section = ["dep:critical-section", "sync"] + [[example]] name = "bench" required-features = ["std"] diff --git a/src/imp_cs.rs b/src/imp_cs.rs new file mode 100644 index 0000000..53848f8 --- /dev/null +++ b/src/imp_cs.rs @@ -0,0 +1,131 @@ +#[cfg(feature = "atomic-polyfill")] +use atomic_polyfill as atomic; +#[cfg(not(feature = "atomic-polyfill"))] +use core::sync::atomic; + +use atomic::{AtomicU8, Ordering}; + +use core::panic::{RefUnwindSafe, UnwindSafe}; + +use crate::unsync::OnceCell as UnsyncOnceCell; + +pub(crate) struct OnceCell { + state: AtomicU8, + value: UnsyncOnceCell, +} + +const INCOMPLETE: u8 = 0; +const RUNNING: u8 = 1; +const COMPLETE: u8 = 2; + +// Why do we need `T: Send`? +// Thread A creates a `OnceCell` and shares it with +// scoped thread B, which fills the cell, which is +// then destroyed by A. That is, destructor observes +// a sent value. +unsafe impl Sync for OnceCell {} +unsafe impl Send for OnceCell {} + +impl RefUnwindSafe for OnceCell {} +impl UnwindSafe for OnceCell {} + +impl OnceCell { + pub(crate) const fn new() -> OnceCell { + OnceCell { state: AtomicU8::new(INCOMPLETE), value: UnsyncOnceCell::new() } + } + + pub(crate) const fn with_value(value: T) -> OnceCell { + OnceCell { state: AtomicU8::new(COMPLETE), value: UnsyncOnceCell::with_value(value) } + } + + #[inline] + pub(crate) fn is_initialized(&self) -> bool { + self.state.load(Ordering::Acquire) == COMPLETE + } + + #[cold] + pub(crate) fn initialize(&self, f: F) -> Result<(), E> + where + F: FnOnce() -> Result, + { + let mut f = Some(f); + let mut res: Result<(), E> = Ok(()); + let slot: &UnsyncOnceCell = &self.value; + initialize_inner(&self.state, &mut || { + let f = unsafe { crate::take_unchecked(&mut f) }; + match f() { + Ok(value) => { + unsafe { crate::unwrap_unchecked(slot.set(value).ok()) }; + true + } + Err(err) => { + res = Err(err); + false + } + } + }); + res + } + + #[cold] + pub(crate) fn wait(&self) { + while !self.is_initialized() { + core::hint::spin_loop(); + } + } + + /// Get the reference to the underlying value, without checking if the cell + /// is initialized. + /// + /// # Safety + /// + /// Caller must ensure that the cell is in initialized state, and that + /// the contents are acquired by (synchronized to) this thread. + pub(crate) unsafe fn get_unchecked(&self) -> &T { + debug_assert!(self.is_initialized()); + self.value.get_unchecked() + } + + #[inline] + pub(crate) fn get_mut(&mut self) -> Option<&mut T> { + self.value.get_mut() + } + + #[inline] + pub(crate) fn into_inner(self) -> Option { + self.value.into_inner() + } +} + +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); + } +} + +#[inline(never)] +fn initialize_inner(state: &AtomicU8, init: &mut dyn FnMut() -> bool) { + loop { + match state.compare_exchange_weak(INCOMPLETE, RUNNING, Ordering::Acquire, Ordering::Acquire) + { + Ok(_) => { + let mut guard = Guard { state, new_state: INCOMPLETE }; + if init() { + guard.new_state = COMPLETE; + } + return; + } + Err(COMPLETE) => return, + Err(RUNNING) | Err(INCOMPLETE) => core::hint::spin_loop(), + Err(_) => { + debug_assert!(false); + unsafe { core::hint::unreachable_unchecked() } + } + } + } +} diff --git a/src/imp_pl.rs b/src/imp_pl.rs index d80ca5e..b723587 100644 --- a/src/imp_pl.rs +++ b/src/imp_pl.rs @@ -1,6 +1,5 @@ use std::{ cell::UnsafeCell, - hint, panic::{RefUnwindSafe, UnwindSafe}, sync::atomic::{AtomicU8, Ordering}, }; @@ -101,15 +100,8 @@ impl OnceCell { /// the contents are acquired by (synchronized to) this thread. pub(crate) unsafe fn get_unchecked(&self) -> &T { debug_assert!(self.is_initialized()); - let slot: &Option = &*self.value.get(); - match slot { - Some(value) => value, - // This unsafe does improve performance, see `examples/bench`. - None => { - debug_assert!(false); - hint::unreachable_unchecked() - } - } + let slot = &*self.value.get(); + crate::unwrap_unchecked(slot.as_ref()) } /// Gets the mutable reference to the underlying value. diff --git a/src/imp_std.rs b/src/imp_std.rs index 4d5b5fd..f828d45 100644 --- a/src/imp_std.rs +++ b/src/imp_std.rs @@ -5,15 +5,12 @@ use std::{ cell::{Cell, UnsafeCell}, - hint::unreachable_unchecked, marker::PhantomData, panic::{RefUnwindSafe, UnwindSafe}, sync::atomic::{AtomicBool, AtomicPtr, Ordering}, thread::{self, Thread}, }; -use crate::take_unchecked; - #[derive(Debug)] pub(crate) struct OnceCell { // This `queue` field is the core of the implementation. It encodes two @@ -81,7 +78,7 @@ impl OnceCell { initialize_or_wait( &self.queue, Some(&mut || { - let f = unsafe { take_unchecked(&mut f) }; + let f = unsafe { crate::take_unchecked(&mut f) }; match f() { Ok(value) => { unsafe { *slot = Some(value) }; @@ -111,15 +108,8 @@ impl OnceCell { /// the contents are acquired by (synchronized to) this thread. pub(crate) unsafe fn get_unchecked(&self) -> &T { debug_assert!(self.is_initialized()); - let slot: &Option = &*self.value.get(); - match slot { - Some(value) => value, - // This unsafe does improve performance, see `examples/bench`. - None => { - debug_assert!(false); - unreachable_unchecked() - } - } + let slot = &*self.value.get(); + crate::unwrap_unchecked(slot.as_ref()) } /// Gets the mutable reference to the underlying value. diff --git a/src/lib.rs b/src/lib.rs index 6de1e3e..d084535 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -329,27 +329,38 @@ #![cfg_attr(not(feature = "std"), no_std)] +#[cfg(not(feature = "std"))] +use core as lib; +#[cfg(feature = "std")] +use std as lib; + +use lib::{ + cell::{Cell, UnsafeCell}, + fmt, mem, + ops::{Deref, DerefMut}, + panic::{RefUnwindSafe, UnwindSafe}, +}; + #[cfg(feature = "alloc")] extern crate alloc; -#[cfg(feature = "std")] +#[cfg(feature = "critical-section")] +#[path = "imp_cs.rs"] +mod imp; + +#[cfg(all(feature = "std", not(feature = "critical-section")))] #[cfg(feature = "parking_lot")] #[path = "imp_pl.rs"] mod imp; -#[cfg(feature = "std")] +#[cfg(all(feature = "std", not(feature = "critical-section")))] #[cfg(not(feature = "parking_lot"))] #[path = "imp_std.rs"] mod imp; /// Single-threaded version of `OnceCell`. pub mod unsync { - use core::{ - cell::{Cell, UnsafeCell}, - fmt, hint, mem, - ops::{Deref, DerefMut}, - panic::{RefUnwindSafe, UnwindSafe}, - }; + use super::*; /// A cell which can be written to only once. It is not thread safe. /// @@ -468,6 +479,17 @@ pub mod unsync { unsafe { &mut *self.inner.get() }.as_mut() } + /// Get the reference to the underlying value, without checking if the + /// cell is initialized. + /// + /// # Safety + /// + /// Caller must ensure that the cell is in initialized state. + #[cfg(feature = "critical-section")] + pub(crate) unsafe fn get_unchecked(&self) -> &T { + crate::unwrap_unchecked(self.get()) + } + /// Sets the contents of this cell to `value`. /// /// Returns `Ok(())` if the cell was empty and `Err(value)` if it was @@ -510,16 +532,14 @@ pub mod unsync { if let Some(old) = self.get() { return Err((old, value)); } + let slot = unsafe { &mut *self.inner.get() }; // This is the only place where we set the slot, no races // due to reentrancy/concurrency are possible, and we've // checked that slot is currently `None`, so this write // maintains the `inner`'s invariant. *slot = Some(value); - Ok(match &*slot { - Some(value) => value, - None => unsafe { hint::unreachable_unchecked() }, - }) + Ok(unsafe { unwrap_unchecked(slot.as_ref()) }) } /// Gets the contents of the cell, initializing it with `f` @@ -592,7 +612,7 @@ pub mod unsync { // `assert`, while keeping `set/get` would be sound, but it seems // better to panic, rather than to silently use an old value. assert!(self.set(val).is_ok(), "reentrant init"); - Ok(self.get().unwrap()) + Ok(unsafe { unwrap_unchecked(self.get()) }) } /// Takes the value out of this `OnceCell`, moving it back to an uninitialized state. @@ -814,16 +834,11 @@ pub mod unsync { } /// Thread-safe, blocking version of `OnceCell`. -#[cfg(feature = "std")] +#[cfg(feature = "sync")] pub mod sync { - use std::{ - cell::Cell, - fmt, mem, - ops::{Deref, DerefMut}, - panic::RefUnwindSafe, - }; + use super::*; - use crate::{imp::OnceCell as Imp, take_unchecked}; + use imp::OnceCell as Imp; /// A thread-safe cell which can be written to only once. /// @@ -942,7 +957,7 @@ pub mod sync { /// // Will return 92, but might block until the other thread does `.set`. /// let value: &u32 = cell.wait(); /// assert_eq!(*value, 92); - /// t.join().unwrap();; + /// t.join().unwrap(); /// ``` pub fn wait(&self) -> &T { if !self.0.is_initialized() { @@ -1109,6 +1124,7 @@ pub mod sync { if let Some(value) = self.get() { return Ok(value); } + self.0.initialize(f)?; // Safe b/c value is initialized. @@ -1356,13 +1372,19 @@ pub mod sync { #[cfg(feature = "race")] pub mod race; -#[cfg(feature = "std")] +#[cfg(feature = "sync")] +#[inline] unsafe fn take_unchecked(val: &mut Option) -> T { - match val.take() { - Some(it) => it, + unwrap_unchecked(val.take()) +} + +#[inline] +unsafe fn unwrap_unchecked(val: Option) -> T { + match val { + Some(value) => value, None => { debug_assert!(false); - std::hint::unreachable_unchecked() + core::hint::unreachable_unchecked() } } } diff --git a/src/race.rs b/src/race.rs index e83e0b9..28acf62 100644 --- a/src/race.rs +++ b/src/race.rs @@ -165,6 +165,7 @@ impl OnceBool { fn from_usize(value: NonZeroUsize) -> bool { value.get() == 1 } + #[inline] fn to_usize(value: bool) -> NonZeroUsize { unsafe { NonZeroUsize::new_unchecked(if value { 1 } else { 2 }) } From 94a7114b244a12e28429eb8fa347af7fa92f8cb0 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 7 Sep 2022 04:58:33 +0200 Subject: [PATCH 02/14] Change `critical-section` implementation to not use spin-lock. --- Cargo.toml | 3 ++ src/imp_cs.rs | 93 ++++++++++----------------------------------------- src/lib.rs | 1 + tests/it.rs | 42 +++++++++++++++++++---- 4 files changed, 56 insertions(+), 83 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 6cba648..40eadfe 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -89,3 +89,6 @@ required-features = ["std"] [package.metadata.docs.rs] all-features = true + +[patch.crates-io] +critical-section = { git = "https://github.com/reitermarkus/critical-section", branch = "lock-poisoning" } diff --git a/src/imp_cs.rs b/src/imp_cs.rs index 53848f8..56aadb0 100644 --- a/src/imp_cs.rs +++ b/src/imp_cs.rs @@ -1,23 +1,13 @@ -#[cfg(feature = "atomic-polyfill")] -use atomic_polyfill as atomic; -#[cfg(not(feature = "atomic-polyfill"))] -use core::sync::atomic; - -use atomic::{AtomicU8, Ordering}; - use core::panic::{RefUnwindSafe, UnwindSafe}; +use critical_section::{CriticalSection, Mutex}; + use crate::unsync::OnceCell as UnsyncOnceCell; pub(crate) struct OnceCell { - state: AtomicU8, - value: UnsyncOnceCell, + value: Mutex>, } -const INCOMPLETE: u8 = 0; -const RUNNING: u8 = 1; -const COMPLETE: u8 = 2; - // Why do we need `T: Send`? // Thread A creates a `OnceCell` and shares it with // scoped thread B, which fills the cell, which is @@ -31,16 +21,18 @@ impl UnwindSafe for OnceCell {} impl OnceCell { pub(crate) const fn new() -> OnceCell { - OnceCell { state: AtomicU8::new(INCOMPLETE), value: UnsyncOnceCell::new() } + OnceCell { value: Mutex::new(UnsyncOnceCell::new()) } } pub(crate) const fn with_value(value: T) -> OnceCell { - OnceCell { state: AtomicU8::new(COMPLETE), value: UnsyncOnceCell::with_value(value) } + OnceCell { value: Mutex::new(UnsyncOnceCell::with_value(value)) } } #[inline] pub(crate) fn is_initialized(&self) -> bool { - self.state.load(Ordering::Acquire) == COMPLETE + // The only write access is synchronized in `initialize` with a + // critical section, so read-only access is valid everywhere else. + unsafe { self.value.borrow(CriticalSection::new()).get().is_some() } } #[cold] @@ -48,30 +40,10 @@ impl OnceCell { where F: FnOnce() -> Result, { - let mut f = Some(f); - let mut res: Result<(), E> = Ok(()); - let slot: &UnsyncOnceCell = &self.value; - initialize_inner(&self.state, &mut || { - let f = unsafe { crate::take_unchecked(&mut f) }; - match f() { - Ok(value) => { - unsafe { crate::unwrap_unchecked(slot.set(value).ok()) }; - true - } - Err(err) => { - res = Err(err); - false - } - } - }); - res - } - - #[cold] - pub(crate) fn wait(&self) { - while !self.is_initialized() { - core::hint::spin_loop(); - } + critical_section::with(|cs| { + let cell = self.value.borrow(cs); + cell.get_or_try_init(f).map(|_| ()) + }) } /// Get the reference to the underlying value, without checking if the cell @@ -83,49 +55,18 @@ impl OnceCell { /// the contents are acquired by (synchronized to) this thread. pub(crate) unsafe fn get_unchecked(&self) -> &T { debug_assert!(self.is_initialized()); - self.value.get_unchecked() + // The only write access is synchronized in `initialize` with a + // critical section, so read-only access is valid everywhere else. + self.value.borrow(CriticalSection::new()).get_unchecked() } #[inline] pub(crate) fn get_mut(&mut self) -> Option<&mut T> { - self.value.get_mut() + self.value.get_mut().get_mut() } #[inline] pub(crate) fn into_inner(self) -> Option { - self.value.into_inner() - } -} - -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); - } -} - -#[inline(never)] -fn initialize_inner(state: &AtomicU8, init: &mut dyn FnMut() -> bool) { - loop { - match state.compare_exchange_weak(INCOMPLETE, RUNNING, Ordering::Acquire, Ordering::Acquire) - { - Ok(_) => { - let mut guard = Guard { state, new_state: INCOMPLETE }; - if init() { - guard.new_state = COMPLETE; - } - return; - } - Err(COMPLETE) => return, - Err(RUNNING) | Err(INCOMPLETE) => core::hint::spin_loop(), - Err(_) => { - debug_assert!(false); - unsafe { core::hint::unreachable_unchecked() } - } - } + self.value.into_inner().into_inner() } } diff --git a/src/lib.rs b/src/lib.rs index d084535..dcc6303 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -959,6 +959,7 @@ pub mod sync { /// assert_eq!(*value, 92); /// t.join().unwrap(); /// ``` + #[cfg(not(feature = "critical-section"))] pub fn wait(&self) -> &T { if !self.0.is_initialized() { self.0.wait() diff --git a/tests/it.rs b/tests/it.rs index 410b93b..5107e08 100644 --- a/tests/it.rs +++ b/tests/it.rs @@ -207,7 +207,7 @@ mod unsync { } #[test] - #[cfg(feature = "std")] + #[cfg(all(feature = "std", not(feature = "critical-section")))] fn lazy_poisoning() { let x: Lazy = Lazy::new(|| panic!("kaboom")); for _ in 0..2 { @@ -249,10 +249,13 @@ mod unsync { } } -#[cfg(feature = "std")] +#[cfg(feature = "sync")] mod sync { use std::sync::atomic::{AtomicUsize, Ordering::SeqCst}; + #[cfg(feature = "critical-section")] + use core::cell::Cell; + use crossbeam_utils::thread::scope; use once_cell::sync::{Lazy, OnceCell}; @@ -354,6 +357,7 @@ mod sync { assert_eq!(cell.get(), Some(&"hello".to_string())); } + #[cfg(not(feature = "critical-section"))] #[test] fn wait() { let cell: OnceCell = OnceCell::new(); @@ -365,6 +369,7 @@ mod sync { .unwrap(); } + #[cfg(not(feature = "critical-section"))] #[test] fn get_or_init_stress() { use std::sync::Barrier; @@ -430,6 +435,7 @@ mod sync { #[test] #[cfg_attr(miri, ignore)] // miri doesn't support processes + #[cfg(not(feature = "critical-section"))] fn reentrant_init() { let examples_dir = { let mut exe = std::env::current_exe().unwrap(); @@ -457,6 +463,20 @@ mod sync { } } + #[cfg(feature = "critical-section")] + #[test] + #[should_panic(expected = "reentrant init")] + fn reentrant_init() { + let x: OnceCell> = OnceCell::new(); + let dangling_ref: Cell> = Cell::new(None); + x.get_or_init(|| { + let r = x.get_or_init(|| Box::new(92)); + dangling_ref.set(Some(r)); + Box::new(62) + }); + eprintln!("use after free: {:?}", dangling_ref.get().unwrap()); + } + #[test] fn lazy_new() { let called = AtomicUsize::new(0); @@ -610,6 +630,7 @@ mod sync { assert_eq!(fib[5], 8) } + #[cfg(not(feature = "critical-section"))] #[test] fn once_cell_does_not_leak_partially_constructed_boxes() { let n_tries = if cfg!(miri) { 10 } else { 100 }; @@ -636,6 +657,7 @@ mod sync { } } + #[cfg(not(feature = "critical-section"))] #[test] fn get_does_not_block() { use std::sync::Barrier; @@ -671,12 +693,11 @@ mod sync { #[cfg(feature = "race")] mod race { + #[cfg(not(feature = "critical-section"))] + use std::sync::Barrier; use std::{ num::NonZeroUsize, - sync::{ - atomic::{AtomicUsize, Ordering::SeqCst}, - Barrier, - }, + sync::atomic::{AtomicUsize, Ordering::SeqCst}, }; use crossbeam_utils::thread::scope; @@ -728,6 +749,7 @@ mod race { assert_eq!(cell.get(), Some(val1)); } + #[cfg(not(feature = "critical-section"))] #[test] fn once_non_zero_usize_first_wins() { let val1 = NonZeroUsize::new(92).unwrap(); @@ -807,12 +829,16 @@ mod race { #[cfg(all(feature = "race", feature = "alloc"))] mod race_once_box { + #[cfg(not(feature = "critical-section"))] + use std::sync::Barrier; use std::sync::{ atomic::{AtomicUsize, Ordering::SeqCst}, - Arc, Barrier, + Arc, }; + #[cfg(not(feature = "critical-section"))] use crossbeam_utils::thread::scope; + use once_cell::race::OnceBox; #[derive(Default)] @@ -842,6 +868,7 @@ mod race_once_box { } } + #[cfg(not(feature = "critical-section"))] #[test] fn once_box_smoke_test() { let heap = Heap::default(); @@ -896,6 +923,7 @@ mod race_once_box { assert_eq!(heap.total(), 0); } + #[cfg(not(feature = "critical-section"))] #[test] fn once_box_first_wins() { let cell = OnceBox::new(); From fb0fe8c5e49066533c84854eb1f418089c182d33 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 7 Sep 2022 05:23:40 +0200 Subject: [PATCH 03/14] Fix feature. --- Cargo.toml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 40eadfe..db39317 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,13 +33,13 @@ parking_lot_core = { version = "0.9.3", optional = true, default_features = fals atomic-polyfill = { version = "1", optional = true } # Uses `critical-section` to implement `once_cell::sync::OnceCell`. -critical-section = { version = "1.1", optional = true } +critical_section = { package = "critical-section", version = "1.1", optional = true } [dev-dependencies] lazy_static = "1.0.0" crossbeam-utils = "0.8.7" regex = "1.2.0" -critical-section = { version = "1.1", features = ["std"] } +critical_section = { package = "critical-section", version = "1.1", features = ["std"] } [features] default = ["std"] @@ -57,7 +57,7 @@ sync = [] parking_lot = ["parking_lot_core"] -critical-section = ["dep:critical-section", "sync"] +critical-section = ["critical_section", "sync"] [[example]] name = "bench" From 16a53bc29b53432828e0691bf0a219ce9455a9da Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 7 Sep 2022 06:09:41 +0200 Subject: [PATCH 04/14] Reenable some tests. --- tests/it.rs | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/it.rs b/tests/it.rs index 5107e08..118be00 100644 --- a/tests/it.rs +++ b/tests/it.rs @@ -207,7 +207,7 @@ mod unsync { } #[test] - #[cfg(all(feature = "std", not(feature = "critical-section")))] + #[cfg(feature = "std")] fn lazy_poisoning() { let x: Lazy = Lazy::new(|| panic!("kaboom")); for _ in 0..2 { @@ -253,6 +253,9 @@ mod unsync { mod sync { use std::sync::atomic::{AtomicUsize, Ordering::SeqCst}; + #[cfg(not(feature = "critical-section"))] + use std::sync::Barrier; + #[cfg(feature = "critical-section")] use core::cell::Cell; @@ -372,7 +375,6 @@ mod sync { #[cfg(not(feature = "critical-section"))] #[test] fn get_or_init_stress() { - use std::sync::Barrier; let n_threads = if cfg!(miri) { 30 } else { 1_000 }; let n_cells = if cfg!(miri) { 30 } else { 1_000 }; let cells: Vec<_> = std::iter::repeat_with(|| (Barrier::new(n_threads), OnceCell::new())) @@ -630,7 +632,6 @@ mod sync { assert_eq!(fib[5], 8) } - #[cfg(not(feature = "critical-section"))] #[test] fn once_cell_does_not_leak_partially_constructed_boxes() { let n_tries = if cfg!(miri) { 10 } else { 100 }; @@ -660,8 +661,6 @@ mod sync { #[cfg(not(feature = "critical-section"))] #[test] fn get_does_not_block() { - use std::sync::Barrier; - let cell = OnceCell::new(); let barrier = Barrier::new(2); scope(|scope| { From 0496e9d29302c48b7de3c01c1614b083ac9304b8 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 7 Sep 2022 06:50:24 +0200 Subject: [PATCH 05/14] Fix `is_initialized` and use atomics if available. --- src/imp_cs.rs | 36 ++++++++++++++++++++++++++++-------- src/lib.rs | 6 ++++++ 2 files changed, 34 insertions(+), 8 deletions(-) diff --git a/src/imp_cs.rs b/src/imp_cs.rs index 56aadb0..a1abede 100644 --- a/src/imp_cs.rs +++ b/src/imp_cs.rs @@ -1,10 +1,14 @@ use core::panic::{RefUnwindSafe, UnwindSafe}; +#[cfg(feature = "atomic-polyfill")] +use atomic_polyfill::{AtomicBool, Ordering}; use critical_section::{CriticalSection, Mutex}; use crate::unsync::OnceCell as UnsyncOnceCell; pub(crate) struct OnceCell { + #[cfg(feature = "atomic-polyfill")] + initialized: AtomicBool, value: Mutex>, } @@ -21,18 +25,30 @@ impl UnwindSafe for OnceCell {} impl OnceCell { pub(crate) const fn new() -> OnceCell { - OnceCell { value: Mutex::new(UnsyncOnceCell::new()) } + OnceCell { + #[cfg(feature = "atomic-polyfill")] + initialized: AtomicBool::new(false), + value: Mutex::new(UnsyncOnceCell::new()), + } } pub(crate) const fn with_value(value: T) -> OnceCell { - OnceCell { value: Mutex::new(UnsyncOnceCell::with_value(value)) } + OnceCell { + #[cfg(feature = "atomic-polyfill")] + initialized: AtomicBool::new(true), + value: Mutex::new(UnsyncOnceCell::with_value(value)), + } } #[inline] pub(crate) fn is_initialized(&self) -> bool { - // The only write access is synchronized in `initialize` with a - // critical section, so read-only access is valid everywhere else. - unsafe { self.value.borrow(CriticalSection::new()).get().is_some() } + #[cfg(feature = "atomic-polyfill")] + { + self.initialized.load(Ordering::Acquire) + } + + #[cfg(not(feature = "atomic-polyfill"))] + critical_section::with(|cs| self.value.borrow(cs).get().is_some()) } #[cold] @@ -42,7 +58,12 @@ impl OnceCell { { critical_section::with(|cs| { let cell = self.value.borrow(cs); - cell.get_or_try_init(f).map(|_| ()) + cell.get_or_try_init(f).map(|_| { + #[cfg(feature = "atomic-polyfill")] + self.initialized.store(true, Ordering::Release); + + () + }) }) } @@ -55,8 +76,7 @@ impl OnceCell { /// the contents are acquired by (synchronized to) this thread. pub(crate) unsafe fn get_unchecked(&self) -> &T { debug_assert!(self.is_initialized()); - // The only write access is synchronized in `initialize` with a - // critical section, so read-only access is valid everywhere else. + // SAFETY: The caller ensures that the value is initialized and access synchronized. self.value.borrow(CriticalSection::new()).get_unchecked() } diff --git a/src/lib.rs b/src/lib.rs index dcc6303..8016e57 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -453,6 +453,7 @@ pub mod unsync { /// Gets a reference to the underlying value. /// /// Returns `None` if the cell is empty. + #[inline] pub fn get(&self) -> Option<&T> { // Safe due to `inner`'s invariant unsafe { &*self.inner.get() }.as_ref() @@ -474,6 +475,7 @@ pub mod unsync { /// *cell.get_mut().unwrap() = 93; /// assert_eq!(cell.get(), Some(&93)); /// ``` + #[inline] pub fn get_mut(&mut self) -> Option<&mut T> { // Safe because we have unique access unsafe { &mut *self.inner.get() }.as_mut() @@ -486,6 +488,7 @@ pub mod unsync { /// /// Caller must ensure that the cell is in initialized state. #[cfg(feature = "critical-section")] + #[inline] pub(crate) unsafe fn get_unchecked(&self) -> &T { crate::unwrap_unchecked(self.get()) } @@ -985,6 +988,7 @@ pub mod sync { /// cell.set(92).unwrap(); /// cell = OnceCell::new(); /// ``` + #[inline] pub fn get_mut(&mut self) -> Option<&mut T> { self.0.get_mut() } @@ -996,6 +1000,7 @@ pub mod sync { /// /// Caller must ensure that the cell is in initialized state, and that /// the contents are acquired by (synchronized to) this thread. + #[inline] pub unsafe fn get_unchecked(&self) -> &T { self.0.get_unchecked() } @@ -1181,6 +1186,7 @@ pub mod sync { /// cell.set("hello".to_string()).unwrap(); /// assert_eq!(cell.into_inner(), Some("hello".to_string())); /// ``` + #[inline] pub fn into_inner(self) -> Option { self.0.into_inner() } From cc0485ff57069b23d53d4e1ed88694f94a694578 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Wed, 7 Sep 2022 07:24:16 +0200 Subject: [PATCH 06/14] Always use `atomic-polyfill` with `critical-section` implementation. --- Cargo.toml | 2 +- src/imp_cs.rs | 18 ++---------------- 2 files changed, 3 insertions(+), 17 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index db39317..9ff96df 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -57,7 +57,7 @@ sync = [] parking_lot = ["parking_lot_core"] -critical-section = ["critical_section", "sync"] +critical-section = ["critical_section", "atomic-polyfill", "sync"] [[example]] name = "bench" diff --git a/src/imp_cs.rs b/src/imp_cs.rs index a1abede..9cc98ec 100644 --- a/src/imp_cs.rs +++ b/src/imp_cs.rs @@ -1,13 +1,11 @@ use core::panic::{RefUnwindSafe, UnwindSafe}; -#[cfg(feature = "atomic-polyfill")] use atomic_polyfill::{AtomicBool, Ordering}; use critical_section::{CriticalSection, Mutex}; use crate::unsync::OnceCell as UnsyncOnceCell; pub(crate) struct OnceCell { - #[cfg(feature = "atomic-polyfill")] initialized: AtomicBool, value: Mutex>, } @@ -25,16 +23,11 @@ impl UnwindSafe for OnceCell {} impl OnceCell { pub(crate) const fn new() -> OnceCell { - OnceCell { - #[cfg(feature = "atomic-polyfill")] - initialized: AtomicBool::new(false), - value: Mutex::new(UnsyncOnceCell::new()), - } + OnceCell { initialized: AtomicBool::new(false), value: Mutex::new(UnsyncOnceCell::new()) } } pub(crate) const fn with_value(value: T) -> OnceCell { OnceCell { - #[cfg(feature = "atomic-polyfill")] initialized: AtomicBool::new(true), value: Mutex::new(UnsyncOnceCell::with_value(value)), } @@ -42,13 +35,7 @@ impl OnceCell { #[inline] pub(crate) fn is_initialized(&self) -> bool { - #[cfg(feature = "atomic-polyfill")] - { - self.initialized.load(Ordering::Acquire) - } - - #[cfg(not(feature = "atomic-polyfill"))] - critical_section::with(|cs| self.value.borrow(cs).get().is_some()) + self.initialized.load(Ordering::Acquire) } #[cold] @@ -59,7 +46,6 @@ impl OnceCell { critical_section::with(|cs| { let cell = self.value.borrow(cs); cell.get_or_try_init(f).map(|_| { - #[cfg(feature = "atomic-polyfill")] self.initialized.store(true, Ordering::Release); () From 1194a690ceb1b3e73979a5ac4e940c387dc6653a Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sat, 17 Sep 2022 05:24:46 +0200 Subject: [PATCH 07/14] Add CI task for `critical-section` feature. --- xtask/src/main.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/xtask/src/main.rs b/xtask/src/main.rs index 654efa9..aaae075 100644 --- a/xtask/src/main.rs +++ b/xtask/src/main.rs @@ -32,6 +32,9 @@ fn main() -> xshell::Result<()> { // Skip doctests for no_std tests as those don't work cmd!(sh, "cargo test --no-default-features --features unstable --test it").run()?; cmd!(sh, "cargo test --no-default-features --features unstable,alloc --test it").run()?; + + cmd!(sh, "cargo test --no-default-features --features critical-section").run()?; + cmd!(sh, "cargo test --no-default-features --features critical-section --release").run()?; } { From 56162365f7b963975e4a649ad28f585ae59e1589 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sat, 17 Sep 2022 04:52:52 +0200 Subject: [PATCH 08/14] Remove `critical-section` patch. --- Cargo.toml | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 9ff96df..94e7aa2 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -39,7 +39,7 @@ critical_section = { package = "critical-section", version = "1.1", optional = t lazy_static = "1.0.0" crossbeam-utils = "0.8.7" regex = "1.2.0" -critical_section = { package = "critical-section", version = "1.1", features = ["std"] } +critical_section = { package = "critical-section", version = "1.1.1", features = ["std"] } [features] default = ["std"] @@ -89,6 +89,3 @@ required-features = ["std"] [package.metadata.docs.rs] all-features = true - -[patch.crates-io] -critical-section = { git = "https://github.com/reitermarkus/critical-section", branch = "lock-poisoning" } From fc9fe1876124c34e4a28e33b2789d3aaadd8ae7c Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sat, 17 Sep 2022 04:53:11 +0200 Subject: [PATCH 09/14] Remove `*` imports. --- src/lib.rs | 27 ++++++++++++++++----------- 1 file changed, 16 insertions(+), 11 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 8016e57..063557a 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -334,13 +334,6 @@ use core as lib; #[cfg(feature = "std")] use std as lib; -use lib::{ - cell::{Cell, UnsafeCell}, - fmt, mem, - ops::{Deref, DerefMut}, - panic::{RefUnwindSafe, UnwindSafe}, -}; - #[cfg(feature = "alloc")] extern crate alloc; @@ -360,7 +353,13 @@ mod imp; /// Single-threaded version of `OnceCell`. pub mod unsync { - use super::*; + use super::lib::{ + cell::{Cell, UnsafeCell}, + fmt, mem, + ops::{Deref, DerefMut}, + panic::{RefUnwindSafe, UnwindSafe}, + }; + use super::unwrap_unchecked; /// A cell which can be written to only once. It is not thread safe. /// @@ -839,9 +838,15 @@ pub mod unsync { /// Thread-safe, blocking version of `OnceCell`. #[cfg(feature = "sync")] pub mod sync { - use super::*; - - use imp::OnceCell as Imp; + use super::lib::{ + cell::Cell, + fmt, mem, + ops::{Deref, DerefMut}, + panic::RefUnwindSafe, + }; + + use super::imp::OnceCell as Imp; + use super::take_unchecked; /// A thread-safe cell which can be written to only once. /// From 223c247702d45ba7186eebc651740e4622a5aabf Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sat, 17 Sep 2022 04:53:36 +0200 Subject: [PATCH 10/14] Use `std` implementation when both `std` and `critical-section` features are enabled. --- src/lib.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 063557a..426c04c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -337,17 +337,15 @@ use std as lib; #[cfg(feature = "alloc")] extern crate alloc; -#[cfg(feature = "critical-section")] +#[cfg(all(feature = "critical-section", not(feature = "std")))] #[path = "imp_cs.rs"] mod imp; -#[cfg(all(feature = "std", not(feature = "critical-section")))] -#[cfg(feature = "parking_lot")] +#[cfg(all(feature = "std", feature = "parking_lot", not(feature = "critical-section")))] #[path = "imp_pl.rs"] mod imp; -#[cfg(all(feature = "std", not(feature = "critical-section")))] -#[cfg(not(feature = "parking_lot"))] +#[cfg(all(feature = "std", not(feature = "parking_lot"), not(feature = "critical-section")))] #[path = "imp_std.rs"] mod imp; From 3f69187068265873eaaca6d07003657c0dc6faa6 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sat, 17 Sep 2022 05:06:59 +0200 Subject: [PATCH 11/14] Clean up code. --- src/imp_cs.rs | 2 -- src/lib.rs | 13 ++++--------- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/imp_cs.rs b/src/imp_cs.rs index 9cc98ec..985ea6f 100644 --- a/src/imp_cs.rs +++ b/src/imp_cs.rs @@ -47,8 +47,6 @@ impl OnceCell { let cell = self.value.borrow(cs); cell.get_or_try_init(f).map(|_| { self.initialized.store(true, Ordering::Release); - - () }) }) } diff --git a/src/lib.rs b/src/lib.rs index 426c04c..1019fa4 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -329,11 +329,6 @@ #![cfg_attr(not(feature = "std"), no_std)] -#[cfg(not(feature = "std"))] -use core as lib; -#[cfg(feature = "std")] -use std as lib; - #[cfg(feature = "alloc")] extern crate alloc; @@ -351,12 +346,13 @@ mod imp; /// Single-threaded version of `OnceCell`. pub mod unsync { - use super::lib::{ + use core::{ cell::{Cell, UnsafeCell}, fmt, mem, ops::{Deref, DerefMut}, panic::{RefUnwindSafe, UnwindSafe}, }; + use super::unwrap_unchecked; /// A cell which can be written to only once. It is not thread safe. @@ -836,15 +832,14 @@ pub mod unsync { /// Thread-safe, blocking version of `OnceCell`. #[cfg(feature = "sync")] pub mod sync { - use super::lib::{ + use core::{ cell::Cell, fmt, mem, ops::{Deref, DerefMut}, panic::RefUnwindSafe, }; - use super::imp::OnceCell as Imp; - use super::take_unchecked; + use super::{imp::OnceCell as Imp, take_unchecked}; /// A thread-safe cell which can be written to only once. /// From 23d129038fddb4ec2336ec5879f6482cafe4bd5c Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sat, 17 Sep 2022 05:07:43 +0200 Subject: [PATCH 12/14] Decrease `critical-section` version. --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index 94e7aa2..1ad618f 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -33,7 +33,7 @@ parking_lot_core = { version = "0.9.3", optional = true, default_features = fals atomic-polyfill = { version = "1", optional = true } # Uses `critical-section` to implement `once_cell::sync::OnceCell`. -critical_section = { package = "critical-section", version = "1.1", optional = true } +critical_section = { package = "critical-section", version = "1", optional = true } [dev-dependencies] lazy_static = "1.0.0" From 32dada4dbe48ea45c63749b45a476a1d46af75a2 Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sat, 17 Sep 2022 05:29:27 +0200 Subject: [PATCH 13/14] Fix features. --- src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 1019fa4..f44bc4c 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -336,11 +336,11 @@ extern crate alloc; #[path = "imp_cs.rs"] mod imp; -#[cfg(all(feature = "std", feature = "parking_lot", not(feature = "critical-section")))] +#[cfg(all(feature = "std", feature = "parking_lot"))] #[path = "imp_pl.rs"] mod imp; -#[cfg(all(feature = "std", not(feature = "parking_lot"), not(feature = "critical-section")))] +#[cfg(all(feature = "std", not(feature = "parking_lot")))] #[path = "imp_std.rs"] mod imp; From 7d9afdb9d794b549b87a6d7c6611e3a28ec1588a Mon Sep 17 00:00:00 2001 From: Markus Reiter Date: Sat, 17 Sep 2022 05:32:52 +0200 Subject: [PATCH 14/14] Add comment explaining `Mutex`. --- src/imp_cs.rs | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/imp_cs.rs b/src/imp_cs.rs index 985ea6f..2548f58 100644 --- a/src/imp_cs.rs +++ b/src/imp_cs.rs @@ -3,11 +3,13 @@ use core::panic::{RefUnwindSafe, UnwindSafe}; use atomic_polyfill::{AtomicBool, Ordering}; use critical_section::{CriticalSection, Mutex}; -use crate::unsync::OnceCell as UnsyncOnceCell; +use crate::unsync; pub(crate) struct OnceCell { initialized: AtomicBool, - value: Mutex>, + // Use `unsync::OnceCell` internally since `Mutex` does not provide + // interior mutability and to be able to re-use `get_or_try_init`. + value: Mutex>, } // Why do we need `T: Send`? @@ -23,13 +25,13 @@ impl UnwindSafe for OnceCell {} impl OnceCell { pub(crate) const fn new() -> OnceCell { - OnceCell { initialized: AtomicBool::new(false), value: Mutex::new(UnsyncOnceCell::new()) } + OnceCell { initialized: AtomicBool::new(false), value: Mutex::new(unsync::OnceCell::new()) } } pub(crate) const fn with_value(value: T) -> OnceCell { OnceCell { initialized: AtomicBool::new(true), - value: Mutex::new(UnsyncOnceCell::with_value(value)), + value: Mutex::new(unsync::OnceCell::with_value(value)), } }