Change view-id as per the selected bottom tab and refactor subscriptions #15636
Conversation
Jenkins BuildsClick to see older builds (24)
|
| @@ -1,38 +1,20 @@ | |||
| (ns status-im2.subs.home | |||
There was a problem hiding this comment.
Maybe we should rename home namespace to chats or some other more informative name
33% of end-end tests have passedNot executed tests (26)Failed tests (2)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Passed tests (1)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
|
Parveshdhull
force-pushed
the
feat/bottom-tabs-view-id
branch
from
0d2cdf5
to
18d941d
Compare
|
Hi @rasom, Please take a look at 18d941d |
|
@Parveshdhull in order to test it I need to open communities from discovery screen and currently app crashes when I'm trying to open that discovery screen |
|
Thank you for finding this issue, should be fixed now |
|
@Parveshdhull yep I can confirm it fixes #15609 |
Thank you very much for finding the problem and testing. |
| :<- [:view-id] | ||
| (fn [[stacks view-id]] | ||
| (if (= view-id :shell) | ||
| (let [sorted-shell-cards (sort-by :clock > (map val stacks))] |
There was a problem hiding this comment.
@Parveshdhull can we do it in event handler on fetching cards instead of using atom? Or on handler which changes those stacks or something. If we need some pre calculated data I suppose it should happen in event handler.
There was a problem hiding this comment.
Yes, it makes sense. will change it
| @@ -1,38 +1,20 @@ | |||
| (ns status-im2.subs.home | |||
| (:require [re-frame.core :as re-frame])) | |||
|
|
|||
| (def memo-home-items (atom nil)) | |||
| (def memo-chats-stack-items (atom nil)) | |||
There was a problem hiding this comment.
by all means computation function in reg-sub should be a pure function, that cashing likely shouldn't happen in subs layer
There was a problem hiding this comment.
should be fixed now
There was a problem hiding this comment.
Moved caching to the view itself instead of subscription. But looks like this slightly increase render time
Caching in subscription render time
Caching in view render time
There was a problem hiding this comment.
probably render is also not the right place to memoize, ideally we use the re-frame subscription mechanism that already uses a cache internally?
There was a problem hiding this comment.
For now keeping the caching in the subscription itself, due to performance. We can look more into this later on how to improve it.
There was a problem hiding this comment.
Hey @cammellos, @rasom. @Parveshdhull recently mentioned this problem to me. I know it's been a while already, but here are my thoughts/findings. No action to be taken, just thoughts. This is going to be a long comment 🤷🏼
The code in develop is still basically the same as when this PR was merged, so I used it as a basis to make the analysis.
we want to keep data unchanged so react doesn't change component when we leave screen
In develop, when I switch between screens (1:1 chats, AC, bottom tabs, etc), the subscription :chats-stack-items always re-runs because it depends on the signal :view-id. The cost of this subscription when there's a cache hit in memo-chats-stack-items is fairly small, but it's not zero.
In develop, if we ignore the manual caching mechanism performed with memo-chats-stack-items, then the subscription :chats-stack-items is essentially a take call.
(re-frame/reg-sub
:chats-stack-items
:<- [:chats/home-list-chats]
:<- [:view-id]
:<- [:home-items-show-number]
(fn [[chats view-id home-items-show-number]]
(take home-items-show-number chats)))And upon further inspection, we see that the view layer does more computation on the results of this subscription.
;; in component home.view/chats
(let [unfiltered-items (rf/sub [:chats-stack-items])
items (filter-and-sort-items-by-tab selected-tab unfiltered-items)]
...)Therefore, the whole extra computation in filter-and-sort-items-by-tab is not being cached by re-frame.
After I moved all the computation to the subscription (including the current tab argument, this is important), then when I switch between bottom tabs or other screens, re-frame doesn't re-run the subscription the component home.view/chats never re-renders.
After switching screens, we get a hit in re-frame's subscription cache, which shows that when leaving or entering the Messages home screen nothing is re-computed or re-rendered. Obviously, if the user changes between Recent or Groups tabs, then the subscription will be re-computed and the view re-rendered.
(re-frame.core/dispatch [:navigate-to :my-profile])
(re-frame.subs/cache-lookup [:chats-stack-items :tab/recent])
;; => still returns cached chats which means nothing is re-rendered or sub re-computed.The new subscription looks like this:
(re-frame/reg-sub
:chats-stack-items
:<- [:chats/home-list-chats]
:<- [:home-items-show-number]
(fn [[chats home-items-show-number] [_ tab]]
(let [k (if (= tab :tab/groups) :group-chat :chat-id)]
(->> chats
(take home-items-show-number)
(filter k)
(sort-by :timestamp >)))))Here's the example video showing the new sub at play:
device-2023-08-10-123339.webm
I measured with re-frisk and the new code delivers the same level of performance. The differences I've seen in my comparisons are more like random fluctuations, which makes me think this manual caching mechanism is not needed.
The upsides of the new code are:
- The subscription doesn't re-run on screen changes throughout the app.
- It is more idiomatic re-frame (no mutation in subscription).
- It's simpler and the whole computation can be unit tested (because it's not split between sub & view).
Side note: I micro-benchmarked the new subscription and the most expensive part is sorting, and because the collection is rather small for the value of home-items-show-number (e.g. 20), using a transducer didn't help because there is a negligible cost to garbage collect intermediary results. If the data coming through was already ordered by timestamp we could consider skipping sorting?
But this sorting detail is in the range of micro optimizations. I would say optimizing the individual render item component chat-list-item/chat-list-item can bear more fruit. For example, it unfortunately creates subscriptions for every item of the flat list; it passes the whole chat instance to the notification component which makes Reagent's life more difficult to diff, and so on.
Also, as far as I know, re-frame doesn't offer any mechanism to control when & how its query-cache is evicted (see https://github.com/day8/re-frame/blob/master/src/re_frame/subs.cljc#L46). One technique that's worth trying is playing with reg-sub-raw and the on-dispose callback. Sounds like a hack, but it's interesting nonetheless.
Complete sidetracking, but I can't help but wish https://github.com/clojure/core.memoize was available for ClojureScript, because then we would be able to use less naive memoization techniques that are less likely to grow too much, e.g. LRU or TTL semantics (see https://github.com/clojure/core.memoize/blob/master/src/main/clojure/clojure/core/memoize.clj#L398). I'm sure this would give us new guns to shoot ourselves in the foot, but I used core.memoize in the past in certain problems and it really opens some good doors.
That's all! 😅 Cheers!
96% of end-end tests have passedFailed tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (25)Click to expandClass TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
|
Parveshdhull
force-pushed
the
feat/bottom-tabs-view-id
branch
3 times, most recently
from
5ee3f77
to
9c83655
Compare
93% of end-end tests have passedFailed tests (2)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Passed tests (27)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
|
Parveshdhull
force-pushed
the
feat/bottom-tabs-view-id
branch
from
f96c96d
to
9c83655
Compare
93% of end-end tests have passedFailed tests (2)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Passed tests (27)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
Parveshdhull
force-pushed
the
feat/bottom-tabs-view-id
branch
from
9c83655
to
63338a1
Compare
|
@Parveshdhull thank you for PR! Nightly: FILE.2023-04-19.11.40.43.mp4PR build: FILE.2023-04-19.11.41.19.mp4Navigation is still twitchy, screens are jumping on and off, and the animation is far from smooth. |
|
Hi @churik, Thank you very much for testing the PR. PR makes sure that few screens(shell, home, communities) are only updated when we navigate to them (not in background). And on bonus, I was hoping it will fix #15609. So, there are two performance-related issues.
We also faced a similar issue with bottom tabs, that's why we only load the current tab for the first time, once the animation is complete. I am currently not sure how to fix this issue, but as I will work on #15565, I hope it will also address animations related to navigation. First I am going to try if navigation provides functionality to implement the required feature and use better animation, if not, probably the next best option will be to implement chats & communities screen as bottom tabs, and control navigation and animation ourselves. |
|
Btw, is this twitchy navigation as you mentioned in the above video, is this affected by the number of communities or it is the same for even one community or chat? |
|
@Parveshdhull you're right, it seems not great for both cases, regardless of communities, though it seems a bit worse to me with communities (which is actually I have no idea how to check) So as the initial issue is not reproducible for me with slowing down the scrolling, I'll ask @VladimrLitvinenko to check on his device just in case and after this PR is ready to be merged. |
|
There are noticeable changes on low-level android device,thanks for checking @VladimrLitvinenko |
Parveshdhull
force-pushed
the
feat/bottom-tabs-view-id
branch
from
63338a1
to
7ba445f
Compare
fixes #15609
Summary
(used to make sure react doesn't change the chats list component when other tabs are selected)
Testing
Please check communities & chats home list items for any regressions.
Note: When the user will be on another tab then the chats-tab, and receive any new message, it will only be updated once he/she navigates back to the chats-tab.