1791767 - Replace IndexedDB connection pool thread management with task queues on top of a thread pool

Status: RESOLVED FIXED

(Core :: Storage: IndexedDB, task)

Description

[Jens Stutte [:jstutte]]

be1cda4f-0d00-45f0-8802-629180220919 shows a case with IndexedDB #1,SHDRCV being stuck on a SpinEventLoopUntil:

0	ZwWaitForAlertByThreadId
1	RtlSleepConditionVariableSRW
2	SleepConditionVariableSRW
3	mozilla::detail::ConditionVariableImpl::wait(mozilla::detail::MutexImpl&)
4	mozilla::OffTheBooksCondVar::Wait()
4	mozilla::ThreadEventQueue::GetEvent(bool, mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator>*)
4	nsThread::ProcessNextEvent(bool, bool*)
5	NS_ProcessNextEvent(nsIThread*, bool)
6	mozilla::SpinEventLoopUntil(nsTSubstring<char> const&, mozilla::dom::indexedDB::(anonymous namespace)::ConnectionPool::ThreadRunnable::Run::<lambda_87>&&, nsIThread*)
6	mozilla::dom::indexedDB::(anonymous namespace)::ConnectionPool::ThreadRunnable::Run()
7	nsThread::ProcessNextEvent(bool, bool*)
8	NS_ProcessNextEvent(nsIThread*, bool)
8	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*)

This indicates a possible misalignment between gLiveDatabaseHashtable->Count() being zero during QuotaManager shutdown (which was not blocked) and the ConnectionPool state, as it needs to explicitly call ConnectionPool::ShutdownThread for all its threads to unblock that SpinEventLoopUntil we see blocking here.

Comment 1

[Jens Stutte [:jstutte]]

I am also wondering if with bug 1121282 fixed we could simplify this whole thing by not managing our own thread pool at all but just using a normal nsThreadPool ? IIUC the main purpose of the ConnectionPool's thread handling is to guarantee to use a connection only on the same thread it was created on ?

Comment 2

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

I think with that relaxed we can actually go a step further and just use an NS_CreateBackgroundTaskQueue-created queue for each connection "thread" and bypass the need for a custom pool at all, noting that ConnectionPool also provides the mechanism for tracking when a connection is idle and triggers closing of the database because it's idle to limit resource usage. Since file descriptors continue to not be infinite, it does seem appropriate to continue to keep the idle mechanism, and there's a bunch of related logic that would probably get renamed/refactored, like ConnectionPool::ScheduleTransaction.

Comment 3

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

As to comment 0 and be1cda4f-0d00-45f0-8802-629180220919, yeah it seems like there's a race with thread creation and shutdown or something like that, because IPDL Background and the QM I/O thread are definitely already gone, which suggests the thread was created very late.

Comment 4

[Jari Jalkanen]

Attachment: WIP: Bug 1791767 - Split ActorsParent of IndexedDB.

Comment 5

[Jari Jalkanen]

Attachment: Bug 1791767 - Remove unused thread management code from IDB ConnectionPool. r=asuth,#dom-storage

Depends on D160471

Comment 6

[Jari Jalkanen]

Attachment: Bug 1791767 - Add ISUPPORTS declaration macro for event targets. r=#xpcom-reviewers,#dom-storage

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d0fd41bff926
Add ISUPPORTS declaration macro for event targets. r=xpcom-reviewers,nika

Comment 8

[Serban Stanca [:SerbanS]]

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

Comment 9

[Jari Jalkanen]

Attachment: Bug 1791767 - Interrupt IDB idle maintenance with atomic flag. r=#dom-storage,janv

Depends on D160472

Comment 10

[Jari Jalkanen]

Attachment: WIP: Bug 1791767 - Separate IDB idle database management.

Depends on D165984

Comment 11

[Jari Jalkanen]

Attachment: Bug 1791767 - Maintain one database at a time. r=#dom-storage

Depends on D165984

Comment 12

[Jari Jalkanen]

Attachment: Bug 1791767 - Replace single element array with a pointer class. r=#dom-storage

Depends on D179089

Comment 13

[Phabricator Automation]

Comment on attachment 9335903 [details]
Bug 1791767 - Replace single element array with a pointer class. r=#dom-storage

Revision D179090 was moved to bug 1837369. Setting attachment 9335903 [details] to obsolete.

Comment 14

[Worcester12345]

Is this still a "leave-open" bug? If so, why?

Comment 15

[Jari Jalkanen]

Originally, the leave-open tag was added because we needed an extension to a utility in a different component as a prerequisite for other work related to this bug, and did not want the automation to close the ticket after the first item landed.

Now, the most relevant patches are still in review but we probably don't want to land (merge) them one by one so from that point of view, we don't need the leave-open tag anymore. After the patches are in, the code path which is relevant for the hang has been removed, so keeping the leave-open tag for monitoring purposes also won't be purposeful.

Comment 16

[Worcester12345]

(In reply to Jari Jalkanen from comment #15)

Originally, the leave-open tag was added because we needed an extension to a utility in a different component as a prerequisite for other work related to this bug, and did not want the automation to close the ticket after the first item landed.

Now, the most relevant patches are still in review but we probably don't want to land (merge) them one by one so from that point of view, we don't need the leave-open tag anymore. After the patches are in, the code path which is relevant for the hang has been removed, so keeping the leave-open tag for monitoring purposes also won't be purposeful.

So, this is all still pending a review?

Comment 17

[Jari Jalkanen]

Yes, it shouldn't take long.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1593dd3fcc0
Use task queues on stream transport service for IDB ConnectionPools. r=dom-storage-reviewers,asuth,jstutte
https://hg.mozilla.org/integration/autoland/rev/6980270f21e4
Stop maintenance when threads are needed or connection is closed. r=dom-storage-reviewers,jstutte

Comment 19

[Cosmin Sabou [:CosminS]]

Backed out as requested, backout link: https://hg.mozilla.org/integration/autoland/rev/9b1e2f9d999edf3ac20bd828cccf22d9553018ca

Comment 20

[Jan Varga [:janv]]

Attachment: Bug 1791767 - Test IDB and OPFS interaction under heavy load; r=#dom-storage

Comment 21

[Jari Jalkanen]

Attachment: Bug 1791767 - Simplify databases complete callbacks. r=#dom-storage

Comment 22

[Jari Jalkanen]

Attachment: Bug 1791767 - Use a serial event target to debug IDB connection threads. r=#dom-storage

Depends on D190279

Comment 23

[Jari Jalkanen]

Attachment: Bug 1791767 - Make thread pool event target available in IDB ConnectionPool. r=#dom-storage

Depends on D190598

Comment 24

[Jari Jalkanen]

Attachment: Bug 1791767 - Remove IDB transaction scheduling queue. r=#dom-storage

Depends on D190599

Comment 25

[Jari Jalkanen]

Attachment: Bug 1791767 - Exclude CachingDatabaseConnection with event target from general thread checks. r=#dom-storage

CachingDatabaseConnection relies on a global macro to toggle thread
ownership checks. The checks do not work with thread pool event targets
and by this change we ensure that the global macro users are not
impacted by such event target changes.

Depends on D190600

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b84d0334c5c2
Simplify databases complete callbacks. r=dom-storage-reviewers,janv

Comment 27

[Cristian Tuns]

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4e3e2379861f
Use a serial event target to debug IDB connection threads. r=dom-storage-reviewers,janv

Comment 29

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4e3e2379861f

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72be6393ca5f
Exclude CachingDatabaseConnection with event target from general thread checks. r=dom-storage-reviewers,janv

Comment 31

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/72be6393ca5f

Comment 32

[Phabricator Automation]

Comment on attachment 9357660 [details]
Bug 1791767 - Remove IDB transaction scheduling queue. r=#dom-storage

Revision D190600 was moved to bug 1791766. Setting attachment 9357660 [details] to obsolete.

Comment 33

[Jari Jalkanen]

Attachment: Bug 1791767 - Manage thread serial numbers from IDB ConnectionPool. r=#dom-storage

Depends on D190599

Comment 34

[Jari Jalkanen]

Attachment: Bug 1791767 - Remove IDB ConnectionPool idle thread management. r=#dom-storage

Depends on D165984

Comment 35

[Jari Jalkanen]

Attachment: Bug 1791767 - Drain IDB task queues on shutdown. r=#dom-storage

Depends on D190600

Comment 36

[Jari Jalkanen]

Attachment: Bug 1791767 - Manage thread serial numbers from IDB ConnectionPool. r=#dom-storage

Depends on D190599

Comment 37

[Jari Jalkanen]

Attachment: Bug 1791767 - Use task queues on nsThreadPool for IDB connections. r=#dom-storage

Depends on D165984

Comment 38

[Jari Jalkanen]

Attachment: Bug 1791767 - Drain IDB task queues on shutdown. r=#dom-storage

Depends on D190600

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2b7b572d19b
Make thread pool event target available in IDB ConnectionPool. r=dom-storage-reviewers,janv

Comment 40

[Sandor Molnar[:smolnar]]

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

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b6bd9b6e697
Manage thread serial numbers from IDB ConnectionPool. r=dom-storage-reviewers,janv

Comment 42

[Jari Jalkanen]

Attachment: Bug 1791767 - Add BackgroundThread context checks. r=#dom-storage

Comment 43

[Jari Jalkanen]

Attachment: Bug 1791767 - Dispatch IDB idle maintenance with InvokeAsync and promises. r=#dom-storage

Depends on D191451

Comment 44

[Jari Jalkanen]

Attachment: Bug 1791767 - Remove IdleConnectionRunnable class. r=#dom-storage

Depends on D191452

Comment 45

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/4b6bd9b6e697

Comment 46

[Jari Jalkanen]

Attachment: Bug 1791767 - Relax timing dependent IDB maintenance test thresholds. r=#dom-storage

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/520bd9381f15
Interrupt IDB idle maintenance with atomic flag. r=janv,dom-storage-reviewers,jstutte

Comment 48

[Norisz Fay [:noriszfay]]

Backed out or causing bustages on ActorsParent.cpp

Backout link

Push with failures

Failure log

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/592e22279f89
Interrupt IDB idle maintenance with atomic flag. r=janv,dom-storage-reviewers,jstutte

Comment 50

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/592e22279f89

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df9f354630cf
Relax timing dependent IDB maintenance test thresholds. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/01eef2ce9d20
Use task queues on nsThreadPool for IDB connections. r=dom-storage-reviewers,janv

Comment 52

[Cristian Tuns]

Backed out for causing build bustages in ActorsParent.cpp

• Backout link
• Push with failures
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/dom/indexedDB/ActorsParent.cpp:8824:1: error: 'mozilla::dom::indexedDB::{anonymous}::ConnectionPool::IdleThreadInfo::~IdleThreadInfo()' defined but not used [-Werror=unused-function]

Comment 53

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/abb086864075
Relax timing dependent IDB maintenance test thresholds. r=dom-storage-reviewers,janv
https://hg.mozilla.org/integration/autoland/rev/58f4bf812f63
Use task queues on nsThreadPool for IDB connections. r=dom-storage-reviewers,janv

Comment 54

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/abb086864075
https://hg.mozilla.org/mozilla-central/rev/58f4bf812f63

Comment 55

[Jan Varga [:janv]]

Attachment: Bug 1791767 - Remove unneeded AdjustIdleTimer call; r=#dom-storage

Comment 56

[Jari Jalkanen]

The main patch replacing the custom thread pool with nsThreadPool landed.

Comment 57

[Jari Jalkanen]

After a thorough discussion about performance regression 1891227, the following plan was conceived:

We would like to back out patch https://phabricator.services.mozilla.com/D191296 from beta, to keep the regression from impacting the users, but keep it in nightly and actively work to overcome the regression. There are already some preliminary investigations on nsThreadPool addressing the challenge.

Comment 58

[Donal Meehan [:dmeehan]]

https://phabricator.services.mozilla.com/D191296 was backed out of Beta
https://hg.mozilla.org/releases/mozilla-beta/rev/0685935ccffc93541dd6d438aba0788cdc152015

Comment 59

[Pascal Chevrel:pascalc]

Jari, we are nearing the start of the 127 beta cycle and bug 1891227 was not fixed, should we back out D191296 again when we reach beta?

Comment 60

[Jari Jalkanen]

It would be nice if you could back it out once more, thank you! The related fix probably won't quite make it to this cycle.

Comment 61

[Pascal Chevrel:pascalc]

Backed out 1 changesets (bug 1791767) from beta for introducing Bug 1891227
https://hg.mozilla.org/releases/mozilla-beta/rev/1d48046b04e7

Comment 62

[Jari Jalkanen]

The fix to the performance surprise 1891227 landed so there is no longer a reason why this could not ride the trains, once more.

Comment 63

[Phabricator Automation]

Comment on attachment 9394936 [details]
Bug 1791767 - Remove unneeded AdjustIdleTimer call; r=#dom-storage

Revision D206588 was moved to bug 1900380. Setting attachment 9394936 [details] to obsolete.

Comment 64

[Phabricator Automation]

Comment on attachment 9352049 [details]
Bug 1791767 - Test IDB and OPFS interaction under heavy load; r=#dom-storage

Revision D187686 was moved to bug 1900382. Setting attachment 9352049 [details] to obsolete.

Comment 65

[Mathew Hodson]

Set status for 126 and 127 to wontfix since it was backed out on those branches.

Comment 66

[Jens Stutte [:jstutte]]

Do we need to move some of the open patches to another bug to be able to close this such that the status flags continue to make sense?

Comment 67

[Phabricator Automation]

Comment on attachment 9357660 [details]
Bug 1791767 - Remove IDB transaction scheduling queue. r=#dom-storage

Revision D190600 was moved to bug 1903603. Setting attachment 9357660 [details] to obsolete.

Comment 68

[Phabricator Automation]

Comment on attachment 9359086 [details]
Bug 1791767 - Drain IDB task queues on shutdown. r=#dom-storage

Revision D191297 was moved to bug 1903607. Setting attachment 9359086 [details] to obsolete.

Comment 69

[Phabricator Automation]

Comment on attachment 9359395 [details]
Bug 1791767 - Remove IdleConnectionRunnable class. r=#dom-storage

Revision D191453 was moved to bug 1903614. Setting attachment 9359395 [details] to obsolete.

Comment 70

[Phabricator Automation]

Comment on attachment 9359394 [details]
Bug 1791767 - Dispatch IDB idle maintenance with InvokeAsync and promises. r=#dom-storage

Revision D191452 was moved to bug 1903614. Setting attachment 9359394 [details] to obsolete.

Comment 71

[Jari Jalkanen]

The patches which were not strictly required for this ticket were moved to dedicated items.

Comment 72

[Mathew Hodson]

All the patches still associated with this bug landed. https://phabricator.services.mozilla.com/D191296 was only backed out of Beta, so it is still fixed for Firefox 128.