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

MMKV storage file grows until app crash #440

Closed
ryskin opened this issue Aug 5, 2022 · 69 comments
Closed

MMKV storage file grows until app crash #440

ryskin opened this issue Aug 5, 2022 · 69 comments

Comments

@ryskin
Copy link

ryskin commented Aug 5, 2022

The library is great but one crucial issue doesn't give me chance to use it.

And it grows by stop 64Mb, 128Mb etc.
If I use slow FS storage, the file size never becomes more than 15Mb. I also used the console log to check the size of the string.

It's working really fast and I want to use that, but with every rewriting, it becomes more significant. I checked key is always stable.

image

"react-native-mmkv": "^2.4.3",
"react-native": "0.66.3",

@mrousavy
Copy link
Owner

mrousavy commented Aug 5, 2022

How often are you writing to MMKV? It's not a database, it's a memory map key value storage.

@ryskin
Copy link
Author

ryskin commented Aug 10, 2022

How often are you writing to MMKV? It's not a database, it's a memory map key value storage.

I'm writing when my state change with a few seconds of throttling. I use it with redux-persist.
But why does the size of the file grow? As I mentioned before, I use FS storage, which is significantly slower, but the file size increases up to 15Mb. I use only one key.

@ramirobg94
Copy link

ramirobg94 commented Sep 9, 2022

i have the same "bug"

Check the explanation here ammarahm-ed/react-native-mmkv-storage#286

Seems that the Key Value doesn't overwrite the value, just add a new row into the storage. @mrousavy

Please @ryskin let me know if you solved the problem

@mark-careplanner
Copy link

Any further thoughts on this? We are seeing a similar issue

@mrousavy
Copy link
Owner

mrousavy commented Oct 7, 2022

Possibly related? #397

@aLemonFox
Copy link

@mrousavy to clear things up, does mmkv insert a new row for every setter, without overwriting the previous value? If so, how can we properly clear 'old' entries?

@frozencap
Copy link

@aLemonFox have you tried removing the previous entry before setting the new one?

@JacobFJ
Copy link

JacobFJ commented Jan 29, 2023

@mrousavy Confirming this contributes to my app crashes. The memory is hogging in Android 12 Go edition which typically 2GB or less ram.

@ramirobg94
Copy link

ramirobg94 commented Feb 1, 2023

@shawarmaz this is dangerous because you can get into two cases:

1 -> async delete, then async write -> maybe write is too slow so there is no advantage to using mmkv
2 -> race condition -> The new data can be deleted

I see the point on how this memory is designed but should have some cleaning method to reduce the memory consumption, there is no point in keeping dozens of versions (Is there any method to recover previous versions? I think that no, so it is pointless)

@frozencap
Copy link

@ramirobg94 neither of those cases apply because the calls are sync. To be clear that means not async.

@mrousavy
Copy link
Owner

mrousavy commented Feb 1, 2023

This is apparently how Tencent/MMKV is designed.

@arthurgeron-work
Copy link

arthurgeron-work commented Feb 7, 2023

Checking if it exists and deleting before set seems to have worked; I've used patch-package, but I can open a pr with the fix:

diff --git a/node_modules/react-native-mmkv/lib/module/MMKV.js b/node_modules/react-native-mmkv/lib/module/MMKV.js
index 8856aea..9cbe764 100644
--- a/node_modules/react-native-mmkv/lib/module/MMKV.js
+++ b/node_modules/react-native-mmkv/lib/module/MMKV.js
@@ -45,6 +45,12 @@ export class MMKV {
   }
   set(key, value) {
     const func = this.getFunctionFromCache('set');
+    const contains = this.getFunctionFromCache('contains');
+    if (key && contains(key)) {
+      const _delete = this.getFunctionFromCache('delete');
+      _delete(key);
+    }
     func(key, value);
     this.onValuesChanged([key]);
   }

You could also just leave the delete call there before set to avoid increasing the time this function takes to finish:

