332748 - Limit history to 20000 entries (new pref browser.history_expire_visits) in addition to 180 days

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Brett Wilson]

Comment 1

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

this is an interesting bug.

ispiked has asked me on a couple occasions if we will be bumping up the default from 9 days to more in fx 3.

full disclosure with large histories, we have some performance issues to worry about.

Comment 2

[Mike Connor [:mconnor]]

We need to address those perf issues, regardless of the default.

I've proposed 180 days, and I'd like to make that change for b1 so users start building up more history so we can get more feedback on perf issues.

Comment 3

[Benjamin Smedberg]

Why 180? I suspect history could be much more valuable than that, and we should default to 18-24 months.

Comment 4

[Brett Wilson]

The database will get larger and larger the more time you store. This will count toward the memory footprint of the app. We'll spill onto disk if it won't fit in some limit (there is a computation for how much memory we're willing to use for this). However, the performance of a non-in-memory DB could be extremely bad, and it is used for all sort of critical stuff. It should be avoided if at all possible.

Comment 5

[Benjamin Smedberg]

If that's the case, can we limit it to a finite number of entries, rather than a time interval?

Comment 6

[Mike Connor [:mconnor]]

Seth, can you attach a patch to bump to 180 days for M7, and we can tweak up or down afterwards?

Comment 7

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

Attachment: patch

here's the patch, at mconnor's request.

note, I haven't done any investigation (as suggested by brett and bsmedberg) about what's the right value would be.

Comment 8

[Mike Connor [:mconnor]]

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

woo, let's ship it

Comment 9

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

fix checked in.

Checking in firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.187; previous revision: 1.186
done

but note, since we haven't figure out what the right value here is (or addressed brett's warning), I think we should move this to m8 (or later).

Comment 10

[Robert Accettura [:raccettura]]

This should be noted somewhat well in the release notes.  I know some people aren't going to want their history to stretch this far back.  No interval will satisfy everyone.  Regardless, this could have privacy concerns for some.

Comment 11

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

> This should be noted somewhat well in the release notes.

Good point, thank you.  Added to http://wiki.mozilla.org/Firefox3/M7/ReleaseNotes:

Privacy / History
* Remembering visited pages default changed from 9 days to 180 days (bug 332748).  

Comment 12

[Peter Kasting]

Incidentally, since I'm someone who used to run with huge history storage intervals, I can tell you that the biggest negative effect I found was that once you had a significant amount of history data, the first time a Firefox instance tried to open the History menu it would basically hang for long periods of time.  I couldn't really tell if the time was linear w.r.t. the amount of history data or exponential, but the problem eventually became severe enough to force me to clear all my data and set a much lower timeout (of around 20 days IIRC).

I don't know what the effect will be on trunk, since this was all with Fx 1.5 and 2.0, pre-places.

Comment 13

[Adam Guthrie]

Peter, that issue was fixed over in bug 385397.

Comment 14

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

>the first time a Firefox instance tried to open the History menu it would
>basically hang for long periods of time

yes, fx 1.5 / 2 has this problem, as did the trunk until recently.  But it's
been fixed on the trunk, see bug #385397 (as adam pointed out).

there are no plans to fix it on the MOZILLA_1_8_BRANCH for fx 2, for mork based history.

But note, with a large history, there are still other known performance issues
on the trunk, for example:

1)  url bar autocomplete performance, but the fix in bug #389491 will address
that.

2)  history sidebar performance.  things can be very slow (fx 3 blocking slow).
 See bug #385245 that tracks this and includes some ideas on how to improve it.

3)  additionally, as brett points out in comment #4 in this bug, there are
other costs of having a huge places.sqlite file which we have not yet
determined or addressed.

Comment 15

[Peter Kasting]

Yeah, I saw bug 385397 right after commenting and felt dumb, but didn't want to spam people by apologizing :)

However, since multiple other people have now pointed it out: Sorry!

Comment 16

[ti]

Additionally:

The back button dropdown should preserve more history as well. Currently it only shows about a dozen items but when going back, more are available (very bad UI), I think it should be more similar to the tab dropdown. However, the total history preserved is also way too low and I often find myself during daily use wanting to go back further than it allows.

Comment 17

[ti]

^^^ above mentioned issue filed as separate bug #390991

