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

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Nicholas Nethercote [inactive]]

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.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: patch

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.)

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 575041 [details] [diff] [review]
patch

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 http://hg.mozilla.org/mozilla-central/annotate/ac667309bea6/storage/style.txt#l38

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.

http://hg.mozilla.org/mozilla-central/annotate/ac667309bea6/storage/style.txt#l9
http://hg.mozilla.org/mozilla-central/annotate/ac667309bea6/storage/style.txt#l55

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: patch v2

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.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: patch v3

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.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 578166 [details] [diff] [review]
patch v3

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

r=sdwilsh

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 http://hg.mozilla.org/mozilla-central/annotate/7dd46087e678/storage/src/mozStorageAsyncStatementExecution.cpp#l366 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.

Comment 6

[Nicholas Nethercote [inactive]]

> > +        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()|.

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1f093067e982

Comment 8

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1f093067e982

Comment 9

[Nicholas Nethercote [inactive]]

Backed out due to the crashes in bug 708248 and bug 708285.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b63bb9f4422

Comment 10

[Ed Morley [:emorley]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/8b63bb9f4422

Comment 11

[Nicholas Nethercote [inactive]]

Landed again, this time with the fix developed in bug 708248 applied.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f33f32134b89

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/f33f32134b89