diff --git a/node_modules/react-native-mmkv/lib/module/MMKV.js b/node_modules/react-native-mmkv/lib/module/MMKV.js
index 8856aea..11b43da 100644
--- a/node_modules/react-native-mmkv/lib/module/MMKV.js
+++ b/node_modules/react-native-mmkv/lib/module/MMKV.js
@@ -45,6 +45,10 @@ export class MMKV {
   }
   set(key, value) {
     const func = this.getFunctionFromCache('set');
+    if (key) {
+      const _delete = this.getFunctionFromCache('delete');
+      _delete(key);
+    }
     func(key, value);
     this.onValuesChanged([key]);
   }

@frozencap
Copy link

I don’t think this should be merged, maybe just a doc update. You can just wrap the method yourself.

@arthurgeron-work
Copy link

I don't think this should be left as is, having it fill up memory is a serious issue, having it go a little slower is much better than having it crash your app.

@frozencap
Copy link

Again, this is how mmkv is designed. You can wrap the method yourself. Wrapping it lib-level would give new non-mmkv semantics to the lib. To keep it 💯 mmkv, the actual solution to this issue is mmkv’s trim method.

@mrousavy , what was the issue with this PR?

#461

@arthurgeron-work
Copy link

I don't know how not managing space allocation could be by design? Managing memory/space allocation is the libs responsibility, otherwise it's a insta leak to unaware devs. There should be a big warning in the readme, a temporary fix like that can be given but the way data is stored needs to be changed to fix the leak, or at least trim should be exposed and it could be called in specific situations, even though this delegates responsibility to devs and they can't really tell when trim should be called because they can't see how much space has been allocated.

@frozencap
Copy link

Read this Tencent/MMKV#610

@arthurgeron-work

This comment was marked as spam.

@ryskin
Copy link
Author

ryskin commented Feb 9, 2023

I don't know, I started to use other MMKV libraries and problem disappear, file is small and works like charm

@codering
Copy link

codering commented Feb 9, 2023

I don't know, I started to use other MMKV libraries and problem disappear, file is small and works like charm

which lib?

@ryskin
Copy link
Author

ryskin commented Feb 9, 2023

I'm thinking not good to post links to other libraries here, but you can google only 2 main libraries available for react-native

@arthurgeron
Copy link

arthurgeron commented Feb 9, 2023

I don't know, I started to use other MMKV libraries and problem disappear, file is small and works like charm

which lib?

react-native-mmkv-storage works just fine

@mrousavy
Copy link
Owner

mrousavy commented Feb 9, 2023

@mrousavy , what was the issue with this PR? #461

@shawarmaz I found out that printing the total size after doing a trim made no difference, so I wasn't able to reproduce your error. I could take another look, but we wanna find the right solution here without sacrificing speed :)

Can you guys create a reproduceable example? Is this on Android only, or iOS too?

@frozencap
Copy link

Personally I just wrapped writes with preprended deletes and called it a day. No noticeable difference in write performance.

If you're testing against high volume of writes or high write size, you're not using the right tool for the job. The official Tencent/MMKV repo benchmarks it against NSUserDefaults/SharedUserPreferences which tells you all you need to know about its intended usage i.e. not as a local runtime database.

If you have a relatively small collection of key-values that you'd like to save, you should use the SharedPreferences APIs.

The NSUserDefaults class provides a programmatic interface for interacting with the defaults system. The defaults system allows an app to customize its behavior to match a user’s preferences.

IMO the main selling point of MMKV is not speed but having a unified API for use cases that would have otherwise required NSUserDefaults/SharedUserPreferences.

If you want to store application data like chat messages or POI geopoints etc, use the right tool for the job i.e. something else. Even if trim was available everybody would be like wtf why do I need to do this. It's counterintuitive because you think MMKV is something that it is not.

This may be a case of false advertisement around MMKV at large. Thoughts welcome

@aLemonFox
Copy link

@shawarmaz I think you're right. I'm struggling with this too.
Are there any other good reactive key value store libraries available which are not based on this? I can't seem to find any. They are either like AsyncStorage (async, not reactive) or WatermelonDB (full db, want key value store like Redis).

@frozencap
Copy link

frozencap commented Feb 9, 2023

@aLemonFox just use this lib and wrap writes with a preprended delete

  setItem: (key, value) => {
    storage.delete(key)
    storage.set(key, value);
  },

...or just unsexy AsyncStorage whose writes do overwrite and that also does not load entire data from cold storage in memory, unexpectedly

