Open Bug 722181 Opened 11 years ago Updated 3 years ago

Consider using LazyIdleThread for SQLite connections

Categories

(Toolkit :: Storage, defect, P3)

12 Branch
defect

Tracking

()

REOPENED

People

(Reporter: khuey, Unassigned)

Details

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).
This is a great idea which would also probably reduce memory reduction for threads that aren't used that much.
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?
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.
What is nsIThread::equals?
That's what I get for saying something from memory.  We currently use nsIEventTarget::isOnCurrentThread.
It looks to me like storage uses do_GetCurrentThread to get the relevant event target, which will return the underlying nsThread, not the LazyIdleThread.
That's fine as long as it doesn't store the nsIThread somewhere and then use it later... Otherwise that won't work.
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.
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
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.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → INACTIVE
Status: RESOLVED → REOPENED
Priority: -- → P3
Resolution: INACTIVE → ---
You need to log in before you can comment on or make changes to this bug.