Last Comment Bug 653630 - Negative value for "heap-used/other" in about:memory due to sqlite reporters double-counting some memory
: Negative value for "heap-used/other" in about:memory due to sqlite reporters ...
Status: RESOLVED FIXED
:
Product: Toolkit
Classification: Components
Component: General (show other bugs)
: unspecified
: All All
: -- normal (vote)
: ---
Assigned To: Nicholas Nethercote [:njn] (on vacation until July 11)
:
Mentors:
Depends on: revampaboutmemory
Blocks:
  Show dependency treegraph
 
Reported: 2011-04-28 18:26 PDT by Nicholas Nethercote [:njn] (on vacation until July 11)
Modified: 2011-05-16 18:36 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
allow non-leaf reporters (against m-c 69037:2dc4783d9270) (19.30 KB, patch)
2011-05-05 21:07 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
no flags Details | Diff | Review
fix sqlite reporters (against m-c 69037:2dc4783d9270) (4.78 KB, patch)
2011-05-05 21:09 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
sdwilsh: review+
Details | Diff | Review
allow non-leaf reporters, v2 (17.66 KB, patch)
2011-05-08 23:48 PDT, Nicholas Nethercote [:njn] (on vacation until July 11)
sdwilsh: review+
Details | Diff | Review

Description Nicholas Nethercote [:njn] (on vacation until July 11) 2011-04-28 18:26:43 PDT
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.
Comment 1 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-04-28 18:31:36 PDT
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
Comment 2 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-04-28 18:37:36 PDT
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.
Comment 3 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-04 00:06:42 PDT
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
Comment 4 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-05 21:07:36 PDT
Created attachment 530526 [details] [diff] [review]
allow non-leaf reporters (against m-c 69037:2dc4783d9270)

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.
Comment 5 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-05 21:09:08 PDT
Created attachment 530528 [details] [diff] [review]
fix sqlite reporters (against m-c 69037:2dc4783d9270)

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
Comment 6 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-05 21:30:38 PDT
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.
Comment 7 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-05 22:18:18 PDT
(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?
Comment 8 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-05-06 08:08:04 PDT
No, they don't need superreview.
Comment 9 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-08 23:48:41 PDT
Created attachment 530989 [details] [diff] [review]
allow non-leaf reporters, v2

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.
Comment 10 Shawn Wilsher :sdwilsh 2011-05-09 10:31:49 PDT
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
Comment 11 Shawn Wilsher :sdwilsh 2011-05-09 10:37:57 PDT
Comment on attachment 530989 [details] [diff] [review]
allow non-leaf reporters, v2

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

r=sdwilsh
Comment 12 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-09 17:57:36 PDT
(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 :)
Comment 13 Nicholas Nethercote [:njn] (on vacation until July 11) 2011-05-11 21:04:07 PDT
http://hg.mozilla.org/mozilla-central/rev/2b4ad7cfb696
http://hg.mozilla.org/mozilla-central/rev/eb114a45cd31

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