[HOLD for payment 2024-07-02] [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.

Issue Owner

Current Issue Owner: @stephanieelliott

[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

[stephanieelliott]

Seems like this PR is held up on the Onyx bump.

[mountiny]

We should be good to proceed soon

[mountiny]

$250 to @allgandalf for their PR review and testing here if there are no regressions.

[melvin-bot]

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

• [NoQA] chore: bump onyx to version 2.0.52 #44065

If no regressions arise, payment will be issued on 2024-07-02. 🎊

For reference, here are some details about the assignees on this issue:

• @fabioh8010 does not require payment (Contractor)
• @allgandalf requires payment (Needs manual offer from BZ)

[melvin-bot]

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

• [@allgandalf] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@stephanieelliott] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[melvin-bot]

Payment Summary

Upwork Job

• Contributor: @fabioh8010 is from an agency-contributor and not due payment
• ROLE: @allgandalf paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@stephanieelliott)

• I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[allgandalf]

No regression test! This was a Onyx Version bump!

[melvin-bot]

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

• [NoQA] chore: bump onyx to version 2.0.52 #44065

If no regressions arise, payment will be issued on 2024-07-10. 🎊

For reference, here are some details about the assignees on this issue:

• @fabioh8010 does not require payment (Contractor)
• @allgandalf requires payment (Needs manual offer from BZ)

[melvin-bot]

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

• [@allgandalf] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@stephanieelliott] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[allgandalf]

Why melvin, This was due payment on 2nd :))))))))

[mountiny]

@stephanieelliott this is ready to pay, no regression testing required

$250 to @allgandalf

[melvin-bot]

@fabioh8010, @stephanieelliott, @mountiny, @allgandalf Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[allgandalf]

@stephanieelliott can you pay this out please, thanks

[melvin-bot]

@fabioh8010, @stephanieelliott, @mountiny, @allgandalf Huh... This is 4 days overdue. Who can take care of this?

[stephanieelliott]

Oop sorry, I was OOO and didn't reassign since I would be back before the original payment date. Paying now.

[stephanieelliott]

@allgandalf I extended an offer to you in Upwork, please accept when you get a chance: https://www.upwork.com/nx/wm/offer/103067959

[allgandalf]

I was OOO and didn't reassign since I would be back

No worries hope you enjoyed your time off, I accepted the offer

[stephanieelliott]

Thanks @allgandalf, all paid!

Summarizing payment on this issue:

• Contributor: @fabioh8010 EC, no payment needed.
• Contributor+: @allgandalf $250 via Upwork - PAID

Upwork job is here: https://www.upwork.com/nx/wm/workroom/37781860