[Performance] Onyx.clear() takes a long time and can leave stale data in storage

[tgolen]

What performance issue do we need to solve?

When Onyx.clear() takes a long time to work, and if the browser is refreshed or closed during the process, it can leave stale data in storage.

What is the impact of this on end-users?

The next user that signs in can then see data from the previous user.

List any benchmarks that show the severity of the issue

TBD

Proposed solution (if any)

Provide a better mechanism to ensure Onyx is cleared fully before redirecting users to the sign out page. Possibly put something in place that prevents the browser from being refreshed while clear() is happening. Also possibly speed up the time it takes for Onyx.clear() to work.

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

TBD

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Previous mentions of bugs caused by this:

• [$2000] BUG: unread indicator count is not reset after logout reported by @parasharrajat #11671
• [$2000] BUG: unread indicator count is not reset after logout reported by @parasharrajat #11671 (comment)
• [Bug] App shows chats list from another user - reported by @0xmiroslav  #11901
• Bug: Endless spinner appears when Sign out and sign in back again  #12055

[tgolen]

cc @fedirjh

[tgolen]

Note to self: there is a plugin for LocalForage which provides multiRemove() functionality that could be quicker than the for loop we are using now to call removeItem() for each key needing removed.

[tgolen]

I'm going to move this to be weekly since I need to do some performance testing on it and we've got a busy week in the office.

[fedirjh]

@tgolen I've captured some metrics for localForage methods : setItem , removeItem and clear , results show :

1. Very huge difference between Chrome and Safari
2. Clear method is faster than multiple removeItem
3. removeItem and setItem are so slow on chrome

note: These tests are related to localForage and not Onyx , this to prove that performance issue is related to localForage and not Onyx

Method	Records	Chrome	Safari
setItem	1K	4926.37 ms	142.10 ms
removeItem	1K	4858.75 ms	155.44 ms
clear	1K	15.21 ms	0.79 ms
setItem	4K	20054.76 ms	608.83 ms
removeItem	4K	19988.10 ms	444.84 ms
clear.	4K	38.94 ms	2.07 ms

Problem

From above results we can tell that Onyx.clear performance issue on chrome is due to removeItem which is taking much time to clear storage . Users usually close the browser tab right after logout (when they see the login page).
If user refresh or close the tab after logout, in this case Onyx.clear will be interrupted and will cause data persist in storage causing above issues.

Solution

We can use localForage.clear to clear data instead of using multi removeItem , I did checked Onyx implementation and found that we have this limitation

    // If we just call Storage.clear other tabs will have no idea which keys were available previously
    // so that they can call keysChanged for them. That's why we iterate and remove keys one by one
    this.clear = () => Storage.getAllKeys()
        .then(keys => _.map(keys, key => this.removeItem(key)))
        .then(tasks => Promise.all(tasks));

@kidroca is this the only limitation for using localForage.clear directly , or there are other limitations ? if this is the only one I can provide another solution for this limitation and we can use localForage.clear directly .

[tgolen]

@fedirjh Thanks for that initial investigation. There has been a string of problems that we have run into which makes LocalForage.clear() not a valid option for us.

Primarly, it's because there are a small handful of keys that we don't want to be removed (preferred locale for example). What we were doing before was:

1. Remember the locale version
2. Clear Onyx
3. Re-set the locale version

This unfortunately, leads to a flash of text as it changes back and forth from different languages.

The idea then is that we should try to clear every key except the ones we want to keep. That's why it uses setItem() or removeItem().

I think we should continue down this path rather than going back to trying to use LocalForage.clear().

What I would like to do next is see about implementing the removeItems() plugin for LocalForage here: https://github.com/localForage/localForage-removeItems to see if that will speed up things to give us the same performance as clear()

[fedirjh]

Primarly, it's because there are a small handful of keys that we don't want to be removed (preferred locale for example).

@tgolen I checked that , you mean the defaultKeys right ? we can pass default keys to the webStorage clear method and reset the keys after clear is done , I think Onyx relies much on cache so this should be fine if we clear storage (not cache) then reset those keys.

[tgolen]

defaultKeys also needs to be accounted for, yes. What I am talking about are these here:

https://github.com/Expensify/App/blob/main/src/libs/actions/SignInRedirect.js#L40-L43

[tgolen]

You might be right though that once the cache is updated with the correct values, we can clear onyx and then reset the keys (at the bottom of the Onyx.clear() method here). Is that what you're saying?

[fedirjh]

Yes that's the point , we only update the clear method to reset keys after clear

   return Storage.clear(defaultKeyValuePairs);

and in here

this.clear = (keysToDefault) => Storage.clear().then(() => Storage.multiSet(keysToDefault));

[tgolen]

OK, got it, thanks! How would you update that to account for the values I pointed to (just so I am clear on what you are proposing)?

[fedirjh]

I think those values are stored locally before we call Onyx.clear . we are not making any changes to the cache so it have same behavior, also I think even if cache updates this block will prevent any data changes

    const activeClients = currentActiveClients;
    const preferredLocale = currentPreferredLocale;
    const isOffline = currentIsOffline;
    const shouldForceOffline = currentShouldForceOffline;

