perf(login)!: Fix slow login by delaying messenger filters initialization #20173
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (44)
|
|
@churik, @qoqobolo, @pavloburykh, @cammellos: This PR build should theoretically solve the slow login issue. As we aligned yesterday, the plan is to merge PR #20164 before this one because it reduces the time to login and it's helpful even if the solution presented in this PR ends up not working. QAs: Once you give the green light that the build solves the login problem, I'll open the PR for reviews, update status-go unit tests and coordinate with desktop if the PR is a breaking change. Just a light check on your end would help a lot. Then you can do a full QA after the PR goes out of WIP and gets dev approvals. |
ilmotta
force-pushed
the
ilmotta-20059/speed-up-login
branch
from
90b7d90
to
1c71628
Compare
PR_ISSUE 1: Cannot sync with desktop 2.29I noticed one breaking change: the sync with the desktop failed and returned an error.
We're currently checking on version 2.29. If we test the desktop builds with the same commit, it might resolve the issue. Unfortunately, we don't have a process in place to create status-desktop builds from status-go changes. |
Thanks a lot @churik. Because we'are delaying some parts of the messenger initialization (and it does prepare installations), this could be the cause of the sync failure. I will investigate the problem 👍🏼 |
PR_ISSUE 2: no redirect to the last opened channel / chatSteps:
Expected result: Actual result: OS: IOS, Android PR_ISSUE 3: after loading history chat view reloads by itslefSteps:
Expected result: Actual result: FILE.2024-05-28.15.23.37.mp4(17 sec to load, and then redirect to Status > general one more time) OS: IOS, Android Honestly, the issue where channel views take 15-18 seconds to load after login is just as noticeable as the slow login itself. We need to address this 17-second loading skeleton because it is highly visible. It's difficult to say which issue is the lesser of two evils. |
I think the new solution is definitely better if we can make it work of course. The login step is a critical part of the funnel to grow our user base. And once the user logs in, they can use many other things in the app, not just chats, whereas the current solution (in develop) simply blocks users from using anything in the app. We can (maybe) speed up how chats are processed in status-go in the future, but even if we manage to do that, it won't be as fast as not doing anything in the login (which is the new proposal). So @churik @cammellos, I think we should pursue this solution because it leads to the fastest possible login and the slow chat processing can be attacked separately. |
|
@ilmotta definitely we should load things once the user logs in, that's also in line with the fact that for 2.30 probably wallet will be the first tab, so it will be much less evident |
ilmotta
force-pushed
the
ilmotta-20059/speed-up-login
branch
from
1c71628
to
f284a23
Compare
Update on 2024-05-29
@churik, could you check this issue using the latest version from GH releases, the one I used 2.29.0-rc.6? I'm asking you that because I successfully synced using two very different desktop versions, so something tells me syncing is working, at least sometimes (which is odd, I'd expect the changes I made in status-go to either always cause a failure or not). Also, which exact desktop git revision did you use, Android version, app flags, steps to sync, etc? Just so I can more faithfully try to reproduce. Thank you! Update on 2024-05-31
|
ilmotta
force-pushed
the
ilmotta-20059/speed-up-login
branch
from
f284a23
to
b4b0141
Compare
|
@ilmotta thank you for your fixes! I have just checked ISSUE 1 and unfortunately still getting error while syncing. I have used latest (30.05) RC6 Desktop
|
ilmotta
force-pushed
the
ilmotta-20059/speed-up-login
branch
from
b4b0141
to
eeb494f
Compare
|
@ilmotta ISSUE 1 (sync error) has been fixed for me in latest builds. Great job! Will continue testing the rest on Monday, hopefully Waku issues will be fixed by that time. |
QA Notes from @ilmotta
|
14% of end-end tests have passedNot executed tests (1)Failed tests (42)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (2)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Passed tests (7)Click to expandClass TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
|
|
@ilmotta please take a look at the following issue ISSUE 4 Contact requests are not deliveredPreconditions: User A and User B both are using current PR builds. Steps:
Actual result: contact request is not delivered. cr_notDelivered.mp4 |
|
@pavloburykh I'll verify ISSUE 4, thank you! By the way, have you checked other parts of the app? Are they working fine overall? Or did you stop testing as soon as you hit ISSUE 4? |
ilmotta
force-pushed
the
ilmotta-20059/speed-up-login
branch
from
eeb494f
to
15df9e8
Compare
|
@pavloburykh, I think this WIP PR proved to be successful in fixing the slow login. I feel confident enough we can promote both the status-go and mobile WIP PRs to be non-WIP, adapt status-go unit tests and get both PRs through the normal PR process, which after all will go through a normal QA cycle in the end. For now, I think it's fine if you stop testing this WIP PR, so that your team avoids rework since you'll be testing this PR in the near future anyway once the PRs are approved in status-go and status-mobile. Thanks QA team for all the help so far o/ |
Hi @ilmotta I have tried smoke checking other parts, but due to Waku issues I was unable to check properly. Also I have stopped testing once I have faced ISSUE 4 as this was a critical issue which can be considered as a criteria to quit testing until bug is addressed. So, as you have mentioned above - we will need to perform another round of checking once PR is approved. Currently we do not have a strong opinion if we want to include this PR in 2.29 release, probably we will have to postpone until 2.30 as we already crossed the deadline for releasing 2.29 (due to lot of found blockers) and all resources are focused on finishing release testing. cc @churik Thank you for your work on this PR @ilmotta |
ilmotta
force-pushed
the
ilmotta-20059/speed-up-login
branch
from
15df9e8
to
0263170
Compare
ilmotta
changed the title
|
@ilmotta thank you for great work! No issues from my side so I am moving PR forward. One thing: please, let me know in case this issue will require fixes on go side. In this case I will need to re-test PR for regression. Also, before merging the PR it is better to make another e2e test run. I have just noticed that status - go branch is very outdated (32 commits behind develop). So we need to run e2e against rebased branch (it can be done right before you will be ready for merging). |
|
oh, we still need to get this PR reviewed...Please, let me know if there will be any changes based on PR review. In this case I will also need to perform re-test. |
|
@pavloburykh, thank you again for testing the build. Great news. Apparently desktop doesn't have regressions. I'll definitely rebase and re-run e2e before merging. If it takes more than next Tuesday to get this PR approved, then it's probably prudent to have another quick QA pass just in case. Thanks for the reminder. |
mohsen-ghafouri
approved these changes
Jun 7, 2024
smohamedjavid
approved these changes
Jun 7, 2024
ilmotta
force-pushed
the
ilmotta-20059/speed-up-login
branch
from
6834012
to
7af9c78
Compare
86% of end-end tests have passedNot executed tests (1)Failed tests (3)Click to expandClass TestDeepLinksOneDevice:
Class TestWalletMultipleDevice:
Expected to fail tests (4)Click to expandClass TestWalletOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (44)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
|
|
Hey @ilmotta! I have smoked checked the PR after rebasing, no issues from my side. Failed e2e are not PR related. Thank you again for great job! ready for merge. |
ilmotta
added a commit
to status-im/status-go
that referenced
this pull request
Jun 10, 2024
Fixes the slow login in mobile devices when users have joined large communities, such as the Status one. A user would get stuck for almost 20s in some devices. We identified that the step to set-up filters in the messenger is potentially expensive and that it is not critical to happen before the node.login signal is emitted. The solution presented in this PR is to set-up filters inside messenger.Start(), which the client already calls immediately after login. With this change, users of the mobile app can login pretty fast even when they joined large communities. They can immediately interact with other parts of the app even if filter initialization is running in the background, like Wallet, Activity Center, Settings, and Profile. Breaking changes: in the mobile repository, we had to change where the endpoint wakuext_startMessenger was called and the order of a few events to process chats. So essentially ordering, but no data changes. - Root issue status-im/status-mobile#20059 - Related mobile PR status-im/status-mobile#20173
Good thing it was found on time! I added the label |
ilmotta
force-pushed
the
ilmotta-20059/speed-up-login
branch
2 times, most recently
from
d4b6690
to
281f540
Compare
ilmotta
force-pushed
the
ilmotta-20059/speed-up-login
branch
from
281f540
to
9400c83
Compare
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #20059
Summary
This PR fixes the slow login when users have joined large communities, such as the Status one. Related status-go PR status-im/status-go#5229.
Note: This PR stayed in WIP for a while until we were sure the solution was reliable enough to deserve code reviews.
About the fix
What we mean by "slow" is that the user was getting stuck on the login screen for almost 20s in some devices (even on iOS things were bad). And this entire process was happening in status-go, hence this PR is not about optimizing the client directly.
By "login" we mean the process to authenticate and initialize vital data in status-go. Setting up message filters can be slow with large communities, and that's exactly this part we moved out of the login phase in status-go. This step now happens implicitly when the client calls
wakuext_startMessenger.In a way, the solution makes sense because setting up filters isn't essential for the user to access other parts of the app, such as the Wallet, Settings, Profile, and Activity Center.
Demo
Login goes fast now :)
fast-login.webm
Once approved, can we merge this PR?
No. This PR and the status-go one can only be merged once we get the green light from Desktop team because there could be breaking changes for them, just as there were for us.
How can we magically eliminate the login delay?
In reality, the time we used to spent during login, blocking the user, still happens, but it happens in the background and after the user is redirected to the home screen. This also means that, until the filters are established, all chats are still in their "loading skeleton state".
In terms of UX, this is probably fine as long as it doesn't take too long for this setup to finish in status-go. In the future, we have room to further optimize how filters are set up in status-go.
Steps to test
This PR was already lightly QAed, but it is recommended to check regressions in these areas:
status: ready