[#17964] emoji screen performance #18213
Conversation
ulisesmac
requested review from
vkjr,
briansztamfater,
smohamedjavid,
J-Son89,
OmarBasem and
mmilad75
| (string/capitalize (first (name token)))]]) | ||
| (some-> token | ||
| name | ||
| first | ||
| string/capitalize)]]) |
There was a problem hiding this comment.
This is the fix for #18212
| (-> | ||
| (gesture/gesture-pan) |
There was a problem hiding this comment.
Fixed the -> sytle
| curr-scroll (reagent/atom 0)] | ||
| (let [scroll-enabled (reagent/atom true) | ||
| curr-scroll (reagent/atom 0) | ||
| animating? (reagent/atom true) |
There was a problem hiding this comment.
An atom that says if the current sheet is being animated or not.
I took the solution suggested by @smohamedjavid in a previous PR (#17891 (review))
There was a problem hiding this comment.
Why reagent atoms? Why not shared value?
There was a problem hiding this comment.
@Parveshdhull I don't think these atoms are used in animations
There was a problem hiding this comment.
The use of reagent atoms looks scary. Before you know, you are passing the same reagent atoms across several files (as we did in the messages screen), and a small reset force updates a whole bunch of screens.
They are not animations, still sharedValue value can also update the required parameters like reagent atoms without force updating the view. In other words
- animating? - animates property window-size
- scroll-enabled - animates property scroll-enabled
- curr-scroll - It looks like it just holds data that is used in drag-gesture, we can just use a simple atom here.
There was a problem hiding this comment.
Hey @Parveshdhull
... a small reset force updates a whole bunch of screens.
Hmm... it doesn't work like that, as I said in this PR, I'm using a late deref approach, this aproach is also being used for the other atoms in this component.
Regarding to:
Before you know, you are passing the same reagent atoms across several files (as we did in the messages screen)
Probably the issue with messages screns was due to a different performance topic.
Remember Reagent does some optimizations before sending things to react, and if we don't perform a deref, the view is not re-rendered. Here's a small working example of the late deref approach and rerendering, that you can copy and test it:
(defn subcomponent-2 [{:keys [a-2]}]
(println "subcomponent-2 repainted")
;; `a-2` is deref'ed only here, but traveled through all components.
[quo/text (str "atom 2 value:" @a-2)])
(defn subcomponent-1 [{:keys [a-1 a-2]}]
(println "subcomponent-1 repainted")
[rn/view
[quo/text (str "atom 1 value:" @a-1)] ;; Here only `a-1` uses deref
[subcomponent-2 {:a-2 a-2}]]) ;; Here we pass the atom `a-2`
(defn test-component []
(let [atom-1 (reagent/atom 0)
atom-2 (reagent/atom 0)
update-atom-1 #(swap! atom-1 inc)
update-atom-2 #(swap! atom-2 inc)]
(fn []
(println "test-component repainted")
[rn/view
;; Atoms are being passed, we'll do the deref only inside the subcomponents (as in this bottom sheet component)
[subcomponent-1 {:a-1 atom-1
:a-2 atom-2}]
[quo/button {:on-press update-atom-1}
"Update atom 1"]
[quo/button {:on-press update-atom-2}
"Update atom 2"]])))It'll render the following:
Here is the result in the terminal after pressing the update atom 2 button 3 times, and after that pressing the update atom 1 three times:
subcomponent-2 repainted
subcomponent-2 repainted
subcomponent-2 repainted
subcomponent-1 repainted
subcomponent-1 repainted
subcomponent-1 repainted
So, it's easy to see that reseting (or swaping) the atom-2 didn't re-render the intermediate components.
On the other hand, if we pass derefed values instead of atoms:
;; Changing this call inside `test-component` and also removing the `@` insde subcomponents
[subcomponent-1 {:a-1 @atom-1
:a-2 @atom-2}]then when press the update atom 2 button, the terminal will print:
test-component repainted
subcomponent-1 repainted
subcomponent-2 repainted
In that case it's actually re executing the render functions of the intermediate components.
But, as said before, that's not the approach in this component.
So @Parveshdhull wdyt?
IMO we shouldn't use shared values for something that aren't animations. Seems like a misuse.
There was a problem hiding this comment.
Thank you @ulisesmac for detailed explanation. Yes you are right, using sharedValue here, don't make much sense.
Should we just use simple atom for curr-scroll, it looks like it is just storing data?
There was a problem hiding this comment.
Also, probably not in this PR, but should we consider using worklets for drag gesture for further improving performance?
There was a problem hiding this comment.
Sorry @Parveshdhull , I forgot to reply to it.
Should we just use simple atom for curr-scroll, it looks like it is just storing data?
I checked the rerenderings, and at least in the emoji picker it's not rerendering each time someone scrolls (since no one is performing deref on that atom).
I think changing that for a regular atom might create an unexpected behvaior in other screens since maybe some screens use it to trigger a re-render.
As said before, since this is not harmful in this screen, we can just keep it.
Regarding to:
Also, probably not in this PR, but should we consider using worklets for drag gesture for further improving performance?
Yes, maybe gestures can use worklets :) we could open an issue to improve the performance of the bottom sheet 👍
| :scroll-enabled scroll-enabled | ||
| :current-scroll curr-scroll | ||
| :on-scroll #(on-scroll % curr-scroll) | ||
| :sheet-animating? animating?}]]]])))) |
There was a problem hiding this comment.
Not a big fan of passing atoms as props to other components 😢 but this atom will travel a long path, tested it and using a late-deref approach improved the performance a little 😄
There was a problem hiding this comment.
hmm, have you considered using react context instead? it will help prevent the prop drilling, additionally you can instead pass accessor functions rather than the ratom. wdyt? 👍
There was a problem hiding this comment.
It sounds as a good idea in terms of not passing the props through many components.
But, I think re-frame might be a simpler option. TBH, I know they solve the same problem, it seems just a matter of the JS world solution vs the Clojure world solution.
Right now, we are passing many atoms to the child of this component (the one I added is just another one). So we can consider adding either react-context or re-frame sub in a later refactoring to this component.
Also, we should consider if moving this helps in terms of readability and performance.
Thanks for the suggestion Jamie! Very interesting topic
Jenkins BuildsClick to see older builds (20)
|
| (let [viewable-item (-> (oops/oget event "viewableItems") | ||
| transforms/js->clj | ||
| first | ||
| :item) | ||
| (let [viewable-item (some-> event | ||
| (oops/oget "viewableItems") | ||
| (aget 0) | ||
| (.-item)) | ||
| header? (and (map? viewable-item) (:header? viewable-item)) | ||
| section-key (if header? | ||
| (:id viewable-item) | ||
| (:id (emoji-picker.data/emoji-group->category (-> viewable-item | ||
| first | ||
| :group))))] | ||
| (-> viewable-item first :group emoji-picker.data/emoji-group->category :id))] |
There was a problem hiding this comment.
This function had a major improvement in performance! 😄
used the time function to wrap this piece of code, in develop it's taking ~22ms!! the less I got was 12ms and as much as 33ms.
Transfroming all values to only get the first item has a big impact in performance.
Now we are just extracting from that JS vector.
The time now for this function is 0.02ms ~ 0.03ms
There was a problem hiding this comment.
The viewableItems inside the event sometimes give out nil when the user scrolls through the list. But some-> is a great choice to tackle that.
There was a problem hiding this comment.
@smohamedjavid Yep, before adding some-> I had that issue 😅
| (defn- emoji-row | ||
| [row-data {:keys [on-select close]}] | ||
| (into [rn/view {:style style/emoji-row-container}] | ||
| (map-indexed | ||
| (fn [col-index {:keys [hexcode] :as emoji}] | ||
| ^{:key hexcode} | ||
| [emoji-item emoji col-index on-select close]) | ||
| row-data))) | ||
| (map-indexed (fn [col-index emoji] | ||
| [emoji-item emoji col-index on-select close])) | ||
| row-data)) |
There was a problem hiding this comment.
Another improvement in performance in two aspects:
1. We don't need hexcode as key,
we need setting a :key when we return a sequence of elements, but this function is using into, so we are just returning hiccup.
Just to explain it better, we need :key metadata when:
[rn/something
( ;; <- a sequence
^{:key 1} [rn/something-else ...]
^{:key 2} [rn/something-else ...]
^{:key 3} [rn/something-else ...])]since we are using into, we are returning just valid hiccup, no need of :key
[rn/something
[rn/something-else ...]
[rn/something-else ...]
[rn/something-else ...])]2. Using xform parameter of into
into allows the use of transducers (a transformation to data in a more performant way), and this piece of code was perfect for it, since we have:
- something we want to move into (
[rn/view...]) - a transformation of data (
(map-indexed (fn [...] ...))) that also returns a transducer when called with no params - data (
row-data).
So, I'm using into as:
(into [] [transformation data)
which is:
(into [rn/view...] (map-indexed ...) row-data)
If you'd like to read more about transducers:
https://clojure.org/reference/transducers
into
To apply a transducer to an input collection and construct a new output collection, use into (which efficiently uses reduce and transients if possible):
There was a problem hiding this comment.
Thanks for the detailed explanation! @ulisesmac.
We need to keep the key in this case, as it's used to track views (when the items change) when the user is searching. If we remove the key, the list will see the last rendered item.
RPReplay_Final1702906615.MP4
We can't use col-index as it will be the same for every row. But, the hexcode is unique.
There was a problem hiding this comment.
@smohamedjavid Thanks I fixed it!
IMO using hexocde is a nice approach!
| [:f> f-view | ||
| (merge sheet-opts | ||
| {:render-emojis? render-emojis? | ||
| :search-text search-text | ||
| :on-change-text on-change-text | ||
| :clear-states clear-states | ||
| :filtered-data @filtered-data | ||
| :set-scroll-ref set-scroll-ref | ||
| :on-select on-select | ||
| :on-viewable-items-changed on-viewable-items-changed | ||
| :active-category active-category | ||
| :scroll-ref scroll-ref})]))) | ||
| (assoc sheet-opts |
There was a problem hiding this comment.
assoc instead of merge since we only want to add some keys to an existing map
|
The code was pretty well written, congrats!, Maybe in the future we can consider using a |
| @@ -16,8 +16,7 @@ | |||
| [status-im2.contexts.emoji-picker.utils :as emoji-picker.utils] | |||
There was a problem hiding this comment.
Not related to this pr but somewhat important to note. The concept of "Contexts" was for large feature chunks - e.g messaging, communities, onboarding etc.
Emoji picker should live in somewhere like status-im2.common.emoji-picker imo. We can make a follow up pr to move it. 👍
There was a problem hiding this comment.
yeah context and common getting messy, re-evaluation is needed, i remember @ilmotta had a vision how it should work
There was a problem hiding this comment.
Correct guys, contexts is just an abbreviation for bounded contexts from DDD. We're not doing DDD, but still, the idea is to separate domains because they tend to evolve in very different ways. We have a glossary and a link to explain what context means. emoji-picker as a bounded context makes no sense.
There was a problem hiding this comment.
Yep, totally agree, let's move it in a follow up! 👍
|
@ulisesmac would be good to add a before and after video for PRs involving animations/performance |
|
@OmarBasem Previous: now: |
Oh did not notice, the videos aren't rendering inline. Thank you. |
| curr-scroll (reagent/atom 0)] | ||
| (let [scroll-enabled (reagent/atom true) | ||
| curr-scroll (reagent/atom 0) | ||
| animating? (reagent/atom true) |
There was a problem hiding this comment.
Why reagent atoms? Why not shared value?
| (defn- emoji-row | ||
| [row-data {:keys [on-select close]}] | ||
| (into [rn/view {:style style/emoji-row-container}] | ||
| (map-indexed | ||
| (fn [col-index {:keys [hexcode] :as emoji}] | ||
| ^{:key hexcode} | ||
| [emoji-item emoji col-index on-select close]) | ||
| row-data))) | ||
| (map-indexed (fn [col-index emoji] | ||
| [emoji-item emoji col-index on-select close])) | ||
| row-data)) |
There was a problem hiding this comment.
Thanks for the detailed explanation! @ulisesmac.
We need to keep the key in this case, as it's used to track views (when the items change) when the user is searching. If we remove the key, the list will see the last rendered item.
RPReplay_Final1702906615.MP4
We can't use col-index as it will be the same for every row. But, the hexcode is unique.
@ulisesmac - The |
There was a problem hiding this comment.
Thanks @ulisesmac!
The PR seems reasonable to merge (apart from this comment that needs a fix https://github.com/status-im/status-mobile/pull/18213/files?diff=unified&w=1#r1430132704). I don't have much to add, so I'm approving in advance.
Just a note, although the code is being improved in some ways perf wise, I couldn't experience a noticeable different on my Android device.
| @@ -16,8 +16,7 @@ | |||
| [status-im2.contexts.emoji-picker.utils :as emoji-picker.utils] | |||
There was a problem hiding this comment.
Correct guys, contexts is just an abbreviation for bounded contexts from DDD. We're not doing DDD, but still, the idea is to separate domains because they tend to evolve in very different ways. We have a glossary and a link to explain what context means. emoji-picker as a bounded context makes no sense.
There was a problem hiding this comment.
Emoji screen performance is better now, thanks @ulisesmac 👍
probably it's next step from performance crew |
ulisesmac
force-pushed
the
17964-emoji-screen-performance
branch
from
be966b6 to
ec303d9
Compare
81% of end-end tests have passedFailed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Expected to fail tests (4)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (39)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
|
81% of end-end tests have passedFailed tests (5)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Expected to fail tests (4)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Passed tests (39)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
|
|
hey @ulisesmac Thank you for the PR. One issue is found on the emoji screen that I've noticed also on the latest nightly. It seems to be related to performance. Do you think it might be resolved within the scope of this PR, or would it be better to address it separately? ISSUE 1: [IOS] Disappearing emojis when selecting a filter during scrolling in emoji drawer on iOSSteps:
Actual result:When scrolling through the emojis and simultaneously selecting a filter, the emojis disappear from view. emojiios.mp4Expected result:The emojis should remain displayed Device:iPhone 11 Pro max, IOS 16 |
|
@VolodLytvynenko I'd prefer opening an issue for it, since it'll take some time to solve it, and this PR is mainly focused on the performance while opening the sheet. For sure that is something that needs to be solved. Edit: Here's the issue: BTW, is this PR ready to get merged? |
ulisesmac
force-pushed
the
17964-emoji-screen-performance
branch
from
ec303d9 to
be77d1b
Compare
|
Hi @ulisesmac Thank you for the follow-up creation. PR is ready to be merged |
ulisesmac
force-pushed
the
17964-emoji-screen-performance
branch
from
be77d1b to
516ec96
Compare
fixes #17964
fixes #18212 <- this is a small issue that is also fixed in this PR
Summary
The emoji picker sheet performance needed to be improved in its opening/closing animations. This PR improves the performance:
Previous:
https://github.com/status-im/status-mobile/assets/90291778/61bcda80-603f-45d4-88c5-47e52ec102cb
now:
https://github.com/status-im/status-mobile/assets/90291778/9de91ab3-565a-4eb9-b205-cd1775a074e1
IMPORTANT
Even though performance has been improved, it still could be slow in some devices. This PR aims to improve the performance but completely solving the issue is a problem that would require a significant amount of time - maybe not in our priorities right now -.
Review notes
The previous fix consisted of delaying the rendering, this PR discarted that approach to reduce the user exposure to the blank screen, this is a screenshot in the middle of the animation, while the sheet is opening:
Before: vs now
Platforms
Non-functional
Steps to test
status: ready