Comment 18

[Mike Connor [:mconnor]]

Are we going to solve this in M8?  Or should we wait until we have a better picture on history size vs. perf?

Comment 19

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

the default change to 180 days was made on 2007-07-25, so it's only been about 30 days, so nightly testers are just starting to get bigger data sets (by default.)

In addition to comparing fx 2 with n days of history to fx 3 with n days of history (so we can hopefully say:  fx 3 is faster with large history!), I really think we need to do some tests and compare fx 2 with 9 day history (the default) to 180 day history (our fx 3 default)

An end user won't know that we've gone from 9 days to 180 by default.  so if fx 3 is slower or (after a month or three months), they won't know why.

IE does 20 days, opera appears to keep 500 pages, and I'm not sure yet what safari does.

180 makes me nervous, as it takes a long time to figure out we're in trouble.  I'd recommend we set the default to 30 days, do our tests, and continue to evaluate it.

Comment 20

[Mike Connor [:mconnor]]

I don't think Fx2 with n days of history compared to Fx3 with n days of history can be an apples to apples comparison.  We should be able to do Fx2 w/defaults vs. Fx3 w/defaults as an overall comparison, and tweak as needed, but that's a much wider scope.

30 days seems very short and conservative, I'd rather scale back later in the cycle than earlier in any case, since one of the main goals I have for Places is to remember as much history as possible to make searching etc much more useful than with the 9 day default.

So, let's punt on this for the time being, M8 isn't a beta, we can discuss this more instead of right before freeze.

Comment 21

[Brett Wilson]

(In reply to comment #20)

Please see bug 373256 comment 24. It is easy to compute how much history you can keep because history must fit in the cache. See how much database size heavy users get per period of time, and then decide how much memory you are willing to sacrifice for the cache. The 

Having a DB that doesn't fit in the cache will require some fundamental design changes.

Comment 22

[Jo Hermans]

Note that by default we cache 6% of physical memory - that's 60MB for a 1GB installation. Isn't that a bit too large, considering that the number of history is now 180 days ? We might end up with 60MB in memory, if the places.sqlite file is growing larger than 60MB.

Comment 23

[Mike Schroepfer]

Recalling a quick hallway convo with Spitzer - couple of quick thoughts:

1) Doing this based on number of entries rather than date range seems more ideal because:
  a) we can measure the db size per entry and pick a value that is likely to be in cache for common cases.  If we use a date range actual performance can vary from person to person based on usage.
  b) In many cases I find the date range frustrating.   If I'm on vacation for a week and don't browse why should history entries get retired.

2) Dunno we can pay less per visit than unique site.

Anyway - can we do a quick test to figure out what max visit count makes sense for a system with 512mb of total ram and set the value to that?

Comment 24

[Mike Schroepfer]

As an aside the history sidebar date range isn't terribly useful since "Older than 6 days" is the last date range bucket and for me that list is incredibly long...

Comment 25

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

> As an aside the history sidebar date range isn't terribly useful

There are (at least) three bugs on improving that, see bug #393506, bug #387730 and bug #390614

Comment 26

[Mike Schroepfer]

Attachment: sqlite_analyze results for 50MB places file

My db is about 50MB on disk and if I'm reading the min(visit_time) from moz_historyvisits right this history goes back to Mon, 29 Jan 2007 05:51:46 GMT.   MOZ_PLACES+MOZ_HISTORYVISITS (with index)= 90.5% of this size or around 45MB.   

MOZ_PLACES (+ index) takes an average of 78 bytes per entry (total of 40Meg)
MOZ_HISTORYVISITS (+ index) takes an average of 12 bytes per entry (total of 6.5meg)

I've got 55k entries in MOZ_PLACES and 92k in History visits.  

So, from this totally skewed data set, 10k limit on MOZ_PLACES would keep it to around 7-8 MB, 40k for visits would keep that around 2-3MB - giving us a max size around 10MB.  This would keep us well under the cache size of even a 512mb system.

Comment 27

[Brett Wilson]

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.

Comment 28

[Marco Bonardo [:mak]]

notice that in moz_places there is an index on URL, and that is duplicating all urls 2 times, and they can be long. And an index on TITLE that is used nowhere. that index is not really useful atm, see https://bugzilla.mozilla.org/attachment.cgi?id=285291 for a deeper investigation. Dropping some "not so good" index could help limiting the db size, without loosing performance.

