From bug 332748: Brett Wilson 2007-10-25 14:09:02 PDT "Also, you can more aggressively expire some pages. My history is mostly Gmail subframe navigations and ad subframes. Subframes that have only been visited once, and never in the last N days, are probably good candidates for more aggressive expiration, which will help keep history size smaller." Drivers: asking for blocking, as this is an easy and unobtrusive perf win. Anything that reduces query time in moz_places will positively affect perf of autocomplete, bookmarks and history search, etc.

going a step further: why keep track of TRANSITION_EMBED visits at all? I see that for bug #325241 brett added support for includeHidden, but we never use it (except in one of our unit tests). If we removed support for the includeHidden option, I think we could also remove support for TRANSITION_EMBED. (Note, I'm not suggesting we removed the hidden column from moz_places, just the ability to use includeHidden from a query option) We could then not to store TRANSITION_EMBED visits in the moz_historyvisits table. For existing users, we would also add code to remove any visits of visit_type 0 (see #375777) or visit_type 4 (TRANSITION_EMBED) from moz_historyvisits. In our queries, we'd still need "hidden <> 1", but we could remove all the "v.visit_type <> 0 AND v.visit_type <> 4" where clauses. For additional background, see also bug #318654

blocking+ since keeping the places.sqlite file size down has a number of big benefits

some notes: 40% of the moz_places entries are linked to visits that only have type=4 stevee had about 60%. presumably we store these for link coloring, but at those numbers, i'm doubting that the benefit outweighs the cost.

> presumably we store these for link coloring for link coloring, nsNavHistory::IsVisited() calls IsURIStringVisited(), which just uses the visit count

...and note that in nsNavHistory::AddVisit(), we don't care that aTransitionType is TRANSITION_EMBED when updating the visit count.

talked w/ seth more about this tonight: for link coloring, we only need to know whether there are visits or not. one option here is to only ever add a single visit entry for an embedded visit. this means that the historyvisits table won't be full of useless entries for the same reloading ad iframe, but instead will be mostly visits to top-level uris. this, combined with the cap on visit count in bug 332748 will help keep the visits table smaller and filled with relevant data.

this is pretty agressive, expires embedded visits after 10 days.

fwiw, I did this, started/shutdown so the moz_places entries were properly cleaned up, and I no longer can reproduce either of bug 333965 or bug 394079

as a followup, I had about 46k moz_places entries and 117k moz_historyvisits entries, and a DB size (vacuumed) of 49.2 MB. There was about 24.5k embedded link places URLs and 78k visits for those urls deleted all visits of type = 4 older than 10 days, which was a bit over 72k entries, started and shut down firefox, and that pruned about 21k moz_places URLs. After a vacuum, I was at 19.9 MB for places.sqlite, and everything feels a lot snappier.

Comment on attachment 286996 [details] [diff] [review] fix v1 r=sspitzer, with one suggestion and two comments: 1) can you make the query be: "DELETE FROM moz_historyvisits WHERE visit_date < ?1 AND (visit_type = ?2 OR visit_type = 0)" bug #375777 for why this is necessary. 2) 2) should we added a comment about how ExpireEmbeddedLinks() should come before ExpireHistoryParanoid()? we have to expire embedding links before ExpireHistoryParanoid(), so that for a given moz_place entry, if there are no visits, we'll remove it. 3) should we allow a hidden pref to configure this 10 day lifetime, or leave it hard coded?

seth's #1 and #2 fixed. re: #3, unless there's a better use-case than inter-frame link coloring, i think it's not worth the overhead of a pref.

Comment on attachment 287026 [details] [diff] [review] fix v2 drivers: places perf improvement, see comment #9.

Comment on attachment 287026 [details] [diff] [review] fix v2 r=sspitzer, thanks dietrich

Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v <-- nsNavHistoryExpire.cpp new revision: 1.19; previous revision: 1.18 done Checking in toolkit/components/places/src/nsNavHistoryExpire.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.h,v <-- nsNavHistoryExpire.h new revision: 1.4; previous revision: 1.3 done

Comment on attachment 287026 [details] [diff] [review] fix v2 per mconnor - doesn't need a= due to blocker status.

This seems to have caused a jump in trace_malloc_maxheap (>10%) on bm-xserve11: http://build-graphs.mozilla.org/graph/query.cgi?tbox=bm-xserve11.build.mozilla.org&testname=trace_malloc_maxheap&autoscale=1&size=&days=7&units=bytes&ltype=&points=&showpoint=&avg=1

see bug 401687 comment 42: I didn't see a corresponding jump on the other trace_malloc_maxheap machines. Is this a test machine artifact? It's a big jump and we need some sort of explanation. Looking closer at the times I agree this bug is a more likely culprit than bug 401687.

I've filed bug 402572 for the regression. I'd be glad to back it out for a few runs to see what happens.

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