[tgolen]

OK, I don't think I'm totally following you, but regardless, I've tried this:

return Storage.clear()
                .then(() => Storage.multiSet(keyValuesToPreserve))

and it appears to be several times slower (like 2-3x) than using Storage.multiRemove() (which is a loop that is calling removeItem() on each key).

So maybe there is something that I'm not quite grasping with the timing of Storage.clear()

[fedirjh]

@tgolen I think you missed my comment , here is diff

Changes in Onyx.js

-            return Storage.multiSet(keyValuesToReset);
+            return Storage.clear(defaultKeyValuePairs);

Changes in WebStorage.js

-       this.clear = () => Storage.getAllKeys()
-           .then(keys => _.map(keys, key => this.removeItem(key)))
-           .then(tasks => Promise.all(tasks));
+       this.clear = (keysToDefault) => Storage.clear().then(() => Storage.multiSet(keysToDefault));

[tgolen]

Ah, thank you for the clear diffs. That makes more sense now! I keep forgetting of WebStorage.js. I'm wondering if I'm still doing something wrong though, because with ~4000 reports, it still takes ~35 seconds to complete Onyx.clear(). You can see my branch here in this WIP https://github.com/Expensify/react-native-onyx/pull/220/files (ignore the multiRemove() stuff for now).

I'm timing this in App with this branch: #13886

[fedirjh]

@tgolen can you set this account ( fedidev+ht@gmail.com ) for 4K reports ? I did tested with 1K account and results are ~200 ms

[tgolen]

Hm, I don't think I can. I'm just testing this in my local environment. I can send you credentials for a high traffic account on staging through. Please send me a DM in slack to @tgolen

…

On Mon, Jan 2, 2023 at 4:34 PM Fedi Rajhi ***@***.***> wrote: @tgolen <https://github.com/tgolen> can you set this account ( ***@***.*** ) for 4K reports ? I did tested with 1K account and results are ~200 ms — Reply to this email directly, view it on GitHub <#13884 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMAB4WAMODHUEKSN2LSFDWQNXYDANCNFSM6AAAAAATMGUI7U> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[tgolen]

OK, the results are looking pretty good now. It appears that the times when it takes a while depends on how quickly you sign out after signing in. For these times that are high, I am signing out about 2-3 seconds after signing in. There must just be a lot of data being put into Onyx which slows things down. If I wait about 15-20 seconds before signing out, then it is very fast to clear.

[tgolen]

Unfortunately, even with this improvement, if you signout quickly, and refresh (before the timing event has fired) then you still end up with report data in Onyx 😞 But, I guess this is still better than what we had before, even if it doesn't fully solve the problem.

[melvin-bot]

Triggered auto assignment to @davidcardoza (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[tgolen]

@davidcardoza I think you can ignore some of the above. I just needed to assign it to BZ so that @mollfpr could get paid (I think)

[melvin-bot]

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

[tgolen]

bump @davidcardoza

[davidcardoza]

@mollfpr I extended a contract to you via Upwork
https://www.upwork.com/jobs/~012e3f7c4d43a1722c

[mollfpr]

@davidcardoza Accepted, thank you! Also, is a bonus apply to internal PR review?

[fedirjh]

@tgolen am I not eligible for compensation here ?

[tgolen]

Oh, maybe I am a little confused here. @mollfpr why are you requesting payment for this? I don't see you providing any proposals or helping out at all.

@fedirjh I am fine with you having some compensation for your comments in this GH which were helpful for me in writing the solution. I am OK with compensating you with $250 for your help.

[mollfpr]

@tgolen Sorry for the confusion, I got assigned to review the PR.

[tgolen]

Oh, right. I was only looking at the react-native-onyx PR: Expensify/react-native-onyx#220. I see you reviewed the PR for App 👍

[davidcardoza]

Thanks for flagging @fedirjh I should have caught your contribution. My fault I will process the contract in Upwork.

[dylanexpensify]

@mollfpr sent payment!

@fedirjh sent offer!

[dylanexpensify]

@davidcardoza mind tackling payment to @fedirjh when they accept?

[MelvinBot]

@tgolen, @davidcardoza, @mollfpr 12 days overdue. Walking. Toward. The. Light...

[davidcardoza]

@fedirjh did you accept? I am having trouble locating the offer sent by @dylanexpensify

[fedirjh]

@davidcardoza yep accepted

[davidcardoza]

Hmmm...Can you link me to the post you accepted? I am not seeing you in the hired or proposal sections of the job posting
https://www.upwork.com/jobs/~012e3f7c4d43a1722c

[mollfpr]

@davidcardoza That’s my Upwork profile 😅

I’ll already get the payment from Dylan.

[davidcardoza]

Sorry my comment comment was for @fedirjh you can ignore.

[davidcardoza]

great, closing this out then!

[fedirjh]

@davidcardoza the offer is accepted but payment isn't yet issued 😅

[davidcardoza]

Payment sent.