Comment 29

[Jo Hermans]

see also bug 398333 for a side-effect of storing 180 days of history : the sqlite file from comment 26 will be using a cache-size of 50MB in memory

Comment 30

[Marco Bonardo [:mak]]

oops, sorry wrong link, i was talking about Bug 381795

Comment 31

[Mike Connor [:mconnor]]

I don't think we can realistically solve this with the necessary UI bits for b1, but its critical to solve ASAP.

Some notes:

- Dietrich has a fix for pruning old entries to ensure we don't overshoot cache size, I think Brett's optimization makes sense in the context of that work.  We're basically going to store "up to N days" instead of "N days" of history, but this will still be a LOT of history.  I think the cache limit should be fixed, not based on memory available, we got burned by that on bfcache.  People with monster systems are more likely to react badly to using a ton of memory unless they opt in.

- We need to figure out how to allow users to either up their cache limit or accept that history will be pruned if they're importing a massive history DB from Firefox 2.  Schrep's 2 years of history is a prime example, that'll stay inside the current cache on , but at the price of 50 MB of memory usage.  That's not the right thing for typical users, but those who want to make that trade should be able to make that tradeoff themselves.

- Favicons and annotations will chew up space at less-predictable rates.  We can fix favicons for the most part by ensuring we cherry pick the 16x16 32bit version and only save that (dolske should be able to help with this).  For annotations, I think we need to expose an API for extensions using the anno service to request a cache limit increase of X MB to accomodate their usage, rather than guessing at how much we should allow by default for annos outside of what we use.  We can implement an overall cap for this.

Based on schrep's data, I'd like to aim for setting an overall limit of 15 MB in b2 as a baseline cap for all users, and figure out how to enable raising this cap where mentioned above.

Comment 32

[Mike Schroepfer]

(In reply to comment #31)
> I don't think we can realistically solve this with the necessary UI bits for
> b1, but its critical to solve ASAP.

Dumb question - what UI bits do we need for b1?  Seems like we just need to set a limit for b1 that doesn't cause horrendously large sqlite files.

> - Dietrich has a fix for pruning old entries to ensure we don't overshoot cache
> size, I think Brett's optimization makes sense in the context of that work. 
> We're basically going to store "up to N days" instead of "N days" of history,
> but this will still be a LOT of history.  I think the cache limit should be
> fixed, not based on memory available, we got burned by that on bfcache.  People
> with monster systems are more likely to react badly to using a ton of memory
> unless they opt in.

Right - again - why are we not setting this based on total number of visits.   Doing so gives us deterministic cache size.

> - We need to figure out how to allow users to either up their cache limit or
> accept that history will be pruned if they're importing a massive history DB
> from Firefox 2.  Schrep's 2 years of history is a prime example, that'll stay
> inside the current cache on , but at the price of 50 MB of memory usage. 
> That's not the right thing for typical users, but those who want to make that
> trade should be able to make that tradeoff themselves.
> 

Sure.  I'd settle for about:config allowing me to up total # of visits or a pref pane.  You decide if it is common enough to be in main prefs.


> - Favicons and annotations will chew up space at less-predictable rates.  We
> can fix favicons for the most part by ensuring we cherry pick the 16x16 32bit
> version and only save that (dolske should be able to help with this).  For
> annotations, I think we need to expose an API for extensions using the anno
> service to request a cache limit increase of X MB to accomodate their usage,
> rather than guessing at how much we should allow by default for annos outside
> of what we use.  We can implement an overall cap for this.

Based on my data Favicons are not significant.  
 
> Based on schrep's data, I'd like to aim for setting an overall limit of 15 MB
> in b2 as a baseline cap for all users, and figure out how to enable raising
> this cap where mentioned above.
> 

If we can't figure out the best solution - can we do something (either date or # of entries) we know will at least perform well for b1?   We should make sure "perform well" is true on lower (512mb-1gb) not brand-new MBP systems...

Comment 33

[Dietrich Ayala (:dietrich)]

One problem with Schrep's figures is that the file size might not represent the size of the actual data set if the file was created before A8, when incremental vacuum landed. When we dropped and recreated the moz_places table in A7 (iirc), some nightly testers reported that their places.sqlite nearly doubled in size, until manually vacuumed.

Many new users, and users upgrading to B1 from Fx2, may not see the performance issues that occur due to outgrowing the cache, because they 1) will not have the schema upgrade detritus and 2) can benefit from incremental vacuum during idle periods.

