Closed Bug 703143 Opened 10 years ago Closed 10 years ago

Use a memory multi-reporter for SQLite's per-connection reporting


(Toolkit :: Storage, defect)

Not set





(Reporter: n.nethercote, Assigned: n.nethercote)




(1 file, 2 obsolete files)

Storage has one memory reporter for total memory allocations done by SQLite, and three (stmt/cache/schema) per connection.  Thanks to bug 702815 it's easy to use a single memory multi-reporter for the per-connection measurements.  This puts all the memory reporting logic in one place, which is nice.  It will also remove the need for non-leaf reporters, allowing bug 700508 to be fixed.
Blocks: 692719
Attached patch patch (obsolete) — Splinter Review
The conversion of multiple vanilla reporters to one multi-reporter is pretty
straightforward.  I also created a new reporter called "storage-sqlite" 
which is used by telemetry.

One consequence of this patch is that we no longer have memory reporters 
sticking around for leaked connections;  any memory from such connections 
will fall into the storage/sqlite/other bucket.  This doesn't worry me.  It
also removes the need for test_asyncClose_leak.xul, which is good because it
was a test that failed fairly often (bug 692719).

I tested the new multi-reporter by initially implementing them without 
removing the old reporters, and checking that the numbers reported were 
identical.  It's hard to do better than that because memory reporters are so
non-deterministic.  (Bug 699307 will take some baby steps in that direction,
but it'll only be able to go so far.)
Assignee: nobody → nnethercote
Attachment #575041 - Flags: review?(sdwilsh)
Comment on attachment 575041 [details] [diff] [review]

Review of attachment 575041 [details] [diff] [review]:

In general, this is good, but I want to check the threadsafety stuff when you fix bug 702815.

::: storage/src/mozStorageConnection.cpp
@@ +793,5 @@
>    { // Make sure we have not executed any asynchronous statements.
> +    // If this fails, the mDBConn will be left open, resulting in a leak.
> +    // Ideally we'd schedule some code to destroy the mDBConn once all its
> +    // async statements have finished executing.

We should file a bug to do that, and then reference that bug number in this comment.

::: storage/src/mozStorageService.cpp
@@ +178,5 @@
> +                            nsISupports *aClosure)
> +  {
> +    nsTArray<Connection *> *connections =
> +        Service::getSingleton()->getConnections();
> +    size_t totalConnSize = 0;

Per the other bug, this isn't actually safe, so I'll need to see this patch again after you fix that up.

@@ +182,5 @@
> +    size_t totalConnSize = 0;
> +
> +    for (PRUint32 i = 0; i < connections->Length(); i++) {
> +      Connection *conn = connections->ElementAt(i);
> +      sqlite3 *nativeConn = conn->GetNativeConnection();

`conn` has a type operator to `sqlite3 *`, so you don't ever need to do this actually.

@@ +215,5 @@
> +private:
> +  nsresult reportStat(nsIMemoryMultiReporterCallback *aCallback,
> +                      nsISupports *aClosure, sqlite3 *aNativeConn,
> +                      const nsACString &aPathHead, const nsACString &aKind,
> +                      const nsACString &aDesc, int aOption, size_t *aTotal)

We don't check this, so there is no reason to make it return an `nsresult`.  Make it a `void` please.

nit: place each parameter on it's own line please per

nit: `aTotal` should be `_total`.

I'd also appreciate a java-doc header here since some of the variables aren't obvious without some digging.

@@ +289,3 @@
>      // the main thread (otherwise you'll get cryptic crashes).
> +    NS_RegisterMemoryMultiReporter(new StorageSQLiteMultiReporter);
> +    NS_RegisterMemoryReporter(new NS_MEMORY_REPORTER_NAME(StorageSQLite));

nit: these are both global methods and we aren't checking their results, so they should be prefixed with `::` and their return values cast to `(void)` per storage style.
Attachment #575041 - Flags: review?(sdwilsh) → review-
Attached patch patch v2 (obsolete) — Splinter Review
I'm taking an optimistic/speculative strategy here, in the hope that you're happy with the connections locking strategy I've used in bug 702815.
Attachment #575041 - Attachment is obsolete: true
Attachment #575790 - Flags: review?(sdwilsh)
Attached patch patch v3Splinter Review
I realized a big problem with the old version -- CollectReports() calls Service::getSingleton(), which addrefs the Service and there's no way to release it.  I worked this out by adding a printf to ~Service() to make sure it was being destroyed at shutdown;  it wasn't unless I commented out the getSingleton() and everything that depended on it.

(I don't understand how getSingleton() can be used safely, maybe it's only meant to be used by the NS_GENERIC_FACTORY_SINGLETON_CONSTRUCTOR in storage/build/mozStorageModule.cpp?)

Also, the fact that we never unregister the memory reporters is interesting, because what happens if Service is destroyed and then the memory reporters are queried?

To fix both these problems, this patch changes Service so it holds strong references to the memory reporters and unregisters them in ~Service().  And StorageSQLiteMultiReporter holds a weak reference to Service so it can get the connections.  The possibly async initialization via ServiceMainThreadInitializer::Run() complicates things a bit, but not too much.  I confirmed via printf statements that both the multi-reporter and Service are destroyed on shut-down after viewing about:memory, so it all appears to be working.
Attachment #575790 - Attachment is obsolete: true
Attachment #575790 - Flags: review?(sdwilsh)
Attachment #578166 - Flags: review?(sdwilsh)
Comment on attachment 578166 [details] [diff] [review]
patch v3

Review of attachment 578166 [details] [diff] [review]:


I trust your judgement on addressing my review comments.

::: storage/src/mozStorageService.cpp
@@ +181,5 @@
> +  // thread is accessing the connection.  This isn't very nice if
> +  // CollectReports is called from the main thread!  But at the time of writing
> +  // this function is only called when about:memory is loaded (not, for
> +  // example, when telemetry pings occur) and any delays in that case aren't so
> +  // bad.

We should grab the mutex before making all three calls.  That's what the SQLiteMutex class is for (it's a recursive lock).  See for an example.

@@ +191,5 @@
> +      nsTArray<nsRefPtr<Connection> > connections;
> +      mService->appendConnections(connections);
> +
> +      for (PRUint32 i = 0; i < connections.Length(); i++) {
> +        nsRefPtr<Connection> conn = connections.ElementAt(i);

You could just use a reference here, right?
nsRefPtr<Connection> &conn = connections[i];

No point in using `ElementAt` since it's the same as `[]`.  Unless you wanted `SafeElementAt`?

@@ +192,5 @@
> +      mService->appendConnections(connections);
> +
> +      for (PRUint32 i = 0; i < connections.Length(); i++) {
> +        nsRefPtr<Connection> conn = connections.ElementAt(i);
> +        sqlite3 *nativeConn = conn->GetNativeConnection();

Just use `conn.get()` where you use `nativeConn`.  We have a type operator, and I want to kill off `GetNativeConnection`.  It's legacy code.

@@ +198,5 @@
> +        nsCString pathHead("explicit/storage/sqlite/");
> +        pathHead.Append(conn->getFilename());
> +        pathHead.AppendLiteral("/");
> +
> +        totalConnSize +=

You'd want to acquire that mutex here, and then it'll be released right after we are done getting all our measurements.
Attachment #578166 - Flags: review?(sdwilsh) → review+
> > +        nsRefPtr<Connection> conn = connections.ElementAt(i);
> > +        sqlite3 *nativeConn = conn->GetNativeConnection();
> Just use `conn.get()` where you use `nativeConn`.  We have a type operator,
> and I want to kill off `GetNativeConnection`.  It's legacy code.

It has to be |*conn.get()|.
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Depends on: 708248
Depends on: 708285
Backed out due to the crashes in bug 708248 and bug 708285.
Resolution: FIXED → ---
Merge of backout:
Target Milestone: mozilla11 → ---
Depends on: 708159
Landed again, this time with the fix developed in bug 708248 applied.
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
You need to log in before you can comment on or make changes to this bug.