Chat view performance improvements #10711
Conversation
Jenkins BuildsClick to see older builds (52)
|
cammellos
force-pushed
the
chore/move-messages-on-own-collection
branch
from
17febbf
to
9b3d210
Compare
64% of end-end tests have passedFailed tests (27)Click to expand
Passed tests (49)Click to expand |
cammellos
force-pushed
the
chore/move-messages-on-own-collection
branch
from
f4d13c2
to
008f526
Compare
51% of end-end tests have passedFailed tests (34)Click to expand
Passed tests (36)Click to expand |
79% of end-end tests have passedFailed tests (20)Click to expand
Passed tests (75)Click to expand |
93% of end-end tests have passedFailed tests (7)Click to expand
Passed tests (87)Click to expand |
89% of end-end tests have passedFailed tests (9)Click to expand
Passed tests (75)Click to expand |
cammellos
force-pushed
the
chore/move-messages-on-own-collection
branch
from
3deff23
to
8a8b885
Compare
cammellos
changed the title
51% of end-end tests have passedFailed tests (24)Click to expand
Passed tests (25)Click to expand |
| @@ -8,8 +8,6 @@ | |||
| [status-im.ui.screens.chat.photos :as photos] | |||
| [status-im.utils.platform :as platform])) | |||
|
|
|||
| ;;TODO REWORK THIS NAMESPACE | |||
There was a problem hiding this comment.
this still need to rework it :)
There was a problem hiding this comment.
oh sorry, that was removed by mistake, generally all caps comment are the things I leave before commiting :)
| @@ -38,13 +38,12 @@ | |||
|
|
|||
| (defview quoted-message | |||
| [_ {:keys [from text]} outgoing current-public-key] | |||
| (letsubs [{:keys [ens-name alias]} [:contacts/contact-name-by-identity from]] | |||
| (letsubs [contact-name [:contacts/contact-name-by-identity from]] | |||
There was a problem hiding this comment.
probably not in this PR but we should add this data to the message itself in the event, because of #10380
There was a problem hiding this comment.
I would say it's even better to do not update the name when you on the chat view, and update it only next time you open chat, because 1- its rare case when this happens, 2 - we have 30s delay for verifications, so from performance and from ux it will be better to show it next time you open chat
There was a problem hiding this comment.
Most of the times though it does happen when you are in the chat view, think of the simplest case:
- Open #status
- Receive messages, no names, status-go verifies them
- Names are received
In such case I'd want to see a name as soon as it's available, not on reloading the chat?
We can though look at performance, my guess is that this is not the bottleneck, more likely is the actually building up the list (datemarks,timestamps etc)
| :on-press #(hide-sheet-and-dispatch [:chat.ui/remove-chat-pressed chat-id])}]]) | ||
| (defn one-to-one-chat-actions [{:keys [chat-id]}] | ||
| (let [photo @(re-frame/subscribe [:chats/photo-path chat-id]) | ||
| contact-name @(re-frame/subscribe [:contacts/contact-name-by-identity chat-id])] |
There was a problem hiding this comment.
we know for sure these fields shouldn't change during this view is opened, so instead of creating and disposing of subscriptions we could use direct access to app-db probably in such cases , yeah i think we should consider this idea in general because in that case we will have less "noise" subscriptions, i mean such subscriptions which are run each time we change something but we know for sure result will be the same
There was a problem hiding this comment.
some stuff is actually already denormalized, for example chat now has the photo (which never changes), I haven't yet used the denormalized version yet as I am not 100% sure it always has it, but I can change that in a separate PR (we can subscribe only if it's nil for example).
| :title truncated-chat-name | ||
| :title (if group-chat | ||
| (utils/truncate-str chat-name 30) | ||
| @(re-frame/subscribe [:contacts/contact-name-by-identity chat-id])) |
There was a problem hiding this comment.
:D , this is tricky, I know it looks a bit funny, but I don't want to run the subscribe if it's not a one-to-one. If I pass a component it will be not applying the header style, so I am not sure what's another option.
cammellos
force-pushed
the
chore/move-messages-on-own-collection
branch
2 times, most recently
from
36630da
to
443066f
Compare
cammellos
force-pushed
the
chore/move-messages-on-own-collection
branch
2 times, most recently
from
8d6942b
to
e3971fa
Compare
cammellos
force-pushed
the
chore/move-messages-on-own-collection
branch
from
e3971fa
to
9ee526f
Compare
99% of end-end tests have passedFailed tests (1)Click to expand
Passed tests (94)Click to expand
|
|
Tested on different types of chats: 1-1, public, group chats - big messages with markdown, links, emoji, stickers, Arabic / Chinese text |
This commit does a few things: Move collections top level Move `messages`,`message-lists`,`pagination-info` from nested in `chats` to top level at the db. The reason for this change is that if any of the `messages` fields change, any `sub` that relies on `chat` will be recomputed, which is unnecessary. Move chat-name to events `chat-name` was computed dynamically, while it is now only calculated when loading chat the first time around. Remove `enrich-chats` Enrich chats was doing a lot of work, and many subscriptions were relying on it. Not all the computations were necessary, for example it would always calculate the name of who invited the user to a group chat, regardless of whether it was actually used in the view. This commit changes that behavior so that we use smaller subscriptions to calculate such fields. In general we should move computations to events, if that's not desirable (there are some cases where we might not want to do that), we should have "bottom/leaf heavy" subscriptions as opposed to "top heavy", especially if they are to be shared, so only when (and if) we load that particular view, the subscription is triggered, while others can be re-used. I have compared performance with current release, and there's a noticeable difference. Opening a chat is faster (messages are loaded faster), and clicking on the home view on a chat is more responsing (the animation on-press is much quicker).
cammellos
force-pushed
the
chore/move-messages-on-own-collection
branch
from
9ee526f
to
9d76913
Compare
Signed-off-by: Andrea Maria Piana <andrea.maria.piana@gmail.com>
Improve chat loading performance
This commit does a few things:
Move collections top level
Move
messages,message-lists,pagination-infofrom nested inchatsto top level at the db.The reason for this change is that if any of the
messagesfieldschange, any
subthat relies onchatwill be recomputed, which isunnecessary.
Move chat-name to events
chat-namewas computed dynamically, while it is now only calculatedwhen loading chat the first time around.
Remove
enrich-chatsEnrich chats was doing a lot of work, and many subscriptions were
relying on it.
Not all the computations were necessary, for example it would always
calculate the name of who invited the user to a group chat, regardless
of whether it was actually used in the view.
This commit changes that behavior so that we use smaller subscriptions
to calculate such fields.
In general we should move computations to events, if that's not
desirable (there are some cases where we might not want to do that), we
should have "bottom/leaf heavy" subscriptions as opposed to "top heavy",
especially if they are to be shared, so only when (and if) we load that
particular view, the subscription is triggered, while others can be
re-used.
I have compared performance with current release, and there's a
noticeable difference. Opening a chat is faster (messages are loaded
faster), and clicking on the home view on a chat is more responsing
(the animation on-press is much quicker).
Sorry for the big PR but changing one thing lead to another. I can split it off for easier reviewing if necessary.
I have left subscriptions names as they are, using
rawfor those that do not do any calculation or don't fallback on defaults, probably it would be good to revisit to have a consistent naming, I would also separatesubs.cljsin multiple files (under a single directory),subs/chats.cljssubs/contacts.cljsetc , as sometimes is difficult to find what can be re-used (or any other scheme is good), but that's for a separate PR pending discussion.Testing
No functional changes, just performance improvement. Performance can be tested if necessary, I have included a video that shows the difference, mind as well that current develop parses markdown so it does a bit more, but pr seems to be faster despite of that.
Areas that are impacted is main chat view, chat view mostly.
Receiving messages is also faster it looks (significantly), although haven't looked much into it as wasn't optimizing that particular part.
status: ready