Closed Bug 615978 Opened 9 years ago Closed 9 years ago

Add SQLITE_DBSTATUS_* memory usage to memory reporter.

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: dougt, Assigned: sdwilsh)

References

Details

Attachments

(2 files, 4 obsolete files)

Attached patch patch v.1 (obsolete) — Splinter Review
See http://www.sqlite.org/c3ref/c_dbstatus_cache_used.html
Attachment #494493 - Flags: review?(sdwilsh)
+    } 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 ?
Note: Core::SQL is dead
Status: NEW → ASSIGNED
Component: SQL → Storage
Product: Core → Toolkit
QA Contact: omarshall → storage
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.
sdwilsh, this is working fine.  are you suggesting a race on (un)registration?  The storageMemoryReporters are called on the main thread.
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.
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.
sdwilsh, in bug 619096 I addressed your concern.  Could you review this patch assuming that 619096 lands first.
Depends on: 619096
sdwilsh, ping?
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.
Attachment #494493 - Flags: review?(sdwilsh) → review-
how so?  phiikon, any details?
(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 on attachment 494493 [details] [diff] [review]
patch v.1

re-requesting review.
Attachment #494493 - Flags: review- → review?(sdwilsh)
Assignee: doug.turner → nobody
Was this patch ever ran in a debug build?  It get a ton of assertions about non-threadsafe AddRef/Release when opening about:memory.
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
Attached patch patch v.2 (obsolete) — Splinter Review
This actually builds on windows (changes were needed to the sqlite.def file for it to link properly).
Attachment #494493 - Attachment is obsolete: true
Attachment #494493 - Flags: review?(sdwilsh)
Comment on attachment 494493 [details] [diff] [review]
patch v.1

r- for threadsafety issues, plus the issue mentioned in comment 9.
Attachment #494493 - Flags: review-
Whiteboard: [good first bug]
Attached patch Storage v1.0 (obsolete) — Splinter Review
Addresses my own review comments and updates the style to follow the style guide.  I'll post a patch for the front-end shortly.
Assignee: nobody → sdwilsh
Attachment #517771 - Attachment is obsolete: true
Attachment #521676 - Flags: review?(bugmail)
Attached patch about:memory v1.0 (obsolete) — Splinter Review
Attachment #521685 - Flags: review?(dtownsend)
Whiteboard: [good first bug] → [needs review asuth][needs review Mossop]
Attachment #521685 - Flags: review?(dtownsend) → review+
Whiteboard: [needs review asuth][needs review Mossop] → [needs review asuth]
Attachment #521676 - Flags: review?(bugmail) → review+
Whiteboard: [needs review asuth] → [can land]
Attachment #521676 - Attachment is obsolete: true
Attachment #521685 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/9e77e9db7128
http://hg.mozilla.org/mozilla-central/rev/f38fa181d902
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [can land]
Target Milestone: --- → mozilla2.2
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.
(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.
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.
Shawn, how can there be more than one reporter for a single path?
(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.
(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!
(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.
(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.
You need to log in before you can comment on or make changes to this bug.