tweedegolf / sequential-storage

A crate for storing data in flash memory with minimal need for erasing pages
Apache License 2.0
87 stars 8 forks source link

Data Format of Map Values can never change #36

Closed t-moe closed 4 months ago

t-moe commented 4 months ago

Hi @diondokter , I realized something today, when using map. I'm using your excellent crate, to store/restore device settings. As key I use a simple u8. Values are byte-arrays serialized/deserialized with postcard.

You have the following code (invoked by fetch_item):

https://github.com/tweedegolf/sequential-storage/blob/6612981f590e690df88f5c2d041e19129d1a323c/src/map.rs#L264-L272

Because you deserialize_from for every item and abort on error, I can never change the data format for a given key.

Example:

I haven't thought about the consequences in detail, but I think we should change the implementation, so that fetch_item only fails, if the last item of the given key can not be deserialized.

Whats your opinion on the topic?

Thank you

diondokter commented 4 months ago

I see your problem. So yeah, this is currently not supported.

It's funny you posted that code snippet because in the name of binary size optimizations I'm looking to change the API in #35. The change would be to separate the key from the item. And the way that that code snippet would work is that it only knows about the key and the item deserialization would be done later.

So that'd sorta already fix your problem you have here.

The way this was thought out is that if you have an updated value, that it should have a new key.

So, question for you.

What would you rather see?

All of these have their pros and cons... Hard to choose

t-moe commented 4 months ago

What would you rather see?

  • store_item(key, item) - fetch_item(key)

  • store_item(item) - fetch_item(key)

    • Key is used separately, but the item knows about its own key (this is the current API)
  • store_item(item) - fetch_item::<ItemType>

    • The key is never used separately and is a const on the Item type

All of these have their pros and cons... Hard to choose

Currently my items always have a key which is a const on the item type. But not allowing runtime keys might be a dealbreaker for others. So I think fully separating the key from the item (->Value?) is the best way to go. I think this is also the API people are expecting when coming from something like std::collections::HashMap.

diondokter commented 4 months ago

Yes, I agree with your reasoning. But what do you think about the item type? With a normal hashmap you can't store multiple types in it.

I could also forego item types fully by making the API only take byte slices. What you want to do with them is then up to you. Same as with queue which is also only byte slices.

t-moe commented 4 months ago

With a normal hashmap you can't store multiple types in it.

Ah yes. You're right. Well, I need multiple types. But I dont care about having an additional item type. It is somehow pointless if the key is no longer part of it. store_item(key, slice) would work nicely for me.

diondokter commented 4 months ago

Ok, #35 implements a new map interface that explicitly allows and facilitates using multiple types (or even &[u8] if you want). Keys and values are separated now and the value is only deserialized at the very end.

t-moe commented 3 months ago

Cool, thanks a lot for fixing this. I will take a look in the coming weeks and report back.

t-moe commented 3 months ago

I've tested the new master branch and I can now update my data format without erasing the flash. Thank you!