[HOLD for Payment 2024-09-06][Performance] Performance regression in opening the Chat Finder page

[hannojg]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

What performance issue do we need to solve?

Our performance regression system is reporting a performance regression for opening the chat finder page between the last prod release and the current main branch:

What is the impact of this on end-users?

Slow TTI of opening the search page.

List any benchmarks that show the severity of the issue

See screenshot

Proposed solution (if any)

I opened this issue to keep track of the issue and that it needs to be investigated. I think the simplest would be to find the PR that caused the regression or to run a git bisect.

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

See screenshot.

Platforms:

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

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Version Number:latest main
Reproducible in staging?: no
Reproducible in production?: no
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation
Expensify/Expensify Issue URL:
Issue reported by:@hannojg
Slack conversation:

View all open jobs on Upwork

Issue Owner

Current Issue Owner: @bfitzexpensify

[melvin-bot]

Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext.

[hannojg]

Note: The e2e system will report every PR as a regression after the faulty PR. So these PRs were tagged as regression, although they didn't cause it:

• fix: press Tab to move around magic code input #46948
• [CP Staging] fix: Report fields - App crashes when disabling/enabling list values via checkbox & dropdown button. #47244
• [No QA] Add better logs for verifying web deploys #47275
• [CP Staging] Remove policyIDs route param when workspace is deleted #47269
• [No QA] Create trip contextual filter #47111
• Handles PAY_AT_ payment type in bookings #46606
• Support image pasting on Native (2) #47203
• fix: redirect to proper place after upgrade #46617

Unfortunately the e2e regression system didn't report the PR that introduced the regression as there was a failure for the pipeline (otherwise we could have just assumed that the first reported PR introduced the regression which isn't the case here unfortunately).

[kirillzyusko]

Me and @hannojg tend to think that regression comes from #46645

I tested locally and on my Xiaomi Redmi Note 5 Pro with these changes I have:

:arrow_right:  Significant changes to duration
 - Open Chat Finder Page TTI: 2161.024 ms → 2383.230 ms (+222.206 ms, +10.3%) :red_circle:
:arrow_right:  Meaningless changes to duration
 - Load Search Options: 270.413 ms → 348.601 ms (+78.187 ms, +28.9%) :large_yellow_circle:
 - Open chat finder page TTI (CPU): 164.812 % → 193.341 % (+28.529 %, +17.3%) :large_yellow_circle:
 - Open chat finder page TTI (FPS): 47.631 FPS → 43.349 FPS (-4.282 FPS, +9.0%)
 - Open chat finder page TTI (RAM): 255.110 MB → 263.051 MB (+7.941 MB, +3.1%)
 - Open chat finder page TTI (CPU/JS): 75.514 % → 76.175 % (+0.661 %, +0.9%)
 - Open chat finder page TTI (CPU/UI): 31.296 % → 36.074 % (+4.778 %, +15.3%) :large_yellow_circle:

And without:

Meaningless changes to duration
 - Load Search Options: 275.023 ms → 312.376 ms (+37.353 ms, +13.6%)
 - Open Chat Finder Page TTI: 2278.598 ms → 2290.360 ms (+11.762 ms, +0.5%)
 - Open chat finder page TTI (CPU): 199.153 % → 185.846 % (-13.307 %, -6.7%)
 - Open chat finder page TTI (FPS): 43.255 FPS → 44.330 FPS (+1.075 FPS, -2.5%)
 - Open chat finder page TTI (RAM): 259.071 MB → 264.679 MB (+5.607 MB, +2.2%)
 - Open chat finder page TTI (CPU/JS): 77.150 % → 76.582 % (-0.568 %, -0.7%)
 - Open chat finder page TTI (CPU/UI): 34.982 % → 34.286 % (-0.696 %, -2.0%)

@daledah do you think that you can optimize the code and improve the performance?

[melvin-bot]

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

[melvin-bot]

Still overdue 6 days?! Let's take care of this!

[daledah]

I came up with the following solution:

1. Remove map and spread operator in here:

App/src/pages/ChatFinderPage/index.tsx

Line 127 in be7cb7e

return {...report, isBold: OptionsListUtils.shouldUseBoldText(report)};

and here:

App/src/pages/ChatFinderPage/index.tsx

Line 135 in be7cb7e

data: localPersonalDetails.map((personalDetail) => ({...personalDetail, isBold: false})),

2. Set isBold property for the localPersonalDetails and recentReports from OptionListUtils.getOptions like below:

Set isBold for recentReports in here:

                continue;
            }

            reportOption.isSelected = isReportSelected(reportOption, selectedOptions);
+           reportOption.isBold = shouldUseBoldText(reportOption);

            if (action === CONST.IOU.ACTION.CATEGORIZE) {

and for localPersonalDetails here

    allPersonalDetailsOptions.forEach((personalDetailOption) => {
        if (personalDetailsOptionsToExclude.some((optionToExclude) => optionToExclude.login === personalDetailOption.login)) {
            return;
        }
+       personalDetailOption.isBold = false;

        personalDetailsOptions.push(personalDetailOption);
    });

This way we can remove 2 map and 2 spread operators, which I believe is the main cause of the poor performance.

What do you think @kirillzyusko

[kirillzyusko]

@daledah yeah, I think we can do that 👍 Feel free to submit a PR and if you want I can test everything locally to compare performance before PR gets merged 😊

[melvin-bot]

Now this issue is 8 days overdue. Are you sure this should be a Daily? Feel free to change it!

[daledah]

@kirillzyusko PR is ready.

[ikevin127]

♻️ I was auto-assigned for PR review as C+. I will handle the PR Reviewer Checklist, making sure to test on a HT account and before merge @kirillzyusko can do some performance tests on the PR to ensure the issue was fixed.

[ikevin127]

⚠️ Automation failed here -> this should be on [HOLD for Payment 2024-09-6] according to today’s production deploy from #47891 (comment). Additionally, this should also have a BZ team member assigned for payments.

cc @luacmartins

[melvin-bot]

@luacmartins, @kirillzyusko, @ikevin127, @daledah Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[ikevin127]

@luacmartins Can we please get a BZ team member assigned here for payments ? Thak you! 🙏

[melvin-bot]

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

[luacmartins]

done

[bfitzexpensify]

Offer sent @ikevin127

[ikevin127]

@bfitzexpensify Offer accepted, thank you!