581606 - Avoid sqlite fragmentation via SQLITE_FCNTL_CHUNK_SIZE

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[(dormant account)]

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)]

Attachment: fs_multiplier pragma

Comment 2

[(dormant account)]

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)]

Attachment: sqlite 3.7.1 snapshot+ some printfs

Comment 4

[(dormant account)]

Attachment: fragmentation avoidance for most commonly fragmented files

Comment 5

[(dormant account)]

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)]

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)]

Attachment: 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)]

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)]

(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] (he/him)]

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)]

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] (he/him)]

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] (he/him)]

Whoops, clipboard fail:
http://www.sqlite.org/capi3ref.html#sqlite3_file_control

Comment 14

[(dormant account)]

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] (he/him)]

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]

(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)]

Attachment: added db name param

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

(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] (he/him)]

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]]

Don't think this blocks but would probably take a patch if it came soon.

Comment 21

[Shawn Wilsher :sdwilsh]

We could write a test case for this to ensure it does what we expect, right?

Comment 22

[(dormant account)]

Attachment: now with testcase

Added a testcase + link in the idl comment, no other changes.

Comment 23

[Shawn Wilsher :sdwilsh]

(In reply to comment #22)
> Added a testcase + link in the idl comment, no other changes.
Thanks!

Comment 24

[(dormant account)]

I ran patch with testcase through try, things look good. Mossop, can you approve the patch?

Comment 25

[(dormant account)]

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]]

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]

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)]

Attachment: 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]]

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)]

Attachment: frag with even more whitespace

Carrying over r/sr+

Comment 31

[Timothy Nikkel (:tnikkel)]

http://hg.mozilla.org/mozilla-central/rev/1065142ddb2c

Comment 32

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/mozIStorageConnection#setGrowthIncrement()

Comment 33

[Florian Quèze [:florian]]

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)]

(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]

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]

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]]

File a bug to get a pref instead of commenting in closed bugs may bring you nearer to your target than you ever thought.