Closed Bug 411894 Opened 12 years ago Closed 6 years ago

Storage service should obey "memory-pressure" and tell sqlite to free memory

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31
blocking-b2g 1.3T+
Tracking Status
b2g-v1.3T --- fixed

People

(Reporter: sdwilsh, Assigned: gsvelto)

References

Details

(Keywords: perf, Whiteboard: [MemShrink:P3])

Attachments

(2 files, 8 obsolete files)

13.39 KB, patch
gsvelto
: review+
Details | Diff | Splinter Review
13.40 KB, patch
Details | Diff | Splinter Review
stuart mentioned on irc yesterday that we should be telling sqlite to free memory when some observer topic is dispatched.  We just need to call sqlite3_release_memory at that time, and we'll be golden.
http://sqlite.org/c3ref/release_memory.html

I'm submitting a question to the sqlite mailing list regarding how to achieve the most "memory-freeing" possible since the function seems to ask for how many bytes you want to free.
Flags: blocking1.9?
oh - stuart, what's that topic that it should be listening for?
eh, not sure we want this:
================================
There is extra overhead associated with SQLITE_ENABLE_MEMORY_MANAGEMENT.
I just ran some tests using the latest code in CVS on SuSE 10.1
and gcc 4.1.0 with -O6 (non-amalgamation).  For the workload
I used the mix of operations found in the speed2.test test file
in the source tree.

+TS means SQLITE_THREADSAFE=1. -TS means SQLITE_THREADSAFE=0.
+MM means SQLITE_ENABLE_MEMORY_MANAGEMENT=1 is defined. -MM
means it is not.

  Configuration      Raw-Time      Normalized
    -TS -MM          34427759         100
    -TS +MM          35589951         103
    +TS -MM          38751594         113
    +TS +MM          41907742         122

So it appears that with threadsafe disabled, the performance
hit for memory management is only about 3%.  With threading
enabled, the hit is more like 9%.  This makes sense because
there is additional mutexing that has to occur when managing
the memory pool across multiple threads.

- D. Richard Hipp <drh@hwaci.com>
the notification is "memory-pressure", with a few underlings:
http://mxr.mozilla.org/mozilla/source/xpcom/base/nsIMemory.idl#49

but if the hit is 9% (what workload?) then this probably isn't worth it.
Summary: storage service should tell sqlite to free memory when neccesary → storage service should tell sqlite to free memory when necessary
Yea - given comment 2 I don't think we want this on by default unless we are sure it will help a bunch.  sqlite is unlikely the biggest consumer of mem...
Flags: blocking1.9? → blocking1.9-
WONTFIX per comment 2
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
> [There is a significant performance hit to enabling mem management] because
> there is additional mutexing that has to occur when managing
> the memory pool across multiple threads.

Surely there's a way to obey memory-pressure without mutexing every alloc!  I'd rather get async memory reduction than no memory reduction at all.

> sqlite is unlikely the biggest consumer of mem...

According to about:memory, my storage caches are >30MB (>35%) after restarting Firefox and opening four tabs.  Most of that is Places and URLClassifier.
Summary: storage service should tell sqlite to free memory when necessary → Storage service should obey "memory-pressure" and tell sqlite to free memory
(In reply to comment #6)
> According to about:memory, my storage caches are >30MB (>35%) after restarting
> Firefox and opening four tabs.  Most of that is Places and URLClassifier.
True, but those numbers are pretty consistent.  They aren't (likely) to grow as you use your browser more (and if they do, it's very likely a bug that should be filed on that consumer).
No longer blocks: MatchStartupMem
Can we please revisit the decision to close this bug?

Dropping caches on memory pressure is particularly important on mobile.  When Firefox goes into the background, it gets a memory pressure notification.  If we free up enough memory, the OS doesn't kill us, and that makes starting the browser up again much faster.

