Closed Bug 1058584 Opened 5 years ago Closed 5 years ago

LIST keeps too many long strings in memory

Categories

(Chat Core :: IRC, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch listleak.diff (obsolete) — Splinter Review
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.
Attachment #8479049 - Flags: feedback?(florian)
Attachment #8479049 - Flags: feedback?(clokep)
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
Attached patch listleak.diff v3Splinter Review
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
Attachment #8479049 - Attachment is obsolete: true
Attachment #8479077 - Attachment is obsolete: true
Attachment #8479049 - Flags: feedback?(florian)
Attachment #8479049 - Flags: feedback?(clokep)
Attachment #8479095 - Flags: review?(florian)
Depends on: 1058653
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?
Attachment #8479095 - Flags: review?(florian) → review+
I'd like this comment to be updated with Florian's suggestions! Thanks. :)
https://hg.mozilla.org/comm-central/rev/8bbfe92295cb

I updated the comment before checking in. Thanks!
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
Depends on: 727615
You need to log in before you can comment on or make changes to this bug.