Skip to content

Commit

Permalink
Fix message ordering and improve performance rec. messages
Browse files Browse the repository at this point in the history 
This commit does a few things:

==== Ordering of messages ====

Change the ordering of messages from a mixture of timestamp/clock-value to use
only clock-value.

Datemarks are now not used for sorting anymore, which means that the
order of messages is always causally related (not the case before, as we
were breaking this property by sorting by datemark), but datemark
calculation is unreliable (a reply to a message might have a timestamp <
then the message that is replied to).
So for timestamp calculation we
naively group them ignoring "out-of-order timestamp" messages, although
there's much to improve.
It fixes an issue whereby the user would change their time and the
message will be displayed in the past, although it is still possible to
craft a message with a lower clock value and order it in the past
(there's no way we can prevent this to some extent, but there are ways
to mitigate, but outside the scope of this PR).

==== Performance of receiving messages ====

The app would freeze on pulling messages from a mailserver (100 or so).
This is due to the JS Thread being hogged by CPU calculation, coupled
with the fact that we always tried to process messages all in one go.

This strategy can't scale, and given x is big enough (200,300,1000) the
UI will freeze.

Instead, each message is now processed separately, and we leave a gap
between processing each message for the UI to respond to user input
(otherwise the app freezes again).
Pulling messages will be longer overall, but the app will be usuable
while this happen (albeit it might slow down).
Other strategies are possible (calculate off-db and do a big swap,
avoiding many re-renders etc), but this is the reccommended strategy by
re-frame author (Solving the CPU Hog problem), so sounds like a safe
base point.

The underlying data structure for holding messages was also changed, we
used an immutable Red and Black Tree, same as a sorted map for clojure, but we use
a js library as is twice as performing then clojure sorted map.

We also don't sort messages again each time we receive them O(nlogn), but we
insert them in order O(logn).

Other data structures considered but discarded:
1) Plain vector, but performance prepending/insertion in the middle
(both O(n)) were not great, as not really suited for these operations.

2) Linked list, appealing as append/prepend is O(1), while insertion is
O(n). This is probably acceptable as messages tend to come in order
(from the db, so adding N messages is O(n)), or the network (most of
them prepends, or close to the head), while mailserver would not follow this path.
An implementation of a linked list was built, which performed roughtly the
same as a clojure sorted-map (although faster append/prepend), but not
worth the complexity of having our own implementation.

3) Clojure sorted-map, probably the most versatile, performance were
acceptable, but nowhere near the javascript implementation we decided on

4) Priority map, much slower than a sorted map (twice as slow)

5) Mutable sorted map, js implementation, (bintrees), not explored this very much, but from
just a quick benchmark, performance were much worse that clojure
immutable sorted map

Given that each message is now processed separately, saving the chat /
messages is also debounced to avoid spamming status-go with network
requests. This is a temporary measure for now until that's done directly
in status-go, without having to ping-pong with status-react.

Next steps performance wise is to move stuff to status-go, parsing of
transit, validation, which is heavy, at which point we can re-consider
performance and how to handle messages.

Fixes also an issue with the last message in the chat, we were using the
last message in the chat list, which might not necessarely be the last
message the chat has seen, in case messages were not loaded and a more
recent message is the database (say you fetch historical messages for
1-to-1 A, you don't have any messages in 1-to-1 chat B loaded, you receive an
historical message for chat B, it sets it as last message).

Also use clj beans instead of js->clj for type conversion

Signed-off-by: Andrea Maria Piana <andrea.maria.piana@gmail.com>
  • Loading branch information
@cammellos
cammellos committed Nov 1, 2019
1 parent e579412 commit c69863c
Show file tree
Hide file tree
Showing 34 changed files with 902 additions and 871 deletions.
2 changes: 2 additions & 0 deletions clj-rn.conf.edn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
"react-navigation"
"react-native-navigation-twopane"
"hi-base32"
"functional-red-black-tree"
"react-native-mail"
"react-native-shake"
"@react-native-community/netinfo"]
Expand Down Expand Up @@ -74,6 +75,7 @@
"react-native-desktop-gesture-handler"
"web3-utils"
"react-navigation"
"functional-red-black-tree"
"react-native-navigation-twopane"
"hi-base32"]

