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.

Sign up for GitHub

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

Jump to bottom

[Performance] CMD + K is slow and laggy #3601

Closed
isagoico opened this issue Jun 15, 2021 · 28 comments · Fixed by #3669
Closed

[Performance] CMD + K is slow and laggy #3601

isagoico opened this issue Jun 15, 2021 · 28 comments · Fixed by #3669
Assignees
@Looxor @timszot @NicMendonca
Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Jun 15, 2021 •
edited by NicMendonca
Loading

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

Action Performed:

  1. While using e.cash use the cmd+k command
  2. Type on the search field

Expected Result:

App should be fast and search should be responsive as soon as the command is used

Actual Result:

Cmd + K is super slow and laggy and is locking up the whole app when used. Search functionality seems completely broken (on desktop staging

Workaround:

No workaround found.

Platform:

Where is this issue occurring?

Web
iOS
Android
Desktop App ✔️
Mobile Web

Version Number: 1.0.68-4

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:
(Unable to add video since I don't have MacOS)

Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/168172

View all open jobs on Upwork

Upwork job: https://www.upwork.com/jobs/~015673ac7d22fbbd42

From @puneetlath https://expensify.slack.com/archives/C01GTK53T8Q/p1623719515341400

ISSUE: cmd + K is super slow and laggy and is locking up the whole app when used. Search functionality seems completely broken (on desktop staging v1.0.68-4)
@MelvinBot
Copy link

Triggered auto assignment to @MariaHCD (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@MariaHCD MariaHCD added the External Added to denote the issue can be worked on by a contributor label Jun 17, 2021
@MelvinBot
Copy link

Triggered auto assignment to @jliexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@jliexpensify jliexpensify self-assigned this Jun 17, 2021
@jliexpensify
Copy link
Contributor

Hi @isagoico - I wasn't able to test this on your version, as I have 1.0.71-0. Here's a video of the search feature - it seems to be working and I don't think it's slow (personally):

2021-06-18_09-13-30.mp4

Do you think we can close this GH?

@marcaaron marcaaron assigned marcaaron and unassigned jliexpensify Jun 18, 2021
@marcaaron
Copy link
Contributor

Investigating this now.

@marcaaron marcaaron mentioned this issue Jun 18, 2021
Merged
5 tasks
@isagoico
Copy link
Author

Reopening the issue following @puneetlath feedback. Looks like the Chat Switcher is faster but it's still a bit sluggish.

The chat switcher seems to have improved, but is still pretty slow. Takes a second to open, then takes another second to filter the results based on what I’ve typed

@isagoico isagoico reopened this Jun 21, 2021
@marcaaron marcaaron removed their assignment Jun 21, 2021
@marcaaron
Copy link
Contributor

Gonna unassign myself as I can't look into further improvements atm. The chat switcher speed should be related to how many chats you have in general.

@marcaaron marcaaron added External Added to denote the issue can be worked on by a contributor and removed External Added to denote the issue can be worked on by a contributor labels Jun 21, 2021
@MelvinBot
Copy link

Triggered auto assignment to @NicMendonca (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@mallenexpensify
Copy link
Contributor

To confirm, it's a lil laggy for me too, it almost always misses the first letter I type

@NicMendonca NicMendonca added Exported Weekly KSv2 and removed Daily KSv2 labels Jun 22, 2021
@MelvinBot
Copy link

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

@Looxor
Copy link
Contributor

Looxor commented Jun 22, 2021

Reason: Too many re-renders
As you see the following screencast, there are too many re-renders every time when I type "Concierge".

There are 2 workarounds

  • Need to use shouldComponentUpdate
    OptionsList component is a good example.
    The component includes "shouldComponentUpdate" on itself and it does not make re-render as long as "props.sections" is not changed.

  • Need to avoid changing the state in the main body of the components
    Here is a structure.
    SearchPage -> OptionsSelector -> OptionsList
    Here, OptionsSelector does not need to have an event onChangeText.

In conclusion, however, I recommend 2nd one because OptionsSelector component does not need to have the onChangeText which is a root of all issues.
In this case, OptionSelector can be an unconditional component because "props.sections" is not a data which changes by any user input.

Thibault LAURIAONO
(I post this after reading the job description on Upwork)

issue-on-expensify.mp4

@tgolen
Copy link
Contributor

tgolen commented Jun 23, 2021

@Looxor Thanks for the proposal! Could you please clarify a couple of things for me?

Reason: Too many re-renders

Can you please quantify this somehow (eg. how many re-renders are happening, and what number are you trying to optimize for?)

OptionsSelector component does not need to have the onChangeText

I'm not really following this because I see that onChangeText is used by the parent SearchPage.js, so I don't see how it needs to be removed. Can you add some more details about that?

OptionSelector can be an unconditional component because "props.sections" is not a data which changes by any user input.

I don't understand this either. Primarily, I'm not sure what "unconditional component" means and I'm also not sure what props.sections has to do with the problem (I thought onChangeText was the problem).

@Looxor
Copy link
Contributor

Looxor commented Jun 24, 2021

#1
Reason: Too many re-renders
Can you please quantify this somehow (eg. how many re-renders are happening, and what number are you trying to optimize for?)

For more convenient, I made a diagram to show you the result.
Please look at the following pic.
expensify-answer-01

#2
OptionsSelector component does not need to have the onChangeText
I'm not really following this because I see that onChangeText is used by the parent SearchPage.js, so I don't see how it needs to be removed. Can you add some more details about that?

I made a simple screencast for you to show how to do that.
I removed onChangeText on SearchPage's OptionsSelector.

expensify-answer-02.mp4

#3
OptionSelector can be an unconditional component because "props.sections" is not a data which changes by any user input.

I don't understand this either. Primarily, I'm not sure what "unconditional component" means and I'm also not sure what props.sections has to do with the problem (I thought onChangeText was the problem).

I am sorry for my typo. I meant the uncontrolled component.
As you can see the video above, TextInputWithFocusStyles does not have a prop named "value" on OptionsSelector.js.
It means that "TextInputWithFocusStyles" is not controlled by React and we refer its value only by using onChangeText on itself.

Thibault LAURIANO

@Looxor
Copy link
Contributor

Looxor commented Jun 24, 2021

I upload this screencast again because above one is crashed.

expensify-answer-02.mp4

@tgolen
Copy link
Contributor

tgolen commented Jun 24, 2021

OK, thanks for the additional information. I think I understand what the proposal is now. It was a little difficult seeing what code changes were being made in the video. In the future, it would help to either link directly to a diff of the code changes you are proposing, or to write out the specific changes that would be made (just to help you write better proposals).

Looks like the rendering is indeed decreased, but it's hard to know if that will totally fix the problem. It's probably a good place to start though and we can test it out with larger data sets once it's been deployed.

🟢 to hire @Looxor for this one.

@NicMendonca
Copy link
Contributor

@Looxor hired on Upwork 🎉

Looxor added a commit to Looxor/Expensify.cash that referenced this issue Jun 25, 2021
@Looxor
Fix Desktop - CMD + K is slow and laggy Expensify#3601
2d74cbc
@Looxor Looxor mentioned this issue Jun 25, 2021
Merged
5 tasks
thienlnam added a commit that referenced this issue Jun 29, 2021
@thienlnam
Merge pull request #3760 from Looxor/thibault-FixSearchPageSlowIssue(…
7299d6c 
…CMD+K)

Fix Desktop - CMD + K is slow and laggy #3601
nickmurray47 added a commit that referenced this issue Jun 30, 2021
@nickmurray47
Revert "Fix Desktop - CMD + K is slow and laggy #3601"
5eccf99
tgolen added a commit that referenced this issue Jun 30, 2021
@tgolen
Merge pull request #3824 from Expensify/revert-3760-thibault-FixSearc…
312e645 
…hPageSlowIssue(CMD+K)

[CP Staging] Revert "Fix Desktop - CMD + K is slow and laggy #3601"
github-actions bot pushed a commit that referenced this issue Jun 30, 2021
@tgolen
Merge pull request #3824 from Expensify/revert-3760-thibault-FixSearc…
10ddb05 
…hPageSlowIssue(CMD+K)

[CP Staging] Revert "Fix Desktop - CMD + K is slow and laggy #3601"

(cherry picked from commit 312e645)
@nickmurray47
Copy link
Contributor

@Looxor the PR created several regressions that were linked in this PR #3824 that reverted your original one. Can you please submit a follow-up PR if you'd still like to complete this one?

github-actions bot pushed a commit that referenced this issue Jun 30, 2021
@tgolen
Merge pull request #3824 from Expensify/revert-3760-thibault-FixSearc…
80c6451 
…hPageSlowIssue(CMD+K)

[CP Staging] Revert "Fix Desktop - CMD + K is slow and laggy #3601"

(cherry picked from commit 312e645)
@Looxor
Copy link
Contributor

Looxor commented Jul 7, 2021

@nickmurray47 , Can you please provide a demo user credential to check "Search" and "New chat" with many users?
I have one, but it's only one user and I have no idea to create members for testing.
Thank you.

@nickmurray47
Copy link
Contributor

@Looxor can you please post the same question in the #expensify-open-source channel first? Someone there might already have set up an account for you to use. If no one is available to assist then I can help from there. Thanks.

@Looxor
Copy link
Contributor

Looxor commented Jul 8, 2021

@nickmurray47 ,
I didn't join the Slack channels so I sent an email "Slack Channel Invite" to contributors@expensify.com (as mentioned in guideline).
But I didn't get any invitation from there.
Can you please check this for me?

@nickmurray47
Copy link
Contributor

@Looxor just saw your request and I made the Slack Channel invite request on our end. You should be in the channel shortly. Let me know if you have any other questions.

@Looxor
Copy link
Contributor

Looxor commented Jul 10, 2021

Dear @nickmurray47 ,
It seems that I found a way to solve this problem without reverting original sources.
But, can you please provide me your testing demo account?
I need to confirm your symptom on my side again.
I tried to update local storage for moc data, and there was no problem on my side.

This was referenced Jul 10, 2021
Closed
Closed
@Looxor
Copy link
Contributor

Looxor commented Jul 13, 2021

Hi @jliexpensify,
I am investing in this issue, and it appears to be slow on my side.
Can you please provide me the credentials (email/password) which are used for this demo?
#3601 (comment)

@NicMendonca
Copy link
Contributor

@Looxor that appears to be @jliexpensify's actual account, so he's not going to be able to share those credentials with you.

Can you please describe what is blocking? Is it a demo account?

@Looxor
Copy link
Contributor

Looxor commented Jul 13, 2021

Hi @NicMendonca , thank you for your reply.
Oh, is it an actual one? I thought it was a demo account because the video showed the staging version app.
Anyway, I need to test the app using a bunch of users, but for me, only one user (Conceige) showed up.

@NicMendonca
Copy link
Contributor

@Looxor thanks for that context! Can you please open up this dialogue in #expensify-open-source to see if someone has credentials or a workaround for you?

@timszot
Copy link
Contributor

timszot commented Jul 13, 2021

Left a comment on the PR, I ran some tests using my account so we can evaluate from there.

@marcaaron marcaaron changed the title Desktop - CMD + K is slow and laggy [Performance] CMD + K is slow and laggy Jul 14, 2021
@NicMendonca
Copy link
Contributor

This issue is not longer a priority. Ended contract in Upwork. Thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Looxor Looxor

@timszot timszot

@NicMendonca NicMendonca

Labels
Engineering External Added to denote the issue can be worked on by a contributor Weekly KSv2
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Debounce search page and reduce number of tooltips created Expensify/App