Bug 488148 - Share the mutex used by AsyncExecuteStatements on Connection

Greatly reduces the number of mutexes used when using the asynchronous storage
API.
r=bent
r=asuth

storage/src/mozStorageAsyncStatementExecution.cpp

@@ -37,7 +37,6 @@
  *
  * ***** END LICENSE BLOCK ***** */
 
-#include "nsAutoLock.h"
 #include "nsAutoPtr.h"
 #include "prtime.h"
 
@@ -183,13 +182,10 @@ AsyncExecuteStatements::execute(sqlite3_stmt_array &aStatements,
     new AsyncExecuteStatements(aStatements, aConnection, aCallback);
   NS_ENSURE_TRUE(event, NS_ERROR_OUT_OF_MEMORY);
 
-  nsresult rv = event->initialize();
-  NS_ENSURE_SUCCESS(rv, rv);
-
   // Dispatch it to the background
   nsCOMPtr<nsIEventTarget> target(aConnection->getAsyncExecutionTarget());
   NS_ENSURE_TRUE(target, NS_ERROR_NOT_AVAILABLE);
-  rv = target->Dispatch(event, NS_DISPATCH_NORMAL);
+  nsresult rv = target->Dispatch(event, NS_DISPATCH_NORMAL);
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Return it as the pending statement object
@@ -198,7 +194,7 @@ AsyncExecuteStatements::execute(sqlite3_stmt_array &aStatements,
 }
 
 AsyncExecuteStatements::AsyncExecuteStatements(sqlite3_stmt_array &aStatements,
-                                               mozIStorageConnection *aConnection,
+                                               Connection *aConnection,
                                                mozIStorageStatementCallback *aCallback)
 : mConnection(aConnection)
 , mTransactionManager(nsnull)
@@ -208,23 +204,11 @@ AsyncExecuteStatements::AsyncExecuteStatements(sqlite3_stmt_array &aStatements,
 , mIntervalStart(::PR_IntervalNow())
 , mState(PENDING)
 , mCancelRequested(PR_FALSE)
-, mLock(nsAutoLock::NewLock("AsyncExecuteStatements::mLock"))
+, mMutex(aConnection->sharedAsyncExecutionMutex)
 {
   (void)mStatements.SwapElements(aStatements);
   NS_ASSERTION(mStatements.Length(), "We weren't given any statements!");
-}
-
-AsyncExecuteStatements::~AsyncExecuteStatements()
-{
-  nsAutoLock::DestroyLock(mLock);
-}
-
-nsresult
-AsyncExecuteStatements::initialize()
-{
-  NS_ENSURE_TRUE(mLock, NS_ERROR_OUT_OF_MEMORY);
   NS_IF_ADDREF(mCallback);
-  return NS_OK;
 }
 
 bool
@@ -236,7 +220,7 @@ AsyncExecuteStatements::shouldNotify()
   NS_ASSERTION(onCallingThread, "runEvent not running on the calling thread!");
 #endif
 
-  // We do not need to acquire mLock here because it can only ever be written
+  // We do not need to acquire mMutex here because it can only ever be written
   // to on the calling thread, and the only thread that can call us is the
   // calling thread, so we know that our access is serialized.
   return !mCancelRequested;
@@ -246,72 +230,41 @@ bool
 AsyncExecuteStatements::executeAndProcessStatement(sqlite3_stmt *aStatement,
                                                    bool aLastStatement)
 {
+  mMutex.AssertNotCurrentThreadOwns();
+
   // We need to hold the mutex for statement execution so we can properly
   // reflect state in case we are canceled.  We release the mutex in a few areas
   // in order to allow for cancelation to occur.
-  nsAutoLock mutex(mLock);
+  MutexAutoLock lockedScope(mMutex);
 
-  nsresult rv = NS_OK;
-  while (true) {
-    int rc = ::sqlite3_step(aStatement);
-    // Break out if we have no more results
-    if (rc == SQLITE_DONE)
-      break;
+  // Execute our statement
+  bool hasResults = executeStatement(aStatement);
 
-    // Some errors are not fatal, and we can handle them and continue.
-    if (rc != SQLITE_OK && rc != SQLITE_ROW) {
-      if (rc == SQLITE_BUSY) {
-        // We do not want to hold our mutex while we yield.
-        nsAutoUnlock cancelationScope(mLock);
+  // If we had an error, bail.
+  if (mState == ERROR)
+    return false;
 
-        // Yield, and try again
-        (void)::PR_Sleep(PR_INTERVAL_NO_WAIT);
-        continue;
-      }
-
-      // Set error state
-      mState = ERROR;
-
-      // Drop our mutex - notifyError doesn't want it held
-      mutex.unlock();
-
-      // Notify
-      sqlite3 *db = ::sqlite3_db_handle(aStatement);
-      (void)notifyError(rc, ::sqlite3_errmsg(db));
-
-      // And stop processing statements
-      return false;
-    }
-
-    // If we do not have a callback, there's no point in executing this
-    // statement anymore, but we wish to continue to execute statements.  We
-    // also need to update our state if we are finished, so break out of the
-    // while loop.
-    if (!mCallback)
-      break;
-
-    // If we have been canceled, there is no point in going on...
-    if (mCancelRequested) {
-      mState = CANCELED;
-      return false;
-    }
-
-    // Build our results and notify if it's time.
-    rv = buildAndNotifyResults(aStatement);
-    if (NS_FAILED(rv))
-      break;
+  // If we have been canceled, there is no point in going on...
+  if (mCancelRequested) {
+    mState = CANCELED;
+    return false;
   }
 
-  // If we have an error that we have not already notified about, set our
-  // state accordingly, and notify.
-  if (NS_FAILED(rv)) {
+  // Build our result set and notify if we got anything back and have a
+  // callback to notify.
+  if (mCallback && hasResults && NS_FAILED(buildAndNotifyResults(aStatement))) {
+    // We had an error notifying, so we notify on error and stop processing.
     mState = ERROR;
 
-    // Drop our mutex - notifyError doesn't want it held
-    mutex.unlock();
+    {
+      // Drop our mutex because notifyError doesn't want it held.
+      MutexAutoUnlock unlockedScope(mMutex);
+
+      // Notify, and stop processing statements.
+      (void)notifyError(mozIStorageError::ERROR,
+                        "An error occurred while notifying about results");
+    }
 
-    // Notify, and stop processing statements.
-    (void)notifyError(mozIStorageError::ERROR, "");
     return false;
   }
 
@@ -329,16 +282,58 @@ AsyncExecuteStatements::executeAndProcessStatement(sqlite3_stmt *aStatement,
   return true;
 }
 
+bool
+AsyncExecuteStatements::executeStatement(sqlite3_stmt *aStatement)
+{
+  mMutex.AssertCurrentThreadOwns();
+
+  while (true) {
+    int rc = ::sqlite3_step(aStatement);
+    // Stop if we have no more results.
+    if (rc == SQLITE_DONE)
+      return false;
+
+    // If we got results, we can return now.
+    if (rc == SQLITE_ROW)
+      return true;
+
+    // Some errors are not fatal, and we can handle them and continue.
+    if (rc == SQLITE_BUSY) {
+      // We do not want to hold our mutex while we yield.
+      MutexAutoUnlock cancelationScope(mMutex);
+
+      // Yield, and try again
+      (void)::PR_Sleep(PR_INTERVAL_NO_WAIT);
+      continue;
+    }
+
+    // Set an error state.
+    mState = ERROR;
+
+    {
+      // Drop our mutex because notifyError doesn't want it held.
+      MutexAutoUnlock unlockedScope(mMutex);
+
+      // And notify.
+      sqlite3 *db = ::sqlite3_db_handle(aStatement);
+      (void)notifyError(rc, ::sqlite3_errmsg(db));
+    }
+
+    // Finally, indicate that we should stop processing.
+    return false;
+  }
+}
+
 nsresult
 AsyncExecuteStatements::buildAndNotifyResults(sqlite3_stmt *aStatement)
 {
   NS_ASSERTION(mCallback, "Trying to dispatch results without a callback!");
-  PR_ASSERT_CURRENT_THREAD_OWNS_LOCK(mLock);
+  mMutex.AssertCurrentThreadOwns();
 
   // At this point, it is safe to not hold the mutex and allow for cancelation.
   // We may add an event to the calling thread, but that thread will not end
   // up running when it checks back with us to see if it should run.
-  nsAutoUnlock cancelationScope(mLock);
+  MutexAutoUnlock cancelationScope(mMutex);
 
   // Build result object if we need it.
   if (!mResultSet)
@@ -375,6 +370,7 @@ AsyncExecuteStatements::buildAndNotifyResults(sqlite3_stmt *aStatement)
 nsresult
 AsyncExecuteStatements::notifyComplete()
 {
+  mMutex.AssertNotCurrentThreadOwns();
   NS_ASSERTION(mState != PENDING,
                "Still in a pending state when calling Complete!");
 
@@ -422,6 +418,8 @@ nsresult
 AsyncExecuteStatements::notifyError(PRInt32 aErrorCode,
                                     const char *aMessage)
 {
+  mMutex.AssertNotCurrentThreadOwns();
+
   if (!mCallback)
     return NS_OK;
 
@@ -438,6 +436,7 @@ AsyncExecuteStatements::notifyError(PRInt32 aErrorCode,
 nsresult
 AsyncExecuteStatements::notifyResults()
 {
+  mMutex.AssertNotCurrentThreadOwns();
   NS_ASSERTION(mCallback, "notifyResults called without a callback!");
 
   nsRefPtr<CallbackResultNotifier> notifier =
@@ -473,7 +472,7 @@ AsyncExecuteStatements::Cancel(PRBool *_successful)
   NS_ENSURE_FALSE(mCancelRequested, NS_ERROR_UNEXPECTED);
 
   {
-    nsAutoLock mutex(mLock);
+    MutexAutoLock lockedScope(mMutex);
 
     // We need to indicate that we want to try and cancel now.
     mCancelRequested = true;
@@ -497,15 +496,16 @@ AsyncExecuteStatements::Cancel(PRBool *_successful)
 NS_IMETHODIMP
 AsyncExecuteStatements::Run()
 {
-  // do not run if we have been canceled
+  // Do not run if we have been canceled.
+  bool cancelRequested;
   {
-    nsAutoLock mutex(mLock);
-    if (mCancelRequested) {
+    MutexAutoLock lockedScope(mMutex);
+    cancelRequested = mCancelRequested;
+    if (cancelRequested)
       mState = CANCELED;
-      mutex.unlock();
-      return notifyComplete();
-    }
   }
+  if (cancelRequested)
+    return notifyComplete();
 
   // If there is more than one statement, run it in a transaction.  We assume
   // that we have been given write statements since getting a batch of read

storage/src/mozStorageAsyncStatementExecution.h

@@ -44,6 +44,7 @@
 #include "nsTArray.h"
 #include "nsAutoPtr.h"
 #include "nsThreadUtils.h"
+#include "mozilla/Mutex.h"
 
 #include "mozIStoragePendingStatement.h"
 #include "mozIStorageStatementCallback.h"
@@ -107,21 +108,14 @@ public:
 
 private:
   AsyncExecuteStatements(sqlite3_stmt_array &aStatements,
-                         mozIStorageConnection *aConnection,
+                         Connection *aConnection,
                          mozIStorageStatementCallback *aCallback);
 
-  /**
-   * Initializes the object so it can be run on the background thread.
-   */
-  nsresult initialize();
-
-  ~AsyncExecuteStatements();
-
   /**
    * Executes a given statement until completion, an error occurs, or we are
    * canceled.  If aLastStatement is true, we should set mState accordingly.
    *
-   * @pre mLock is not held
+   * @pre mMutex is not held
    *
    * @param aStatement
    *        The statement to execute and then process.
@@ -133,11 +127,22 @@ private:
   bool executeAndProcessStatement(sqlite3_stmt *aStatement,
                                   bool aLastStatement);
 
+  /**
+   * Executes one step of a statement, properly handling any error conditions.
+   *
+   * @pre mMutex is held
+   *
+   * @param aStatement
+   *        The statement to execute a step on.
+   * @returns true if results were obtained, false otherwise.
+   */
+  bool executeStatement(sqlite3_stmt *aStatement);
+
   /**
    * Builds a result set up with a row from a given statement.  If we meet the
    * right criteria, go ahead and notify about this results too.
    *
-   * @pre mLock is held
+   * @pre mMutex is held
    *
    * @param aStatement
    *        The statement to get the row data from.
@@ -147,14 +152,14 @@ private:
   /**
    * Notifies callback about completion, and does any necessary cleanup.
    *
-   * @pre mLock is not held
+   * @pre mMutex is not held
    */
   nsresult notifyComplete();
 
   /**
    * Notifies callback about an error.
    *
-   * @pre mLock is not held
+   * @pre mMutex is not held
    *
    * @param aErrorCode
    *        The error code defined in mozIStorageError for the error.
@@ -166,12 +171,12 @@ private:
   /**
    * Notifies the callback about a result set.
    *
-   * @pre mLock is not held
+   * @pre mMutex is not held
    */
   nsresult notifyResults();
 
   sqlite3_stmt_array mStatements;
-  mozIStorageConnection *mConnection;
+  nsRefPtr<Connection> mConnection;
   mozStorageTransaction *mTransactionManager;
   mozIStorageStatementCallback *mCallback;
   nsCOMPtr<nsIThread> mCallingThread;
@@ -206,7 +211,7 @@ private:
    *     held.  It is always read from within the lock on the background thread,
    *     but not on the calling thread (see shouldNotify for why).
    */
-  PRLock *mLock;
+  Mutex &mMutex;
 };
 
 } // namespace storage

storage/src/mozStorageConnection.cpp

@@ -50,6 +50,7 @@
 #include "nsIPrefService.h"
 #include "nsIPrefBranch.h"
 #include "nsThreadUtils.h"
+#include "nsAutoLock.h"
 
 #include "mozIStorageAggregateFunction.h"
 #include "mozIStorageFunction.h"
@@ -241,7 +242,8 @@ sqlite3_T_blob(sqlite3_context *aCtx,
 //// Connection
 
 Connection::Connection(mozIStorageService *aService)
-: mDBConn(nsnull)
+: sharedAsyncExecutionMutex("Connection::sharedAsyncExecutionMutex")
+, mDBConn(nsnull)
 , mAsyncExecutionMutex(nsAutoLock::NewLock("AsyncExecutionMutex"))
 , mAsyncExecutionThreadShuttingDown(PR_FALSE)
 , mTransactionMutex(nsAutoLock::NewLock("TransactionMutex"))

storage/src/mozStorageConnection.h

@@ -42,7 +42,7 @@
 #define _MOZSTORAGECONNECTION_H_
 
 #include "nsCOMPtr.h"
-#include "nsAutoLock.h"
+#include "mozilla/Mutex.h"
 
 #include "nsString.h"
 #include "nsInterfaceHashtable.h"
@@ -53,6 +53,7 @@
 
 #include <sqlite3.h>
 
+struct PRLock;
 class nsIFile;
 class nsIEventTarget;
 class nsIThread;
@@ -86,11 +87,18 @@ public:
    * 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.
-   * 
+   *
    * @returns an event target suitable for asynchronous statement execution.
    */
   already_AddRefed<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).
+   */
+  Mutex sharedAsyncExecutionMutex;
+
 private:
   ~Connection();
 
@@ -159,8 +167,7 @@ private:
   nsCOMPtr<mozIStorageProgressHandler> mProgressHandler;
 
   // This isn't accessed but is used to make sure that the connections do
-  // not outlive the service. The service, for example, owns certain locks
-  // in mozStorageAsyncIO file that the connections depend on.
+  // not outlive the service.
   nsCOMPtr<mozIStorageService> mStorageService;
 };