From d41252249299e5a7cb2da3b210945a6236c56962 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 28 Apr 2020 21:56:38 +0200 Subject: [PATCH 1/6] Different implementations based on type --- CHANGELOG.md | 1 + src/lib.rs | 104 ++++++++++++++++++++++++++++++++++++++------------ tests/test.rs | 28 +++++++------- 3 files changed, 95 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0637d20..85409f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,6 @@ Unreleased ========== +* Provide different implementations based on AccessMode template parameter [@bugadani](https://github.com/bugadani) * Verify that keys are used to access data in their associated Slots instances [@bugadani](https://github.com/bugadani) * Allow compiler to reduce memory footprint if possible [@chrysn](https://github.com/chrysn) * Allow using FnOnce closures in `try_read`, `read` and `modify` [@chrysn](https://github.com/chrysn) diff --git a/src/lib.rs b/src/lib.rs index 494f906..571b8c9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -8,10 +8,10 @@ //! Example usage: //! //! ``` -//! use slots::Slots; +//! use slots::{Slots, Strict}; //! use slots::consts::U6; //! -//! let mut slots: Slots<_, U6> = Slots::new(); // Capacity of 6 elements +//! let mut slots: Slots<_, U6, Strict> = Slots::new(); // Capacity of 6 elements //! //! // Store elements //! let k1 = slots.store(2).unwrap(); // returns Err(2) if full @@ -59,6 +59,13 @@ pub use generic_array::ArrayLength; use generic_array::typenum::Unsigned; +pub struct Relaxed {} +pub struct Strict {} +pub trait AccessMode {} + +impl AccessMode for Relaxed {} +impl AccessMode for Strict {} + pub struct Key { #[cfg(feature = "verify_owner")] owner_id: usize, @@ -69,7 +76,7 @@ pub struct Key { impl Key where N: ArrayLength> + Unsigned { - fn new(owner: &Slots, idx: usize) -> Self { + fn new(owner: &Slots, idx: usize) -> Self { Self { #[cfg(feature = "verify_owner")] owner_id: owner.id, @@ -95,15 +102,17 @@ enum EntryInner { // Data type that stores values and returns a key that can be used to manipulate // the stored values. // Values can be read by anyone but can only be modified using the key. -pub struct Slots - where N: ArrayLength> + Unsigned { +pub struct Slots + where N: ArrayLength> + Unsigned, + A: AccessMode { #[cfg(feature = "verify_owner")] id: usize, items: GenericArray, N>, // Could be optimized by making it just usize and relying on free_count to determine its // validity next_free: Option, - free_count: usize + free_count: usize, + _mode_marker: PhantomData } #[cfg(feature = "verify_owner")] @@ -113,8 +122,9 @@ fn new_instance_id() -> usize { COUNTER.fetch_add(1, Ordering::Relaxed) } -impl Slots - where N: ArrayLength> + Unsigned { +impl Slots + where N: ArrayLength> + Unsigned, + A: AccessMode { pub fn new() -> Self { let size = N::to_usize(); @@ -123,19 +133,11 @@ impl Slots id: new_instance_id(), items: GenericArray::generate(|i| Entry(i.checked_sub(1).map(EntryInner::EmptyNext).unwrap_or(EntryInner::EmptyLast))), next_free: size.checked_sub(1), - free_count: size + free_count: size, + _mode_marker: PhantomData } } - #[cfg(feature = "verify_owner")] - fn verify_key(&self, key: &Key) { - assert!(key.owner_id == self.id, "Key used in wrong instance"); - } - - #[cfg(not(feature = "verify_owner"))] - fn verify_key(&self, _key: &Key) { - } - pub fn capacity(&self) -> usize { N::to_usize() } @@ -163,6 +165,19 @@ impl Slots self.free_count -= 1; Some(index) } +} + +impl Slots + where N: ArrayLength> + Unsigned { + + #[cfg(feature = "verify_owner")] + fn verify_key(&self, key: &Key) { + assert!(key.owner_id == self.id, "Key used in wrong instance"); + } + + #[cfg(not(feature = "verify_owner"))] + fn verify_key(&self, _key: &Key) { + } pub fn store(&mut self, item: IT) -> Result, IT> { match self.alloc() { @@ -178,9 +193,11 @@ impl Slots self.verify_key(&key); let taken = core::mem::replace(&mut self.items[key.index], Entry(EntryInner::EmptyLast)); - self.free(key.index); match taken.0 { - EntryInner::Used(item) => item, + EntryInner::Used(item) => { + self.free(key.index); + item + }, _ => unreachable!("Invalid key") } } @@ -194,19 +211,58 @@ impl Slots } } + pub fn modify(&mut self, key: &Key, function: F) -> T where F: FnOnce(&mut IT) -> T { + self.verify_key(&key); + + match self.items[key.index].0 { + EntryInner::Used(ref mut item) => function(item), + _ => unreachable!("Invalid key") + } + } + pub fn try_read(&self, key: usize, function: F) -> Option where F: FnOnce(&IT) -> T { match &self.items[key].0 { EntryInner::Used(item) => Some(function(&item)), _ => None } } +} - pub fn modify(&mut self, key: &Key, function: F) -> T where F: FnOnce(&mut IT) -> T { - self.verify_key(&key); +impl Slots + where N: ArrayLength> + Unsigned { + + pub fn store(&mut self, item: IT) -> Result { + match self.alloc() { + Some(i) => { + self.items[i] = Entry(EntryInner::Used(item)); + Ok(i) + } + None => Err(item) + } + } + pub fn take(&mut self, key: Key) -> Option { + let taken = core::mem::replace(&mut self.items[key.index], Entry(EntryInner::EmptyLast)); + match taken.0 { + EntryInner::Used(item) => { + self.free(key.index); + Some(item) + }, + _ => None + } + } + + pub fn modify(&mut self, key: &Key, function: F) -> Option where F: FnOnce(&mut IT) -> T { match self.items[key.index].0 { - EntryInner::Used(ref mut item) => function(item), - _ => unreachable!("Invalid key") + EntryInner::Used(ref mut item) => Some(function(item)), + _ => None + } + } + + pub fn read(&self, key: usize, function: F) -> Option where F: FnOnce(&IT) -> T { + match &self.items[key].0 { + EntryInner::Used(item) => Some(function(&item)), + _ => None } } } diff --git a/tests/test.rs b/tests/test.rs index aba69f6..10117a4 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -1,9 +1,9 @@ -use slots::Slots; +use slots::{Slots, Strict}; use slots::consts::*; #[test] fn key_can_be_used_to_read_value() { - let mut slots: Slots<_, U8> = Slots::new(); + let mut slots: Slots<_, U8, Strict> = Slots::new(); let k1 = slots.store(5).unwrap(); assert_eq!(5, slots.read(&k1, |&w| w)); @@ -11,7 +11,7 @@ fn key_can_be_used_to_read_value() { #[test] fn size_can_be_1() { - let mut slots: Slots<_, U1> = Slots::new(); + let mut slots: Slots<_, U1, Strict> = Slots::new(); let k1 = slots.store(5).unwrap(); assert_eq!(5, slots.read(&k1, |&w| w)); @@ -27,7 +27,7 @@ fn size_can_be_1() { #[test] fn index_can_be_used_to_read_value() { - let mut slots: Slots<_, U8> = Slots::new(); + let mut slots: Slots<_, U8, Strict> = Slots::new(); slots.store(5).unwrap(); slots.store(6).unwrap(); @@ -40,14 +40,14 @@ fn index_can_be_used_to_read_value() { #[test] fn trying_to_read_missing_element_returns_none() { - let slots: Slots = Slots::new(); + let slots: Slots = Slots::new(); assert_eq!(None, slots.try_read(0, |&w| w)); } #[test] fn trying_to_read_deleted_element_returns_none() { - let mut slots: Slots = Slots::new(); + let mut slots: Slots = Slots::new(); slots.store(5).unwrap(); let k = slots.store(6).unwrap(); @@ -62,7 +62,7 @@ fn trying_to_read_deleted_element_returns_none() { #[test] fn elements_can_be_modified_using_key() { - let mut slots: Slots = Slots::new(); + let mut slots: Slots = Slots::new(); let k = slots.store(5).unwrap(); @@ -75,7 +75,7 @@ fn elements_can_be_modified_using_key() { #[test] fn store_returns_err_when_full() { - let mut slots: Slots = Slots::new(); + let mut slots: Slots = Slots::new(); slots.store(5); @@ -88,8 +88,8 @@ fn store_returns_err_when_full() { #[cfg(feature = "verify_owner")] #[should_panic(expected = "Key used in wrong instance")] fn use_across_slots_verify() { - let mut a: Slots = Slots::new(); - let mut b: Slots = Slots::new(); + let mut a: Slots = Slots::new(); + let mut b: Slots = Slots::new(); let k = a.store(5).expect("There should be room"); // Store an element in b so we don't get a different panic @@ -101,8 +101,8 @@ fn use_across_slots_verify() { #[test] #[cfg(not(feature = "verify_owner"))] fn use_across_slots_no_verify() { - let mut a: Slots = Slots::new(); - let mut b: Slots = Slots::new(); + let mut a: Slots = Slots::new(); + let mut b: Slots = Slots::new(); let k = a.store(5).expect("There should be room"); // Store an element in b so we don't get a different panic @@ -138,12 +138,12 @@ fn is_compact() { if cfg!(feature = "verify_owner") { expected_size += core::mem::size_of::(); // an extra usize for object id } - assert_eq!(core::mem::size_of::>(), expected_size, "Compiled size does not match expected"); + assert_eq!(core::mem::size_of::>(), expected_size, "Compiled size does not match expected"); } #[test] fn capacity_and_count() { - let mut slots: Slots = Slots::new(); + let mut slots: Slots = Slots::new(); assert_eq!(slots.capacity(), 4); assert_eq!(slots.count(), 0); From afda384dbf246003a5a87230753f31e537fa973f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 28 Apr 2020 23:26:29 +0200 Subject: [PATCH 2/6] Don't reserve space for object id in relaxed Slots --- src/lib.rs | 75 +++++++++++++++++++++++++++++++-------------------- tests/test.rs | 20 ++++++++++---- 2 files changed, 61 insertions(+), 34 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 571b8c9..2caeff5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -50,8 +50,6 @@ #![cfg_attr(not(test), no_std)] use core::marker::PhantomData; -#[cfg(feature = "verify_owner")] -use core::sync::atomic::{AtomicUsize, Ordering}; use generic_array::{GenericArray, sequence::GenericSequence}; pub use generic_array::typenum::consts; @@ -59,16 +57,50 @@ pub use generic_array::ArrayLength; use generic_array::typenum::Unsigned; -pub struct Relaxed {} -pub struct Strict {} -pub trait AccessMode {} +mod access_mode { + pub struct Relaxed {} + pub struct Strict {} -impl AccessMode for Relaxed {} -impl AccessMode for Strict {} + pub trait AccessMode { + type ObjectId; -pub struct Key { - #[cfg(feature = "verify_owner")] - owner_id: usize, + fn new_obj_id() -> Self::ObjectId; + } + + impl AccessMode for Relaxed { + type ObjectId = (); + + fn new_obj_id() -> Self::ObjectId { + () + } + } + impl AccessMode for Strict { + #[cfg(feature = "verify_owner")] + type ObjectId = usize; + + #[cfg(not(feature = "verify_owner"))] + type ObjectId = (); + + #[cfg(feature = "verify_owner")] + fn new_obj_id() -> Self::ObjectId { + use core::sync::atomic::{AtomicUsize, Ordering}; + static COUNTER: AtomicUsize = AtomicUsize::new(0); + + COUNTER.fetch_add(1, Ordering::Relaxed) + } + + #[cfg(not(feature = "verify_owner"))] + fn new_obj_id() -> Self::ObjectId { + () + } + } +} + +pub use access_mode::{Strict, Relaxed}; + +pub struct Key + where N: ArrayLength> + Unsigned { + owner_id: ::ObjectId, index: usize, _item_marker: PhantomData, _size_marker: PhantomData @@ -78,7 +110,6 @@ impl Key where N: ArrayLength> + Unsigned { fn new(owner: &Slots, idx: usize) -> Self { Self { - #[cfg(feature = "verify_owner")] owner_id: owner.id, index: idx, _item_marker: PhantomData, @@ -104,9 +135,8 @@ enum EntryInner { // Values can be read by anyone but can only be modified using the key. pub struct Slots where N: ArrayLength> + Unsigned, - A: AccessMode { - #[cfg(feature = "verify_owner")] - id: usize, + A: access_mode::AccessMode { + id: A::ObjectId, items: GenericArray, N>, // Could be optimized by making it just usize and relying on free_count to determine its // validity @@ -115,22 +145,14 @@ pub struct Slots _mode_marker: PhantomData } -#[cfg(feature = "verify_owner")] -fn new_instance_id() -> usize { - static COUNTER: AtomicUsize = AtomicUsize::new(0); - - COUNTER.fetch_add(1, Ordering::Relaxed) -} - impl Slots where N: ArrayLength> + Unsigned, - A: AccessMode { + A: access_mode::AccessMode { pub fn new() -> Self { let size = N::to_usize(); Self { - #[cfg(feature = "verify_owner")] - id: new_instance_id(), + id: A::new_obj_id(), items: GenericArray::generate(|i| Entry(i.checked_sub(1).map(EntryInner::EmptyNext).unwrap_or(EntryInner::EmptyLast))), next_free: size.checked_sub(1), free_count: size, @@ -170,15 +192,10 @@ impl Slots impl Slots where N: ArrayLength> + Unsigned { - #[cfg(feature = "verify_owner")] fn verify_key(&self, key: &Key) { assert!(key.owner_id == self.id, "Key used in wrong instance"); } - #[cfg(not(feature = "verify_owner"))] - fn verify_key(&self, _key: &Key) { - } - pub fn store(&mut self, item: IT) -> Result, IT> { match self.alloc() { Some(i) => { diff --git a/tests/test.rs b/tests/test.rs index 10117a4..5a60999 100644 --- a/tests/test.rs +++ b/tests/test.rs @@ -1,5 +1,6 @@ -use slots::{Slots, Strict}; +use slots::{Slots, Strict, Relaxed}; use slots::consts::*; +use core::mem::size_of; #[test] fn key_can_be_used_to_read_value() { @@ -132,13 +133,13 @@ fn is_compact() { b: bool, } - assert_eq!(core::mem::size_of::(), 16); + assert_eq!(size_of::(), 16); - let mut expected_size = 32 * 16 + 3 * core::mem::size_of::(); + let mut expected_size = 32 * 16 + 3 * size_of::(); if cfg!(feature = "verify_owner") { - expected_size += core::mem::size_of::(); // an extra usize for object id + expected_size += size_of::(); // an extra usize for object id } - assert_eq!(core::mem::size_of::>(), expected_size, "Compiled size does not match expected"); + assert_eq!(size_of::>(), expected_size, "Compiled size does not match expected"); } #[test] @@ -165,3 +166,12 @@ fn capacity_and_count() { assert_eq!(slots.count(), 0); } + +#[test] +fn relaxed_instance_does_not_have_object_id() { + if cfg!(feature = "verify_owner") { + assert_eq!(size_of::>(), size_of::>() + size_of::()); + } else { + assert_eq!(size_of::>(), size_of::>()); + } +} From e6caee34a6c2145999acf577d5d8abab4e62d43e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 28 Apr 2020 23:30:32 +0200 Subject: [PATCH 3/6] Actually use the index instead of an impossible key --- src/lib.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 2caeff5..37f4553 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -258,7 +258,7 @@ impl Slots } } - pub fn take(&mut self, key: Key) -> Option { + pub fn take(&mut self, idx: usize) -> Option { let taken = core::mem::replace(&mut self.items[key.index], Entry(EntryInner::EmptyLast)); match taken.0 { EntryInner::Used(item) => { @@ -269,15 +269,15 @@ impl Slots } } - pub fn modify(&mut self, key: &Key, function: F) -> Option where F: FnOnce(&mut IT) -> T { - match self.items[key.index].0 { + pub fn modify(&mut self, idx: usize, function: F) -> Option where F: FnOnce(&mut IT) -> T { + match self.items[idx].0 { EntryInner::Used(ref mut item) => Some(function(item)), _ => None } } - pub fn read(&self, key: usize, function: F) -> Option where F: FnOnce(&IT) -> T { - match &self.items[key].0 { + pub fn read(&self, idx: usize, function: F) -> Option where F: FnOnce(&IT) -> T { + match &self.items[idx].0 { EntryInner::Used(item) => Some(function(&item)), _ => None } From 6901cc6a623de647f2a6a1c0aeebda943016f605 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 28 Apr 2020 23:40:47 +0200 Subject: [PATCH 4/6] Extract common implementations --- src/lib.rs | 101 +++++++++++++++++++++++++---------------------------- 1 file changed, 48 insertions(+), 53 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 37f4553..55ef589 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -187,6 +187,42 @@ impl Slots self.free_count -= 1; Some(index) } + + fn _store(&mut self, item: IT) -> Result { + match self.alloc() { + Some(i) => { + self.items[i] = Entry(EntryInner::Used(item)); + Ok(i) + } + None => Err(item) + } + } + + fn _remove(&mut self, idx: usize) -> Option { + let taken = core::mem::replace(&mut self.items[idx], Entry(EntryInner::EmptyLast)); + + match taken.0 { + EntryInner::Used(item) => { + self.free(idx); + Some(item) + }, + _ => None + } + } + + fn _read(&self, idx: usize, function: F) -> Option where F: FnOnce(&IT) -> T { + match &self.items[idx].0 { + EntryInner::Used(item) => Some(function(&item)), + _ => None + } + } + + fn _modify(&mut self, idx: usize, function: F) -> Option where F: FnOnce(&mut IT) -> T { + match self.items[idx].0 { + EntryInner::Used(ref mut item) => Some(function(item)), + _ => None + } + } } impl Slots @@ -197,51 +233,29 @@ impl Slots } pub fn store(&mut self, item: IT) -> Result, IT> { - match self.alloc() { - Some(i) => { - self.items[i] = Entry(EntryInner::Used(item)); - Ok(Key::new(self, i)) - } - None => Err(item) - } + self._store(item).map(|i| Key::new(self, i)) } pub fn take(&mut self, key: Key) -> IT { self.verify_key(&key); - let taken = core::mem::replace(&mut self.items[key.index], Entry(EntryInner::EmptyLast)); - match taken.0 { - EntryInner::Used(item) => { - self.free(key.index); - item - }, - _ => unreachable!("Invalid key") - } + self._remove(key.index).expect("Invalid key") } pub fn read(&self, key: &Key, function: F) -> T where F: FnOnce(&IT) -> T { self.verify_key(&key); - match self.try_read(key.index, function) { - Some(t) => t, - None => unreachable!("Invalid key") - } + self._read(key.index, function).expect("Invalid key") + } + + pub fn try_read(&self, idx: usize, function: F) -> Option where F: FnOnce(&IT) -> T { + self._read(idx, function) } pub fn modify(&mut self, key: &Key, function: F) -> T where F: FnOnce(&mut IT) -> T { self.verify_key(&key); - match self.items[key.index].0 { - EntryInner::Used(ref mut item) => function(item), - _ => unreachable!("Invalid key") - } - } - - pub fn try_read(&self, key: usize, function: F) -> Option where F: FnOnce(&IT) -> T { - match &self.items[key].0 { - EntryInner::Used(item) => Some(function(&item)), - _ => None - } + self._modify(key.index, function).expect("Invalid key") } } @@ -249,37 +263,18 @@ impl Slots where N: ArrayLength> + Unsigned { pub fn store(&mut self, item: IT) -> Result { - match self.alloc() { - Some(i) => { - self.items[i] = Entry(EntryInner::Used(item)); - Ok(i) - } - None => Err(item) - } + self._store(item) } pub fn take(&mut self, idx: usize) -> Option { - let taken = core::mem::replace(&mut self.items[key.index], Entry(EntryInner::EmptyLast)); - match taken.0 { - EntryInner::Used(item) => { - self.free(key.index); - Some(item) - }, - _ => None - } + self._remove(idx) } pub fn modify(&mut self, idx: usize, function: F) -> Option where F: FnOnce(&mut IT) -> T { - match self.items[idx].0 { - EntryInner::Used(ref mut item) => Some(function(item)), - _ => None - } + self._modify(idx, function) } pub fn read(&self, idx: usize, function: F) -> Option where F: FnOnce(&IT) -> T { - match &self.items[idx].0 { - EntryInner::Used(item) => Some(function(&item)), - _ => None - } + self._read(idx, function) } } From 2bcc0dcd67b1d17e5bc16506e8066c8e89527700 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Tue, 28 Apr 2020 23:44:52 +0200 Subject: [PATCH 5/6] Store element count directly --- src/lib.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 55ef589..77936de 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -138,10 +138,9 @@ pub struct Slots A: access_mode::AccessMode { id: A::ObjectId, items: GenericArray, N>, - // Could be optimized by making it just usize and relying on free_count to determine its - // validity + // Could be optimized by making it just usize and relying on count to determine its validity next_free: Option, - free_count: usize, + count: usize, _mode_marker: PhantomData } @@ -155,7 +154,7 @@ impl Slots id: A::new_obj_id(), items: GenericArray::generate(|i| Entry(i.checked_sub(1).map(EntryInner::EmptyNext).unwrap_or(EntryInner::EmptyLast))), next_free: size.checked_sub(1), - free_count: size, + count: 0, _mode_marker: PhantomData } } @@ -165,7 +164,7 @@ impl Slots } pub fn count(&self) -> usize { - self.capacity() - self.free_count + self.count } fn free(&mut self, idx: usize) { @@ -174,7 +173,7 @@ impl Slots None => Entry(EntryInner::EmptyLast), }; self.next_free = Some(idx); - self.free_count += 1; + self.count -= 1; } fn alloc(&mut self) -> Option { @@ -184,7 +183,7 @@ impl Slots EntryInner::EmptyLast => None, _ => unreachable!("Non-empty item in entry behind free chain"), }; - self.free_count -= 1; + self.count += 1; Some(index) } From 73f2e246f565542ed8c09db198b8db3d3a3dfbae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Wed, 29 Apr 2020 09:33:20 +0200 Subject: [PATCH 6/6] Better change description --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 85409f3..39e4b04 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ Unreleased ========== -* Provide different implementations based on AccessMode template parameter [@bugadani](https://github.com/bugadani) +* Add Relaxed access mode to allow copyable keys and failable access [@bugadani](https://github.com/bugadani) * Verify that keys are used to access data in their associated Slots instances [@bugadani](https://github.com/bugadani) * Allow compiler to reduce memory footprint if possible [@chrysn](https://github.com/chrysn) * Allow using FnOnce closures in `try_read`, `read` and `modify` [@chrysn](https://github.com/chrysn)