To some extent, this is a flaw in our testing environment: nightly testers who move from alpha to alpha have a database file that is not at all representative of average (or even hardcore) users who only upgrade at >=beta releases, both in size and configuration.

> 
> Right - again - why are we not setting this based on total number of visits.  
> Doing so gives us deterministic cache size.

Maybe for B1 we could:

1. Cap history at 40k visits or 180 days, whichever is hit first. Add a pref for the visit ceiling.

2. Use a deterministic cache size, or reduce our current 6% default.

3. Educate testers about vacuuming and browser.history_cache_percentage, as they all have old places.sqlite files.

We can relnote these changes for B1, and figure out UI (and/or change policy) in B2 based on the B1 feedback.

Comment 34

[Brett Wilson]

From the sqlite documentation: "Auto-vacuum does not defragment the database nor repack individual database pages the way that the VACUUM command does. In fact, because it moves pages around within the file, auto-vacuum can actually make fragmentation worse."

I assume fragmentation mans additional wasted space.

Comment 35

[Mike Schroepfer]

(In reply to comment #33)
> One problem with Schrep's figures is that the file size might not represent the
> size of the actual data set if the file was created before A8, when incremental
> vacuum landed. When we dropped and recreated the moz_places table in A7 (iirc),
> some nightly testers reported that their places.sqlite nearly doubled in size,
> until manually vacuumed.
> 
> Many new users, and users upgrading to B1 from Fx2, may not see the performance
> issues that occur due to outgrowing the cache, because they 1) will not have
> the schema upgrade detritus and 2) can benefit from incremental vacuum during
> idle periods.

Just manually vaccumed. New analyze file attached. File size dropped just slightly to 49MB.  So that didn't solve it. 
 
> Maybe for B1 we could:
> 
> 1. Cap history at 40k visits or 180 days, whichever is hit first. Add a pref
> for the visit ceiling.

Like the approach - not sure about the choices - why so high?
 
> 2. Use a deterministic cache size, or reduce our current 6% default.
> 3. Educate testers about vacuuming and browser.history_cache_percentage, as
> they all have old places.sqlite files.
> 
> We can relnote these changes for B1, and figure out UI (and/or change policy)
> in B2 based on the B1 feedback.
> 
++ to the rest of this.

Comment 36

[Mike Schroepfer]

Attachment: Analyze results after vacuum

Comment 37

[Dietrich Ayala (:dietrich)]