Expand Down
3 changes: 2 additions & 1 deletion desktop/js_files/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
"emojilib": "^2.2.9",
"eth-phishing-detect": "^1.1.13",
"events": "^1.1.1",
"functional-red-black-tree": "^1.0.1",
"google-breakpad": "git+https://github.com/status-im/google-breakpad.git#v0.9.0",
"hi-base32": "^0.5.0",
"i18n-js": "^3.1.0",
Expand Down Expand Up @@ -52,8 +53,8 @@
"@babel/preset-env": "7.1.0",
"@babel/register": "7.6.2",
"babel-preset-react-native": "^5.0.2",
"metro-react-native-babel-preset": "^0.45.6",
"coveralls": "^3.0.4",
"metro-react-native-babel-preset": "^0.45.6",
"nyc": "^14.1.1",
"patch-package": "^5.1.1",
"rn-snoopy": "git+https://github.com/status-im/rn-snoopy.git#v2.0.2-status"
Expand Down
5 changes: 5 additions & 0 deletions desktop/js_files/yarn.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3210,6 +3210,11 @@ function-bind@^1.0.2, function-bind@^1.1.1:
resolved "https://registry.yarnpkg.com/function-bind/-/function-bind-1.1.1.tgz#a56899d3ea3c9bab874bb9773b7c5ede92f4895d"
integrity sha512-yIovAzMX49sF8Yl58fSCWJ5svSLuaibPxXQJFLmBObTuCr0Mf1KiPopGM9NiFjiYBCbfaa2Fh6breQ6ANVTI0A==

functional-red-black-tree@^1.0.1:
version "1.0.1"
resolved "https://registry.yarnpkg.com/functional-red-black-tree/-/functional-red-black-tree-1.0.1.tgz#1b0ab3bd553b2a0d6399d29c0e3ea0b252078327"
integrity sha1-GwqzvVU7Kg1jmdKcDj6gslIHgyc=

