Closed
Bug 671159
Opened 13 years ago
Closed 13 years ago
Mark entries in about:memory that come from multiple same-named memory reporters
Categories
(Toolkit :: about:memory, defect)
Toolkit
about:memory
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
(Whiteboard: [MemShrink:P2][inbound])
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
justin.lebar+bug
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
In the sandbox case, we may be able to get back to something useful about the sandbox, maybe...
Assignee | ||
Comment 2•13 years ago
|
||
(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.
Assignee | ||
Comment 3•13 years ago
|
||
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.
Summary: Don't combine explicit/* memory reporters with the same path → Mark entries in about:memory that come from multiple same-named memory reporters
Assignee | ||
Comment 4•13 years ago
|
||
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.
Attachment #546707 -
Flags: review?(justin.lebar+bug)
Assignee | ||
Comment 5•13 years ago
|
||
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.
Attachment #546707 -
Attachment is obsolete: true
Attachment #546717 -
Flags: review?(justin.lebar+bug)
Attachment #546707 -
Flags: review?(justin.lebar+bug)
Comment 6•13 years ago
|
||
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.
Attachment #546717 -
Flags: review?(justin.lebar+bug) → review+
Assignee | ||
Comment 7•13 years ago
|
||
I removed the "Warning:" and updated test_aboutmemory.xul.
http://hg.mozilla.org/integration/mozilla-inbound/rev/1bee4ff7c91c
Whiteboard: [MemShrink:P2] → [MemShrink:P2][inbound]
Comment 8•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in
before you can comment on or make changes to this bug.
Description
•