Binary size optimizations and map interface changes

CHANGELOG.md

@@ -4,6 +4,13 @@
 
 ## Unreleased
 
+- *Breaking:* Made the cache API a bit more strict. Caches now always have to be passed as a mutable reference.
+  The API before would lead to a lot of extra unncesessary binary size.
+- *Breaking:* Removed the `StorageItem` trait in favor of two separate `Key` and `Value` traits. This helps cut
+  binary size and is closer to what users of the map APIs were expecting.
+- *Breaking:* The error type is no longer generic over the Item error. That error variant has been renamed `MapValueError`
+  and carries a predefined error subtype.
+
 ## 1.0.0 01-03-24
 
 - *Breaking:* Corruption repair is automatic now! The repair functions have been made private.

Cargo.toml

@@ -1,6 +1,6 @@
 [package]
 name = "sequential-storage"
-version = "1.0.0"
+version = "2.0.0"
 edition = "2021"
 license = "MIT OR Apache-2.0"
 description = "A crate for storing data in flash with minimal erase cycles."

README.md

@@ -25,8 +25,8 @@ The in-flash representation is not (yet?) stable. This too follows semver.
 
 - A breaking change to the in-flash representation will lead to a major version bump
 - A feature addition will lead to a minor version bump
-  - This is always backward-compatible. So data created by e.g. `0.8.0` can be used by `0.8.1`.
-  - This may not be forward-compatible. So data created by e.g. `0.8.1` may not be usable by `0.8.0`.
+  - This is always backward-compatible. So data created by e.g. `1.0.0` can be used by `1.1.0`.
+  - This may not be forward-compatible. So data created by e.g. `1.0.1` may not be usable by `1.0.0`.
 - After 1.0, patch releases only fix bugs and don't change the in-flash representation
 
 For any update, consult the changelog to see what changed. Any externally noticable changes are recorded there.
@@ -88,8 +88,8 @@ These numbers are taken from the test cases in the cache module:
 | ---------------: | -------------------------------------------: | ----------------: | -------------------: | ------------------: | ---------------------: |
 |          NoCache |                                            0 |              100% |                 100% |                100% |                   100% |
 |   PageStateCache |                                1 * num pages |               77% |                  97% |                 51% |                    90% |
-| PagePointerCache |                                9 * num pages |               69% |                  89% |                 35% |                    61% |
-|  KeyPointerCache | 9 * num pages + (sizeof(KEY) + 4) * num keys |              6.5% |                 8.5% |                   - |                      - |
+| PagePointerCache |                                9 * num pages |               70% |                  89% |                 35% |                    61% |
+|  KeyPointerCache | 9 * num pages + (sizeof(KEY) + 4) * num keys |              6.2% |                 8.2% |                   - |                      - |
 
 #### Takeaways
 

fuzz/fuzz_targets/map.rs

@@ -6,7 +6,6 @@ use libfuzzer_sys::fuzz_target;
 use rand::SeedableRng;
 use sequential_storage::{
     cache::{KeyCacheImpl, KeyPointerCache, NoCache, PagePointerCache, PageStateCache},
-    map::StorageItem,
     mock_flash::{MockFlashBase, MockFlashError, WriteCountCheck},
     Error,
 };
