Closed Bug 403702 Opened 13 years ago Closed 11 years ago

add "Compact" to "Organize" menu in the bookmarks organizer that does VACUUM (with progress)

Categories

(Firefox :: Bookmarks & History, enhancement)

enhancement
Not set

Tracking

()

RESOLVED DUPLICATE of bug 334010

People

(Reporter: moco, Unassigned)

References

Details

add "Compact" to "Organize" menu in the bookmarks organizer that does VACUUM (with progress)

as discussed repeately (see bug #395020 and bug #394379), we aren't doing VACUUM yet as it blocks the UI.

additionally, while vacuuming defrags the database, sqlite is good at reusing the freed page list, and I claim the typical user won't see / won't care about the size of the places.sqlite file on disk.

but, that said, we might find ourselves needed to recommend vacuum for users after we ship firefox 3.

note, we'd need a progress window for this (see bug #329736) as it can block the UI for a long period of time.

an irc discussion about this:

	sspitzerMsgMe	I just makred a vacuum bug from p1/m10 to p3/m11
	dietrich	yep, was just about to comment
	dietrich	that's fine, given our new understanding of vacuum
	sspitzerMsgMe	you should have gotten bug email.  given what we now know
	sspitzerMsgMe	right
	sspitzerMsgMe	ok, coolness
	dietrich	we really should get the "vacuum on upgrade" done though
	sspitzerMsgMe	(glad we are on the same page)
	sspitzerMsgMe	why do you think that is critical?
	dietrich	as well as the move those inc. vacuum folks off of that
	sspitzerMsgMe	well, keep in mind, that won't work.
	sspitzerMsgMe	unless we fix vacuum
	sspitzerMsgMe	to allow resetting of pragmas
	dietrich	right, drh's suggestion of change pragma
	dietrich	here's my thoughts: the most vocal and prolific testers we have all have inc. vacuum
	sspitzerMsgMe	so, we'd need that first.  and keep in mind, we'd have to vacuum all profiles (or do you mean, vacuum after first run after upgrade)
	dietrich	and they'll continually have probs
	sspitzerMsgMe	keep in mind, they might be configured for incremental vacuum, but we are no longer doing the incremental vacuum
	sspitzerMsgMe	(we were not set up for auto)
	sspitzerMsgMe	so they have databases configured for it, but we don't do it.
	dietrich	oh right, regular vacuum will still work
	sspitzerMsgMe	(anymore, on idle)
	dietrich	we don't need to un-inc.vacuum them
	sspitzerMsgMe	right.
	dietrich	ok, maybe my concern is unfounded.
	dietrich	i worry that ongoing usage will increase fragmentation to a point where vacuum will take so long that we don't want to do it
	dietrich	and then there'll be really popular extensions for vacuuming
	dietrich	:P
	sspitzerMsgMe	I think we discussed this:
	sspitzerMsgMe	some sort of file | compact
	sspitzerMsgMe	in the organizer
	sspitzerMsgMe	once we get prgress working
	dietrich	yes we did
	dietrich	maybe we should morph that bug
	dietrich	to cover that
	sspitzerMsgMe	or should I start a new one?
	dietrich	yes, that's a better plan
	sspitzerMsgMe	ok, doing that now.
	sspitzerMsgMe	also:
	sspitzerMsgMe	if we had a vacuum that could change pragmas, and if we had progress ui, we could do a vacuum on startup (do a schema bump).  I don't think we want to add this code to software update / installer.
	sspitzerMsgMe	multiple profiles on a machine or multiple profiles for a given user makes this very messy.
	sspitzerMsgMe	keep in mind, all we are gaining is a smaller places.sqlite file on disk.
	sspitzerMsgMe	(and sqlite should be re-using freed pages)
	sspitzerMsgMe	I guess, there is a price to be paid if you have a place.sqlite site up for incremental vacuum
	sspitzerMsgMe	dr. hipp tells me there is some overhead
	dietrich	yes, and new users should already have that smaller places.sqlite
	sspitzerMsgMe	but he made it sound like it was minimal.
	sspitzerMsgMe	as long as you don't do an incremental vacuum
	dietrich	hm
	sspitzerMsgMe	(or have auto enabled, and we don't)
	sspitzerMsgMe	I think we should log the bug about "file | compact" and punt on vacuum.  I could defniitely ship b2 even rtm without vacuum.
	dietrich	ok, i think we should change bug 395020 to be only about single-profile-vacuum-on-upgrade, as you just described
	dietrich	and block it on the progress UI bug
	sspitzerMsgMe	ok, will do
	sspitzerMsgMe	I'll post this chat log
	sspitzerMsgMe	in the bug as well
	sspitzerMsgMe	for reference.
	sspitzerMsgMe	ok?
	dietrich	right, new bug for vacuum on organizer
	sspitzerMsgMe	right, will do that too.
	dietrich	change bug 395020 and recommend de-blockerizing it
I believe you get more than just a smaller file - since it completely rebuilds you should have a very unfragemented file - which may over the long haul impact perf.  I know it does on postgress if you don't vacuum.

I don't think we need a progress UI at all .. just a spinning "i'm working" update with a cancel button would do it for me.  There are plenty of progress dialogs (like the updater dialog) that are so non-linear that they don't provide much value at all.

So anyway - giving users an easy way to vacuum with a simple cancel button seems like a big + easy win to me...
> I believe you get more than just a smaller file

yes, you'd get a smaller file.

as marco mentions in another bug: "since sqlite uses freelists to speed up inserts", so that's something to keep in mind.

> I don't think we need a progress UI at all .. just a spinning "i'm working" update with a cancel button 
> would do it for me. 

that's the sort of progress UI I'm talking about. 

We won't know how long vacuum will take (dr hipp had some ideas on how to "guess", but he quickly pointed out the flaws with the suggestion).  But, we can get called back on every <n>  SQLite virtual machine opcodes to prevent starving the UI thread (and to process the "cancel".

> So anyway - giving users an easy way to vacuum with a simple cancel button
> seems like a big + easy win to me...

not sure how easy it would be, but I agree, I think this could pay dividends later, if we ever realize that performance problem xyz has workaround:  vacuum.  

(note, if you google for vacuum and Mail.app, you'll see this sort of thing has happened before.)
I'm not a big fan of adding UI to let users work around bugs :/

Can we estimate the amount of space that will be saved, and either show that in the UI or to decide when to vacuum automatically?

Can we fix SQLite so that it can vacuum (or do something very similar to vacuuming) without blocking?  Or fix its "incremental vacuum" feature to not suck?

Why only bookmarks and not our other databases?

See also bug 205756, "Compact folders should be enabled by default" for mail.
some note:

vacuum time is not connected to how much the file is fragmented. 
Vacuum takes its time based on the size of the DB and the number of indexes on it, since everything it does is creating a new db, attaching it to the current connection, moving everything to it, then replacing the old db with the new. If you defragment non fragmented db it will take "about" the same time.

places.sqlite is the worst fragmentation case because in moz_places and moz_history and moz_favicons there will be a lot of inserts and deletes, while in other DBs they will be a limited amount. Also since places.sqlite contains a high number of tables with uris, titles, and favicons it is big, and it is a better target for vacuuming. but it should not be done too frequently, sqlite does already a good work with freelist. 
Imho the idea to do a vacuum on upgrade is the best since the user is ready to wait some minute, but surely with a progress bar!
Definetely all files will need a vacuum sometime in the future, only it's not now, could be in a year or two, since they are small files it will take a few seconds on each...

There is some way to guess the fragmentation percentage, the tool 	
POPFile SQLite Database Analyser Utility (http://www.sugelan.co.uk/popfile/utilities.html#analysesqlite) makes some move toward this, it can count orphaned pages, freelists and fragmentation into a db, unfortunately it's not open source, so don't know if it uses sqlite_analize or raw access.
Some similar way could be achieved, but the best thing would be sqlite itself provide a function to guess the fragmentation of a table or a db, since it probably requires raw access to the file contents, layers and pages.
I don't like the idea of automatically vacuuming as part of the upgrade process.

* Security updates are important, and I don't want users to put them off because they're slow.  The fact that the users already have to wait a minute during Firefox updates is a bug, IMO; see bug 307181.

* Performance shouldn't rely on updates being regular.  There's currently no guarantee that they'll be regular.
humm dupe of Bug 334010?
i'm duping, we are handling vacuum different way btw (bug 512854)
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 334010
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.