Which in fact also illustrates that comparisons/benchmarks between AsyncStorage and MMKV are inherently false/misleading.

@frozencap
Copy link

@aLemonFox Btw mmm MMKV is not reactive

@arthurgeron-work
Copy link

@aLemonFox Btw mmm MMKV is not reactive

Personally I just wrapped writes with preprended deletes and called it a day. No noticeable difference in write performance.

If you're testing against high volume of writes or high write size, you're not using the right tool for the job. The official Tencent/MMKV repo benchmarks it against NSUserDefaults/SharedUserPreferences which tells you all you need to know about its intended usage i.e. not as a local runtime database.

If you have a relatively small collection of key-values that you'd like to save, you should use the SharedPreferences APIs.

The NSUserDefaults class provides a programmatic interface for interacting with the defaults system. The defaults system allows an app to customize its behavior to match a user’s preferences.

IMO the main selling point of MMKV is not speed but having a unified API for use cases that would have otherwise required NSUserDefaults/SharedUserPreferences.

If you want to store application data like chat messages or POI geopoints etc, use the right tool for the job i.e. something else. Even if trim was available everybody would be like wtf why do I need to do this. It's counterintuitive because you think MMKV is something that it is not.

This may be a case of false advertisement around MMKV at large. Thoughts welcome

It's not only falsely advertised, it's broken by design. Moving from async to mmkv is like switching to a bmw but that suddenly crashes into a lamp post if you push it too hard.

Anyway, we need to test to be sure that prepending delete to write calls does actually fix the leak.

@arthurgeron-work
Copy link

@aLemonFox Btw mmm MMKV is not reactive

I think local storage should not be reactive, for that it's better to actually use something like Zustand or Redux and persist that data on specific moments, either with a specific solution or something like mmkv

@mrousavy
Copy link
Owner

mrousavy commented Feb 16, 2023

Okay since no one else did it, I created a minimal reproduceable example:

const storage = new MMKV()

for (let i = 0; true; i++) {
  console.log(`Writing "test":"Some Value in here." to MMKV for the ${i}th time.`)
  storage.set('test', 'Some Value in here.')
}

This runs for a while. After writing for the 1.270.708th time, memory size reaches 500MB. So writing 1 million times to the same key makes it grow to 500MB of memory.

Yes, that sounds like an issue. But this is a core MMKV issue, apparently they don't truncate the file after a few writes. I think this is due to DEFAULT_MMAP_SIZE being very large. I'll investigate this if I have some free time.

Since this is an issue with the core MMKV C++ library, this is also an issue with the other rn mmkv library (react-native-mmkv-storage).

I'm wondering if this is only happening in simulator due to more RAM being available? IDK what DEFAULT_MMAP_SIZE / pagesize is

@harjotmalhi94
Copy link

Thanks for testing the limits @mrousavy

I am attempting to patch version 2.5.1 with the same delete you did inside set here: https://github.com/mrousavy/react-native-mmkv/pull/509/files

But it still results in the old entries staying there and new ones being added. Any idea why the same patch in the older version does not work? Even prepending the set with a delete does not fix the issue:

setItem: (key, value) => {
    storage.delete(key)
    storage.set(key, value);
  }

@frozencap
Copy link

frozencap commented Feb 16, 2023

Here's what the main developer of Tencent/MMKV at WeChat has to say:

MMKV won't increase to an unacceptable big size under normal usage. Before each growth, MMKV will always try to pack existing data to see if there's enough space for new commer.
However, if there's an unreasonable big value stored inside once, and if there's not enough space for that big value, MMKV might grow to an unexpected large size.

Tencent/MMKV#610 (comment)

As I mentioned earlier, WeChat benchmarked MMKV against NSUserDefaults and SharedPreferences. Everybody should be referred to those to understand what MMKV is intended to be used for, high up in the docs. IMO this is the only part that needed to be fixed, otherwise people will keep trying to use MMKV as a database.

IMO fastWrites was a faux-pas, the naming implies one way to use it is for high volume of writes. However, the very fact WeChat ran benchmarks on something built for use cases that are not performance sensitive (storing auth token, color preferences, locale) is most likely the source of all this confusion.

And please don't fool yourselves, WeChat does not use MMKV as its database. It developed and uses a database for that: https://github.com/Tencent/wcdb

The whole point is if you even start hitting these issues in the first place with MMKV, chances are you're using it wrong.

anurag-alla pushed a commit to anurag-alla/react-native-mmkv that referenced this issue Feb 28, 2023
Adds a `fastWrites` property to the MMKV constructor.

* When `true`, MMKV does not overwrite previous values when calling `set`. This is faster, but uses more memory.
* When `false`, MMKV deletes the previous value before calling `set`. This makes sure the storage size is always as small as possible, with the added performance cost of one delete call before setting. 

Before this PR, MMKV behaved as if `fastWrites` was set to `true` (i.e.; fast, but use more memory). Now after this PR, `fastWrites` is `false` by default, because users were complaining in mrousavy#440 that the storage size grows too big.
@matt-dalton
Copy link

matt-dalton commented Mar 31, 2023

It would be great to have some guidance if possible on when we should consider MMKV (assuming we don't have knowledge of the underlying native storage systems mentioned).
If we're happy with slower writes (which from the benchmarking I've seen would still be quicker than async storage), are there any other issues with using MMKV as a direct swap for async storage? What kind of use-cases would be workable/unworkable?

e.g.

  • Persisting local state (e.g. larger JSON structures)
  • Using MMKV as an app cache (i.e. persisting data required to load screens, via something like Redux persist)
    I would have thought you'd still see performance improvements in those cases using this over Async storage, but could be wrong

At what point does async storage become the right choice? Any specific limits we should watch for?

@billnbell
Copy link

Async Storage is limited size on IOS/Android. So that is why we even considered using MMKV and this library.

So what is the solution ?

@frozencap
Copy link

@matt-dalton @billnbell

Do not use MMKV nor AsyncStorage as a db.

First off, they serve different use cases, and claiming it's faster than AsyncStorage fuels ongoing confusion. It's like saying a plane is faster than the car. There are numerous threads about this.

MMKV is designed for small data: color, locale, maybe tokens. AsyncStorage is good for larger, but not db/sqlite-scale data.

If you want to store large amounts of data, for the love of God, use a ... database.

Margelo (mrousavy's dev shop) just took over react-native-quick-sqlite, which is what should be used for larger data. So imo it won't be long before they clear out in the README this generalized lingering confusion of when to use react-native-mmkv

@billnbell
Copy link

I am just using this for caching. And things like temp tokens.

So it is not a lot of data.

Seems like I am good then.

@matt-dalton
Copy link

Yeah likewise. We're mainly storing simple settings flags etc.

What's your view on using MMKV for cached data @shawarmaz, assuming we turn fastWrites off? The chunks of data can be fairly large (because they represent the screen's state), but:

  • They are cleared from the cache fairly quickly
  • We don't care about persisting indefinitely
    This seems like a use-case that would be ideal for MMKV from a performance perspective vs Async Storage. Is storage size the main danger here? It sounds like that only becomes a problem for enormous data sets from what I can see in this thread.

@mrousavy
Copy link
Owner

mrousavy commented May 2, 2023

@shawarmaz is right here, do not use MMKV as a database, it is doing lots of in-memory magic to make it fast, which means storing large amounts of data isn't really a good idea.

Temp tokens and stuff like that sounds like a good fit for MMKV, but I suggest you just try it out and if it uses too much memory switch to a database :)

@billnbell
Copy link

What is the actual size limit for a key - value pair?

@matt-dalton
Copy link

Out of interest, what would you consider the best React Native choice for an app cache? By that I mean storing server data with a TTL, which could be reasonably large JSON-style objects for each screen, but nothing enormous.

I don't think async storage is a great choice here, because you're sending data across the bridge every time you restore or persist server data. But it sounds like MMKV is only a borderline fit. Do you think there's a better option? What do people typically use for native apps?

(Sorry if this is slightly stretching the scope of this discussion)

@evelant
Copy link

evelant commented Jun 10, 2023

@matt-dalton I'm also interested.

