401985 - change places.sqlite default page_size value to 4K

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Mike Schroepfer]

We set the default sqlite page size to 1024 (http://lxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavHistory.cpp#124).   

We should set this to the cluster size of the file system.  Which, unfortunately, will depend on OS + file system, plus other stuff.   So we should first make sure it is worth it, then see if we can query the cluster size, if not might want to set different defaults for different OSs.

Comment 1

[(not reading, please use [e-mail] instead)]

from the google gears documentation

http://code.google.com/apis/gears/api_database.html#sqlite_changes 

PRAGMA page_size = 4096;
Desktop operating systems mostly have default virtual memory and disk block sizes of 4k and higher. 


As schrep points out, for PRAGMA page_size, we are doing 1024 (instead of 4096). 

We can't hange this for an existing places.sqlite db, from http://www.sqlite.org/pragma.html, "The page-size may only be set if the database has not yet been created."

Comment 2

[(not reading, please use [e-mail] instead)]

odd, in bug #331158 comment #11, brett writes:  "I also upped the default page size to 4K for new databases since seeks look like they're the limiting factor when paging."

and the patch in that bug has:

+#define DEFAULT_DB_PAGE_SIZE 4096

but in lxr, we have (as schrep points out) 1024

(note the code use to live in mozilla/browser but now lives in mozilla/toolkit)

Comment 3

[Mike Schroepfer]

Sounds like an easy 1 liner for ya seth :-)

Comment 4

[(not reading, please use [e-mail] instead)]

> Sounds like an easy 1 liner for ya seth :-)

yes, that change would be good to do before beta.

the harder part is about migrating any existing users.  note, we already have this same problem with existing places.sqlite and incremental auto vacuuming.

(http://lxr.mozilla.org/seamonkey/source/toolkit/components/places/src/nsNavHistory.cpp#514)

I'll whip up a patch for this bug, and log another one about tracking the harder issues (with existing sqlite databases)

Comment 5

[(not reading, please use [e-mail] instead)]

> Sounds like an easy 1 liner for ya seth :-)

the easier one liners are getting hard to come by.

while testing to make sure the on liner indeed sets the page size, and I find that it does not!

the problem is that when I added the code to set "pragma auto_vacuum=2" (see bug #385834), I added it before the call to set the page size.

a side effect of setting that auto_vacuum pragma is that we create the database, and once created, pageSizeFixed is set (so future calls to change the page size will fail.)

moving around the code, patch coming.

Comment 6

[(not reading, please use [e-mail] instead)]

Attachment: patch. use brettw's original comment (from bug #331158) move the pragmas around (setting page_size *must* be first)

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 286936 [details] [diff] [review]
patch. use brettw's original comment (from bug #331158) move the pragmas around (setting page_size *must* be first)

looks good, r=me.

Comment 8

[Mike Schroepfer]

Comment on attachment 286936 [details] [diff] [review]
patch. use brettw's original comment (from bug #331158) move the pragmas around (setting page_size *must* be first)

a+ from schrep so that folks in b1 can get DB's created witht he right page size.

Note that the comment is wrong you say "8K is
+// sqlite's default max page size."  s/8k/1k and you are good.

Comment 9

[(not reading, please use [e-mail] instead)]

> Note that the comment is wrong

the comment is wrong, but for another reason.

1k is the default page size.  at some point, 8K may have been the default max page size, but  the default max page size is now 32k (see http://lxr.mozilla.org/seamonkey/source/db/sqlite3/src/sqlite3.c#2940)

I'll fix that before checking in.

thanks for logging and approving this one, schrep!

Comment 10

[(not reading, please use [e-mail] instead)]

fixed.

Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHis
tory.cpp
new revision: 1.180; previous revision: 1.179
done

Comment 11

[(not reading, please use [e-mail] instead)]

note to qa:  one way to verify this is to create a new profile, exit, then use sqlite browser, open places.sqlite and verify that "pragma page_size" gives you 4096.

> and log another one about tracking the harder issues (with existing sqlite 
> databases)

logged bug #402076

Comment 12

[Marcia Knous [:marcia]]

Seth: I tried your steps in Comment 11 to verify, however I do not see the pragma page_size listed when I browse with SQLLite Database Browser 1.3.

Comment 13

[(not reading, please use [e-mail] instead)]

marcia, you have to execute "pragma page_size" as a query.

Comment 14

[(not reading, please use [e-mail] instead)]

note:  dr hipp, while he has also seen reports about how this benefits windows users, never noticed the performance gains of 4k vs 1k on linux.   (he chose 1k as the default because on linux that gave the best results (by a slight amount.)

today he ran the speed1.test with 1k, 2k, 4k, 8k for page size (I'll attach the test) on his mac book pro, and we didn't see any gains on mac, either (against sqlite 3.5.1)

Comment 15

[Shawn Wilsher :sdwilsh]

just a note - we don't have 3.5.1 in tree, and probably won't for 1.9 (unless someone tells me to work on it for perf reasons - it'll be a big change).

Comment 16

[Marcia Knous [:marcia]]

(In reply to comment #13)
> marcia, you have to execute "pragma page_size" as a query.
> 

Sorry, I am sql-lite challenged. Here is what I get on the Mac, with my exact STR:

1. Create a new Minefield profile on the Mac.
2. Launch the browser with that profile.
3. Do nothing in the browser. File->Quit
4. Launch sql-lite browser and perform pragma page_size query.
5. Returns 1024, not 4096.

I am running Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9a9pre) Gecko/2007110104 Minefield/3.0a9pre. I tried this twice with two different profiles and got the same results.

Comment 17

[(not reading, please use [e-mail] instead)]

marcia, those are the right steps

but you are testing 2007110104 (4 am on 11/1), but I checked in at 10 am on 11/1.

can you try again with tomorrow's build?

Comment 18

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9a9pre) Gecko/2007110204 Minefield/3.0a9pre. I verified using the steps in Comment 16. Thanks to seth for the very helpful STR.

Comment 19

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv