Last Comment Bug 842852 - LocalStorage performance improvements
: LocalStorage performance improvements
Status: RESOLVED FIXED
[Snappy:P2]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla22
Assigned To: Vladan Djeric (:vladan)
:
Mentors:
Depends on:
Blocks: shutdown-faster
  Show dependency treegraph
 
Reported: 2013-02-19 16:47 PST by Vladan Djeric (:vladan)
Modified: 2013-03-27 00:19 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Auto-checkpoint WAL, impose max size on WAL, and use separate SQLite connections for reading & writing (14.75 KB, patch)
2013-03-11 17:42 PDT, Vladan Djeric (:vladan)
mak77: review+
bajaj.bhavana: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Vladan Djeric (:vladan) 2013-02-19 16:47:32 PST
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.
Comment 1 Honza Bambas (:mayhemer) 2013-03-11 11:15:27 PDT
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.
Comment 2 Vladan Djeric (:vladan) 2013-03-11 17:42:30 PDT
Created attachment 723723 [details] [diff] [review]
Auto-checkpoint WAL, impose max size on WAL, and use separate SQLite connections for reading & writing

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?
Comment 3 Vladan Djeric (:vladan) 2013-03-11 17:45:14 PDT
(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.
Comment 4 Honza Bambas (:mayhemer) 2013-03-11 17:49:23 PDT
(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.
Comment 5 Marco Bonardo [::mak] 2013-03-12 03:20:49 PDT
(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 6 Marco Bonardo [::mak] 2013-03-12 06:37:13 PDT
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 8 Ed Morley [:emorley] 2013-03-13 05:42:53 PDT
https://hg.mozilla.org/mozilla-central/rev/4b67ec4c0d4e
Comment 9 Vladan Djeric (:vladan) 2013-03-13 17:00:45 PDT
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 10 bhavana bajaj [:bajaj] 2013-03-14 11:16:44 PDT
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 .
Comment 11 Vladan Djeric (:vladan) 2013-03-15 20:58:38 PDT
https://hg.mozilla.org/releases/mozilla-aurora/rev/8f067a1714bf

Note You need to log in before you can comment on or make changes to this bug.