From d65c88d3f08c28777899b26637acc510ecbfd11b Mon Sep 17 00:00:00 2001 From: Doug Thayer Date: Thu, 1 Jun 2017 14:46:53 -0700 Subject: [PATCH] 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 --- storage/StorageBaseStatementInternal.cpp | 32 +++++++------ storage/mozStorageConnection.cpp | 60 ++++++++++++------------ storage/mozStorageConnection.h | 13 ++++- storage/mozStorageService.cpp | 10 +++- storage/mozStorageStatementData.h | 13 ----- 5 files changed, 68 insertions(+), 60 deletions(-) diff --git a/storage/StorageBaseStatementInternal.cpp b/storage/StorageBaseStatementInternal.cpp index d6545fcb4d26..626731595ca8 100644 --- 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 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 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 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 index d89963376b68..241b977732a2 100644 --- a/storage/mozStorageConnection.cpp +++ b/storage/mozStorageConnection.cpp @@ -456,8 +456,7 @@ public: } 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 closeEvent; - { - // We need to lock because we're modifying mAsyncExecutionThread - MutexAutoLock lockedScope(sharedAsyncExecutionMutex); - closeEvent = new AsyncCloseConnection(this, - nativeConn, - completeEvent, - mAsyncExecutionThread.forget()); - } + nsCOMPtr 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 stmt; nsresult rv = CreateAsyncStatement(aSQLStatement, getter_AddRefs(stmt)); diff --git a/storage/mozStorageConnection.h b/storage/mozStorageConnection.h index 7b96c411687f..282fd020d99a 100644 --- a/storage/mozStorageConnection.h +++ b/storage/mozStorageConnection.h @@ -139,6 +139,8 @@ public: * 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 @@ public: * asynchronous statements (they are serialized on mAsyncExecutionThread). * Currently protects: * - Connection.mAsyncExecutionThreadShuttingDown - * - Connection.mAsyncExecutionThread * - Connection.mConnectionClosed * - AsyncExecuteStatements.mCancelRequested */ @@ -234,6 +235,14 @@ public: */ 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 @@ private: * 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 mAsyncExecutionThread; diff --git a/storage/mozStorageService.cpp b/storage/mozStorageService.cpp index 69478a2a227f..741d800bd54e 100644 --- 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 ps; + DebugOnly 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 index 8baaf2fa73ac..b6c40615ae0a 100644 --- a/storage/mozStorageStatementData.h +++ b/storage/mozStorageStatementData.h @@ -78,19 +78,6 @@ public: inline void reset() { NS_PRECONDITION(mStatementOwner, "Must have a statement owner!"); -#ifdef DEBUG - { - nsCOMPtr 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.