1060891 - Stats service uses too much memory

Status: RESOLVED FIXED

(Instantbird Graveyard :: Other, defect)

Comment 1

[aleth [:aleth]]

Attachment: nobind.diff

This doesn't actually save any memory, especially as all these functions are gc'd, but since I have the patch we may as well use it.

Comment 2

[aleth [:aleth]]

Attachment: statsmemory.diff

The big win here is avoiding retaining references to the XPConnect wrapped prplIRoomInfo objects from the stats service. Further gains from slimming down the roomInfo objects (we may wish to store prplIChatRoomFields there at some point in the future, if we need to store passwords for some reason, but we certainly don't need it now) and dropping unnecessary references to the prplAccount. (Replacing those references with account.id leads to thousands of copies of "account3"-like id strings.)

This suggests the XPC_WN_NoMods_NoCall_Proto_JSClass entries in about:memory are indeed cross-compartment wrappers rather than the "reflectors" internal to a compartment ms2ger suggested.

The Function class references must be to the closure in the ircRoomInfo chatRoomFields getter (the only function removed here). It's amazing how huge the memory cost of this appears to be, and that the memory usage showed up in the stats service despite the LIST results being stored as roomInfos in the prpl.

It's interesting to note these changes leave the memory used by irc.js and ircBase.jsm effectively unchanged, despite the ircRoomInfo changes.

I would like to understand better exactly what the Function/XPC_WN_NoMods_NoCall_Proto_JSClass/XPCWrappedNative_NoHelper corresponded to and why the memory cost was so large (are effectively copies made in wrapping?). String memory usage goes up a bit with this patch, suggesting that some of this must have been hidden in the roomInfos previously somehow.


Measurements for Freenode + Mozilla connected, awesometab opened (triggering LIST) and closed after completion, memory usage minimized.

Before patch:

140.26 MB (100.0%) -- explicit
├───67.34 MB (48.01%) -- js-non-window
│   ├──61.49 MB (43.84%) -- zones
│   │  ├──59.60 MB (42.49%) -- zone(0x10935e000)
│   │  │  ├──22.82 MB (16.27%) ── unused-gc-things
│   │  │  ├──15.28 MB (10.90%) -- compartment([System Principal], file:///Users/helvellyn/buildhg/cc/obj-im/dist/Instantbird.app/Contents/MacOS/./components/ibConvStatsService.js)
│   │  │  │  ├──11.99 MB (08.54%) -- classes
│   │  │  │  │  ├───4.69 MB (03.34%) ++ class(Function)
│   │  │  │  │  ├───4.65 MB (03.32%) ++ class(XPC_WN_NoMods_NoCall_Proto_JSClass)
│   │  │  │  │  └───2.64 MB (01.88%) -- (4 tiny)
│   │  │  │  │      ├──1.34 MB (00.95%) ++ class(XPCWrappedNative_NoHelper)
│   │  │  │  │      ├──1.14 MB (00.81%) ++ class(Object)
│   │  │  │  │      ├──0.13 MB (00.09%) ++ class(Array)
│   │  │  │  │      └──0.03 MB (00.02%) ++ class(<non-notable classes>)
│   │  │  │  ├───3.25 MB (02.32%) ── compartment-tables
│   │  │  │  └───0.05 MB (00.03%) ++ (3 tiny)
│   │  │  ├──12.04 MB (08.59%) -- (121 tiny)
│   │  │  │  ├───1.12 MB (00.80%) ++ compartment([System Principal], resource:///modules/ircBase.jsm)
│   │  │  │  │   ├──1.06 MB (00.79%) -- classes
│   │  │  │  │   │  ├──0.98 MB (00.73%) ++ class(Object)
│   │  │  │  │   │  ├──0.05 MB (00.04%) ++ class(Function)
│   │  │  │  │   │  └──0.02 MB (00.02%) ++ class(<non-notable classes>)
│   │  │  │  │   ├──0.04 MB (00.03%) ── scripts/gc-heap
│   │  │  │  │   ├──0.01 MB (00.01%) ── sundries/malloc-heap
│   │  │  │  │   └──0.01 MB (00.01%) ── compartment-tables
│   │  │  ├───4.67 MB (03.33%) -- strings/string(<non-notable strings>)
│   │  │  │   ├──3.05 MB (02.17%) -- malloc-heap
│   │  │  │   │  ├──2.59 MB (01.84%) ── latin1
│   │  │  │   │  └──0.46 MB (00.33%) ── two-byte
│   │  │  │   └──1.62 MB (01.16%) ++ gc-heap
│   │  │  ├───2.59 MB (01.85%) -- compartment([System Principal], file:///Users/helvellyn/buildhg/cc/obj-im/dist/Instantbird.app/Contents/MacOS/./components/irc.js)
│   │  │  │   ├──1.27 MB (00.90%) ++ classes
│   │  │  │   ├──1.25 MB (00.89%) ── cross-compartment-wrapper-table
│   │  │  │   ├──0.03 MB (00.02%) ── scripts/gc-heap
│   │  │  │   ├──0.02 MB (00.01%) ── compartment-tables
│   │  │  │   ├──0.01 MB (00.01%) ── sundries/malloc-heap
│   │  │  │   └──0.01 MB (00.01%) ── type-inference/type-scripts

After patch:

101.54 MB (100.0%) -- explicit
├───43.28 MB (42.62%) -- js-non-window
│   ├──37.47 MB (36.91%) -- zones
│   │  ├──35.56 MB (35.02%) -- zone(0x10935e000)
│   │  │  ├──11.44 MB (11.26%) ── unused-gc-things
│   │  │  ├──11.21 MB (11.04%) ++ (122 tiny)
│   │  │  ├───6.82 MB (06.72%) -- strings/string(<non-notable strings>)
│   │  │  │   ├──4.89 MB (04.82%) -- malloc-heap
│   │  │  │   │  ├──4.16 MB (04.10%) ── latin1
│   │  │  │   │  └──0.73 MB (00.72%) ── two-byte
│   │  │  │   └──1.93 MB (01.90%) -- gc-heap
│   │  │  ├───2.58 MB (02.54%) -- compartment([System Principal], file:///Users/helvellyn/buildhg/cc/obj-im/dist/Instantbird.app/Contents/MacOS/./components/irc.js)
│   │  │  │   ├──1.26 MB (01.24%) ++ classes
│   │  │  │   ├──1.25 MB (01.23%) ── cross-compartment-wrapper-table
│   │  │  │   └──0.07 MB (00.07%) ++ (4 tiny)
│   │  │  ├───2.39 MB (02.36%) -- compartment([System Principal], file:///Users/helvellyn/buildhg/cc/obj-im/dist/Instantbird.app/Contents/MacOS/./components/ibConvStatsService.js)
│   │  │  │   ├──2.33 MB (02.29%) -- classes
│   │  │  │   │  ├──2.11 MB (02.08%) -- class(Object)
│   │  │  │   │  │  ├──1.99 MB (01.96%) ++ objects
│   │  │  │   │  │  └──0.12 MB (00.12%) ++ shapes
│   │  │  │   │  └──0.22 MB (00.21%) -- (3 tiny)
│   │  │  │   │     ├──0.13 MB (00.13%) ++ class(Array)
│   │  │  │   │     ├──0.06 MB (00.05%) ++ class(Function)
│   │  │  │   │     └──0.03 MB (00.03%) ++ class(<non-notable classes>)
│   │  │  │   └──0.06 MB (00.06%) ++ (4 tiny)
│   │  │  └───1.12 MB (01.10%) -- compartment([System Principal], resource:///modules/ircBase.jsm)
│   │  │      ├──1.06 MB (01.04%) -- classes
│   │  │      │  ├──0.98 MB (00.97%) ++ class(Object)
│   │  │      │  ├──0.05 MB (00.05%) ++ class(Function)
│   │  │      │  └──0.02 MB (00.02%) ++ class(<non-notable classes>)
│   │  │      └──0.06 MB (00.06%) -- (3 tiny)
│   │  │         ├──0.03 MB (00.03%) ── scripts/gc-heap
│   │  │         ├──0.01 MB (00.01%) ── sundries/malloc-heap
│   │  │         └──0.01 MB (00.01%) ── compartment-tables

Comment 3

[aleth [:aleth]]

Forgot to note that the large drop in unused-gc-things with this patch also seems to be reproducible. Another thing to understand better...

Comment 4

[Florian Quèze [:florian]]

(In reply to aleth [:aleth] from comment #2)

Thanks for experimenting with this! :-)

> The big win here is avoiding retaining references to the XPConnect wrapped
> prplIRoomInfo objects from the stats service.

Ok.

> Further gains from slimming
> down the roomInfo objects (we may wish to store prplIChatRoomFields there at
> some point in the future, if we need to store passwords for some reason, but
> we certainly don't need it now) and dropping unnecessary references to the
> prplAccount.

I don't really feel comfortable dropping access to the ChatRoomFields information. On some prpls having access to them may be required to join a room.

Are we expecting the account reference to be expensive? These shouldn't involve any wrapper if we are storing the prplAccount instance from within the prpl, right?

> (Replacing those references with account.id leads to thousands
> of copies of "account3"-like id strings.)

Would using numericId instead help?

> The Function class references must be to the closure in the ircRoomInfo
> chatRoomFields getter (the only function removed here).

I don't see why the chatRoomFieldValues getter would use any memory, it's on the prototype and doesn't seem to be a closure.

> It's amazing how
> huge the memory cost of this appears to be, and that the memory usage showed
> up in the stats service despite the LIST results being stored as roomInfos
> in the prpl.

Maybe there's somehow a side-effect within the stats service?

> String memory usage goes up a bit with this patch,
> suggesting that some of this must have been hidden in the roomInfos
> previously somehow.

Isn't this the account ids you are now storing instead of account references?

Comment 5

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)

I've done some more investigating to understand things better, making changes in steps. It turns out the Function/XPC_WN_NoMods_NoCall_Proto_JSClass/XPCWrappedNative_NoHelper classes overhead is entirely due to keeping a reference to the roomInfo objects from the stats service. Whether those objects contain any getters or any references to the prplIAccount makes a neglibile difference (not that surprising since these are references from within the prpl). What is really surprising to me is that a simple this.roomInfo = aRoomInfo seems to cause all three classes to appear irrespective of the roomInfo contents.

> I don't really feel comfortable dropping access to the ChatRoomFields
> information. On some prpls having access to them may be required to join a
> room.

This is true. It's a hypothetical though, which can be revisited when such a prpl implements requestRoomInfo.

The "problem" with keeping chatRoomFields info is that I don't know how to unwrap the getter without calling it (by 'unwrap' I mean keeping a reference to the getter from PossibleChat without keeping a reference to the roomInfo).

If instead of the getter we store the resulting info, it can be unwrapped, but this adds back 3.5M of memory use to store this precalculated data.

Puzzle: why does keeping references (from the stats service) to prplIChatRoomFieldValues (stored in the prpl, inside prplIRoomInfos) cost 3.5M which appear under ircBase in about:memory while keeping references to the whole prplIRoomInfo (also stored in the prpl) cost >15M which appear under ibConvStatsService in about:memory?

> > (Replacing those references with account.id leads to thousands
> > of copies of "account3"-like id strings.)

I think you misunderstood: the patch doesn't retain any references to the prplAccount or to the account.id in prplIRoomInfo. It's not needed, as each account gets its own callback from the stats service during LIST.
 
> > String memory usage goes up a bit with this patch,
> > suggesting that some of this must have been hidden in the roomInfos
> > previously somehow.
> 
> Isn't this the account ids you are now storing instead of account references?

No.

Comment 6

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #5)
> It turns out the
> Function/XPC_WN_NoMods_NoCall_Proto_JSClass/XPCWrappedNative_NoHelper
> classes overhead is entirely due to keeping a reference to the roomInfo
> objects from the stats service.

I wonder if this is somehow related to the convoluted way XPCOM does callbacks.

Comment 7

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #6)
> > It turns out the
> > Function/XPC_WN_NoMods_NoCall_Proto_JSClass/XPCWrappedNative_NoHelper
> > classes overhead is entirely due to keeping a reference to the roomInfo
> > objects from the stats service.
> 
> I wonder if this is somehow related to the convoluted way XPCOM does
> callbacks.

Doesn't seem to be the case. Not related to passing arrays of prplIRoomInfo either.

Comment 8

[Patrick Cloke [:clokep]]

(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> > Further gains from slimming
> > down the roomInfo objects (we may wish to store prplIChatRoomFields there at
> > some point in the future, if we need to store passwords for some reason, but
> > we certainly don't need it now) and dropping unnecessary references to the
> > prplAccount.
> 
> I don't really feel comfortable dropping access to the ChatRoomFields
> information. On some prpls having access to them may be required to join a
> room.

I agree, without this...I think you're making too many assumptions.

Thanks for looking at this though! It's been enlightening to explore these memory issues. Hopefully we can find what's really taking up that memory soon.

Comment 9

[aleth [:aleth]]

Attachment: statsmemory.diff v2

This gets rid of prplIRoomInfo objects altogether in favour of providing an API which can retrieve the various info elements from the prpl when needed. Instead of passing prplIRoomInfos to the stats service, we pass the room names. Since we were already storing the roominfo data in the prpl so as not to have to request it from the server too often, this makes some sense. Compared to "unwrapping" roomInfo objects as in the previous patch, it also reduces the data stored in the stats service (e.g. references to topics are only added while we actually display a PossibleChat, which never happens to most of them).

The big memory saving is due to avoiding the XPConnect problems/overhead associated with storing thousands of references to wrapped objects (at the cost of a slightly messier API, but I think it's bearable given the benefits). The obvious question is whether we have a similar overhead somewhere for prplIMessages, but it's not clear to me why XPConnect is doing what it is doing here.

String memory usage has also gone down, probably because there is less accidental flattening and making of copies due to wrapping/unwrapping.

While updating the /list command to reflect the changes, I made it async, which gets rid of the UI freeze/jank that has been reported. I used a Task just to see what it would look like (and similarly in the stats service). It's possible something similar could/should be abstracted into a module later.

Memory usage (same STR as above) is now

100.38 MB (100.0%) -- explicit
├───42.00 MB (41.84%) -- js-non-window
│   ├──37.24 MB (37.10%) -- zones
│   │  ├──35.42 MB (35.28%) -- zone(0x109361000)
│   │  │  ├──14.45 MB (14.39%) ── unused-gc-things
│   │  │  ├──11.10 MB (11.06%) ++ (121 tiny)
│   │  │  ├───4.10 MB (04.08%) -- strings/string(<non-notable strings>)
│   │  │  │   ├──2.94 MB (02.93%) -- malloc-heap
│   │  │  │   │  ├──2.65 MB (02.64%) ── latin1
│   │  │  │   │  └──0.29 MB (00.29%) ── two-byte
│   │  │  │   └──1.16 MB (01.16%) -- gc-heap
│   │  │  │      ├──1.13 MB (01.13%) ── latin1
│   │  │  │      └──0.03 MB (00.03%) ── two-byte
│   │  │  ├───2.51 MB (02.50%) -- compartment([System Principal], file:///Users/helvellyn/buildhg/cc/obj-im/dist/Instantbird.app/Contents/MacOS/components/irc.js)
│   │  │  │   ├──1.25 MB (01.25%) ── cross-compartment-wrapper-table
│   │  │  │   ├──1.18 MB (01.18%) -- classes
│   │  │  │   │  ├──1.02 MB (01.02%) -- class(Proxy)
│   │  │  │   │  │  ├──1.02 MB (01.02%) ── objects/gc-heap
│   │  │  │   │  │  └──0.00 MB (00.00%) ++ shapes/gc-heap
│   │  │  │   │  └──0.16 MB (00.16%) ++ (3 tiny)
│   │  │  │   └──0.07 MB (00.07%) ++ (4 tiny)
│   │  │  ├───1.67 MB (01.66%) -- compartment([System Principal], resource:///modules/ircBase.jsm)
│   │  │  │   ├──1.61 MB (01.61%) -- classes
│   │  │  │   │  ├──1.53 MB (01.53%) -- class(Object)
│   │  │  │   │  │  ├──1.53 MB (01.52%) -- objects
│   │  │  │   │  │  └──0.01 MB (00.01%) ++ shapes
│   │  │  │   │  └──0.08 MB (00.08%) ++ (2 tiny)
│   │  │  │   └──0.06 MB (00.06%) ++ (3 tiny)
│   │  │  └───1.59 MB (01.59%) -- compartment([System Principal], file:///Users/helvellyn/buildhg/cc/obj-im/dist/Instantbird.app/Contents/MacOS/components/ibConvStatsService.js)
│   │  │      ├──1.53 MB (01.52%) -- classes
│   │  │      │  ├──1.18 MB (01.18%) -- class(Object)
│   │  │      │  │  ├──1.06 MB (01.06%) -- objects
│   │  │      │  │  └──0.12 MB (00.12%) ++ shapes
│   │  │      │  └──0.34 MB (00.34%) ++ (3 tiny)
│   │  │      └──0.07 MB (00.07%) ++ (4 tiny)

Comment 10

[aleth [:aleth]]

One could probably compromise on the API by keeping the existing prplRoomInfo object interface and using something like getRoomInfo(aName) to retrieve it when needed, rather than separate functions for each property. Of course that would add back some overhead.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8485410 [details] [diff] [review]
statsmemory.diff v2

Review of attachment 8485410 [details] [diff] [review]:
-----------------------------------------------------------------

Lots of good changes here! I didn't do a full review because if you agree with some of the API changes I suggested, I'll have to re-review the whole patch anyway, but I looked at most (about 80-90%) of the lines changed.

::: chat/components/public/imIAccount.idl
@@ +44,5 @@
>  
>  /*
>   * Callback passed to an account's requestRoomInfo function.
>   */
> +[scriptable, function, uuid(43102a36-883a-421d-a6ac-126aafee5a28)]

Random comment not directly related to the current patch:
We are seeing high Function and XPC_WN_NoMods_NoCall_Proto_JSClass memory usage. Have we tried removing the "function" keyword from this interface definition, and implementing a real object (including QueryInterface) instead?
I'm wondering if this high unexplained memory usage could be due to xpconnect never free'ing the generated wrappers around these functions-given-as-XPCOM-object.

@@ +109,2 @@
>     */
>    void requestRoomInfo(in prplIRoomInfoCallback aCallback);

Shound't this become requestRoomList if we are only getting a list of names?

@@ +109,5 @@
>     */
>    void requestRoomInfo(in prplIRoomInfoCallback aCallback);
> +  AUTF8String roomInfoTopic(in AUTF8String aRoomName);
> +  unsigned long roomInfoParticipantCount(in AUTF8String aRoomName);
> +  prplIChatRoomFieldValues roomInfoChatRoomFieldValues(in AUTF8String aRoomName);

How would you feel about having a getRoomInfo(in AUTF8String aRoomName) method that would return a prplIRoomInfo instead of these 3 methods?

Then the stats service would only store the list of room names, and would request the actual prplIRoomInfo object only when it's about to create an XBL binding displaying that room; which happens only for a small subset of the room list for large IRC networks.

::: chat/protocols/irc/irc.js
@@ +955,5 @@
>      return true;
>    },
>  
> +  // Room info: maps channel names to {topic, participantCount}.
> +  _channelList: new Map(),

Should this name be changed to _channelMap?

::: chat/protocols/irc/ircCommands.jsm
@@ +7,5 @@
>  const EXPORTED_SYMBOLS = ["commands"];
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
> +Cu.import("resource:///modules/imXPCOMUtils.jsm");

I don't see this used anywhere. Did you add it to use executeSoon before converting to using |yield Promise.resolve()| in a Task?

@@ +254,5 @@
> +                account.roomInfoParticipantCount(name);
> +              serverConv.writeMessage(serverName,
> +                name + " (" + participantCount + ") " + topic,
> +                {incoming: true, noLog: true});
> +              // Unblock every 10ms.

nit: add an empty line before this comment for better readability of the block.

::: im/components/ibConvStatsService.js
@@ +201,5 @@
>    // Account ids from which chat room info has been requested.
>    // We send an update notification if this is empty after adding chat rooms.
>    _accountsRequestingRoomInfo: new Set(),
> +  _addPendingChats: function(aAccountId, aRoomInfo) {
> +    if (!this._pendingChats) {

Could you reverse this test to reduce the indent level of the whole method?

if (this._pendingChats) (
  this._pendingChats.push(...
  return;
}

@@ +202,5 @@
>    // We send an update notification if this is empty after adding chat rooms.
>    _accountsRequestingRoomInfo: new Set(),
> +  _addPendingChats: function(aAccountId, aRoomInfo) {
> +    if (!this._pendingChats) {
> +      this._pendingChats = [];

Can we initialize this directly with aAccountId and aRoomInfo?

@@ +208,5 @@
> +        let lastUpdateNotification = 0;
> +        let sendUpdateNotification = () => {
> +          if ((!this._accountsRequestingRoomInfo.size &&
> +               !this._pendingChats.length) ||
> +              t - lastUpdateNotification > 500) {

The variable 't' is used before it's declared. 'let' has become stricter recently, so I think this won't work anymore. It's not very readable anyway.

@@ +214,5 @@
> +            lastUpdateNotification = t;
> +          }
> +        };
> +
> +        yield Promise.resolve(); // Wait until _pendingChat is non-empty.

Is this really needed (after my suggestion above)?

@@ +218,5 @@
> +        yield Promise.resolve(); // Wait until _pendingChat is non-empty.
> +        let t = Date.now();
> +        while (this._pendingChats.length) {
> +          let [accountId, rooms] = this._pendingChats.pop();
> +          let chatList = this._chatsByAccountIdAndName.get(accountId);

nit: could do with an empty line before this and after 'continue;'.

@@ +220,5 @@
> +        while (this._pendingChats.length) {
> +          let [accountId, rooms] = this._pendingChats.pop();
> +          let chatList = this._chatsByAccountIdAndName.get(accountId);
> +          if (!chatList) // Account no longer connected.
> +            continue;

Is this how we abort the task if the chat code is uninitialized before we are done?

@@ +687,5 @@
>      this._lowerCaseName || (this._lowerCaseName = this.displayName.toLowerCase()),
> +  get statusText() {
> +    let account = this.account.prplAccount;
> +    let topic = account.roomInfoTopic(this.displayName);
> +    let participantCount = account.roomInfoParticipantCount(this.displayName);

nit: would be a bit more logical to set these 2 variables in the order in which they are used.

Comment 12

[aleth [:aleth]]

Attachment: statsmemory.diff v3

(In reply to Florian Quèze [:florian] [:flo] from comment #11)
> ::: chat/components/public/imIAccount.idl
> @@ +44,5 @@
> >  
> >  /*
> >   * Callback passed to an account's requestRoomInfo function.
> >   */
> > +[scriptable, function, uuid(43102a36-883a-421d-a6ac-126aafee5a28)]
> 
> Random comment not directly related to the current patch:
> We are seeing high Function and XPC_WN_NoMods_NoCall_Proto_JSClass memory
> usage. Have we tried removing the "function" keyword from this interface
> definition, and implementing a real object (including QueryInterface)
> instead?
> I'm wondering if this high unexplained memory usage could be due to
> xpconnect never free'ing the generated wrappers around these
> functions-given-as-XPCOM-object.

Yeah, but that's not what is causing it. I had a version doing without callbacks altogether (and even one using observer notifications), and as long as a reference to a roomInfo object is kept across compartment boundaries, the behaviour appears.

> @@ +109,2 @@
> >     */
> >    void requestRoomInfo(in prplIRoomInfoCallback aCallback);
> 
> Shound't this become requestRoomList if we are only getting a list of names?

No, because we are (potentially) causing the prpl to request all the room info from the server.

> >    void requestRoomInfo(in prplIRoomInfoCallback aCallback);
> > +  AUTF8String roomInfoTopic(in AUTF8String aRoomName);
> > +  unsigned long roomInfoParticipantCount(in AUTF8String aRoomName);
> > +  prplIChatRoomFieldValues roomInfoChatRoomFieldValues(in AUTF8String aRoomName);
> 
> How would you feel about having a getRoomInfo(in AUTF8String aRoomName)
> method that would return a prplIRoomInfo instead of these 3 methods?

I'm OK with that change (see comment #10).

> > +Cu.import("resource:///modules/imXPCOMUtils.jsm");
> 
> I don't see this used anywhere. Did you add it to use executeSoon before
> converting to using |yield Promise.resolve()| in a Task?

No, it defines the lazy getter used in the following line.

> @@ +220,5 @@
> > +        while (this._pendingChats.length) {
> > +          let [accountId, rooms] = this._pendingChats.pop();
> > +          let chatList = this._chatsByAccountIdAndName.get(accountId);
> > +          if (!chatList) // Account no longer connected.
> > +            continue;
> 
> Is this how we abort the task if the chat code is uninitialized before we
> are done?

Yes, that too. Though bug 955649 still needs fixing (I was kind of hoping nhnt11 would take a look at that)

Comment 13

[Florian Quèze [:florian]]

(In reply to aleth [:aleth] from comment #12)

> > @@ +109,2 @@
> > >     */
> > >    void requestRoomInfo(in prplIRoomInfoCallback aCallback);
> > 
> > Shound't this become requestRoomList if we are only getting a list of names?
> 
> No, because we are (potentially) causing the prpl to request all the room
> info from the server.

I don't understand this answer.

> > > +Cu.import("resource:///modules/imXPCOMUtils.jsm");
> > 
> > I don't see this used anywhere. Did you add it to use executeSoon before
> > converting to using |yield Promise.resolve()| in a Task?
> 
> No, it defines the lazy getter used in the following line.

Ah, you meant XPCOMUtils, not imXPCOMUtils, and fixed it in this new patch :).

Comment 14

[aleth [:aleth]]

(In reply to Florian Quèze [:florian] [:flo] from comment #13)
> (In reply to aleth [:aleth] from comment #12)
> 
> > > @@ +109,2 @@
> > > >     */
> > > >    void requestRoomInfo(in prplIRoomInfoCallback aCallback);
> > > 
> > > Shound't this become requestRoomList if we are only getting a list of names?
> > 
> > No, because we are (potentially) causing the prpl to request all the room
> > info from the server.
> 
> I don't understand this answer.

When someone calls requestRoomInfo, they are doing it because they want to be able to obtain info on all the rooms on the server, and that's what the prpl will request from the server if it's not already available in memory. There's no such thing as just requesting the room list. The fact that the callback now receives a list of names rather than the full room info seems to me to be an API detail. That might be different if for XMPP for example there was a difference between fetching room list and room info, so if you prefer I will make the change, but so far I dislike introducing the distinction.

Comment 15

[aleth [:aleth]]

Attachment: statsmemory.diff v3, unbitrotted

Comment 16

[aleth [:aleth]]

(Another) review ping, the API changes here should really land (or not) before abdelrhman adds XMPP roominfos.

Comment 17

[aleth [:aleth]]

pING

Comment 18

[aleth [:aleth]]

We're going to need this reviewed for gsoc in the next few weeks, see comment 16.

Comment 19

[Florian Quèze [:florian]]

Comment on attachment 8599524 [details] [diff] [review]
statsmemory.diff v3, unbitrotted

Review of attachment 8599524 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like:
- it's not crazy;
- it has already received a relatively thorough review in comment 11;
- it has been blocked for an ashamingly long time;
- I won't realistically find time in a decently near future for another really thorough review.

So... if you can confirm it's not bitrotted or (more likely) unbitrot it, I'll r+.

::: chat/protocols/irc/irc.js
@@ +988,5 @@
>        this.sendMessage("LIST");
>      }
>      // Otherwise, pass channels that have already been received to the callback.
>      else {
> +      let rooms = [...this._channelList.keys()];

I think this syntax has been deprecated/removed recently.

::: chat/protocols/irc/ircCommands.jsm
@@ +262,4 @@
>        account.requestRoomInfo({onRoomInfoAvailable: function(aRooms) {
> +        if (!pendingChats.length) {
> +          Task.spawn(function*() {
> +            yield Promise.resolve();

Does this need a comment to explain why it's needed?

@@ +271,5 @@
> +                name + " (" + roomInfo.participantCount + ") " + roomInfo.topic,
> +                {incoming: true, noLog: true});
> +
> +              // Unblock every 10ms.
> +              if (Date.now() > t + 10) {

I'm tempted to say the " + 10" should be in the |t = ...| lines to avoid doing that operation in each iteration of the loop, but realistically the performance impact is irrelevant here :) (feel free to ignore this comment)

Comment 20

[aleth [:aleth]]

Attachment: Reduce stats service memory usage and avoid jank on /list

Unbitrotted, comment added, t+10 restructured.

Comment 21

[aleth [:aleth]]

I did a quick check and this still saves around 30M for a freenode account.

Comment 22

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/4a53054a8bece7cdc2dd33a2bd13e10eba962535
Bug 1060891 - Get rid of .bind(this) in ibConvStatsService. r=florian

https://hg.mozilla.org/comm-central/rev/1f209168f4540a757ab33463be4d0858229d311a
Bug 1060891 - Reduce stats service memory usage and avoid jank on /list. r=florian

Comment 24

[aleth [:aleth]]

purple part: https://hg.mozilla.org/users/florian_queze.net/purple/rev/c13d58d643bf