[HOLD for payment 2023-09-29] [Performance] Improve SidebarLinks by caching getOrderedReportIDs

[mountiny]

Coming from here

[Performance] Improve SidebarLinks:

Problem

Now all the Records to show on LHN are being calculated multiple times and the logic to fill it is complex, this one is a bit trickier to solve but I think it worth it

Solution

Summary

getOrderedReportIDs is a complex function that would be good to re-architecture.
Applying just caching like I did, isn’t the full solution, it gives me a good number to know if worth taking the time to improve it, and even if we aim to get ~40% to ~50% of improvement will be awesome

[mountiny]

Asking in callstack room who will take these on

[waterim]

Hello, Im Artem from Callstack and would like to help with this one

[mountiny]

Making this daily as the getOrderedReportIDs is one of the main offenders when it comes to performance for accounts with thousands reports

[mountiny]

@waterim What are the next steps here?

[waterim]

@mountiny will update you tomorrow, mostly performance here will be improved with caching, because just 1 filter in this function is taking 90% of the whole time and its impossible to optimize this one significantly, but caching is saving a lot of time.
One more thing I just started: removing some unnecessary sorts, after a discussion in Slack.
Also removing lodash with underscore and optimising the util function shouldReportBeInOptionList for filtering all this sidabar links

[mountiny]

thanks for an update

[mountiny]

Any updates here?

[waterim]

@mountiny yes, have a progress, will open a PR today

[waterim]

@mountiny My update:
I added caching, improved the code base with removing undrescore and lodash.
But one tricky heavy thing still here, I had few calls with Eduardo and playing with a kind of "leetcode" trying to optimise the filter function which is taking another filter function and has n^2 complexity.
For now we have this filter:
const reportsToDisplay = Object.values(allReportsDict).filter((report) => ReportUtils.shouldReportBeInOptionList(report, currentReportId, isInGSDMode, betas, policies, allReportActions, true), );
And shouldReportBeInOptionList function has:
const lastVisibleMessage = ReportActionsUtils.getLastVisibleMessage(report.reportID);
And this lastVisibleMessage calculation is taking 0.2ms for each iteration(which is really heavy for a filtering)
And Im trying to optimise somehow the way we are getting lastVisibleMessage here

[mountiny]

thanks for the update

[mountiny]

Waterim has a PR up, @waterim do you think it will be ready for a review soon? I could make an adhoc build for testing

[mountiny]

Thanks for an update

[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.3.72-11 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Improvement: Sidebar Links Performance #26447

If no regressions arise, payment will be issued on 2023-09-29. 🎊

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:

• @waterim does not require payment (Contractor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

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

[melvin-bot]

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

[mountiny]

No checklist required, this is a performance improvement

@parasharrajat is due $500 for internal review

[alexpensify]

Here is the payment summary:

• External issue reporter - N/A
• Contributor that fixed the issue - N/A
• Contributor+ that helped on the issue and/or PR - @parasharrajat $500

Upwork Job: N/A

*If applicable, the bonuses will be applied on the final payment

Extra Notes regarding payment: See the notes here - #25467 (comment)

@parasharrajat please request a payment in Chat and then we can close this out. Thanks!

[alexpensify]

Ok, going to close out here.

[parasharrajat]

Payment requested as per #25467 (comment)

[JmillsExpensify]

$500 payment approved for @parasharrajat based on BZ summary.