handle multiple incoming messages at once by rasom · Pull Request #4260 · status-im/status-legacy

rasom · 2018-05-14T18:48:55Z

Idea of this PR #4260 is to make changes to web3 so that all messages received from go-ethereum side as a batch are sent to client all together, instead of separate event for each. status-im/web3.js@47318ea#diff-2ccb13c15898607b8166dd84c582b56dL124

Thus all these messages are handled together in react app.
I run the next test:

sent 100 messages to empty chat when chat is opened. Measured time between calling http://offsite.chat:8099/ping/ and first rendered message with “PING no 99X”
went back to chats list. Sent same 1000 messages. Measured time between http://offsite.chat:8099/ping/ and appearing of label with 100 unread messages
went again to chat, waited for all messages to be rendered. Then sent 1000 messages again an d measured time as in 1.

for develop results are
02:00,41 (minutes:seconds)
02:24,03
02:09,71

for experiment
00:54,76
00:35,02
01:01,17

Though for 100 messages results are less noticeable:

Old
00:12,53
00:08,01
00:17,24

New
00:10,02
00:04,58
00:15,54

status: ready

cammellos · 2018-05-15T10:17:32Z

-        ;; regular non command message, we can add it right away
-       (message-model/receive message cofx)))))
+ (fn [cofx args]
+   (if (vector (first args))


did you mean vector? here? Also mind that (vector? '()) -> false so maybe is best to check for the opposite (is it a hash map?), as we might at some point do some processing.

We can probably just wrap and always call add if it's a single message, but adds a bit of overhead for single messages, but clearer code paths.

yep i mean vector? for sure, typo, thanks

janherich · 2018-05-15T10:37:58Z

@rasom If I understand the code correctly, the github:status-im/web3.js#experiment/filter web3.js version now always returns array of messages instead of one message object.
In that case I think that the correct solution would be to change signature of models.message/receive to always take sequence of messages as the first argument instead of the one message object + change the jail code so that previews/shortPreviews can be fetched for multiple messages in one call.
Doing so will disperse the need for the hacky effect masking dispatch underneath (:chat-received-message/add-fx), and make it possible to more efficiently receive multiple messages by performing some things like reindexing message groups (not in develop, but PR with it opened) more smartly (only once per message batch).
I understand that the idea was apparently to make it work with minimal changes (and that's great for general perf profiling) but it seems like it was "bolted" on additionally on the message receive model instead of the proper design.

rasom · 2018-05-15T10:43:42Z

yes sure @janherich, idea was to check difference asap

rasom · 2018-05-15T10:44:51Z

by performing some things like reindexing message groups (not in develop, but PR with it opened) more smartly (only once per message batch).

though this is not going to be included into this PR for sure

janherich · 2018-05-15T10:48:05Z

+            (let [temp-cofx (handlers-macro/update-db cofx fx)]
+              (handlers-macro/safe-merge
+               fx
+               (add-one-message [message] temp-cofx))))


We can't just reduce effects produced by add-one-message:
What if there are multiple command type messages in the batch and all needs their preview to be rendered?
What if the messages are for currently opened chat and we need to produce protocol seen messages for all of them ?
What about adding request objects to realm (currently, there is no plural effect for that, till my realm-transactions-2 PR is merged).
Thats why I think we need to tackle this differently.

What if there are multiple command type messages in the batch and all needs their preview to be rendered?

we can always "accumulate" these preview requests here, not a big deal

What if the messages are for currently opened chat and we need to produce protocol seen messages for all of them ?

same answer, if we need to call some fx it can be batched as well

What about updating adding request objects to realm (currently, there is no plural effect for that, till my realm-transactions-2 PR is merged).

so we will merge it, anyway

What if the messages are for currently opened chat and we need to produce protocol seen messages for all of them ?

tbh i even doubt that this makes sense in case if user opened public group after starting app and then received like 100 messages from mailserver or so. Why these messages should be marked as seen? That doesn't make sense, the fact that chat was opened doesn't mean that messages were seen. I believe this should be handled completely differently considering UX pov.

@rasom We don't even have send confirmations for public chats anymore (@cammellos removed them recently). But the same situation can happen in 1-1 chat (going online after longer time and receiving bunch of messages from offline inboxing) and there, it makes perfect sense to send just one seen protocol message with N confirmations, instead of N protocol messages (whisper envelopes are expensive).
Regarding if it makes sense to mark messages as seen upon opening the chat, I don't know how to otherwise detect it, as the condition that chat is opened and new message arrived is synonymous with rendering the message in messages list. The only alternative I see is having some explicit confirmation or much more aggressive lazy rendering when only what's in the content of the screen is really rendered (and hooking seen confirmation into that).

one seen protocol message with N confirmations

if you actually seen them and not just received. But we can't know this when handling message.

as the condition that chat is opened and new message arrived is synonymous with rendering the message in messages list

That's what we will change soon as well. There is no need to render all incoming messages.

if you actually seen them and not just received. But we can't know this when handling message.

Sure you can - any good design should be data first. Currently we know that because of the :current-chat-id in db and :chat-id of the message, in the future, with lazy rendering (the change you are talking about), what's rendered will be determined by some range like :rendered-messages [from to] and you will now if the new message is destined for the currently opened chat and will be included in rendered range.

and also we need to take into account how long is message, how big is screen and is keyboard shown or not. I still doubt that this makes sense on handling, though it's doable, I agree :D

janherich · 2018-05-15T10:50:15Z

@rasom I thought that this is "normal" PR which is to be merged into develop, not experiment only. Sorry for the confusion.

rasom · 2018-05-15T10:53:49Z

Nope it is normal PR which is going to be merged. Though perfect design is not objective as well, objective is performance.

cammellos · 2018-05-15T11:05:37Z

web3.js version now always returns array of messages
@rasom is that the case?

Also @janherich just to understand correctly, you are saying that at the moment message preview will not work if multiple messages needs a preview?

If both are true, it seems to me easier to do as janerich proposed, changing the signature of receive to handle a collection and less prone to error, rather than accumulating, what do you think @rasom ? How long do you think it would take to implement one or the other? Also do you have a clear preference or is mainly time your concern?

rasom · 2018-05-15T16:03:56Z

tbh i'm not sure that i really get how moving this stuff to models.message/receive will help with avoiding accumulation, because :chat-received-message/add-fx is actually returned by StatusMessage/receive. So i mean, if we are going to have this method for each message, and this message returns effects we will have to merge("accumulate") them anyway.

So my point is pretty simple, if we can quickly find the way to batch effects produced by StatusMessage/receive without rewriting the way how messages are handled atm - let's do this.

change the jail code so that previews/shortPreviews can be fetched for multiple messages in one call

this can be done as a separate PR, anyway impact on performance is pretty small because we are not using commands that heavily, main objective are regular messages anyway

janherich · 2018-05-15T16:21:47Z

@rasom I think the point is rewriting the model.message/receive method to always accept array of messages -> I don't see how rewriting that is problematic, it will be fairly easy (the "hardest" part is fetching multiple previews/shortPreviews from jail) and will pay off big time, much cleaner code + further optimisations (for example instead of producing N :message-seen protocol messages, only one will be produced, as that protocol message can already take multiple message ids in payload, or the more efficient message-groups indexing logic I mentioned).
It's not like we are chasing some super critical fix which has to be done in 1 day, so I don't see any reason why not to do it properly from the start, it's not such big effort (we are talking about 2-3 days of work max).
If you insist, we can of course proceed with the quick-fix solution, although I'm pretty sure the PR as it stands will fail QA with multiple regressions (request handling, seen messages handling + previews) and fixes to them will make it more complicated.

rasom · 2018-05-16T05:13:39Z

I think the point is rewriting the model.message/receive method to always accept array of messages

as i said, this is only about moving of place where effects produced by StatusMessage/receive are merged, i will do this, that's really easy. It doesn't change the fact that we would need merge those effects

we are talking about 2-3 days of work max

for now 2-3 days is a big effort and big amount of time, considering that i have other ideas for improvements on which i would like to work and release should happen soon.

So as i said, i will make sure that all effects produced by StatusMessage/receive can be merged ("accumulated"), and that's it. @janherich @cammellos If you guys want to make any further improvements or want to make other things "properly" - don't hesitate to take over this task later. For now if we will have 15-30% speed up on 100 incoming messages - we are good to go.

rasom · 2018-05-16T06:20:25Z

oh wow, i'm taking my words back, i'm not going to touch model.message/receive at all

rasom · 2018-05-16T06:45:46Z

How we handle incoming message in 1:1 and pub chats in develop is:

:protocol/receive-whisper-message is dispached
it calls StatusMessage/receive under the hood, and this method returns some effects. We are not going to change this method (its interface) to handle multiple messages, because this doesn't make sense as that's how we handle polymorphism for different types/versions of messages
for 1:1 messages and pub chat messages resulting effect is dispatching of :chat-received-message/add (well, it might be handled without dispatching events at all but as we have cyclic deps in this properly designed way to handle messages... ¯\(ツ)/¯)
:chat-received-message/add actually produces changes to app-db and necessary effects for a new messages

So as objective here are pub groups and 1-1 messages what we need to ensure is that dispatching of :chat-received-message/add batched properly, and that effects produced by :chat-received-message/add are batched properly too.
In order to dispatch multiple :chat-received-message/add, :chat-received-message/add-fx effect was added.
And now we need to make sure that effects produced by :chat-received-message/add are batched properly as well, these effects are :db, :call-jail, :data-store/tx, :data-store/save-request (until it is rewritten).

So that's literally it, what is planned to be done by this PR. I don't see why refactoring of the way how we send requests to jail should be included to the scope, as well why rewriting of models.message/receive.

yenda · 2018-05-16T11:19:55Z

+
+(re-frame/reg-fx
+ :shh/add-new-sym-keys
+ (fn [keys]


Is this working ? Isn't it suppose to receive a map as parameter ? and add-new-sym-key as well ?

Here in PR it receives vector always. add-new-sym-key receives map. Basically messaging is working in this PR, so i believe it's not broken.

it is broken but we use public key for messages. try updating a contact

maybe it is not broken but it must be renamed args because keys is just plain wrong this is not keys

yenda · 2018-05-16T11:38:19Z

-       (message-model/receive message cofx)))))
+ (fn [cofx args]
+   (if (vector? (first args))
+     (add args cofx)


add-messages

i don't get what you mean by this

yenda · 2018-05-16T11:38:27Z

+ (fn [cofx args]
+   (if (vector? (first args))
+     (add args cofx)
+     (add-one-message args cofx))))


add-message

both names are too un-conventionnal, add-message and add-messages has clearer intent

yenda · 2018-05-16T11:38:59Z

+      (message/receive status-message (or chat-id sig) sig cofx))))
+
+(defn receive-whisper-message [{:keys [db] :as cofx} [js-error js-message chat-id]]
+  (reduce (fn [fx message]


docstring explaining that a whisper message can contain a vector of messages or a single message

i will make sure that whisper message will always be a single now, at least if you are speaking about message parameter, that's always single message. No need for doc exactly here

maybe my comment should be one line above, I mean that js-message is a vector of message. is it always the case ?

yes, i will rename it to js messages

yenda · 2018-05-16T22:21:05Z

      (handlers-macro/merge-fx cofx
-                               {:shh/get-new-sym-key {:web3       (:web3 db)
-                                                      :on-success on-success}}
+                               {:shh/get-new-sym-key [{:web3       (:web3 db)


why isn't this one plural ?

because i forget to rename it

yenda · 2018-05-16T22:22:40Z

-                 :on-error   log-error})))
+ :shh/add-new-sym-keys
+ (fn [args]
+   (doseq [key args]


this is still args. first it's a list of maps of args and then a map of args that is passed to add-new-sym-key, still not a key

eh man, so how would you like to rename args and key here?

I can't think of a good name for both :(
key -> add-new-sym-key-params
would be a good start to remove possible confusion

rasom · 2018-05-17T10:44:59Z

apk uploaded to https://i.diawi.com/3TYhTw for branch PR-4260

Install - Diawi - Development and In-house Apps Wireless Installation

rasom · 2018-05-17T10:52:07Z

ipa uploaded to https://i.diawi.com/SaZwT2 for branch PR-4260

Install - Diawi - Development and In-house Apps Wireless Installation

statustestbot · 2018-05-17T14:31:13Z

93% of end-end tests have passed

Total executed tests: 15
Failed tests: 1
Passed tests: 14

Failed tests (1)

Click to expand

1. test_send_stt_from_wallet_via_enter_recipient_address
Tap on PassphraseInput Enter 'six runway asthma blur secret rebuild parent logic horror decline rib buyer' using native keyboard `E selenium.common.exceptions.WebDriverException: Message: An unknown server-side error occurred while processing the command. Original error: Could not proxy command to remote server. Original error: Error: ESOCKETTIMEDOUT` Device sessions: Device 1

Passed tests (14)

Click to expand

1. test_one_to_one_chat_messages
Device sessions: Device 1 Device 2
2. test_send_transaction_from_daap
Device sessions: Device 1
3. test_send_eth_from_wallet_sign_now
Device sessions: Device 1
4. test_send_eth_to_request_from_wallet
Device sessions: Device 1 Device 2
5. test_contact_profile_view
Device sessions: Device 1
6. test_public_chat
Device sessions: Device 1 Device 2
7. test_network_switch
Device sessions: Device 1
8. test_transaction_send_command_one_to_one_chat
Device sessions: Device 1
9. test_browse_link_entering_url_in_dapp_view
Device sessions: Device 1
10. test_transaction_send_command_wrong_password
Device sessions: Device 1
11. test_send_eth_to_request_in_one_to_one_chat
Device sessions: Device 1 Device 2
12. test_send_eth_to_request_in_group_chat
Device sessions: Device 1 Device 2
13. test_transaction_send_command_group_chat
Device sessions: Device 1
14. test_group_chat_messages_and_delete_chat
Device sessions: Device 1 Device 2 Device 3

rasom · 2018-05-18T11:27:11Z

apk uploaded to https://i.diawi.com/Hhs8aN for branch PR-4260

Install - Diawi - Development and In-house Apps Wireless Installation

rasom · 2018-05-18T11:34:19Z

ipa uploaded to https://i.diawi.com/PmXBLP for branch PR-4260

Install - Diawi - Development and In-house Apps Wireless Installation

rasom force-pushed the experiment/handle-many-incoming-messages-at-once branch from 03049a8 to b1eaf12 Compare May 14, 2018 19:02

rasom self-assigned this May 15, 2018

rasom changed the title ~~[WIP] handle many incoming messages at once~~ handle many incoming messages at once May 15, 2018

cammellos reviewed May 15, 2018

View reviewed changes

janherich suggested changes May 15, 2018

View reviewed changes

yenda reviewed May 16, 2018

View reviewed changes

rasom force-pushed the experiment/handle-many-incoming-messages-at-once branch from 19a9d13 to 98b54a4 Compare May 16, 2018 17:59

yenda reviewed May 16, 2018

View reviewed changes

rasom force-pushed the experiment/handle-many-incoming-messages-at-once branch from 98b54a4 to b27bd2b Compare May 17, 2018 03:29

rasom changed the title ~~handle many incoming messages at once~~ handle multiple incoming messages at once May 17, 2018

rasom force-pushed the experiment/handle-many-incoming-messages-at-once branch 2 times, most recently from 7e1731e to 602220b Compare May 17, 2018 09:38

yenda approved these changes May 17, 2018

View reviewed changes

janherich approved these changes May 17, 2018

View reviewed changes

asemiankevich self-assigned this May 18, 2018

asemiankevich added the Tested - OK label May 18, 2018

handle multiple incoming messages at once

9b5058d

rasom force-pushed the experiment/handle-many-incoming-messages-at-once branch from 087a567 to 9b5058d Compare May 18, 2018 11:13

rasom merged commit 9b5058d into develop May 18, 2018

rasom deleted the experiment/handle-many-incoming-messages-at-once branch May 18, 2018 11:15

StatusWrike mentioned this pull request Nov 9, 2020

Bump pillow from 5.1.0 to 7.1.0 in /test/appium #11409

Closed

Uh oh!

Conversation

rasom commented May 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cammellos May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janherich commented May 15, 2018

Uh oh!

rasom commented May 15, 2018

Uh oh!

rasom commented May 15, 2018

Uh oh!

janherich May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rasom May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janherich commented May 15, 2018

Uh oh!

rasom commented May 15, 2018

Uh oh!

cammellos commented May 15, 2018

Uh oh!

rasom commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janherich commented May 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rasom commented May 16, 2018

Uh oh!

rasom commented May 16, 2018

Uh oh!

rasom commented May 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rasom commented May 14, 2018 •

edited

Loading

cammellos May 15, 2018 •

edited

Loading

janherich May 15, 2018 •

edited

Loading

rasom May 15, 2018 •

edited

Loading

rasom commented May 15, 2018 •

edited

Loading

janherich commented May 15, 2018 •

edited

Loading

rasom May 17, 2018 •

edited

Loading

rasom commented May 17, 2018 •

edited by ghost

Loading

rasom commented May 17, 2018 •

edited by ghost

Loading

rasom commented May 18, 2018 •

edited by ghost

Loading

rasom commented May 18, 2018 •

edited by ghost

Loading