improve profile screen performance #18281
Conversation
flexsurfer
requested review from
ilmotta,
OmarBasem,
mohsen-ghafouri and
ulisesmac
56% of end-end tests have passedNot executed tests (1)Failed tests (16)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Passed tests (27)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
|
|
@status-im/mobile-qa could you pls confirm e2e not related |
I'm afraid it is related. The most of the failures are caused by the screen size change when getting a link to profile - the element is not intractable. This is how it looks like now: |
|
merged because these changes not related to this PR |
|
it is likely related to last wallet changes, communicated to wallet team |
|
introduced here #18167 |
There was a problem hiding this comment.
Hey @flexsurfer
I know this has been merged, but I wanted to share some questions.
| [rn/flat-list | ||
| {:data data | ||
| :style (style/settings-items props) | ||
| :render-fn settings-item/view | ||
| :separator [rn/view {:style (style/settings-separator props)}]}]]) |
There was a problem hiding this comment.
I've always wondered if our flat-list wrapper is properly transforming the render-fn in terms of performance.
Also, that wrapper is doing many things. Have we considered if that is the part of the problem? We use this wrapper a lot, and it's involved in many performance issues because of flat-list, so it's worth checking (or did we already check?).
There was a problem hiding this comment.
One thing we certainly don't do is avoid regenerating anonymous functions. For our lower-level components we can do better using something like the factory functions explained in https://github.com/day8/re-frame/blob/39adca93673f334dc751ee2d99d340b51a9cc6db/docs/on-stable-dom-handlers.md#more-advanced-again
There was a problem hiding this comment.
Thanks for replying to this comment also in the other PR! :)
I read that in the past, about a fabric of fabrics, I found it a little confusing and probably hard to maintain, but it works.
It'd be good to revisit our flat list implementation then :) - and if we apply that performance technique, make sure it's well documented -
There was a problem hiding this comment.
I agree with you, the factory function technique should be used only in critical places. To me the infrastructure/abstractions we build should be as optimal as possible because they're reused over and over, and the flat-list component is one such place.
Thanks for raising your concern, it's something I don't remember seeing being discussed in the last year or so.
| #js {:length 48 :offset (* 48 index) :index index}) | ||
| #js {:length 100 :offset (* 100 index) :index index}) |
There was a problem hiding this comment.
I'm wondering why this has been changed. Were the dimensions wrong? does this affect the UI?
There was a problem hiding this comment.
it's just a random number, i just wanted to reserve more space
There was a problem hiding this comment.
I believe getItemLayout should reflect the actual item dimensions:
getItemLayout is an optional optimization that allows skipping the measurement of dynamic content if you know the size (height or width) of items ahead of time.
https://reactnative.dev/docs/flatlist#getitemlayout
So maybe changing this for random values might have an unexpected behavior
There was a problem hiding this comment.
yes it should, but here it's used only for first "empty" rendering, and then the layout will be calculated by real item size, in this case, it will be different for items
There was a problem hiding this comment.
Sorry, I'm not understanding how this will be done.
Do you mean that if we pass wrong dimensions, RN will only use them for the first rendering and in following renders they will be automatically calculated?
Does RN know that calculations are wrong?
Many questions, just not sure what the approach used here will do.
| (defn delay-render | ||
| [content] | ||
| (let [render? (reagent/atom false)] | ||
| (js/setTimeout #(reset! render? true) 0) |
There was a problem hiding this comment.
Have you considered setImmediate?
There was a problem hiding this comment.
no why? setTimeout fits perfectly here
There was a problem hiding this comment.
Just because of reading.
We are setting a timeout as zero, so there is no "time" involved. SetImmediate sounds as what we want 🤔
Now I'm wondering if removing the timeout gives the same result:
(let [render? (reagent/atom false)
_ (reset! render? true)]
(fn []
,,,))Since once an atom is changed, reagent perform the updates the next cycle, so it means the first time the component is rendered using false and the second time with true 🤔
There was a problem hiding this comment.
probably, but for me (js/setTimeout 0) is known thing, so i'm sure it works how it should
There was a problem hiding this comment.
Update on this:
It won't work, the atom is always received in the render function as false.
I also tried using setImmediate (to trigger the atom reset! at the end of the JS cycle) and it works too!
One thing I've just noticed is that we are not receiving content in the render function, it means, if content changes, it won't trigger a re-render.
Is this by mistake or intended?
As @ilmotta said, since this is a general purpose component, and content can be anything, maybe we should receive content in the render fn
There was a problem hiding this comment.
in this case it's intentional,ok i think it would be better to move it to profile settings namespace, because there is confusion now, this was done only for this current case but for some reason i decided to put it into utils re-frame namespace
There was a problem hiding this comment.
Interesting work @flexsurfer!
I think the solution is fine, if it works it works, but I wanted to hear more about this solution and where it wouldn't work in the PR description, because clearly we shouldn't delay rendering everywhere, so where does the abstraction breaks you know?
I'm (just) slightly concerned we might apply this workaround in too many places, masking other performance problems that should be solved before.
But let's see, nice work in any case!
| ^{:key item} | ||
| [:<> | ||
| [settings-item/view item] | ||
| (when-not (= item (last data)) |
There was a problem hiding this comment.
Just a note @flexsurfer. last is inefficient for larger sequences because like a linked list, Clojure has to go one element at a time to find the last one. There's no sort of pointer to the last element. Much faster in larger lists is to grab the index of the last item and compare to that. Also the comparison with = would be much faster than comparing item, which is a more complex structure.
Take a look at CLJS source code, last is expensive.
(defn last
"Return the last item in coll, in linear time"
[s]
(let [sn (next s)]
(if-not (nil? sn)
(recur sn)
(first s))))There was a problem hiding this comment.
We could restrict the type of data to be a vector and use peek instead.
Btw, I wonder how expensive the transformation from list to vector is and what is faster:
(peek (vec data))
vs
(last data)
In case we receive a list.
I guess vec transforms in O(n), but whishing it's not 😅
There was a problem hiding this comment.
in this current case it's fine i guess because we know there will be a small number of items, and i don't think it's super critical here, just don't like indexes in clojure code :)
There was a problem hiding this comment.
Interesting question @ulisesmac, I don't know either without benchmarks.
In general, we should just not call last inside a loop of a collection, it's quite inefficient, unless we know it's a vector as you hinted.
I think just calling (-> data count dec) is significantly cheaper than trying to convert to a vector and repeatedly call last inside a loop because then we can just do a number comparison of (= index last-index), which is probably as fast as we can go to identify the last element.
There was a problem hiding this comment.
In general, we should just not call last inside a loop of a collection, it's quite inefficient, unless we know it's a vector as you hinted.
I agree, we could just extract that calculation outside the for loop.
| (defn delay-render | ||
| [content] | ||
| (let [render? (reagent/atom false)] | ||
| (js/setTimeout #(reset! render? true) 0) |
There was a problem hiding this comment.
The RN doc says setTimeout or setImmediate may delay animations. runAfterInteractions doesn't suffer from that, although only experimentation can tell if it works better.
There was a problem hiding this comment.
we don't have animations here, what kind of animation they might delay? setTimeout just put code in the next js task
There was a problem hiding this comment.
delay-render is a general component, and content can be anything. How can this function know there's no animation?
There was a problem hiding this comment.
that's expected, if you delay component :)
There was a problem hiding this comment.
but i think documentation is old, and that about times there were only js animation based on timers etc , now with reanimated it's not actual anymore
There was a problem hiding this comment.
You said there are no animations here, but there's no way to know from here. My point is just that setTimeout with 0 is a workaround that may delay animations as pointed by the RN docs, and there's no way for this function to know what content does.
At the very least I think it should be investigated how this solution fares against content that has animations versus using runAfterInteractions.
This of course in a future branch/PR because this one is already merged.
There was a problem hiding this comment.
i said that because i believe docs says that if you have animation in your code somewhere , like in the parent component it will be affected by setTimeout because Timeout has a priority, but in our case if you use delay you know that entire component will be delayed, so offcourse animation will be delayed as well, if content has it, so it should be obvious for developer who uses delay, but again, i don't say it's a general solution, so probably we should move it in separate namespace and add description how to use it, for now it must be used only in this current case, and in other cases it should be used with understanding how it works
There was a problem hiding this comment.
and add description how to use it, for now it must be used only in this current case,
That would be great @flexsurfer.
and in other cases it should be used with understanding how it works
👍🏼 perfect
so it seems like there is a quick simple way to improve performance, by moving part of views into next js task by using timeout