Last Comment Bug 672439 - Avoid duplicate names for system compartment reporters
: Avoid duplicate names for system compartment reporters
Status: RESOLVED FIXED
[MemShrink:P1][inbound]
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla8
Assigned To: Nicholas Nethercote [:njn]
:
:
Mentors:
Depends on:
Blocks: MemShrinkTools
  Show dependency treegraph
 
Reported: 2011-07-18 21:42 PDT by Nicholas Nethercote [:njn]
Modified: 2012-05-06 19:44 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
sample output (52.86 KB, text/plain)
2011-07-18 23:03 PDT, Nicholas Nethercote [:njn]
no flags Details
patch (3.62 KB, patch)
2011-07-18 23:07 PDT, Nicholas Nethercote [:njn]
gal: review+
Details | Diff | Splinter Review

Description Nicholas Nethercote [:njn] 2011-07-18 21:42:05 PDT
It's possible to have many compartments with the same codebase, and these all get lumped together in about:memory.  This is particularly bad if you have a Jetpack-based add-on, because you can have dozens of compartments merged under the name "[System Principal]".  It's misleading to lump them all together, we should separate them.

The very simplest thing we could do is add the compartment's address as a suffix to its name.  That'll give the required separation without giving any indication of where each duplicate compartment came from.  Maybe we can do better, eg. indicate which system-principal compartments are sandboxes.
Comment 1 Nicholas Nethercote [:njn] 2011-07-18 23:03:18 PDT
Created attachment 546713 [details]
sample output

Here's the output of about:memory upon start-up with Bugzilla Tweaks installed, with the soon-to-be-attached patch applied.
Comment 2 Nicholas Nethercote [:njn] 2011-07-18 23:07:44 PDT
Created attachment 546715 [details] [diff] [review]
patch

This patch adds the address of a compartment, but only does it for system compartments, because (a) they're the only ones we're seeing lots of compartments with the same name, and (b) the addresses are a bit ugly, so we shouldn't show them where they're not useful.

An example:

│  ├──14.04 MB (20.68%) -- compartment([System Principal], 0x7ffb8c6a5000)
│  │  ├───9.20 MB (13.56%) -- gc-heap
│  │  │   ├──4.59 MB (06.76%) -- objects
│  │  │   ├──2.15 MB (03.17%) -- shapes
│  │  │   ├──1.70 MB (02.51%) -- strings
│  │  │   ├──0.56 MB (00.82%) -- arena-unused
│  │  │   └──0.20 MB (00.29%) -- (3 omitted)
│  │  ├───2.02 MB (02.97%) -- scripts
│  │  ├───0.97 MB (01.43%) -- string-chars
│  │  ├───0.81 MB (01.20%) -- mjit-code
│  │  ├───0.58 MB (00.85%) -- (3 omitted)
│  │  └───0.46 MB (00.68%) -- object-slots

And see the attachment for more examples.
Comment 3 Andreas Gal :gal 2011-07-18 23:12:13 PDT
Comment on attachment 546715 [details] [diff] [review]
patch

There is a special place reserved in hell for people who do hard-coding like this. Just say'n.
Comment 4 Nicholas Nethercote [:njn] 2011-07-18 23:21:31 PDT
Which hard-coding?  I'm not relying on the name of the system principal, I'm using JSCompartment::isSystemCompartment.  Indeed, the patch removes one place where the name was hard-coded, using SYSTEM_PRINCIPAL_SPEC instead.
Comment 5 Andreas Gal :gal 2011-07-18 23:39:38 PDT
Yeah, the original sin was done by gregor. We will try to add some clean API any-time-now (tm).
Comment 6 Nicholas Nethercote [:njn] 2011-07-18 23:50:09 PDT
Gregor's hack was fixed by bug 671482.  Bug 669123 is still open for a more general partitioning of compartments, should we desire it.
Comment 7 Nicholas Nethercote [:njn] 2011-07-19 17:10:43 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/877914fc95bb
Comment 8 Marco Bonardo [::mak] 2011-07-20 06:59:08 PDT
http://hg.mozilla.org/mozilla-central/rev/877914fc95bb

Note You need to log in before you can comment on or make changes to this bug.