Browser locks/freezes with many roster users

[ffPjrZUGXfcxuAj]

with 500 user in the roster converjs lock the browser

[jcbrand]

I've found the likely cause for this. A regression in RosterView which results in roster contacts being sorted too frequently (once for every contact instead of once for all of them).

I came across this while doing some refactoring for #83 and commit 04d2b3a contains a fix.

I stumbled across this while working on something else, so it wasn't feasible to create a separate patch/commit just for this issue.

[mckaygerhard]

i have similar problem, my server has 3000 users, and loading all online and offline lock browsers and freezes , in #205 i request a feature as possible solution, only loading online users!

[jcbrand]

@camaran and @mckaygerhard Can you check whether this is still a problem with the 0.8 release?

[mckaygerhard]

hi @jcbrand i can tested but in two weeks, due i'm very busy now.. also i must to adapt to the mail service

due my interes its the mail integration, i cannot said something now, alone, the converse.js works pretty good now. due i cannot reopens this issue, if still happen, i'll open new issue about it or notify to u to reopen this

[jcbrand]

thanks @mckaygerhard

[juanborda]

I can confirm this issue still happening for me.
Roster with ~153 (and more too) locks the browsers for about 2-3 seconds.

Im using v0.8.1, will try upgrading to 0.8.3 and see if it helps.

Thanks, awesome job :)

[juanborda]

Hi @jcbrand ,
Upgrading to v0.8.3 didnt' help.

i've tracked down the issue to the rosterHandler function.
Apparently it's being called once per roster item; and the function iterates through all the roster items every time.

Im not able to identify why though, still going through the code.
Any help is greatly appreaciated.

Thanks!
Regards.

[juanborda]

rosterHandler is called as a callback from Strophe.
Strophe sends 2 arguments to rosterHandler, however rosterHandler only parses/uses the first one. (items).
The 2nd argument i presume is the contact? that needs to be updated, however converse loops through every roster item again.

Right now i have make it check if the 2nd argument is undefined, if it is i just loop through all items, but if its not i only update that particular roster item.

