If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Lots of indexeddb transactions bring FF to its knees

RESOLVED FIXED in mozilla21

Status

()

Core
DOM: IndexedDB
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jrmuizel, Assigned: khuey)

Tracking

({perf})

Trunk
mozilla21
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
The following page http://people.mozilla.com/~jmuizelaar/indexeddb/full-text-search/fts.html creates a bunch of transactions when you click populate. This causes FF to basically get in a loop churning through these transactions, adding and removing them from an array. This continues long after closing the page.

Chrome handles the same task with relative ease completing the whole import in about 60 seconds. FF never finishes.
Profile against Mozilla/5.0 (Windows NT 6.1; WOW64; rv:17.0) Gecko/17.0 Firefox/17.0 ID:20120728030524: http://people.mozilla.com/~bgirard/cleopatra/?report=e96c9da3a114b7965d348453b4fca6f9cbe7d326
Keywords: perf
OS: Mac OS X → All
Version: unspecified → Trunk
We debugged this at the work week in Toronto.  The problem is that FinishTransactionRunnable relies on redispatching all the runnables to maintain the proper ordering.  This ends up destroying, reallocating, reallocing, etc, a giant array every time a transaction finishes.  The solution is to explicitly maintain the dependency graph and only dispatch the runnables that are ready to run.
Created attachment 648804 [details] [diff] [review]
Patch

Rewrite transaction handling to use explicit dependencies.
Attachment #648804 - Flags: review?(bent.mozilla)
(Reporter)

Comment 4

5 years ago
Ping?
(Reporter)

Updated

5 years ago
Summary: Lots of transactions bring FF to its knees → Lots of indexeddb transactions bring FF to its knees
I'm swamped with B2G stuff until the feature freeze date. Do you need this sooner?
(Reporter)

Comment 6

5 years ago
No

Comment 7

5 years ago
Created attachment 709351 [details] [diff] [review]
rebased patch
Comment on attachment 648804 [details] [diff] [review]
Patch

Review of attachment 648804 [details] [diff] [review]:
-----------------------------------------------------------------

This looks really good! Let's get it in.

::: dom/indexedDB/TransactionThreadPool.cpp
@@ +153,5 @@
>  
>    return NS_OK;
>  }
>  
> +struct ObjectStoreLists {

This is unused.

@@ +157,5 @@
> +struct ObjectStoreLists {
> +  nsTArray<nsString> storesWriting, storesReading;
> +};
> +
> +/* static */ PLDHashOperator

Nit: // static on its own line, here and below.

@@ +247,3 @@
>      }
>  
> +    PRUint32 i = blockInfo->lastBlockingWrites.IndexOf(info);

We need to change all the PR* to the new types.

@@ +253,4 @@
>    }
>  
> +  info->blocking.EnumerateEntries(MaybeUnblockTransaction, info);
> +  info = nullptr;

This seems unnecessary?

@@ +273,5 @@
>    if (!mTransactionsInProgress.Get(databaseId, &dbTransactionInfo)) {
> +    // First transaction for this database.
> +    dbTransactionInfo = new DatabaseTransactionInfo();
> +    dbTransactionInfo->transactions.Init();
> +    dbTransactionInfo->blockingTransactions.Init();

Let's just put this in a new DatabaseTransactionInfo constructor.

@@ +290,5 @@
> +  transactionInfo->transaction = aTransaction;
> +  transactionInfo->queue = new TransactionQueue(aTransaction);
> +  transactionInfo->objectStoreNames.AppendElements(objectStoreNames);
> +  transactionInfo->blockedOn.Init();
> +  transactionInfo->blocking.Init();

Similar here.

@@ +300,5 @@
> +    TransactionInfoPair* blockInfo =
> +      dbTransactionInfo->blockingTransactions.Get(objectStoreNames[index]);
> +    if (!blockInfo) {
> +      blockInfo = new TransactionInfoPair();
> +      blockInfo->lastBlockingReads = nullptr;

And same here.

@@ +312,5 @@
> +      transactionInfo->blockedOn.PutEntry(blockingInfo);
> +      blockingInfo->blocking.PutEntry(transactionInfo);
> +    }
> +
> +    if (aTransaction->mMode == IDBTransaction::READ_WRITE && 

Nit: trailing whitespace

@@ +415,4 @@
>      return;
>    }
>  
>    nsAutoTArray<nsRefPtr<IDBTransaction>, 50> transactions;

Nit: Move this down to right before you use it.

@@ +438,5 @@
>      transactions[index]->Abort();
>    }
>  }
>  
> +struct TransactionSearchInfo {

Nit: { on its own line

@@ +461,1 @@
>  bool

Nit: need a line in between there.

@@ +465,5 @@
>    NS_ASSERTION(aDatabase, "Null pointer!");
>  
> +  DatabaseTransactionInfo* dbTransactionInfo = nullptr;
> +  mTransactionsInProgress.Get(aDatabase->Id(), &dbTransactionInfo);
> +  if (!dbTransactionInfo) {

Either use the bool return from Get() or use the form of get that only takes one arg.

@@ +471,5 @@
>    }
>  
> +  TransactionSearchInfo info;
> +  info.db = aDatabase;
> +  info.found = false;

Constructor for this.

::: dom/indexedDB/TransactionThreadPool.h
@@ +65,5 @@
>      NS_DECL_NSIRUNNABLE
>  
> +    inline TransactionQueue(IDBTransaction* aTransaction);
> +
> +    inline void Unblock();

Weird, I don't remember why these are supposed to be inlined. Let's remove that.

@@ +90,5 @@
> +    nsTHashtable<nsPtrHashKey<TransactionInfo> > blockedOn;
> +    nsTHashtable<nsPtrHashKey<TransactionInfo> > blocking;
> +  };
> +
> +  struct TransactionInfoPair {

Nit: { on its own line.

@@ +94,5 @@
> +  struct TransactionInfoPair {
> +    // Multiple reading transactions can block future writes.
> +    nsTArray<TransactionInfo*> lastBlockingWrites;
> +    // But only a single writing transaction can block future reads.
> +    TransactionInfo* lastBlockingReads;

Nit: not plural ;)
Attachment #648804 - Flags: review?(bent.mozilla) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4990982fd823
Assignee: nobody → khuey
https://hg.mozilla.org/mozilla-central/rev/4990982fd823
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Depends on: 859591
You need to log in before you can comment on or make changes to this bug.