Closed Bug 581606 Opened 10 years ago Closed 10 years ago

Avoid sqlite fragmentation via SQLITE_FCNTL_CHUNK_SIZE

Categories

(Toolkit :: Storage, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- -

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 7 obsolete files)

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.
Attached patch fs_multiplier pragma (obsolete) — Splinter Review
Note this should eventually extend the vfs api with a fallocate call.
Implemented by SetFileInformationbyHandle/SetEndOfFile on windows, posix_fallocate elsewhere.
Summary: fs_multiplier pragma(to avoid sqlite fragmentation) → Avoid sqlite fragmentation via SQLITE_FCNTL_CHUNK_SIZE
Attachment #459961 - Attachment is obsolete: true
Attachment #467602 - Flags: review?(sdwilsh)
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.
Depends on: SQLite3.7.1
this should block because not doing fragmentation avoidance leads to some incredibly bad performance for our heaviest users(2-3x slowdowns).
blocking2.0: --- → ?
Attached patch fragmentation avoidance (obsolete) — Splinter Review
Shawn doesn't have time for more reviews this week. Andrew would you take a look at this instead? The code changes are trivial.
Attachment #467601 - Attachment is obsolete: true
Attachment #467602 - Attachment is obsolete: true
Attachment #469188 - Flags: superreview?
Attachment #469188 - Flags: review?(bugmail)
Attachment #467602 - Flags: review?(sdwilsh)
Attachment #469188 - Flags: superreview? → superreview?(shaver)
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.
Attachment #469188 - Flags: superreview?(shaver) → superreview+
(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.
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?
Component: Places → Storage
QA Contact: places → storage
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).
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.
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)?
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.
(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?
Attached patch added db name param (obsolete) — Splinter Review
Attachment #469188 - Attachment is obsolete: true
Attachment #469547 - Flags: review?(bugmail)
Attachment #469188 - Flags: review?(bugmail)
(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 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.
Attachment #469547 - Flags: review?(bugmail) → review+
Don't think this blocks but would probably take a patch if it came soon.
blocking2.0: ? → -
We could write a test case for this to ensure it does what we expect, right?
Attached patch now with testcase (obsolete) — Splinter Review
Added a testcase + link in the idl comment, no other changes.
Attachment #469547 - Attachment is obsolete: true
Attachment #470108 - Flags: review?(bugmail)
Attachment #470108 - Flags: review?(bugmail) → review+
(In reply to comment #22)
> Added a testcase + link in the idl comment, no other changes.
Thanks!
I ran patch with testcase through try, things look good. Mossop, can you approve the patch?
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.
Attachment #470108 - Flags: approval2.0?
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.
Attachment #470108 - Flags: approval2.0? → approval2.0+
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.
Attached patch frag avoidance (obsolete) — Splinter Review
Minor formatting changes, made it apply to today's tree. Carrying over r+/sr+ from asuth/shaver. Seems like I can't carryover approvals.
Attachment #470108 - Attachment is obsolete: true
Attachment #470496 - Flags: superreview+
Attachment #470496 - Flags: review+
Attachment #470496 - Flags: approval2.0?
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
Attachment #470496 - Flags: approval2.0? → approval2.0+
Carrying over r/sr+
Attachment #470496 - Attachment is obsolete: true
Attachment #471323 - Flags: superreview+
Attachment #471323 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/1065142ddb2c
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Blocks: 597215
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?
(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.
Depends on: 644543
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.
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....
File a bug to get a pref instead of commenting in closed bugs may bring you nearer to your target than you ever thought.
Blocks: 666611
See Also: → 1001579
You need to log in before you can comment on or make changes to this bug.