mrousavy / react-native-mmkv

โšก๏ธ The fastest key/value storage for React Native. ~30x faster than AsyncStorage!
https://mrousavy.com
MIT License
5.76k stars 249 forks source link

How is this library different from another mmkv? #100

Open Kiura opened 3 years ago

Kiura commented 3 years ago

How is this library different from another mmkv react-native library?

https://github.com/ammarahm-ed/react-native-mmkv-storage

azeeka commented 3 years ago

As far as I know, react-native-mmkv-storage supports encryption:

Full encryption support The library supports full encryption on Android and iOS. You can choose to store your encryption key securely for continuious usage. The library uses Keychain on iOS and Android Keystore on android (API 23 and above). On android for lower api levels (API 22 and below), it uses secure prefrences which provides not perfect but incremental security on older Android APIs.

mrousavy commented 3 years ago

Originally there was only react-native-mmkv-storage by ammarahm-ed, which was a legacy bridge module (slow). At that time, I was experimenting with JSI and decided to create a new MMKV library for React Native which was way faster and synchronous by using JSI. So that's what I did, it was a lot faster and you could access all functions synchronously, so no await. After a few weeks, ammarahm-ed apparently saw that, and also decided to migrate his library to JSI, so now the performance difference might be smaller. I noticed that a lot of his code looks similar to mine, but I don't want to make any accusations here. I do however have a way simpler interface, so to get a value you just do:

const storage = new MMKV()
const name = storage.getString('name')

Whereas with his library you kinda have this loader interface, I don't really know how this works so here's just the code taken from his docs:

const MMKV = new MMKVStorage.Loader().initialize(); // Returns an MMKV Instance 

// Then make are read/write requests

let string = await MMKV.getStringAsync("string");

So mine has a cleaner and more lightweight interface.

It's a matter of personal preference whether you would like to use my library or his.

Also:

Edit

My library now also supports

  1. multiple instances
  2. encryption
  3. custom IDs
  4. custom paths

So:

const storage = new MMKV({
  id: 'some-user-storage',
  path: ...,
  encryptionKey: ...
})
Kiura commented 3 years ago

Alright, now it is clear, I thought there were fundamental differences. Thank you for making this clear.

mrousavy commented 3 years ago

let's leave this open for future reference

mtroskot commented 2 years ago

I think in the best interest of RN community would be that your mmkv library and react-native-mmkv-storage merge into 1 library, since they wrap the same storage and have the same functionalities. The end result would be a more stable library.

Raymond-Cox commented 2 years ago

I think in the best interest of RN community would be that your mmkv library and react-native-mmkv-storage merge into 1 library, since they wrap the same storage and have the same functionalities. The end result would be a more stable library.

Currently we get to coin-flip which one will stop maintaining their project first ๐Ÿ˜„

mrousavy commented 2 years ago

Well if you know my libraries, you know that I don't stop maintaining them. All of them work with the latest RN version.

Also, since I am running an app development agency, I use react-native-mmkv (and VisionCamera and others) in multiple production apps, so they work there and it's in my (and my clients') best interested to keep the maintained.

If you are missing some features or need advanced support/help, or just want to show appreciation, you can always sponsor this project. :)

mrousavy commented 2 years ago

Also, I'm not merging my library into his, I like my code, it's very clean and minimal. I also like my API more. And my library was using JSI first ๐Ÿ˜„

But it's always a matter of preference, you wouldn't prefer someone else's baby over yours would you?

Raymond-Cox commented 2 years ago

I meant no offense mate. I ended up digging through the implementation/code thoroughly before choosing your library. I've spent the last hour implementing your migration script (https://github.com/mrousavy/react-native-mmkv/issues/52#issuecomment-919890532), and refactoring the codebase (no more await for storage, yay ๐ŸŽ‰ ). So far it's been painless and I'm thoroughly impressed with this library. Thanks for your efforts @mrousavy ! I'll consider sponsoring. ๐Ÿ’ฏ

mrousavy commented 2 years ago

Thank you Raymond, and no offense taken!

raven619claw commented 2 years ago

hi, first of all thanks for this great library!

I wanted to know is there any difference or security risk in providing the encryption key in JS code vs generating a random key and storing in keychain(which react-native-mmkv-storage does https://github.com/ammarahm-ed/react-native-mmkv-storage#full-encryption-support)?

mrousavy commented 2 years ago

Hi! Storing the encryption key in keychain seems to be a good idea, you can either manually do that or shoot me a PR so I can add it to MMKV :)

pke commented 2 years ago

Storing in the key chain should not be part of this library, there are other libraries out there already handling this I think.

Aman1706 commented 1 year ago

@mrousavy Hey I have a question, I am currently using the react-native-mmkv-storage package but want to shift to your package, can I do so without breaking my app? My app is live on the app and play store so will it be a problem for users if I push an update with your package?

