[HOLD for payment 2023-11-30] [HOLD for payment 2023-11-01] Refactor heavy operations when user starts to type as main thread hangs on 4x and 6x cpu throttling

[mountiny]

coming from here

Proposal

Refactor heavy operations when user starts to type as main thread hangs on 4x and 6x cpu throttling

Problem

As soon as the user starts to type, the main thread gets blocked for a noticeable amount of time and user can’t perform any other action. The reason the main thread gets blocked is due to heavy JS operations. Using Chrome’s profiler on new HT accounts, we can see that there are many pieces that comes together and halts the main thread. Details of them are below:

• Right now, we are updating a flag hasDraft in reports_ collection and as a result SidebarLinksData gets re-rendered. The main issue arises in Report.setReportWithDraft as it’s calling Onyx.merge as a result of which chatReports gets recreated. This whole process is what’s mainly causing the issue in blocking the main thread. If we comment this Report.setReportWithDraft everything seems to work fine.
• We also have UnreadIndicatorUpdater which listens to reports_ collection then updates the read count on relevant platforms, without checking if it’s already read or not. And since, we are updating hasDraft in above step, reports_ collection gets updated and this is fired, each time hasDraft is toggled or each time reports_ is updated.
• getOrdererdReportIDs still have some heavy operations including JSON.Stringify . In the code, we are passing in allReportDict and it’s basically all the key/ value pair for reports and stringifying them along with other 5 parameters is a heavy task and it takes around 25-30 ms itself.
• JS utility functions like lodashGet really gets slower when CPU has limited availability, in our case testing this with CPU throttling.
• String.trim appears to be consuming more milliseconds than necessary. It is used to trim the whitespaces around message. In my case on new HT accounts, I wasn’t able to see any message on the screen because of maybe lots of data but this function was still being called and took around 33 ms.
• There’s also some interesting thing about String literals or template literals, they tend to consume more milliseconds than the plain old concatenation using a plus operator :thinking_face:
• In getOrderedReportIDs we are looping over reportsToDisplay two times and this can be combined into one.

## Solution

There’s a lot going on in only on initial letter typing. To get near to an ideal performance, I tried to apply the following solutions to fix the above findings:

• We can have a separate object in Onyx which would contain the draftReportIDs , so this way we can avoid using Onyx.merge on a heavy collection and use this lighter one. Also, we can avoid re-rendering the SidebarLinksData totally with this approach, since we won’t be updating chatReports now and to keep the draftReportIDs lighter we can add reportID to it if there’s a draft and remove the reportID , when there’s no draft. And since, it will be an object we won’t have to iterate over and we can just get the draft status using draftReportIDs[reportID] .
• The simplest solution is to check whether the previous read count is equal to the upcoming one, if it’s equal we will not update unread status.
• Instead of passing in whole allReportDict we can pass only the keys, which is basically the reportID to JSON.Stringify and doing so reduces the time by 15-20 ms.
  • Only issue I saw with this was now drafts are not sorted in LHN each time we start to type. This is something we can fix later.
• Replacing loadshGet with typically accessing properties only in places which were taking noticeable milliseconds.
• For String.trim I decided to apply this method only to those messages who has some data and not every other message.
  • On latest HT accounts, this was the specific case and doing so resulted in saved milliseconds.
• For String literals, I replaced only the places which were consuming more milliseconds. I replaced them with typical way of concatenating two strings.
• We are looping over reportsToDisplay twice, so I refactored and combined the operations in one loop.

If we look at before and after results, we save about 400 milliseconds 👏

Testing Environment:
Web at 6x CPU throttling on profiling build with production mode
Macbook M1 Pro

Note:
I haven’t tested this on Mobile and Desktop platforms, so this should also be tested when implementing this proposal.

[melvin-bot]

Triggered auto assignment to @kevinksullivan (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

[hurali97]

@mountiny Can you assign this to me ?

[sebryu]

I've posted another proposal that resolves this issue, but instead of creating new onyxKey, I rely on existing onyxKey, but with changed onyx selectors. Moreover I've changed withReportCommentDrafts to regular withOnyx call in OptionRowLHNData to make sure components will subscribe only to resources which they relay on.
Link to PR: #30031

[mountiny]

Thanks!

[mountiny]

I will keep this open for @hurali97 to add the further improvements

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[mountiny]

Had ro be reverted for regression and next week we can address that

[melvin-bot]

@mountiny, @muttmuure, @hurali97, @robertKozik Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[muttmuure]

Not overdue

[mountiny]

I believe we had to revert the PR @hurali97 would you be able to create it again and address the regression there?

[mountiny]

@hurali97 Any update on this one? Thanks!

[melvin-bot]

@mountiny, @muttmuure, @hurali97, @robertKozik Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@mountiny, @muttmuure, @hurali97, @robertKozik Whoops! This issue is 2 days overdue. Let's get this updated quick!

[melvin-bot]

@mountiny, @muttmuure, @hurali97, @robertKozik Whoops! This issue is 2 days overdue. Let's get this updated quick!

[mountiny]

Asking if someone else can help here

[hurali97]

I am taking a look at this !

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

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

• perf: improve heavy operations when user starts to type #31295

If no regressions arise, payment will be issued on 2023-11-30. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

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

• @hurali97 does not require payment (Contractor)
• @robertKozik does not require payment (Contractor)

[melvin-bot]

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

• [@robertKozik] The PR that introduced the bug has been identified. Link to the PR:
• [@robertKozik] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@robertKozik] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@robertKozik] Determine if we should create a regression test for this bug.
• [@robertKozik] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@muttmuure] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[mountiny]

Performance improvement handled by agencies, can close