diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8e34b8557..733956793 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -32,6 +32,16 @@ jobs: - run: cargo test --doc --features test if: matrix.rust == 'stable' + loom: + runs-on: ubuntu-latest + needs: features + steps: + - uses: actions/checkout@v4 + - uses: dtolnay/rust-toolchain@stable + - run: cargo test -p musli --release -- tests::loom + env: + RUSTFLAGS: --cfg loom + unexpected_lints: runs-on: ubuntu-latest steps: @@ -40,7 +50,7 @@ jobs: - run: sed -i '/#!\[allow(unexpected_cfgs)\]/d' crates/*/src/lib.rs tests/src/lib.rs - run: cargo check --all-features --all-targets env: - RUSTFLAGS: -D warnings --check-cfg=cfg(rune_nightly,doc_cfg) -F unexpected_cfgs + RUSTFLAGS: -D warnings --check-cfg=cfg(loom,rune_nightly,doc_cfg) -F unexpected_cfgs tests_clippy: runs-on: ubuntu-latest @@ -64,7 +74,7 @@ jobs: - run: cargo +nightly run -p no-std --example ${{matrix.example}} fuzz: - needs: [test, tests_clippy, features, recursive] + needs: [test, loom, tests_clippy, features, recursive] runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 diff --git a/crates/musli-zerocopy/src/pointer/ref.rs b/crates/musli-zerocopy/src/pointer/ref.rs index 75df5d929..b184862c5 100644 --- a/crates/musli-zerocopy/src/pointer/ref.rs +++ b/crates/musli-zerocopy/src/pointer/ref.rs @@ -758,9 +758,9 @@ where } #[cfg(test)] - pub(crate) fn cast(self) -> Ref + pub(crate) fn cast(self) -> Ref where - U: Pointee, + U: ?Sized + Pointee, { Ref { offset: self.offset, diff --git a/crates/musli/Cargo.toml b/crates/musli/Cargo.toml index 73d8c6992..b600465b9 100644 --- a/crates/musli/Cargo.toml +++ b/crates/musli/Cargo.toml @@ -42,6 +42,9 @@ itoa = { version = "1.0.10", optional = true } ryu = { version = "1.0.17", optional = true } serde = { version = "1.0.198", optional = true, default-features = false} +[target.'cfg(loom)'.dependencies] +loom = "0.7.2" + [dev-dependencies] musli = { path = ".", features = ["test"] } tests = { path = "../../tests" } diff --git a/crates/musli/src/allocator/mod.rs b/crates/musli/src/allocator/mod.rs index 6f031778e..63732a10c 100644 --- a/crates/musli/src/allocator/mod.rs +++ b/crates/musli/src/allocator/mod.rs @@ -50,6 +50,10 @@ mod system; #[cfg_attr(doc_cfg, doc(cfg(feature = "alloc")))] pub use self::system::System; +/// The static system allocator instance. +#[cfg(all(feature = "alloc", not(loom)))] +pub static SYSTEM: System = System::new(); + mod disabled; pub use self::disabled::Disabled; @@ -109,12 +113,23 @@ macro_rules! __default { #[doc(inline)] pub use __default as default; -#[cfg(feature = "alloc")] +#[cfg(all(feature = "alloc", not(loom)))] +#[macro_export] +#[doc(hidden)] +macro_rules! __default_allocator_impl { + (|$alloc:ident| $body:block) => {{ + let $alloc = &$crate::allocator::SYSTEM; + $body + }}; +} + +#[cfg(all(feature = "alloc", loom))] #[macro_export] #[doc(hidden)] macro_rules! __default_allocator_impl { (|$alloc:ident| $body:block) => {{ let $alloc = $crate::allocator::System::new(); + let $alloc = &$alloc; $body }}; } diff --git a/crates/musli/src/allocator/system.rs b/crates/musli/src/allocator/system.rs index 4ab3b7c65..993780cbc 100644 --- a/crates/musli/src/allocator/system.rs +++ b/crates/musli/src/allocator/system.rs @@ -1,4 +1,3 @@ -use core::cell::UnsafeCell; use core::fmt::{self, Arguments}; use core::ptr::NonNull; @@ -6,27 +5,133 @@ use alloc::boxed::Box; use alloc::vec::Vec; use crate::buf::Error; +use crate::loom::cell::UnsafeCell; +use crate::loom::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use crate::loom::sync::with_mut_usize; use crate::{Allocator, Buf}; +/// The initial capacity of an allocated region. +#[cfg(not(loom))] +const INITIAL_CAPACITY: usize = 128; +#[cfg(loom)] +const INITIAL_CAPACITY: usize = 4; +/// The max capacity of an allocated region as it's being handed back. +#[cfg(not(loom))] +const MAX_CAPACITY: usize = 4096; +#[cfg(loom)] +const MAX_CAPACITY: usize = 12; +/// The maximum number of regions we hold onto to avoid leaking too much memory. +#[cfg(not(loom))] +const MAX_REGIONS: usize = 64; +#[cfg(loom)] +const MAX_REGIONS: usize = 2; + /// System buffer that can be used in combination with an [`Allocator`]. /// /// This uses the [`System`] allocator. /// /// [`System` allocator]: https://doc.rust-lang.org/std/alloc/struct.System.html +/// +/// # Examples +/// +/// ``` +/// use musli::allocator::System; +/// use musli::{Allocator, Buf}; +/// +/// let allocator = System::new(); +/// +/// let mut buf1 = allocator.alloc().expect("allocation failed"); +/// let mut buf2 = allocator.alloc().expect("allocation failed"); +/// +/// assert!(buf1.write(b"Hello, ")); +/// assert!(buf2.write(b"world!")); +/// +/// assert_eq!(buf1.as_slice(), b"Hello, "); +/// assert_eq!(buf2.as_slice(), b"world!"); +/// +/// buf1.write_buffer(buf2); +/// assert_eq!(buf1.as_slice(), b"Hello, world!"); +/// ``` pub struct System { - internal: UnsafeCell, + root: Root, } impl System { /// Construct a new allocator. #[inline] + #[cfg(not(loom))] pub const fn new() -> Self { Self { - internal: UnsafeCell::new(Internal { head: None }), + root: Root { + locked: AtomicBool::new(false), + head: UnsafeCell::new(None), + regions: AtomicUsize::new(0), + }, + } + } + + /// Construct a new allocator. + #[cfg(loom)] + pub fn new() -> Self { + Self { + root: Root { + locked: AtomicBool::new(false), + head: UnsafeCell::new(None), + regions: AtomicUsize::new(0), + }, } } } +impl Root { + fn spin(&self) { + while self.locked.load(Ordering::Relaxed) { + crate::loom::spin_loop(); + } + } + + fn lock(&self) -> Guard<'_> { + loop { + self.spin(); + + match self + .locked + .compare_exchange(false, true, Ordering::Acquire, Ordering::Relaxed) + { + Ok(_) => break, + Err(_) => continue, + } + } + + Guard { root: self } + } + + fn unlock(&self) { + self.locked.store(false, Ordering::SeqCst); + } +} + +struct Guard<'a> { + root: &'a Root, +} + +impl Guard<'_> { + #[inline] + fn with_mut<'this, O>( + &'this mut self, + f: impl FnOnce(&'this mut Option>) -> O, + ) -> O { + // SAFETY: We have access to the inner root under a lock guard. + self.root.head.with_mut(|inner| f(unsafe { &mut *inner })) + } +} + +impl Drop for Guard<'_> { + fn drop(&mut self) { + self.root.unlock(); + } +} + impl Default for System { #[inline] fn default() -> Self { @@ -39,32 +144,35 @@ impl Allocator for System { #[inline(always)] fn alloc(&self) -> Option> { + let region = self.root.alloc(); + Some(SystemBuf { - region: Internal::alloc(&self.internal), - internal: &self.internal, + root: &self.root, + region, }) } } impl Drop for System { fn drop(&mut self) { - let internal = unsafe { &mut *self.internal.get() }; + let mut current = self.root.lock().with_mut(|current| current.take()); - while let Some(mut head) = internal.head.take() { - // SAFETY: This collection has exclusive access to any heads it - // contain. - unsafe { - internal.head = head.as_mut().next.take(); - drop(Box::from_raw(head.as_ptr())); - } + with_mut_usize(&mut self.root.regions, |v| *v = 0); + + while let Some(this) = current.take() { + // SAFETY: While the system allocator is being dropped we hold a + // mutable reference to it, which ensures exclusive access to all + // allocated regions. + let b = unsafe { Box::from_raw(this.as_ptr()) }; + current = b.next; } } } /// A vector-backed allocation. pub struct SystemBuf<'a> { + root: &'a Root, region: &'a mut Region, - internal: &'a UnsafeCell, } impl<'a> Buf for SystemBuf<'a> { @@ -100,12 +208,11 @@ impl fmt::Write for SystemBuf<'_> { impl<'a> Drop for SystemBuf<'a> { fn drop(&mut self) { - Internal::free(self.internal, self.region); + self.root.free(self.region); } } /// An allocated region. -#[repr(C)] struct Region { data: Vec, // Pointer to the next free region. @@ -113,42 +220,69 @@ struct Region { } /// Internals of the allocator. -struct Internal { - // Regions of re-usable allocations we can hand out. - head: Option>, +struct Root { + /// Spin lock used for root. + locked: AtomicBool, + /// Regions of re-usable allocations we can hand out. + head: UnsafeCell>>, + /// The number of allocated regions. + regions: AtomicUsize, } -impl Internal { +unsafe impl Send for Root {} +unsafe impl Sync for Root {} + +impl Root { /// Allocate a new region. /// /// Note that this will return a leaked memory region, so the unbound /// lifetime is intentional. - fn alloc<'a>(this: &UnsafeCell) -> &'a mut Region { - // SAFETY: We take care to only access internals in a single-threaded - // mutable fashion. - let internal = unsafe { &mut *this.get() }; + /// + /// Clippy note: We know that we are correctly returning mutable references + /// to different mutable regions. + #[allow(clippy::mut_from_ref)] + fn alloc(&self) -> &mut Region { + self.regions.fetch_add(1, Ordering::SeqCst); - if let Some(mut head) = internal.head.take() { - // SAFETY: This collection has exclusive access to any heads it contain. - unsafe { - let head = head.as_mut(); - internal.head = head.next.take(); - head - } - } else { - Box::leak(Box::new(Region { - data: Vec::new(), - next: None, - })) + // SAFETY: We have exclusive access to all regions. + let this = self.lock().with_mut(|current| unsafe { + let mut this = current.take()?; + let this = this.as_mut(); + *current = this.next.take(); + Some(this) + }); + + if let Some(this) = this { + return this; } + + Box::leak(Box::new(Region { + data: Vec::with_capacity(INITIAL_CAPACITY), + next: None, + })) } - fn free(this: &UnsafeCell, region: &mut Region) { - unsafe { - let this = &mut *this.get(); - region.data.clear(); - region.next = this.head; - this.head = Some(NonNull::from(region)); + fn free<'a>(&'a self, region: &'a mut Region) { + let regions = self.regions.fetch_sub(1, Ordering::SeqCst); + + if regions >= MAX_REGIONS { + // SAFETY: We have exclusive access to the region, which we + // previously leaked. + unsafe { + drop(Box::from_raw(region)); + } + + return; } + + region.data.clear(); + region.data.shrink_to(MAX_CAPACITY); + + let mut current = self.lock(); + + current.with_mut(|current| { + region.next = current.take(); + *current = Some(NonNull::from(region)); + }); } } diff --git a/crates/musli/src/context/system_context.rs b/crates/musli/src/context/system_context.rs index 637034154..9eeeb2a53 100644 --- a/crates/musli/src/context/system_context.rs +++ b/crates/musli/src/context/system_context.rs @@ -6,6 +6,8 @@ use core::ops::Range; use alloc::string::{String, ToString}; use alloc::vec::Vec; +#[cfg(not(loom))] +use crate::allocator::System; use crate::buf::{self, BufString}; use crate::{Allocator, Context}; @@ -26,12 +28,27 @@ pub struct SystemContext { _marker: PhantomData, } +#[cfg(not(loom))] +impl SystemContext<&'static System, M> { + /// Construct a new context which uses the system allocator for memory. + #[inline] + pub fn new() -> Self { + Self::with_alloc(&crate::allocator::SYSTEM) + } +} + +#[cfg(not(loom))] +impl Default for SystemContext<&'static System, M> { + fn default() -> Self { + Self::new() + } +} + impl SystemContext { /// Construct a new context which uses allocations to store arbitrary /// amounts of diagnostics about decoding. - /// - /// Or at least until we run out of memory. - pub fn new(alloc: A) -> Self { + #[inline] + pub fn with_alloc(alloc: A) -> Self { Self { access: Access::new(), mark: Cell::new(0), diff --git a/crates/musli/src/lib.rs b/crates/musli/src/lib.rs index 75dfc0c66..e1afd0e64 100644 --- a/crates/musli/src/lib.rs +++ b/crates/musli/src/lib.rs @@ -406,6 +406,8 @@ extern crate std; pub mod macros; +mod loom; + #[cfg(test)] mod tests; diff --git a/crates/musli/src/loom/cell.rs b/crates/musli/src/loom/cell.rs new file mode 100644 index 000000000..88ddfb493 --- /dev/null +++ b/crates/musli/src/loom/cell.rs @@ -0,0 +1,31 @@ +#[cfg(loom)] +pub(crate) use loom::cell::UnsafeCell; + +#[cfg(not(loom))] +pub(crate) struct UnsafeCell +where + T: ?Sized, +{ + inner: core::cell::UnsafeCell, +} + +#[cfg(not(loom))] +impl UnsafeCell { + #[inline(always)] + pub(crate) const fn new(inner: T) -> Self { + Self { + inner: core::cell::UnsafeCell::new(inner), + } + } +} + +#[cfg(not(loom))] +impl UnsafeCell +where + T: ?Sized, +{ + #[inline(always)] + pub(crate) fn with_mut(&self, f: impl FnOnce(*mut T) -> O) -> O { + f(self.inner.get()) + } +} diff --git a/crates/musli/src/loom/mod.rs b/crates/musli/src/loom/mod.rs new file mode 100644 index 000000000..e33751d2e --- /dev/null +++ b/crates/musli/src/loom/mod.rs @@ -0,0 +1,10 @@ +#![cfg(feature = "alloc")] + +pub(crate) mod cell; +pub(crate) mod sync; + +#[cfg(loom)] +pub(crate) use loom::hint::spin_loop; + +#[cfg(not(loom))] +pub(crate) use core::hint::spin_loop; diff --git a/crates/musli/src/loom/sync.rs b/crates/musli/src/loom/sync.rs new file mode 100644 index 000000000..085020a2f --- /dev/null +++ b/crates/musli/src/loom/sync.rs @@ -0,0 +1,23 @@ +#[cfg(loom)] +pub(crate) use loom::sync::atomic; + +#[cfg(not(loom))] +pub(crate) use core::sync::atomic; + +#[cfg(not(loom))] +#[inline(always)] +pub(crate) fn with_mut_usize( + ptr: &mut atomic::AtomicUsize, + f: impl FnOnce(&mut usize) -> R, +) -> R { + f(ptr.get_mut()) +} + +#[cfg(loom)] +#[inline(always)] +pub(crate) fn with_mut_usize( + ptr: &mut atomic::AtomicUsize, + f: impl FnOnce(&mut usize) -> R, +) -> R { + ptr.with_mut(f) +} diff --git a/crates/musli/src/macros/test.rs b/crates/musli/src/macros/test.rs index da4a007b2..ba31d966c 100644 --- a/crates/musli/src/macros/test.rs +++ b/crates/musli/src/macros/test.rs @@ -43,7 +43,7 @@ macro_rules! test_fns { } $crate::allocator::default!(|alloc| { - let mut cx = $crate::context::SystemContext::new(&alloc); + let mut cx = $crate::context::SystemContext::with_alloc(alloc); cx.include_type(); let out = match ENCODING.to_vec_with(&cx, &value) { @@ -127,7 +127,7 @@ macro_rules! test_fns { } $crate::allocator::default!(|alloc| { - let mut cx = $crate::context::SystemContext::new(&alloc); + let mut cx = $crate::context::SystemContext::with_alloc(alloc); cx.include_type(); out.clear(); @@ -173,7 +173,7 @@ macro_rules! test_fns { use ::core::any::type_name; $crate::allocator::default!(|alloc| { - let mut cx = $crate::context::SystemContext::new(alloc); + let mut cx = $crate::context::SystemContext::with_alloc(alloc); cx.include_type(); match ENCODING.to_vec_with(&cx, &value) { diff --git a/crates/musli/src/tests/loom.rs b/crates/musli/src/tests/loom.rs new file mode 100644 index 000000000..06e0944f4 --- /dev/null +++ b/crates/musli/src/tests/loom.rs @@ -0,0 +1,32 @@ +use std::sync::Arc; + +use crate::allocator::System; +use crate::{Allocator, Buf}; + +const BIG1: &[u8] = &[ + 0x00, 0x01, 0x02, 0x03, 0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x0a, 0x0b, 0x0c, 0x0d, 0x0e, 0x0f, +]; +const BIG2: &[u8] = &[ + 0x20, 0x21, 0x22, 0x23, 0x24, 0x25, 0x26, 0x27, 0x28, 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f, +]; + +fn work(alloc: &System) { + let mut buf1 = alloc.alloc().expect("allocation failed"); + let mut buf2 = alloc.alloc().expect("allocation failed"); + + assert!(buf1.write(BIG1)); + assert!(buf2.write(BIG2)); + + buf1.write_buffer(buf2); + assert!(buf1.as_slice().iter().eq(BIG1.iter().chain(BIG2))); +} + +#[test] +fn test_concurrent_allocator() { + loom::model(|| { + let alloc = Arc::new(System::new()); + let alloc2 = alloc.clone(); + loom::thread::spawn(move || work(&alloc2)); + work(&alloc); + }); +} diff --git a/crates/musli/src/tests/mod.rs b/crates/musli/src/tests/mod.rs index 4e2fec307..eb468e97c 100644 --- a/crates/musli/src/tests/mod.rs +++ b/crates/musli/src/tests/mod.rs @@ -1 +1,4 @@ mod pack_compat; + +#[cfg(loom)] +mod loom; diff --git a/crates/musli/tests/storage_trace.rs b/crates/musli/tests/storage_trace.rs index 401e6d8d8..df423e50b 100644 --- a/crates/musli/tests/storage_trace.rs +++ b/crates/musli/tests/storage_trace.rs @@ -30,35 +30,36 @@ struct To { #[test] fn storage_trace() { - let alloc = System::new(); - let cx = SystemContext::new(&alloc); + musli::allocator::default!(|alloc| { + let cx = SystemContext::with_alloc(alloc); - let from = From { - ok: 10, - field: InnerFrom::Variant2 { + let from = From { ok: 10, - vector: vec![42], - }, - }; + field: InnerFrom::Variant2 { + ok: 10, + vector: vec![42], + }, + }; - let encoding = musli::storage::Encoding::new(); + let encoding = musli::storage::Encoding::new(); - let Ok(bytes) = encoding.to_vec_with(&cx, &from) else { - if let Some(error) = cx.errors().next() { - panic!("{error}"); - } + let Ok(bytes) = encoding.to_vec_with(&cx, &from) else { + if let Some(error) = cx.errors().next() { + panic!("{error}"); + } - unreachable!() - }; + unreachable!() + }; - let Ok(..) = encoding.from_slice_with::<_, To>(&cx, &bytes) else { - if let Some(error) = cx.errors().next() { - assert_eq!(error.to_string(), ".field = Variant2 { .vector[0] }: Tried to read 42 bytes from slice, with 0 byte remaining (at byte 11)"); - return; - } + let Ok(..) = encoding.from_slice_with::<_, To>(&cx, &bytes) else { + if let Some(error) = cx.errors().next() { + assert_eq!(error.to_string(), ".field = Variant2 { .vector[0] }: Tried to read 42 bytes from slice, with 0 byte remaining (at byte 11)"); + return; + } - unreachable!() - }; + unreachable!() + }; - panic!("Expected decoding to error"); + panic!("Expected decoding to error"); + }) } diff --git a/crates/musli/tests/trace_collection.rs b/crates/musli/tests/trace_collection.rs index ff0f3a8fe..49ae6b703 100644 --- a/crates/musli/tests/trace_collection.rs +++ b/crates/musli/tests/trace_collection.rs @@ -19,38 +19,39 @@ struct Collection { #[test] fn trace_collection() { - let alloc = System::new(); - let cx = SystemContext::new(&alloc); + musli::allocator::default!(|alloc| { + let cx = SystemContext::with_alloc(alloc); - let mut values = HashMap::new(); + let mut values = HashMap::new(); - values.insert("Hello".to_string(), "World".to_string()); + values.insert("Hello".to_string(), "World".to_string()); - let from = From { values }; + let from = From { values }; - let encoding = musli::json::Encoding::new(); + let encoding = musli::json::Encoding::new(); - let Ok(bytes) = encoding.to_vec_with(&cx, &from) else { - if let Some(error) = cx.errors().next() { - panic!("{error}"); - } + let Ok(bytes) = encoding.to_vec_with(&cx, &from) else { + if let Some(error) = cx.errors().next() { + panic!("{error}"); + } - unreachable!() - }; + unreachable!() + }; - let cx = SystemContext::new(&alloc); + let cx = SystemContext::with_alloc(alloc); - let Ok(..) = encoding.from_slice_with::<_, Collection>(&cx, &bytes) else { - if let Some(error) = cx.errors().next() { - assert_eq!( - error.to_string(), - ".values[Hello]: Invalid numeric (at bytes 19-20)" - ); - return; - } + let Ok(..) = encoding.from_slice_with::<_, Collection>(&cx, &bytes) else { + if let Some(error) = cx.errors().next() { + assert_eq!( + error.to_string(), + ".values[Hello]: Invalid numeric (at bytes 19-20)" + ); + return; + } - unreachable!() - }; + unreachable!() + }; - panic!("Expected decoding to error"); + panic!("Expected decoding to error"); + }) } diff --git a/crates/musli/tests/trace_complex.rs b/crates/musli/tests/trace_complex.rs index 4082c47dd..fabb0038f 100644 --- a/crates/musli/tests/trace_complex.rs +++ b/crates/musli/tests/trace_complex.rs @@ -33,41 +33,42 @@ struct To { #[test] fn trace_complex() { - let alloc = System::new(); - let cx = SystemContext::new(&alloc); + musli::allocator::default!(|alloc| { + let cx = SystemContext::with_alloc(alloc); - let mut field = HashMap::new(); + let mut field = HashMap::new(); - field.insert( - "hello".to_string(), - InnerFrom::Variant2 { - vector: vec![42], - ok: 10004000, - }, - ); + field.insert( + "hello".to_string(), + InnerFrom::Variant2 { + vector: vec![42], + ok: 10004000, + }, + ); - let from = From { ok: 10, field }; + let from = From { ok: 10, field }; - let encoding = musli::json::Encoding::new(); + let encoding = musli::json::Encoding::new(); - let Ok(bytes) = encoding.to_vec_with(&cx, &from) else { - if let Some(error) = cx.errors().next() { - panic!("{error}"); - } + let Ok(bytes) = encoding.to_vec_with(&cx, &from) else { + if let Some(error) = cx.errors().next() { + panic!("{error}"); + } - unreachable!() - }; + unreachable!() + }; - let cx = SystemContext::new(&alloc); + let cx = SystemContext::with_alloc(alloc); - let Ok(..) = encoding.from_slice_with::<_, To>(&cx, &bytes) else { - if let Some(error) = cx.errors().next() { - assert_eq!(error.to_string(), ".field[hello] = Variant2 { .vector[0] }: Expected string, found (at byte 49)"); - return; - } + let Ok(..) = encoding.from_slice_with::<_, To>(&cx, &bytes) else { + if let Some(error) = cx.errors().next() { + assert_eq!(error.to_string(), ".field[hello] = Variant2 { .vector[0] }: Expected string, found (at byte 49)"); + return; + } - unreachable!() - }; + unreachable!() + }; - panic!("Expected decoding to error"); + panic!("Expected decoding to error"); + }) }