mrousavy commented 1 year ago

@Aman1706 do you want users to keep their data? I would recommend to wipe all of the data and set up a new storage. This means that your users will probably need to log in again. This would be the simplest approach, if you have critical data stored in MMKV that you can't wipe, it will still be possible (we both use the same MMKV lib under the hood), but you need to test it carefully.

Aman1706 commented 1 year ago

@mrousavy Yeah ideally I would like the users to not have to logout, but there is nothing critical per say Will try it and see what happens if I add your package without clearing the data otherwise will clear the data as you mentioned thanks!

arthurgeron-work commented 1 year ago

Another big difference worth mentioning is that react-native-mmkv-storage has no memory leaks by design

pke commented 1 year ago

Another big difference worth mentioning is that react-native-mmkv-storage has no memory leaks by design

How so? What is that design, that makes this possible?

arthurgeron-work commented 1 year ago

Another big difference worth mentioning is that react-native-mmkv-storage has no memory leaks by design

How so? What is that design, that makes this possible?

Apparently as discussed here this mmkv lib does not re-write it's internal hash map position for a existing key when updating a entry, instead it creates a new entry without deleting the old one, which is a memory leak and has been reported to cause crashes.

frozencap commented 1 year ago

What @arthurgeron is saying is nonsense. Read the Tencent/MMKV README to understand its usage. There is no memory leak.

arthurgeron-work commented 1 year ago

Shawarmaz is not a maintainer and is driving drama around the subject, so please do not take his or my word for granted, just read the thread I linked, the replies from the owner and take your own conclusions

frozencap commented 1 year ago

0 drama, just patently false claims of a "memory leak" from @arthurgeron. Native MMKV does not overwrite keys. This is not an issue with the js implementation but is the very design of MMKV itself.

More explanation here: https://github.com/mrousavy/react-native-mmkv/issues/440#issuecomment-1424412162

mrousavy commented 1 year ago

I get what you're saying @arthurgeron-work about MMKV skipping storage resizes so it can write faster (that's by design, if you use it right, it is really fast. You just gotta understand that MMKV is not a large database) - but why are you saying that react-native-mmkv-storage does not have that limitation? It is a JS wrapper around the native Tencent/MMKV storage just like my library. Did you test the difference here? Can you share your code and results that made you conclude that the other lib does not grow in size but mine does?

Either way, in 2.7.0, I added a property called fastWrites - that is now on npm and you can test that. It is false by default, so it should no longer grow in size. If you enable it, MMKV will become faster but uses more memory.

arthurgeron-work commented 1 year ago

I get what you're saying @arthurgeron-work about MMKV skipping storage resizes so it can write faster (that's by design, if you use it right, it is really fast. You just gotta understand that MMKV is not a large database) - but why are you saying that react-native-mmkv-storage does not have that limitation? It is a JS wrapper around the native Tencent/MMKV storage just like my library. Did you test the difference here? Can you share your code and results that made you conclude that the other lib does not grow in size but mine does?

Either way, in 2.7.0, I added a property called fastWrites - that is now on npm and you can test that. It is false by default, so it should no longer grow in size. If you enable it, MMKV will become faster but uses more memory.

Because Tencente MMKV itself is not designed to be use just as a interface for fast writes/reads, as explained here:

Using append to implement incremental updates brings a new problem, that is, if you continue to append, the file size will grow uncontrollably. [...] This is obviously not desirable. We need to make a compromise between performance and space: apply for space in units of pagesize of memory, and use append mode until the space is exhausted; when append reaches the end of the file, perform file reorganization, key deduplication, and try to serialize and save Duplication results; if the space is still not enough after deduplication, double the size of the file until there is enough space.

Hence the delete before prepend is the wrong approach, we haven't even tested yet before assuming that fixes the issue.

The other MMKV lib apparently makes the correct use as instructed by the lib and does not crash when used "too much" when I left it on loop rewriting values, and there are 0 issues there related to crashes/leaks. Unfortunately I do not have statistics to share, feel free to do your own evaluation.

This would've been much clearer had @shawarmaz not lied about it being part of Tecent's MMKV "design" and confused me when it was clearly stated in the readme, maybe that's my mistake for believing without checking.

mrousavy commented 1 year ago

@arthurgeron-work can you please create a new GitHub repo that is a reproduceable example for the problem you keep talking about?

We can compare RN MMKV with the other rn mmkv lib to see if there really is a change in behaviour there, in the code there is not much difference.

mrousavy commented 1 year ago

Because Tencente MMKV itself is not designed to be use just as a interface for fast writes/reads,

How did you come to that conclusion? The way I read this, is that they are sharing internal insights on how MMKV works under the hood. They write as fast as they can, and if the size is too large, they truncate it. If the size is too small, they double it. Are you assuming that the user needs to use the code they shared there on their end so that MMKV works as expected? ๐Ÿ˜„

frozencap commented 1 year ago

@arthurgeron-work

Tencente MMKV itself is not designed to be use just as a interface for fast writes/reads

I get it's in Chinese and translations can be ambiguous, but surely you don't believe you're "the only one" who finally understood how MMKV works?

What I said is backed by the docs and the experience of, apparently, many. MMKV memory usage will grow as you overwrite keys, yes, by design. What you're saying is backed by nothing. Post repo or stop these made-up rants

arthurgeron-work commented 1 year ago

It behaves in a way that'll crash if done without following docs instructions, that does not mean it was intended to be like that, since clearly the Readme explains that using without truncanting will cause crashes and how to avoid it. "By design" is not default, unmanaged, behavior of the code, it's the intended and correct use following the documentation. But reading docs is already something

they truncate it They're explaining how you'd manage data and truncate before appending, Tecent's mmkv itself does not apply that append strategy by default, even through translation it's a clear instruction for other maintainers/devs that'll use the lib Are you assuming that the user needs to use the code they shared there on their end so that MMKV works as expected? You mean, the c++ append approach that the docs clearly explain should be used when using Tencent MMKV lib? No, maintainers like you of libs should be using it. @arthurgeron-work can you please create a new GitHub repo that is a reproduceable example for the problem you keep talking about? Simple async for loop with writes and wait for the app to crash, not complicated at all. Personally I don't have the time. Feel free to test yourself.

mrousavy commented 1 year ago

It behaves in a way that'll crash

@arthurgeron-work Again, can you please post a reproduction example? I cannot reproduce this.

arthurgeron-work commented 1 year ago

I get it's in Chinese and translations can be ambiguous, but surely you don't believe you're "the only one" who finally understood how MMKV works?

So basically it could just be exactly as I said and you clearly can't think on anything else meaningful to say.

arthurgeron-work commented 1 year ago

It behaves in a way that'll crash

@arthurgeron-work Again, can you please post a reproduction example? I cannot reproduce this.

I'll try to do a sample in expo (I don't use it here, so idk if it crashes), if it doesn't crash I'll make a example in bare RN, but I can't promise it'll be soon

arthurgeron-work commented 1 year ago

@mrousavy BTW, if you run xcode with app inspection you can clearly see the memory not being freed after doing X writes on same key, which is characteristic of leaks. This might be a way to repro it without having to wait for crashes, which can take some time

mrousavy commented 1 year ago

@arthurgeron-work I think it's really annoying how you just complain without ever creating a reproduceable example.

Either way, I still took the time & created a reproduceable example for what you're seeing, here is the code:

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.

Also, stop spreading misinformation. You said that this isn't an issue with the other MMKV lib (rn-mmkv-storage), which is just complete bullshit and shows that you didn't even test your claims. It is exactly the same problem, because again, it is how core MMKV works. Fun fact: With the other library, CPU usage is at 98%, with mine it stays at 54%.

arthurgeron commented 1 year ago

I think it's really annoying how you just complain without ever creating a reproduceable example.

Iโ€™m not the one that originally reported the crash and usually maintainers kindly wait for a reproducible repo to be made instead of demanding it asap, which isnโ€™t how to world works.

Maybe all of this wouldโ€™ve been avoided if instead of attacking me and pretending it wasnโ€™t a issue we focused on understanding the problem, reading the docs and seeing what could be done to solve it.

mrousavy commented 1 year ago

instead of demanding it asap

@arthurgeron Never said I want anything asap, but you wrote comment after comment saying how it's broken, falsely advertised, and works in the other rn-mmkv lib all without providing any reproduction. What do you expect? A thumbs up comment? Other people working for you to help you for your app?

I quote your first comment here in this issue:

Another big difference worth mentioning is that react-native-mmkv-storage has no memory leaks by design

Wrong.

Checking if it exists and deleting before set seems to have worked

Wrong.

So it's either delete and trim manually or move to another lib like react-native-mmkv-storage, gotcha.

Wrong.

react-native-mmkv-storage works just fine

Wrong.

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

"we" = other people?

Regular use of a lib that does not describe itself as "not production ready" should never cause a crash regardless

Okay so if there is a bug in the code I should warn everybody that they should not use my lib at all?

still, not safe to use in production, even with workarounds, because it by default uses it as a "DB"of sorts which it isn't intended to do in the first place

I don't know how many users are using apps that use mmkv, but there's a ton of apps that depend on it. WeChat for example uses it.

I understand the "issue" is in Tencent MMKV, but having a "just let people know" attitude instead of searching for a fix ( like creating a modified Tencent MMKV base ) isn't right The minimum would be to add a change (if possible to 2.5.x) as well that avoids this problem entirely

