Bug 1166166 - Shrink storage cache off main thread r=mak
authorDoug Thayer <dothayer@mozilla.com>
Thu, 01 Jun 2017 14:46:53 -0700
changeset 415860 ec459f72f2ea2d7ed50f1935e1da3e8efe5c1185
parent 415859 1eb85e2b1467709e8cdce533f74135da718fa08d
child 415861 6d7315535184bc97ea41b5fa496b2b7acb0d005d
push id1517
push userjlorenzo@mozilla.com
push dateThu, 14 Sep 2017 16:50:54 +0000
treeherdermozilla-release@3b41fd564418 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1166166
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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,17 @@ public:
     , mClone(aClone)
     , mReadOnly(aReadOnly)
     , mCallback(aCallback)
   {
     MOZ_ASSERT(NS_IsMainThread());
   }
 
   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);
     }
     return Dispatch(NS_OK,
                     NS_ISUPPORTS_CAST(mozIStorageAsyncConnection*, mClone));
   }
 
@@ -577,17 +576,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 +935,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 +1217,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 +1327,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 +1678,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;
     }