744959 - Disable SQLite DB locking for browser.db and tabs.db

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

If we can meet the required caveats for not needing the locking, we could improve our DB performance.

This is a spin off of bug 741224. We should investigate. I think Sriram found that Webkit disables locking for their history DB. In that bug we made a few comments:
--------------------------------------------------------------------

Sriram Ramasubramanian:
Searching around the big web, I finally found this guy:

http://developer.android.com/reference/android/database/sqlite/SQLiteDatabase.html#setLockingEnabled%28boolean%29

It states:
"This is pretty expensive, so if you know that your DB will only be used by a single thread then you should set this to false. The default is true."

Since every DB access goes through ContentProvider, I think we are ensuring thread safety there. Aren't we? If so, we should probably turn off setLockingEnabled(). This "might" gives us performance win too.
----
Richard Newman:
If you can guarantee it (by never allowing the DB handle and the bridge to escape), then absolutely. Good spot!
----
Lucas Rocha
About thread-safety: the important thing for keeping SQLite databases sane in Android is to avoid opening multiple DB connections from different threads. Our content provider solves this problem (as a side effect) because it concentrates all DB accesses into only one DB connection (the SQLiteOpenHelper instance).

So, as long as we don't create separate DB connections to tabs.db or browser.db outside the content provider, it should be (in theory) safe to disable locking. But yes, we need a reliable way to guarantee that this actually fixes the bug here.

Comment 1

[Richard Newman [:rnewman]]

If you *always* talk to the DB through a `ContentResolver`, then Android will ensure that you have a single `ContentProvider`, in which you can concentrate your DB handle.

That said, you also have to ensure that your DB lifecycle aligns with the CP lifecycle, and you have to pay attention to the concurrency notes in the `ContentProvider` docs -- any CP method other than `onCreate` can be concurrently invoked from multiple threads, so you need to perform some kind of synchronization/queuing/whatever.

And right now we see stuff like this in the log:

  E SQLiteBridge(989)           Bridge finalized without closing the database

aaaaaalll the time. That can't be good for performance or correctness.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

The Webkit reference:
https://github.com/android/platform_frameworks_base/blob/master/core/java/android/webkit/WebViewDatabase.java#L254

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #1)
> If you *always* talk to the DB through a `ContentResolver`, then Android
> will ensure that you have a single `ContentProvider`, in which you can
> concentrate your DB handle.
> 
> That said, you also have to ensure that your DB lifecycle aligns with the CP
> lifecycle, and you have to pay attention to the concurrency notes in the
> `ContentProvider` docs -- any CP method other than `onCreate` can be
> concurrently invoked from multiple threads, so you need to perform some kind
> of synchronization/queuing/whatever.

Yeah, this will be the hard part of turning locking off. 

> And right now we see stuff like this in the log:
> 
>   E SQLiteBridge(989)           Bridge finalized without closing the database
> 
> aaaaaalll the time. That can't be good for performance or correctness.

The bridge is around to access "Gecko" DBs and we'll probably be unable to turn off locking there. The Java only DBs might be a better place to start. We should probably file a bug on the above issue in any case.

Comment 4

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

This is a patch to turn off locking in DB's accessed through Java.

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

See also: bug 741224 and bug 741718

This might be a solution to those bugs. Maybe not.

Comment 6

[Lucas Rocha (:lucasr)]

Ok, I've spend some time looking through android's code and ours. We use  enableWriteAheadLogging() (aka WAL) on android >= Honeycomb, which makes disabling locking a no go. It creates a pool of connections and rely on locking to work properly. Furthermore, the content provider execute DB commands from different threads so there's no guarantee that it will always use the DB connection from the same thread (so my comment above is incorrect). Disabling locking means SQLite will not be thread-safe anymore, even if we always use only one DB connection inside the content provider.

So, my current understanding is that we'll have to deal with those locking issues in some other way. I'm experimenting with creating a big dump of the locked browser.db (if we get to connect in read-only mode at least) and re-creating the database on the fly. Might not scale well for huge databases. Let's see.

Another option is somehow ensuring that we always run use the DB connection from the same thread. We don't do anything to ensure that right now.

I'm still struggling to write a test case that reliably reproduces the lock problem.

Comment 7

[Lucas Rocha (:lucasr)]

Closing this bug as it's not safe to disable locking.