In bug 807021, I moved LocalStorage writes off the main thread but the reading & writing threads still used the same SQLite connection. In rare cases, this may cause the main thread (the reader) to block on the helper thread (the writer). It was also brought to my attention that SQLite WAL files can become quite sizeable, resulting in slower (main-thread) read times and longer browser shutdowns. Check-pointing the WAL more aggressively during the session (on the helper thread) should improve reads & DB close times. Reviewers: See https://bugzilla.mozilla.org/show_bug.cgi?id=807021#c31 for a description of the current LocalStorage design. Note: There is an even newer LocalStorage implementation coming in bug 600307, but it will land in Firefox 22. I'd like to uplift this improvement to Firefox 21 and possibly even Firefox 20.

Please, consider work on this bug to happen for the new code from bug 600307. I am starting push on that bug and it would be better to spend resource on these optimization for the new code rather. Thanks.

Per mak's recommendations: - PRAGMA wal_autocheckpoint = 0.5 MB (in pages) - PRAGMA journal_size_limit = 1.5 MB - LocalStorage now uses separate connections for reading and writing (from separate threads) to prevent blocking Marco: During testing I noticed that even with the journal_size_limit = 1.5MB, a single large transaction could cause the WAL to grow past this limit (e.g 4MB). Only subsequent transactions caused the WAL to shrink. Do you know if this is by design?

(In reply to Honza Bambas (:mayhemer) from comment #1) > Please, consider work on this bug to happen for the new code from bug > 600307. I am starting push on that bug and it would be better to spend > resource on these optimization for the new code rather. Thanks. I'm really targeting this patch for Aurora (Firefox 21) and I think you can use the code in this patch directly in 600307.

(In reply to Vladan Djeric (:vladan) from comment #3) > I'm really targeting this patch for Aurora (Firefox 21) OK > and I think you can > use the code in this patch directly in 600307. No, I will do that in a followup. As I said, I want to land it ASAP as is and not do any more review rounds for that large patch. It's actually code frozen, if to say it that way.

(In reply to Vladan Djeric (:vladan) from comment #2) > Marco: During testing I noticed that even with the journal_size_limit = > 1.5MB, a single large transaction could cause the WAL to grow past this > limit (e.g 4MB). Only subsequent transactions caused the WAL to shrink. Do > you know if this is by design? Yes, it's by design, that pragma tells SQLite to truncate the journal at the next checkpoint, but the checkpoint of a large transaction may still cause a large journal. The difference is that without the limit the journal would grow for that large transaction and then would never shrink.

Comment on attachment 723723 [details] [diff] [review] Auto-checkpoint WAL, impose max size on WAL, and use separate SQLite connections for reading & writing Review of attachment 723723 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/src/storage/nsDOMStoragePersistentDB.cpp @@ +220,5 @@ > // Spawn helper thread > rv = ::NS_NewNamedThread("DOM Storage", getter_AddRefs(mFlushThread)); > NS_ENSURE_SUCCESS(rv, rv); > + // Auto-checkpoint the WAL every 0.5 MB > + rv = ConfigureWalBehavior(); the comment is hinting at the function contents and even more specifically to a value that in future may change, that's not very nice imo. I think the fucntion name is pretty much self documenting

Comment on attachment 723723 [details] [diff] [review] Auto-checkpoint WAL, impose max size on WAL, and use separate SQLite connections for reading & writing [Approval Request Comment] Bug caused by (feature/regressing bug #): This patch is an easy performance win. User impact if declined: This patch changes thresholds that control how the LocalStorage database is maintained by SQLite. It also increases database concurrency. The benefits are reduced disk space usage (several MBs, significant for mobile), faster database reads, faster Firefox shutdown and reduced frequency of main-thread jank. Telemetry data shows that LocalStorage DB reads often cause jank. Testing completed (on m-c, etc.): Tested locally, regression tests, Nightly, etc. Risk to taking this patch (and alternatives if risky): The changes are confined to LocalStorage functionality. In the worst imaginable scenario, this patch would cause deadlock or LocalStorage "cookies" to be lost, but this is extremely unlikely. String or UUID changes made by this patch: None

Comment on attachment 723723 [details] [diff] [review] Auto-checkpoint WAL, impose max size on WAL, and use separate SQLite connections for reading & writing It looks like a good perf win especially for mobile and considering where we are in the cycle the risk seems manageable vs the reward. Based on the risk pointed out, I am already cc'ing mobile QA on this bug to lookout for any new regression's this patch may have caused in the areas of crashes,lost cookies,unexpected shutdown/hang etc .