390244 - code to drop the user_title column from moz_places table (see bug #389876) makes the places.sqlite file 30% bigger

Status: RESOLVED WONTFIX

(Firefox :: Bookmarks & History, defect)

Description

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

code to drop the user_title column from moz_places table (see bug #389876) makes the places.sqlite file 30% bigger

running with ispiked's 52 mb places.sqlite file, after upgrading and quitting, my places.sqlite file is now 68.3 MB

I then opened up the 68.3 mb places.sqlite in the SQLite Database Browser, executed a VACUUM command, and quit, taking it down to 55 MB.

so, this is a good candidate for vacuum on an idle timer (see bug #385834) or PRAGMA auto_vacuum = 2 (or incremental), but I think we need a new version of sqlite for that (but shawn might have early m8 plans to take us there!)

Comment 1

[Shawn Wilsher :sdwilsh]

That patch is good to go for M8

Comment 2

[Marco Bonardo [:mak]]

that is because the table is renamed, a new table is created and then the old table deleted (space is not recalled), so its space is duped... 
a solution could be instead create a temporary table (forcing it in memory with a PRAGMA if it's not already so) copy all data to temporary table then delete and recreate the local table. I've see that this way the db does not grow, but it needs 2 select INTO instead of one

Comment 3

[Mike Connor [:mconnor]]

Seth, can you take this?

Comment 4

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

> Seth, can you take this?

yes.  fix coming with bug #385834

Comment 5

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

Attachment: patch

for existing users, where we don't have incremental vacuuming turned on (because the database was created before the fix for bug #385834) this will fix this bug.

upon 15 minutes of idle, for those users, we'll do a full vacuum and compact users hit by this 30% increase.

Comment 6

[Dietrich Ayala (:dietrich)]

Comment on attachment 278556 [details] [diff] [review]
patch

r=me. lets give this a shot, and carefully watch the build forum for perf complaints.

Comment 7

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

dietrich (and others), one reason we'd like to do this, even when doing auto vacuum, for de-fragmentation issues.

from http://www.sqlite.org/lang_vacuum.html:

As of SQLite version 3.1, an alternative to using the VACUUM command is auto-vacuum mode, enabled using the auto_vacuum pragma. When auto-vacuum is enabled for a database, large deletes cause the size of the database file to shrink. However, auto-vacuum also causes excess fragmentation of the database file. And auto-vacuum does not compact partially filled pages of the database as VACUUM does.

thanks for the review, dietrich.

Comment 8

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

fixed.

/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.160; previous revision: 1.159
done

will log a spin off bug about how we should consider doing a full vacuum after every <x> minutes of idle for all users (no matter what the auto_vacuum mode), for database defragmentation issues (see comment #7)

Comment 9

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

re-opening, had to back out, the tree was closed.

Comment 10

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

fixed.

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

Comment 11

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

> will log a spin off bug about how we should consider doing a full vacuum after
> every <x> minutes of idle for all users (no matter what the auto_vacuum mode),
> for database defragmentation issues (see comment #7)

see bug #395020

Comment 12

[Benjamin Smedberg]

Backed out due to tbox orange

Comment 13

[Phil Ringnalda (:philor)]

Since our tests are bound to not be the only thing someone wants to run while they aren't typing, and this is bound to not be the only slow thing we'll try to run off the idleservice, I wonder if it needs a lie_and_say_I_typed or a tell_them_I_will_never_be_idle pref.

Comment 14

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

benjamin tells me that the tests can take about 20 minutes.  perhaps doing the
vacuum after 15 minute of idle causes the orange.

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

except, that this fix should only affect "old" profiles.  

are the tests on tinderbox creating a new profile each time, or it using an "old" (pre m8?) profile that it copies in there every time?

Comment 15

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

additionally, one part of the orange was this:

REFTEST UNEXPECTED FAIL (LOADING): file:///C:/slave/trunk/mozilla/layout/reftests/pixel-rounding/image-left-width-4.html

not sure how my change caused that one, but it too when green once bsmedberg backed me out.

Comment 16

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

robcee tells me that the unit test tinderboxes are using a 122 MB "old" places.sqlite, which is some what of a shocker to me.

more details (such as the user version pragma of that places.sqlite file) coming soon.

Comment 17

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

for the unit tests, robcee also tells me that

1) it's 122 MB because we keep using the same places.sqlite file, so it keeps growing!

2) he's got a bug filed to create that profile dynamically at reftest startup so this'll go away

Comment 18

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

relanding, as rob has removed the 122 MB places.sqlite files from the tinderboxen.

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

Comment 19

[Steve England [:stevee]]

Is this thing meant to run just once? Because it's already run once for me, but just now I left my machine idle for more than 15 minutes and it appeared to be doing the same thing again (CPU at 99%, Memory slowly increasing).

(And btw, this reduced my 47.8MB places.sqlite file to 45.1MB, a ~5.6% saving)

Comment 20

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

> Is this thing meant to run just once?

the current code will vacuum more than once, good catch.  on a long idle, we'll vacuum every 5 minutes (after the first 15 minutes of idle).

but perhaps we should remember the last vacuum time to avoid that and to avoid vacuuming twice in one day.   I'll log a spin off bug on that.

> just now I left my machine idle for more than 15 minutes and it appeared to be
> doing the same thing again (CPU at 99%, Memory slowly increasing).

to clarify, you noticed the cpu spike and memory usage because we were vacuuming? Did the cpu drop back to normal after vacuuming was done?

Comment 21

[Steve England [:stevee]]

(In reply to comment #20)
> to clarify, you noticed the cpu spike and memory usage because we were
> vacuuming? Did the cpu drop back to normal after vacuuming was done?

I didn't give firefox enough time to finish whatever it was doing. Last time it took about 20 minutes and I didn't have enough time to let it finish whatever it was doing. However, the symptoms were the same: Happened about 15 minutes after my machine was idle (i went to eat), CPU usage was maxed out, and memory was under 200MB and jumping up and down, so I'm pretty sure it was the same thing.

Comment 22

[Steve England [:stevee]]

Sorry Seth, I guess there's something I don't understand. I thought this bug introduced something that would run only once (ever), and remove a column from my places.sqlite file to make it smaller. (So once it had been run, it would never run again; the conversion or whatever would be done).

So when you say "but perhaps we should remember the last vacuum time to avoid that and to avoid vacuuming twice in one day." this worries me. If i leave my PC idle for more than 15 minutes each day, then it's going to lock up for 20 odd minutes for some reason?

Comment 23

[Jim Jeffery not reading bug-mail 1/2/11]

I am seeing what I think is something similiar, but I'm using Vista HP, and
when I resume Minefield from the TaskBar the Vista 'faded' screen kicks in
reporting 'Not Responding' with the CPU spiking to 100%, forcing a 'kill app'
via the Task Manager.

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007090604
Minefield/3.0a8pre Firefox/3.0 ID:2007090604

Comment 24

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

given what steve and jim are seeing, I need to disable the vacuuming until it is ready for prime time, and I'm now concerned about bug #395020, which would vacuum on idle.

I'm going to disable this code for m8 and reopen the bug.

Comment 25

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

I've disabled the full vacuum code, thanks steve and jim.

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

Comment 26

[Jo Hermans]

Maybe we should do only an auto-vacuum. People with old sqlite files (where an auto-vacuum doesn't work) will have to recreate their profiles (or in case of places.sqlite, drop it and let is be recreated automatically). If you don't do that, then you just have to live with a places file that slowly grows.

A full vacuum can still be useful, but should not be done very frequently. Once per month or so would be enough. Look for example at bug 394379 ; not necessarily the best solution, but more acceptable than a lockup every 15 minutes.

Comment 27

[Jim Jeffery not reading bug-mail 1/2/11]

Seth, 
Quick note, the backout has stopped my Lock-ups I was getting on Vista as noted in my comment above. 

Thanks for the quick turn-around. 

Comment 28

[bomfog]

(In reply to comment #26)
> Maybe we should do only an auto-vacuum. People with old sqlite files (where an
> auto-vacuum doesn't work) will have to recreate their profiles (or in case of
> places.sqlite, drop it and let is be recreated automatically). If you don't do
> that, then you just have to live with a places file that slowly grows.

Another reason I'd like history ex/import. Guess I'd better bite the bullet now, before I have any more history to lose.

Might be worth a heads-up in appropriate fora if that's the route taken. Other trunk testers with bigger histories they're loather to lose than I would probably prefer to cut their losses.

Comment 29

[Jo Hermans]

(In reply to comment #28)
> Another reason I'd like history ex/import. Guess I'd better bite the bullet
> now, before I have any more history to lose.
> 
> Might be worth a heads-up in appropriate fora if that's the route taken. Other
> trunk testers with bigger histories they're loather to lose than I would
> probably prefer to cut their losses.
> 

There's a better solution : download Storage Explorer <http://shawnwilsher.com/archives/119>, click on the places.sqlite entry, then type "vacuum;" as the command and press execute. That will do a full vacuum.

Comment 30

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

Comment on attachment 278556 [details] [diff] [review]
patch

patch is obsolete, as it was backed out (and it causes problems)

Comment 31

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

moving out to m10, as doing a vacuum is is known to lockup firefox.

Comment 32

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

We should stop worrying about the size of places.sqlite on disk.

The size of an un-vacuumed sqlite file (on disk) has nothing to do overrunning the sqlite cache (set by the cache_size PRAGMA).  (Dr. Hipp confirmed this.)

Dr. Hipp writes:

"The size of the database file is the minimum size
needed to hold all of the data when the volume of data was
the largest.  Database files do not shrink.  When you DELETE
or DROP data from the database, the information is zeroed
out (assuming you compile with SQLITE_SECURE_DELETE=1) and
added to a list of reusable areas.  Future INSERTs will reused
the freed space.  But the unused space is not handed back to
the operating system."

About autovacuum, which we were using but have since disabled (see bug #385834), Dr. Hipp writes:

"That all changes if you enable autovacuum.  With autovacuum,
the file does shrink when information is deleted.  In order
to pull this off, SQLite has to rearrange information within
the database to move the free space to the end of the file,
then truncate the file.  The rearragement takes time.  And
it causes additional fragmentation.  So it is not recommended
unless you really, really need it."

We're more concerned about perf than disk space, so we've taken Dr. Hipp's advice and disabled autovacuum.

About a full vacuum, dr. hipp writes:

"Vacuum works by making a brand new database containing the
same information, optimally packed, then copying the new
database over top of the old and truncating the file size
to the minimum necessary.  Vacuum takes temporary disk space
which can be as large as 3x the size of the original database.
(It usually takes less than 3x.  That's a worst case.)"

We've got other bugs concerning when we might be able to full vacuum.

Finally, when not doing autovacuuming, dr hipp writes:

"Free pages are not eliminated.  But they are reused when new
information is inserted.  So if the quantity of information in
the database is bounded, then eventually the SQLite database
file will reach a terminal size and grow no larger."

While our visits are now bounded (to 20,000 by default, per dietrich), other things (favicons, places, bookmarks) are not bounded, so place.sqlite will continue to grow.

Based on all of this, marking WONTFIX.

Comment 33

[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