Bug 1166166 - Shrink storage cache off main thread r?mak
authorDoug Thayer <dothayer@mozilla.com>
Thu, 01 Jun 2017 14:46:53 -0700
changeset 1160847 1eed5f6ba5dc1c4fcd028952cc859381e52d8a66
parent 1143409 94906c37940c6b1c371dc7c22ed2098face96d8b
child 1160848 7f05196c258ef6c88047ee288d2e9e2673111040
push id198613
push userdothayer@mozilla.com
push dateFri, 16 Jun 2017 15:45:07 +0000
treeherdertry@7f05196c258e [default view] [failures only]
reviewersmak
bugs1166166
milestone55.0a1
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
storage/StorageBaseStatementInternal.cpp
storage/mozStorageConnection.cpp
storage/mozStorageConnection.h
storage/mozStorageService.cpp
storage/mozStorageStatementData.h
--- a/storage/StorageBaseStatementInternal.cpp
+++ b/storage/StorageBaseStatementInternal.cpp
@@ -131,35 +131,37 @@ StorageBaseStatementInternal::asyncFinal
 }
 
 void
 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);
-
+  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);
-    if (isAsyncThread) {
-      (void)event->Run();
-    } else {
-      (void)target->Dispatch(event, NS_DISPATCH_NORMAL);
-    }
+    (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.
   mAsyncStatement = nullptr;
 }
 
 NS_IMETHODIMP
 StorageBaseStatementInternal::NewBindingParamsArray(
--- a/storage/mozStorageConnection.cpp
+++ b/storage/mozStorageConnection.cpp
@@ -451,18 +451,16 @@ public:
     , mClone(aClone)
     , mReadOnly(aReadOnly)
     , mCallback(aCallback)
   {
     MOZ_ASSERT(NS_IsMainThread());
   }
 
   NS_IMETHOD Run() override {
-    MOZ_ASSERT (NS_GetCurrentThread() == mConnection->getAsyncExecutionTarget());
-
     nsresult rv = mConnection->initializeClone(mClone, mReadOnly);
     if (NS_FAILED(rv)) {
       return Dispatch(rv, nullptr);
     }
     return Dispatch(NS_OK,
                     NS_ISUPPORTS_CAST(mozIStorageAsyncConnection*, mClone));
   }
 
@@ -577,17 +575,17 @@ Connection::getSqliteRuntimeStatus(int32
   if (aMaxValue)
     *aMaxValue = max;
   return curr;
 }
 
 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.
   if (mAsyncExecutionThreadShuttingDown)
     return nullptr;
 
   if (!mAsyncExecutionThread) {
     static nsThreadPoolNaming naming;
@@ -936,16 +934,23 @@ Connection::isClosing()
 
 bool
 Connection::isClosed()
 {
   MutexAutoLock lockedScope(sharedAsyncExecutionMutex);
   return mConnectionClosed;
 }
 
+bool
+Connection::isAsyncExecutionThreadAvailable()
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  return !!mAsyncExecutionThread;
+}
+
 void
 Connection::shutdownAsyncThread(nsIThread *aThread) {
   MOZ_ASSERT(!mAsyncExecutionThread);
   MOZ_ASSERT(mAsyncExecutionThreadIsAlive);
   MOZ_ASSERT(mAsyncExecutionThreadShuttingDown);
 
   DebugOnly<nsresult> rv = aThread->Shutdown();
   MOZ_ASSERT(NS_SUCCEEDED(rv));
@@ -1211,40 +1216,45 @@ Connection::GetInterface(const nsIID &aI
 //// mozIStorageConnection
 
 NS_IMETHODIMP
 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.
   sqlite3 *nativeConn = mDBConn;
   nsresult rv = setClosedState();
   NS_ENSURE_SUCCESS(rv, rv);
 
   return internalClose(nativeConn);
 }
 
 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
   // states mean and how we handle them:
   //
   // - (mDBConn && asyncThread): The expected case where we are either an
   //   async connection or a sync connection that has been used asynchronously.
   //   Either way the caller must call us and not Close().  Nothing surprising
@@ -1316,42 +1326,35 @@ Connection::AsyncClose(mozIStorageComple
 
   // setClosedState nullifies our connection pointer, so we take a raw pointer
   // off it, to pass it through the close procedure.
   sqlite3 *nativeConn = mDBConn;
   nsresult rv = setClosedState();
   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);
 
   return NS_OK;
 }
 
 NS_IMETHODIMP
 Connection::AsyncClone(bool aReadOnly,
                        mozIStorageCompletionCallback *aCallback)
 {
   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)
     return NS_ERROR_UNEXPECTED;
 
   int flags = mFlags;
   if (aReadOnly) {
     // Turn off SQLITE_OPEN_READWRITE, and set SQLITE_OPEN_READONLY.
@@ -1674,19 +1677,17 @@ Connection::ExecuteAsync(mozIStorageBase
                                          _handle);
 }
 
 NS_IMETHODIMP
 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));
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   nsCOMPtr<mozIStoragePendingStatement> pendingStatement;
--- a/storage/mozStorageConnection.h
+++ b/storage/mozStorageConnection.h
@@ -134,27 +134,28 @@ public:
     return mDBConn && static_cast<bool>(::sqlite3_get_autocommit(mDBConn));
   };
 
   /**
    * Lazily creates and returns a background execution thread.  In the future,
    * 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();
 
   /**
    * Mutex used by asynchronous statements to protect state.  The mutex is
    * declared on the connection object because there is no contention between
    * asynchronous statements (they are serialized on mAsyncExecutionThread).
    * Currently protects:
    *  - Connection.mAsyncExecutionThreadShuttingDown
-   *  - Connection.mAsyncExecutionThread
    *  - Connection.mConnectionClosed
    *  - AsyncExecuteStatements.mCancelRequested
    */
   Mutex sharedAsyncExecutionMutex;
 
   /**
    * Wraps the mutex that SQLite gives us from sqlite3_db_mutex.  This is public
    * because we already expose the sqlite3* native connection and proper
@@ -229,16 +230,24 @@ public:
 
   /**
    * True if the underlying connection is closed.
    * Any sqlite resources may be lost when this returns true, so nothing should
    * try to use them.
    */
   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:
   ~Connection();
   nsresult initializeInternal();
 
   /**
    * Sets the database into a closed state so no further actions can be
@@ -301,16 +310,18 @@ private:
    * default this will be the leaf of the path to the database file.
   */
   nsCString mTelemetryFilename;
 
   /**
    * 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;
 
   /**
    * Set to true by Close() or AsyncClose() prior to shutdown.
    *
    * If false, we guarantee both that the underlying sqlite3 database
    * connection is still open and that getAsyncExecutionTarget() can
--- a/storage/mozStorageService.cpp
+++ b/storage/mozStorageService.cpp
@@ -360,18 +360,24 @@ Service::minimizeMemory()
       // This is a mozIStorageAsyncConnection, it can only be used on the main
       // thread, so we can do a straight API call.
       nsCOMPtr<mozIStoragePendingStatement> ps;
       DebugOnly<nsresult> rv =
         conn->ExecuteSimpleSQLAsync(shrinkPragma, nullptr, getter_AddRefs(ps));
       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.
       nsCOMPtr<nsIRunnable> event =
         NewRunnableMethod<const nsCString>(
           conn, &Connection::ExecuteSimpleSQL, shrinkPragma);
       conn->threadOpenedOn->Dispatch(event, NS_DISPATCH_NORMAL);
     }
--- a/storage/mozStorageStatementData.h
+++ b/storage/mozStorageStatementData.h
@@ -73,29 +73,16 @@ public:
 
   /**
    * NULLs out our sqlite3_stmt (it is held by the owner) after reseting it and
    * clear all bindings to it.  This is expected to occur on the async thread.
    */
   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.
     if (mStatement) {
       (void)::sqlite3_reset(mStatement);
       (void)::sqlite3_clear_bindings(mStatement);
       mStatement = nullptr;
     }