gauge@~1.2.5:
version "1.2.7"
resolved "https://registry.yarnpkg.com/gauge/-/gauge-1.2.7.tgz#e9cec5483d3d4ee0ef44b60a7d99e4935e136d93"
Expand Down
10 changes: 10 additions & 0 deletions externs.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -575,6 +575,16 @@ var TopLevel = {
"FIRSTWEEKCUTOFFDAY": function () {},
"decimalPlaces": function () {},
"_android": function () {},
"next": function() {},
"prev": function() {},
"hasNext": function() {},
"hasPrev": function() {},
"key": function() {},
"keys": function() {},
"values": function() {},
"find": function() {},
"insert": function() {},
"update": function() {},
"isSupported" : function () {},
"authenticate" : function () {},
"createAppContainer" : function () {},
Expand Down
1 change: 1 addition & 0 deletions mobile/js_files/package.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
"create-react-class": "^15.6.2",
"emojilib": "^2.4.0",
"eth-phishing-detect": "^1.1.13",
"functional-red-black-tree": "^1.0.1",
"hermes-engine": "0.2.1",
"hi-base32": "^0.5.0",
"i18n-js": "^3.3.0",
Expand Down
5 changes: 5 additions & 0 deletions mobile/js_files/yarn.lock
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2577,6 +2577,11 @@ function-bind@^1.1.1:
resolved "https://registry.yarnpkg.com/function-bind/-/function-bind-1.1.1.tgz#a56899d3ea3c9bab874bb9773b7c5ede92f4895d"
integrity sha512-yIovAzMX49sF8Yl58fSCWJ5svSLuaibPxXQJFLmBObTuCr0Mf1KiPopGM9NiFjiYBCbfaa2Fh6breQ6ANVTI0A==

functional-red-black-tree@^1.0.1:
version "1.0.1"
resolved "https://registry.yarnpkg.com/functional-red-black-tree/-/functional-red-black-tree-1.0.1.tgz#1b0ab3bd553b2a0d6399d29c0e3ea0b252078327"
integrity sha1-GwqzvVU7Kg1jmdKcDj6gslIHgyc=

gauge@~2.7.3:
version "2.7.4"
resolved "https://registry.yarnpkg.com/gauge/-/gauge-2.7.4.tgz#2c03405c7538c39d7eb37b317022e325fb018bf7"
Expand Down
3 changes: 2 additions & 1 deletion project.clj
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,8 @@
[day8.re-frame/tracing "0.5.0"]
[hawk "0.2.11"]]
:source-paths ["src" "env/dev" "react-native/src/cljsjs" "components/src" "dev"]}]
:test {:dependencies [[day8.re-frame/test "0.1.5"]]
:test {:dependencies [[com.taoensso/tufte "2.1.0"]
[day8.re-frame/test "0.1.5"]]
:plugins [[lein-doo "0.1.9"]]
:cljsbuild {:builds
[{:id "test"
Expand Down
177 changes: 48 additions & 129 deletions src/status_im/chat/db.cljs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,63 +53,59 @@
{}
chats))

(defn sort-message-groups
"Sorts message groups according to timestamp of first message in group"
[message-groups messages]
(sort-by
(comp :timestamp (partial get messages) :message-id first second)
message-groups))

(defn quoted-message-data
"Selects certain data from quoted message which must be available in the view"
[message-id messages]
(when-let [{:keys [from content]} (get messages message-id)]
{:from from
:text (:text content)}))

(defn add-datemark
[[datemark
message-references]]
(let [{:keys [whisper-timestamp timestamp]} (first message-references)]
(conj message-references
{:value datemark
:type :datemark
:whisper-timestamp whisper-timestamp
:timestamp timestamp})))

(defn datemark? [{:keys [type]}]
(= type :datemark))

(defn intersperse-datemark
"Reduce step which expects the input list of messages to be sorted by clock value.
It makes best effort to group them by day.
We cannot sort them by :timestamp, as that represents the clock of the sender
and we have no guarantees on the order.
We naively and arbitrarly group them assuming that out-of-order timestamps
fall in the previous bucket.
A sends M1 to B with timestamp 2000-01-01T00:00:00
B replies M2 with timestamp 1999-12-31-23:59:59
M1 needs to be displayed before M2
so we bucket both in 1999-12-31"
[{:keys [acc last-timestamp last-datemark]} {:keys [whisper-timestamp datemark] :as msg}]
(cond (empty? acc) ; initial element
{:last-timestamp whisper-timestamp
:last-datemark datemark
:acc (conj acc msg)}

(and (not= last-datemark datemark) ; not the same day
(< whisper-timestamp last-timestamp)) ; not out-of-order
{:last-timestamp whisper-timestamp
:last-datemark datemark
:acc (conj acc {:value last-datemark ; intersperse datemark message
:type :datemark}
msg)}
:else
{:last-timestamp (min whisper-timestamp last-timestamp) ; use last datemark
:last-datemark last-datemark
:acc (conj acc (assoc msg :datemark last-datemark))}))

(defn add-datemarks
"Add a datemark in between an ordered seq of messages when two datemarks are not
the same. Ignore messages with out-of-order timestamps"
[messages]
(when (seq messages)
(let [messages-with-datemarks (:acc (reduce intersperse-datemark {:acc []} messages))]
; Append last datemark
(conj messages-with-datemarks {:value (:datemark (peek messages-with-datemarks))
:type :datemark}))))

(defn gap? [{:keys [type]}]
(= type :gap))

(defn transform-message
[messages]
(fn [{:keys [message-id timestamp-str] :as reference}]
(if (or (datemark? reference)
(gap? reference))
reference
(let [{:keys [content quoted-message] :as message} (get messages message-id)
{:keys [response-to response-to-v2]} content
quote (if quoted-message
quoted-message
(some-> (or response-to-v2 response-to)
(quoted-message-data messages)))]
(cond-> (-> message
(update :content dissoc :response-to :response-to-v2)
(assoc :timestamp-str timestamp-str))
;; quoted message reference
quote
(assoc-in [:content :response-to] quote))))))

(defn check-gap
[gaps previous-message next-message]
(let [previous-timestamp (:whisper-timestamp previous-message)
next-whisper-timestamp (:whisper-timestamp next-message)
next-timestamp (quot (:timestamp next-message) 1000)
next-timestamp (:timestamp next-message)
ignore-next-message? (> (js/Math.abs
(- next-whisper-timestamp next-timestamp))
120)]
120000)]
(reduce
(fn [acc {:keys [from to id]}]
(if (and next-message
Expand Down Expand Up @@ -137,15 +133,13 @@
:value (clojure.string/join (:ids gaps))
:gaps gaps}))

(defn messages-with-datemarks
(defn add-gaps
"Converts message groups into sequence of messages interspersed with datemarks,
with correct user statuses associated into message"
[message-groups messages messages-gaps
[message-list messages-gaps
{:keys [highest-request-to lowest-request-from]} all-loaded? public?]
(transduce
(comp
(mapcat add-datemark)
(map (transform-message messages)))
(map identity)
(fn
([]
(let [acc {:messages (list)
Expand All @@ -162,9 +156,8 @@
:value (str :first-gap)
:first-gap? true})
acc)))
([{:keys [messages datemark-reference previous-message gaps]} message]
(let [new-datemark? (datemark? message)
{:keys [gaps-number gap]}
([{:keys [messages previous-message gaps]} message]
(let [{:keys [gaps-number gap]}
(check-gap gaps previous-message message)
add-gap? (pos? gaps-number)]
{:messages (cond-> messages
Expand All @@ -173,90 +166,16 @@
(add-gap gap)

:always
(conj
(cond-> message
(not new-datemark?)
(assoc
:datemark
(:value datemark-reference)))))
(conj message))
:previous-message message
:datemark-reference (if new-datemark?
message
datemark-reference)
:gaps (if add-gap?
(drop gaps-number gaps)
gaps)}))
([{:keys [messages gaps]}]
(cond-> messages
(seq gaps)
(add-gap {:ids (map :id gaps)}))))
message-groups))

(defn- set-previous-message-info [stream]
(let [{:keys [display-photo? message-type] :as previous-message} (peek stream)]
(conj (pop stream) (assoc previous-message
:display-username? (and display-photo?
(not= :system-message message-type))
:first-in-group? true))))

(defn display-photo? [{:keys [outgoing message-type]}]
(or (= :system-message message-type)
(and (not outgoing)
(not (= :user-message message-type)))))

;; any message that comes after this amount of ms will be grouped separately
(def ^:private group-ms 60000)

(defn add-positional-metadata
"Reduce step which adds positional metadata to a message and conditionally
update the previous message with :first-in-group?."
[{:keys [stream last-outgoing-seen]}
{:keys [type message-type from datemark outgoing timestamp] :as message}]
(let [previous-message (peek stream)
;; Was the previous message from a different author or this message
;; comes after x ms
last-in-group? (or (= :system-message message-type)
(not= from (:from previous-message))
(> (- (:timestamp previous-message) timestamp) group-ms))
;; Have we seen an outgoing message already?
last-outgoing? (and (not last-outgoing-seen)
outgoing)
datemark? (= :datemark (:type message))
;; If this is a datemark or this is the last-message of a group,
;; then the previous message was the first
previous-first-in-group? (or datemark?
last-in-group?)
new-message (assoc message
:display-photo? (display-photo? message)
:last-in-group? last-in-group?
:last-outgoing? last-outgoing?)]
{:stream (cond-> stream
previous-first-in-group?
;; update previous message if necessary
set-previous-message-info

:always
(conj new-message))
;; mark the last message sent by the user
:last-outgoing-seen (or last-outgoing-seen last-outgoing?)}))

(defn messages-stream
"Enhances the messages in message sequence interspersed with datemarks
with derived stream context information, like:
`:first-in-group?`, `last-in-group?`, `:last?` and `:last-outgoing?` flags."
[ordered-messages]
(when (seq ordered-messages)
(let [initial-message (first ordered-messages)
message-with-metadata (assoc initial-message
:last-in-group? true
:last? true
:display-photo? (display-photo? initial-message)
:last-outgoing? (:outgoing initial-message))]
(->> (rest ordered-messages)
(reduce add-positional-metadata
{:stream [message-with-metadata]
:last-outgoing-seen (:last-outgoing? message-with-metadata)})
:stream))))
(reverse message-list)))

(def map->sorted-seq
(comp (partial map second) (partial sort-by first)))
Expand Down
32 changes: 24 additions & 8 deletions src/status_im/chat/models.cljs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
[status-im.utils.fx :as fx]
[status-im.utils.gfycat.core :as gfycat]
[status-im.utils.platform :as platform]
[status-im.utils.priority-map :refer [empty-message-map]]
[status-im.utils.utils :as utils]
[taoensso.timbre :as log]))

Expand Down Expand Up @@ -92,18 +91,35 @@
:timestamp now
:contacts #{chat-id}
:last-clock-value 0
:messages empty-message-map}))
:messages {}}))

