The default bug view has changed. See this FAQ.

Negative value for "heap-used/other" in about:memory due to sqlite reporters double-counting some memory

RESOLVED FIXED

Status

()

Toolkit
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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

6 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

6 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

6 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

6 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

6 years ago
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.
Attachment #530526 - Flags: review?(gavin.sharp)
(Assignee)

Comment 5

6 years ago
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
Attachment #530528 - Flags: review?(sdwilsh)
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

6 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

6 years ago
Attachment #530526 - Flags: review?(gavin.sharp) → review?(sdwilsh)
No, they don't need superreview.
(Assignee)

Comment 9

6 years ago
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.
Attachment #530526 - Attachment is obsolete: true
Attachment #530526 - Flags: review?(sdwilsh)
Attachment #530989 - Flags: review?(sdwilsh)
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 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

6 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

6 years ago
http://hg.mozilla.org/mozilla-central/rev/2b4ad7cfb696
http://hg.mozilla.org/mozilla-central/rev/eb114a45cd31

Updated

6 years ago
Depends on: 656773
(Assignee)

Updated

6 years ago
No longer depends on: 656773
(Assignee)

Updated

6 years ago
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.