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

Feature: async #446

Open
mfbx9da4 opened this issue Aug 24, 2022 · 22 comments
Open

Feature: async #446

mfbx9da4 opened this issue Aug 24, 2022 · 22 comments

Comments

@mfbx9da4
Copy link

While mmkv is super fast, and that's awesome 😎 it still blocks the main JS thread and for situations where you have to get a bunch of keys in a loop of some kind, that can add up. It would be great if mmkv could provide a threaded async version where

  • Stored values are loaded from mmkv in a thread
  • JSI strings and numbers are constructed in a thread
  • Promise / callback is resolved back to main JS thread with jsCallInvoker

Do you think that would be possible?

@mrousavy
Copy link
Owner

Yep, that's definitely possible!

I'm not sure if I'd be happy adding an async method for every sync method though, because the API will look a bit messy then. Right now it's very simple and minimal. 😅 Maybe we can find a way around that

Either way, it's a little bit of work, not sure how much exactly but we need to set up a Promise resolver that runs on a separate Thread, custom batched methods, etc etc

@mfbx9da4
Copy link
Author

mfbx9da4 commented Aug 24, 2022

API wise maybe the counterpart to all the standard methods eg. mmkv.getString() and mmkv.getNumber() could be mirrored under the property async ie mmkv.async.getString(), mmkv.async.getNumber(), mmkv.async.getMany()?

Do you like that? @mrousavy

Another hacky thing you could do is have mmkv.getString.async(), I've seen Zustand do that but I kind of hate it and it won't help with batch methods.

Personally I think the API mmkv.getStringAsync() and mmkv.getNumberAsync() are the most natural as they will be the easiest to find for typescript users and I think I've seen some NodeJS APIs use that pattern.

@mfbx9da4
Copy link
Author

mfbx9da4 commented Aug 25, 2022

Just one question around thread safety, say for example we implement the method mmkv.setManyAsync(items: Array<[string, string | number | boolean]). Will it actually be thread safe to create a thread and then inside the thread do convertJSIStringToNSString(argument.getNumber()), argument.getBool(), argument.getNumber() or will that have to be done outside the thread?

@mrousavy
Copy link
Owner

Arg parsing will need to happen outside, but that's fast anyways.

@mrousavy
Copy link
Owner

mrousavy commented Sep 8, 2022

I'm thinking of a potential solution for setting.

I can implement one additional method on the MMKV instance called batch.

const storage = new MMKV()
await storage.batch((s) => {
  s.set('hello', 'world')
  s.set('test', 'another')
  // ...
})

Inside you will use the s instance (same MMKV instance, but a Proxy), which will schedule the methods instead of actually running them.

The only problem I see with this now is that you cannot use getters (s.getString, s.getNumber, s.getBoolean) because those would have to be synchronous.

A "solution" for that would be to make those funcs async, but that's a bit complicated again.

@mfbx9da4
Copy link
Author

mfbx9da4 commented Sep 8, 2022

🤔

Perhaps batch would have to return an array of items, does that solve it?

const storage = new MMKV()
const [foo] = await storage.batch((s) => [
  s.getString('foo'),
  s.set('hello', 'world'),
  s.set('test', 'another')
])

@mfbx9da4
Copy link
Author

mfbx9da4 commented Sep 8, 2022

Also could you not make storage to be context aware, then you could have this API:

const storage = new MMKV()
const [foo] = await storage.batch(() => [
  storage.getString('foo'),
  storage.set('hello', 'world'),
  storage.set('test', 'another')
])

Provided you enforce that batch is always a synchronous function.

BTW, I love the batch() idea

@mrousavy
Copy link
Owner

mrousavy commented Sep 8, 2022

Perhaps batch would have to return an array of items

That's a good idea, but then you'd be very limited in terms of doing stuff contextually (e.g. only set if the value in storage is ....), but yea that would probably work.

Provided you enforce that batch is always a synchronous function.

Wdym? The whole idea is to have batch asynchronous and awaitable

@mfbx9da4
Copy link
Author

mfbx9da4 commented Sep 8, 2022

Wdym? The whole idea is to have batch asynchronous and awaitable

Sorry I mean the batch implementation would enforce that the batch argument callback is synchronous, this is so that the batchFn doesn't need to be injected with a proxy.

class Storage {
  // ...
  batch(fn) {
    ensureSynchronous(fn)
    this._batchingEnabled = true
    try {
      const items = fn().filter(x => x)
      items.every(x => ensureIsBatchItem(x))
      return await this._executeBatch(items)
    } finally {
      this._batchingEnabled = false
    }
  }
}

@mfbx9da4
Copy link
Author

mfbx9da4 commented Sep 8, 2022

but then you'd be very limited in terms of doing stuff contextually (e.g. only set if the value in storage is ....)

What use case might be limited?

@mrousavy
Copy link
Owner

mrousavy commented Sep 8, 2022

eg. this (but on a larger scale with tons of items like arrays):

const storage = new MMKV()
await storage.batch((s) => {
  const x = s.getBoolean('bla')
  if (x === true) {
    s.set('hello', 'world') 
  } else {
    s.set('test', 'another')
  }
  // ...
})

@mfbx9da4
Copy link
Author

mfbx9da4 commented Sep 8, 2022

Hmm yeah it's true that wouldn't work and having a callback does give the impression that it should be possible. One saving grace is that typescript users will get the following error:

const storage = new MMKV()
await storage.batch((s) => {
  const x = s.getBoolean('bla')
  // ❌ This condition will always return 'false' since the types 'MMKVBatchItem' and 'boolean' have no overlap
  if (x === true) {
    s.set('hello', 'world') 
  } else {
    s.set('test', 'another')
  }
  // ...
})

I don't see how you could support the above API, unless... you implemented a similar strategy to react-native-multithreading and all batchFns were marked automatically with a "worklet" like tag? While it's probably overkill for MMKV, it's fun to dream.

@mfbx9da4
Copy link
Author

mfbx9da4 commented Sep 8, 2022

BTW one thing which is way better about .batch vs storage.async.{{method}} is that there will be less back and forth between Microtasks Queue, C++, JSI and JS and you will only pay the cost once per batch. Whereas, with my original API you would expect to pay the cost three times here:

await Promise.all([
  storage.async.getString('foo'), 
  storage.async.getString('bar'), 
  storage.async.getString('baz')
])

Although, thinking about it more, even with the .async approach, in theory you could batch any calls within the same microtask 🤔. Still though, explicit batching is probably preferable.

@mfbx9da4
Copy link
Author

mfbx9da4 commented Sep 8, 2022

Also maybe just twitter poll the options?

@elliotsayes
Copy link
Contributor

+1 For async, as it will make larger buffer storage play nicely :)

In the meantime, is there any way to wrap the existing methods so they don't block the main thread ?

@ex3ndr
Copy link

ex3ndr commented Apr 7, 2023

We have a project where mmkv overgrown it and currently blocks often main thread

@mrousavy
Copy link
Owner

mrousavy commented Apr 7, 2023

Yea I think the async batch methods are cool, there are some things we need to think about though. I also need a lot of free time to work on this, so if you want to see this happening consider sponsoring me / emailing me about those efforts

@bfricka
Copy link

bfricka commented Aug 3, 2023

+1 For async, as it will make larger buffer storage play nicely :)

In the meantime, is there any way to wrap the existing methods so they don't block the main thread ?

Not really, if the underlying implementation is synchronous. The best you could do would be to defer execution to the next tick as a microtask, using promises, requestAnimationFrame, timeout, etc. E.g.

const resolvedPromise = Promise.resolve()
const setAsync = (k: string, v: string) => resolvedPromise.then(() => storage.set(k, v))
setAsync('foo', 'bar')

This would defer to the next tick, running before any timeouts, but it would still block. Async in this way is only useful if your underlying implementation eventually uses threads and doesn't block the main thread. Otherwise, you're just blocking later instead of now.

@emzet93
Copy link

emzet93 commented Aug 21, 2023

I was wondering if it would be possible to delegate saving to MMKV to Worklet, so it doesn't block the main thread? I was trying to implement that but I'm facing issues with executing mmkv instance methods within the worklet. And I'm actually worried that the reason is that I don't really understand the concept of worklets properly 😅

@XantreDev
Copy link

This shouldn't be implemented on worklets, because data sharing with other thread is a big overhead

@emzet93
Copy link

emzet93 commented Sep 20, 2023

that makes sense, thanks for explanation @XantreGodlike!

@JustJoostNL
Copy link

Is this feature planned for in the future?

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

8 participants