(fx/defn upsert-chat
"Upsert chat when not deleted"
(fx/defn ensure-chat
"Add chat to db and update"
[{:keys [db] :as cofx} {:keys [chat-id] :as chat-props}]
(let [chat (merge
(or (get (:chats db) chat-id)
(create-new-chat chat-id cofx))
chat-props)]
(fx/merge cofx
{:db (update-in db [:chats chat-id] merge chat)}
(chats-store/save-chat chat))))
{:db (update-in db [:chats chat-id] merge chat)}))

(fx/defn upsert-chat
"Upsert chat when not deleted"
[{:keys [db] :as cofx} {:keys [chat-id] :as chat-props}]
(fx/merge cofx
(ensure-chat chat-props)
#(chats-store/save-chat % (get-in % [:db :chats chat-id]))))

(fx/defn handle-save-chat
{:events [::save-chat]}
[{:keys [db] :as cofx} chat-id]
(chats-store/save-chat cofx (get-in db [:chats chat-id])))

(fx/defn save-chat-delayed
"Debounce saving the chat"
[_ chat-id]
{:dispatch-debounce [{:key :save-chat
:event [::save-chat chat-id]
:delay 500}]})

(fx/defn add-public-chat
"Adds new public group chat to db"
Expand Down Expand Up @@ -134,7 +150,7 @@
(fx/merge
cofx
{:db (update-in db [:chats chat-id] merge
{:messages empty-message-map
{:messages {}
:message-groups {}
:last-message-content nil
:last-message-content-type nil
Expand Down
Loading

0 comments on commit c69863c

Please sign in to comment.