Closed
Bug 653630
Opened 13 years ago
Closed 13 years ago
Negative value for "heap-used/other" in about:memory due to sqlite reporters double-counting some memory
Categories
(Toolkit :: General, defect)
Toolkit
General
Tracking
()
RESOLVED
FIXED
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(2 files, 1 obsolete file)
|
4.78 KB,
patch
|
sdwilsh
:
review+
|
Details | Diff | Splinter Review |
|
17.66 KB,
patch
|
sdwilsh
:
review+
|
Details | Diff | Splinter Review |
about:memory prints trees like this:
├───3,913,376 B (11.07%) -- storage
│ ├──2,233,600 B (06.32%) -- sqlite
│ │ ├──1,521,600 B (04.30%) -- pagecache
│ │ └────712,000 B (02.01%) -- other
│ ├──1,497,904 B (04.24%) -- places.sqlite
│ │ ├──1,290,280 B (03.65%) -- cache-used
│ │ ├────132,440 B (00.37%) -- stmt-used
│ │ └─────75,184 B (00.21%) -- schema-used
│ ├────127,352 B (00.36%) -- cookies.sqlite
│ │ ├──120,072 B (00.34%) -- cache-used
│ │ ├────4,336 B (00.01%) -- stmt-used
│ │ └────2,944 B (00.01%) -- schema-used
│ ├─────21,808 B (00.06%) -- content-prefs.sqlite
│ │ ├───9,632 B (00.03%) -- stmt-used
│ │ ├───8,152 B (00.02%) -- cache-used
│ │ └───4,024 B (00.01%) -- schema-used
│ ├─────13,848 B (00.04%) -- downloads.sqlite
│ │ ├───6,704 B (00.02%) -- stmt-used
│ │ ├───4,320 B (00.01%) -- cache-used
│ │ └───2,824 B (00.01%) -- schema-used
│ ├─────13,160 B (00.04%) -- permissions.sqlite
│ │ ├───6,776 B (00.02%) -- stmt-used
│ │ ├───4,328 B (00.01%) -- cache-used
│ │ └───2,056 B (00.01%) -- schema-used
│ └──────5,704 B (00.02%) -- formhistory.sqlite
│ ├──3,056 B (00.01%) -- cache-used
│ ├──2,648 B (00.01%) -- schema-used
│ └──────0 B (00.00%) -- stmt-used
Currently, all the reporters have to correspond to leaf nodes in the tree. There are two exceptions; "mapped" and "mapped/heap/used" (a.k.a. "heap-used").
The "mapped/other" and "heap-used/other" reporters are auto-computed by aboutMemory.js.
It would be useful if non-leaf reporters could be used more generally, and "other" reporters be auto-computed. For example, the SQLite memory usage is currently over-reported due to different reporters double-counting some bytes. We can get "storage/sqlite" (all the memory that SQLite uses) easily from SQLite, and then the per-connection values ("stmt-used", "cache-used", "schema-used"), but computing the memory used by SQLite that isn't used for connections is difficult. With this facility, aboutMemory.js would auto-compute it.
|
Assignee | |
Comment 1•13 years ago
|
||
Just to clarify the problem with the sqlite reporters, currently:
- storage/sqlite/pagecache overlaps with storage/<connection>.sqlite/cache-used
- storage/sqlite/other overlaps with storage/<connection>.sqlite/{stmt,schema}-used|
|
Assignee | |
Comment 2•13 years ago
|
||
Just to clarify some more... with this change, we'll have reporters for the
following:
- storage/sqlite
- storage/sqlite/<connection>.sqlite/cache-used
- storage/sqlite/<connection>.sqlite/stmt-used
- storage/sqlite/<connection>.sqlite/schema-used
(where there can be an arbitrary number of connections), and aboutMemory.js
will auto-compute:
- storage/sqlite/other
This will result in tree structure in about:memory like this:
-- storage
-- sqlite *
-- places.sqlite
-- cache-used *
-- schema-used *
-- stmt-used *
-- cookies.sqlite
-- cache-used *
-- schema-used *
-- stmt-used *
-- other +
The entries with '*' have a memory reporter, the entry with '+' is
auto-computed by aboutMemory.js, and the others are just found by summing
their children.|
|
Assignee | |
Comment 3•13 years ago
|
||
Jesse got the following, where heap-used/other goes negative due to the double-counting of the sqlite memory:
Main Process
Mapped Memory
3,331,096,576 B (100.0%) -- mapped
├──3,200,246,880 B (96.07%) -- other
├────128,687,008 B (03.86%) -- heap
│ ├───89,980,464 B (02.70%) -- used
│ └───38,706,544 B (01.16%) -- unused
└──────2,162,688 B (00.06%) -- js
├──1,900,544 B (00.06%) -- mjit-code
└────262,144 B (00.01%) -- tjit-code
Used Heap Memory
89,980,464 B (100.0%) -- heap-used
├──92,101,352 B (102.36%) -- storage
│ ├──46,574,536 B (51.76%) -- sqlite
│ │ ├──45,092,416 B (50.11%) -- pagecache
│ │ └───1,482,120 B (01.65%) -- other
│ ├──33,684,720 B (37.44%) -- places.sqlite
│ │ ├──33,316,424 B (37.03%) -- cache-used
│ │ ├─────289,984 B (00.32%) -- stmt-used
│ │ └──────78,312 B (00.09%) -- schema-used
│ ├───8,227,744 B (09.14%) -- urlclassifier3.sqlite
│ │ ├──8,122,560 B (09.03%) -- cache-used
│ │ ├────100,944 B (00.11%) -- stmt-used
│ │ └──────4,240 B (00.00%) -- schema-used
│ ├───1,303,976 B (01.45%) -- cookies.sqlite
│ │ ├──1,288,264 B (01.43%) -- cache-used
│ │ ├─────12,752 B (00.01%) -- stmt-used
│ │ └──────2,960 B (00.00%) -- schema-used
│ ├─────501,560 B (00.56%) -- webappsstore.sqlite
│ │ ├──430,224 B (00.48%) -- cache-used
│ │ ├───64,984 B (00.07%) -- stmt-used
│ │ └────6,352 B (00.01%) -- schema-used
│ ├─────462,504 B (00.51%) -- extensions.sqlite
│ │ ├──363,824 B (00.40%) -- cache-used
│ │ ├───87,496 B (00.10%) -- stmt-used
│ │ └───11,184 B (00.01%) -- schema-used
│ ├─────406,368 B (00.45%) -- downloads.sqlite
│ │ ├──396,840 B (00.44%) -- cache-used
│ │ ├────6,704 B (00.01%) -- stmt-used
│ │ └────2,824 B (00.00%) -- schema-used
│ ├─────335,688 B (00.37%) -- chromeappsstore.sqlite
│ │ ├──265,152 B (00.29%) -- cache-used
│ │ ├───64,184 B (00.07%) -- stmt-used
│ │ └────6,352 B (00.01%) -- schema-used
│ ├─────212,408 B (00.24%) -- content-prefs.sqlite
│ │ ├──198,752 B (00.22%) -- cache-used
│ │ ├────9,632 B (00.01%) -- stmt-used
│ │ └────4,024 B (00.00%) -- schema-used
│ ├─────108,536 B (00.12%) -- permissions.sqlite
│ │ ├───99,704 B (00.11%) -- cache-used
│ │ ├────6,776 B (00.01%) -- stmt-used
│ │ └────2,056 B (00.00%) -- schema-used
│ ├─────107,368 B (00.12%) -- search.sqlite
│ │ ├───99,688 B (00.11%) -- cache-used
│ │ ├────5,672 B (00.01%) -- stmt-used
│ │ └────2,008 B (00.00%) -- schema-used
│ ├─────106,624 B (00.12%) -- signons.sqlite
│ │ ├───99,688 B (00.11%) -- cache-used
│ │ ├────4,464 B (00.00%) -- schema-used
│ │ └────2,472 B (00.00%) -- stmt-used
│ └──────69,320 B (00.08%) -- formhistory.sqlite
│ ├──66,688 B (00.07%) -- cache-used
│ ├───2,632 B (00.00%) -- schema-used
│ └───────0 B (00.00%) -- stmt-used
├──25,729,310 B (28.59%) -- js
│ ├──24,117,248 B (26.80%) -- gc-heap
│ ├───1,014,472 B (01.13%) -- tjit-data
│ │ └──1,014,472 B (01.13%) -- allocators
│ │ ├────740,000 B (00.82%) -- reserve
│ │ └────274,472 B (00.31%) -- main
│ ├─────371,324 B (00.41%) -- mjit-data
│ └─────226,266 B (00.25%) -- string-data
├─────803,101 B (00.89%) -- layout
│ ├──803,101 B (00.89%) -- all
│ └────────0 B (00.00%) -- bidi
├─────304,604 B (00.34%) -- gfx
│ └──304,604 B (00.34%) -- surface
│ └──304,604 B (00.34%) -- image
├─────301,295 B (00.33%) -- images
│ ├──274,360 B (00.30%) -- chrome
│ │ ├──274,360 B (00.30%) -- used
│ │ │ ├──274,360 B (00.30%) -- uncompressed
│ │ │ └────────0 B (00.00%) -- raw
│ │ └────────0 B (00.00%) -- unused
│ │ ├──0 B (00.00%) -- raw
│ │ └──0 B (00.00%) -- uncompressed
│ └───26,935 B (00.03%) -- content
│ ├──26,935 B (00.03%) -- used
│ │ ├──17,540 B (00.02%) -- uncompressed
│ │ └───9,395 B (00.01%) -- raw
│ └───────0 B (00.00%) -- unused
│ ├──0 B (00.00%) -- raw
│ └──0 B (00.00%) -- uncompressed
├───────────0 B (00.00%) -- content
│ └──0 B (00.00%) -- canvas
│ └──0 B (00.00%) -- 2d-pixel-bytes
└──-29,259,198 B (-32.52%) -- other
Other Measurements
204,017,664 B -- resident
89,982,800 B -- heap-zone0-committed
125,542,400 B -- heap-zone0-used|
|
Assignee | |
Updated•13 years ago
|
Summary: Allow non-leaf reporters in the about:memory trees → Negative value for "heap-used/other" in about:memory due to sqlite reporters double-counting some memory
|
|
Assignee | |
Comment 4•13 years ago
|
||
This patch: - Modifies aboutMemory.js to handle memory reporters for non-leaf reporters in the "mapped" and "heap-used" trees. This actually simplifies things a bit, because it means the special-casing for "mapped" and "heap-used" (which were previously the only non-leaf reporters) can be removed. - Updates the chrome test to include a non-leaf reporter. - Adds a new reporter for "heap-used" which is a duplicate of "mapped/heap/used". This was necessary due to the removal of the special-casing in aboutMemory.js.
Attachment #530526 -
Flags: review?(gavin.sharp)
|
|
Assignee | |
Comment 5•13 years ago
|
||
This patch:
- Fixes the sqlite reporters to avoid the double-reporting:
- Removes "heap-used/storage/sqlite/pagecache" and
"heap-used/storage/sqlite/other"; adds a non-leaf reporter
"heap-used/storage/sqlite".
- Moves the per-connection reporters under "heap-used/storage/sqlite".
Here's an example output. The "sqlite/other" value added by aboutMemory.js
is relatively small -- only 18% of the sqlite total -- which is good.
├────5,084,240 B (02.33%) -- storage
│ └──5,084,240 B (02.33%) -- sqlite
│ ├──2,432,240 B (01.11%) -- places.sqlite
│ │ ├──2,082,664 B (00.95%) -- cache-used
│ │ ├────271,896 B (00.12%) -- stmt-used
│ │ └─────77,680 B (00.04%) -- schema-used
│ ├────908,008 B (00.42%) -- other
│ ├────462,368 B (00.21%) -- extensions.sqlite
│ │ ├──363,688 B (00.17%) -- cache-used
│ │ ├───87,496 B (00.04%) -- stmt-used
│ │ └───11,184 B (00.01%) -- schema-used
│ ├────431,504 B (00.20%) -- urlclassifier3.sqlite
│ │ ├──326,320 B (00.15%) -- cache-used
│ │ ├──100,944 B (00.05%) -- stmt-used
│ │ └────4,240 B (00.00%) -- schema-used
│ ├────335,528 B (00.15%) -- chromeappsstore.sqlite
│ │ ├──264,992 B (00.12%) -- cache-used
│ │ ├───64,184 B (00.03%) -- stmt-used
│ │ └────6,352 B (00.00%) -- schema-used
│ ├────269,480 B (00.12%) -- webappsstore.sqlite
│ │ ├──198,944 B (00.09%) -- cache-used
│ │ ├───64,184 B (00.03%) -- stmt-used
│ │ └────6,352 B (00.00%) -- schema-used
│ ├────164,800 B (00.08%) -- cookies.sqlite
│ │ ├──149,328 B (00.07%) -- cache-used
│ │ ├───12,528 B (00.01%) -- stmt-used
│ │ └────2,944 B (00.00%) -- schema-used
│ ├─────21,808 B (00.01%) -- content-prefs.sqlite
│ │ ├───9,632 B (00.00%) -- stmt-used
│ │ ├───8,152 B (00.00%) -- cache-used
│ │ └───4,024 B (00.00%) -- schema-used
│ ├─────13,848 B (00.01%) -- downloads.sqlite
│ │ ├───6,704 B (00.00%) -- stmt-used
│ │ ├───4,320 B (00.00%) -- cache-used
│ │ └───2,824 B (00.00%) -- schema-used
│ ├─────13,800 B (00.01%) -- signons.sqlite
│ │ ├───6,864 B (00.00%) -- cache-used
│ │ ├───4,464 B (00.00%) -- schema-used
│ │ └───2,472 B (00.00%) -- stmt-used
│ ├─────13,160 B (00.01%) -- permissions.sqlite
│ │ ├───6,776 B (00.00%) -- stmt-used
│ │ ├───4,328 B (00.00%) -- cache-used
│ │ └───2,056 B (00.00%) -- schema-used
│ ├─────11,992 B (00.01%) -- search.sqlite
│ │ ├───5,672 B (00.00%) -- stmt-used
│ │ ├───4,312 B (00.00%) -- cache-used
│ │ └───2,008 B (00.00%) -- schema-used
│ └──────5,704 B (00.00%) -- formhistory.sqlite
│ ├──3,056 B (00.00%) -- cache-used
│ ├──2,648 B (00.00%) -- schema-used
│ └──────0 B (00.00%) -- stmt-used
Attachment #530528 -
Flags: review?(sdwilsh)
|
||
Comment 6•13 years ago
|
||
Comment on attachment 530526 [details] [diff] [review] allow non-leaf reporters (against m-c 69037:2dc4783d9270) I'm not going to have time to properly review this any time soon. I don't mind rubber-stamping it if you just want to land it without comprehensive review (about:memory is isolated and non-critical, this has tests), but if you want someone who can provide useful review comments you'll have to look somewhere else.
|
|
Assignee | |
Comment 7•13 years ago
|
||
(In reply to comment #6) > > I'm not going to have time to properly review this any time soon. Ok. Who else is suitable? AFAICT Vlad's the only person other than me who has hacked on aboutMemory.js in any significant way, but he too has limited time these days, and it would be nice to fix this sooner rather than later. I'll ask sdwilsh since I've asked him for the other patch. Oh, do these patches need super-review?
|
|
Assignee | |
Updated•13 years ago
|
Attachment #530526 -
Flags: review?(gavin.sharp) → review?(sdwilsh)
|
|
||
Comment 8•13 years ago
|
||
No, they don't need superreview.
|
|
Assignee | |
Comment 9•13 years ago
|
||
Having the duplicate "mapped/heap/used" and "heap-used" reporters created a problem -- even though they're supposed to be the same number, they would differ by small amounts due to the JS code that executed between them being read. So I removed the "heap-used" one and added code in aboutMemory.js to do the duplication instead.
Attachment #530526 -
Attachment is obsolete: true
Attachment #530526 -
Flags: review?(sdwilsh)
Attachment #530989 -
Flags: review?(sdwilsh)
|
||
Comment 10•13 years ago
|
||
Comment on attachment 530528 [details] [diff] [review] fix sqlite reporters (against m-c 69037:2dc4783d9270) Sorry for the delay on this review! I was traveling last week, which made it hard to look at my queue. >+++ b/storage/src/mozStorageService.cpp >+NS_MEMORY_REPORTER_IMPLEMENT(StorageSQLiteMemoryUsed, >+ "heap-used/storage/sqlite", >+ "Memory used by SQLite. ", nit: you have a space after the period here. r=sdwilsh
Attachment #530528 -
Flags: review?(sdwilsh) → review+
|
|
||
Comment 11•13 years ago
|
||
Comment on attachment 530989 [details] [diff] [review] allow non-leaf reporters, v2 Review of attachment 530989 [details] [diff] [review]: ----------------------------------------------------------------- r=sdwilsh
Attachment #530989 -
Flags: review?(sdwilsh) → review+
|
|
Assignee | |
Comment 12•13 years ago
|
||
(In reply to comment #10) > > Sorry for the delay on this review! No problem, it gave me time to find the problems in the first version and fix them :)
|
|
Assignee | |
Comment 13•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/2b4ad7cfb696 http://hg.mozilla.org/mozilla-central/rev/eb114a45cd31
|
|
Assignee | |
Updated•13 years ago
|
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•