1058584 - LIST keeps too many long strings in memory

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[aleth [:aleth]]

About:memory reports many long strings after LIST which stick around even after the awesometab has been closed and gc, indicating we are at the very least wasting memory here.

Comment 1

[aleth [:aleth]]

Attachment: listleak.diff

This is not strictly speaking a leak - it's just that we run into a case where the tradeoffs in the way JS internally handles strings don't act in our favour.

The long strings in memory are a symptom - they correspond to raw socket data, eg:
string(length=67888, copies=1, ":asimov.freenode.net 322 dobredansk2 #krazyk 7 :All things D3 Root || #OPMOSH || CM9/AOKP/etc: http://bit.ly/yKlExu || Exploit - http://bit.ly/qGTPri || Root for Windows/Linux&Mac - http://bit.ly/rdK4ll / http://bit.ly/mYNYn1 || Leaked-OTA - http://bit.ly/ra0TFf ||  Bootloop - http://is.gd/hSPoga || oye has code!/r/n:asimov.freenode.net 322 dobredansk2 #fedora-hu 10 :Magyar Fedora k/xC3/xB6z/xC3/xB6ss/xC3/xA9gi csatorna | Port/xC3/xA1l - http://fedora-hungary.org | K/xC3/xB6z/xC3/xB6ss/xC3/xA9gi h/xC3/xA1l/xC3/xB3: http://is.gd/G7xus8 http://is.gd/W4mF70 | Let/xC3/xB6lt/xC3/xA9sek - http://get.fedoraproject.org/r/n:asimov.freenode.net 322 dobredansk2 #AlchemyViewer 6 :Please stick around for a few hours to get any help or leave contact information to get back on you - we have better things to do than to stare into this window./r/n:asimov.freenode.net 322 dobredansk2 #satgnu 17 :SatGNU - The internet's worst! - http://techland.time.com/2013/02/13/13-years-later-system-shock-2-lives-again//r/n:asimov.freenode." (truncated))

Because we hold on to the topic substrings, which make up a significant fraction of the data, the JS engine decides to retain large chunks of socket data whole, rather than just keeping the parts we need. This adds up to a significant overhead:

Freenode + Mozilla account connected, after memory minimization:
54.15 MB (100.0%) -- explicit
│  │  │   ├──0.82 MB (01.51%) -- strings/string(<non-notable strings>)
│  │  │   │  ├──0.61 MB (01.13%) -- gc-heap
│  │  │   │  │  ├──0.61 MB (01.13%) ── latin1
│  │  │   │  │  └──0.00 MB (00.00%) ── two-byte
│  │  │   │  └──0.20 MB (00.37%) ── malloc-heap/latin1
│  │  │   └──0.03 MB (00.05%) ++ (4 tiny)

Status quo, after LIST, awesometab closed and memory minimized:
152.31 MB (100.0%) -- explicit
│   │  │  ├───8.53 MB (05.60%) -- strings
│   │  │  │   ├──7.43 MB (04.88%) -- string(<non-notable strings>)
│   │  │  │   │  ├──5.53 MB (03.63%) -- malloc-heap
│   │  │  │   │  │  ├──4.13 MB (02.71%) ── two-byte
│   │  │  │   │  │  └──1.40 MB (00.92%) ── latin1
│   │  │  │   │  └──1.91 MB (01.25%) ++ gc-heap
│   │  │  │   └──1.10 MB (00.72%) -- (37 tiny)
│   │  │  │      ├──0.13 MB (00.08%) ++ string(length=67888, copies=1, ...
[multiple long strings omitted]

Patch applied, after LIST, awesometab closed and memory minimized:
143.24 MB (100.0%) -- explicit
│   │  │  ├───4.89 MB (03.41%) -- strings/string(<non-notable strings>)
│   │  │  │   ├──3.18 MB (02.22%) -- malloc-heap
│   │  │  │   │  ├──2.71 MB (01.89%) ── latin1
│   │  │  │   │  └──0.48 MB (00.33%) ── two-byte
│   │  │  │   └──1.71 MB (01.19%) ++ gc-heap
[no long strings!]

Two points

* I wonder if there's a better way to force the JS engine to allocate new strings for the topics?

* Most of the memory cost of having opened and closed an awesometab isn't strings at all.

Comment 2

[aleth [:aleth]]

Attachment: listleak.diff with JSON.parse/stringify

For future reference, this also works and is a slight improvement. The non-empty check avoids 0.10 MB (00.07%) ── string(length=0, copies=4204, "")/gc-heap/latin1

Comment 3

[aleth [:aleth]]

Attachment: listleak.diff v3

str.normalize() seems like the best option for now.

For future reference:
17:26:53 - lth: aleth: normalize flattens the string (probably for ease of implementation)
17:27:02 - Ms2ger: normalize() seems like it'd be a QoI issue, yes
17:27:26 - lth: there are a few others that flatten too, but none seem reliably accessible from JS

Comment 4

[Florian Quèze [:florian]]

Comment on attachment 8479095 [details] [diff] [review]
listleak.diff v3

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

::: chat/protocols/irc/ircBase.jsm
@@ +762,5 @@
>        // Omit this.
>        topic = topic.replace(/^\[\+[a-zA-Z]*\] /, "");
> +      // Force the allocation of a new copy of the string so as to prevent
> +      // the JS engine from retaining the whole original socket string.
> +      topic = topic ? topic.normalize() : "";

Should the comment here include bug numbers for both the topic test, and the reason why we need this hack at all?

Comment 5

[Patrick Cloke [:clokep]]

I'd like this comment to be updated with Florian's suggestions! Thanks. :)

Comment 6

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/8bbfe92295cb

I updated the comment before checking in. Thanks!