Last Comment Bug 581606 - Avoid sqlite fragmentation via SQLITE_FCNTL_CHUNK_SIZE
: Avoid sqlite fragmentation via SQLITE_FCNTL_CHUNK_SIZE
Status: RESOLVED FIXED
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Storage (show other bugs)
: unspecified
: x86 Linux
: -- normal with 1 vote (vote)
: ---
Assigned To: (dormant account)
:
: Marco Bonardo [::mak]
Mentors:
Depends on: 644543 SQLite3.7.1
Blocks: 572459 597215 666611
  Show dependency treegraph
 
Reported: 2010-07-23 16:37 PDT by (dormant account)
Modified: 2011-06-23 09:35 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
fs_multiplier pragma (5.65 KB, patch)
2010-07-23 16:37 PDT, (dormant account)
no flags Details | Diff | Splinter Review
sqlite 3.7.1 snapshot+ some printfs (1.40 MB, patch)
2010-08-19 16:54 PDT, (dormant account)
no flags Details | Diff | Splinter Review
fragmentation avoidance for most commonly fragmented files (2.85 KB, patch)
2010-08-19 16:55 PDT, (dormant account)
no flags Details | Diff | Splinter Review
fragmentation avoidance (3.18 KB, patch)
2010-08-25 14:22 PDT, (dormant account)
shaver: superreview+
Details | Diff | Splinter Review
added db name param (3.53 KB, patch)
2010-08-26 12:04 PDT, (dormant account)
bugmail: review+
Details | Diff | Splinter Review
now with testcase (5.27 KB, patch)
2010-08-27 18:29 PDT, (dormant account)
bugmail: review+
dtownsend: approval2.0+
Details | Diff | Splinter Review
frag avoidance (5.39 KB, patch)
2010-08-30 11:36 PDT, (dormant account)
taras.mozilla: review+
taras.mozilla: superreview+
dtownsend: approval2.0+
Details | Diff | Splinter Review
frag with even more whitespace (5.39 KB, patch)
2010-09-01 16:14 PDT, (dormant account)
taras.mozilla: review+
taras.mozilla: superreview+
Details | Diff | Splinter Review

Description (dormant account) 2010-07-23 16:37:26 PDT
This adds a fs_multipler pragma. It determines the increments that the sqlite db grows in(or shrinks with vacuum).

Once one sets fs_multipler, the size of the sqlite db will be some multiple of fs_multplier*page_size. In my testing this drastically reduces fragmentation while appending to the db.
Comment 1 (dormant account) 2010-07-23 16:37:54 PDT
Created attachment 459961 [details] [diff] [review]
fs_multiplier pragma
Comment 2 (dormant account) 2010-07-23 17:49:24 PDT
Note this should eventually extend the vfs api with a fallocate call.
Implemented by SetFileInformationbyHandle/SetEndOfFile on windows, posix_fallocate elsewhere.
Comment 3 (dormant account) 2010-08-19 16:54:42 PDT
Created attachment 467601 [details] [diff] [review]
sqlite 3.7.1 snapshot+ some printfs
Comment 4 (dormant account) 2010-08-19 16:55:19 PDT
Created attachment 467602 [details] [diff] [review]
fragmentation avoidance for most commonly fragmented files
Comment 5 (dormant account) 2010-08-19 16:57:43 PDT
Sqlite will kindly grow a SQLITE_FCNTL_CHUNK_SIZE knob in 3.7.1 which we can utilize to not fragment the hell out of important files. This patch makes use of that functionality. I hope this can land in ff4.
Comment 6 (dormant account) 2010-08-25 14:17:57 PDT
this should block because not doing fragmentation avoidance leads to some incredibly bad performance for our heaviest users(2-3x slowdowns).
Comment 7 (dormant account) 2010-08-25 14:22:21 PDT
Created attachment 469188 [details] [diff] [review]
fragmentation avoidance

Shawn doesn't have time for more reviews this week. Andrew would you take a look at this instead? The code changes are trivial.
Comment 8 Mike Shaver (:shaver -- probably not reading bugmail closely) 2010-08-25 14:45:45 PDT
Comment on attachment 469188 [details] [diff] [review]
fragmentation avoidance

I would prefer the API be

attribute PRInt32 growthIncrement;

