[slow sign in] fix slow sign in (first iteration) #6722
Conversation
Pull Request Checklist
|
rasom
force-pushed
the
fix/slow-sign-in
branch
from
0338e73 to
becb1b3
Compare
|
What clients are slow to sign in? My iOS is almost instant, but Mac desktop takes a long time. |
|
@chadyj well, any of them can be slow right now because duration of sign in depends on iteration over ALL messages in db. So assuming you have enough messages it might be 20-30 seconds or even longer. At least @jarradh and @pombeirp are experiencing this problem, and it can be reproduced.
so the reason for the recent preformance regression is this code https://github.com/status-im/status-react/blob/26ec3f8cd7906a0a80cd75a937e8c8028a358fa9/src/status_im/data_store/messages.cljs#L65 , but also we iterate all messages here https://github.com/status-im/status-react/blob/26ec3f8cd7906a0a80cd75a937e8c8028a358fa9/src/status_im/data_store/messages.cljs#L43 and this happens during sign in as well this PR already contains the fix for the first iteration mentioned, but i'm still working on the second. Besides this problem, even on empty account sign in takes 3.2-3.5s on galaxy s9 (so it might be slower on many other android devices) which is not perfect, as i can say. There are ways to improve this duration as well, but i'll probably focus on this in another PR. |
rasom
force-pushed
the
fix/slow-sign-in
branch
from
ec5b96e to
0f2955d
Compare
|
I downloaded the build to try it, but after 7 seconds I got the warning that it couldn't decrypt the db so I'll wait until it is rebased on latest develop. |
|
thanks for trying, @pombeirp! |
rasom
force-pushed
the
fix/slow-sign-in
branch
2 times, most recently
from
82b7385 to
084ab9d
Compare
rasom
force-pushed
the
fix/slow-sign-in
branch
from
a8ddff4 to
cff385d
Compare
OK, so feel free to merge once review is done. |
I don't understand on the recipient end this message isn't this message stored? If that's the case is there a clock-value there to differentiate messages from the same sender and avoid overwritting the previous ones? |
|
Also I agree about the rest it seems like a reasonable change and a broader fix for message-ids can be done later. If the group chat thing is a non-issue I'll remove by request for changes |
the way to test it is to create a group chat, make sure that update was sent and check if there are messages with |
|
definitely do not wait for me, the message id in that case is used to show
the sent status in the callback, just send a group chat message and check
that the spinner goes away, collision might be a problem if multiple
messages are sent in quick succession, but why can't we just leave the code
as it was?
…On Wed, Nov 21, 2018, 10:06 AM Roman Volosovskyi :: Darkviolet Lightgreen Halcyon ***@***.*** wrote:
if that's the case is there a clock-value there to differentiate messages
from the same sender and avoid overwritting the previous ones?
the way to test it is to create a group chat, make sure that update was
sent and check if there are messages with clock-value = 0 in db. Will
check this later today.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#6722 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA-EsJ-3BVxntkqtils9Tqgk06eNf94Tks5uxSWTgaJpZM4YejiL>
.
|
do you mean the way how that id was generated for membership update or which part of code? |
rasom
force-pushed
the
fix/slow-sign-in
branch
2 times, most recently
from
05cedd3 to
9fcce33
Compare
|
@lukaszfryc did you test migration from the older version of app? |
97% of end-end tests have passedFailed tests (2)Click to expand
Passed tests (56)Click to expand |
|
@rasom yes, and I just re-tested it on Android. Works fine. |
|
thanks @lukaszfryc |
fix iterating over all messages from realm db (was done for deduplication) async loading of chats (:init-chats event)
rasom
force-pushed
the
fix/slow-sign-in
branch
from
9fcce33 to
d66198a
Compare
Issue was caused by #6722 Implementation: 1. `old-message-id` field (indexed) was introduced in `message` entity and is calculated as `message-id` was calculated in `0.9.31` ```clojure (defn old-message-id [message] (sha3 (pr-str message))) ``` 2. When a reply message is sent from the PR version of app both `response-to` and `response-to-v2` fields are sent as a part of `message`'s `content` field, so that it can be recognized by `0.9.31`. 3. When PR version of app receives reply from `0.9.31` we check whether message's `content` contains `response-to` but doesn't contain `response-to-v2`, and if so we check whether DB contains message with `old-message-id=response-to`. If such message has been found we assoc `response-to-v2` to content. 4. If message from DB contains only `response-to` but not `response-to-v2` attempt to fetch the message by `old-message-id` is done.
Issue was caused by #6722 Implementation: 1. `old-message-id` field (indexed) was introduced in `message` entity and is calculated as `message-id` was calculated in `0.9.31` ```clojure (defn old-message-id [message] (sha3 (pr-str message))) ``` 2. When a reply message is sent from the PR version of app both `response-to` and `response-to-v2` fields are sent as a part of `message`'s `content` field, so that it can be recognized by `0.9.31`. 3. When PR version of app receives reply from `0.9.31` we check whether message's `content` contains `response-to` but doesn't contain `response-to-v2`, and if so we check whether DB contains message with `old-message-id=response-to`. If such message has been found we assoc `response-to-v2` to content. 4. If message from DB contains only `response-to` but not `response-to-v2` attempt to fetch the message by `old-message-id` is done.
rasom
changed the title
12 participants
Summary:
Signing in is slow for account with tons of messages. This is caused by many reasons (see this note), this PR fixes some of them:
Review notes (optional):
message-idis calculated assha3(from + chat-id + clock-value)for each messageobjectForPrimaryKeyis used to check ifmessage-idalready exists in DB:init-chatsevent is dispatched 100ms after:home'scomponentDidMounton mobile. The reason for this is that otherwise on Android event will handled before:homeis actually rendered and it will be shown only after:init-chats.messages-idinmessage(:messages-id,:reference-idin:content),messages-status(:status-id,:messages-id) entities, let me now if any other reference was missedmessage-idfrom the old one directly, without the message)Testing notes (optional):
Platforms (optional)
Areas that maybe impacted (optional)
Functional
Steps to test:
status: ready