Closed Bug 955600 Opened 10 years ago Closed 10 years ago

UI lags while stats service is receiving chat rooms

Categories

(Instantbird Graveyard :: Conversation, defect)

x86
Other
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nhnt11, Assigned: nhnt11)

References

Details

Attachments

(1 file, 5 obsolete files)

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

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Patch (obsolete) — Splinter Review
*** 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.
Attachment #8354614 - Flags: review?(aleth)
Attachment #8354614 - Flags: review?(florian)
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?
Attachment #8354614 - Flags: review?(aleth) → review-
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.
Attachment #8354614 - Flags: review?(florian) → review-
Attached patch Patch 2 (obsolete) — Splinter Review
*** 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 ***
Attachment #8354625 - Flags: review?(aleth)
Attachment #8354625 - Flags: review?(florian)
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 ***
Attachment #8354614 - Attachment is obsolete: true
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 :).
Attachment #8354625 - Flags: review?(florian) → review+
*** 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.
*** 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).
*** 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.
*** 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 :)
Attached patch Patch 3 (obsolete) — Splinter Review
*** 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.
Attachment #8354638 - Flags: review?(aleth)
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 ***
Attachment #8354625 - Attachment is obsolete: true
Attachment #8354625 - Flags: review?(aleth)
Attached patch The real patch 3 (obsolete) — Splinter Review
*** Original post on bio 2160 as attmnt 2869 at 2013-09-11 17:28:00 UTC ***

Attached the wrong file, sorry.
Attachment #8354639 - Flags: review?(aleth)
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 ***
Attachment #8354638 - Attachment is obsolete: true
Attachment #8354638 - Flags: review?(aleth)
Attached patch Patch 4 (obsolete) — Splinter Review
*** Original post on bio 2160 as attmnt 2870 at 2013-09-11 17:53:00 UTC ***

Ensure _removeChatsForAccount removes chats from _pendingChats too.
Attachment #8354640 - Flags: review?(aleth)
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 ***
Attachment #8354639 - Attachment is obsolete: true
Attachment #8354639 - Flags: review?(aleth)
Attached patch Patch 5Splinter Review
*** 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.
Attachment #8354641 - Flags: review?(aleth)
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 ***
Attachment #8354640 - Attachment is obsolete: true
Attachment #8354640 - Flags: review?(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 :)
Attachment #8354641 - Flags: review?(aleth) → review+
Whiteboard: [checkin-needed]
Assignee: nobody → nhnt11
*** 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."
*** Original post on bio 2160 at 2013-09-12 23:04:06 UTC ***

http://hg.instantbird.org/instantbird/rev/93e68de93e21
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
Depends on: 955665
Depends on: 955662
Depends on: 955668
You need to log in before you can comment on or make changes to this bug.