rosterHandler: function (items, item) {
    converse.emit('roster', items);

    if (typeof item !== 'undefined') {
        items = [item];
    } else {
        this.clearCache(items);
    }

    // the rest of the function

Hopefully im understanding it correctly and this doesn't break anything else, lol.
Regards.

[jcbrand]

Excellent! Thanks for investigating this @juanpborda

I'm not sure whether there will be any side-effects, we'll have to check. Will you make a pull request with your changes?

In a similar vein, the clearCache method is also a potential bottleneck (all items are looped through there as well).
https://github.com/jcbrand/converse.js/blob/master/converse.js#L3483

I actually think that both strophe.muc and strophe.roster can be removed as dependencies and instead their functionality should be implemented inside converse.js itself. (Additionally, converse.js should be made more modular, but that's a different story).

The reason for this, is that both these libraries implement their own data structures for modeling and storing data (such as roster contacts), and then converse.js duplicates all that effort by again putting this data into Backbone.js models. Converse.js's models are the desired ones however, because they have automatic syncing with browser storage and automatic updating of DOM elements (via views).

So there is a duplication going on there, and this duplication is exactly the reason for the clearCache method. Without this duplication, that method would not be necessary and one would also have a better idea of exactly which items (e.g. roster contacts) have changed.

[mckaygerhard]

i then test again using lasted git from here and get many errors on js loading..
note that with oler verson works but with newers not

on firefox i got:

Error: extStatus is not a function
Source File: chrome://priv/content/priv.js
Line: 292

on chrome/chromium i got a jquery.timepicker error

the log bosh are:

Request URL:http://domain.net.ve/http-bind/
Request Method:POST
Status Code:200 OK
Request Headersview parsed
POST /http-bind/ HTTP/1.1
Host: domain.net.ve
Connection: keep-alive
Referer: http://domain.net.ve/roundcube/?_task=mail
Content-Length: 115
Origin: http://domain.net.ve
User-Agent: Mozilla/5.0 (X11; Linux i686) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/13.0.782.220 Safari/535.1
Content-Type: application/xml
Accept: /
Accept-Encoding: gzip,deflate,sdch
Accept-Language: es-419,es;q=0.8,en-US;q=0.6,en;q=0.4
Accept-Charset: ISO-8859-1,utf-8;q=0.7,*;q=0.3
Cookie: shellInABox=467306938:111011010; TRACKID=3befe1e8ba612f0c2c6b9154c03e3df9; prefsviewsplitter=195; roundcube_sessid=g55pa0tu98siitca97ond450p3; roundcube_sessauth=S6f1f9f6fd34c7d07959b5c28a44cb125ccd7684e
Request Payload

Response Headers HTTP/1.1 200 OK Access-Control-Allow-Origin: * Access-Control-Allow-Origin: * Access-Control-Allow-Headers: Origin, X-Requested-With Access-Control-Allow-Headers: Content-Type Content-Length: 51 Content-Type: text/xml; charset=utf-8 Connection: close Date: Wed, 01 Oct 2014 17:56:18 GMT Server: lighttpd/1.4.35

[juanborda]

@jcbrand Sorry for the delay, hard week..
I'll clone and make a pull request today; i was just downloading the .zip before 😊

Best regards.

[mckaygerhard]

hello @juanpborda i ask if u'r browser are lock only when loading ¡? my browser only lock when load the users, lock for about 20 seconds on a celeron older machine and for about 8 seconds on a dual core 2.2 GHz cpu machie with recents browsers both

for me, the lock only happened with loading users (on each refresh)

[jcbrand]

@mckaygerhard jquery.timepicker and priv.js are not used or required by converse.js. Looks like your problems lie elsewhere.

[mckaygerhard]

i removed the jquery and related jquery artifacts.. but error still persis, now i not get any error, event in the prebind debug info on chromium..

[jcbrand]

@juanpborda How's it going with that pull request?

[someone3210]

@juanpborda I tried your solution of modifying the rosterHandler but the result was that the roster contacts in each group were only sorted alphabetically; presence sorting did not occur. Have you experienced that as well? Any other solutions? Thanks!

[jcbrand]

This should now be fixed in the latest code in master.

[juanborda]

@jcbrand
Glad you were able to add it.
I havent got the time to properly fork and start committing yet.

Bad news is this still an issue with latest version.
Roster with 1200+ items (i know right..) will lock the browser.

Is there any undesired side effect if only calling clearCache if item is undefined?
Im trying to figure out if it wouldnt be better to just clearCache to an item instead of looping through them all.

@someone3210 I havent got the issue you mentioned, but i dont use groups so i really wouldn't know.

[juanborda]

Here's a Chrome CPU Profiling (we talked at Prosody's chat)
http://i.imgur.com/Mav0KYg.png

Im using Chrome 39 on Win7 x64. Havent tested another browser

[jcbrand]

@juanpborda As you can see, only 5% of the time is spent in clearCache.

However, your idea of only calling clearCache when the item parameter is undefined makes sense, it might work and speed things up a little bit.

Almost half of the time is spent in _.map._collect, which I think has to do with sorting the roster alphabetically. I'm not sure how that can be further improved.

What you can try right now, is to set hide_offline_users to true and see if that improves things. A lot of the time is spent on rendering and sorting the roster users. If some users are not shown (because they're offline), then the roster should render more quickly.

[juanborda]

Is there any way to disable sorting?
Or at least only sorting by status (online/offline)?

ps: This is the _.map._collect entry expanded: http://i.imgur.com/KII62Rq.png

[jcbrand]

Is there any way to disable sorting? Or at least only sorting by status (online/offline)?

No.

ps: This is the _.map._collect entry expanded: http://i.imgur.com/KII62Rq.png

Looks like clearCache is indeed responsible here. I'm now not sure why clearCache only showed 5% on the first Profiling chart, but I think the reason is that your view (Bottom Up) only shows the time directly spent inside a specific method and doesn't include time spent in methods called from that method.

So, 5% of total time was spent in clearCache directly, but clearCache calls _.pluck which calls _.map (and the time spent in those methods is not included in the profiling data for clearCache).

One improvement that can be made here, is to call _.pluck only once at the top of the method, instead of every time in the for loop.

[juanborda]

Does items change from callback to callback?
Because it seems to me that it's always iterating over the same array.

That's why for me it makes sense to only iterate once.

rosterHandler seems to be called once with items and then once per item with items and item.

Then its calling clearCache once per item and iterating over items every time per item.
What i do not know if that's needed since i'd assume that items is always the same (per what i was able to see it was) and im assuming that's why it also receives item.

(Sorry for these reports, im aware they are barely readable... This is really difficult to explain).

Adding this improved the performance:

if (!item) this.clearCache(items);

Still looking on how to further optimize.

Do you think there would be any benefit or undesired side effect if instead of clearing the cache, you just replace it with whatever items has?

Thanks a lot for you help!

[jcbrand]

Does items change from callback to callback?

Yes. The items change when a roster contact removes you from their roster.

I've made a commit to at least stop calling _.pluck for every iteration of the for loop.

To really fix this, strophe.muc needs to be removed and instead the roster code needs to be rewritten inside converse.js using the same Backbone models.

cc @juanpborda