[Performance] JSI Storage implementation #95
Conversation
Captured existing storage interface from Onyx to separate file Extracted AsyncStorage usage to provider.js
When we have provider.js and provider.native.js we need to have the mock implemented at the provider level instead of the AsyncStorage level
kidroca
force-pushed
the
kidroca/jsi-storage
branch
from
177cf92 to
51ab035
Compare
|
Wow, I can't wait to see this on android! |
|
Sorry, I've realised that the "After" benchmark was running on a freshly populated DB, while my "Before" benchmark used whatever I previously had I'm still to try this on Android, will post results tomorrow |
|
We should also document a benchmark process for capturing init times as timings can vary a lot based on content Initial storage state
chat content
I think there should be account created for benchmark purposes that everyone (working on perf) can use It will also allow to make some benchmark automation or semi-automation - currently I'm spending
This leaves about 40% of my time to come up with ideas/proposals and code I think the extraction to excel can be coded, we already have the csv data, instead of manually printing it and then copying it to excel it can be send to google sheets using some API. Or the least it can be saved to a .csv file that can be synced somewhere |
Or maybe instead of sharing a pre seeded account, there can be a dev command / API call that will seed my account with a sample chat |
| import AsyncStorage from '@react-native-async-storage/async-storage'; | ||
| import Str from 'expensify-common/lib/str'; | ||
| import lodashMerge from 'lodash/merge'; | ||
| import Storage from './storage'; |
There was a problem hiding this comment.
Instead of using AsyncStorage or MMKV directly, we use the Storage facade
There was a problem hiding this comment.
Would love to see a simpler version of this PR where we do something like
import AsyncStorage from './AsyncStorage
Then have
index.js - import / export from @react-native-async-storage/async-storage
index.native.js - export a custom AsyncStorage
There was a problem hiding this comment.
I don't think we should use the name AsyncStorage if the underlying storage implementation could be of a different library. A more generic term should be used so it's clear we're not tied to AsyncStorage
There was a problem hiding this comment.
Not sure it matters for the purposes of the POC - the code will run the same there will just be less of it.
| @@ -0,0 +1,3 @@ | |||
| import * as Storage from '@react-native-async-storage/async-storage/jest/async-storage-mock'; | |||
There was a problem hiding this comment.
In tests storage is mocked by by the AsyncStorage's mock
Currently it matches perfectly with our storage interface as the interface is based on the AsyncStorage api
| /** | ||
| * @interface | ||
| * The interface Onyx uses to communicate with the storage layer | ||
| */ | ||
| class ProviderInterface { |
There was a problem hiding this comment.
This can serve as an abstract class and even provide some base functionality like parsing/stringifying data
ATM no-one is extending or using the class created here it serves for documentation purposes
- declaring a new Provider and annotating it with
@implements {ProviderInterface}would prompt to implement all methods (Webstorm™) - Providers will also inherit all method documentation (and param types)
There was a problem hiding this comment.
no-one is extending or using the class created here it serves for documentation purposes
Can we maybe explore this when there is a clear need? It feels like an unnecessary distraction for the current exercise.
|
I've added full Android benchmarks (in the issue description: #95 (comment)). I've re-run both before and after cases on a clean install app with no existing storage data My benchmark data doesn't show any improvement for "time to interactive", and there doesn't seem to be any performance improvement while using the app (judging by feel), but anyone can try it out relatively easy and see for themselves To try this locally:
The current code uses this RN implementation https://github.com/mrousavy/react-native-mmkv there is another: https://github.com/ammarahm-ed/react-native-mmkv-storage. But both use MMKV and JSI so I don't think there would be much difference |
|
I think the data captured here is confirming the performance issues of What JSI can help with regarding rendering is: We can add a
|
|
IMO we should consider merging the changes that allow switching the storage provider even if we delete These changes would allows to hook and try different providers at any time |
|
Wow, that's a disappointing surprise. I guess this is why we benchmark! Though I guess the "silver lining" is that the problem is in our code, so it's a lot easier to fix... once we find it. |
Random thought, but maybe we don't need the JSI at all to test an imperfect version of this. We can first:
I would guess there are very few places where we are trying to access values that are not cached and in most cases would be able to get data sync from the cache? In some cases, we would need to read from storage but those feel like outliers. Started a draft here last week experimenting with this idea but haven't had a chance to properly test it out
While the changes are interesting I think we can dig them back up if there is a need for them later. |
|
I like the thought about coming up with some sort of standard test or test account (at least) to measure performance. It's a pain, but the only reliable way to test "app startup time" is to run release builds on actual devices. I've gotten into the habit of doing all testing on release builds because dev builds are not particularly optimized and feel very inconsistent. |
|
Maybe before disregarding this entirely, can we update a much simpler
benchmark that basically directly compares this versus AsyncStorage, and
then we gradually build up bit by bit the Onyx design, to see where the
bottleneck is introduced?
Basically, let's do the simplest possible test to start, hopefully to show
that tencent knocks the socks off AsyncStorage in an artificial test, and
then gradually make the test more and more real to see where it loses its
advantage.
…On Mon, Aug 9, 2021, 1:38 PM Marc Glasser ***@***.***> wrote:
What JSI can help with regarding rendering is: We can add a getAllSync
method to the storage provider interface which would allow us to set
initial withOnyx state instantly. This simplifies the connect procedure and
would skip some re-renders that happen
Random thought, but maybe we don't need the JSI at all to test an
imperfect version of this. We can first:
1. Check to see if all withOnyx() mapped values are cached (meaning we
can get them sync)
2. Set them sync with loading: false and prevent the multiple
setState() calls after mount
I would guess there are very few places where we are trying to access
values that are not cached and in most cases would be able to get data sync
from the cache? In some cases, we would need to read from storage but those
feel like outliers.
Started a draft here last week experimenting with this idea but haven't
had a chance to properly test it out
https://github.com/Expensify/react-native-onyx/compare/marcaaron-withOnyxConnectSync
IMO we should consider merging the changes that allow switching the
storage provider even if we delete storage/index.native.js and still use
AsyncStorage everywhere
While the changes are interesting I think we can dig them back up if there
is a need for them later.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#95 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAEMNUTZLLKUWVFE2ZBR34TT4A4D5ANCNFSM5BXYGRIA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
I can run this in the stand alone benchmark app that was made to benchmark AsyncStorage times
From what I've seen working on the current PR, it's very likely tencent will be bellow 1ms for the same data |
|
@marcaaron |
Wait, let's not give up on it yet! Have we tried an Android release build? I was about to do that. Maybe just hold off on the changes I'm asking about for now. |
|
Ok testing Android build now but doesn't seem to work so far I have...
App builds OK but crashes immediately. Am I missing something? |
|
Everything you need is here
checkout that hash and do That's why the hash I've shared uses |
|
Got it thanks. I was confused because the link just has the |
|
After setting this up on Android and running a release build I can confirm things don't feel faster in any obvious way. Startup time and chat switching look to be about the same as they are on I think it makes sense to continue to investigate with this strategy:
Though, I'm not entirely sure what that process will look like. |
This is the only thing that comes to my mind because that's what we did for AsyncStorage: #95 (comment)
|
|
Can we close this out? Or are there any other steps we were looking to take with this investigation? |
|
We can close this |
|
Thanks! |
Details
Extract a storage interfacing contract
This allow hooking Onyx to different storage providers like react-native-mmkv
Implement storage providers for AsyncStorage and MMKV
Update native platforms to use MMKV for storage while desktop/web still uses AsyncStorage
Related Issues
$ Expensify/App#4492
Automated Tests
Covered by existing tests. No new functionality added or altered
Trying this locally
Checkout from this hash: kidroca/Expensify.cash@8950ce5
Thanks to the extracted provider interface you can easily switch between storage providers by editing the import here:
node_modules/react-native-onyx/lib/storage/index.native.jsimport StorageProvider from './providers/AsyncStorageProvider';import StorageProvider from './providers/MMKV_Provider';Benchmarks
iOS (Before with AsyncStorage)
iOS (After with MMKV)
Android (Before with AsyncStorage)
Android (After with MMKV)
Linked PRs