From c4d3a4451dbc6bf410ccccffdeb7f8a11c15f9ec Mon Sep 17 00:00:00 2001 From: luca3s <41015240+luca3s@users.noreply.github.com> Date: Wed, 13 Nov 2024 15:29:43 +0100 Subject: [PATCH] imporved internals. less code duplication. one less byte allocated. Update version because of new Send/Sync requirements. --- Cargo.lock | 2 +- simple-left-right/Cargo.toml | 2 +- simple-left-right/src/inner.rs | 179 +++++++++++++++++++++------------ simple-left-right/src/lib.rs | 147 +++++++-------------------- 4 files changed, 154 insertions(+), 176 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8265b15..1c44ee1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -714,7 +714,7 @@ checksum = "0fda2ff0d084019ba4d7c6f371c95d8fd75ce3524c3cb8fb653a3023f6323e64" [[package]] name = "simple-left-right" -version = "0.1.1" +version = "0.2.0" dependencies = [ "async-io", "crossbeam-utils", diff --git a/simple-left-right/Cargo.toml b/simple-left-right/Cargo.toml index 155de6a..d4a95fa 100644 --- a/simple-left-right/Cargo.toml +++ b/simple-left-right/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "simple-left-right" -version = "0.1.1" +version = "0.2.0" edition = "2021" rust-version = "1.82" readme = "README.md" diff --git a/simple-left-right/src/inner.rs b/simple-left-right/src/inner.rs index 3391038..cf3f064 100644 --- a/simple-left-right/src/inner.rs +++ b/simple-left-right/src/inner.rs @@ -1,11 +1,15 @@ use core::{ - cell::UnsafeCell, hint::unreachable_unchecked, mem::MaybeUninit, sync::atomic::{self, AtomicU8, Ordering} + cell::UnsafeCell, + hint::assert_unchecked, + mem::MaybeUninit, + sync::atomic::{self, AtomicU8, Ordering}, }; -#[derive(PartialEq, Eq, Clone, Copy, Debug)] +#[derive(Clone, Copy, Debug)] +#[repr(u8)] pub(crate) enum Ptr { - Value1, - Value2, + Value1 = 0, + Value2 = 0b10000, } impl Ptr { @@ -15,50 +19,51 @@ impl Ptr { Ptr::Value2 => Self::Value1, }; } - - /// SAFETY: Assumes no read bits (lower two) are set - pub(crate) unsafe fn from_u8_no_read(value: u8) -> Self { - match value { - 0b000 => Self::Value1, - 0b100 => Self::Value2, - // SAFETY: unsafe fn. communicated in docs - _ => unsafe { unreachable_unchecked() }, - } - } } -#[derive(Debug)] -pub(crate) enum ReadState { - None, - Value(Ptr), -} +#[repr(transparent)] +struct State(u8); -impl ReadState { - /// is writing on the passed ptr parameter valid with the current read state? +impl State { + const NOREAD_VALUE1_1ACCESS: Self = Self(0b00001); + const NOREAD_MASK: u8 = 0b10011; + const READ_MASK: u8 = 0b01100; + + // this should be inlined to have the assert_unchecked be useful #[inline] - pub(crate) fn can_write(&self, ptr: Ptr) -> bool { - match self { - ReadState::None => true, - ReadState::Value(p) => *p != ptr, + fn access_count(self) -> u8 { + // mask out the top bits + let access = self.0 & 0b00011; + // SAFETY: The library only supports one reader and one writer at a time. This is checked in the is_unique functions of Shared + // the compiler already knows that it is 3 at max. we constrict it further + unsafe { assert_unchecked(access <= 2); } + access + } + + fn read_ptr(self) -> Ptr { + // mask out everything except the read ptr + // (self.0 & 0b10000) as Ptr + if self.0 & 0b10000 == 0 { + Ptr::Value1 + } else { + Ptr::Value2 } } - /// SAFETY: needs to be the internal state u8. - pub(crate) unsafe fn from_u8_ignore_ptr(value: u8) -> Self { - match value & 0b011 { - 0b00 => Self::None, - 0b01 => Self::Value(Ptr::Value1), - 0b10 => Self::Value(Ptr::Value2), - // SAFETY: Internal Library Value only. - _ => unsafe { unreachable_unchecked() }, + fn with_read(self, ptr: Ptr) -> Self { + match ptr { + Ptr::Value1 => Self(self.0 | 0b00100), + Ptr::Value2 => Self(self.0 | 0b01000), } } -} -impl From for ReadState { - #[inline] - fn from(value: Ptr) -> Self { - Self::Value(value) + fn can_write(self, ptr: Ptr) -> bool { + #[expect(clippy::match_like_matches_macro)] // i think it's more readable like this + match (self.0 & Self::READ_MASK, ptr) { + (0b0100, Ptr::Value1) => false, + (0b1000, Ptr::Value2) => false, + _ => true + } } } @@ -66,21 +71,66 @@ impl From for ReadState { pub(crate) struct Shared { pub value_1: UnsafeCell, pub value_2: UnsafeCell, - /// bit 0: is value 1 being read - /// bit 1: is value 2 being read - /// bit 3: which value should be read next (0: value 1, 1: value 2) - pub state: AtomicU8, - pub access_count: AtomicU8, + /// ### Bits from low to high + /// | bit | meaning | + /// |---|---| + /// | 0-1 | how many Reader or Writer exist (max 2) | + /// | 2 | is value 1 being read | + /// | 3 | is value 2 being read | + /// | 4 | which value should be read next (0: value 1, 1: value 2) | + /// + /// This mixed use doesn't lead to more contention because there are only two threads max. + /// + /// Access count is in the lower bits, so that fetch_add and fetch_sub still work for that purpose. + /// Locking and Unlocking is done via bitand / or and fetch_update anyways + state: AtomicU8, } impl Shared { - /// initializes everything except for both values. - pub(crate) fn initialize_state(this: &mut MaybeUninit) { + pub(crate) fn lock_read(&self) -> Ptr { + let result = self + .state + .fetch_update(Ordering::Relaxed, Ordering::Acquire, |value| { + Some(State(value).with_read(State(value).read_ptr()).0) + }); + // SAFETY: fetch_update closure always returns Some, so the result is alwyays Ok + let result = unsafe { result.unwrap_unchecked() }; + // result is the previous value, so the read_state isn't set, only the read_ptr + State(result).read_ptr() + } + + pub(crate) fn release_read_lock(&self) { + self.state + .fetch_and(State::NOREAD_MASK, Ordering::Release); + } + + /// tries to get the write lock to the ptr. + pub(crate) fn lock_write(&self, ptr: Ptr) -> Result<(), ()> { + let state = State(self.state.load(Ordering::Relaxed)); + if state.can_write(ptr) { + // only need to synchronize with another thread when locking was successfull + atomic::fence(Ordering::Acquire); + Ok(()) + } else { + Err(()) + } + } + + /// Releases the read lock + pub(crate) fn set_read_ptr(&self, ptr: Ptr) { + match ptr { + Ptr::Value1 => self.state.fetch_and(0b01111, Ordering::Release), + Ptr::Value2 => self.state.fetch_or(0b10000, Ordering::Release), + }; + } + + /// initializes the internal state. returns the ptr that + pub(crate) fn initialize_state(this: &mut MaybeUninit) -> Ptr { // SAFETY: takes &mut self, so no writing is okay unsafe { - (&raw mut (*this.as_mut_ptr()).access_count).write(AtomicU8::new(1)); - (&raw mut (*this.as_mut_ptr()).state).write(AtomicU8::new(0b000)); + (&raw mut (*this.as_mut_ptr()).state).write(AtomicU8::new(State::NOREAD_VALUE1_1ACCESS.0)); } + Ptr::Value2 } pub(crate) fn get_value(&self, ptr: Ptr) -> &UnsafeCell { @@ -90,25 +140,32 @@ impl Shared { } } + /// SAFETY: needs to have synchronized shared access to the ptr. + /// If the access is not unique T needs to be Sync + pub(crate) unsafe fn get_value_ref(&self, ptr: Ptr) -> &T { + // SAFETY: requirements on the function make it safe + unsafe { &*self.get_value(ptr).get() } + } + /// If self is unique increase the count and returns true. /// Otherwise returns false. /// /// If this returns true another smart pointer has to be created otherwise memory will be leaked pub(crate) unsafe fn is_unique_with_increase(&self) -> bool { - // Relaxed taken from std Arc - let old_access = self.access_count.load(Ordering::Acquire); - debug_assert!( - old_access <= 2, - "at maximum there is one Reader and one Writer" - ); - if old_access == 1 { - self.access_count.fetch_add(1, Ordering::Relaxed); + if self.is_unique() { + // relaxed taken from std Arc + self.state.fetch_add(1, Ordering::Relaxed); true } else { false } } + pub(crate) fn is_unique(&self) -> bool { + let access = State(self.state.load(Ordering::Acquire)).access_count(); + access == 1 + } + /// decreases the access_count and returns if self should now be dropped. /// /// | return | needed reaction @@ -118,11 +175,7 @@ impl Shared { /// /// dropping self when this returns true is safe and needed synchronisation has been done. pub(crate) unsafe fn should_drop(&self) -> bool { - let old_access = self.access_count.fetch_sub(1, Ordering::Release); - debug_assert!( - old_access <= 2, - "at maximum there is one Reader and one Writer" - ); + let old_access = State(self.state.fetch_sub(1, Ordering::Release)).access_count(); if old_access != 1 { return false; } @@ -130,10 +183,4 @@ impl Shared { atomic::fence(Ordering::Acquire); true } - - pub(crate) fn is_unique(&self) -> bool { - let access = self.access_count.load(Ordering::Acquire); - debug_assert!(access <= 2, "at maximum there is one Reader and one Writer"); - access == 1 - } } diff --git a/simple-left-right/src/lib.rs b/simple-left-right/src/lib.rs index a9ce4ff..a77124f 100644 --- a/simple-left-right/src/lib.rs +++ b/simple-left-right/src/lib.rs @@ -31,14 +31,18 @@ use core::time::Duration; use std::thread; use core::{ - cell::UnsafeCell, marker::PhantomData, mem::MaybeUninit, ops::Deref, ptr::NonNull, sync::atomic::{fence, AtomicU8, Ordering} + cell::UnsafeCell, + marker::PhantomData, + mem::MaybeUninit, + ops::Deref, + ptr::NonNull, }; -use alloc::{collections::vec_deque::VecDeque, boxed::Box}; +use alloc::{boxed::Box, collections::vec_deque::VecDeque}; mod inner; -use inner::{Ptr, ReadState, Shared}; +use inner::{Ptr, Shared}; /// Should be implemented on structs that want to be shared with this library pub trait Absorb { @@ -69,36 +73,11 @@ impl Reader { /// this function never blocks. (`fetch_update` loop doesn't count) pub fn lock(&mut self) -> ReadGuard<'_, T> { - let inner_ref = self.shared_ref(); - // sets the corresponding read bit to the write ptr bit - // happens as a single atomic operation so the 'double read' state isn't needed - // ptr bit doesnt get changed - // always Ok, as the passed closure never returns None - let update_result = - inner_ref - .state - .fetch_update(Ordering::Relaxed, Ordering::Acquire, |value| { - // SAFETY: At this point no Read bit is set, as creating a ReadGuard requires a &mut Reader and the Guard holds the &mut Reader - unsafe { - match Ptr::from_u8_no_read(value) { - Ptr::Value1 => Some(0b001), - Ptr::Value2 => Some(0b110), - } - } - }); - - // here the read ptr and read state of update_result match. maybe the atomic was already changed, but that doesn't matter. - // we continue working with the state that we set. - - // SAFETY: the passed closure always returns Some, so fetch_update never returns Err - let ptr = unsafe { Ptr::from_u8_no_read(update_result.unwrap_unchecked()) }; - - // SAFETY: the Writer allowed the read on this value because the ptr bit was set. The read bit has been set - let data = unsafe { inner_ref.get_value(ptr).get().as_ref().unwrap_unchecked() }; + let shared_ref = self.shared_ref(); ReadGuard { - data, - state: &inner_ref.state, + shared: shared_ref, + value: shared_ref.lock_read(), reader: PhantomData, } } @@ -126,8 +105,8 @@ impl Drop for Reader { /// Doesn't implement Clone as that would require refcounting to know when to unlock. #[derive(Debug)] pub struct ReadGuard<'a, T> { - data: &'a T, - state: &'a AtomicU8, + shared: &'a Shared, + value: Ptr, /// PhantomData makes the borrow checker prove that there only ever is one ReadGuard. /// This allows resetting the readstate without some kind of counter reader: PhantomData<&'a mut Reader>, @@ -137,7 +116,8 @@ impl<'a, T> Deref for ReadGuard<'a, T> { type Target = T; fn deref(&self) -> &Self::Target { - self.data + // SAFETY: ReadGuard was created, so the Writer knows not to write in this spot + unsafe { self.shared.get_value_ref(self.value) } } } @@ -151,15 +131,15 @@ where } } -/// SAFETY: behaves like a ref to T. https://doc.rust-lang.org/std/marker/trait.Sync.html -unsafe impl Send for ReadGuard<'_, T> {} -/// SAFETY: behaves like a ref to T. https://doc.rust-lang.org/std/marker/trait.Sync.html -unsafe impl Sync for ReadGuard<'_, T> {} +// /// SAFETY: behaves like a ref to T. https://doc.rust-lang.org/std/marker/trait.Sync.html +// unsafe impl Send for ReadGuard<'_, T> {} +// /// SAFETY: behaves like a ref to T. https://doc.rust-lang.org/std/marker/trait.Sync.html +// unsafe impl Sync for ReadGuard<'_, T> {} impl Drop for ReadGuard<'_, T> { fn drop(&mut self) { // release the read lock - self.state.fetch_and(0b100, Ordering::Release); + self.shared.release_read_lock(); } } @@ -190,10 +170,7 @@ impl Writer { return; } - match self.write_ptr { - Ptr::Value1 => self.shared_ref().state.fetch_and(0b011, Ordering::Release), - Ptr::Value2 => self.shared_ref().state.fetch_or(0b100, Ordering::Release), - }; + self.shared_ref().set_read_ptr(self.write_ptr); self.write_ptr.switch(); } @@ -222,16 +199,7 @@ impl, O> Writer { let backoff = crossbeam_utils::Backoff::new(); loop { - // operation has to be aquire, but only the time it breaks the loop - let state = self.shared_ref().state.load(Ordering::Relaxed); - - // SAFETY: is in state internal only value which is only set by library code - let state = unsafe { ReadState::from_u8_ignore_ptr(state) }; - - if state.can_write(self.write_ptr) { - // make the load operation aquire only when it actually breaks the loop - // the important (last) load is aquire, while all loads before are relaxed - fence(Ordering::Acquire); + if self.shared_ref().lock_write(self.write_ptr).is_ok() { break; } @@ -252,16 +220,7 @@ impl, O> Writer { let backoff = crossbeam_utils::Backoff::new(); loop { - // operation has to be aquire, but only the time it breaks the loop - let state = self.shared_ref().state.load(Ordering::Relaxed); - - // SAFETY: is in state internal only value which is only set by library code - let state = unsafe { ReadState::from_u8_ignore_ptr(state) }; - - if state.can_write(self.write_ptr) { - // make the load operation aquire, only when it actually breaks the loop - // the important (last) load is aquire, while all loads before are relaxed - fence(Ordering::Acquire); + if self.shared_ref().lock_write(self.write_ptr).is_ok() { break; } @@ -284,16 +243,7 @@ impl, O> Writer { let backoff = crossbeam_utils::Backoff::new(); loop { - // operation has to be aquire, but only the time it breaks the loop - let state = self.shared_ref().state.load(Ordering::Relaxed); - - // SAFETY: is in state internal only value which is only set by library code - let state = unsafe { ReadState::from_u8_ignore_ptr(state) }; - - if state.can_write(self.write_ptr) { - // make the load operation aquire, only when it actually breaks the loop - // the important (last) load is aquire, while all loads before are relaxed - fence(Ordering::Acquire); + if self.shared_ref().lock_write(self.write_ptr).is_ok() { break; } @@ -312,24 +262,17 @@ impl, O> Writer { /// doesn't block. Returns None if the Reader has a `ReadGuard` pointing to the old value pub fn try_lock(&mut self) -> Option> { - let state = self.shared_ref().state.load(Ordering::Acquire); - - // SAFETY: is in state internal only value which is only set by library code - let state = unsafe { ReadState::from_u8_ignore_ptr(state) }; - - if state.can_write(self.write_ptr) { - // SAFETY: ReadState allows this - unsafe { Some(WriteGuard::new(self)) } - } else { - None - } + self.shared_ref() + .lock_write(self.write_ptr) + .ok() + // SAFETY: locking was successful + .map(|_| unsafe { WriteGuard::new(self) }) } } impl Writer { /// Creates a new Writer by cloning the value once to get two values pub fn new(value: T) -> Self { - // SAFETY: ptr was just alloced, so is valid and unique. let mut shared: Box>> = Box::new_uninit(); Shared::initialize_state(&mut shared); let shared_ptr = shared.as_mut_ptr(); @@ -358,7 +301,6 @@ impl Default for Writer { /// /// Could leak a T object if T::default() panics. fn default() -> Self { - // SAFETY: ptr was just alloced, so is valid and unique. let mut shared: Box>> = Box::new_uninit(); Shared::initialize_state(&mut shared); let shared_ptr = shared.as_mut_ptr(); @@ -389,19 +331,13 @@ impl Writer { // SAFETY: Only the WriteGuard can write to the values / create mut refs to them. // The WriteGuard holds a mut ref to the writer so this function can't be called while a writeguard exists // This means that reading them / creating refs is safe to do - unsafe { - self.shared_ref() - .get_value(self.write_ptr) - .get() - .as_ref() - .unwrap_unchecked() - } + unsafe { self.shared_ref().get_value_ref(self.write_ptr) } } } /// SAFETY: owns T and O unsafe impl Send for Writer {} -/// SAFETY: &self fn can only create a &T and doesn't allow access to O +/// SAFETY: &self fn can only create a &T and never gives shared access to O unsafe impl Sync for Writer {} impl Drop for Writer { @@ -438,11 +374,9 @@ impl WriteGuard<'_, T, O> { // The WriteGuard holds a mut ref to the writer so this function can't be called while a writeguard exists // This means that reading them / creating refs is safe to do unsafe { - self.writer.shared_ref() - .get_value(self.writer.write_ptr) - .get() - .as_ref() - .unwrap_unchecked() + self.writer + .shared_ref() + .get_value_ref(self.writer.write_ptr) } } @@ -451,14 +385,11 @@ impl WriteGuard<'_, T, O> { fn get_data_mut(&mut self) -> &mut T { // SAFETY: When creating the writeguad it is checked that the reader doesnt have access to the same data // This function requires &mut self so there also isn't any ref created by writeguard. - // SAFETY: the ptr is never null, therefore unwrap_unchecked unsafe { - self.writer + &mut *self.writer .shared_ref() .get_value(self.writer.write_ptr) .get() - .as_mut() - .unwrap_unchecked() } } } @@ -484,8 +415,8 @@ impl, O: Clone> WriteGuard<'_, T, O> { /// applies operation to the current write Value and stores it to apply to the other later. /// If there is no reader the operation is applied to both values immediately and not stored. pub fn apply_op(&mut self, operation: O) { + let shared_ref = self.writer.shared_ref(); if self.writer.shared_ref().is_unique() { - let shared_ref = self.writer.shared_ref(); // SAFETY: is_unique checked that no Reader exists. I am the only one with access to Shared, so i can modify whatever i want. unsafe { (*shared_ref.value_1.get()).absorb(operation.clone()); @@ -498,11 +429,11 @@ impl, O: Clone> WriteGuard<'_, T, O> { } } -/// SAFETY: behaves like a &mut T and &mut Vec. https://doc.rust-lang.org/stable/std/marker/trait.Sync.html -unsafe impl Send for WriteGuard<'_, T, O> {} +// /// SAFETY: behaves like a &mut T and &mut Vec. https://doc.rust-lang.org/stable/std/marker/trait.Sync.html +// unsafe impl Send for WriteGuard<'_, T, O> {} -/// Safety: can only create shared refs to T, not to O. https://doc.rust-lang.org/stable/std/marker/trait.Sync.html -unsafe impl Sync for WriteGuard<'_, T, O> {} +// /// Safety: can only create shared refs to T, not to O. https://doc.rust-lang.org/stable/std/marker/trait.Sync.html +// unsafe impl Sync for WriteGuard<'_, T, O> {} impl Drop for WriteGuard<'_, T, O> { fn drop(&mut self) { -- 2.51.2