574507 - IndexedDB: Fix transaction queue logic to prevent starving transactions across multiple objectStores

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch

We talked about this yesterday, see patch.

Comment 1

[Shawn Wilsher :sdwilsh]

This patch doesn't seem to apply on trunk.  Can you update it?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 453913 [details] [diff] [review]
Patch

>+CheckOverlapAndMergeObjectStores(nsTArray<nsString>& aLockedStores,
>+                                 const nsTArray<nsString>& aObjectStores,
>+                                 bool aShouldMerge,
>+                                 bool* aStoresOverlap)
>+{
>+  PRUint32 length = aObjectStores.Length();
>+  if (!length) {
>+    *aStoresOverlap = false;
>+    return NS_OK;
>+  }
>+
>+  PRUint32 lockedLength = aLockedStores.Length();
>+  if (!lockedLength) {
>+    if (!aLockedStores.AppendElements(aObjectStores)) {
>+      NS_WARNING("Out of memory");
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    }
>+
>+    *aStoresOverlap = false;
>+    return NS_OK;
>+  }

You can get rid of all of the above code (except getting the lengths). The below code should and does handle those two edge cases just fine.

>+  nsAutoTArray<nsString, 20> storesToAdd;
>+  if (aShouldMerge && !storesToAdd.SetCapacity(length)) {
>+    NS_WARNING("Out of memory");
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }
>+
>+  bool overlap = false;
>+
>+  for (PRUint32 index = 0; index < length; index++) {
>+    const nsString& storeName = aObjectStores[index];
>+    if (aLockedStores.Contains(storeName)) {
>+      overlap = true;
>+    }
>+    else if (aShouldMerge) {
>+      storesToAdd.AppendElement(storeName);
>+    }
>+  }

Rather than adding to storesToAdd, why not add to aLockedStores directly?

>@@ -240,27 +279,34 @@ TransactionThreadPool::FinishTransaction
>                  "Didn't find the transaction we were looking for!");
>   }
> 
>+  // If this was a write transaction then we need to rebuild the locked object
>+  // store list.
>+  if (aTransaction->mMode != nsIIDBTransaction::READ_ONLY &&
>+      count > 1) {
>+    dbTransactionInfo->lockedObjectStores.Clear();
>+  }

I think you need to do this on all transactions. Once a read transaction finishes, a write transaction might now be schedule-able.

>+nsresult
> TransactionThreadPool::TransactionCanRun(IDBTransactionRequest* aTransaction,
>-                                         TransactionQueue** aQueue)
>+                                         bool* aCanRun,
>+                                         TransactionQueue** aExistingQueue)
> {
>   NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>   NS_ASSERTION(aTransaction, "Null pointer!");
>-  NS_ASSERTION(aQueue, "Null pointer!");
>+  NS_ASSERTION(aCanRun, "Null pointer!");
>+  NS_ASSERTION(aExistingQueue, "Null pointer!");
> 
>   const PRUint32 databaseId = aTransaction->mDatabase->Id();
>   const nsTArray<nsString>& objectStoreNames = aTransaction->mObjectStoreNames;
>@@ -270,123 +316,67 @@ TransactionThreadPool::TransactionCanRun
>   DatabaseTransactionInfo* dbTransactionInfo;
>   if (!mTransactionsInProgress.Get(databaseId, &dbTransactionInfo)) {
>     // First transaction for this database, fine to run.
>-    *aQueue = nsnull;
>-    return true;
>+    *aCanRun = true;
>+    *aExistingQueue = nsnull;
>+    return NS_OK;
>   }
> 
>   nsTArray<TransactionInfo>& transactionsInProgress =
>     dbTransactionInfo->transactions;
> 
>   PRUint32 transactionCount = transactionsInProgress.Length();
>+  NS_ASSERTION(transactionCount, "Should never be 0!");
> 
>   if (mode == IDBTransactionRequest::FULL_LOCK) {
>-    switch (transactionCount) {
>-      case 0: {
>-        *aQueue = nsnull;
>-        return true;
>+    if (transactionCount == 1) {
>+      const TransactionInfo& info = transactionsInProgress[0];
>+      if (info.transaction == aTransaction) {
>+        *aCanRun = true;
>+        *aExistingQueue = info.queue;
>+        return NS_OK;
>       }
>+    }

Doesn't the for-loop below cover this case? In other words, wouldn't simply always checking that if aTransaction is in transactionsInProgress then you can always simply return true. All you need is

if (mode == IDBTransactionRequest::FULL_LOCK) {
  dbTransactionInfo->lockPending = true;
}

>+  nsTArray<nsString>& lockedObjectStoreNames =
>+    dbTransactionInfo->lockedObjectStores;
>+
>+  bool shouldMerge = mode == nsIIDBTransaction::READ_WRITE;
>+
>+  bool overlap;
>+  nsresult rv = CheckOverlapAndMergeObjectStores(lockedObjectStoreNames,
>+                                                 objectStoreNames, shouldMerge,
>+                                                 &overlap);

You always need to set shouldMerge to true, no? Even read transactions need to block down-the-line write transactions.

Though really I think you need to track both which tables are "readlocked" and which ones are "writelocked". Consider the following code:

trans1 = db.transaction(["foo"], READ_ONLY);
trans1.objectStore("foo").get(...);

trans2 = db.transaction(["bar"], READ_WRITE);
trans2.objectStore("bar").get(...);

trans3 = db.transaction([X], READ_ONLY);
trans3.objectStore(X).get(...);

When the third transaction is started, both "foo" and "bar" are locked. However if X == "foo", then the third transaction can be started immediately, but if X == "bar", then the third transaction will have to wait until "bar" is unlocked.

r- based on that.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Patch, v2

Fixed the issues you pointed out.

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Attachment: Properly rebuild locked objectStore lists

Jonas! You let a bug slip through! ;-)

This was causing tinderbox test failures. Interdiff attached.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

http://hg.mozilla.org/mozilla-central/rev/fe58a89b685d