mrousavy / react-native-mmkv

⚡️ The fastest key/value storage for React Native. ~30x faster than AsyncStorage!
https://mrousavy.com
MIT License
6.15k stars 258 forks source link

MMKV storage file grows until app crash #440

Closed ryskin closed 1 year ago

ryskin commented 2 years ago

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",

matt-dalton commented 1 year ago

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.

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

billnbell commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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:

mrousavy commented 1 year ago

@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 commented 1 year ago

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

matt-dalton commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 :)

MaxToyberman commented 1 month ago

@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 commented 1 month ago

fixed in V3. Not in the old V2.

MaxToyberman commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

Ok so they have to updated their docs !

Thank you @mrousavy