Closed Bug 390244 Opened 14 years ago Closed 14 years ago

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

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
Firefox 3 beta2

People

(Reporter: moco, Assigned: moco)

References

Details

Attachments

(1 obsolete file)

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!)
Flags: blocking-firefox3?
That patch is good to go for M8
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
Flags: blocking-firefox3? → blocking-firefox3+
Seth, can you take this?
Assignee: nobody → sspitzer
> Seth, can you take this?

yes.  fix coming with bug #385834
Status: NEW → ASSIGNED
OS: Windows XP → All
Hardware: PC → All
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #278556 - Flags: review?(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.
Attachment #278556 - Flags: review?(dietrich) → review+
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.
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)
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
re-opening, had to back out, the tree was closed.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
> 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
Backed out due to tbox orange
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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?
Status: REOPENED → ASSIGNED
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.
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.
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
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
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)
> 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?
(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.
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?
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
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.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 3 M8 → Firefox 3 M9
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.
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. 
(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.
(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 on attachment 278556 [details] [diff] [review]
patch

patch is obsolete, as it was backed out (and it causes problems)
Attachment #278556 - Attachment is obsolete: true
moving out to m10, as doing a vacuum is is known to lockup firefox.
Status: REOPENED → ASSIGNED
Target Milestone: Firefox 3 M9 → Firefox 3 M10
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → WONTFIX
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.