The ~30mb here is really significant on mobile since we (try to) get rid of basically everything else.
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Whiteboard: [MemShrink]
(In reply to Justin Lebar [:jlebar] from comment #8)
> The ~30mb here is really significant on mobile since we (try to) get rid of
> basically everything else.

That 30mb quote was from desktop Firefox, since mobile doesn't even have SafeBrowsing (and therefore no urlclassifier.sqlite).

Anyone have memory numbers for mobile?
> Anyone have memory numbers for mobile?

On my Nexus-S, SQLite is using 19mb, 16mb of which are for the Places cache.
Just to be clear, I'm not saying that we should absolutely do this, no matter what the performance penalty is.  But it doesn't look like we investigated too much: The numbers in comment 2 are for a SQLite benchmark which may or may not mirror how Firefox uses SQLite, and it's not clear whether turning on that SQLite option is the only way we'll be able to solve this problem.
Have seen that sqlite extends memory use when going into private browsing and this is not freed when you come back out of it - normal Firefox not mobile Firefox but would think its a similar issue
That's bug 689910, which you filed.  The fact that the SQLite caches increase by .5mb after switching between private browsing mode and normal mode a few times doesn't have anything to do with this bug.
(In reply to Justin Lebar [:jlebar] from comment #13)
> That's bug 689910, which you filed.  The fact that the SQLite caches
> increase by .5mb after switching between private browsing mode and normal
> mode a few times doesn't have anything to do with this bug.

Hi Justin - that was seen in the other bug - however this bug appears to deal with it whilst the other bug is simply dealing with an increase in memory without sqlite being involved
Whiteboard: [MemShrink] → [MemShrink:P2]
See also bug 681525, "Bug 681525 - Provide ability to blow away sqlite caches based on inactivity".
In the trunk of the SQLite source tree, but as of yet unreleased, is a new feature that might help here:

    PRAGMA shrink_memory;

When this pragma is invoked, SQLite will free up as much of its page cache memory as it can from the database that invoked the pragma.  (NB:  This happens at sqlite3_prepare(), not at sqlite3_step(), if that makes a different to your implementation.)

Unknown pragmas are silently ignored in SQLite, so if you can go ahead and inserted calls to this pragma in FF if you want.  They will currently be no-ops.  Then at some point in the future after you upgrade SQLite, they will magically start working.
(In reply to D. Richard Hipp from comment #16)
> When this pragma is invoked, SQLite will free up as much of its page cache
> memory as it can from the database that invoked the pragma.  (NB:  This
> happens at sqlite3_prepare(), not at sqlite3_step(), if that makes a
> different to your implementation.)

I assume sqlite3_exec() is fine with it?

You told about some threading requirements, if we have 2 threads on a connection, should we invoke it from both of them?
Yes, sqlite3_exec() would be the ideal way to invoke PRAGMA shrink_memory.

The shrink_memory pragma needs to be invoked once for each database connection.  If two or more threads are sharing the same database connection, then only one thread needs to call the pragma, though it is harmless if more than one does.

But if two or more threads have their own connections to the same database file, then must be a separate invocation of PRAGMA shrink_memory for each connection.
Thank you very much, this is very helpful.
Depends on: 702815
drh, I don't see |shrink_memory| at http://www.sqlite.org/pragma.html.  Will it be added to an SQLite release any time soon?

Nb: if we ever make any movement on bug 703427, that would obviate the need for this change.
(In reply to Nicholas Nethercote [:njn] from comment #20)
> drh, I don't see |shrink_memory| at http://www.sqlite.org/pragma.html.  Will
> it be added to an SQLite release any time soon?

it's in upcoming 3.7.10, we will shortly update to 3.7.9 and update to 3.7.10 when available.
Bug 718455 is open for upgrading Firefox to SQLite 3.7.10.
Depends on: SQLite3.7.10
Downgrading to MemShrink:P3 because it's not relevant on mobile now that mobile doesn't use the storage service for anything large any more.
Whiteboard: [MemShrink:P2] → [MemShrink:P3]
(In reply to Nicholas Nethercote [:njn] from comment #23)
> Downgrading to MemShrink:P3 because it's not relevant on mobile now that
> mobile doesn't use the storage service for anything large any more.

Specifically, *android* doesn't use the storage service.  B2G, otoh, uses indexeddb for a bunch of things, and indexeddb uses sqlite.
I'm trying to figure out if this can still be helpful for FxOS. It seems that we have all the necessary functionality to implement this as part of the shrinkMemory() method in Sqlite.jsm (and compact() in storage.jsm which relies on it). shrinkMemory() is already being called a period of inactivity if an option is set (shrinkMemoryOnConnectionIdleMS) but it seems to me that none of the Sqlite consumers use it.

Anyway I'm willing to take this and test if it helps on FxOS but not being familiar with the IndexedDB implementation I need a couple of clarifications on how this should be implemented. Does IndexedDB rely on storage.jsm for its implementation? If it does then listening to memory-pressure events in storage.jsm would seem the way to go. Also, would this have an impact on other consumers of storage.jsm (if there are any)? Alternatively would it be better to implement this in Sqlite.jsm and have consumers enable this functionality via an option similar to what shrinkMemoryOnConnectionIdleMS does?
Flags: needinfo?(n.nethercote)
I don't know much about storage... needinfo'ing Marco.
Flags: needinfo?(n.nethercote) → needinfo?(mak77)
IndexedDB does not use Sqlite.jsm.  Sqlite.jsm is a JS convenience wrapper on top of mozStorage.  IndexedDB does use mozStorage for some things like opening the databases, but it also does some low-level SQLite poking itself.

The most thorough fix for this would be to have the storage service observe the event in:
http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageService.cpp#883

As you can see in the existing observer implementation, the service maintains a list of live connections.  We would want to add a shrinkMemory method on each connection, then run through the list of connections and call the method on each.  In the method, we check the connection state and mAsyncExecutionThread.  Bail if the connection is closed.  If there is an async thread, we dispatch the PRAGMA to be executed on it.  If there is no async thread, dispatch synchronously on the main thread.  This should avoid the main thread blocking on I/O happening on an async thread.
(In reply to Andrew Sutherland (:asuth) from comment #27)
> IndexedDB does not use Sqlite.jsm.  Sqlite.jsm is a JS convenience wrapper
> on top of mozStorage.  IndexedDB does use mozStorage for some things like
> opening the databases, but it also does some low-level SQLite poking itself.

I assume this is done via mozStorageConnection, correct? I mean, IndexedDB doesn't bypass mozStorageConnection from what I understand.

> As you can see in the existing observer implementation, the service
> maintains a list of live connections.  We would want to add a shrinkMemory
> method on each connection, then run through the list of connections and call
> the method on each.  In the method, we check the connection state and
> mAsyncExecutionThread.  Bail if the connection is closed.  If there is an
> async thread, we dispatch the PRAGMA to be executed on it.  If there is no
> async thread, dispatch synchronously on the main thread.  This should avoid
> the main thread blocking on I/O happening on an async thread.

I've tried to implement what you described in this patch but since I'm not familiar with this code I'm not sure if I'm handling the connections / locking / threading properly. Also I haven't tested this yet, so consider it a WIP.
Assignee: nobody → gsvelto
Status: REOPENED → ASSIGNED
Attachment #8339766 - Flags: feedback?(bugmail)
Flags: needinfo?(mak77)
Comment on attachment 8339766 [details] [diff] [review]
[PATCH] Flush all mozStorageConnections' caches in response to memory pressure events

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

High level architectural question for :mak, moving feedback.  In general this all seems sane and good.

From a testing perspective, I'm not sure how much/if any testing we need here, which also seems like a good mak question.  If we were obsessive compulsive comprehensive, I think the test we would want would be to (for both sync and async connections):
- Open our connection, don't do much.
- Trigger our about:memory reporters for storage, latch the value.
- Do something that will cause the caches to fill up.
- Trigger the about:memory reporters, make sure their usage goes up.
- Explicitly call the minimizeMemory method, wait for it to complete.
- Trigger the about:memory reporters, make sure the usage went back down again.
- Maybe refill the caches, repeat the check process by triggering the observer.

If we already have higher level tests that do something similar for triggering the observer, that might already be sufficient.

::: storage/src/mozStorageConnection.cpp
@@ +484,5 @@
> +      NS_LITERAL_CSTRING("PRAGMA shrink_memory;"));
> +  }
> +
> +private:
> +  nsRefPtr<Connection> mConnection;

mConnection will be released on the async thread which could potentially cause destruction from the async thread, which is not what we want.  I think we may need to proxy the release with NS_ProxyRelease like we do for AsyncCloseConnection.

(Of course, a properly utilized Connection will have the caller call AsyncClose which will already keep us safe there, but properly utilized is not a good thing to depend on... :)

Another alternative might be to introduce a helper called AsyncExecuteSimpleSQL which would just create an async statement, dispatch it, and mark it for finalization.  That way we don't need an additional runnable class.  A nice thing is that this mechanism could then support a callback for free, which could be useful for unit tests.

I think this is a :mak call, so I will feedback him.

::: storage/src/mozStorageConnection.h
@@ +182,5 @@
>  
> +  /**
> +   * Minimize the memory used by this connection.
> +   */
> +  nsresult minimizeMemory();

I was thinking we might put this on mozIStorageBaseStatement so that callers could call this directly in cases where they know they're done doing stuff for a while.

::: storage/src/mozStorageService.cpp
@@ +366,5 @@
> +
> +  for (uint32_t i = 0; i < connections.Length(); i++) {
> +    nsRefPtr<Connection> &conn = connections[i];
> +
> +    SQLiteMutexAutoLock lockedScope(conn->sharedDBMutex);

I am not sure why you are taking a lock here.  You definitely do not want to be taking this lock, since it's the SQLite connection lock and is doing the opposite of what we want for async purposes.

In general, the only reason you would want to ever take this lock is if you are calling a bunch of sub-operations that would themselves take this lock and there's another thread that could fight you for the lock, resulting in crazy interleaving of operations.  That's not the case here.

So I'd remove this lock.

@@ +534,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // Register for memory-pressure events in order to flush the connections'
> +  // caches when we're tight on memory.
> +  rv = os->AddObserver(this, "memory-pressure", /* onwsWeak */ false);

typo nit: 'onws' should be 'owns'
Attachment #8339766 - Flags: feedback?(bugmail) → feedback?(mak77)
> - Open our connection, don't do much.
> - Trigger our about:memory reporters for storage, latch the value.
> - Do something that will cause the caches to fill up.
> - Trigger the about:memory reporters, make sure their usage goes up.
> - Explicitly call the minimizeMemory method, wait for it to complete.
> - Trigger the about:memory reporters, make sure the usage went back down
> again.
> - Maybe refill the caches, repeat the check process by triggering the
> observer.

If you take this route, please use the nsIMemoryReporter.storageSQLite property.  It's a "distinguished amount", i.e. one that is designed to be measured in isolation.  (In contrast, most memory reporters are designed to only be accessed in a group with all the other memory reporters.)
(In reply to Andrew Sutherland (:asuth) from comment #29)
> Comment on attachment 8339766 [details] [diff] [review]

Thanks for your feedback!

> From a testing perspective, I'm not sure how much/if any testing we need
> here, which also seems like a good mak question.  If we were obsessive
> compulsive comprehensive, I think the test we would want would be to (for
> both sync and async connections):
> [...] 
> If we already have higher level tests that do something similar for
> triggering the observer, that might already be sufficient.

I did some testing myself on a FxOS device to see how much this would gain us and I've observed a ~50KiB drop in the storage/sqlite component in the main process. Looking further into the breakdown shows it coming from the cache component of the sqlite connections so this superficially seems to work a-ok. I haven't observed any drops in the content processes but I'm not sure how those handle connections (or if this does not cover all connections in the system).

> mConnection will be released on the async thread which could potentially
> cause destruction from the async thread, which is not what we want.  I think
> we may need to proxy the release with NS_ProxyRelease like we do for
> AsyncCloseConnection.

I've changed it accordingly.

> Another alternative might be to introduce a helper called
> AsyncExecuteSimpleSQL which would just create an async statement, dispatch
> it, and mark it for finalization.  That way we don't need an additional
> runnable class.  A nice thing is that this mechanism could then support a
> callback for free, which could be useful for unit tests.

That sounds like a cleaner and more flexible approach, I'll give it a try and post another patch.

> I was thinking we might put this on mozIStorageBaseStatement so that callers
> could call this directly in cases where they know they're done doing stuff
> for a while.

You mean having a method that adds the shrink operation at the end of the other statements so that it gets executed once we're finished?

> I am not sure why you are taking a lock here.  You definitely do not want to
> be taking this lock, since it's the SQLite connection lock and is doing the
> opposite of what we want for async purposes.
> 
> In general, the only reason you would want to ever take this lock is if you
> are calling a bunch of sub-operations that would themselves take this lock
> and there's another thread that could fight you for the lock, resulting in
> crazy interleaving of operations.  That's not the case here.
> 
> So I'd remove this lock.

I had copied the memory reporter code that iterated over the connections which contained the lock. Thanks for explaining why this is superfluous here. I'm leaving :mak's feedback flag so he can chime in if he wants too. I hope that the next iteration of this patch will be good enough for review.
Attachment #8339766 - Attachment is obsolete: true
Attachment #8339766 - Flags: feedback?(mak77)
Attachment #8348196 - Flags: feedback?(mak77)
(In reply to Gabriele Svelto [:gsvelto] from comment #31)
> > I was thinking we might put this on mozIStorageBaseStatement so that callers
> > could call this directly in cases where they know they're done doing stuff
> > for a while.
> 
> You mean having a method that adds the shrink operation at the end of the
> other statements so that it gets executed once we're finished?

Sort-of. You've added the shrink method as something that's not exposed via XPCOM.  I propose exposing it like the other public API methods so that users of mozStorage can explicitly reduce memory usage  Because of the way the async queue works and the way the method is being implemented, callers could schedule their async job and then call this method and what you propose would effectively be the net result.  (They could also just manually include the PRAGMA in their batch too, if they wanted.)

I guess it could also make sense to have a defaulting boolean flag on batch invocation, but that gets into higher level caching issues that might be better dealt with in other ways.  If this memory-pressure thing is something that will fire on platforms where we log telemetry, it might be useful to have add a telemetry probe about how much memory the SQLite shrink operation saves each time it fires.  Then if we see that value climbing, we could know that there is fat to be cut and it's time to investigate and deal with that more.
(In reply to Andrew Sutherland (:asuth) from comment #32)
> Sort-of. You've added the shrink method as something that's not exposed via
> XPCOM.  I propose exposing it like the other public API methods so that
> users of mozStorage can explicitly reduce memory usage  Because of the way
> the async queue works and the way the method is being implemented, callers
> could schedule their async job and then call this method and what you
> propose would effectively be the net result.  (They could also just manually
> include the PRAGMA in their batch too, if they wanted.)

Actually, since this is well supported by a pragma, I'd not care to have an explicit API for it. The existing API imo makes simple enough to just add such pragma statement to a queue of statements. And I think we should do our best so that consumers don't even need to do such "low-level" calls themselves, we could manage them ourselves (as we do here with memory-pressure, and we could on idle and so on).
My take is that we should provide sort of minimal APIs for execution and helpers, but only in the case using such APIs brings to bloated code or repeated boilerplate. This doesn't appear to be the case.

> If this memory-pressure thing is something
> that will fire on platforms where we log telemetry, it might be useful to
> have add a telemetry probe about how much memory the SQLite shrink operation
> saves each time it fires.

This may be useful for a follow-up bug.
(In reply to Andrew Sutherland (:asuth) from comment #29) 
> From a testing perspective, I'm not sure how much/if any testing we need
> here, which also seems like a good mak question.  If we were obsessive
> compulsive comprehensive, I think the test we would want would be to (for
> both sync and async connections):
> - Open our connection, don't do much.
> - Trigger our about:memory reporters for storage, latch the value.
> - Do something that will cause the caches to fill up.
> - Trigger the about:memory reporters, make sure their usage goes up.
> - Explicitly call the minimizeMemory method, wait for it to complete.
> - Trigger the about:memory reporters, make sure the usage went back down
> again.
> - Maybe refill the caches, repeat the check process by triggering the
> observer.

Actually, we don't need to test that shrink_memory works, that's Sqlite's land, I suppose they have their own test for that, and we don't need to duplicate them. So ideally for us it would be enough to verify that on memory-pressure (and in future other events) we issue the pragma.
That said, I'm not sure such check is easier to do, apart prlog I don't remember us having any other queries logging facility. So it's possible doing this through memory loggers is easier in the end. The above steps look fine, apart that I'd only go once through the observer call (by not providing an external API)

> Another alternative might be to introduce a helper called
> AsyncExecuteSimpleSQL which would just create an async statement, dispatch
> it, and mark it for finalization.  That way we don't need an additional
> runnable class.  A nice thing is that this mechanism could then support a
> callback for free, which could be useful for unit tests.
> 
> I think this is a :mak call, so I will feedback him.

sounds like this may be useful, could be part of mozIStorageAsyncConnection
Attachment #8348196 - Flags: feedback?(mak77) → feedback+
Keywords: perf
Whiteboard: [MemShrink:P3] → [MemShrink:P3][tarako]
do we know how much memory saving we get from this?


moving to 1.3T? flag and therefore removing [Tarako] whiteboard
blocking-b2g: --- → 1.3T?
Flags: needinfo?(gsvelto)
Whiteboard: [MemShrink:P3][tarako] → [MemShrink:P3]
you may look at the sum of all of the "cache-used" entries under each database in the Storage section of about:memory. It will be a little bit less than that though, since not all of that memory may be released. I can't give you numbers since I don't have a device atm.
(In reply to Joe Cheng [:jcheng] from comment #35)
> do we know how much memory saving we get from this?

In my simple testing I saw only a ~50KiB drop in the main app. That was done with light reference workloads and an empty profile though. I plan on testing again using my main phone which has populated databases and all in the hope of getting some more realistic data.
Flags: needinfo?(gsvelto)
thanks Gabriele, just ni? you again so when you have the data ready, you can update this bug. thanks
Flags: needinfo?(gsvelto)
I am removing this form the 1.3T? queue until we see data on significant gains/losses based on :gsvelto's testing and an outline on risk here if this is worth uplifting for Tarako.

I am going to poke Gabriele by email and request him to renom if needed.
blocking-b2g: 1.3T? → ---
This is a refreshed patch that applies to the current master branch. I didn't have time to clean it up as discussed here but will do soon. In the meantime I did some more thorough testing with a populated profile and real databases from my daily use.

The memory saved here varies depending on what you're using, for normal applications when we trigger the memory pressure event I see savings ranging from 25 to 50 KiB mostly coming exclusively from the main b2g process. When using the browser on the other hand the savings are more pronounced, I've seen up to 100 KiB in memory savings, still coming from the main b2g process.

To measure the amount of saved memory I've hardcoded two calls to sqlite3_db_status() before and after flushing the connections' cache and reported the difference. It's a crude method but definitely accurate. BTW the savings come entirely from sqlite's cache, the remaining structures seem unaffected.
Attachment #8348196 - Attachment is obsolete: true
Flags: needinfo?(gsvelto)
Not sure if bug 974811 is similar or a different memory one
Updated patch that adds an executeSimpleSQLAsync() method to mozIStorageAsyncConnection and then uses it to execute the "PRAGMA shrink_memory" directive. This is simpler and IMHO looks much better than my previous patch; it also seems to work fine - I've tested it on my buri.
Attachment #8387654 - Attachment is obsolete: true
Attachment #8388789 - Flags: review?(mak77)
Comment on attachment 8388789 [details] [diff] [review]
[PATCH v3] Flush all mozStorageConnections' caches in response to memory pressure events

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

This needs tests. For sure tests for the new API are mandatory.
If making a test for the shrink memory case is hard I'd accept a follow-up bug for it. As I said we'd only care that on a fake memory-pressure topic (just QI the service to nsIObserver and call .observe) we either check the shrink_memory query ran or that memory report reduced. The former is problematic cause we don't have queries logging facilities, I don't know about the latter, it may be non-trivial at all.

::: storage/public/mozIStorageAsyncConnection.idl
@@ +137,5 @@
> +   *        The callback object that will be notified of progress, errors, and
> +   *        completion.
> +   */
> +  void executeSimpleSQLAsync(in AUTF8String aSQLStatement,
> +    [optional] in mozIStorageStatementCallback aCallback);

should return mozIStoragePendingStatement, you are already getting it in the patch but not exposing it

::: storage/src/mozStorageConnection.cpp
@@ +1391,5 @@
> +Connection::ExecuteSimpleSQLAsync(const nsACString &aSQLStatement,
> +                                  mozIStorageStatementCallback *aCallback)
> +{
> +  nsCOMPtr<mozIStorageAsyncStatement> stmt;
> +  CreateAsyncStatement(aSQLStatement, getter_AddRefs(stmt));

needs error check

@@ +1395,5 @@
> +  CreateAsyncStatement(aSQLStatement, getter_AddRefs(stmt));
> +
> +  // Dispatch to the background
> +  nsCOMPtr<mozIStoragePendingStatement> pendingStatement;
> +  return stmt->ExecuteAsync(aCallback, getter_AddRefs(pendingStatement));

please add an outparam for pendingStatement

We should also finalize() this statement just after executeAsync...

::: storage/src/mozStorageService.cpp
@@ +348,5 @@
> +  getConnections(connections);
> +
> +  for (uint32_t i = 0; i < connections.Length(); i++) {
> +    connections[i]->ExecuteSimpleSQLAsync(
> +      NS_LITERAL_CSTRING("PRAGMA shrink_memory;"), nullptr);

you don't need the semicolon

Actually this is good for main-thread connections, but for connections created off the main-thread we should send a runnable to conn->threadOpenedOn using the sync executeSimpleSQL... this requires some additional code but should be more correct than involving a third thread.

By design we made all async methods in mozIStorageAsyncConnections be main-thread only (see for example http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageConnection.cpp#1115), so unless someone has any objections, we should continue like that. There is no much value using the async API off the main-thread. So, I'd suggest adding the main-thread check in executeSimpleSQLAsync, and here act differently based on threadOpenedOn, as suggested.

@@ +887,4 @@
>      nsCOMPtr<nsIObserverService> os =
>        mozilla::services::GetObserverService();
>      os->RemoveObserver(this, "xpcom-shutdown-threads");
> +    os->RemoveObserver(this, "memory-pressure");

could you please also remove xpcom-shutdown while here?
Attachment #8388789 - Flags: review?(mak77)
Thanks for the review, I'll update my patch according to your comments and I'll try to come up with tests too but I've also got a couple of questions:

(In reply to Marco Bonardo [:mak] from comment #43)
> We should also finalize() this statement just after executeAsync...

I've assumed that the statement would be finalized once the reference left its scope having seen the comment here:

http://hg.mozilla.org/mozilla-central/file/b1b1f0e5f8da/storage/public/mozIStorageBaseStatement.idl#l29

I'm I misunderstanding this?

> could you please also remove xpcom-shutdown while here?

Since I'm at it can I replace the AddObsever/RemoveObserver list with an array of topics I iterate over to add/remove them? This was discussed on dev-platform recently and I feel like it's a good idea pretty much in every place where we have multiple AddObserver/RemoveObserver calls.
(In reply to Gabriele Svelto [:gsvelto] from comment #44)
> I've assumed that the statement would be finalized once the reference left
> its scope having seen the comment here:

bah, yes, as usual I'm mixing up js and cpp :/ Luckily in any case Finalize doesn't hurt, but you are right.

> > could you please also remove xpcom-shutdown while here?
> 
> Since I'm at it can I replace the AddObsever/RemoveObserver list with an
> array of topics I iterate over to add/remove them? This was discussed on
> dev-platform recently and I feel like it's a good idea pretty much in every
> place where we have multiple AddObserver/RemoveObserver calls.

Fine by me.
Updated patch now with unit-tests added. I've basically taken the test_connection_executeAsync.js, trimmed it down a bit and made it use executeSimpleSQLAsync() instead.
Attachment #8388789 - Attachment is obsolete: true
Attachment #8394035 - Flags: review?(mak77)
Comment on attachment 8394035 [details] [diff] [review]
[PATCH v4] Flush all mozStorageConnections' caches in response to memory pressure events

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

The most changes are in the test, so I think this is worth an r+
If you need another quick look at it I'm available, even if I think I may put some faith on test changes. Just ping me on IRC in case you're in a hurry to have that.

::: storage/public/mozIStorageAsyncConnection.idl
@@ +135,5 @@
> +   * @param aSQLStatement  The SQL statement to execute
> +   * @param aCallback [optional]
> +   *        The callback object that will be notified of progress, errors, and
> +   *        completion.
> +   * @return an object that can be used to cancel the statements execution.

s/statements/statement/ (no plural)

::: storage/src/mozStorageConnection.cpp
@@ +1401,5 @@
> +  if (NS_FAILED(rv)) {
> +    return rv;
> +  }
> +
> +  // Dispatch to the background

I don't think this comment is useful, the code here is self documenting... Please remove it

::: storage/src/mozStorageService.cpp
@@ +350,5 @@
> +
> +  getConnections(connections);
> +
> +  for (uint32_t i = 0; i < connections.Length(); i++) {
> +    connections[i]->ExecuteSimpleSQLAsync(

ExecuteSimpleSQLAsync will aready do that, but I think it's better to be explicit here, so please check ConnectionReady, before using the connection. The documentation of getConnections is pretty clear about that  (http://mxr.mozilla.org/mozilla-central/source/storage/src/mozStorageService.h#116)

@@ +351,5 @@
> +  getConnections(connections);
> +
> +  for (uint32_t i = 0; i < connections.Length(); i++) {
> +    connections[i]->ExecuteSimpleSQLAsync(
> +      NS_LITERAL_CSTRING("PRAGMA shrink_memory"), nullptr, getter_AddRefs(ps));

please add a DebugOnly rv and MOZ_ASSERT(NS_SUCCEEDED(rv))

@@ +481,5 @@
>  
> +static const char* sObserverTopics[] = {
> +  "xpcom-shutdown",
> +  "xpcom-shutdown-threads",
> +  "memory-pressure"

nit: I'd like to keep these in the likely received order, so please move memory-pressure above xpcom-shutdown

@@ +519,5 @@
>    nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
>    NS_ENSURE_TRUE(os, NS_ERROR_FAILURE);
> +  size_t length = ArrayLength(sObserverTopics);
> +
> +  for (size_t i = 0; i < length; ++i) {

why not just put ArrayLength(sObserverTopics) in the for condition?

@@ +894,5 @@
>      nsCOMPtr<nsIObserverService> os =
>        mozilla::services::GetObserverService();
> +    size_t length = ArrayLength(sObserverTopics);
> +
> +    for (size_t i = 0; i < length; ++i) {

ditto

@@ +895,5 @@
>        mozilla::services::GetObserverService();
> +    size_t length = ArrayLength(sObserverTopics);
> +
> +    for (size_t i = 0; i < length; ++i) {
> +      os->RemoveObserver(static_cast<nsIObserver*>(this), sObserverTopics[i]);

Is this cast really needed? I think it's already implicit. Not that it's a problem, mostly guessing if there's other reasons apart making it explicit.

::: storage/src/mozStorageService.h
@@ +150,5 @@
>     */
>    nsTArray<nsRefPtr<Connection> > mConnections;
>  
>    /**
> +   * Frees as much memory as possible.

"Frees as much heap memory as possible from all of the known open connections."

::: storage/test/unit/test_connection_executeSimpleSQLAsync.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * This file tests the functionality of
> + * mozIStorageConnection::executeSimpleSQLAsync for.

The method is in mozIStorageAsyncConnection.

It's true that mozIStorageConnection inherits from mozIStorageAsyncConnection, but they are not exactly the same. Indded mozIStorageConnection on the main-thread is deprecated, even if that's hard to do really due to too many consumers.

@@ +12,5 @@
> +const REAL = 3.23;
> +
> +function test_create_and_add()
> +{
> +  getOpenedDatabase().executeSimpleSQL(

I fear you will have to open a database manually since getOpenedDatabase() returns a mozIStorageConnection, and we'd like to move to the async version...
and use executeSimpleSQLAsync here already.

See openAsyncDatabase in test_storage_connection.js

due to that, I suggest you use add_task to run tests here, so you can just yield openAsyncDatabase

@@ +36,5 @@
> +      do_throw("unexpected error!");
> +    },
> +    handleCompletion: function(aReason)
> +    {
> +      dump("handleCompletion("+aReason+")\n");

the dumps don't look useful, we are already throwing or running tests.

@@ +42,5 @@
> +
> +      // Check that the result is in the table
> +      let stmt = getOpenedDatabase().createStatement(
> +        "SELECT string, number FROM test WHERE id = ?"
> +      );

please again, use an async connection, createAsyncStatement and executeAsync. Basically anything that is not in mozIStorageAsyncConnection, should not be used in new code.

you may want to write a small promise-based helper to avoid callbacks in callbacks (not mandatory). I think test_storage_connection.js has some.

@@ +54,5 @@
> +        stmt.finalize();
> +      }
> +
> +      // Make sure we have two rows in the table
> +      stmt = getOpenedDatabase().createStatement(

ditto

@@ +84,5 @@
> +    handleError: function(aError)
> +    {
> +      print("Error code " + aError.result + " with message '" +
> +            aError.message + "' returned.");
> +      do_throw("Unexpected error!");

remove the print and just make a better do_throw message

@@ +89,5 @@
> +    },
> +    handleCompletion: function(aReason)
> +    {
> +      print("handleCompletion(" + aReason +
> +            ") for test_asyncClose_does_not_complete_before_statements");

no reason for this print

@@ +111,5 @@
> +
> +[
> +  test_create_and_add,
> +  test_asyncClose_does_not_complete_before_statement
> +].forEach(add_test);

please just use inline add_task(function* () { /*the test*/ });
Attachment #8394035 - Flags: review?(mak77) → review+
Updated patch with all review comments taken into account, carrying over the r+ flag as there were no changes to the logic besides explicitly finalizing the statement which apparently wasn't being finalized when going out of scope and triggered some warnings in the xpcshell tests. The try run is here:

https://tbpl.mozilla.org/?tree=Try&rev=3872410d3dc9
Attachment #8394035 - Attachment is obsolete: true
Attachment #8396325 - Flags: review+
This is not looking good, there's a bunch of tests that are failing and though they're marked as intermittent I don't see them failing in other runs from today so I fear that they're caused by my patch. I'll try to reproduce the issue locally and update the patch accordingly.
ah, this sucks, the problem is that we are asynchronously executing the PRAGMA on every connection, also on synchronous connections.  due to this we are basically enforcing every connection to use asyncClose, even if they never executed async statements. But many connections are still using .close().
This is a fact due to our legacy behavior of having 2 APIs usable from the main-thread.
I fear we'll have to make minimizeMemory distinguish synchronous from asynchronous connections and call executeSimpleSQL or executeSimpleSQL async depending on that...
Since mAsyncExecutionThread is an internal detail we are not exposing, I'd say we may try to distinguish only based on the interface, so use executesimpleSQL on mozIStorageConnection, executeSimpleSQLAsync on mozIStorageAsyncConnection.
and just to clarify, you must finalize() statements in js, it's just on C that you can rely on the destructor doing that for you. Though, you don't have to wait for onHandleCompletion to call finalize(), you can asyncExecute(callback);finalize() immediately, they will just be enqueued.
(In reply to Marco Bonardo [:mak] from comment #51)
> and just to clarify, you must finalize() statements in js, it's just on C
> that you can rely on the destructor doing that for you. Though, you don't
> have to wait for onHandleCompletion to call finalize(), you can
> asyncExecute(callback);finalize() immediately, they will just be enqueued.

Actually, I think I messed up and posted an obsolete patch with the old tests and w/o the finalization change I've mentioned in comment 49. Silly me :-/
Updated patch that now distinguishes between sync and async connections when sending the PRAGMA directive. This also uses executeSimpleSQLAsync() method exclusively to exercise it a bit more. In local testing this seems to pass the tests that were failing correctly.

I'm asking for review again because there's some small but important changes compared to the previous version and I'm not sure if the tests are written as they should as I'm not familiar with task.js yet. As I mentioned before I'm now explicitly finalizing the statement in the C++ code as the test harness was complaining of it not being finalized when calling asyncClose().

This was due to the statement being retained by the asynchronous execution code (see here http://hg.mozilla.org/mozilla-central/file/9afe2a1145bd/storage/src/StorageBaseStatementInternal.cpp#l197 and http://hg.mozilla.org/mozilla-central/file/9afe2a1145bd/storage (/src/mozStorageAsyncStatementExecution.cpp#l170 ) and so when it went out of scope its refcount didn't drop to zero in turn preventing it from being finalized.

The try run is here: https://tbpl.mozilla.org/?tree=Try&rev=9ebad784ff34
Attachment #8396325 - Attachment is obsolete: true
Attachment #8397102 - Flags: review?(mak77)
Comment on attachment 8397102 [details] [diff] [review]
[PATCH v6] Flush all mozStorageConnections' caches in response to memory pressure events

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

::: storage/src/mozStorageConnection.cpp
@@ +1408,5 @@
> +    return rv;
> +  }
> +
> +  stmt->Finalize();
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));

I think your issue here is due to the test being wrong, see my comments below.
(fwiw, this MOZ_ASSERT is bogus, since rv is not being assigned, so here it's always NS_OK)

::: storage/src/mozStorageService.cpp
@@ +345,5 @@
>  void
> +Service::minimizeMemory()
> +{
> +  nsTArray<nsRefPtr<Connection> > connections;
> +  DebugOnly<nsresult> rv;

pleae move rv definition inside the if (that is where it's being used)

@@ +354,5 @@
> +    nsRefPtr<Connection> conn = connections[i];
> +
> +    if (conn->ConnectionReady()) {
> +      nsCOMPtr<mozIStorageConnection> syncConn = do_QueryInterface(
> +        NS_ISUPPORTS_CAST(mozIStorageAsyncConnection *, conn));

nit: no space before *

@@ +910,5 @@
>      nsCOMPtr<nsIObserverService> os =
>        mozilla::services::GetObserverService();
> +
> +    for (size_t i = 0; i < ArrayLength(sObserverTopics); ++i) {
> +      os->RemoveObserver(this, sObserverTopics[i]);

please (void)os->RemoveObs... per module code style convention

::: storage/test/unit/test_connection_executeSimpleSQLAsync.js
@@ +34,5 @@
> +  });
> +  return deferred.promise;
> +}
> +
> +function executeAsync(statement, onResult) {

doesn't look like this is being used anywhere...

@@ +53,5 @@
> +  return deferred.promise;
> +}
> +
> +add_task(function test_create_and_add() {
> +  getOpenedDatabase().executeSimpleSQL(

just do this after openAsyncDatabase using executeSimpleSQLAsync?

@@ +97,5 @@
> +                       + aError.message);
> +            },
> +            handleCompletion: function(aReason) {
> +            }
> +          });

doesn't look like your test is properly waiting for completion, as it is now, as soon as you invoke executeSimpleSQLAsync the test harness proceeds to the next test... and then the next test does the same, the harness starts shutdown and your code didn't run yet.

you need to make your add_task function* return a promise, and resolve it in the most internal handleCompletion, in this case

@@ +103,5 @@
> +      });
> +    }
> +  });
> +
> +  asyncClose(adb);

the other problem here is that you are posting a close runnable here, just after the first async execution, but this way your second async execution won't happen, cause it will be enqueued after the close runnable.

so basically here you have
executeasync
close
executeasync

This is not going to work. You may do two ways:
make a helper that yields until the execution is complete, so you

yield executeAsync
yield executeasync
yield asyncClose

Or just define a deferred, and resolve it in the internal handleCompletion... clearly in this case you should not invoke asyncClose until you have executed both statements, and you should not proceed until asyncClose is done, to avoid possible interactions between the tests.

@@ +113,5 @@
> +  let executed = false;
> +
> +  adb.executeSimpleSQLAsync("SELECT * FROM sqlite_master", {
> +    handleResult: function(aResultSet) {
> +    },

you expect this to return something, to make the test more complete would be nice to also check handleResult is invoked at least once

@@ +129,5 @@
> +    // Ensure that the statement executed to completion.
> +    do_check_true(executed);
> +
> +    // Reset gDBConn so that later tests will get a new connection object.
> +    gDBConn = null;

again here you are not making the harness wait for you test to be complete before proceeding
Attachment #8397102 - Flags: review?(mak77)
Further modifications to the patch:

- I'm not finalizing the statement in executeSimpleSQLAsync() anymore, it's not needed and was only peppering over the broken tests

- I've fixed the tests to execute in proper order, I've also hardened them to check all possible steps in order to catch erroneous control-flow as was happening before

- I've removed all the synchronous leftovers from the tests harness

The try run is here: https://tbpl.mozilla.org/?tree=Try&rev=406e639c3361
Attachment #8397102 - Attachment is obsolete: true
Attachment #8397241 - Flags: review?(mak77)
Comment on attachment 8397241 [details] [diff] [review]
[PATCH v7] Flush all mozStorageConnections' caches in response to memory pressure events

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

mostly nits, nice work!

::: storage/public/mozIStorageAsyncConnection.idl
@@ +134,5 @@
> +   *
> +   * @param aSQLStatement  The SQL statement to execute
> +   * @param aCallback [optional]
> +   *        The callback object that will be notified of progress, errors, and
> +   *        completion.

nit: the first param description should be indented like the second param description (newline after the param name, indented as the param name)

::: storage/src/mozStorageService.cpp
@@ +346,5 @@
> +Service::minimizeMemory()
> +{
> +  nsTArray<nsRefPtr<Connection> > connections;
> +
> +  getConnections(connections);

nit: please remove the newline between these

@@ +351,5 @@
> +
> +  for (uint32_t i = 0; i < connections.Length(); i++) {
> +    nsRefPtr<Connection> conn = connections[i];
> +
> +    if (conn->ConnectionReady()) {

nit: as well as the newline between these

@@ +359,5 @@
> +
> +      if (!syncConn) {
> +        nsCOMPtr<mozIStoragePendingStatement> ps;
> +
> +        rv = connections[i]->ExecuteSimpleSQLAsync(

nit: and this one :)

@@ +364,5 @@
> +          NS_LITERAL_CSTRING("PRAGMA shrink_memory"), nullptr,
> +          getter_AddRefs(ps));
> +      } else {
> +        rv = connections[i]->ExecuteSimpleSQL(
> +          NS_LITERAL_CSTRING("PRAGMA shrink_memory"));

the query is the same, so you could define it outside the if/else with
NS_NAMED_LITERAL_CSTRING(shrinkPragma, "PRAGMA shrink_memory");

@@ +367,5 @@
> +        rv = connections[i]->ExecuteSimpleSQL(
> +          NS_LITERAL_CSTRING("PRAGMA shrink_memory"));
> +      }
> +
> +      MOZ_ASSERT(NS_SUCCEEDED(rv));

please add a message to the assertion, like "Should have been able to shrink Sqlite memory" or such.

::: storage/test/unit/test_connection_executeSimpleSQLAsync.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/*
> + * This file tests the functionality of
> + * mozIStorageAsyncConnection::executeSimpleSQLAsync for.

s/ for//

@@ +43,5 @@
> +    },
> +    handleResult: function(result) {
> +      if (onResult) {
> +        onResult(result);
> +      }

this is fine, though, if there's no onResult handler, this should call do_throw, since in such cases we don't expect results.

@@ +54,5 @@
> +}
> +
> +add_task(function test_create_and_add() {
> +  let adb = yield openAsyncDatabase(getTestDB());
> +  let string = "CREATE TABLE test (" +

so, I don't like that this var is called "string", and I don't see what's its usefulness here, why not just inlining the queries into the calls?
The only case where it would make the code less readable, is when you have the onResult handler, in such a case it may be useful, but should be called "query", "sql" or whatever else that is not "string" :)

@@ +59,5 @@
> +                 "id INTEGER, " +
> +                 "string TEXT, " +
> +                 "number REAL " +
> +               ")"
> +  let result = null;

please define just before first use (just before the first executeSimpleSQLAsync using it will be fine)

@@ +61,5 @@
> +                 "number REAL " +
> +               ")"
> +  let result = null;
> +
> +  yield executeSimpleSQLAsync(adb, string);

check returned reason?

@@ +72,5 @@
> +  do_check_eq(Ci.mozIStorageStatementCallback.REASON_FINISHED, completion);
> +
> +  string = "SELECT string, number FROM test WHERE id = 1";
> +
> +  yield executeSimpleSQLAsync(adb, string, function(aResultSet) {

check returned reason?
Attachment #8397241 - Flags: review?(mak77) → review+
(In reply to Marco Bonardo [:mak] from comment #56)
> mostly nits, nice work!

Thanks for your quick review and your help on this bug! Here's the updated patch with the following changes:

- rename |string| to |query| in the |executeSimpleSQLAsync()| promise helper
- inlined all the SQL queries
- added further checks for the SQL statements completion return value
- addressed all remaining nits

Carrying over the r+ flag from attachment 8397241 [details] [diff] [review]. The try run is here: https://tbpl.mozilla.org/?tree=Try&rev=151c35bf87dc

Fingers crossed :)
Attachment #8397241 - Attachment is obsolete: true
Attachment #8397754 - Flags: review+
Try is green, time for check-in.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e7fb993a5f8a
Status: ASSIGNED → RESOLVED
Closed: 11 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
This is a patch that applies cleanly to mozilla-b2g28_v1_3t in case we'd want to take this for FxOS v1.3t. I've tested it on my Tarako and it works well there.
Nomming for v1.3t since it's already landed on master and it seems to work fine both there and on the actual device using a v1.3t build. The savings are not huge (up to ~100KiB in my measurements) but worth pursuing since they happen in the main b2g process.
blocking-b2g: --- → 1.3T?
Gabriele, please uplift.
blocking-b2g: 1.3T? → 1.3T+
(In reply to Fabrice Desré [:fabrice] from comment #63)
> Gabriele, please uplift.

Thanks Fabrice. Setting checkin-needed again, attachment 8398506 [details] [diff] [review] needs to land on mozilla-b2g28_v1_3t, the patch should apply cleanly.
You need to log in before you can comment on or make changes to this bug.