955600 - UI lags while stats service is receiving chat rooms

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, defect)

Description

[(inactive) Nihanth Subramanya [:nhnt11]]

*** Original post on bio 2160 at 2013-09-06 23:18:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***

Comment 1

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch

*** Original post on bio 2160 as attmnt 2844 at 2013-09-06 23:18:00 UTC ***

The patch introduces a queue of pending chats, and adds them asynchronously in batches.

Comment 2

[aleth [:aleth]]

Comment on attachment 8354614 [details] [diff] [review]
Patch

*** Original change on bio 2160 attmnt 2844 at 2013-09-07 15:31:37 UTC ***

>+  _addPendingChats: function() {
>+    if (!this._addingPendingChats)
>+      return;

Do we need this check?

>+    let begin = Date.now();
>+    while (this._pendingChats.length) {
>+      let chat = this._pendingChats.shift();

Would pop() be faster than shift()?

>+      if (Date.now() - begin > 40) {
>+        executeSoon(this._addPendingChats.bind(this));
>+        return;
>+      }
>     }
>-    // If a chat is already added, we remove it and re-add to refresh.
>-    else if (chatList.has(aRoomInfo.name)) {
>-      this._chats.splice(
>-        this._chats.indexOf(chatList.get(aRoomInfo.name)), 1);
>+    let now = Date.now();
>+    if (this._forceUpdateNotification ||
>+        now - this._lastUpdateNotification > 100) {
>+      this._notifyObservers("updated");
>+      this._lastUpdateNotification = now;

No update notifications will be sent until the very end if the pendingChat list is never emptied completely during processing. This seems wrong to me?

>+      this._forceUpdateNotification = false;

delete this._forceUpdateNotification;

>+            this._pendingChats = this._pendingChats.concat(aRoomInfo);

I thought you had replaced this with an enumerator (a single wrapped object as opposed to 250) when you went back to batches of 250?

Comment 3

[Florian Quèze [:florian]]

Comment on attachment 8354614 [details] [diff] [review]
Patch

*** Original change on bio 2160 attmnt 2844 at 2013-09-07 16:09:41 UTC ***

>   // Callbacks receive at most this many channels per call.
>-  _channelsPerBatch: 25,
>+  _channelsPerBatch: 250,

Why this change? Any reason to think it's an improvement?

>diff --git a/instantbird/components/ibConvStatsService.js b/instantbird/components/ibConvStatsService.js

>+  // Used to force an update notification when all of an account's chat rooms
>+  // have been received.
>+  _forceUpdateNotification: false,

Shouldn't we instead keep a count of how many accounts are currently listing chatrooms, and do an immediate notification only when that count reaches 0?

>+  _addPendingChats: function() {
>+    if (!this._addingPendingChats)
>+      return;
>+    let begin = Date.now();
>+    while (this._pendingChats.length) {

I find it somewhat confusing that there's a check for the end of the loop both at the top and at the bottom.

How would you feel about:
for (let time = 0; time < 40 && this._pendingChats.length; time = Date.now() - begin)

>+      if (Date.now() - begin > 40) {
>+        executeSoon(this._addPendingChats.bind(this));
>+        return;

Having spent 40ms should exit the loop but not return, as we likely want to send a notification once in a while, as aleth noted.

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch 2

*** Original post on bio 2160 as attmnt 2855 at 2013-09-09 20:50:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 5

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8354614 [details] [diff] [review]
Patch

*** Original change on bio 2160 attmnt 2844 at 2013-09-09 20:50:41 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 6

[Florian Quèze [:florian]]

Comment on attachment 8354625 [details] [diff] [review]
Patch 2

*** Original change on bio 2160 attmnt 2855 at 2013-09-09 21:48:59 UTC ***

The code seems reasonable. I would like to profile before and after to be sure of the result though :).

Comment 7

[Florian Quèze [:florian]]

*** Original post on bio 2160 at 2013-09-10 09:31:17 UTC ***

(In reply to comment #4)

> I would like to profile before and after to be sure
> of the result though :).

I did yesterday evening and the result matched my expectations for this patch.

Comment 8

[aleth [:aleth]]

*** Original post on bio 2160 at 2013-09-11 15:25:08 UTC ***

(In reply to comment #1)
> >+    let begin = Date.now();
> >+    while (this._pendingChats.length) {
> >+      let chat = this._pendingChats.shift();
> 
> Would pop() be faster than shift()?

I would like you to check whether the fact that incoming channels may be ordered alphabetically by the server (which /may/ help when finding the position to insert) makes up for the fact that shift() is much slower than pop() (especially should the array get long, i.e. on slow machines).

Comment 9

[aleth [:aleth]]

*** Original post on bio 2160 at 2013-09-11 16:11:35 UTC ***

(In reply to comment #6)
> I would like you to check whether the fact that incoming channels may be
> ordered alphabetically by the server (which /may/ help when finding the
> position to insert) makes up for the fact that shift() is much slower than
> pop() (especially should the array get long, i.e. on slow machines).

Not intended as a rhetorical question btw! I don't know the answer.

Comment 10

[aleth [:aleth]]

*** Original post on bio 2160 at 2013-09-11 16:34:14 UTC ***

(In reply to comment #7)
As flo pointed out, the channels are actually not returned in any obvious order by freenode at least, so let's just use pop(). 

r+ with that change :)

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch 3

*** Original post on bio 2160 as attmnt 2868 at 2013-09-11 17:11:00 UTC ***

Changed shift() to pop().

Sorry for not addressing this in the previous patch.

Comment 12

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8354625 [details] [diff] [review]
Patch 2

*** Original change on bio 2160 attmnt 2855 at 2013-09-11 17:11:59 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 13

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: The real patch 3

*** Original post on bio 2160 as attmnt 2869 at 2013-09-11 17:28:00 UTC ***

Attached the wrong file, sorry.

Comment 14

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8354638 [details] [diff] [review]
Patch 3

*** Original change on bio 2160 attmnt 2868 at 2013-09-11 17:28:53 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 15

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch 4

*** Original post on bio 2160 as attmnt 2870 at 2013-09-11 17:53:00 UTC ***

Ensure _removeChatsForAccount removes chats from _pendingChats too.

Comment 16

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8354639 [details] [diff] [review]
The real patch 3

*** Original change on bio 2160 attmnt 2869 at 2013-09-11 17:53:08 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 17

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Patch 5

*** Original post on bio 2160 as attmnt 2871 at 2013-09-11 18:04:00 UTC ***

_removeChatsForAccount was trying to use PossibleChat._accountId, which is no longer available, for filtering. This exposes the account id through a getter in the PossibleChat prototype and uses it.

Comment 18

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8354640 [details] [diff] [review]
Patch 4

*** Original change on bio 2160 attmnt 2870 at 2013-09-11 18:04:44 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 19

[aleth [:aleth]]

Comment on attachment 8354641 [details] [diff] [review]
Patch 5

*** Original change on bio 2160 attmnt 2871 at 2013-09-11 18:08:51 UTC ***

Thanks! Let's try this in nightlies :)

Comment 20

[Florian Quèze [:florian]]

*** Original post on bio 2160 at 2013-09-12 22:26:13 UTC ***

I'm changing "0" to "empty" in this comment: "We send an update notification if this is 0 after adding chat rooms."

Comment 21

[Patrick Cloke [:clokep]]

*** Original post on bio 2160 at 2013-09-12 23:04:06 UTC ***

http://hg.instantbird.org/instantbird/rev/93e68de93e21