(In reply to comment #34)
> From the sqlite documentation: "Auto-vacuum does not defragment the database
> nor repack individual database pages the way that the VACUUM command does. In
> fact, because it moves pages around within the file, auto-vacuum can actually
> make fragmentation worse."
> 
> I assume fragmentation mans additional wasted space.
> 

Incremental vacuum does not defragment, but it does truncate the file, removing unused space.

Comment 38

[Dietrich Ayala (:dietrich)]

> > Maybe for B1 we could:
> > 
> > 1. Cap history at 40k visits or 180 days, whichever is hit first. Add a pref
> > for the visit ceiling.
> 
> Like the approach - not sure about the choices - why so high?

I don't see a point in reducing the maximum days kept given that we'll have the other deterministic limit in place. Keeping a high max days takes care of the scenario where we want to retain history for the user who goes on vacation for a month.

About the visits cap: moz_historyvisits in your db was 91k entries, but only 13% of total db size, so capping it very low won't have a large affect on file size. The numbers in your attachments show that moz_places was almost 80% of the db, but w/o indexes was only 30%. In order to radically reduce file size our efforts will be best spent fixing bug 381795 (comments about this below).

Also note that we want to optimize moz_places but not directly cap it. moz_places can grow due to non-history-related activity such as bookmark sync, etc. Capping visits will reduce the size of moz_places as a by product of expiration, as entries that are not bookmarked or visited will be removed at shutdown.


(In reply to comment #28)
> notice that in moz_places there is an index on URL, and that is duplicating all
> urls 2 times, and they can be long. And an index on TITLE that is used nowhere.
> that index is not really useful atm, see
> https://bugzilla.mozilla.org/attachment.cgi?id=285291 for a deeper
> investigation. Dropping some "not so good" index could help limiting the db
> size, without loosing performance.
> 

I think you meant bug 381795?

Lots of helpful analysis there, thanks Marco. I'd rather not make any changes that require a schema upgrade this close to freeze, but these might be the most successful changes in reducing file size. However, definitely considering dropping the moz_places.title index, as that won't require a schema revision.

(In reply to comment #27)
> 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.
> 

Yep, expiring entries that are not top-level would help tremendously, especially in reducing the size of the visits table. I filed bug 401722 for this.

Comment 39

[Dietrich Ayala (:dietrich)]

Attachment: wip

- Adds pref browser.history_expire_visits.
- Expire the oldest items outside the cap, then expire visits outside the date range.
- Adds a LIMIT to the date-based query. currently fetching all, and filtering post-query, which is nonsense.
- Kick off expiration whenever the vacuum timer hits, to speed expiration and to reduce file size more often (catch up when user is not active).
- Kick off expiration whenever the prefs change.
- Always run expiration (removed bogus conservatism).

Comment 40

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

- changes visit cap to 20k from 40k. this was done to take into account that a large percentage of moz_places and moz_historyvisits contains embedded records (eg: a self-reloading ad in an iframe), for which the only use-case for keeping it is inter-frame link coloring.

- more aggressive expiration at idle time and shutdown. this allows us to catch up if we've fallen behind, but doesn't affect perf if we are caught up.


Note: A scenario this doesn't yet support is when a user has a custom history_expire_days - we should respect that, and toss our visit cap, using their custom value instead. If this sounds good, i'll add it to the patch.

Also, in B2 we'll need to reconcile our wording around the days UI, to indicate that the visit cap takes precedence.

Comment 41

[Dietrich Ayala (:dietrich)]

Attachment: fix v2

uses custom history_expire_days instead of visit cap.

Comment 42

[Dietrich Ayala (:dietrich)]

Attachment: fix v2.1

with tests

Comment 43

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

+  nsCOMPtr<nsIPrefBranch> defaultPrefBranch;
+  rv = prefService->GetDefaultBranch(PREF_BRANCH_BASE,
+    getter_AddRefs(defaultPrefBranch));
+  NS_ENSURE_SUCCESS(rv, rv);
+  PRInt32 defaultExpireDays;
+  rv = defaultPrefBranch->GetIntPref(PREF_BROWSER_HISTORY_EXPIRE_DAYS, &defaultExpireDays);
+  NS_ENSURE_SUCCESS(rv, rv);

why the default branch?  doesn't this mean the user won't be able to override that pref value?  

I think you want GetBranch(), not GetDefaultBranch()

Comment 44

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

> why the default branch?

oops, now I see why.  ignore that comment.

Comment 45

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

an initial review.  please see comment / question 8 and 9 below.

0)

+  // This transaction is important for performance. It makes the DB flush
+  // everything to disk in one larger operation rather than many small ones.
+  // Note that this transaction always commits.
+  mozStorageTransaction transaction(connection, PR_TRUE);


curious, why'd you move that code?

1)

+    nsCAutoString sqlVisits;
+    sqlVisits.Assign(sqlBase);

any special reason why sqlVisits was an autostring but sql base is a cstring (heap)


2) 

+    if (aNumToExpire != 0) {

style nit, consider instead:

+    if (aNumToExpire) {

3)

+      sqlVisits.AppendLiteral("ORDER BY v.visit_date DESC ");
+      sqlVisits.AppendLiteral("LIMIT ?1 OFFSET ?2 ");
+    }

why two append literals (and not one?)


4)

+
+    if (aNumToExpire != 0) {

style nit, consider instead:

+    if (aNumToExpire) {

5)


+  if (aExpireThreshold != 0 && aRecords.Length() < aNumToExpire) {

style nit, consider instead:

+  if (aExpireThreshold && aRecords.Length() < aNumToExpire) {

6)

aNumToExpire is a PRUint32, but we are binding as an Int64:

rv = visitsStatement->BindInt64Parameter(0, aNumToExpire);


7)

mExpireVisits is a PRInt32, but we are binding as an Int64

rv = visitsStatement->BindInt64Parameter(1, mHistory->mExpireVisits);

8)

