Closed Bug 955114 Opened 11 years ago Closed 11 years ago

Nicklist speedup: low-hanging fruit

Categories

(Instantbird Graveyard :: Conversation, defect)

Product:
Instantbird Graveyard
Component:
Conversation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(1 file, 5 obsolete files)

Patch
11.08 KB, patch
florian
: review+
Details | Diff | Splinter Review 
*** Original post on bio 1686 at 2012-09-01 21:58:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***
Attached patch Patch (obsolete) — — Splinter Review 
*** Original post on bio 1686 as attmnt 1885 at 2012-09-01 21:58:00 UTC ***

Sets buddy colours lazily and reduces the number of DOM queries needed to populate the nicklist. I have no numbers for how much this helps, but it should help some.
Attachment #8353643 - Flags: review?(clokep)
Attached patch Patch (obsolete) — — Splinter Review 
*** Original post on bio 1686 as attmnt 1886 at 2012-09-01 22:34:00 UTC ***

Even less DOM queries.
Attachment #8353644 - Flags: review?(clokep)
Comment on attachment 8353643 [details] [diff] [review]
Patch

*** Original change on bio 1686 attmnt 1885 at 2012-09-01 22:34:47 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353643 - Attachment is obsolete: true
Attachment #8353643 - Flags: review?(clokep)
Blocks: 953721
No longer blocks: 953721
Attached patch Patch with sort (obsolete) — — Splinter Review 
*** Original post on bio 1686 as attmnt 1887 at 2012-09-02 09:38:00 UTC ***

Also sorts the initial batch of nicks before adding them to the nicklist. (This won't affect all the nicks in e.g. #ubuntu, but a good chunk.)
Attachment #8353645 - Flags: review?(clokep)
Comment on attachment 8353644 [details] [diff] [review]
Patch

*** Original change on bio 1686 attmnt 1886 at 2012-09-02 09:38:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353644 - Attachment is obsolete: true
Attachment #8353644 - Flags: review?(clokep)
Attached patch Patch v4 (obsolete) — — Splinter Review 
*** Original post on bio 1686 as attmnt 1888 at 2012-09-02 10:13:00 UTC ***

Tiny speed hit, but better code.
Comment on attachment 8353645 [details] [diff] [review]
Patch with sort

*** Original change on bio 1686 attmnt 1887 at 2012-09-02 10:13:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353645 - Attachment is obsolete: true
Attachment #8353645 - Flags: review?(clokep)
Comment on attachment 8353646 [details] [diff] [review]
Patch v4

*** Original change on bio 1686 attmnt 1888 at 2012-09-02 10:17:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353646 - Flags: review?(clokep)
Comment on attachment 8353646 [details] [diff] [review]
Patch v4

*** Original change on bio 1686 attmnt 1888 at 2012-09-02 12:37:28 UTC ***

I'd rather flo take a look at this. This touches quite a bit of the participants list code, which I don't know very well.
Attachment #8353646 - Flags: review?(clokep) → review?(florian)
Assignee: nobody → aleth
Comment on attachment 8353646 [details] [diff] [review]
Patch v4

*** Original change on bio 1686 attmnt 1888 at 2012-09-23 23:27:12 UTC ***

>diff --git a/chrome/instantbird/content/instantbird/conversation.xml b/chrome/instantbird/content/instantbird/conversation.xml

>+            if (!buddy.color)
>+              this._setBuddyColor(buddy, name);
>             color = buddy.color;

If you make _setBuddyColor return the color, you can just write:
  color = buddy.color || this._setBuddyColor(buddy, name);


>+     <!-- Set buddy color attributes -->
>+     <method name="_setBuddyColor">
>+       <parameter name="aBuddy"/>
>+       <parameter name="aBuddyName"/>
>+       <body>
>+       <![CDATA[
>+         let color = this._computeColor(aBuddyName);

Isn't there a trivial way to get the name from aBuddy? If so, you can get rid of the aBuddyName parameter.


>      <!-- Add a buddy in the visible list of participants -->
>      <method name="addBuddy">
>        <parameter name="aBuddy"/>
>+       <parameter name="aShouldAppendDirectly"/>

This needs a comment to explain what the aShouldAppendDirectly parameter is.
It should say that it's an optional boolean for a performance optimization and that when it's true the addBuddy method will assume it's called with nicks in the correct order and should skip the binary search for the insert position.


>          // Insert item at the right position

This comment looks like it should stay above the addNick call.

>-         this.addNick(item);
>+         if (aShouldAppendDirectly) {
>+           this.nicklistElt.appendChild(item);
>+           this.nicks.push(name.toLowerCase());
>+           return;
>+         }
>+         this.addNick(item, name);
>        ]]>


>+     <!-- Array of lowercase nicks kept in the order of the nicklist -->
>+     <field name="nicks">[]</field>

I wondered for a while if it's the right decision to lowercase nicks before storing them in the nicks array. I think it's OK, but maybe that comment should mention that the nicks array is here only for a performance optimization, and that the list of nicks can be obtained from the keys of this.buddies?

>      <method name="addNick">

>+           if (nick < this.nicks[middle])

Shouldn't we use localeCompare here?


>            // Populate the nicklist
>            this.buddies = {};
>-           var nicks = getIter(this.conv.getParticipants());
>-           for (let n in nicks)
>-             this.addBuddy(n);
>+           this.nicks = [];
>+           this.nicklistElt = this.getElt("nicklist");
>+           let enumerator = this.conv.getParticipants();
>+           let chatbuddies = [];
>+           while (enumerator.hasMoreElements()) {
>+             let buddy = enumerator.getNext();
>+             chatbuddies.push([buddy.name.toLowerCase(), buddy]);

I'm not sure how expensive it is to create an array for each buddy, but I think we can find a faster solution.

>+           }
>+           chatbuddies.sort(function(a,b) a[0] < b[0] ? -1 : 1);
>+           chatbuddies.forEach(function(a) this.addBuddy(a[1], true), this);

You don't need to repeat "chatbuddies" on that second line, just remove the ; on the line above :).

How do you like this suggestion?

let buddies = {};
while (enumerator.hasMoreElements()) {
  let buddy = enumerator.getNext();
  buddies[buddy.name.toLowerCase()] = buddy;
}

Object.keys(buddies)
      .sort(localeCompare)
      .forEach(function(name) {
        this.addBuddy(buddies[name], true);
      }, this);

(You may not need that many line breaks, but I see you have a large indent level, so I played it safe :))
Attachment #8353646 - Flags: review?(florian) → review-
Attached patch Patch (obsolete) — — Splinter Review 
*** Original post on bio 1686 as attmnt 1942 at 2012-10-10 20:50:00 UTC ***

