1131766 - Use a simpler thread model for IndexedDB transactions

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, defect)

Description

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

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.

Comment 1

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

Attachment: Part 1: Move TransactionThreadPool to ActorsParent, v1

This just moves TransactionThreadPool to ActorsParent and integrates it with the current code.

Comment 2

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

Attachment: Part 2: Simpler thread model, v1

This is the real meat.

Comment 3

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

Attachment: Part 3: Remove unlock_notify, v1

And with those changes we can remove the unlock_notify stuff from our builds!

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

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 https://mxr.mozilla.org/addons/ident?i=openDatabase&filter= 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.

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 7

[Jan Varga [:janv]]

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 8

[Jan Varga [:janv]]

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>
> +MOZ_CONSTEXPR size_t
> +LiteralStringLength(const char (&aArr)[N])

If you move it here, SetDefaultPragmas() fails to compile.

Comment 9

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

(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 10

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

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.

Comment 11

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

Attachment: Part 3: Always run StartTransactionOp, v1

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.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 13

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1b4ead852ae0

Comment 14

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/beb016173f6f - see bug 866846 comment 21

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb8d8484cce

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for frequent bc1 oranges
https://treeherder.mozilla.org/logviewer.html#?job_id=8256324&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c99969b3abc

Comment 17

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0566885a653

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d0566885a653