I feel the current react-native-mmkv docs might be a bit misleading if mmkv is indeed not suitable for things like offline caching of small amounts of user data. I came to mmkv because I wanted a simple sync way to cache requests so my users can load the app offline. It seemed like a perfect alterantive to async storage for the job based on the readme/docs. After reading this thread I'm a bit confused however since it seems it is not intended for this use case.

If there are good alternatives perhaps they can be mentioned in the readme along with a clarification about what mmkv is for? If I'm understanding this discussion correctly perhaps something along the lines of

"MMKV is only designed to store small bits of state such as user preferences (under 1kb, infrequently written). If you're looking to cache requests or larger user data check out ----insert alternative here----"

added to the readme could be useful in reducing future confusion.

@mrousavy
Copy link
Owner

Well tbh this is just purely your decision/needs for the app.

You want something sync? It has to be fast, so there are compromises - i.e. in-memory caching. -> react-native-mmkv

You want something async? It can be a bit slower, i.e. optimized databases like SQL. -> react-native-quick-sqlite

@VirtualDOMinic
Copy link

Okay since no one else did it, I created a minimal reproduceable example:

const storage = new MMKV()

for (let i = 0; true; i++) {
  console.log(`Writing "test":"Some Value in here." to MMKV for the ${i}th time.`)
  storage.set('test', 'Some Value in here.')
}

This runs for a while. After writing for the 1.270.708th time, memory size reaches 500MB. So writing 1 million times to the same key makes it grow to 500MB of memory.

Yes, that sounds like an issue. But this is a core MMKV issue, apparently they don't truncate the file after a few writes. I think this is due to DEFAULT_MMAP_SIZE being very large. I'll investigate this if I have some free time.

Since this is an issue with the core MMKV C++ library, this is also an issue with the other rn mmkv library (react-native-mmkv-storage).

I'm wondering if this is only happening in simulator due to more RAM being available? IDK what DEFAULT_MMAP_SIZE / pagesize is

Hey @mrousavy , thanks for the library! Also enjoyed your RN Radio appearance recently.

Was the above error reproduction done with or without the fastWrites feature in? I see it was done between the dates of the merge and reversal of that feature/commit. Am I correct in assuming that the benchmark was done WITH that feature added and that the outcome was that deleting the item from storage before set (i.e. fastWrites: false) didn't actually fix the issue?

If the above is true, is there any known custom function/patch that does solve this problem?

@mrousavy
Copy link
Owner

Hey - thanks! The podcast was really cool.

As far as I know the problem should be fixed or is not an actual problem - can anyone re-test this with the latest react-native-mmkv version and confirm?

@VirtualDOMinic
Copy link

Perhaps a stupid question, but what tool/code were you (@mrousavy / @ryskin ) using to get the size of the storage?

I'm happy to give Marc's minimum reproducible example a go on an older version and again on the latest version (pending an upgrade of my app to RN 71) but want to be sure I'm measuring it correctly.

Thanks!

@mrousavy
Copy link
Owner

Hey! Closing for now, if this issue still persists I'll re-open and take another look - but we need a reproduceable example app for that! (Make sure to test in release builds btw)

Thanks everyone :)

BamMironov added a commit to ChildMindInstitute/mindlogger-app-refactor that referenced this issue Oct 25, 2023
… MMKV storage

Apparently, this is how MMKV has been designed. Please see the issue and workarounds (one of them was implemented in this commit): mrousavy/react-native-mmkv#440)
@MaxToyberman
Copy link

@mrousavy should the issue be fixed in 2.12.2 ?

I wanted to use it for my offline mode but i guess that i can't (the object is not small)

Thanks

@mrousavy
Copy link
Owner

fixed in V3. Not in the old V2.

@MaxToyberman
Copy link

Thanks @mrousavy , I am using the old architecture because it is in beta so i cant use V3 currently.

is there a workaround for this ?

@mrousavy
Copy link
Owner

I am using the old architecture because it is in beta

It's not in beta. In fact, new architecture is enabled by default in react-native 0.76.

is there a workaround for this ?

Yes, upgrade to new arch & V3 😄
I don't plan on fixing this for V2 unless it is funded / sponsored.

@MaxToyberman
Copy link

Ok so they have to updated their docs !

Thank you @mrousavy

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