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)
Tracking
(Not tracked)
RESOLVED
FIXED
1.5
People
(Reporter: nhnt11, Assigned: nhnt11)
References
Details
Attachments
(1 file, 5 obsolete files)
7.46 KB,
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 2160 at 2013-09-06 23:18:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***
Assignee | ||
Comment 1•10 years ago
|
||
*** 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)
Assignee | ||
Updated•10 years ago
|
Attachment #8354614 -
Flags: review?(florian)
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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-
Assignee | ||
Comment 4•10 years ago
|
||
*** 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)
Assignee | ||
Updated•10 years ago
|
Attachment #8354625 -
Flags: review?(florian)
Assignee | ||
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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+
Comment 7•10 years ago
|
||
*** 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•10 years ago
|
||
*** 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•10 years ago
|
||
*** 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•10 years ago
|
||
*** 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 :)
Assignee | ||
Comment 11•10 years ago
|
||
*** 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)
Assignee | ||
Comment 12•10 years ago
|
||
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)
Assignee | ||
Comment 13•10 years ago
|
||
*** 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)
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
*** 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)
Assignee | ||
Comment 16•10 years ago
|
||
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)
Assignee | ||
Comment 17•10 years ago
|
||
*** 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)
Assignee | ||
Comment 18•10 years ago
|
||
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 19•10 years ago
|
||
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+
Updated•10 years ago
|
Whiteboard: [checkin-needed]
Updated•10 years ago
|
Assignee: nobody → nhnt11
Comment 20•10 years ago
|
||
*** 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•10 years ago
|
||
*** 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
You need to log in
before you can comment on or make changes to this bug.
Description
•