Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove memory_id requirement #89

Open
lastmjs opened this issue Jun 4, 2023 · 10 comments
Open

Remove memory_id requirement #89

lastmjs opened this issue Jun 4, 2023 · 10 comments

Comments

@lastmjs
Copy link

lastmjs commented Jun 4, 2023

Requiring a memory id is not desirable, as it adds complexity to the creation of stable structures. I just ran into an issue where a dependency of my application was also using stable structures under-the-hood, and it had decided to use memory_ids 0, 1, and 2. My application of course was also using these memory ids. We had a clash, thus I had to figure this out by looking through source code, and eventually I changed my own application.

Not an ideal situation, and these clashes and issues I imagine will get more painful over time.

@ielashi
Copy link
Contributor

ielashi commented Jun 23, 2023

The problem you're describing sounds more of an architectural problem to me. The memory manager, should one choose to use it, is a global resource. IMHO dependencies shouldn't be using global resources directly, since you'll run into conflicts just like the one you're describing. Instead, a more resilient architecture is for dependencies to ask for resources from the user. In the above example, the dependency should initialize itself with three empty memories, specified by you, so that you can avoid these clashes.

This clash isn't specific to the memory manager. It equally applies to all other global resources, like stable memory and timers.

As for dropping the memory ID requirement, on first thought, I'm not sure how that can be done. When you declare a stable structure, there are two scenarios in which the stable structure is initialized:

  1. The very first time, where a new version of this structure is written into memory.
  2. All the following times (after upgrades), where the structure looks at the given memory, observes that it was already initialized, and loads the data.

When you specify the memory explicitly, as you currently do, it's clear to the stable structure which memory to look at, but if memory ID is somehow implicit, then it's not clear to me how we can distinguish between these two cases. Ideas on how to resolve this problem would be very welcome.

@witter-deland
Copy link
Contributor

I agree with @lastmjs . Using memory_id, for application developers, exposes too many underlying details. Application developers should spend their energy on [how to build a good application] instead of spending more energy on [underlying storage management details]

@ielashi
Copy link
Contributor

ielashi commented Jul 4, 2023

As I mentioned above, there are technical reasons why we haven't been able to do so. We're of course happy to incorporate concrete technical suggestions you have that address the problems I laid out above.

@lastmjs
Copy link
Author

lastmjs commented Oct 19, 2023

As for dropping the memory ID requirement, on first thought, I'm not sure how that can be done. When you declare a stable structure, there are two scenarios in which the stable structure is initialized:

The very first time, where a new version of this structure is written into memory.
All the following times (after upgrades), where the structure looks at the given memory, observes that it was already initialized, and loads the data.
When you specify the memory explicitly, as you currently do, it's clear to the stable structure which memory to look at, but if memory ID is somehow implicit, then it's not clear to me how we can distinguish between these two cases. Ideas on how to resolve this problem would be very welcome.

I will be digging into this, I hope I can come up with some ideas.

@hpeebles
Copy link
Contributor

hpeebles commented Nov 4, 2023

What if we added fn next_available_memory() -> MemoryId? (Might need to return Option<MemoryId>)

Then against each object that you wish to store in stable memory you could have an Option<MemoryId> (this would be on the heap).

If it is None, get the next available MemoryId, else use the existing value.

@lastmjs
Copy link
Author

lastmjs commented Nov 4, 2023

But then the memory id would be based upon the position or order of the stable structures being initialized right? If the dev decided to move a map somewhere else in the code, I could see them easily messing up the order.

@hpeebles
Copy link
Contributor

hpeebles commented Nov 4, 2023

I think libraries that use this crate should simply always take the memoryIds as inputs then everything can be controlled by the devs of the main app.

@ielashi
Copy link
Contributor

ielashi commented Nov 6, 2023

I don't understand the suggestion. Can you share an example? Let's say you have the following code:

thread_local! {
    static MEMORY_MANAGER: RefCell<MemoryManager<DefaultMemoryImpl>> =
        RefCell::new(MemoryManager::init(DefaultMemoryImpl::default()));

    static BTREE_1: RefCell<StableBTreeMap<String, Vec<u8>, Memory>> = RefCell::new(
        StableBTreeMap::init(
            MEMORY_MANAGER.with(|m| m.borrow().get(MemoryId::new(0))),
        )
    );

  static BTREE_2: RefCell<StableBTreeMap<String, Vec<u8>, Memory>> = RefCell::new(
        StableBTreeMap::init(
            MEMORY_MANAGER.with(|m| m.borrow().get(MemoryId::new(1))),
        )
    );
}

If we replace the actual memory IDs with calls to next_available_memory, how would the canister know which memory ID belongs to which stable structure?

@hpeebles
Copy link
Contributor

hpeebles commented Nov 6, 2023

My suggestion was to keep using MemoryId exactly as it is now, but to add the next_available_memory function solely to handle the case where libraries you depend on are themselves using some of the memoryIds.
But I think that is still a bad pattern and that the solution to the original issue is for libraries to never hard code memoryIds.
If libraries hard code the memoryIds then you could have clashes between libraries, or you may be using a memoryId already and then you integrate with the library and it overwrites your data.
So essentially, I think everything is fine as is, and that my comment earlier was just trying to cover up a problem that shouldn't exist in the first place.

@ielashi
Copy link
Contributor

ielashi commented Nov 6, 2023

Agreed, libraries should be passed in memories explicitly and not use specific IDs themselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants