722181 - Consider using LazyIdleThread for SQLite connections

Status: REOPENED

(Toolkit :: Storage, defect, P3)

Description

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

My understand is that async sqlite connections have a thread per connection.  If many of these connections go significant amounts of time without being used it might be advantageous to switch them to using LazyIdleThread (which goes away after a certain period of inactivity, and recreates the thread when needed).

Comment 1

[Shawn Wilsher :sdwilsh]

This is a great idea which would also probably reduce memory reduction for threads that aren't used that much.

Comment 2

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

Since LazyIdleThread goes away when it isn't being used it means that technically a SQLite connection could be used from different OS threads (though not concurrently of course). Any chance that will trip assertions or cause other fun problems?

Comment 3

[Shawn Wilsher :sdwilsh]

In storage, we just check that we are on the thread we created using nsIThread::equals.  As long as that still works, I can't think of anything offhand.

Comment 4

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

What is nsIThread::equals?

Comment 5

[Shawn Wilsher :sdwilsh]

That's what I get for saying something from memory.  We currently use nsIEventTarget::isOnCurrentThread.

Comment 6

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

It looks to me like storage uses do_GetCurrentThread to get the relevant event target, which will return the underlying nsThread, not the LazyIdleThread.

Comment 7

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

That's fine as long as it doesn't store the nsIThread somewhere and then use it later... Otherwise that won't work.

Comment 8

[Shawn Wilsher :sdwilsh]

We use do_GetCurrentThread to ensure that we are opened and closed on the same thread, but we shouldn't be using that for dispatching to our background thread.  For that, we use mAsyncExecutionThread, which is currently an nsCOMPtr<nsIThread>.  We also have some logic in getAsyncExecutionTarget that actually creates it, so that'd have to be refactored.

Comment 9

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

One newer possibility for this is to use the TaskQueue abstraction that provides ordering guarantees on top of a thread pool.  The downsides are:
- Successor runnables are not dispatched (to the thread pool) until their immediate predecessor runnable has completed.  On a highly contended thread pool, this can increase latencies over what would happen in a dedicated thread pool.  On the other hand, it increases fairness by effectively having connections operate in a round-robin fashion.
- Dispatching to a thread pool potentially loses "affinity" benefits if the runnable gets dispatched to a different thread which the OS has no reason to tie to a specific CPU core and its warm cache.  SQLite has non-trivial code size and the page cache can have non-trivial data size, so these are things that can matter.  At least the code caching issue can be mitigated by having mozStorage use its own threadpool so threads aren't also being used for non-mozStorage purposes.  For data locality, the simplest fix would be to alter TaskQueue to greedily keep consuming its own events rather than re-dispatching itself (at the cost of fairness).

MutableBlobStorage recently started using this idiom on top of the StreamTransportService threadpool.  See https://hg.mozilla.org/mozilla-central/rev/40d99f377bdd#l1.176

Comment 10

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.