if I've changed my expired days setting from the default, we won't expire by the visit cap

but if we call FindVisits() with a aExpireThreshHold of zero, we also won't expire by date

before;

-  sql.AssignLiteral("SELECT "
-      "v.id, v.place_id, v.visit_date, h.url, h.favicon_id, h.hidden, b.fk "
-      "FROM moz_historyvisits v LEFT JOIN moz_places h ON v.place_id = h.id "
-      "LEFT OUTER JOIN moz_bookmarks b on v.place_id = b.fk AND b.type = ?1 ");
-  if (aExpireThreshold != 0)
-    sql.AppendLiteral(" WHERE visit_date < ?2");


after:


+    sqlDate.AssignLiteral("SELECT "
+        "v.id, v.place_id, v.visit_date, h.url, h.favicon_id, h.hidden "
+        "FROM moz_historyvisits v LEFT JOIN moz_places h ON v.place_id = h.id "
+        "LEFT OUTER JOIN moz_bookmarks b on v.place_id = b.fk "
+        "AND visit_date < ?1 LIMIT ?2");

so I think this means expiring all history no longer works if my expired days pref has changed.  or am I mistaken?

9)

also, why did you remove b.type (and bind to type bookmark) from that query?

Comment 46

[Dietrich Ayala (:dietrich)]

Attachment: fix v3

> +  // This transaction is important for performance. It makes the DB flush
> +  // everything to disk in one larger operation rather than many small ones.
> +  // Note that this transaction always commits.
> +  mozStorageTransaction transaction(connection, PR_TRUE);
> 
> curious, why'd you move that code?

The read query doesn't need to be in the transaction. It just ends up making the db locked longer.

Hm, actually, since that's a deferred transaction, the lock won't start until the first write query, so this change doesn't need to be made at all. I'll revert that.

> 
> so I think this means expiring all history no longer works if my expired days
> pref has changed.  or am I mistaken?

gah, fixed.

> 
> 9)
> 
> also, why did you remove b.type (and bind to type bookmark) from that query?
> 

The only bookmark type that has a value in the fk column is a bookmark, therefore the type distinction is unnecessary.

Comment 47

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

Comment on attachment 287249 [details] [diff] [review]
fix v3

sorry for the delay here, dietrich and for not spotting some of these things earlier:

1)

+  // If history_expire_days is the default then go ahead and expire up to the
+  // visit cap, else we respect the user's value, or if we're clearing history.
+  if (defaultExpireDays == mHistory->mExpireDays || !aNumToExpire) {
+    // build capped query
+    nsCOMPtr<mozIStorageStatement> visitsStatement;
+    nsCAutoString sqlVisits;
+    sqlVisits.Assign(sqlBase);
+    if (aNumToExpire) {
+      sqlVisits.AppendLiteral("ORDER BY v.visit_date DESC LIMIT ?1 OFFSET ?2 ");
+    }
+    rv = aConnection->CreateStatement(sqlVisits, getter_AddRefs(visitsStatement));
     NS_ENSURE_SUCCESS(rv, rv);

Ah, it took me a minute, but I get your query.  You order by date descending, skip ahead to the mExpireVisits, and from there, limit to aNumToExpire items.

perhaps that's worth a comment?

(I assume if offset is greater than the total number of items, we get none back?)

2)

Previously you wrote:

> also, why did you remove b.type (and bind to type bookmark) from that query?
> 

The only bookmark type that has a value in the fk column is a bookmark,
therefore the type distinction is unnecessary.

So the select no longer returns b.fk:

+    sqlDate.AssignLiteral("SELECT "
+        "v.id, v.place_id, v.visit_date, h.url, h.favicon_id, h.hidden "
+        "FROM moz_historyvisits v LEFT JOIN moz_places h ON v.place_id = h.id "
+        "LEFT OUTER JOIN moz_bookmarks b on v.place_id = b.fk "
+        "AND visit_date < ?1 LIMIT ?2");


But see nsNavHistoryExpireRecord::nsNavHistoryExpireRecord(), where we try to get the 7th value and set it as the
"bookmarked" value in the nsNavHistoryExpireRecord struct.

Because it's an outer join with moz_bookmarks, Doesn't this mean you want the select to be:

+    sqlDate.AssignLiteral("SELECT "
+        "v.id, v.place_id, v.visit_date, h.url, h.favicon_id, h.hidden, b.fk "

3)


 nsNavHistory::OnIdle()
 {
+  // Keep the expiration engine chugging along
+  // prior to a possible vacuum.
+  PRBool dummy;
+  (void)mExpire.ExpireItems(200, &dummy);
+

we currently fire the idle timer every 5 minutes.  

so if I'm idle at all (even 1 second), when the timer fires, we'll call OnIdle(), and then we'll execute ExpireItems()

I think you want to add another define, like we have for vacuum, to decide on how much idle should we expire?

if (idleTime > EXPIRE_IDLE_TIME_IN_MSECS) {
  // Keep the expiration engine chugging along
  // prior to a possible vacuum.
  PRBool dummy;
  (void)mExpire.ExpireItems(200, &dummy);
}

Comment 48

[Dietrich Ayala (:dietrich)]

Attachment: fix v3

> 1)
...
> 
> Ah, it took me a minute, but I get your query.  You order by date descending,
> skip ahead to the mExpireVisits, and from there, limit to aNumToExpire items.
> 
> perhaps that's worth a comment?

added

> (I assume if offset is greater than the total number of items, we get none
> back?)

right

> 2)
...
> 
> Because it's an outer join with moz_bookmarks, Doesn't this mean you want the
> select to be:
> 
> +    sqlDate.AssignLiteral("SELECT "
> +        "v.id, v.place_id, v.visit_date, h.url, h.favicon_id, h.hidden, b.fk "

Oops, didn't mean to remove that bit, added it back in, thanks.

> 3)
...
> so if I'm idle at all (even 1 second), when the timer fires, we'll call
> OnIdle(), and then we'll execute ExpireItems()
> 
> I think you want to add another define, like we have for vacuum, to decide on
> how much idle should we expire?
> 

Fixed, also moved the amount to a define.

Comment 49

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

Comment on attachment 287327 [details] [diff] [review]
fix v3

r=sspitzer, thanks dietrich.

since you are using sqlBase in  nsNavHistoryExpire::FindVisits() now, can you remove this commented out code before checking in:

+    /*
+    sqlDate.AssignLiteral("SELECT "
+        "v.id, v.place_id, v.visit_date, h.url, h.favicon_id, h.hidden, b.fk "
+        "FROM moz_historyvisits v LEFT JOIN moz_places h ON v.place_id = h.id "
+        "LEFT OUTER JOIN moz_bookmarks b on v.place_id = b.fk "
+        "AND visit_date < ?1 LIMIT ?2");
+    */

Comment 50

[Dietrich Ayala (:dietrich)]

Checking in browser/app/profile/firefox.js;
/cvsroot/mozilla/browser/app/profile/firefox.js,v  <--  firefox.js
new revision: 1.217; previous revision: 1.216
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.184; previous revision: 1.183
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.110; previous revision: 1.109
done
Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v  <--  nsNavHistoryExpire.cpp
new revision: 1.20; previous revision: 1.19
done
Checking in toolkit/components/places/src/nsNavHistoryExpire.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.h,v  <--  nsNavHistoryExpire.h
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/components/places/tests/unit/test_expiration.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_expiration.js,v  <--  test_expiration.js
new revision: 1.8; previous revision: 1.7
done

Comment 51

[Dietrich Ayala (:dietrich)]

Attachment: as checked in

Comment 52

[Steffen Wilberg]

Tweaking summary to reflect what happened: Limit history to 20000 entries (new pref browser.history_expire_visits) in addition to 180 days.
Was: Figure out how much history we can show by default (browser.history_expire_days now set to 180 days)

Comment 53

[Tracy Walker [:tracy]]

I've got jay's history with about 15,000 entries. That's the largest I have. Could someone with a larger history either post it or verify this? Thanks.

Comment 54

[Aleksei Perepelkin [:chaos]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007110604 Minefield/3.0a9pre

verified fixed

Comment 55

[Tracy Walker [:tracy]]

per conversation with chaos in irc  he was able to confirm 20052 got reduced to 20000

Comment 57

[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