Closed Bug 911641 Opened 11 years ago Closed 11 years ago

Reduce about:compartments to a vestigial nub

Categories

(Toolkit :: about:memory, defect)

Product:
Toolkit
Component:
about:memory
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla26

People

(Reporter: n.nethercote, Assigned: n.nethercote)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file, 2 obsolete files)

(part 1) - Remove about:compartments, and show the "compartments" and "ghost-window" lists into about:memory.
46.55 KB, patch
johns
: review+
Details | Diff | Splinter Review 
about:compartments was created at the height of the "leaky add-ons" crisis, to
make it easy for add-on authors and reviewers to determine if add-on caused
zombie compartments.  Since bug 695480 landed, its usefulness has plummeted;  I
haven't personally looked at it for ages.  It also doesn't support loading and
saving the way about:memory does.  It's time to fold it into about:memory.
This patch (which applies on top of the patch in bug moves about:compartment's
lists (of compartments and ghost windows) into about:memory's "Other
Measurements" section.

Pros:

- Less code, mostly in aboutMemory.js and test_aboutcompartments.xul.  Diff
  stats:

    13 files changed, 57 insertions(+), 572 deletions(-)

- Less coupling and special-casing.  Compartment and ghost window reporters had
  additional constraints beyond normal reporters (see the removed code in
  nsIMemoryReporter.idl), and aboutMemory.js had to treat compartments and
  ghost windows reporters/reports specially -- ignoring them for about:memory,
  and ignoring everything but them for about:compartments.  All this is gone
  now.

Cons:

- It's marginally harder to see the compartments and ghost windows, because
  they're in among the other reports.  But not much harder!

- about:compartments automatically minimized memory usage on loading, which
  reduced the chance that stale compartments were listed.  You have to do that
  manually in about:memory.

- It makes about:memory slightly longer, though in non-verbose mode it usually
  ends up pretty compact, like this:

> 112 (100.0%) -- compartments
> ├───75 (66.96%) -- user
> │   ├──45 (40.18%) ++ (45 tiny)
> │   ├──28 (25.00%) ── http://www.theatlantic.com/infocus/, about:blank [28]
> │   └───2 (01.79%) ── http://www.theatlantic.com/infocus/, javascript:"<html><body%20style='background:transparent'></body></html>" [2]
> └───37 (33.04%) ++ system

Other things about the patch:

- I kept a vestigial about:compartments that just tells the user to visit
  about:memory.  (I thought about a redirect, but this seems better because the
  user can't fail to notice it.)  We can remove the vestigial
  about:compartments later on.

- test_aboutcompartments.xul mostly tested the special treatment of the
  compartments and ghost windows reports that I mentioned above.  About the
  only useful thing it did that test_aboutmemory.xul didn't was to have a
  report with a very long URL, so I coped that across.

johns, I would have asked mccr8 to do this review but he's currently doing a
much bigger review for me in bug 910517, so I decided to spare him.  This
review should be pretty easy for anyone with a passing familiarity with the
memory reporting infrastructure.
Attachment #798387 - Flags: review?(jschoenick)
One sub-optimal thing about this patch... we now see this in about:memory:

52 (100.0%) -- compartments
├──37 (71.15%) ++ system
└──15 (28.85%) ++ user

52 (100.0%) -- js-compartments
├──37 (71.15%) ── system
└──15 (28.85%) ── user

The first tree is a (collapsed) list of all the compartments.  The second one is just counts of the compartments.  The obvious thing to do is just get rid of the second one, since the first one subsumes it.  *Except*, the two measurements in the second one are used for telemetry, and so can't be removed;  the totals shown in the first one aren't accessible to telemetry.

Ideas:

1. Live with the duplication.  Ugly, but not the end of the world.  People will probably notice and complain.

2. Abandon the telemetry, and remove the second one.  Would be a shame, though the telemetry numbers aren't exactly massively useful right now.

3. Special case the second one so that about:memory doesn't show it.  That gives the best functionality, at the cost of making the code uglier, though there is precedent with the "resident-fast" report which about:memory already ignores.

In other words:  silly-looking output, no telemetry, or ugly code.
> 3. Special case the second one so that about:memory doesn't show it.  That
> gives the best functionality, at the cost of making the code uglier, though
> there is precedent with the "resident-fast" report which about:memory
> already ignores.

I thought of a nicer way of doing this.  So:  we have several reports (from uni-reporters) that are redundant w.r.t. other reports, and so which we wish to ignore when we are dealing with all the reports, such as in about:memory.  In other words, these reports are only of interest when we are dealing with a subset of the available reports, and in practice, we currently only do that for telemetry.

So we want a way of marking these reports as such.  We could add a boolean flag to the callback, but that seems like overkill.  Less invasive would be to say that any path with a particular prefix would be considered as one of these special reports.  I.e. if the chosen prefix was "redundant/", then we would have "redundant/resident-fast", "redundant/js-compartments/system", and "redundant/js-compartments/user".  "telemetry/" is an another possibility for the prefix, albeit one that focuses on how these reports are used, rather than what they are.

Anyway, I think it's a reasonable solution.  I'll modify the patch accordingly, tomorrow.
Attachment #798387 - Flags: review?(jschoenick)
Whiteboard: [MemShrink:P2]
This patch is similar to the previous one except that it renames some of the
JS-related reporters to make it clearer that they measure just the main
JSRuntime (and not the JSRuntimes in worker threads).

With this patch, we have both "js-main-runtime-compartments" and
"js-main-runtime-compartment-counts" trees.  I'll fix up the latter in the next
patch.
Attachment #799203 - Flags: review?(jschoenick)
Attachment #798387 - Attachment is obsolete: true
This fixes it so about:memory doesn't show both the compartment trees, and
makes its ignoring of some reporters/reports more general, to boot.  Yay.
Attachment #799204 - Flags: review?(jschoenick)
BTW, here's some sample about:memory output after part 1 is applied:

> 45.14 MB (100.0%) -- js-main-runtime
> ├──24.97 MB (55.33%) ++ compartments
> ├──13.69 MB (30.33%) ++ zones
> ├───5.01 MB (11.11%) ── runtime
> └───1.46 MB (03.23%) ++ gc-heap
> 
> 192 (100.0%) -- js-main-runtime-compartments
> ├──187 (97.40%) -- system
> │  ├──181 (94.27%) ++ (181 tiny)
> │  ├────2 (01.04%) ── [System Principal], about:blank [2]
> │  ├────2 (01.04%) ── [System Principal], about:newtab [2]
> │  └────2 (01.04%) ── [System Principal], inProcessTabChildGlobal?ownedBy=chrome://browser/content/browser.xul [2]
> └────5 (02.60%) -- user
>      ├──3 (01.56%) ++ (3 tiny)
>      └──2 (01.04%) ── resource://gre-resources/hiddenWindow.html [2]
> 
> 192 (100.0%) -- js-main-runtime-compartments-count
> ├──187 (97.40%) ── system
> └────5 (02.60%) ── user
> 
> 28.02 MB (100.0%) -- js-main-runtime-gc-heap-committed
> ├──25.11 MB (89.61%) -- used
> │  ├──24.28 MB (86.63%) ── gc-things
> │  ├───0.44 MB (01.56%) ── chunk-admin
> │  └───0.40 MB (01.42%) ── arena-admin
> └───2.91 MB (10.39%) -- unused
>     ├──1.89 MB (06.75%) ── gc-things
>     ├──1.00 MB (03.57%) ── chunks
>     └──0.02 MB (00.07%) ── arenas

Part 2 removes the "js-main-runtime-compartments-count" tree.
Comment on attachment 799203 [details] [diff] [review]
(part 1) - Remove about:compartments, and show the "compartments" and "ghost-window" lists into about:memory.

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

::: toolkit/components/aboutmemory/content/aboutMemory.js
@@ +128,5 @@
>  }
>  
>  function onLoad()
>  {
> +  onLoadAboutMemory();

onLoadAboutMemory could just be renamed to onLoad at this point
Attachment #799203 - Flags: review?(jschoenick) → review+
Attachment #799204 - Flags: review?(jschoenick) → review+
Depends on: 910517
Comment on attachment 799204 [details] [diff] [review]
(part 2) - Prefix some reporters with "redundant/", and make about:memory ignore them.

I won't land part 2 -- I have better ideas for solving that problem in bug 913260.
Attachment #799204 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/21e1bf789add
https://hg.mozilla.org/integration/mozilla-inbound/rev/00c58bce0d2d

In the end I did land part 2, because it's a clear improvement over the current code, and I'm not 100% certain that bug 913260 will pan out.
https://hg.mozilla.org/mozilla-central/rev/21e1bf789add
https://hg.mozilla.org/mozilla-central/rev/00c58bce0d2d
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Blocks: 740525
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: