[LOW] [Performance] Improve useOnyx and tryGetCachedValue performance

[mountiny]

Coming from here in Slack.

Problem

A while back when analysing the selection issue in Composer, where user was unable to select the text, the root cause was reported and fixed by SWM.

However, while analysing the trace we saw that there are numerous calls of useOnyx when someone taps in the composer and tries to select a text. This happens in the SuggestionMention component where we only have 1 useOnyx usage. Now, this happens because useOnyx leverages useSyncExternalStore under the hood which expects getSnapShot function as one of it’s arguments. React will call this function several times when it needs to determine whether the hook needs a re-render or not. So, this getSnapShot should be efficient and it’s job is to return the current state of the store.

Now. with that explained, the root cause was stemming from the fact that we don’t have a ready-made store which we can return from getSnapShot and to overcome this, we leverage calling tryGetCachedValue in getSnapShot function. So each time React invokes, getSnapShot the tryGetCachedValue function gets invoked as well. Now, for a collection key, this function would execute 3 loop in order to do the job, first converting Set to Array , then doing a filer and later reducing the returned Array. If you test this on a heavy account with ~15k Reports, so this approach doesn’t scale.

Solution

Now, we understand that when React repeatedly calls getSnapShot function, we shouldn’t be calling tryGetCachedValue each time and we should only call it when the store has changed. In all other cases, we should be returning the previous cached value. We can achieve this by adding a flag in our subscriber which is another argument that useSyncExternalStore expects and in our subscriber function we have Onyx.connect , so each time the callback is fired from the connection, we set a flag and in turn call tryGetCachedValue and store it to the cache.

This fixes our issue but we can do a little better. We can take some time and also improve tryGetCachedValue as it uses 3 loops for a collection key and since we are first converting Set to an Array , we can do a forEach loop on Set directly and remove the other filter and reduce loop as well. So we can do our job in 1 loop instead of 3.

We can also do a little better by only using deepEquals if key is a collection key and selector is defined. shallowEquals is enough for the other cases and will be used instead.

In the before video, we can see that we have useOnyx calls ~40 times and each time it takes around ~75ms. In after video, we only have 2-3 calls to useOnyx and now they take way less execution time. It may be possible that in after, the useOnyx calls are more than several but due to their less execution time they are not recorded or displayed.

Here’s a PR which we created together with
@Fábio Henriques
which improves the useOnyx
Here’s a PR which improves the tryGetCachedValue function, this PR also refactors other places where we are doing the same job in multiple loops.

[melvin-bot]

Triggered auto assignment to @stephanieelliott (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

[fabioh8010]

Hey 👋 I'm Fabio - expert agency contributor - and I would like to take over this issue from @hurali97

[stephanieelliott]

PR comments have been resolved, just awaiting approving review!

[mountiny]

We are waiting for previous onyx changes to be merged and deployed.

We had to revert unfortunately because some changes in the latest update caused a regression