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

Duplication of entire / partial cache object in MMKV storage #500

Open
vaibhav-toddleapp opened this issue Sep 20, 2023 · 7 comments
Open

Comments

@vaibhav-toddleapp
Copy link

vaibhav-toddleapp commented Sep 20, 2023

In case of MMKV storage, it doesn’t check for key duplication, and just appends the key-value pair even if key is duplicated. So, our cache object will be saved with duplicate data every time cache object gets updated ( in 'write' trigger ).

For example, lets say first query named getUser is called and persisted in MMKV storage then internally file will look like this:
key_given_in_persist_constructor :: { ROOT_QUERY: { getUser: {......} } }

further on lets say we call another query getUserTasks then now file will look like this:
key_given_in_persist_constructor :: { ROOT_QUERY: { getUser: {......} } }
key_given_in_persist_constructor :: { ROOT_QUERY: { getUser: {......}, getUserTasks: {.......} } }

ignore cache object structure, it is for understanding purpose*

Correct me if i am wrong/misunderstood something.

And if it is correct then we should mention this in documentation and if possible alter some behaviour as well.

@wodCZ
Copy link
Collaborator

wodCZ commented Sep 20, 2023

Is this for MMKVStorageWrapper or MMKVWrapper?

In both cases, wrappers use set/setItem, which should override any existing value instead of adding a new duplicate key. If you're experiencing a different behaviour, please provide a minimal reproduction so I can look into it.

@vaibhav-toddleapp
Copy link
Author

vaibhav-toddleapp commented Sep 20, 2023

Yes, i am using MMKVStorageWrapper

It seems like it is default behaviour of MMKV.
see this issue has one comment: ammarahm-ed/react-native-mmkv-storage#286, comment: ammarahm-ed/react-native-mmkv-storage#286 (comment)

And here is the comment from official tencent/mmkv: Tencent/MMKV#617 (comment)

Here the snippet of how i have used it:

export const cachePersistor = new CachePersistor({
  cache: myInMemcache,
  trigger: "write",
  storage: new MMKVStorageWrapper(MMKVCacheStorageInstance),
  debug: true,
  maxSize: false,
  key: "user_todo_app_cache"
});

cachePersistor.restore(); 

Also note that multiple queries will be called at a time.

@wodCZ
Copy link
Collaborator

wodCZ commented Sep 20, 2023

I wasn't aware of that. But since it's just implementation detail, it doesn't affect the functionality of cache persistor.

Feel free to send a PR updating the docs, preferably adding ** to the providers table next to mmkv wrappers (similar to AsyncStorage caveat), noting that mmkv wrappers, while performant, have bigger storage usage demands. Links to the issues would be great too.

Thank you!

@vaibhav-toddleapp
Copy link
Author

yes, but this is kind of crucial when application is complex and graphql queries are huge in terms of data. it will grow drastically and fills mobile storage heavily.

@wodCZ
Copy link
Collaborator

wodCZ commented Sep 20, 2023

I understand your concern, but there's nothing we can do about it on our side (other than warning about this this in the docs). Like mentioned in the linked issues, it's the trade-off for performance.

If that's a deal-breaker for your use-case, either use a different storage, or use any means the mmkv storage gives you to perform garbage-collection, or periodic full clears.

We're using this exact combination in production for years and never got a report of excessive storage usage, even though we have quite heavy queries.

@vaibhav-toddleapp
Copy link
Author

ohh! that's satisfying 🙂.
Do you guys use any particular approach for cleanup in your implementation?

@wodCZ
Copy link
Collaborator

wodCZ commented Sep 20, 2023

Not really, we only clear the storage on logout, no other mechanism was needed. I'm guessing the mmkv integrations or mmkv itself has some kind of GC built in, else we'd hear about the problem by now.

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

2 participants