Ah yes, it crashes for you in your app, so I need to fork Tencent/MMKV and implement custom/smart mmap truncation in C++

All of this is off-topic to this issue, so I'm going to mark all comments off-topic. Feel free to continue the discussion in #440.

frozencap commented 1 year ago

Muting notifications after posting this.

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.

https://github.com/Tencent/MMKV/issues/610#issuecomment-752015762

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.

there's a ton of apps that depend on it. WeChat for example uses it.

Certainly! But it uses it for the use cases it benchmarked it against: NSUserDefaults and SharedPreferences. For actually storing data they developed and use.. a database: https://github.com/Tencent/wcdb

but you wrote comment after comment saying how it's broken, falsely advertised

I was the one who mentioned that, that this may be a case of false advertisement over MMKV at large (not just react-native-mmkv), and I stand by that: benchmarking MMKV imo is backwards. The whole point is if you even start hitting these issues in the first place, the chances that you're using it for the wrong data approach 100%.

arthurgeron-work commented 1 year ago

instead of demanding it asap

@arthurgeron Never said I want anything asap, but you wrote comment after comment saying how it's broken, falsely advertised, and works in the other rn-mmkv lib all without providing any reproduction. What do you expect? A thumbs up comment? Other people working for you to help you for your app?

I quote your first comment here in this issue:

Another big difference worth mentioning is that react-native-mmkv-storage has no memory leaks by design

Wrong.

Checking if it exists and deleting before set seems to have worked

Wrong.

So it's either delete and trim manually or move to another lib like react-native-mmkv-storage, gotcha.

Wrong.

react-native-mmkv-storage works just fine

Wrong.

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

"we" = other people?

Regular use of a lib that does not describe itself as "not production ready" should never cause a crash regardless

Okay so if there is a bug in the code I should warn everybody that they should not use my lib at all?

still, not safe to use in production, even with workarounds, because it by default uses it as a "DB"of sorts which it isn't intended to do in the first place

I don't know how many users are using apps that use mmkv, but there's a ton of apps that depend on it. WeChat for example uses it.

I understand the "issue" is in Tencent MMKV, but having a "just let people know" attitude instead of searching for a fix ( like creating a modified Tencent MMKV base ) isn't right The minimum would be to add a change (if possible to 2.5.x) as well that avoids this problem entirely

Ah yes, it crashes for you in your app, so I need to fork Tencent/MMKV and implement custom/smart mmap truncation in C++

All of this is off-topic to this issue, so I'm going to mark all comments off-topic. Feel free to continue the discussion in #440.

You're bringing old conversations back, many aren't really even "wrong" but it's beyond me answering some ego driven discussion any further, all this confusion because everything was either taken personally or made into a personal attack. Anyway best of luck for you.

fukemy commented 1 year ago

other difference: this owner was handsome more than mmkv-storage owner โœŒ๏ธ

thoth-seshat commented 1 year ago

I would love to see this working out of the box with keychain โค๏ธ

wezter96 commented 11 months ago

Has it been discussed having a similar API for the encryption as react-native-mmkv-storage?

const storage = new MMKVLoader() .withEncryption() // Generates a random key and stores it securely in Keychain .initialize();

What is the currently recommended approach if you want to store something somewhat securely with this library?

If I keep the encryption key on the client I assume it could easily be found and used by someone with malicious intent export const storage = new MMKV({ id: user-${userId}-storage, path: ${USER_DIRECTORY}/storage, encryptionKey: 'hunter2' // This or anything else set from the client point of view would be insecure right? })

mrousavy commented 11 months ago

This is how react-native-mmkv-storage generates an encryption key.

It's just a random string that changes on every call.

wezter96 commented 11 months ago

@mrousavy but they store that encryption key on the keychain right?

To get similar level of security with this lib I would have to implement the storing of the encryption key on the keychain myself I guess?

Or maybe I am misunderstanding something. If I set an encryptionkey in my app when creating my storage:

export const storage = new MMKV({ id: user-${userId}-storage, path: ${USER_DIRECTORY}/storage, encryptionKey: 'hunter2' }) Someone with malicious intent could easily find this encryptionkey and use it to decode what I have stored on any device with react-native-mmkv I guess?

With the react-native-mmkv-storage solution I would have to get their individual encryption key from their keychain in order to decrypt what has been stored with it right?

Would be so amazing if this library offered similar security level as one of the libraries mentioned here so it could be the first sync secure library endorsed by React Native. https://reactnative.dev/docs/security#secure-storage

tyson90 commented 1 month ago

I think in the best interest of RN community would be that your mmkv library and react-native-mmkv-storage merge into 1 library, since they wrap the same storage and have the same functionalities. The end result would be a more stable library.

Currently we get to coin-flip which one will stop maintaining their project first ๐Ÿ˜„

And we have a winner I think ๐ŸŽ‰