cookies.sqlite-wal takes too much space for Fennec

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: mbrubeck, Assigned: dwitte)

Tracking

Trunk
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b2+)

Details

Attachments

(2 attachments)

Reporter

Description

9 years ago
Using the latest nightly build of Fennec for Android, the cookies.sqlite-wal file in the profile directory grows to over 25 MB after about ten minutes of browsing to about a dozen different sites.  After exiting Fennec, the file is removed and the storage is freed.

Because disk space is scarce on many mobile devices, we want to prevent this file from growing so large in Fennec.
Reporter

Updated

9 years ago
tracking-fennec: --- → ?
Assignee

Comment 1

9 years ago
Shawn, drh, any idea why this would grow so large? Cookie databases themselves are typically 1MB. Are there (pathological or otherwise) use patterns of sqlite where this is known to happen?
Reporter

Comment 2

9 years ago
Bug 598196 enabled WAL for cookies.sqlite.
Blocks: 598196
Got this from drh via e-mail:
The WAL file can fail to be truncated if a read transaction is being held
open.  This could be due to a unfinalized statement, or it could happen if
there are multiple readers that overlap and there is never a moment when
nobody is reading the database.

The WAL file is truncated a checkpoint operation occurs while no reads are
taking place.  If there is an active reader, then it is not possible for the
checkpoint to truncate the WAL since doing so would remove the WAL out from
under the reader.


Now, why is it 25MB, I don't know.  Seeing the contents of that file would possibly be interesting.

In the future, please don't make the first action of a possible issue with SQLite be to cc the primary developer of SQLite.  Most issues we see end up being bugs in our own code, and I'd hate to waste upstream's time.  cc'ing me is generally sufficient.
Reporter

Comment 4

9 years ago
(In reply to comment #3)
> Now, why is it 25MB, I don't know.  Seeing the contents of that file would
> possibly be interesting.

cookies.sqlite-wal is 32MB in my Minefield profile after a day's use in the latest nightly.  If you'd like me to send you the file, let me know (or I can generate one from a throwaway profile and post a link here).  Or you can check your own profile to see if you see the same thing.
Assignee

Comment 5

9 years ago
(In reply to comment #3)

> The WAL file can fail to be truncated if a read transaction is being held
> open.  This could be due to a unfinalized statement, or it could happen if
> there are multiple readers that overlap and there is never a moment when
> nobody is reading the database.

Thanks -- I'll take a look at what's going on. It's most likely that we have a statement sticking around.

> In the future, please don't make the first action of a possible issue with
> SQLite be to cc the primary developer of SQLite.  Most issues we see end up
> being bugs in our own code, and I'd hate to waste upstream's time.  cc'ing me
> is generally sufficient.

I didn't at all mean to imply this was a sqlite issue -- I was merely asking what would lead to this behavior.
Assignee

Comment 6

9 years ago
From http://www.sqlite.org/wal.html:
"The default strategy is to allow successive write transactions to grow the WAL until the WAL becomes about 1000 pages in size."

See also: http://mxr.mozilla.org/mozilla-central/source/storage/public/mozIStorageConnection.idl#72

So it's going to grow to 32MB. We need to use 'PRAGMA wal_autocheckpoint=N' to drop it down.
Assignee

Comment 7

9 years ago
Patch forthcoming. Can you guys test Fennec with this and make sure it does what you want? WAL size should be limited to around 500 - 700K.
Assignee

Comment 8

9 years ago
Posted patch patchSplinter Review
Assignee: nobody → dwitte
Status: NEW → ASSIGNED
after rebuilding with this patch, browsing around and restarting my cookies.sqlite-wal is down to 590kb

Updated

9 years ago
tracking-fennec: ? → 2.0b2+
Assignee

Updated

9 years ago
Attachment #487140 - Flags: review?(sdwilsh)
Reporter

Comment 10

9 years ago
This patch is working for me too.
Comment on attachment 487140 [details] [diff] [review]
patch

>+  // Use write-ahead-logging for performance. We cap the autocheckpoint limit at
>+  // 16 pages (around 500KB).
>   mDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING("PRAGMA journal_mode = WAL"));
>+  mDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+    "PRAGMA wal_autocheckpoint=16"));
nit: use a space around '=' like you do everywhere else

Can you also add a test so we don't accidentally regress this in the future?  Shouldn't be too hard to check the WAL file's file size while doing a bunch of cookie operations.

r=sdwilsh
Attachment #487140 - Flags: review?(sdwilsh) → review+
pushed http://hg.mozilla.org/mozilla-central/rev/97ea3fe9232a

dwitte said he'd follow up with a test
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Reporter

Comment 13

9 years ago
Pushed to Fennec 4.0b2 relbranch (GECKO20b7pre_20101029_RELBRANCH):
http://hg.mozilla.org/mozilla-central/rev/57bc64a3949f
(In reply to comment #12)
> pushed http://hg.mozilla.org/mozilla-central/rev/97ea3fe9232a
> 
> dwitte said he'd follow up with a test
So do we have an ETA for the test?  I understand you guys want to move fast, but my review was conditional on having a test.
(In reply to comment #14)
> (In reply to comment #12)
> > pushed http://hg.mozilla.org/mozilla-central/rev/97ea3fe9232a
> > 
> > dwitte said he'd follow up with a test
> So do we have an ETA for the test?  I understand you guys want to move fast,
> but my review was conditional on having a test.

Noted. We'll make sure the test lands soon.

Dan - someone from mobile can work on the test. Let me know if you have particular ideas for the test, otherwise, we'll add a simple file size check at the end of some cookie tests.
Assignee

Comment 16

9 years ago
I'll write one today!
Assignee

Comment 17

9 years ago
Posted patch testSplinter Review
Attachment #487681 - Flags: review?(mark.finkle)
Comment on attachment 487681 [details] [diff] [review]
test

Simple but effective. Not checking for exactly 500K is wise too. I'm sure there is some "play" in the values returned by some filesystems.
Attachment #487681 - Flags: review?(mark.finkle) → review+
Reporter

Updated

9 years ago
Blocks: 625981
You need to log in before you can comment on or make changes to this bug.