Load all storage data into cache during Onyx.init#752
Load all storage data into cache during Onyx.init#752fabioh8010 wants to merge 1 commit intoExpensify:mainfrom
Conversation
|
Reassure output The 🔴 are expected because of this PR very purpose – load all storage data into cache during startup. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c251c8440
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Web tests well. Mobile app testing currently blocked by this bug on main |
| return provider.store.executeAsync<OnyxSQLiteKeyValuePair>('SELECT record_key, valueJSON FROM keyvaluepairs;').then(({rows}) => { | ||
| // eslint-disable-next-line no-underscore-dangle | ||
| const result = rows?._array.map((row) => [row.record_key, JSON.parse(row.valueJSON)]); | ||
| return (result ?? []) as StorageKeyValuePair[]; | ||
| }); |
There was a problem hiding this comment.
You tested this on a high traffic account right? I wonder if parsing all JSON synchronously may slow the app down or even block it. It's unlikely, just worth checking imo.
There was a problem hiding this comment.
I agree with the concern. Maybe you can set up some performance timing for this? I'd like to know what impact it will have on real accounts, especially the large ones.
There was a problem hiding this comment.
results from dev environment with 33k keys:
- web: 320ms
- iOS: 700ms
It's hard to compare it 1:1 with current solution, because on main this cost is spread across the entire startup - but in general ManualAppStartup metric gets 2.5s faster for this account on iOS
adhorodyski
left a comment
There was a problem hiding this comment.
nothing more than what is already pointed out, lgtm
tgolen
left a comment
There was a problem hiding this comment.
This is a very large PR, so naturally, I have concerns with it. What I would suggest is to rewrite the PR to only contain the absolute bare-minimum changes necessary to introduce the cache loading on it. This would mean:
- eviction code stays for now
- RAM only keys stay for now
If we pre-load everything into cache, then we won't have to make any other changes initially, right? cache.hasCacheKey() would naturally return true for all the keys. At least, that's the idea anyway for the least amount of changes.
I think that will allow us to really evaluate if this change is truly a good one, or one that we want to revert. It is hard to determine it's effectiveness until it's in the real world, and then we can clean that stuff up later if we decide to keep it permanent.
| return provider.store.executeAsync<OnyxSQLiteKeyValuePair>('SELECT record_key, valueJSON FROM keyvaluepairs;').then(({rows}) => { | ||
| // eslint-disable-next-line no-underscore-dangle | ||
| const result = rows?._array.map((row) => [row.record_key, JSON.parse(row.valueJSON)]); | ||
| return (result ?? []) as StorageKeyValuePair[]; | ||
| }); |
There was a problem hiding this comment.
I agree with the concern. Maybe you can set up some performance timing for this? I'd like to know what impact it will have on real accounts, especially the large ones.
2c251c8 to
5a0ef6d
Compare
a2037d3 to
fc85004
Compare
|
Ready to review again @tgolen @marcaaron @Julesssss @situchan – we removed the cache eviction changes |
tgolen
left a comment
There was a problem hiding this comment.
This looks great now, thank you! I really appreciate the verbose comments. Those are super helpful.
marcaaron
left a comment
There was a problem hiding this comment.
LGTM overall. Didn't have too many comments. I think we should give this a shot.
| const allDataFromStorage: Record<string, unknown> = {}; | ||
| for (const [key, value] of pairs) { | ||
| // RAM-only keys should never be loaded from storage as they may have stale persisted data | ||
| // from before the key was migrated to RAM-only. |
There was a problem hiding this comment.
nit pick - they are now being loaded from storage just not cached so the comment is stale
| continue; | ||
| } | ||
|
|
||
| // Skip collection members that are marked as skippable |
There was a problem hiding this comment.
NAB, but I went to try to understand what is a skippable collection member id and there's not really any documentation in react-native-onyx about it.
It seems like it exists to prevent the possibility of malformed collection keys which is reasonable, but I think we could document this better at the library level.
|
|
||
| // Notify subscribers about default key states so that any subscriber that connected | ||
| // before init (e.g. during module load) receives the merged default values immediately | ||
| for (const [key, value] of Object.entries(merged ?? {})) keyChanged(key, value); |
There was a problem hiding this comment.
| for (const [key, value] of Object.entries(merged ?? {})) keyChanged(key, value); | |
| for (const [key, value] of Object.entries(merged ?? {})) { | |
| keyChanged(key, value); | |
| }; |
nab/style
| for (const [key, value] of Object.entries(merged ?? {})) keyChanged(key, value); | ||
| }) | ||
| .catch((error) => { | ||
| Logger.logAlert(`Failed to load data from storage during init. The app will boot with default key states only. Error: ${error}`); |
There was a problem hiding this comment.
We should probably keep track of this in logs somehow. I'd be very curious to see the cases where this throws and why.
| cache.merge(merged ?? {}); | ||
|
|
||
| // Notify subscribers about default key states so that any subscriber that connected | ||
| // before init (e.g. during module load) receives the merged default values immediately |
There was a problem hiding this comment.
Interesting. Who connects before? I guess we do not block subscriptions on init phase?
Details
This PR replaces Onyx's lazy per-key cache population strategy with a single bulk read of the entire storage database during init(), and removes the cache eviction system that is no longer needed.
E/App PR: Expensify/App#85210
Related Issues
Expensify/App#85252
Automated Tests
Unit tests added/changed to cover the changes.
Manual Tests
Author Checklist
### Related Issuessection aboveTestssectiontoggleReportand notonIconClick)myBool && <MyComponent />.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)Avataris modified, I verified thatAvataris working as expected in all cases)mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
android.mov
Android: mWeb Chrome
We couldn't record for Android web as emulator crashes randomly.
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop