Closed Bug 403702 Opened 17 years ago Closed 15 years ago

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

Categories

(Firefox :: Bookmarks & History, enhancement)

enhancement
Not set
normal

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: 15 years ago
Resolution: --- → DUPLICATE
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.