Bug 1166166 - Shrink storage cache off main thread r=mak

Per bug: it can take around 200ms to shrink memory for Places.sqlite. This can end up janking the browser if we hit a memory-pressure. This patch simply removes the #if DEBUG guards around the mAsyncExecutionThreadIsAlive boolean which is already being updated and exposes it so that we can shrink memory off the main thread when possible. MozReview-Commit-ID: LoDGKrOXs8u --HG-- extra : rebase_source : adf03f187e7297835566f3bac92a8be8a6147131
dothq · Jun 1, 2017 · d65c88d · d65c88d
1 parent f13fa44
commit d65c88d
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 60 deletions.
diff --git a/storage/StorageBaseStatementInternal.cpp b/storage/StorageBaseStatementInternal.cpp
@@ -136,25 +136,27 @@ StorageBaseStatementInternal::destructorAsyncFinalize()
   if (!mAsyncStatement)
     return;
 
-  // If we reach this point, our owner has not finalized this
-  // statement, yet we are being destructed. If possible, we want to
-  // auto-finalize it early, to release the resources early.
-  nsIEventTarget *target = mDBConnection->getAsyncExecutionTarget();
-  if (target) {
-    // If we can get the async execution target, we can indeed finalize
-    // the statement, as the connection is still open.
-    bool isAsyncThread = false;
-    (void)target->IsOnCurrentThread(&isAsyncThread);
-
-    nsCOMPtr<nsIRunnable> event =
-      new LastDitchSqliteStatementFinalizer(mDBConnection, mAsyncStatement);
-    if (isAsyncThread) {
-      (void)event->Run();
-    } else {
+  bool isOwningThread = false;
+  (void)mDBConnection->threadOpenedOn->IsOnCurrentThread(&isOwningThread);
+  if (isOwningThread) {
+    // If we are the owning thread (currently that means we're also the
+    // main thread), then we can get the async target and just dispatch
+    // to it.
+    nsIEventTarget *target = mDBConnection->getAsyncExecutionTarget();
+    if (target) {
+      nsCOMPtr<nsIRunnable> event =
+        new LastDitchSqliteStatementFinalizer(mDBConnection, mAsyncStatement);
       (void)target->Dispatch(event, NS_DISPATCH_NORMAL);
     }
+  } else {
+    // If we're not the owning thread, assume we're the async thread, and
+    // just run the statement.
+    nsCOMPtr<nsIRunnable> event =
+      new LastDitchSqliteStatementFinalizer(mDBConnection, mAsyncStatement);
+    (void)event->Run();
   }
 
+
   // We might not be able to dispatch to the background thread,
   // presumably because it is being shutdown. Since said shutdown will
   // finalize the statement, we just need to clean-up around here.

diff --git a/storage/mozStorageConnection.cpp b/storage/mozStorageConnection.cpp
@@ -456,8 +456,7 @@ class AsyncInitializeClone final: public Runnable
   }
 
   NS_IMETHOD Run() override {
-    MOZ_ASSERT (NS_GetCurrentThread() == mConnection->getAsyncExecutionTarget());
-
+    MOZ_ASSERT(!NS_IsMainThread());
     nsresult rv = mConnection->initializeClone(mClone, mReadOnly);
     if (NS_FAILED(rv)) {
       return Dispatch(rv, nullptr);
@@ -582,7 +581,7 @@ Connection::getSqliteRuntimeStatus(int32_t aStatusOption, int32_t* aMaxValue)
 nsIEventTarget *
 Connection::getAsyncExecutionTarget()
 {
-  MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
+  NS_ENSURE_TRUE(NS_IsMainThread(), nullptr);
 
   // If we are shutting down the asynchronous thread, don't hand out any more
   // references to the thread.
@@ -941,6 +940,13 @@ Connection::isClosed()
   return mConnectionClosed;
 }
 
+bool
+Connection::isAsyncExecutionThreadAvailable()
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  return !!mAsyncExecutionThread;
+}
+
 void
 Connection::shutdownAsyncThread(nsIThread *aThread) {
   MOZ_ASSERT(!mAsyncExecutionThread);
@@ -1216,14 +1222,21 @@ Connection::Close()
   if (!mDBConn)
     return NS_ERROR_NOT_INITIALIZED;
 
-  { // 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;  see bug 704030.
-    MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
-    bool asyncCloseWasCalled = !mAsyncExecutionThread;
-    NS_ENSURE_TRUE(asyncCloseWasCalled, NS_ERROR_UNEXPECTED);
-  }
+#ifdef DEBUG
+  // Since we're accessing mAsyncExecutionThread, we need to be on the opener thread.
+  // We make this check outside of debug code below in setClosedState, but this is
+  // here to be explicit.
+  bool onOpenerThread = false;
+  (void)threadOpenedOn->IsOnCurrentThread(&onOpenerThread);
+  MOZ_ASSERT(onOpenerThread);
+#endif // DEBUG
+
+  // 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;  see bug 704030.
+  bool asyncCloseWasCalled = !mAsyncExecutionThread;
+  NS_ENSURE_TRUE(asyncCloseWasCalled, NS_ERROR_UNEXPECTED);
 
   // setClosedState nullifies our connection pointer, so we take a raw pointer
   // off it, to pass it through the close procedure.
@@ -1237,9 +1250,7 @@ Connection::Close()
 NS_IMETHODIMP
 Connection::AsyncClose(mozIStorageCompletionCallback *aCallback)
 {
-  if (!NS_IsMainThread()) {
-    return NS_ERROR_NOT_SAME_THREAD;
-  }
+  NS_ENSURE_TRUE(NS_IsMainThread(), NS_ERROR_NOT_SAME_THREAD);
 
   // The two relevant factors at this point are whether we have a database
   // connection and whether we have an async execution thread.  Here's what the
@@ -1321,15 +1332,10 @@ Connection::AsyncClose(mozIStorageCompletionCallback *aCallback)
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Create and dispatch our close event to the background thread.
-  nsCOMPtr<nsIRunnable> closeEvent;
-  {
-    // We need to lock because we're modifying mAsyncExecutionThread
-    MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
-    closeEvent = new AsyncCloseConnection(this,
-                                          nativeConn,
-                                          completeEvent,
-                                          mAsyncExecutionThread.forget());
-  }
+  nsCOMPtr<nsIRunnable> closeEvent = new AsyncCloseConnection(this,
+                                                              nativeConn,
+                                                              completeEvent,
+                                                              mAsyncExecutionThread.forget());
 
   rv = asyncThread->Dispatch(closeEvent, NS_DISPATCH_NORMAL);
   NS_ENSURE_SUCCESS(rv, rv);
@@ -1344,9 +1350,7 @@ Connection::AsyncClone(bool aReadOnly,
   PROFILER_LABEL("mozStorageConnection", "AsyncClone",
     js::ProfileEntry::Category::STORAGE);
 
-  if (!NS_IsMainThread()) {
-    return NS_ERROR_NOT_SAME_THREAD;
-  }
+  NS_ENSURE_TRUE(NS_IsMainThread(), NS_ERROR_NOT_SAME_THREAD);
   if (!mDBConn)
     return NS_ERROR_NOT_INITIALIZED;
   if (!mDatabaseFile)
@@ -1679,9 +1683,7 @@ Connection::ExecuteSimpleSQLAsync(const nsACString &aSQLStatement,
                                   mozIStorageStatementCallback *aCallback,
                                   mozIStoragePendingStatement **_handle)
 {
-  if (!NS_IsMainThread()) {
-    return NS_ERROR_NOT_SAME_THREAD;
-  }
+  NS_ENSURE_TRUE(NS_IsMainThread(), NS_ERROR_NOT_SAME_THREAD);
 
   nsCOMPtr<mozIStorageAsyncStatement> stmt;
   nsresult rv = CreateAsyncStatement(aSQLStatement, getter_AddRefs(stmt));

diff --git a/storage/mozStorageConnection.h b/storage/mozStorageConnection.h
@@ -139,6 +139,8 @@ class Connection final : public mozIStorageConnection
    * the thread may be re-claimed if left idle, so you should call this
    * method just before you dispatch and not save the reference.
    *
+   * This must be called from the main thread.
+   *
    * @returns an event target suitable for asynchronous statement execution.
    */
   nsIEventTarget *getAsyncExecutionTarget();
@@ -149,7 +151,6 @@ class Connection final : public mozIStorageConnection
    * asynchronous statements (they are serialized on mAsyncExecutionThread).
    * Currently protects:
    *  - Connection.mAsyncExecutionThreadShuttingDown
-   *  - Connection.mAsyncExecutionThread
    *  - Connection.mConnectionClosed
    *  - AsyncExecuteStatements.mCancelRequested
    */
@@ -234,6 +235,14 @@ class Connection final : public mozIStorageConnection
    */
   bool isClosed();
 
+  /**
+  * True if the async execution thread is alive and able to be used (i.e., it
+  * is not in the process of shutting down.)
+  *
+  * This must be called from the main thread.
+  */
+  bool isAsyncExecutionThreadAvailable();
+
   nsresult initializeClone(Connection *aClone, bool aReadOnly);
 
 private:
@@ -306,6 +315,8 @@ class Connection final : public mozIStorageConnection
    * Lazily created thread for asynchronous statement execution.  Consumers
    * should use getAsyncExecutionTarget rather than directly accessing this
    * field.
+   *
+   * This must be accessed only on the main thread.
    */
   nsCOMPtr<nsIThread> mAsyncExecutionThread;
 

diff --git a/storage/mozStorageService.cpp b/storage/mozStorageService.cpp
@@ -365,8 +365,14 @@ Service::minimizeMemory()
       MOZ_ASSERT(NS_SUCCEEDED(rv), "Should have purged sqlite caches");
     } else if (NS_SUCCEEDED(conn->threadOpenedOn->IsOnCurrentThread(&onOpenedThread)) &&
                onOpenedThread) {
-      // We are on the opener thread, so we can just proceed.
-      conn->ExecuteSimpleSQL(shrinkPragma);
+      if (conn->isAsyncExecutionThreadAvailable()) {
+        nsCOMPtr<mozIStoragePendingStatement> ps;
+        DebugOnly<nsresult> rv =
+          conn->ExecuteSimpleSQLAsync(shrinkPragma, nullptr, getter_AddRefs(ps));
+        MOZ_ASSERT(NS_SUCCEEDED(rv), "Should have purged sqlite caches");
+      } else {
+        conn->ExecuteSimpleSQL(shrinkPragma);
+      }
     } else {
       // We are on the wrong thread, the query should be executed on the
       // opener thread, so we must dispatch to it.

diff --git a/storage/mozStorageStatementData.h b/storage/mozStorageStatementData.h
@@ -78,19 +78,6 @@ class StatementData
   inline void reset()
   {
     NS_PRECONDITION(mStatementOwner, "Must have a statement owner!");
-#ifdef DEBUG
-    {
-      nsCOMPtr<nsIEventTarget> asyncThread =
-        mStatementOwner->getOwner()->getAsyncExecutionTarget();
-      // It's possible that we are shutting down the async thread, and this
-      // method would return nullptr as a result.
-      if (asyncThread) {
-        bool onAsyncThread;
-        NS_ASSERTION(NS_SUCCEEDED(asyncThread->IsOnCurrentThread(&onAsyncThread)) && onAsyncThread,
-                     "This should only be running on the async thread!");
-      }
-    }
-#endif
     // In the AsyncStatement case we may never have populated mStatement if the
     // AsyncExecuteStatements got canceled or a failure occurred in constructing
     // the statement.