@@ -45,57 +44,13 @@ struct StoreOp {
 }
 
 impl StoreOp {
-    fn into_test_item(self, rng: &mut impl rand::Rng) -> TestItem {
-        TestItem {
-            key: self.key,
-            value: (0..(self.value_len % 8) as usize)
+    fn into_test_item(self, rng: &mut impl rand::Rng) -> (u8, Vec<u8>) {
+        (
+            self.key,
+            (0..(self.value_len % 8) as usize)
                 .map(|_| rng.gen())
                 .collect(),
-        }
-    }
-}
-
-#[derive(Debug, Clone, PartialEq, Eq)]
-struct TestItem {
-    key: u8,
-    value: Vec<u8>,
-}
-
-impl StorageItem for TestItem {
-    type Key = u8;
-
-    type Error = ();
-
-    fn serialize_into(&self, buffer: &mut [u8]) -> Result<usize, Self::Error> {
-        if buffer.len() < 1 + self.value.len() {
-            return Err(());
-        }
-
-        buffer[0] = self.key;
-        buffer[1..][..self.value.len()].copy_from_slice(&self.value);
-
-        Ok(1 + self.value.len())
-    }
-
-    fn deserialize_from(buffer: &[u8]) -> Result<Self, Self::Error>
-    where
-        Self: Sized,
-    {
-        Ok(Self {
-            key: buffer[0],
-            value: buffer[1..].to_vec(),
-        })
-    }
-
-    fn key(&self) -> Self::Key {
-        self.key
-    }
-
-    fn deserialize_key_only(buffer: &[u8]) -> Result<Self::Key, Self::Error>
-    where
-        Self: Sized,
-    {
-        Ok(buffer[0])
+        )
     }
 }
 
@@ -139,36 +94,35 @@ fn fuzz(ops: Input, mut cache: impl KeyCacheImpl<u8> + Debug) {
 
         match op.clone() {
             Op::Store(op) => {
-                let item = op.into_test_item(&mut rng);
+                let (key, value) = op.into_test_item(&mut rng);
                 match block_on(sequential_storage::map::store_item(
                     &mut flash,
                     FLASH_RANGE,
                     &mut cache,
                     &mut buf.0,
-                    &item,
+                    key,
+                    &value.as_slice(),
                 )) {
                     Ok(_) => {
-                        map.insert(item.key, item.value);
+                        map.insert(key, value);
                     }
                     Err(Error::FullStorage) => {}
                     Err(Error::Storage {
                         value: MockFlashError::EarlyShutoff(_),
                         backtrace: _backtrace,
                     }) => {
-                        match block_on(sequential_storage::map::fetch_item::<TestItem, _>(
+                        match block_on(sequential_storage::map::fetch_item::<u8, &[u8], _>(
                             &mut flash,
                             FLASH_RANGE,
                             &mut cache,
                             &mut buf.0,
-                            item.key,
+                            key,
                         )) {
-                            Ok(Some(check_item))
-                                if check_item.key == item.key && check_item.value == item.value =>
-                            {
+                            Ok(Some(check_item)) if check_item == value => {
                                 #[cfg(fuzzing_repro)]
                                 eprintln!("Early shutoff when storing {item:?}! (but it still stored fully). Originated from:\n{_backtrace:#}");
                                 // Even though we got a shutoff, it still managed to store well
-                                map.insert(item.key, item.value);
+                                map.insert(key, value);
                             }
                             _ => {
                                 // Could not fetch the item we stored...
@@ -188,7 +142,7 @@ fn fuzz(ops: Input, mut cache: impl KeyCacheImpl<u8> + Debug) {
                 }
             }
             Op::Fetch(key) => {
-                match block_on(sequential_storage::map::fetch_item::<TestItem, _>(
+                match block_on(sequential_storage::map::fetch_item::<u8, &[u8], _>(
                     &mut flash,
                     FLASH_RANGE,
                     &mut cache,
@@ -199,8 +153,7 @@ fn fuzz(ops: Input, mut cache: impl KeyCacheImpl<u8> + Debug) {
                         let map_value = map
                             .get(&key)
                             .expect(&format!("Map doesn't contain: {fetch_result:?}"));
-                        assert_eq!(key, fetch_result.key, "Mismatching keys");
-                        assert_eq!(map_value, &fetch_result.value, "Mismatching values");
+                        assert_eq!(map_value, &fetch_result, "Mismatching values");
                     }
                     Ok(None) => {
                         assert_eq!(None, map.get(&key));
@@ -223,7 +176,7 @@ fn fuzz(ops: Input, mut cache: impl KeyCacheImpl<u8> + Debug) {
                 }
             }
             Op::Remove(key) => {
-                match block_on(sequential_storage::map::remove_item::<TestItem, _>(
+                match block_on(sequential_storage::map::remove_item::<u8, _>(
                     &mut flash,
                     FLASH_RANGE,
                     &mut cache,
@@ -237,7 +190,7 @@ fn fuzz(ops: Input, mut cache: impl KeyCacheImpl<u8> + Debug) {
                         value: MockFlashError::EarlyShutoff(_),
                         backtrace: _backtrace,
                     }) => {
-                        match block_on(sequential_storage::map::fetch_item::<TestItem, _>(
+                        match block_on(sequential_storage::map::fetch_item::<u8, &[u8], _>(
                             &mut flash,
                             FLASH_RANGE,
                             &mut cache,

src/cache/mod.rs

@@ -23,11 +23,10 @@ pub(crate) use page_states::PageStatesCache;
 /// Trait implemented by all cache types
 #[allow(private_bounds)]
 pub trait CacheImpl: PrivateCacheImpl {}
-impl<T: CacheImpl> CacheImpl for &mut T {}
+
 /// Trait implemented by all cache types that know about keys
 #[allow(private_bounds)]
 pub trait KeyCacheImpl<KEY: Eq>: CacheImpl + PrivateKeyCacheImpl<KEY> {}
-impl<KEY: Eq, T: KeyCacheImpl<KEY>> KeyCacheImpl<KEY> for &mut T {}
 
 pub(crate) trait Invalidate {
     fn invalidate_cache_state(&mut self);
@@ -216,6 +215,12 @@ impl NoCache {
     }
 }
 
+impl Default for NoCache {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
 impl PrivateCacheImpl for NoCache {
     type PSC = UncachedPageStates;
     type PPC = UncachedPagePointers;
@@ -280,6 +285,12 @@ impl<const PAGE_COUNT: usize> PageStateCache<PAGE_COUNT> {
     }
 }
 
+impl<const PAGE_COUNT: usize> Default for PageStateCache<PAGE_COUNT> {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
 impl<const PAGE_COUNT: usize> PrivateCacheImpl for PageStateCache<PAGE_COUNT> {
     type PSC = CachedPageStates<PAGE_COUNT>;
     type PPC = UncachedPagePointers;
@@ -347,6 +358,12 @@ impl<const PAGE_COUNT: usize> PagePointerCache<PAGE_COUNT> {
     }
 }
 
+impl<const PAGE_COUNT: usize> Default for PagePointerCache<PAGE_COUNT> {
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
 impl<const PAGE_COUNT: usize> PrivateCacheImpl for PagePointerCache<PAGE_COUNT> {
     type PSC = CachedPageStates<PAGE_COUNT>;
     type PPC = CachedPagePointers<PAGE_COUNT>;
@@ -419,6 +436,14 @@ impl<const PAGE_COUNT: usize, KEY: Eq, const KEYS: usize> KeyPointerCache<PAGE_C
     }
 }
 
+impl<const PAGE_COUNT: usize, KEY: Eq, const KEYS: usize> Default
+    for KeyPointerCache<PAGE_COUNT, KEY, KEYS>
+{
+    fn default() -> Self {
+        Self::new()
+    }
+}
+
 impl<const PAGE_COUNT: usize, KEY: Eq, const KEYS: usize> PrivateCacheImpl
     for KeyPointerCache<PAGE_COUNT, KEY, KEYS>
 {