Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-04-15] [Performance] Implement caching of list options #37878

Closed
muttmuure opened this issue Mar 7, 2024 · 15 comments
Closed
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2

Comments

@muttmuure
Copy link
Contributor

Problem

When opening the search page and calling getOptions function, which is responsible for generating a list of options based on specific parameters and conditions, it loops through all the reports and personal details. The most expensive function that is called on each item is createOption - it fills almost all the time of getOptions.
These calculations are done from scratch each time any search page utilizing getOptions is being opened. It means each time Search page is getting opened, user has to wait up to a few seconds to get a list of options. Additionally, the bigger input of getOptions is, the slower the function gets.

Solution

We can implement a solution where all available options are generated only once. They can be shared across all searchable lists, like Search page, New chat, Invite to workspace etc by using React Context. The only thing needed when opening the search page is actually filtering the list based on options currently passed to getOptions.

Options should be generated while opening any search screen for the first time. It means if the screen is opened for the first time, it would display a skeleton first and display options once they are generated, but every next time it will just filter options and display them almost instantly.

Options list will be updated automatically in the background whenever personal details or reports change. Since calling getOptions is costy, we should instead retrieve the updated report ID, create just a single option instead of generating all of them and update it in the options array.

Some results I was able to achieve with PoC:

1st opening of the Search Page: Before: ~4500ms → After: ~4500ms -> no change
2nd and each subsequent opening of the Search Page: Before: ~4500ms → After: ~380ms -> ~12x faster
Update of a single option in an option list: ~0.3ms

from: @TMisiukiewicz here

@muttmuure muttmuure moved this from HIGH to CRITICAL in [#whatsnext] #quality Mar 7, 2024
@muttmuure muttmuure moved this from CRITICAL to HIGH in [#whatsnext] #quality Mar 7, 2024
@TMisiukiewicz
Copy link
Contributor

can I be assigned to this issue?

@TMisiukiewicz
Copy link
Contributor

TMisiukiewicz commented Mar 13, 2024

Update: I have opened a draft PR, I just need to adjust a few things tomorrow morning - e.g. add required comment for props etc. I am keeping an eye on #37909 as well, because whatever gets merged first, it will require some changes in the second PR. It should not be something time-consuming anyway. I also need to take care of a failing tests. I'll prepare a PR for opening once I finish all the work that is left. A quick demo of how cached options work on my Android device:

Record_2024-03-13-12-22-27.1.mp4

@TMisiukiewicz
Copy link
Contributor

Update: unit tests should be fixed now, I'm gonna take a look on reassure tests on Monday, this and some cleanup on branch are the only things left before it's ready for review

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Monthly KSv2 labels Mar 21, 2024
@TMisiukiewicz
Copy link
Contributor

PR #38207 has an ongoing C+ review

Copy link

melvin-bot bot commented Mar 27, 2024

Triggered auto assignment to @grgia, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@hungvu193
Copy link
Contributor

@grgia Since I reviewed the PR, can you assign me this issue? Thank you 😄

Copy link

melvin-bot bot commented Apr 4, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

Copy link

melvin-bot bot commented Apr 5, 2024

⚠️ Looks like this issue was linked to a Deploy Blocker here

If you are the assigned CME please investigate whether the linked PR caused a regression and leave a comment with the results.

If a regression has occurred and you are the assigned CM follow the instructions here.

If this regression could have been avoided please consider also proposing a recommendation to the PR checklist so that we can avoid it in the future.

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Apr 8, 2024
@melvin-bot melvin-bot bot changed the title [Performance] Implement caching of list options [HOLD for payment 2024-04-15] [Performance] Implement caching of list options Apr 8, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label Apr 8, 2024
Copy link

melvin-bot bot commented Apr 8, 2024

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

Copy link

melvin-bot bot commented Apr 8, 2024

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

If no regressions arise, payment will be issued on 2024-04-15. 🎊

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Apr 14, 2024
Copy link

melvin-bot bot commented Apr 15, 2024

Issue is ready for payment but no BZ is assigned. @slafortune you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

Copy link

melvin-bot bot commented Apr 15, 2024

Payment Summary

Upwork Job

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

BugZero Checklist (@slafortune)

  • 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

@slafortune
Copy link
Contributor

@hungvu193 - offer sent here -https://www.upwork.com/nx/wm/offer/101887481

@hungvu193
Copy link
Contributor

@hungvu193 - offer sent here -https://www.upwork.com/nx/wm/offer/101887481

I've just accepted. Thanks.

@slafortune
Copy link
Contributor

Contributor: @TMisiukiewicz is from an agency-contributor and not due payment
ROLE: @hungvu193 paid $500 via Upwork

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Daily KSv2
Projects
Development

No branches or pull requests

5 participants