I think it's useful to be able to read it back, and that it's more natural as an attribute. sr=shaver with that change, toss it back at me if you want to argue about it.
Comment 9 (dormant account) 2010-08-25 14:47:17 PDT
(In reply to comment #8)
> I think it's useful to be able to read it back, and that it's more natural as
> an attribute. sr=shaver with that change, toss it back at me if you want to
> argue about it.

I think it's useful to read it back to, but sqlite devs don't provide a way to do that and I'd rather not store a duplicate for this value.
Comment 10 Andrew Sutherland [:asuth] 2010-08-25 15:49:59 PDT
Per storage code guidelines, you need to cast the call to sqlite3_file_control to void if you're ignoring its return value.

Since you only pass a zDbName of NULL it is not possible to affect the growth of temporary files or attached databases.  I think it makes sense to be able to surface that functionality.

Once this change is instituted, will there be problems for a user using their profile with an earlier version of Firefox?  I seem to briefly recall some mention of earlier versions of SQLite perceiving databases with slack as corrupt.  Are the latest releases of all supported firefoxes cool with this?
Comment 11 (dormant account) 2010-08-25 16:13:10 PDT
I don't know what an attached database is. Temporary files don't need this because by their nature they don't persist(and hopefully stay in the cache while they do).
Comment 12 Andrew Sutherland [:asuth] 2010-08-25 16:36:02 PDT
The documentation for the new functionality you are using mentions them:
https://bugzilla.mozilla.org/show_bug.cgi?id=581606

In a nutshell, you can ATTACH up to 9 (31 other if you change the -D's) other databases to a single connection.

This is the kind of thing that would likely be useful for any consumers that wish to shard and potentially leverage lower turnover in other databases to reduce fragmentation.  For example, I believe that chromium creates per-month databases for their searchable history.
Comment 13 Andrew Sutherland [:asuth] 2010-08-25 17:12:34 PDT
Whoops, clipboard fail:
http://www.sqlite.org/capi3ref.html#sqlite3_file_control
Comment 14 (dormant account) 2010-08-25 17:18:48 PDT
Ok. So what do you want the api for passing the name to be?
AUTF8String name param? How would a NULL be passed then(or some other default db indicator)?
Comment 15 Andrew Sutherland [:asuth] 2010-08-25 17:32:46 PDT
As the documentation says, you can pass "main" for the main string.  AUTF8String seems reasonable.  You could do a void/empty check and turn that into a NULL or "main", I suppose.  Although requiring someone to pass "main" does not seem overly arduous.
Comment 16 Shawn Wilsher :sdwilsh 2010-08-25 17:42:29 PDT
(In reply to comment #15)
> As the documentation says, you can pass "main" for the main string. 
> AUTF8String seems reasonable.  You could do a void/empty check and turn that
> into a NULL or "main", I suppose.  Although requiring someone to pass "main"
> does not seem overly arduous.
Could be an optional second argument?
Comment 17 (dormant account) 2010-08-26 12:04:55 PDT
Created attachment 469547 [details] [diff] [review]
added db name param
Comment 18 Andrew Sutherland [:asuth] 2010-08-26 15:34:11 PDT
(In reply to comment #10)
> Once this change is instituted, will there be problems for a user using their
> profile with an earlier version of Firefox?  I seem to briefly recall some
> mention of earlier versions of SQLite perceiving databases with slack as
> corrupt.  Are the latest releases of all supported firefoxes cool with this?

Okay, this was just the 3.7.0/3.6.23.1 dance that is fixed in 3.7.0.1 and 3.7.1
http://www.sqlite.org/src/info/51ae9cad317a1
by
http://www.sqlite.org/src/info/65b8636ac6

I thought I had seen something else on the users list at one point, but must have been mistaken or reading speculation of mysterious problems by third parties.

Consider this point moot.
Comment 19 Andrew Sutherland [:asuth] 2010-08-26 15:40:06 PDT
Comment on attachment 469547 [details] [diff] [review]
added db name param

Given the conciseness of your comment, it might be nice to throw in a link so the semantics of the name argument are better understood:
http://sqlite.org/c3ref/file_control.html

I'm not claiming to be able to review the non-storage bits; given that they are one-off targeted single lines of code and your performance wizardry is well known and desired by all, and shaver gave an sr, I presume it's all good.
Comment 20 Dave Townsend [:mossop] 2010-08-27 14:19:32 PDT
Don't think this blocks but would probably take a patch if it came soon.
Comment 21 Shawn Wilsher :sdwilsh 2010-08-27 14:24:42 PDT
We could write a test case for this to ensure it does what we expect, right?
Comment 22 (dormant account) 2010-08-27 18:29:20 PDT
Created attachment 470108 [details] [diff] [review]
now with testcase

Added a testcase + link in the idl comment, no other changes.
Comment 23 Shawn Wilsher :sdwilsh 2010-08-27 21:03:08 PDT
(In reply to comment #22)
> Added a testcase + link in the idl comment, no other changes.
Thanks!
Comment 24 (dormant account) 2010-08-28 10:50:10 PDT
I ran patch with testcase through try, things look good. Mossop, can you approve the patch?
Comment 25 (dormant account) 2010-08-30 10:15:47 PDT
Comment on attachment 470108 [details] [diff] [review]
now with testcase

I would like to land this asap. This patch passes try server and should dramatically improve long-term places perf.
Comment 26 Dave Townsend [:mossop] 2010-08-30 10:58:10 PDT
Comment on attachment 470108 [details] [diff] [review]
now with testcase

Approved to land early in the b6 cycle however the patch has some style issues. Please make sure to put spaces around operators before you land.
Comment 27 Shawn Wilsher :sdwilsh 2010-08-30 11:06:03 PDT
Comment on attachment 470108 [details] [diff] [review]
now with testcase

>+   * @param aDatabaseName
>+   *        Sqlite database name. "" means pass NULL for zDbName to sqlite3_file_control.
nit: SQLite not Sqlite.  Also, we generally try to make the idl docs sufficient and not require people to go look at SQLite docs.
Comment 28 (dormant account) 2010-08-30 11:36:37 PDT
Created attachment 470496 [details] [diff] [review]
frag avoidance

Minor formatting changes, made it apply to today's tree. Carrying over r+/sr+ from asuth/shaver. Seems like I can't carryover approvals.
Comment 29 Dave Townsend [:mossop] 2010-09-01 11:51:48 PDT
Comment on attachment 470496 [details] [diff] [review]
frag avoidance

>diff --git a/storage/test/unit/test_chunk_growth.js b/storage/test/unit/test_chunk_growth.js

>+  var orig_size = get_size(filename);
>+  /* Dump in at least 32K worth of data.
>+   * While writing ensure that the file size growth in chunksize set above.
>+   */
>+  const str1024 = new Array(1024).join("T");
>+  for(var i = 0;i < 32;i++) {

Spaces after the ;'s please

>+    run_sql(d, "INSERT INTO bloat VALUES('"+str1024+"')");

Spaces around the +'s please

>+    var size = get_size(filename)
>+    // Must not grow in small increments.
>+    do_check_true(size == orig_size || size >=CHUNK_SIZE);

Space after the >= please

>+  }
>+  /* In addition to growing in chunk-size increments, the db
>+   * should shrink in chunk-size increments too.
>+   */
>+  run_sql(d,"DELETE FROM bloat")
>+  run_sql(d,"VACUUM")

Spaces after the ,'s please
Comment 30 (dormant account) 2010-09-01 16:14:22 PDT
Created attachment 471323 [details] [diff] [review]
frag with even more whitespace

Carrying over r/sr+
Comment 31 Timothy Nikkel (:tnikkel) 2010-09-01 18:52:18 PDT
http://hg.mozilla.org/mozilla-central/rev/1065142ddb2c
Comment 32 Eric Shepherd [:sheppy] 2010-09-02 11:13:54 PDT
Documented:

https://developer.mozilla.org/en/mozIStorageConnection#setGrowthIncrement()
Comment 33 Florian Quèze [:florian] [:flo] 2010-10-08 09:52:31 PDT
While I understand this change is useful for Firefox, for XULRunner applications that have very little use for the places database, creating a 10MB places.sqlite file as soon as the profile is created seems a significant waste of space (and also a waste of time if it's part of the roamed profile in an university or high school with an overloaded network).

Would it be acceptable to add a preference for the default size of the file?
Comment 34 (dormant account) 2010-10-08 10:08:58 PDT
(In reply to comment #33)
> While I understand this change is useful for Firefox, for XULRunner
> applications that have very little use for the places database, creating a 10MB
> places.sqlite file as soon as the profile is created seems a significant waste
> of space (and also a waste of time if it's part of the roamed profile in an
> university or high school with an overloaded network).
> 
> Would it be acceptable to add a preference for the default size of the file?

We could take a pref to enable preallocation and set it to on by default for firefox, and/or pref for chunk size as you suggest.

There shouldn't be overhead on network drives on Windows. Files do not grow by being written to.
Comment 35 Aaron Whitehouse 2011-06-21 21:04:07 PDT
We only have very limited profiles in my corporate environment (20MB), so a 10MB Places file makes it very hard for me to use FF4 here.
Comment 36 Patrick Groleau 2011-06-23 08:56:43 PDT
10mb chunk for the places.sqlite is huge for nothing worthy. The hard disk are faster than ever and I've never found Firefox slow when using bokmark with the "old style" places.sqlite. 
Here, at the College where I work, we must fix a limit of the space use in the users profile store on network drives. We have 7000 users to care for and your "little change taht will affect no one.."  make me sad. We must "reserve" 70gb of disk space juste for bookmarks ?? Come on guys, think about companies that use your browser with network profiles. Can you add a parameters to "fix" the size f the chunk we like to use ? If not, I dont think we would upgrade....
Comment 37 Marco Bonardo [::mak] 2011-06-23 09:04:16 PDT
File a bug to get a pref instead of commenting in closed bugs may bring you nearer to your target than you ever thought.

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