Closed Bug 1131766 Opened 8 years ago Closed 8 years ago

Use a simpler thread model for IndexedDB transactions


(Core :: Storage: IndexedDB, defect)

Not set



Tracking Status
firefox40 --- fixed


(Reporter: bent.mozilla, Assigned: bent.mozilla)




(3 files, 1 obsolete file)

Instead of having one thread and one SQLite sharedcache connection per IndexedDB transaction we're going to have just a single thread and connection per database file. This should save us memory and time.
This just moves TransactionThreadPool to ActorsParent and integrates it with the current code.
Attachment #8562314 - Flags: review?(khuey)
This is the real meat.
Attachment #8562315 - Flags: review?(khuey)
Attached patch Part 3: Remove unlock_notify, v1 (obsolete) — Splinter Review
And with those changes we can remove the unlock_notify stuff from our builds!
Attachment #8562316 - Flags: review?(mak77)
Attachment #8562314 - Attachment description: Move TransactionThreadPool to ActorsParent, v1 → Part 1: Move TransactionThreadPool to ActorsParent, v1
Attachment #8562315 - Attachment description: Simpler thread model, v1 → Part 2: Simpler thread model, v1
Comment on attachment 8562316 [details] [diff] [review]
Part 3: Remove unlock_notify, v1

Review of attachment 8562316 [details] [diff] [review]:

I'm happy indexedDB is moving to a much simpler connections management.
IndexedDB was so far the most common consumer for this code (originally added in bug 571599 exactly for iDB), and we are "unlikely" to use shared cache in the product DBs.
But I cannot say the same for add-ons, nor I can tell if the various openDatabase() calls in our codebase could lead to multiple connections accessing the same shared cache db, even if just for a brief time.

The win (one less symbol imported from Sqlite and one less condition) doesn't seem to balance the risks (add-ons or unexpected code using a shared cache and failing unpredictably with SQLITE_LOCKED).
My feeling is that this isn't worth it. The removed code seems to have an ignorable runtime and maintenance cost.
If we'd move towards deprecating shared cache connections (And I'd be all-in for that), than after removing any possibility to create them, I'd be happy to also remove this code. But this should happen elsewhere and be explicitly about removing the shared cache connections option.

Now, since Andrew reviewed the original patch that added it, I'm going to forward the request to him, that surely has some additional background about the original intent.
Attachment #8562316 - Flags: review?(mak77) → feedback?(bugmail)
Comment on attachment 8562316 [details] [diff] [review]
Part 3: Remove unlock_notify, v1

I've just skimmed dxr for in-tree consumers that look like they would get upset.  I'm not seeing any.  I was expecting to see Places doing it, but it looks like it uses unshared connections and doesn't randomly open extra connections.

Add-on-wise, there are indeed a lot of addons using the openDatabase signature (probably, mxr is naive) per and it is appropriate to be sensitive to existing addons.

However, we need to consider in what circumstances this logic would actually benefit them.  C++ extensions are not supported, so the (JS) extension logic is confined to running on the main thread.  Additionally, it is confined to either perform synchronous main-thread DB access, or async database manipulations.

Our sqlite3_unlock_notify functionality explicitly avoids blocking on the main thread which means that mixed sync/async usage will already result in SQLITE_LOCKED errors on the main thread.  So the only time it will reliably hide sketchy usage is if it is used for multiple async-ish connections to the same database.

It's conceivable there is code that creates multiple async shared-cache connections thinking it will make things go faster.  (It won't, of course, since mozStorage async threads are I/O bound (unless the SQL is just really, really bad) and the way the shared-cache works means that only one I/O request can be in flight at a time.)

So I think the important questions are:
- How likely are extensions that open multiple connections to the same database for overlapping async use with queries that access multiple table?
- Where does that fall on the "supported" spectrum?
- Can we take some other action to offset the potential fallout of this change?  Like mapping all of the shared-cache connections to use the same async execution thread.  Silently disabling shared-cache wouldn't work unless we also defaulted everything to WAL and automatically upgraded everything.  (Otherwise a trip to SQLITE_BUSY-town could very easily happen, and that's much worse.)
- Given that SQLITE_LOCKED will already be seen on the main thread, does the async edge case phenomenon matter at all?

:mak makes a good case for the trade-offs.  Given that, I would propose that we do one of two things:
1) Just forget about this.
2) Land a telemetry probe on this bug that counts the number of times we actually use the SQLITE_LOCKED logic (possibly indicating the database name like slow SQL does) and spin this patch off to be landed/further considered after the telemetry probe hits baseline.
Attachment #8562316 - Flags: feedback?(bugmail)
In ConnectionPool::ThreadRunnable, there is no "transaction thread" anymore, so update the comment.

We should verify that the SQLite WAL journal honors truncates, so we don't end up with bug 1072773 again.

queuedRunnables.Compact() is unnecessary, in fact that whole conditional around the if is unnecessary, although I understand if you want to leave it.

scheduledTransactions.Compact() is unnecessary
Comment on attachment 8562316 [details] [diff] [review]
Part 3: Remove unlock_notify, v1

Review of attachment 8562316 [details] [diff] [review]:

::: storage/src/mozStorageConnection.cpp
@@ -970,5 @@
> -
> -  int srv;
> -  while ((srv = ::sqlite3_step(aStatement)) == SQLITE_LOCKED_SHAREDCACHE) {
> -    if (!checkedMainThread) {
> -      checkedMainThread = true;

|bool checkedMainThread = false;| needs to be removed above
Comment on attachment 8562314 [details] [diff] [review]
Part 1: Move TransactionThreadPool to ActorsParent, v1

Review of attachment 8562314 [details] [diff] [review]:

::: dom/indexedDB/ActorsParent.cpp
@@ +6013,5 @@
>   ******************************************************************************/
> +template <size_t N>
> +LiteralStringLength(const char (&aArr)[N])

If you move it here, SetDefaultPragmas() fails to compile.
(In reply to Jan Varga [:janv] from comment #8)
> If you move it here, SetDefaultPragmas() fails to compile.

Yeah, sorry. I had fixed that locally.
Comment on attachment 8562316 [details] [diff] [review]
Part 3: Remove unlock_notify, v1

Ok, I'll file a followup for doing this part later with telemetry.
Attachment #8562316 - Attachment is obsolete: true
Found this in testing, we need to make sure that StartTransactionOp always runs, even if the database has been invalidated (otherwise the logging gets messed up). And then some assertions can fail in this case, so relax them if the StartTransactionOp didn't actually hook up a DatabaseConnection.
Attachment #8582474 - Flags: review?(khuey)
Comment on attachment 8582474 [details] [diff] [review]
Part 3: Always run StartTransactionOp, v1

Review of attachment 8582474 [details] [diff] [review]:

This needs to be rebased over the death of MOZ_OVERRIDE, but you'll find that out soon enough.
Attachment #8582474 - Flags: review?(khuey) → review+
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Flags: needinfo?(bent.mozilla)
You need to log in before you can comment on or make changes to this bug.