[experiment] limit number of rendered messages when open a chat #8191
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (18)
|
|
just wondering why is it so |
| @@ -299,6 +344,8 @@ | |||
| :on-layout (fn [e] | |||
| (re-frame/dispatch [:set :layout-height (-> e .-nativeEvent .-layout .-height)]))} | |||
| [chat-toolbar current-chat public? modal?] | |||
| [react/text {:style {:height 14}} | |||
There was a problem hiding this comment.
in current build when you open chat screen first time limit enabled: true is shown which means it is rendered with new limits, the next time limit enabled: false is shown - that's an old version. Just for testing
There was a problem hiding this comment.
i mean, do you plan to remove this after testing?
yep I have a branch in progress with 18-20% improvement on this number. We definitely have some redundant components there. But still impact of such changes will be much less noticeable |
|
|
||
| (defn increment-limit [lim] | ||
| (+ lim | ||
| (cond |
There was a problem hiding this comment.
maybe better to use case here and put the numerical values directly
There was a problem hiding this comment.
case is an order of magnitude faster, and that not even considering that you are doing some additions here in the conditions that are not sure can be optimize by the compiler
(case lim
;; first step
5 11
;; second step
11 16
;; next steps
(+ lim 20)
status-im.wallet.custom-tokens.core> (time (dotimes [x 10000000] (cond (= x 1) 2 (= x 3) 4 :else 5)))
"Elapsed time: 5275.000000 msecs"
nil
status-im.wallet.custom-tokens.core> (time (dotimes [x 10000000] (case x 1 2 3 4 5)))
"Elapsed time: 24.000000 msecs"
nil
There was a problem hiding this comment.
- relying on magic numbers is going to be an issue, I prefer to avoid doing this. If values are co-dependent, that should be expressed in the code.
- The difference in efficiency in negligible compering to the whole rendering thing as long as this operation is not going to be executed repeatedly, repl on slow android device https://gist.github.com/rasom/b6a154bef04894c3d5ecff5fca6fbfcf
There was a problem hiding this comment.
Eric, 10000000 times, really? :)
There was a problem hiding this comment.
A docstring explaining why you use theses steps would be better than renaming the numbers 5 11 16 and 20
There was a problem hiding this comment.
It's only magic numbers if there is no explanation
There was a problem hiding this comment.
I disagree, because numbers are co-dependent. And as optimization this change improves nothing.
There was a problem hiding this comment.
Would that work for you then?
(def ^:const first-step 5)
(def ^:const second-step 11)
(def ^:const third-step 16)
(def ^:const step-increment 20)
(defn increment-limit [lim]
(case lim
first-step second-step
second-step third-step
(+ lim step-increment)))
Works because the steps are defined as constants and the compiler will replace them by the int
I would still like to understand why the increments are +6 then +4 then +20, looks like the real magic numbers here
There was a problem hiding this comment.
I would still like to understand why the increments are +6 then +4 then +20, looks like the real magic numbers here
At first, it is 5, then +6, then +11. Not +6 then +4 then +20.
And... well that's true, they are magic numbers (but not in terms of programming), and as I'm 99% sure we are going to change these numbers I prefer to code the relations between them.
The way how I "inferred" those numbers: on a slower device (samsung a500d, 720 x 1280 pixels, 16:9 ratio (~294 ppi density)) I opened #status and #status-core chats during several days, and almost always not more than 5 messages were shown initially. 11 is the max number of messages which fits on that screen. Both number are device specific, and probably can be derived more precisely in some better way, but 11 is kind of a safe number for almost all devices, chances that more than 11 messages will be shown on the initial screen in a regular chat are quite low at the moment.
|
Great, @rasom, no regressions and improved performance when navigating to chat with messages.
@rasom could you please remove code required for testing, we'll do smoke test of new builds and then merge. |
|
@Serhy "without limit" (check it when transition is ended) is actually the old version. So if it is faster, then there is no improvement. |
Sorry, misplaced the values above:
|
|
@Serhy that sounds better, I hope it is so :D |
|
Wait, that is the whole payload that can make the app choke? |
|
And that's 9.5 seconds for 20 messages? |
that's not exactly a problem which this pr targeted, but that's definitely something we must fix |
|
@mandrigin @rasom we should store the messages as a tree of element based to avoid having to do any parsing at runtime, that is why I suggested Jan to do but he insisted on storing the rendering info separately which can be quite expensive in some cases. And since we already store the raw payload anyway we can always use it in case we do some migrations on rendering info |
|
I will create a separate issue for that |
|
@yenda do you mean we parse that data each time messages are rendered? If not, the way how we store data doesn't matter. |
|
@rasom afaik we parse them once to build up this kind of index that says where to cut the file to build a tree of elements when rendering, I think we should store this tree of elements in edn instead so that we only have to render |
rasom
force-pushed
the
experement-limit-rendered-messages
branch
from
d1d5eea to
7b5f19a
Compare
@Serhy done |
100% of end-end tests have passedPassed tests (49)Click to expand |
|
I don't see any regressions with Android 6.0.1, Android 8.1, iOS 12.2, MacOS builds. |
rasom
force-pushed
the
experement-limit-rendered-messages
branch
from
7b5f19a to
932ef27
Compare
Results:
navigation from chat list to chat (#status) w/o opacity wrapper, galaxy s9
navigation from chat list to chat with opacity wrapper, galaxy s9
navigation from chat list to chat (#status) w/o opacity wrapper, galaxy a500d (slower device)
navigation from chat list to chat with opacity wrapper, galaxy a500d (slower device)
Navigation from chats list to chat is 30-45% faster.