-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Improve getSearchText perf #40193
Improve getSearchText perf #40193
Conversation
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@kbecciv Would you be able to test this build and see if the performance issues you experienced are fixed? |
@jayeshmangwani Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@janicduplessis Do we need to test this issue #39864 in all environments? |
I think if you just test the environment where you saw the issues initially is fine. |
@janicduplessis QA team retested the issue in Android app with link provided above and issue is not reproducible! screen-20240416-152244.1.mp4 |
Awesome, thanks! |
@mountiny I think this is ready for final review / merge. |
@jayeshmangwani can you please complete the checklist here? |
@mountiny Yes sure, completing the checklist in 30 minutes. |
@mountiny @janicduplessis I had a strange issue when I first ran the app in iOS. I could see an empty user, but once I signed out and logged in again, the issue was gone, so I'm not sure if the problem is with this PR or some cache-related issue. |
@kbecciv Can we get tested on Android mobile web? I am getting a lag in Android Chrome, but I am not sure if it's for just my device; Android-mWeb.mp4 |
Reviewer Checklist
Screenshots/VideosAndroid: NativeAndroid.mp4Android: mWeb ChromeAndroid-mWeb.mp4iOS: NativeiOS.moviOS: mWeb SafarimWeb-ios.movMacOS: Chrome / Safariweb.movMacOS: Desktopdesktop.mov |
Please ignore this comment ,tested with another device and it is working like staging |
@jayeshmangwani Maybe it is an existing bug? I didn't really change the code that does the search, it just removes the check so users are always added to the search instead of only when |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.67-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.67-7 🚀
|
1 similar comment
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.67-7 🚀
|
Details
When working with very large number of chats with many participants the perf of getSearchText becomes really bad. It comes from the usage of concat to add search terms for each participant. This becomes a lot slower than using array.push when the array is big since concat must copy the array.
When testing with chats with 1000 participants this lead to about a 10x performance improvement. This should be enough to be able to re-enable searching for chats by participant that was reverted in #40019.
I also noticed that we seemed to add participants twice in certain cases now that we always add participants this branch is no longer needed as it would just be duplicate work.
Fixed Issues
$ #39864
PROPOSAL:
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop