From 8d1f393c39012c33c2fee9df4e4b4d00323c3218 Mon Sep 17 00:00:00 2001 From: Cass Fridkin Date: Fri, 14 Jun 2024 11:51:14 -0400 Subject: [PATCH 1/5] Address clippy lints --- examples/arena.rs | 2 +- examples/fluentresource.rs | 13 ++++---- src/map.rs | 24 +++++++-------- src/sync.rs | 61 +++++++++++++++++++++++--------------- src/vec.rs | 10 +++---- 5 files changed, 63 insertions(+), 47 deletions(-) diff --git a/examples/arena.rs b/examples/arena.rs index 79913c2..b2f38b7 100644 --- a/examples/arena.rs +++ b/examples/arena.rs @@ -52,5 +52,5 @@ impl<'arena> Arena<'arena> { } fn cmp_ref(x: &T, y: &T) -> bool { - x as *const T as usize == y as *const T as usize + std::ptr::eq(x, y) } diff --git a/examples/fluentresource.rs b/examples/fluentresource.rs index dba4aae..a618abd 100644 --- a/examples/fluentresource.rs +++ b/examples/fluentresource.rs @@ -9,12 +9,17 @@ impl<'mgr> FluentResource<'mgr> { // very simple parse step FluentResource(&s[0..1]) } + + pub fn get(&self) -> &'mgr str { + self.0 + } } /// Stores loaded files and parsed ASTs /// /// Parsed ASTs are zero-copy and /// contain references to the files +#[derive(Default)] pub struct ResourceManager<'mgr> { strings: FrozenMap, resources: FrozenMap>>, @@ -22,10 +27,7 @@ pub struct ResourceManager<'mgr> { impl<'mgr> ResourceManager<'mgr> { pub fn new() -> Self { - ResourceManager { - strings: FrozenMap::new(), - resources: FrozenMap::new(), - } + Self::default() } pub fn get_resource(&'mgr self, path: &str) -> &'mgr FluentResource<'mgr> { @@ -46,5 +48,6 @@ impl<'mgr> ResourceManager<'mgr> { fn main() { let manager = ResourceManager::new(); let resource = manager.get_resource("somefile.ftl"); - println!("{:?}", resource); + + assert_eq!(resource.get(), "f"); } diff --git a/src/map.rs b/src/map.rs index 2143e61..7afdcf4 100644 --- a/src/map.rs +++ b/src/map.rs @@ -97,10 +97,10 @@ impl FrozenMap { /// assert_eq!(map.get(&1), Some(&"a")); /// assert_eq!(map.get(&2), None); /// ``` - pub fn get(&self, k: &Q) -> Option<&V::Target> + pub fn get(&self, k: &Q) -> Option<&V::Target> where K: Borrow, - Q: Hash + Eq, + Q: Hash + Eq + ?Sized, { assert!(!self.in_use.get()); self.in_use.set(true); @@ -128,10 +128,10 @@ impl FrozenMap { /// assert_eq!(map.map_get(&1, Clone::clone), Some(Box::new("a"))); /// assert_eq!(map.map_get(&2, Clone::clone), None); /// ``` - pub fn map_get(&self, k: &Q, f: F) -> Option + pub fn map_get(&self, k: &Q, f: F) -> Option where K: Borrow, - Q: Hash + Eq, + Q: Hash + Eq + ?Sized, F: FnOnce(&V) -> T, { assert!(!self.in_use.get()); @@ -192,10 +192,10 @@ impl FrozenMap(&self, k: &Q) -> Option<(&K::Target, &V::Target)> + pub fn get_key_value(&self, k: &Q) -> Option<(&K::Target, &V::Target)> where K: Borrow, - Q: Hash + Eq, + Q: Hash + Eq + ?Sized, { assert!(!self.in_use.get()); self.in_use.set(true); @@ -279,7 +279,7 @@ impl Clone for FrozenMap { in_use: Cell::from(false), }; self.in_use.set(false); - return self_clone; + self_clone } } @@ -384,10 +384,10 @@ impl FrozenBTreeMap { /// assert_eq!(map.get(&1), Some(&"a")); /// assert_eq!(map.get(&2), None); /// ``` - pub fn get(&self, k: &Q) -> Option<&V::Target> + pub fn get(&self, k: &Q) -> Option<&V::Target> where K: Borrow, - Q: Ord, + Q: Ord + ?Sized, { assert!(!self.in_use.get()); self.in_use.set(true); @@ -415,10 +415,10 @@ impl FrozenBTreeMap { /// assert_eq!(map.map_get(&1, Clone::clone), Some(Box::new("a"))); /// assert_eq!(map.map_get(&2, Clone::clone), None); /// ``` - pub fn map_get(&self, k: &Q, f: F) -> Option + pub fn map_get(&self, k: &Q, f: F) -> Option where K: Borrow, - Q: Ord, + Q: Ord + ?Sized, F: FnOnce(&V) -> T, { assert!(!self.in_use.get()); @@ -531,7 +531,7 @@ impl Clone for FrozenBTreeMap { in_use: Cell::from(false), }; self.in_use.set(false); - return self_clone; + self_clone } } diff --git a/src/sync.rs b/src/sync.rs index bb9e608..509ffb1 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -188,10 +188,10 @@ impl FrozenMap { /// assert_eq!(map.get(&1), Some(&"a")); /// assert_eq!(map.get(&2), None); /// ``` - pub fn get(&self, k: &Q) -> Option<&V::Target> + pub fn get(&self, k: &Q) -> Option<&V::Target> where K: Borrow, - Q: Hash + Eq, + Q: Hash + Eq + ?Sized, { let map = self.map.read().unwrap(); let ret = unsafe { map.get(k).map(|x| &*(&**x as *const V::Target)) }; @@ -214,10 +214,10 @@ impl FrozenMap { /// assert_eq!(map.map_get(&1, Clone::clone), Some(Box::new("a"))); /// assert_eq!(map.map_get(&2, Clone::clone), None); /// ``` - pub fn map_get(&self, k: &Q, f: F) -> Option + pub fn map_get(&self, k: &Q, f: F) -> Option where K: Borrow, - Q: Hash + Eq, + Q: Hash + Eq + ?Sized, F: FnOnce(&V) -> T, { let map = self.map.read().unwrap(); @@ -308,10 +308,10 @@ impl FrozenMap { /// assert_eq!(map.get_copy(&1), Some(6)); /// assert_eq!(map.get_copy(&2), None); /// ``` - pub fn get_copy(&self, k: &Q) -> Option + pub fn get_copy(&self, k: &Q) -> Option where K: Borrow, - Q: Hash + Eq, + Q: Hash + Eq + ?Sized, { let map = self.map.read().unwrap(); map.get(k).cloned() @@ -511,7 +511,7 @@ impl FrozenVec { let mut vec = self.vec.write().unwrap(); let index = vec.len(); vec.push(val); - return index; + index } pub fn get(&self, index: usize) -> Option<&T::Target> { @@ -704,7 +704,7 @@ impl Default for LockFreeFrozenVec { /// any heap allocations until the first time data is pushed to it. fn default() -> Self { Self { - data: [Self::NULL; NUM_BUFFERS], + data: std::array::from_fn(|_| Self::null()), len: AtomicUsize::new(0), locked: AtomicBool::new(false), } @@ -712,7 +712,10 @@ impl Default for LockFreeFrozenVec { } impl LockFreeFrozenVec { - const NULL: AtomicPtr = AtomicPtr::new(std::ptr::null_mut()); + const fn null() -> AtomicPtr { + AtomicPtr::new(std::ptr::null_mut()) + } + pub fn new() -> Self { Default::default() } @@ -752,7 +755,7 @@ impl LockFreeFrozenVec { } self.lock(|| { // These values must be consistent with the pointer we got. - let len = self.len.load(Ordering::Acquire); + let len = self.len(); let buffer_idx = buffer_index(len); let mut ptr = self.data[buffer_idx].load(Ordering::Acquire); if ptr.is_null() { @@ -789,8 +792,7 @@ impl LockFreeFrozenVec { // independently of the read is fine. Worst case we // read an old length value and end up returning `None` even if // another thread already inserted the value. - let len = self.len.load(Ordering::Acquire); - if index >= len { + if index >= self.len() { return None; } let buffer_idx = buffer_index(index); @@ -800,12 +802,22 @@ impl LockFreeFrozenVec { } pub fn is_empty(&self) -> bool { - self.len.load(Ordering::Relaxed) == 0 + self.len() == 0 + } + + #[inline(always)] + pub fn len(&self) -> usize { + self.len.load(Ordering::Acquire) } /// Load an element (if it exists). This operation is lock-free and /// performs no synchronized atomic operations. This is a useful primitive to /// implement your own data structure with newtypes around the index. + /// + /// ## Safety + /// + /// `index` must be in bounds, i.e. it must be less than `self.len()` + #[inline] pub unsafe fn get_unchecked(&self, index: usize) -> T { let buffer_idx = buffer_index(index); let buffer_ptr = self.data[buffer_idx].load(Ordering::Relaxed); @@ -844,8 +856,8 @@ impl LockFreeFrozenVec { impl PartialEq for LockFreeFrozenVec { fn eq(&self, other: &Self) -> bool { // first check the length - let self_len = self.len.load(Ordering::Acquire); - let other_len = other.len.load(Ordering::Acquire); + let self_len = self.len(); + let other_len = other.len(); if self_len != other_len { return false; } @@ -857,7 +869,8 @@ impl PartialEq for LockFreeFrozenVec { return false; } } - return true; + + true } } @@ -897,7 +910,7 @@ fn test_non_lockfree_unchecked() { impl Clone for LockFreeFrozenVec { fn clone(&self) -> Self { - let mut coppied_data = [Self::NULL; NUM_BUFFERS]; + let mut coppied_data = std::array::from_fn(|_| Self::null()); // for each buffer, copy the data self.for_each_buffer(|buffer_slice, buffer_index| { // allocate a new buffer @@ -916,11 +929,11 @@ impl Clone for LockFreeFrozenVec { *coppied_data[buffer_index].get_mut() = new_buffer_ptr; }); - return Self { + Self { data: coppied_data, - len: AtomicUsize::new(self.len.load(Ordering::Relaxed)), + len: AtomicUsize::new(self.len()), locked: AtomicBool::new(false), - }; + } } } @@ -1015,10 +1028,10 @@ impl FrozenBTreeMap { /// assert_eq!(map.get(&1), Some(&"a")); /// assert_eq!(map.get(&2), None); /// ``` - pub fn get(&self, k: &Q) -> Option<&V::Target> + pub fn get(&self, k: &Q) -> Option<&V::Target> where K: Borrow, - Q: Ord, + Q: Ord + ?Sized, { let map = self.0.read().unwrap(); let ret = unsafe { map.get(k).map(|x| &*(&**x as *const V::Target)) }; @@ -1061,10 +1074,10 @@ impl FrozenBTreeMap { /// assert_eq!(map.map_get(&1, Clone::clone), Some(Box::new("a"))); /// assert_eq!(map.map_get(&2, Clone::clone), None); /// ``` - pub fn map_get(&self, k: &Q, f: F) -> Option + pub fn map_get(&self, k: &Q, f: F) -> Option where K: Borrow, - Q: Ord, + Q: Ord + ?Sized, F: FnOnce(&V) -> T, { let map = self.0.read().unwrap(); diff --git a/src/vec.rs b/src/vec.rs index ccd71d0..f9d36d5 100644 --- a/src/vec.rs +++ b/src/vec.rs @@ -62,7 +62,7 @@ impl FrozenVec { /// `index` must be in bounds, i.e. it must be less than `self.len()` pub unsafe fn get_unchecked(&self, index: usize) -> &T::Target { let vec = self.vec.get(); - &**(*vec).get_unchecked(index) + (*vec).get_unchecked(index) } } @@ -309,7 +309,7 @@ fn test_iteration() { fn test_accessors() { let vec: FrozenVec = FrozenVec::new(); - assert_eq!(vec.is_empty(), true); + assert!(vec.is_empty()); assert_eq!(vec.len(), 0); assert_eq!(vec.first(), None); assert_eq!(vec.last(), None); @@ -319,7 +319,7 @@ fn test_accessors() { vec.push("b".to_string()); vec.push("c".to_string()); - assert_eq!(vec.is_empty(), false); + assert!(!vec.is_empty()); assert_eq!(vec.len(), 3); assert_eq!(vec.first(), Some("a")); assert_eq!(vec.last(), Some("c")); @@ -332,7 +332,7 @@ fn test_non_stable_deref() { struct Moo(i32); let vec: FrozenVec = FrozenVec::new(); - assert_eq!(vec.is_empty(), true); + assert!(vec.is_empty()); assert_eq!(vec.len(), 0); assert_eq!(vec.get_copy(1), None); @@ -340,7 +340,7 @@ fn test_non_stable_deref() { vec.push(Moo(2)); vec.push(Moo(3)); - assert_eq!(vec.is_empty(), false); + assert!(!vec.is_empty()); assert_eq!(vec.len(), 3); assert_eq!(vec.get_copy(1), Some(Moo(2))); } From 435933f171e0dc4c0785fb22ef34422a89f81c12 Mon Sep 17 00:00:00 2001 From: Cass Fridkin Date: Tue, 18 Jun 2024 20:54:36 -0400 Subject: [PATCH 2/5] Address code review comments --- src/sync.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/sync.rs b/src/sync.rs index 509ffb1..99f67c6 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -704,7 +704,7 @@ impl Default for LockFreeFrozenVec { /// any heap allocations until the first time data is pushed to it. fn default() -> Self { Self { - data: std::array::from_fn(|_| Self::null()), + data: [const { Self::null() }; NUM_BUFFERS], len: AtomicUsize::new(0), locked: AtomicBool::new(false), } @@ -910,7 +910,7 @@ fn test_non_lockfree_unchecked() { impl Clone for LockFreeFrozenVec { fn clone(&self) -> Self { - let mut coppied_data = std::array::from_fn(|_| Self::null()); + let mut coppied_data = [const { Self::null() }; NUM_BUFFERS]; // for each buffer, copy the data self.for_each_buffer(|buffer_slice, buffer_index| { // allocate a new buffer @@ -931,6 +931,8 @@ impl Clone for LockFreeFrozenVec { Self { data: coppied_data, + // Since stores always use `Ordering::Release`, this call to `self.len()` (which uses `Ordering::Acquire`) reults + // in the operation overall being `Ordering::Relaxed` len: AtomicUsize::new(self.len()), locked: AtomicBool::new(false), } From 416560705e2b30b617f570cf594eaf2b337ca692 Mon Sep 17 00:00:00 2001 From: Cass Fridkin Date: Tue, 18 Jun 2024 21:06:36 -0400 Subject: [PATCH 3/5] Try a different strategy for const null pointers --- src/sync.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/sync.rs b/src/sync.rs index 99f67c6..351e8c0 100644 --- a/src/sync.rs +++ b/src/sync.rs @@ -704,7 +704,7 @@ impl Default for LockFreeFrozenVec { /// any heap allocations until the first time data is pushed to it. fn default() -> Self { Self { - data: [const { Self::null() }; NUM_BUFFERS], + data: Self::null(), len: AtomicUsize::new(0), locked: AtomicBool::new(false), } @@ -712,8 +712,8 @@ impl Default for LockFreeFrozenVec { } impl LockFreeFrozenVec { - const fn null() -> AtomicPtr { - AtomicPtr::new(std::ptr::null_mut()) + const fn null() -> [AtomicPtr; NUM_BUFFERS] { + [const { AtomicPtr::new(std::ptr::null_mut()) }; NUM_BUFFERS] } pub fn new() -> Self { @@ -910,7 +910,7 @@ fn test_non_lockfree_unchecked() { impl Clone for LockFreeFrozenVec { fn clone(&self) -> Self { - let mut coppied_data = [const { Self::null() }; NUM_BUFFERS]; + let mut coppied_data = Self::null(); // for each buffer, copy the data self.for_each_buffer(|buffer_slice, buffer_index| { // allocate a new buffer From d22485fd84e3c633ffcc91b5a3dc14ad5b40121c Mon Sep 17 00:00:00 2001 From: Cass Fridkin Date: Tue, 18 Jun 2024 21:32:29 -0400 Subject: [PATCH 4/5] Add rust-toolchain.toml --- rust-toolchain.toml | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 rust-toolchain.toml diff --git a/rust-toolchain.toml b/rust-toolchain.toml new file mode 100644 index 0000000..292fe49 --- /dev/null +++ b/rust-toolchain.toml @@ -0,0 +1,2 @@ +[toolchain] +channel = "stable" From a39c9b1475acbaa53b27c00567c48196ece734ad Mon Sep 17 00:00:00 2001 From: Cass Fridkin Date: Tue, 25 Jun 2024 12:05:48 -0400 Subject: [PATCH 5/5] Revert "Add rust-toolchain.toml" This reverts commit d22485fd84e3c633ffcc91b5a3dc14ad5b40121c. --- rust-toolchain.toml | 2 -- 1 file changed, 2 deletions(-) delete mode 100644 rust-toolchain.toml diff --git a/rust-toolchain.toml b/rust-toolchain.toml deleted file mode 100644 index 292fe49..0000000 --- a/rust-toolchain.toml +++ /dev/null @@ -1,2 +0,0 @@ -[toolchain] -channel = "stable"