>Shouldn't we use localeCompare here?
Using localeCompare is slower by a factor of 250 per if clause. Since the number of comparisons will be O(n log(n)) for n participants, this adds up. The matching toLocaleLowerCase is also O(100) times slower than toLowerCase. Since this does not affect IRC at all and we have had no bug reports of incorrect sorting from XMPP MUC users so far, I think we should leave this for now and handle it in a separate bug if it turns out to be an issue, as many more users would be affected by the slowdown.

>I'm not sure how expensive it is to create an array for each buddy, but I think
>we can find a faster solution.
Creating an array actually turns out to be quite cheap (x.push([a,b]) does not even take three times as long as x.push(a)). I tried to use your proposed solution, but couldn't find a good way to work around the problem that there might be multiple participants whose lowercase nicks match (this can't happen for IRC, but unlike the sorting issue would cause real problems if it did happen for some other MUC protocol).

The colour handling could be nicer, but that's for a separate bug (and probably requires looking imThemes and the %codes% too)
Attachment #8353698 - Flags: review?
Comment on attachment 8353646 [details] [diff] [review]
Patch v4

*** Original change on bio 1686 attmnt 1888 at 2012-10-10 20:50:44 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353646 - Attachment is obsolete: true
Comment on attachment 8353698 [details] [diff] [review]
Patch

*** Original change on bio 1686 attmnt 1942 at 2012-10-10 20:51:41 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353698 - Flags: review? → review?(florian)
*** Original post on bio 1686 at 2012-10-10 23:29:31 UTC ***

(In reply to comment #6)

> I tried to use your proposed
> solution, but couldn't find a good way to work around the problem that there
> might be multiple participants whose lowercase nicks match (this can't happen
> for IRC, but unlike the sorting issue would cause real problems if it did
> happen for some other MUC protocol).

Don't you just need to remove the .toLowerCase() call in the code I suggested?
*** Original post on bio 1686 at 2012-10-11 15:25:09 UTC ***

(In reply to comment #7)
> Don't you just need to remove the .toLowerCase() call in the code I suggested?
You'd have to move it to the comparison function in sort, which would then more than make up for the time savings due to not creating the arrays.
Comment on attachment 8353698 [details] [diff] [review]
Patch

*** Original change on bio 1686 attmnt 1942 at 2012-11-02 01:20:24 UTC ***

>diff --git a/chrome/instantbird/content/instantbird/conversation.xml b/chrome/instantbird/content/instantbird/conversation.xml

>         if (!aMsg.system && conv.isChat) {
>           let name = aMsg.who;
>-          let color;
>           if (this._hasBuddy(name)) {
>             let buddy = this.buddies[name];
>-            color = buddy.color;
>             this._activateBuddy(buddy, aMsg.time);
>+            aMsg.color = buddy.colorStyle;
>           }
>           else {
>             // Buddy no longer in the room
>-            color = this._computeColor(name);
>+            aMsg.color = this._getColorStyle(this._computeColor(name));
>           }
>-          aMsg.color = "color: hsl(" + color + ", 100%, 40%);";

Can we make this._activateBuddy return the colorStyle? If so, we can simplify to:

aMsg.color =
  this._hasBuddy(name) ? this._activateBuddy(this.buddies[name], aMsg.time)
                       : this._getColorStyle(this._computeColor(name));

(and if that's impossible to indent correctly, I think I would still prefer keeping the color variable, and doing "aMsg.color =" only once. That's not really important though...)

>@@ -1110,84 +1131,99 @@
>            if (aBuddy.activeTimer)
>              clearTimeout(aBuddy.activeTimer);
>            aBuddy.activeTimer = setTimeout(function() {
>              delete aBuddy.activeTimer;
>              aBuddy.setAttribute("inactive", "true");
>            }, waitBeforeInactive);
>          }
>          aBuddy.lastMsgTime = aLastMsgTime;
>+         if (!aBuddy.color)
>+           this._setBuddyColor(aBuddy);

This test doesn't do what you want if the color value is 0.
Possible fixes:
- test colorStyle instead of color
- test ("color" in aBuddy)
- test color !== undefined



>+     <!-- Array of lowercase nicks kept in the order of the nicklist.

"the order of the nicklist" confused Mook over IRC. I think you want to specify the order in which the nicklist is displayed (otherwise it could be understood as the order the nicklist is received).

>+          This array is for performance optimization only. To obtain the
>+          capitalized list of nicks use Object.keys(this.buddies) -->
>+     <field name="_nicks">[]</field>



>+           while (enumerator.hasMoreElements()) {
>+             let buddy = enumerator.getNext();
>+             chatBuddies.push([buddy.name.toLowerCase(), buddy]);

Would it be much slower to do:
 chatBuddyes.push({name: buddy.name.toLowerCase(), buddy: buddy});
?
(the following code would be much more readable)

>+           }
>+           chatBuddies.sort(function(a,b) a[0] < b[0] ? -1 : 1)

Nit: space after the comma in "function(a, b)"

Seems ready otherwise :-).
Attachment #8353698 - Flags: review?(florian) → review-
Attached patch Patch — — Splinter Review 
*** Original post on bio 1686 as attmnt 2033 at 2012-11-02 20:13:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353793 - Flags: review?(florian)
Comment on attachment 8353698 [details] [diff] [review]
Patch

*** Original change on bio 1686 attmnt 1942 at 2012-11-02 20:13:02 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353698 - Attachment is obsolete: true
Comment on attachment 8353793 [details] [diff] [review]
Patch

*** Original change on bio 1686 attmnt 2033 at 2012-11-02 22:09:14 UTC ***

Thanks!
Attachment #8353793 - Flags: review?(florian) → review+
*** Original post on bio 1686 at 2012-11-03 04:22:35 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/f56c2f9f57a5
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: