IndexedDB 2.0: The client's transaction operation queue should flush as much to the...
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Nov 2016 21:08:12 +0000 (21:08 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 29 Nov 2016 21:08:12 +0000 (21:08 +0000)
https://bugs.webkit.org/show_bug.cgi?id=164932

Reviewed by Alex Christensen.

No new tests (No new test necessary, covered extensively by all existing tests).

Profiles showed that on tests with lots of rapid IDBRequests in a row, both the main thread and database
threads were largely idle.

The explanation was simple. Currently the client IDBTransaction queues up operations and only vends them out
to the server 1 at a time, waiting for the previous operation to complete.

While some operations do need to wait for the server to reply, by making the change to send most operations
(all operations with an associated IDBRequest) to the server without waiting we get rid of most of the idleness.

It is possible we can find a few other types of operations to send without waiting, but we haven't yet seen any
test case where they would show up on profiles.

Sending more than one operation at a time was actually a very small part of this change.
As many "edge case" regression tests revealed, we also needed to start having IDBTransaction track all of their
"in progress" operations such that they could be aborted on the client side in exceptional circumstances.

* Modules/indexeddb/IDBTransaction.cpp:
(WebCore::IDBTransaction::abortInProgressOperations): Abort's all in-progress operations (ones that have already
  been sent to the server)
(WebCore::IDBTransaction::abortOnServerAndCancelRequests): Abort in-progress operations before pending ones.
(WebCore::IDBTransaction::operationTimerFired): If we just started an operation with an associated IDBRequest,
  schedule the timer to send another one right away.
(WebCore::IDBTransaction::operationDidComplete):
(WebCore::IDBTransaction::connectionClosedFromServer): Abort in-progress operations before pending ones.
* Modules/indexeddb/IDBTransaction.h:

* Modules/indexeddb/client/TransactionOperation.cpp:
(WebCore::IDBClient::TransactionOperation::TransactionOperation):
* Modules/indexeddb/client/TransactionOperation.h:
(WebCore::IDBClient::TransactionOperation::completed):
(WebCore::IDBClient::TransactionOperation::hasIDBRequest):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@209086 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/Modules/indexeddb/IDBTransaction.cpp
Source/WebCore/Modules/indexeddb/IDBTransaction.h
Source/WebCore/Modules/indexeddb/client/TransactionOperation.h

index a705f05..71e7faa 100644 (file)
@@ -1,3 +1,44 @@
+2016-11-29  Brady Eidson  <beidson@apple.com>
+
+        IndexedDB 2.0: The client's transaction operation queue should flush as much to the server as possible.
+        https://bugs.webkit.org/show_bug.cgi?id=164932
+
+        Reviewed by Alex Christensen.
+
+        No new tests (No new test necessary, covered extensively by all existing tests).
+
+        Profiles showed that on tests with lots of rapid IDBRequests in a row, both the main thread and database 
+        threads were largely idle.
+
+        The explanation was simple. Currently the client IDBTransaction queues up operations and only vends them out 
+        to the server 1 at a time, waiting for the previous operation to complete.
+
+        While some operations do need to wait for the server to reply, by making the change to send most operations 
+        (all operations with an associated IDBRequest) to the server without waiting we get rid of most of the idleness.
+
+        It is possible we can find a few other types of operations to send without waiting, but we haven't yet seen any
+        test case where they would show up on profiles.
+
+        Sending more than one operation at a time was actually a very small part of this change.
+        As many "edge case" regression tests revealed, we also needed to start having IDBTransaction track all of their
+        "in progress" operations such that they could be aborted on the client side in exceptional circumstances.
+
+        * Modules/indexeddb/IDBTransaction.cpp:
+        (WebCore::IDBTransaction::abortInProgressOperations): Abort's all in-progress operations (ones that have already
+          been sent to the server)
+        (WebCore::IDBTransaction::abortOnServerAndCancelRequests): Abort in-progress operations before pending ones.
+        (WebCore::IDBTransaction::operationTimerFired): If we just started an operation with an associated IDBRequest,
+          schedule the timer to send another one right away.
+        (WebCore::IDBTransaction::operationDidComplete):
+        (WebCore::IDBTransaction::connectionClosedFromServer): Abort in-progress operations before pending ones.
+        * Modules/indexeddb/IDBTransaction.h:
+
+        * Modules/indexeddb/client/TransactionOperation.cpp:
+        (WebCore::IDBClient::TransactionOperation::TransactionOperation):
+        * Modules/indexeddb/client/TransactionOperation.h:
+        (WebCore::IDBClient::TransactionOperation::completed):
+        (WebCore::IDBClient::TransactionOperation::hasIDBRequest):
+
 2016-11-29  Dave Hyatt  <hyatt@apple.com>
 
         [CSS Parser] Fix ::cue parsing
index 00c9131..8704544 100644 (file)
@@ -251,6 +251,34 @@ void IDBTransaction::internalAbort()
     scheduleOperation(IDBClient::createTransactionOperation(*this, nullptr, &IDBTransaction::abortOnServerAndCancelRequests));
 }
 
+void IDBTransaction::abortInProgressOperations(const IDBError& error)
+{
+    LOG(IndexedDB, "IDBTransaction::abortInProgressOperations");
+
+    Vector<RefPtr<IDBClient::TransactionOperation>> inProgressAbortVector;
+    inProgressAbortVector.reserveInitialCapacity(m_transactionOperationsInProgressQueue.size());
+    while (!m_transactionOperationsInProgressQueue.isEmpty())
+        inProgressAbortVector.uncheckedAppend(m_transactionOperationsInProgressQueue.takeFirst());
+
+    for (auto& operation : inProgressAbortVector) {
+        m_transactionOperationsInProgressQueue.append(operation.get());
+        m_currentlyCompletingRequest = nullptr;
+        operation->doComplete(IDBResultData::error(operation->identifier(), error));
+    }
+
+    Vector<RefPtr<IDBClient::TransactionOperation>> completedOnServerAbortVector;
+    completedOnServerAbortVector.reserveInitialCapacity(m_completedOnServerQueue.size());
+    while (!m_completedOnServerQueue.isEmpty())
+        completedOnServerAbortVector.uncheckedAppend(m_completedOnServerQueue.takeFirst().first);
+
+    for (auto& operation : completedOnServerAbortVector) {
+        m_currentlyCompletingRequest = nullptr;
+        operation->doComplete(IDBResultData::error(operation->identifier(), error));
+    }
+
+    connectionProxy().forgetActiveOperations(inProgressAbortVector);
+}
+
 void IDBTransaction::abortOnServerAndCancelRequests(IDBClient::TransactionOperation& operation)
 {
     LOG(IndexedDB, "IDBTransaction::abortOnServerAndCancelRequests");
@@ -260,13 +288,19 @@ void IDBTransaction::abortOnServerAndCancelRequests(IDBClient::TransactionOperat
     m_database->connectionProxy().abortTransaction(*this);
 
     ASSERT(m_transactionOperationMap.contains(operation.identifier()));
+    ASSERT(m_transactionOperationsInProgressQueue.last() == &operation);
     m_transactionOperationMap.remove(operation.identifier());
+    m_transactionOperationsInProgressQueue.removeLast();
 
     m_currentlyCompletingRequest = nullptr;
     
     IDBError error(IDBDatabaseException::AbortError);
+
+    abortInProgressOperations(error);
+
     for (auto& operation : m_abortQueue) {
         m_currentlyCompletingRequest = nullptr;
+        m_transactionOperationsInProgressQueue.append(operation.get());
         operation->doComplete(IDBResultData::error(operation->identifier(), error));
     }
 
@@ -367,10 +401,21 @@ void IDBTransaction::pendingOperationTimerFired()
     if (!m_startedOnServer)
         return;
 
+    // If the last in-progress operation we've sent to the server is not an IDBRequest operation,
+    // then we have to wait until it completes before sending any more.
+    if (!m_transactionOperationsInProgressQueue.isEmpty() && !m_transactionOperationsInProgressQueue.last()->nextRequestCanGoToServer())
+        return;
+
     if (!m_pendingTransactionOperationQueue.isEmpty()) {
         auto operation = m_pendingTransactionOperationQueue.takeFirst();
+        m_transactionOperationsInProgressQueue.append(operation.get());
         operation->perform();
 
+        // If this operation we just started has an associated IDBRequest, we might be able to send
+        // another operation to the server before it completes.
+        if (operation->idbRequest() && !m_pendingTransactionOperationQueue.isEmpty())
+            schedulePendingOperationTimer();
+
         return;
     }
 
@@ -1147,6 +1192,10 @@ void IDBTransaction::putOrAddOnServer(IDBClient::TransactionOperation& operation
         return;
     }
 
+    // Since this request won't actually go to the server until the blob writes are complete,
+    // stop future requests from going to the server ahead of it.
+    operation.setNextRequestCanGoToServer(false);
+
     value->writeBlobsToDiskForIndexedDB([protectedThis = makeRef(*this), this, protectedOperation = Ref<IDBClient::TransactionOperation>(operation), keyData = IDBKeyData(key.get()).isolatedCopy(), overwriteMode](const IDBValue& idbValue) mutable {
         ASSERT(currentThread() == originThreadID());
         ASSERT(isMainThread());
@@ -1238,11 +1287,13 @@ void IDBTransaction::operationCompletedOnClient(IDBClient::TransactionOperation&
 {
     LOG(IndexedDB, "IDBTransaction::operationCompletedOnClient");
 
-    ASSERT(m_transactionOperationMap.get(operation.identifier()) == &operation);
     ASSERT(currentThread() == m_database->originThreadID());
     ASSERT(currentThread() == operation.originThreadID());
+    ASSERT(m_transactionOperationMap.get(operation.identifier()) == &operation);
+    ASSERT(m_transactionOperationsInProgressQueue.first() == &operation);
 
     m_transactionOperationMap.remove(operation.identifier());
+    m_transactionOperationsInProgressQueue.removeFirst();
 
     schedulePendingOperationTimer();
 }
@@ -1281,11 +1332,15 @@ void IDBTransaction::connectionClosedFromServer(const IDBError& error)
 
     m_state = IndexedDB::TransactionState::Aborting;
 
+    abortInProgressOperations(error);
+
     Vector<RefPtr<IDBClient::TransactionOperation>> operations;
     copyValuesToVector(m_transactionOperationMap, operations);
 
     for (auto& operation : operations) {
         m_currentlyCompletingRequest = nullptr;
+        m_transactionOperationsInProgressQueue.append(operation.get());
+        ASSERT(m_transactionOperationsInProgressQueue.first() == operation.get());
         operation->doComplete(IDBResultData::error(operation->identifier(), error));
     }
 
index df223e1..af96c4c 100644 (file)
@@ -160,6 +160,7 @@ private:
     void internalAbort();
     void notifyDidAbort(const IDBError&);
     void finishAbortOrCommit();
+    void abortInProgressOperations(const IDBError&);
 
     void scheduleOperation(RefPtr<IDBClient::TransactionOperation>&&);
     void pendingOperationTimerFired();
@@ -243,8 +244,10 @@ private:
     RefPtr<IDBOpenDBRequest> m_openDBRequest;
 
     Deque<RefPtr<IDBClient::TransactionOperation>> m_pendingTransactionOperationQueue;
+    Deque<IDBClient::TransactionOperation*> m_transactionOperationsInProgressQueue;
     Deque<std::pair<RefPtr<IDBClient::TransactionOperation>, IDBResultData>> m_completedOnServerQueue;
     Deque<RefPtr<IDBClient::TransactionOperation>> m_abortQueue;
+
     HashMap<IDBResourceIdentifier, RefPtr<IDBClient::TransactionOperation>> m_transactionOperationMap;
 
     mutable Lock m_referencedObjectStoreLock;
index 71a1eba..cd95bef 100644 (file)
@@ -83,7 +83,13 @@ public:
     void doComplete(const IDBResultData& data)
     {
         ASSERT(m_originThreadID == currentThread());
-        ASSERT(m_completeFunction);
+
+        // Due to race conditions between the server sending an "operation complete" message and the client
+        // forcefully aborting an operation, it's unavoidable that this method might be called twice.
+        // It's okay to handle that gracefully with an early return.
+        if (!m_completeFunction)
+            return;
+
         m_completeFunction(data);
         m_transaction->operationCompletedOnClient(*this);
 
@@ -99,6 +105,9 @@ public:
 
     IDBRequest* idbRequest() { return m_idbRequest.get(); }
 
+    bool nextRequestCanGoToServer() const { return m_nextRequestCanGoToServer && m_idbRequest; }
+    void setNextRequestCanGoToServer(bool nextRequestCanGoToServer) { m_nextRequestCanGoToServer = nextRequestCanGoToServer; }
+
 protected:
     TransactionOperation(IDBTransaction& transaction)
         : m_transaction(transaction)
@@ -127,6 +136,7 @@ private:
 
     ThreadIdentifier m_originThreadID { currentThread() };
     RefPtr<IDBRequest> m_idbRequest;
+    bool m_nextRequestCanGoToServer { true };
 };
 
 template <typename... Arguments>