Perf: use idb-keyval

[hannojg]

Details

While investigating performance issues on large accounts we found an issue where storing large amounts of data in onyx takes a very long time (E.g. merging the data from a OpenApp command).
Largely this was due to the underlying storage provider opening a new database transaction for every operation in a multiMerge/multiSet operation.
The idb-keyval package optimises this, as it can handle multiple operations in one database transaction.
The good thing is, that idb-keyval also uses indexedDB as storage provider, so no migration is needed.

I created a performance comparison tool, and it showed how much better idb-keyval performs:

Each operation has been executed with 10,000 items (depth 1), which has a size of ~2.3MB

Operation	Onyx w/ Localforage	Onyx w/ idb-keyval	Improvement in %
Single Set	42ms	36ms	-14%
Get (collection)	26ms	11ms	-58%
MergeCollection	57926ms	775ms	-99%
Single Merge	48ms	30ms	-38%

How to run the performance tests yourself:

1. Hop on this branch (its on our fork, sorry)
2. cd example
3. npm i
4. npm start
5. Open in your web browser http://localhost:8080

For the product the results can be seen in this video:

https://expensify.slack.com/archives/C035J5C9FAP/p1691063111819169

Related Issues

#281

Automated Tests

Linked PRs

[MonilBhavsar]

Looks great! Any idea how can we test this in product? 🚀

[MonilBhavsar]

@marcaaron could you please review, as you have better context of the issue

[marcaaron]

Looks great! Any idea how can we test this in product?

I would try this:

1. Run npm build from inside react-native-onyx
2. Replace the /dist folder in App/node_modules/react-native-onyx
3. Use the production API proxy in the .env file
4. Sign in with an account that is completely buggered and has tons of reports + policies
5. See if it improves performance

[marcaaron]

Maybe we should just remove all traces of localforage if nothing will use it? 🤔

[marcaaron]

confirming: This will have the existing data from the current DB previously set up by localforage?

[hannojg]

Yes exactly 😊 It can just be replaced in place

[hannojg]

@marcaaron You mean to delete the provider file?

[marcaaron]

You mean to delete the provider file?

👍

[hannojg]

Alright, I removed all traces of local forage 😊

[parasharrajat]

Wow...

[roryabraham]

NAB @hannojg maybe we can benchmark Onyx.clear as well since I know there are some race conditions around it and it's unclear how well the localforage-removeItems plugin ever actually worked

[marcaaron]

oh yeah great thought @roryabraham! adding to that point, I think the place where I see this slowing things down the most is when someone signs out of an account with a ton of data. so we should be sure to A/B test that flow and see if things improved.

[danieldoglas]

maybe we can benchmark Onyx.clear as well since I know there are some race conditions around it and it's unclear how well the localforage-removeItems plugin ever actually worked

I don't think this is blocker for merging this PR since we don't know if the old one actually worked, right?

It would be great to have this merged Today so we can see the benefits for this mostly in Guides and AM accounts

[danieldoglas]

Both of them return 0ms when running Clear

[hannojg]

Let me fix the benchmark for clearing and give numbers for that 😊 With this implementation deleting / clearing should definitely work

[hannojg]

Clearing seems to be a bit slower with idb-keyval, however the impact doesn't look too dramatic. I think we should be able to optimise it to be on par with local forage. However, I don't think that has too much priority at the moment / blocking this from getting merged?

(Note: for a lower item count there is almost no difference between the two implementations)

[mountiny]

Thanks @hannojg

The changes look good to me and seems like the concerns of Marc and Rory has been addressed as well. I think it would be great to test this one out in the App so given that @danieldoglas and @MonilBhavsar are approving of this too, it could hopefully improve the Guides open app start time masivelly so I will go ahead and merge this and Margelo can put up a PR with the bump

[marcaaron]

While investigating performance issues on large accounts we found an issue where storing large amounts of data in onyx takes a very long time (E.g. merging the data from a OpenApp command).
Largely this was due to the underlying storage provider opening a new database transaction for every operation in a multiMerge/multiSet operation.

Are we sure this is faster than the previous solution of setting one value at a time synchronously? Did we verify that things improved on a large account? Which account did we test with?

The idb-keyval package optimises this, as it can handle multiple operations in one database transaction.

Is is possible we can ask it to do too many at once? I am seeing various weird logs like:

Error: TimeoutError: Transaction timed out due to inactivity.. onyxMethod: Ae ~~ parameters: '' userAgent: 'Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/116.0.0.0 Safari/537.36 Edg/116.0.1938.54'

[mountiny]

I believe @hannojg is on vacation @Szymon20000 Would you be able to look into the above?

[Szymon20000]

I will try but my focus this week is on improving chat switching and batching updates.

[Szymon20000]

Added to my todo list!