615978 - Add SQLITE_DBSTATUS_* memory usage to memory reporter.

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Doug Turner (:dougt)]

Attachment: patch v.1

See http://www.sqlite.org/c3ref/c_dbstatus_cache_used.html

Comment 1

[Jo Hermans]

+    } else if (mType == Cache_Used) {
+      *desc = strdup("Approximate number of of bytes of heap memory used by all pager caches");
+    } else if (mType == Schema_Used) {
+      *desc = strdup("Approximate number of of bytes of heap memory used to store the schema for all databases associated with the connection");
+    } else if (mType == Stmt_Used) {
+      *desc = strdup("Approximate number of of bytes of heap and lookaside memory used by all prepared statements");
+    }

number of of ?

Comment 2

[Shawn Wilsher :sdwilsh]

Note: Core::SQL is dead

Comment 3

[Shawn Wilsher :sdwilsh]

Note: I don't think memory reporters work off of the main thread, but you'll certainly be adding them off the main thread if you do this in Connection::initialize.

Comment 4

[Doug Turner (:dougt)]

sdwilsh, this is working fine.  are you suggesting a race on (un)registration?  The storageMemoryReporters are called on the main thread.

Comment 5

[Shawn Wilsher :sdwilsh]

I recall vlad telling me that I couldn't register reporters off of the main thread, which is why we do this in Service:
https://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageService.cpp#191

I don't recall the reasons; maybe they've been fixed?

Connection::initialize is called by Service::Open[Unshared|Special]Database, which can be called off of the main thread (urlclassifier does this, we should have tests for it but don't (test_service_init_background_thread.cpp would be easy to modify to test this behavior).  It's been supported since I inherited this code, and we can't break that at this point in the release cycle.

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

it's not a good idea to do so, there's no locking done when the objects are inserted into the reporter hashtable.   But we can probably make it do that, since resgistering reporters is infrequent.  BUT, it does happen at startup, so someone should watch Ts numbers very closely if they do so -- but it would make this stuff a lot simpler.

Comment 7

[Doug Turner (:dougt)]

sdwilsh, in bug 619096 I addressed your concern.  Could you review this patch assuming that 619096 lands first.

Comment 8

[Doug Turner (:dougt)]

sdwilsh, ping?

Comment 9

[Shawn Wilsher :sdwilsh]

Comment on attachment 494493 [details] [diff] [review]
patch v.1

Sorry, I thought I had commented in this bug, but it doesn't look like I did.  This doesn't work in the case where we have multiple connections to the same database (which we currently have two of: places.sqlite and cookies.sqlite).  I'm not really sure what happens exactly, but philiKON had a build with this patch and the numbers were certainly not right for places as a result.

Comment 10

[Doug Turner (:dougt)]

how so?  phiikon, any details?

Comment 11

[Philipp von Weitershausen [:philikon]]

(In reply to comment #9)
> Sorry, I thought I had commented in this bug, but it doesn't look like I did. 
> This doesn't work in the case where we have multiple connections to the same
> database (which we currently have two of: places.sqlite and cookies.sqlite). 
> I'm not really sure what happens exactly, but philiKON had a build with this
> patch and the numbers were certainly not right for places as a result.

I don't recall ever having made such a build (which doesn't necessarily mean that I never did ;))

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 494493 [details] [diff] [review]
patch v.1

re-requesting review.

Comment 13

[Shawn Wilsher :sdwilsh]

Was this patch ever ran in a debug build?  It get a ton of assertions about non-threadsafe AddRef/Release when opening about:memory.

Comment 14

[Shawn Wilsher :sdwilsh]

The issue I mentioned in comment 9 does in fact exist, and it's because of how the front-end displays the results:
https://hg.mozilla.org/mozilla-central/annotate/5666cb06e519/toolkit/components/aboutmemory/content/aboutMemory.js#l169

Comment 15

[Shawn Wilsher :sdwilsh]

Attachment: patch v.2

This actually builds on windows (changes were needed to the sqlite.def file for it to link properly).

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 494493 [details] [diff] [review]
patch v.1

r- for threadsafety issues, plus the issue mentioned in comment 9.

Comment 17

[Shawn Wilsher :sdwilsh]

Attachment: Storage v1.0

Addresses my own review comments and updates the style to follow the style guide.  I'll post a patch for the front-end shortly.

Comment 18

[Shawn Wilsher :sdwilsh]

Attachment: about:memory v1.0

Comment 19

[Shawn Wilsher :sdwilsh]

Attachment: Storage v1.0 for checkin

Comment 20

[Shawn Wilsher :sdwilsh]

Attachment: about:memory v1.0 for checkin

Comment 21

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/9e77e9db7128
http://hg.mozilla.org/mozilla-central/rev/f38fa181d902

Comment 22

[Nicholas Nethercote [inactive]]

This change roughly doubled the number of entries in about:memory, while only accounting for a small fraction of memory used.  I think some consolidation of entries might be necessary in bug 633653 or a follow-up.

Comment 23

[Shawn Wilsher :sdwilsh]

(In reply to comment #22)
> This change roughly doubled the number of entries in about:memory, while only
> accounting for a small fraction of memory used.  I think some consolidation of
> entries might be necessary in bug 633653 or a follow-up.
As I understand it, the mobile team has been using the patch in this bug for a while to help reduce memory usage with consumers of SQLite.  While there is a lot of data, it has been handy for developers.

Comment 24

[Nicholas Nethercote [inactive]]

Thinking some more, having a (possibly adjustable) threshold mechanism would be best, eg. don't show subtrees that account for less than 0.1% or something.  That's what Massif does and it works well.

Comment 25

[Nicholas Nethercote [inactive]]

Shawn, how can there be more than one reporter for a single path?

Comment 26

[Shawn Wilsher :sdwilsh]

(In reply to comment #25)
> Shawn, how can there be more than one reporter for a single path?
Yes, that's why I changed the display in this bug to handle that.

Comment 27

[Nicholas Nethercote [inactive]]

(In reply to comment #26)
> > Shawn, how can there be more than one reporter for a single path?
>
> Yes, that's why I changed the display in this bug to handle that.

Perhaps I wasn't clear.  I don't understand how there can be multiple reporters for a single path.  Can you please explain the circumstances under which this happens?  Thanks!

Comment 28

[Daniel C]

(In reply to comment #26)
> (In reply to comment #25)
> > Shawn, how can there be more than one reporter for a single path?
> Yes, that's why I changed the display in this bug to handle that.

I think you missed the "how" in comment 25.

Comment 29

[Shawn Wilsher :sdwilsh]

(In reply to comment #27)
> Perhaps I wasn't clear.  I don't understand how there can be multiple reporters
> for a single path.  Can you please explain the circumstances under which this
> happens?  Thanks!
Our "path" in this case is the name of the database file.  You can open more than one connection to a given database file, which causes duplicates.