671159 - Mark entries in about:memory that come from multiple same-named memory reporters

Status: RESOLVED FIXED

(Toolkit :: about:memory, defect)

Description

[Nicholas Nethercote [inactive]]

It's possible to have multiple explicit/* memory reporters with the same name, and about:memory will combine them into one.  This was added in bug 615978 because there can be multiple connections to each SQLite database.  And it's also apparently possible to have multiple compartments with the same name due to sandboxes.

This merging is arguably a bad thing, especially for the "[System Principal]" compartment, which can get quite large.  There's no point hiding information that we have, so it should be undone.  Each duplicate reporter needs to be distinguished in some way;  appending a number would probably do, though getting the exact presentation right may be a little tricky.

Comment 1

[Boris Zbarsky [:bzbarsky]]

In the sandbox case, we may be able to get back to something useful about the sandbox, maybe...

Comment 2

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> In the sandbox case, we may be able to get back to something useful about
> the sandbox, maybe...

If we do that, it would make sense to do it when creating the name for the compartment reporter (in XPConnectJSCompartmentsMultiReporter) rather than in about:memory.

Comment 3

[Nicholas Nethercote [inactive]]

Hmm, doing this in about:memory won't give good results.  Consider part of a compartment's measurements:

│  ├───57,082,913 B (16.29%) -- compartment([System Principal])
│  │   ├──37,629,952 B (10.74%) -- gc-heap
│  │   │  ├──12,449,344 B (03.55%) -- shapes
│  │   │  ├──12,118,248 B (03.46%) -- objects
│  │   │  ├───9,616,856 B (02.74%) -- arena-unused
│  │   │  ├───2,750,240 B (00.78%) -- strings
│  │   │  ├─────473,656 B (00.14%) -- arena-padding
│  │   │  ├─────220,488 B (00.06%) -- arena-headers
│  │   │  └───────1,120 B (00.00%) -- xml

If we have two compartments with the same name, and give a different suffix to the 2nd compartment's reporters, we'll end up with something like this (ignore the numbers, they're meaningless):

│  ├───57,082,913 B (16.29%) -- compartment([System Principal])
│  │   ├──37,629,952 B (10.74%) -- gc-heap
│  │   │  ├──12,449,344 B (03.55%) -- shapes
│  │   │  ├──12,449,344 B (03.55%) -- shapes2
│  │   │  ├──12,118,248 B (03.46%) -- objects
│  │   │  ├──12,118,248 B (03.46%) -- objects2
│  │   │  ├───9,616,856 B (02.74%) -- arena-unused
│  │   │  ├───9,616,856 B (02.74%) -- arena-unused2
│  │   │  ├───2,750,240 B (00.78%) -- strings
│  │   │  ├───2,750,240 B (00.78%) -- strings2
│  │   │  ├─────473,656 B (00.14%) -- arena-padding
│  │   │  ├─────473,656 B (00.14%) -- arena-padding2
│  │   │  ├─────220,488 B (00.06%) -- arena-headers
│  │   │  ├─────220,488 B (00.06%) -- arena-headers2
│  │   │  ├───────1,120 B (00.00%) -- xml
│  │   │  └───────1,120 B (00.00%) -- xml2

when clearly we want something like this:

│  ├───57,082,913 B (16.29%) -- compartment([System Principal])
│  │   ├──37,629,952 B (10.74%) -- gc-heap
│  │   │  ├──12,449,344 B (03.55%) -- shapes
│  │   │  ├──12,118,248 B (03.46%) -- objects
│  │   │  ├───9,616,856 B (02.74%) -- arena-unused
│  │   │  ├───2,750,240 B (00.78%) -- strings
│  │   │  ├─────473,656 B (00.14%) -- arena-padding
│  │   │  ├─────220,488 B (00.06%) -- arena-headers
│  │   │  └───────1,120 B (00.00%) -- xml
...
│  ├───57,082,913 B (16.29%) -- compartment([System Principal])2
│  │   ├──37,629,952 B (10.74%) -- gc-heap
│  │   │  ├──12,449,344 B (03.55%) -- shapes
│  │   │  ├──12,118,248 B (03.46%) -- objects
│  │   │  ├───9,616,856 B (02.74%) -- arena-unused
│  │   │  ├───2,750,240 B (00.78%) -- strings
│  │   │  ├─────473,656 B (00.14%) -- arena-padding
│  │   │  ├─────220,488 B (00.06%) -- arena-headers
│  │   │  └───────1,120 B (00.00%) -- xml

To do that, you have to know which component of the path should have a suffix added, which depends on the reporter.

So, we'll need to do special stuff for each reporter kind (as per comment 2) that has duplicates.

However, something that we can do in a general fashion that will be useful is to mark entries in about:memory that involved the merging of entries.  So I'll morph this bug to be about that.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: patch

This patch changes about:memory so that entries created from multiple memory reporters have a "[+]" after their name.  This is like the existing "[*]" which comes after entries that encountered an error.  If you hover your mouse over the "[+]" it tells you how many reporters were merged together.

I need to update test_aboutmemory.xul but I haven't because someone's broken the chrome test harness somehow.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: patch, v2

Much like the last version, but instead of printing "[+]" it prints "[n]", where "n" is the number of reporters that were merged.  Eg:

│      ├────849,896 B (01.46%) -- places.sqlite
│      │    ├──729,168 B (01.26%) -- cache-used [4]
│      │    ├───77,464 B (00.13%) -- schema-used [4]
│      │    └───43,264 B (00.07%) -- stmt-used [4]

Much better.

Comment 6

[Justin Lebar (not reading bugmail)]

Comment on attachment 546717 [details] [diff] [review]
patch, v2

>+    const dupDesc = "Warning: This value is the sum of " + aNMerged +
>+                    " memory reporters that all have the same path.";

I might not say "Warning:" for this one, since it's a perfectly normal (and usually harmless) occurrence.

Comment 7

[Nicholas Nethercote [inactive]]

I removed the "Warning:" and updated test_aboutmemory.xul.

http://hg.mozilla.org/integration/mozilla-inbound/rev/1bee4ff7c91c

Comment 8

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/1bee4ff7c91c