-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Bump Onyx version to help with IndexedDB slow writes #8053
Conversation
cc @AndrewGable @Julesssss since y'all approved the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Soooo. Things do seem quicker, but I'm seeing a new issue that is not occurring on staging. Tapping a chat will temporarily hide it from the LHN...
Chats-Dissapearing.mov
Hmm... my LHN is in a completely different order on staging than it is on dev (staging looks wrong). But I do see the chat disappearing thing.... let me try and debug why it happens. |
Only seems to happen for certain chats. After clicking through a bunch where it was happening I'm having a hard time reproducing. 🤔 |
One thing I'm noticing is that when it happens we make an extra call to I suspect this is happening here: App/src/libs/actions/PersonalDetails.js Lines 180 to 181 in 47b4541
We only set the details for the participants of that report. But whenever we do a merge we always get first and then combine the data together see Onyx code here. That should be fine to do sync and I can't think of why it would make the option row disappear. The lack of consistent reproduction here makes it hard to debug. Moving on... the only time this should happen however is when this gets called here: App/src/pages/home/report/ReportActionsView.js Lines 133 to 135 in 47b4541
So that must mean that we are missing the Forced the report fetch to run by removing the conditional and verified the sidebar row did not disappear consistently (so it's maybe related to the missing report thing which I'm not able to reproduce). Not sure what we should do in this case. I think we should maybe just revert the Onyx PR for now. The reported issue does eventually seem to sort itself out and I can't repro it at all with my current data... |
Alright so verified the issue is because the Here's what the report page had for a report when the bug occurs (first one is fine next one is not) Going to look at what might cause partial report data to be set. My assumption was that since we have caching in Onyx that it doesn't really matter whether we throttle the writes or not, but data is getting corrupted somehow. The fact that we only have |
Full report data appears and we try to set it here: App/src/libs/actions/Report.js Line 402 in 47b4541
But IndexedDB only shows partial data... 🤔 Something is broken 😅 |
Got a bit closer to figuring this out, but not sure if I understand the problem entirely. Could use some 👀 from @kidroca here. Basically, the implementation of This is bad. But we should also be updating the cache when we call And on a page refresh looks fine... however, the data that is making it to the report page does not seem synced with the cache since we can still end up on the |
Ok scratch the part about the cache being fine on refresh. It looks like it can be only a partial cache sometimes and with bad data having been set during I think if we prevent setting the bad partial data somehow we will be good. Will think about how to do this. |
I'll take a look as well |
@marcaaron When I implemented Making writing happening in sequence just made the problem easier to recreated:
Another thing that "helped" review the problem is this maxCachedKeysCount = 55, When we hit the limit some keys are removed from cache, when we connect to the same key since there's no cache data is read from storage - the partial data that was written to storage Maybe we can make Also the limit of |
Ok, that's not everything, cache is getting in a partial state due to // Merge original data to cache
cache.merge(collection);
|
@marcaaron I've fixed the problem for myself locally by making // Try to free some cache whenever we connect to a safe eviction key
cache.removeLeastRecentlyUsedKeys(); LocalForage diffIndex: node_modules/react-native-onyx/lib/storage/providers/LocalForage.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/node_modules/react-native-onyx/lib/storage/providers/LocalForage.js b/node_modules/react-native-onyx/lib/storage/providers/LocalForage.js
--- a/node_modules/react-native-onyx/lib/storage/providers/LocalForage.js
+++ b/node_modules/react-native-onyx/lib/storage/providers/LocalForage.js (date 1646865700134)
@@ -7,6 +7,7 @@
import localforage from 'localforage';
import _ from 'underscore';
import lodashMerge from 'lodash/merge';
+import createDeferredTask from '../../createDeferredTask';
localforage.config({
name: 'OnyxDB'
@@ -14,6 +15,7 @@
const writeQueue = [];
let isQueueProcessing = false;
+let queueTask = null;
/**
* Writing very quickly to IndexedDB causes performance issues and can lock up the page and lead to jank.
@@ -25,16 +27,26 @@
return;
}
+ if (!queueTask) {
+ queueTask = createDeferredTask();
+ }
+
isQueueProcessing = true;
- const {
- key, value, resolve, reject,
- } = writeQueue.shift();
+ const {key, value, resolve, reject} = writeQueue.shift();
+
localforage.setItem(key, value)
.then(resolve)
.catch(reject)
.finally(() => {
isQueueProcessing = false;
+
+ if (writeQueue.length === 0) {
+ queueTask.resolve();
+ queueTask = null;
+ return;
+ }
+
processNextWrite();
});
}
@@ -61,6 +73,11 @@
* @return {Promise<void>}
*/
multiMerge(pairs) {
+ // If the write queue is pending wait and perform the multiMerge after
+ if (queueTask) {
+ return queueTask.promise.then(() => this.multiMerge(pairs));
+ }
+
const tasks = _.map(pairs, ([key, partialValue]) => this.getItem(key)
.then((existingValue) => {
const newValue = _.isObject(existingValue)
I think we're safe removing cache cleanup for the moment as it would be very hard to fill cache to levels that would crash the app We can capture a task to add better cache handling that deals only with safe eviction keys as they are the ones to watch out for |
Thanks @kidroca! Gonna start by writing some more tests in |
Hmm so as I'm writing these tests I'm realizing a few things...
Not sure if it's likely in practice, but one idea to prevent this is to make sure we always call
|
@Julesssss @kidroca pushed up a variation of the make multiMerge sync solution + automated test to try and address the weirdness in this thread. Things are running pretty smooth now (but probably will need to clean up the changes a bit still). |
Taking HOLD off here and added merge commit. @thienlnam are you able to give this a look ? 🙇 Would recommend testing this against the production API with a main Expensify account full of data to see the improvements. |
Going to test this locally and will report back |
Hellll yeah, it's much snappier 🔥 🐎 |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by @thienlnam in version: 1.1.46-0 🚀
|
Details
This is a hot fix for a
react-native-onyx
issue affecting web/desktop related to slow IndexedDB writing blocking the main thread.Fixed Issues
$ Expensify/react-native-onyx#119
Tests (tested against the production API via local proxy)
QA Steps
Screenshots
❌