[HOLD for payment 2023-01-19] [Performance] Use react-native-quick-sqlite instead of asyncStorage

[Szymon20000]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

What performance issue do we need to solve?

reducing time spend on serialisation/deserialisation and generally bridge overhead.

What is the impact of this on end-users?

slower app

List any benchmarks that show the severity of the issue

https://github.com/mrousavy/react-native-mmkv#benchmark

Proposed solution (if any)

use react-native-mmkv for Onyx

List any benchmarks after implementing the changes to show impacts of the proposed solution (if any)

Note: These should be the same as the benchmarks collected before any changes.

Platform:

Where is this issue occurring?

• Web
• iOS
• Android
• Desktop App
• Mobile Web

Version Number:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:

View all open jobs on Upwork

[melvin-bot]

Triggered auto assignment to @thienlnam (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[melvin-bot]

📣 @Szymon20000! 📣
Please report bugs or suggest features in the #expensify-open-source Slack channel, don't directly open issues in this repo!
Instructions here to join the channel 📖

[roryabraham]

Switching this to weekly

[trjExpensify]

👋 Should we update the title of this issue to be "use react-quick-sqlite instead of asyncStorage" and does this PR need to be linked to this issue?

[melvin-bot]

This issue has not been updated in over 15 days. @Szymon20000, @roryabraham eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[melvin-bot]

This issue has not been updated in over 15 days. @Szymon20000, @roryabraham eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

[JmillsExpensify]

Commenting again for Melvin.

[JmillsExpensify]

@Szymon20000 following on all the activity last week in the PR, is your plan to circle back this week and address? Looks like we are hopefully close and can close this out ahead of the end of the year.

[trjExpensify]

PR is in review, adding the Reviewing label.

[JmillsExpensify]

Nice catch. CCing a couple of people in the linked PR. @iwiznia @marcaaron @roryabraham What's needed to get this PR to the finish line ahead of the holidays?

[iwiznia]

Approved the PR. I think we will need to:

• Get the reviews done
• Send a PR to update App to use the new version
• Add some testing to ensure this does not break everything
• I assume we will need some kind of migration to move all data from one place to the other

[JmillsExpensify]

Get the reviews done

Alright, @yuwenmemon @kidroca @parasharrajat @thienlnam @marcaaron @mountiny @roryabraham Can you all please prioritize a review so that we can unblock this specific PR? We still have several steps left, let's dig in and get this one crossed off the list.

[kidroca]

@iwiznia

• Add some testing to ensure this does not break everything

Do you mean a testing strategy/steps for QA?

Since Onyx logic remains the same - it is still covered by existing tests
We can add storage provider tests - they can ensure the correct SQL queries are sent to quick-squlite, but nothing more than that (e.g. not critical to do now, perhaps in a follow-up PR)

• I assume we will need some kind of migration to move all data from one place to the other

IMO this would be very little value compared to the time we'll need to spend writing the migration

It's not a simple migration like an Onyx key getting renamed to something else

To write a migration for the storage driver it:

• the migration file (written in App) would have to (try) read values using AsyncStorage - meaning AsyncStorage have to stay as package.json dependency (forever, because the migration file stays in the source forever)
• then write/migrate data to Onyx using Onyx.update or raw commands
• verify the migration would not corrupt something (which is a hard task to do)

The alternative is to write no migration, which would simply render the user logged out, and once they log in they'll have their data synchronized automatically (as @marcaaron pointed out earlier: Expensify/react-native-onyx#211 (review))

A middle ground would be to try and migrate only auth data (token, session) so the user does not need to re-login. Then anything missing should be automatically synced from the server

[iwiznia]

Do you mean a testing strategy/steps for QA?

I mean using the new react-native-onyx version in the app and playing around with it, checking nothing breaks.

The alternative is to write no migration, which would simply render the user logged out, and once they log in they'll have their data synchronized automatically (as @marcaaron pointed out earlier: Expensify/react-native-onyx#211 (review))

Good points. I think I am ok with this.

A middle ground would be to try and migrate only auth data (token, session) so the user does not need to re-login. Then anything missing should be automatically synced from the server

If this is easy, then let's go for it.

Ah, also, I think we should add something to delete all the existing data from the old place, no? Or would the browser clear it out eventually?

[marcaaron]

Web Browser - no as we are not messing with web storage in this change.
Native - I guess we could clear this. I'm not sure if it's worth it. Fresh install of the app would eventually clear out this data. We can always do a simple migration that just does AsyncStorage.clear() (but requires that we include the library forever just for this purpose).

[kidroca]

Ah, also, I think we should add something to delete all the existing data from the old place, no? Or would the browser clear it out eventually?

Good point, I don't know - need to research this

In any case to make migrations easier to manage problems like these we need to update the migration system a bit so that

• migrations can be loaded on demand (e.g. require calls)
• irrelevant migrations can be dropped, or at least not required

This would allow us to use dependencies in migrations (e.g. AsyncStorage), and perhaps carry them for a month or 2 (using some heuristics - e.g. are there any users using a version predating the migration) after they are no longer needed we can drop them altogether

[JmillsExpensify]

@tgolen Isn't this issue done at this point?

[roryabraham]

The PR to upgrade E/App to use the new quick-sqlite based version of Onyx was deployed to staging in #14188, not yet deployed to production

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.52-4 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Migrate to react-native-quick-sqlite / Update react-native-onyx to 1.0.32 #13967

If no regressions arise, payment will be issued on 2023-01-19. 🎊

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [] The PR that introduced the bug has been identified. Link to the PR:
• [] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@JmillsExpensify] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here:

[mountiny]

I think there is nothing else to do here since payments are handled by different flow in this case. Should we close this issue or anything else left? 🎉

[JmillsExpensify]

@Szymon20000 Can you make sure that we handle your individual $1,000 bonus for this issue on Margelo invoice?

[JmillsExpensify]

@mountiny I'll go ahead and close the issue, though can you double confirm that the BZ checklist above doesn't apply?

[mountiny]

It does not apply in this case!