I'm starting posting the patches in pieces, first of all i'ìll post some refactoring and cleanup of history, then the new code. This way you will be able to provide feedback on my decisions in case. Actually i have the full code but i still need to: 1. squash 1 memory leak due to observing a prefBranch 2. write new expiration tests This part exposes the hidden pref that allows to change the cache to memory percentage (e.g. the database cache size used by Places), i need the value (in sync) in both history and expiration code to evaluate how much history we can retain, it will also somehow allow users to increase or decrease our history aggressivity and memory usage, but this is a secondary target. I will also provide an hidden max number of pages pref that will allow them to lock us to a fixed amount of pages. Notice i renamed the pref, even if it's usually wrong, i think it's fine here because it was an hidden and really rarely used pref, also since the meaning slightly changes (being used for expiration) it's better avoid using old values.

Since i need to get rid of browser_expire prefs, i can't rely on them to build date containers. So i've added a small method to get how many days exist between now and the first visit in the db. This is already tested through test_history_sidebar.js also removed a nowhere used member. I've renamed constants since i'm also cleaning up history file, constants should be grouped and with more meaningful names.

not strictly needed, but while removing properties i noticed this

not strictly needed, but cleanup of the startup path (Ts).

More cleanup, this touches parts of code i'll use later making code clearer

cleanups constants names, some comment, a couple warnings.

the above changes (till part 6) can be reviewed and applied already, from now on changes will be disruptive.

use correct pref branch to load pref (i will unify pref branches later).

since i'm going to remove all time constraints, we need a new pref to enable or disable history. This will be places.history.enabled true by default this patch also fixes the preferences privacy panel i can't ensure all tests across the sources will pass at this point, some failures could be catched in later patches.

for some strange astral connection Places toolkit tests are still passing, cool. This replaces onPageExpired with onDeleteVisits, why? Because onPageExpired was partially duplicating a functionality we already had in onDeleteURI, so it should really only notify on visits deletion, otherwise listeners have to handle twice the same event.

This patch removes all code depending on the old expiration code. Unfortunatly there is a bunch of other components depending on our preferences, so i'll need to provide separate patches to decouple them :(

i must admit my ignorance about photon, but if we don't have a pref it should not need to translate it...

satchel was using history pref if its own pref was not set, it should either provide a default pref or have a default value. i added a default value since i don't know how much a default pref is wanted.

Dolske pointed out i should migrate users with disabled history (expire_days = 0) to the new history.enabled pref. Annotating here as a reminder.

and most likely will have to backout bug 525700 from satchel.

unbitrot

Comment on attachment 412015 [details] [diff] [review] Part1: expose a default pref for cache_to_memory_percentage +++ b/browser/app/profile/firefox.js These preferences don't belong to firefox.js. I'm not sure if all.js is the right fallback though. I guess we can figure it out later. >+// The percentage of sytem memory the Places database can use. Out of the typo: system. s/the/that the/ >+// The value is effective after an application restart. nit: "Changes to this value are"... . >+// Acceptable values are included between 0 and 50. nit: "are between" >diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp >--- a/toolkit/components/places/src/nsNavHistory.cpp >+++ b/toolkit/components/places/src/nsNavHistory.cpp >-// Default (integer) value of PREF_DB_CACHE_PERCENTAGE from 0-100 >+#define PREF_CACHE_TO_MEMORY_PERCENTAGE "pl >-#define DEFAULT_DB_CACHE_PERCENTAGE 6 >+// Out of this cache SQLite will use at most the size of the DB file. >+#define DATABASE_DEFAULT_CACHE_TO_MEMORY_PERCENTAGE 6 nit: add a coma after "cache". >- // Compute the size of the database cache. >+ // Compute the size of the database cache using the device's memory size. >+ // We don't use PRAGMA default_cache_size, since the database could be moved >+ // among different devices and the value will adapt accordingly. nit: would >+ nsCOMPtr<nsIPrefBranch> prefs = >+ do_GetService("@mozilla.org/preferences-service;1"); > PRInt32 cachePercentage; >- if (NS_FAILED(mPrefBranch->GetIntPref(PREF_DB_CACHE_PERCENTAGE, >- &cachePercentage))) >- cachePercentage = DEFAULT_DB_CACHE_PERCENTAGE; >+ if (!prefs || NS_FAILED(prefs->GetIntPref(PREF_CACHE_TO_MEMORY_PERCENTAGE, >+ &cachePercentage))) I would not just fail silently on the (!perfs) case. At least add a warning. >+ cachePercentage = DATABASE_DEFAULT_CACHE_TO_MEMORY_PERCENTAGE; r=mano otherwise.

Comment on attachment 411966 [details] [diff] [review] Part2: Stop using history prefs to build date containers [Checkin: Comment 99] r=mano

Comment on attachment 411968 [details] [diff] [review] Part3: cleanup GetNow() >diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp >--- a/toolkit/components/places/src/nsNavHistory.cpp >+++ b/toolkit/components/places/src/nsNavHistory.cpp >@@ -144,8 +144,12 @@ using namespace mozilla::places; > // sqlite's default max page size. > #define DEFAULT_DB_PAGE_SIZE 4096 > >-// the value of mLastNow expires every 3 seconds >-#define HISTORY_EXPIRE_NOW_TIMEOUT (3 * PR_MSEC_PER_SEC) >+// We use a cached "now" value for some repeating stuff, so we avoid calling >+// PR_Now() too often. These are milliseconds between "now" cache refreshes. Please rephrase this ("In order to avoid..."). >diff --git a/toolkit/components/places/src/nsNavHistory.h b/toolkit/components/places/src/nsNavHistory.h >--- a/toolkit/components/places/src/nsNavHistory.h >+++ b/toolkit/components/places/src/nsNavHistory.h > nsresult LoadPrefs(PRBool aInitializing); > >- // Current time optimization >- PRTime mLastNow; >- PRBool mNowValid; >+ /** >+ * Calculates and returns value for mCachedNow. >+ * This is an hack to avoid calling PR_Now() too often, as is the case when >+ * we're asked the ageindays of many history entries in a row. A timer is >+ * set which will clear our valid flag after a short timeout. >+ */ nit: missing space before "A timer". Nice cleanup and docs. r=mano.

Comment on attachment 412392 [details] [diff] [review] Part5: Cleanup topics and observer service usage [Checkin: Comment 99] r=mano.

Fix preferences component b-c tests, as well.

fix a satchel xpcshell failure

Comment on attachment 411982 [details] [diff] [review] Part4: use a getter for last session id [Checkin: Comment 99] r=mano

Comment on attachment 412003 [details] [diff] [review] Part 6: generic constants and comments cleanup [Checkin: Comment 99] r=mano

(In reply to comment #27) > Created an attachment (id=412709) [details] > Part9 (satchel): fix satchel dependancies (including backout of 525700) > > fix a satchel xpcshell failure do you need review on this? or is this all ready to land now?

First 6 parts are reviewed and could land (i have patches with addressed comments locall), other parts are waiting, i have to finish all coding and testing before i can call those ready for review since i could still find some unexpected bug. Also some external component could depend on new history.enabled pref, so parts 7 to 14 (yes i'm there) will land later.

addressed comments

addressed comments

I attached all the patches i have (and ideally this should be everything), these are passing tests locally, but i've pushed to the tryserver both to check remaining test failures, get talos results, and a build to test. For the new component i'll probably ask multiple reviews, just to collect more feedback. i'll start asking reviews as soon as i get tryserver info.

Awesome, got a completely green Linux Unittests run, and a completely orange Windows Unittests run. And i cannot reproduce any of those Windows failures on Windows :(

trybuilds: http://build.mozilla.org/tryserver-builds/mak77@bonardo.net-try-dc7a64f196de

the builds in comment 44 are broken due to my mistake (forgot to add the component to the installer package). i started working on bug 531151 to understand if the annotations failures here are real or VM fakes.

fixed component name and installer

YAY! looks like i finally solved random oranges i was seeing, they were actually due to the fact unit boxes are practically always considering the user idle, and i was expiring on idle. Should start requesting reviews on part 7-13 soon.

this is the privacy preferences panel with the simple "on/off" remember history preference, in place of our current days pref.

Comment on attachment 416066 [details] [diff] [review] Part7: Provide a new preference to toggle history [Checkin: Comment 120] Johnath, i don't know if you're the best choice to review preferences dialogs changes, in case forward please.

asking SR since it's an idl change

Comment on attachment 412077 [details] [diff] [review] Part9 (libpref): fix all.js dependancies [Checkin: Comment 120] Mano, is your review enough here?

Comment on attachment 416074 [details] [diff] [review] Part10: Add a new expiration component better having a double review here, dietrich or shawn is fine, feel free to balance load.

got a full green on try server (unrelated media test random orange on os x, ), and the builds linked in whiteboard should be good for testing.

Comment on attachment 412081 [details] [diff] [review] Part9 (photon): Fix photon dependancies [Checkin: Comment 120] Sorry, not doing code reviews. (Haven't done one in a year or two!)

(In reply to comment #60) > (From update of attachment 412081 [details] [diff] [review]) > Sorry, not doing code reviews. (Haven't done one in a year or two!) indeed photon codebase is about one year old. Any idea who can i ask review on that?

Comment on attachment 414261 [details] [diff] [review] Part11: Fix download manager tests [Checkin: Comment 120] r=sdwilsh

Comment on attachment 416063 [details] bool history pref This regresses user control, but if I remember correctly from the earlier conversations this is the only way to solve larger issues, so giving a ui-r+ even though it technically makes the ui a bit worse.

Comment on attachment 416066 [details] [diff] [review] Part7: Provide a new preference to toggle history [Checkin: Comment 120] r=mano, but I think it's worth renaming the now-hidden days preference. My review used to be sufficient for changs in preferences-panels, given that you have ui-r.

Comment on attachment 412077 [details] [diff] [review] Part9 (libpref): fix all.js dependancies [Checkin: Comment 120] r=mano

Comment on attachment 416069 [details] [diff] [review] Part8: Change onPageExpired to onDeleteVisits [Checkin: Comment 120] r=mano. File a bug for SeaMonkey, if needed.

Comment on attachment 416071 [details] [diff] [review] Part9: remove old code, migrate old pref Please ignore my previous comment about renaming the pref. I read this wrong. > > // Window watcher for Library window. > var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"]. >@@ -318,8 +318,8 @@ function runNextTest() { > gBrowser.removeTabsProgressListener(gTabsListener); > > // Restore history. >- if (gPrefService.prefHasUserValue(DISABLE_HISTORY_PREF)) >- gPrefService.clearUserPref(DISABLE_HISTORY_PREF); >+ if (gPrefService.prefHasUserValue(ENABLE_HISTORY_PREF)) >+ gPrefService.clearUserPref(ENABLE_HISTORY_PREF); You don't need to call prefHasUserValue before calling clearUserPref. >+nsNavHistory::LoadPrefs() >+{ >+ if (!mPrefBranch) >+ return; >+ >+ // History preferences. >+ // Check the old preference and migrate disabled state. >+ nsCOMPtr<nsIPrefBranch> prefSvc = do_GetService(NS_PREFSERVICE_CONTRACTID); >+ PRInt32 oldDaysPref = 0; >+ if (prefSvc && >+ NS_SUCCEEDED(prefSvc->GetIntPref("browser.history_expire_days", >+ &oldDaysPref)) && >+ !oldDaysPref) { >+ // Clear the old pref, otherwise we will keep using it. >+ prefSvc->ClearUserPref("browser.history_expire_days"); >+ mPrefBranch->SetBoolPref(PREF_HISTORY_ENABLED, PR_FALSE); >+ mHistoryEnabled = PR_FALSE; >+ } >+ else >+ mPrefBranch->GetBoolPref(PREF_HISTORY_ENABLED, &mHistoryEnabled); >+ The old pref should be cleaned up regardless of its value. r=mano otherwise.

Some drive-by comments: +// Notice this is a lazy limit. This means we will start to expire if we will +// go over it, but we won't ensure that we will stop exactly when we reach it, +// instead we will stop after the next expiration step that will bring us +// below it. +// If this preference does not exist or has a negative value we will calculate nit: ", " before "we". +// The queries we use to expire. +const EXPIRATION_QUERIES Dietrich is a better reviewer for the actual queries. + let clearHistoryOnShutdown; + try { + let prefs = Cc["@mozilla.org/preferences-service;1"]. + getService(Ci.nsIPrefBranch); + clearHistoryOnShutdown = prefs.getBoolPref(PREF_SANITIZE_ON_SHUTDOWN) && + prefs.getBoolPref(PREF_SANITIZE_ITEM_HISTORY); + } + catch(e) { + clearHistoryOnShutdown = false; + } + nit: convention is to initialize the variable to the fallback value and leave the |catch| block empty. + if (!clearHistoryOnShutdown) { + let queries = [ ... + ]; Right now, this is an hardcoded list of everything that's in EXPIRATION_QUERIES. Unless I misunderstand it, you could loop EXPIRATION_QUERIES using |for (let x in EXPIRATION_QUERIES)|. + notify: function PEX_timerCallback() + { ... + let queries = [ ... + ]; But here you have 12 out of 14 queries. Maybe add an extra flag for each query in the queries array? + // Dispatch to history that we've finished expiring if we have results. + // We will notify just once per url + if ("_expiredResults" in this) { + if (!this._shuttingDown) { _shuttingDown should be set to false somewhere, otherwise this may raise the "undefined" warning. + // We will try to live inside the database cache size, since working out + // of it can be really slow. + let cache_percentage = PREF_CACHE_PER_MEMORY_PERCENTAGE_DEFAULT; + try { + let prefs = Cc["@mozilla.org/preferences-service;1"]. + getService(Ci.nsIPrefBranch); I wonder if we the branches. + cache_percentage = prefs.getIntPref(PREF_CACHE_PER_MEMORY_PERCENTAGE); + if (cache_percentage < 0) + cache_percentage = 0; + else if (cache_percentage > 50) + cache_percentage = 50; + } + catch(e) {} + let cachesize = memsize * cache_percentage / 100; nit: use braces for readability here. + * Execute async statements to expire with the specified queries. + * + * @param aQueryNames + * The names of the queries to use for this expiration step. + */ + _expireWithQueriesAndLimit: function PEX_expireWithQueriesAndLimit(aQueryNames, aLimit) + { + // Skip expiration during batch mode. + if (this._inBatchMode) + return; + See my comment on _shuttingDown. + /** + * Finalizes all of our mozIStorageStatements so we can properly close the + * database. + */ + _finalizeInternalStatements: function PEX_finalizeInternalStatements() + { + for each (let stmt in this._cachedStatements) { + if (stmt instanceof Ci.mozIStorageStatement) + stmt.finalize(); Er, what's the instanceof checkoff. This sort of errors shouldn't be caught. + _cachedStatements: [], + _getBindedStatement: function PEX_getBindedStatement(aQueryType, aLimit) + { + // Statements creation can be expensive, so try to cache them if possible. + if (!this._cachedStatements[aQueryType]) { + this._cachedStatements[aQueryType] = + this._db.createStatement(EXPIRATION_QUERIES[aQueryType]); + } + let stmt = this._cachedStatements[aQueryType]; + Better: let stmt = this._cachedStatements[aQueryType]; if (stmt === undefined) { stmt = this._cachedStatements[aQueryType] = … ; } + let baseLimit; + switch (aLimit) { + case LIMIT.NONE: + baseLimit = -1; + break; + case LIMIT.SMALL: + baseLimit = EXPIRE_LIMIT_PER_STEP; + break; + case LIMIT.LARGE: + baseLimit = EXPIRE_LIMIT_PER_STEP * EXPIRE_LIMIT_PER_LARGE_STEP_MULTIPLIER; + break; + case LIMIT.DEBUG: + baseLimit = this._debugLimit; + break; + } Couldn't you simply this code path by setting LIMIT.* to their associated values? + _newTimer: function PEX_newTimer() + { ... + let interval = (this._status == STATUS.CLEAN) ? ... + return (this._timer = timer); nit: remove the braces.

(In reply to comment #67) > You don't need to call prefHasUserValue before calling clearUserPref. But it would throw if prefHasUserValue is false.

Comment on attachment 416075 [details] [diff] [review] Part12: fix existing Places tests >diff --git a/toolkit/components/places/tests/unit/test_317472.js b/toolkit/components/places/tests/unit/test_317472.js >--- a/toolkit/components/places/tests/unit/test_317472.js >+++ b/toolkit/components/places/tests/unit/test_317472.js >-} >+} >\ No newline at end of file Fix that. >+function next_test() { ... >+ } >+} >\ No newline at end of file ditto. >+function tagURI(aURI, aTags) { >+ bmksvc.insertBookmark(bmksvc.unfiledBookmarksFolder, aURI, >+ bmksvc.DEFAULT_INDEX, "bleh"); Hrm, Shouldn't we move it to tagURI? Please file a bug. >+function next_test() { ... >+ else { >+ do_test_finished(); >+ } nit: remove the braces. r=mano.

(In reply to comment #69) > (In reply to comment #67) > > You don't need to call prefHasUserValue before calling clearUserPref. > But it would throw if prefHasUserValue is false. So do most of the not-so-nice old perf APIs. Thus, we wrap them in try-catch blocks.

Continuing comment 68. >+ lockFactory: function (aDoLock) {}, >+ QueryInterface: XPCOMUtils.generateQI([ >+ Ci.nsIFactory, >+ ]), Ci.nsIFactory, ? And on one line please. >+const PREF_MAX_PAGES = "max_pages"; >+const PREF_MAX_PAGES_DEFAULT = -1; // Automatic limit. >+ I don't like the naming. This is the *no*-max value. >+ // Determine whether we can skip shutdown expiration of dangling entries >+ // because we will execute a ClearHistory. >+ let clearHistoryOnShutdown; >+ try { >+ let prefs = Cc["@mozilla.org/preferences-service;1"]. >+ getService(Ci.nsIPrefBranch); >+ clearHistoryOnShutdown = prefs.getBoolPref(PREF_SANITIZE_ON_SHUTDOWN) && >+ prefs.getBoolPref(PREF_SANITIZE_ITEM_HISTORY); >+ } >+ catch(e) { >+ clearHistoryOnShutdown = false; >+ } >+ Can we somehow move this browser/ optimization to browser/? >+ if (url in this._expiredResults) { >+ this._expiredResults[url].url = url; … >+ this._expiredResults[url] = { >+ url: url, That seems wrong, somehow. >+ _loadPrefs: function PEX_loadPrefs() { ... >+ catch(e) {} >+ if (this._pagesLimit < 0) { nit: add a new line in between. >+ * Execute async statements to expire with the specified queries. >+ * >+ * @param aQueryNames >+ * The names of the queries to use for this expiration step. >+ */ >+ _expireWithQueriesAndLimit: function PEX_expireWithQueriesAndLimit(aQueryNames, aLimit) >+ { So, I prefer having a flag set on each query in the queries array, noting when it should run (and check against it with & etc).

Please add "(Original Author)" in the relevant places (also in the tests patch).

Comment on attachment 416076 [details] [diff] [review] Part13: New expiration tests First pass. + var provider = { ... + if (prop == NS_APP_USER_PROFILE_50_DIR) { + return dirSvc.get("CurProcD", Ci.nsIFile); + } nit: remove the braces. +function uri(spec) { Any chance we can move this, log etc. to a common file for places xpcshell tests? + // print the row nit: ucfirst and end with period. + stmt = null; +} What's this for? Also noticed it in few other methods. + +/** + * Function tests to see if the place associated with the bookmark with id + * aBookmarkId has the uri aExpectedURI. The event will call do_test_finished() + * if aFinish is true. nit: extra space. +function new_test_bookmark_uri_event(aBookmarkId, aExpectedURI, aExpected, aFinish) ... + do_check_false(stmt.executeStep()); + } nit: remove the braces. Ditto in other methods. +function DBConn() ... + try { + var dbConn = storageService.openDatabase(file); + } catch (ex) { + return null; + } + return dbConn; Let's make that: ---- try { return dbConn = storageService.openDatabase(file); } catch(ex) { } return null; ---- Otherwise this may raise the "not always returning" warning. +// Simulates am expiration at shutdown. an +function clearMaxPages() { + try { + return prefs.clearUserPref("places.history.expiration.max_pages"); + } catch(ex) {} clearUserPref is void. +function setHistoryEnabled(aHistoryEnabled) { ... + return aHistoryEnabled; +} This is not really a setter... +function clearHistoryEnabled() { + try { + return prefs.clearUserPref("places.history.enabled"); + } catch(ex) {} +} As above. diff --git a/toolkit/components/places/tests/expiration/test_annos_expire_policy.js b/toolkit/components/places/tests/expiration/test_annos_expire_policy.js new file mode 100644 --- /dev/null +++ b/toolkit/components/places/tests/expiration/test_annos_expire_policy.js + * + * Annotations can be set with a timed expiration policy. + * Supported policies are: + * - EXPIRE_DAYS: annotation will be expired after 7 days + * - EXPIRE_WEEKS: annotation will be expired after 30 days + * - EXPIRE_MONTHS: annotation will be expired after 180 days "would" and periods. +} \ No newline at end of file Please fix that everywhere.

just as a side note i had a brief talk with Faaborg and explained it why we think it is good to regress" user's controls. We are still available to provide better real-privacy UI features if needed, in place of old fake-privacy UI.

I'm bringing over reviews since it makes easier for me to check status of the various parts with new bugzilla "hide obsoletes" behavior (it should probably not hide r+ or a+ patches still, they're useful even if obsolete).

(In reply to comment #68) > + // We will try to live inside the database cache size, since working out > + // of it can be really slow. > + let cache_percentage = PREF_CACHE_PER_MEMORY_PERCENTAGE_DEFAULT; > + try { > + let prefs = Cc["@mozilla.org/preferences-service;1"]. > + getService(Ci.nsIPrefBranch); > > I wonder if we the branches. what? if we should unify them? like observing all places.history for changes? this value is read just once, so i'm not sure if observing a larger branch would be better. Unless you meant something else.

(In reply to comment #68) > + _finalizeInternalStatements: function PEX_finalizeInternalStatements() > + { > + for each (let stmt in this._cachedStatements) { > + if (stmt instanceof Ci.mozIStorageStatement) > + stmt.finalize(); > > Er, what's the instanceof checkoff. This sort of errors shouldn't be caught. I actually inherited this code path from a Shawn's component, so i thought to leave it in thinking it was to avoid some Storage race condition on finalization. But could be it's just paranoid, i'll ask him if it's possible for it to happen in Storage. > + let baseLimit; > + switch (aLimit) { > + case LIMIT.NONE: > + baseLimit = -1; > + break; > + case LIMIT.SMALL: > + baseLimit = EXPIRE_LIMIT_PER_STEP; > + break; > + case LIMIT.LARGE: > + baseLimit = EXPIRE_LIMIT_PER_STEP * > EXPIRE_LIMIT_PER_LARGE_STEP_MULTIPLIER; > + break; > + case LIMIT.DEBUG: > + baseLimit = this._debugLimit; > + break; > + } > > Couldn't you simply this code path by setting LIMIT.* to their associated > values? Ideally, but DEBUG limit can vary each time, and other limits are actually set to the consts but could change in future as we tweak them, so for now i preferred having them explicit till we reach values stability.

You don't have to observe the entire branch. It's also not-so-ideal that we have to change our code paths that much for debug-matter, but I cannot think of a better solution right now.

(In reply to comment #72) > >+ // Determine whether we can skip shutdown expiration of dangling entries > >+ // because we will execute a ClearHistory. > >+ let clearHistoryOnShutdown; > >+ try { > >+ let prefs = Cc["@mozilla.org/preferences-service;1"]. > >+ getService(Ci.nsIPrefBranch); > >+ clearHistoryOnShutdown = prefs.getBoolPref(PREF_SANITIZE_ON_SHUTDOWN) && > >+ prefs.getBoolPref(PREF_SANITIZE_ITEM_HISTORY); > >+ } > >+ catch(e) { > >+ clearHistoryOnShutdown = false; > >+ } > >+ > > Can we somehow move this browser/ optimization to browser/? I don't have interesting ideas to move that right now, we just run cleanup on shutdown, and we check to see if prefs are set, doing the opposite would still require us to listen to some browser event. Actually i'm not sure we are really saving some interesting time in this edge case, and we are also skipping expiration of session annotations in such a case, that is wrong. I'll just set status.CLEAN on clearHistory, and on shutdown skip some expiration if it's CLEAN, but not everything.

(In reply to comment #70) > >+function tagURI(aURI, aTags) { > >+ bmksvc.insertBookmark(bmksvc.unfiledBookmarksFolder, aURI, > >+ bmksvc.DEFAULT_INDEX, "bleh"); > > Hrm, Shouldn't we move it to tagURI? Please file a bug. ideally yes, we do that in PlacesTransactionsService for now, to be able to easily undo, if we do that in tagURI how do we know if the bookmark has been created by us of by the user?

(In reply to comment #74) > + stmt = null; > +} > > What's this for? Also noticed it in few other methods. i think it is to satisfy Shawn's paranoid-mode :) > +function new_test_bookmark_uri_event(aBookmarkId, aExpectedURI, aExpected, > aFinish) > ... > + do_check_false(stmt.executeStep()); > + } > > nit: remove the braces. Ditto in other methods. Actually, looking at coding style page on MDC instead (https://developer.mozilla.org/En/Developer_Guide/Coding_Style) it says "Always brace controlled statements, even a single-line consequent of an if else else." this is interesting. (this line was added by Brendan on August during the code style cleanup). Storage code style instead says "if a side of an if else if else requires braces, all sides will get braces". I think leaving braces there won't hurt both.

I actually did not change the pref branch use for various reasons: in some case GC could collect the prefs observer if we don't hold a ref to the branch in the component, changing it makes code more verbose, if we add more prefs in future to the expiration. branch this will already be working correctly, i'm not sure if having an observer on a branch with only 2 prefs is really slower then having 2 observers, one for each pref. Apart previous comments, where applicable, i should have fixed all the remaining ones.

Comment on attachment 417586 [details] [diff] [review] Part10: Add a new expiration component >diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp >--- a/toolkit/components/places/src/nsNavHistory.cpp >+++ b/toolkit/components/places/src/nsNavHistory.cpp >@@ -5540,6 +5540,16 @@ nsNavHistory::Observe(nsISupports *aSubj >+ // This covers a special case that can happen in tests, if the test >+ // does never interrupt the main thread we could receive xpcom-shutdown >+ // before we fire any notification, that means that if we notify now >+ // we will init the category cache after xpcom-shutdown, category >+ // observing services will be then initialized and leaked. >+ // We could shutdown earlier (see bug 529821) but also in such a case >+ // we could have async statements trying to notify after xpcom-shutdown, >+ // so, for now, we just avoid to notify from now on. >+ mCanNotify = false; >+ Shouldn't we turn in on in test-builds only then? Ditto for everything related to TOPIC_DEBUG_START_EXPIRATION (except the constant declaration). >diff --git a/toolkit/components/places/src/nsPlacesExpiration.js b/toolkit/components/places/src/nsPlacesExpiration.js >new file mode 100644 >--- /dev/null >+++ b/toolkit/components/places/src/nsPlacesExpiration.js >+const ANNOS_EXPIRE_POLICIES = [ >+ QueryInterface: XPCOMUtils.generateQI([ >+ _xpcom_categories: [ Remove the trailing commas. >+// Represents actions on which a query will run. >+const ACTION = { >+ TIMED: 1 << 0, >+ CLEARHISTORY: 1 << 1, >+ SHUTDOWN: 1 << 2, >+ CLEANSHUTDOWN: 1 << 3, I prefer CLEAR_HISTORY and CLEAN_SHUTDOWN. >+ ////////////////////////////////////////////////////////////////////////////// >+ //// nsINavHistoryObserver >+ >+ onBeginUpdateBatch: function PEX_onBeginUpdateBatch() >+ { >+ this._inBatchMode = true; >+ Please declare and initialize _inBatchMode above onBeginUpdateBatch. >+ onClearHistory: function PEX_onClearHistory() { >+ this._lastClearHistoryTime = Date.now(); And _lastClearHistoryTime above onClearHistory. >+ this._expiredResults[url] = { >+ url: url, This is still wrong. >+ * @returns a REPEATING_SLACK nsITimer that runs every this._syncInterval. >+ */ @return Looks good otherwise.

Comment on attachment 417589 [details] [diff] [review] Part13: New expiration tests >+ stmt.reset(); >+ stmt.finalize(); >+ stmt = null; >+} ... >+ stmt = null; ( * 2 ) >+ >+ if (aFinish) >+ do_test_finished(); >+} !!!!1!!1!!! >+ >+/** >+ * Function tests to see if the place associated with the bookmark with id nit: This function. >+ >+ { desc: "Add 10 pages, all bookmarked.", >+ addPages: 10, >+ addBookmarks: 10, >+ expectedNotifications: 0, // No expirable pages. >+ }, >+ >+]; Remove the trailing comma. r=mano otherwise.

(In reply to comment #86) > Shouldn't we turn in on in test-builds only then? > > Ditto for everything related to TOPIC_DEBUG_START_EXPIRATION (except the > constant declaration). hm, actually i don't know which defines to look into to limit this to test-builds. will look into it, and eventually produce a Part14 since i think i'd like to opt-out other code.

ENABLE_TESTS?

Comment on attachment 417586 [details] [diff] [review] Part10: Add a new expiration component please add an overview at the top of the component, that explains the general approach. otherwise people have to read 42 constants and 15 preferences to get an idea of the flow. >+// Max number of pages to retain in history. >+// Notice this is a lazy limit. This means we will start to expire if we will >+// go over it, but we won't ensure that we will stop exactly when we reach it, >+// instead we will stop after the next expiration step that will bring us >+// below it. >+// If this preference does not exist or has a negative value, we will calculate >+// a limit based on current hardware. >+const PREF_MAX_PAGES = "max_pages"; >+const PREF_MAX_PAGES_NOTSET = -1; // Use our internally calculated limit. by "pages" you're actually talking about unique URLs. i'd prefer that you use "unique URLs" in comments instead of pages, due to all the confusion we've had about how expiration actually works. update: after getting all the way through this patch, i'm convinced we need to s/page/URL/ everywhere. "page" is good for text the user sees, but in the code we're working with URLs. >+ >+// Seconds between each expiration step. >+const PREF_INTERVAL = "interval_seconds"; >+const PREF_INTERVAL_NOTSET = 3 * 60; why such a long interval? now that both adding visits and expiring visits will be on a background thread, neither should block UI at all. shouldn't we be expiring as much as we can, as often as we can? also, please indicate the unit of measurement in the const name. >+ >+// The percentage of system memory we will use for the database's cache. >+// Use the same value set in nsNavHistory.cpp >+const PREF_CACHE_PER_MEMORY_PERCENTAGE = "places.history.cache_per_memory_percentage"; >+const PREF_CACHE_PER_MEMORY_PERCENTAGE_NOTSET = 6; anything specific to sqlite should have "db" in there somewhere, so that it's clear what is being tweaked. >+ >+// Minimum number of pages to retain, in case system-info returns bogus values. >+const MIN_PAGES = 1000; >+ >+// Max number of items to expire at each expiration step. >+const EXPIRE_LIMIT_PER_STEP = 6; >+// When we run a large expiration step, the above limit is multiplied by this. >+const EXPIRE_LIMIT_PER_LARGE_STEP_MULTIPLIER = 10; what are "items" here? pages (urls) or visits? >+ >+// When history is clean or dirty enough we will adapt the expiration algorithm >+// to be more lazy or more aggressive. >+// This is done acting on the interval between expiration steps and the number >+// of expirable items. >+// - clean history: interval * EXPIRE_AGGRESSIVITY_MULTIPLIER >+// standard number of expirable items >+// - dirty history: standard interval >+// expirable items number * EXPIRE_AGGRESSIVITY_MULTIPLIER. >+const EXPIRE_AGGRESSIVITY_MULTIPLIER = 3; the somewhat-formula-looking part of this comment is really hard to follow. please simplify/clarify. >+ >+// Average size per page is got trough analyzing distribution of ratio >+// between number of pages and database size. We use a more pessimistic >+// ratio on single cores, since we handle some stuff in a separate thread. >+const PAGE_AVG_SIZE_MIN = 2000; >+const PAGE_AVG_SIZE_MAX = 4000; please explain that this is memory pages, and size is referring to number of bytes, and also explain what all of this is eventually used for. s/trough/through/ >+ >+// Seconds of idle time before starting a larger expiration step. >+// Notice during idle we stop the expiration timer since we don't want to hurt >+// stand-by or mobile devices batteries. >+const IDLE_TIMEOUT = 5 * 60; >+ >+// If a clear history ran just before we shutdown, we will skip most of the >+// expiration at shutdown. This is maximum number of seconds from last >+// clearHistory to decide to skip expiration at shutdown. >+const SHUTDOWN_WITH_RECENT_CLEARHISTORY_TIMEOUT = 10; in both the above const names, please indicate the unit of measurement. haven't reviewed the sql yet, will do once the renamings etc are done.

(In reply to comment #86) > Remove the trailing commas. We have started using them to avoid polluting blame in future, i'll leave them in unless you think they can cause any sort of issue. about ENABLE_TESTS, i'm not sure if it's worth the devtime cost to remove a couple lines (some of them are real bugs, even if actually we make impossible to hit them in a real app), btw if done, will be apart from Part10.

Comment on attachment 418306 [details] [diff] [review] Part10: Add a new expiration component >+// Seconds between each expiration step. >+const PREF_INTERVAL_SECONDS = "interval_seconds"; >+const PREF_INTERVAL_SECONDS_NOTSET = 3 * 60; i'm concerned that we won't keep up, and expiration will fall behind for people who powerbrowse. as discussed on irc, please add a check in the maintence code for how far behind expiration is. we can take it out once we've confirmed falling-behind is not an issue. >+ >+// The percentage of system memory we will use for the database's cache. >+// Use the same value set in nsNavHistory.cpp >+const PREF_DATABASE_CACHE_PER_MEMORY_PERCENTAGE = >+ "places.history.cache_per_memory_percentage"; >+const PREF_DATABASE_CACHE_PER_MEMORY_PERCENTAGE_NOTSET = 6; please note *why* this is here, what it's used for >+ >+// Minimum number of unique URIs to retain, in case system-info returns bogus values. >+const MIN_URIS = 1000; what if user sets max_pages pref to zero? does it behave as if history is disabled? >+// This is the average size in bytes of an URI entry in the database. >+// Magic numbers are got through analysis of the distribution of a ratio s/got/determined/ >+ >+// When we expire we can use these limits: >+// - SMALL for usual partial expirations, will expire a small chunk. >+// - LARGE for idle or shutdown expirations, will expire a large chunk. >+// - NONE for clearHistory, will expire everything. >+// - DEBUG will use a known limit, passed along with the debug notification. >+const LIMIT = { >+ SMALL: 0, >+ LARGE: 1, >+ NONE: 2, >+ DEBUG: 3, >+}; nit: using "none" to mean "everything" is confusing.. maybe use "unlimited" instead? >+ // Finds orphan URIs in the database. >+ // We don't bother taking in count the temp table here, it will be a subset >+ // of all history most of the times. nit: s/in count/into account/ and s/times/time/ >+ handleCompletion: function PEX_handleCompletion(aReason) >+ { >+ if (aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED) { >+ // Adapt the aggressivity of partial steps based on the status of history. >+ // A dirty history will return all the entries we are expecting, while a >+ // clean one will return less results or no results at all. >+ this._status = STATUS.UNKNOWN; >+ if ("_expectedResultsCount" in this && "_expiredResults" in this) { this would happen if nothing to expire? any reason to not just set defaults for these, instead of property presence/absence? >+ >+ /** >+ * Generate the statement used for expiration. >+ * >+ * @param aQueryType >+ * Type of the query to build statement for. >+ * @param aLimit >+ * Whether to use small, large or no limits when expiring. nit: add something like "(see the LIMIT const for values)" >+ */ >+ _cachedStatements: {}, >+ _getBoundStatement: function PEX__getBoundStatement(aQueryType, aLimit) >+ { >+ // Statements creation can be expensive, so try to cache them if possible. why would it not be possible? r=me w/ these changes.

(In reply to comment #94) > (From update of attachment 418306 [details] [diff] [review]) > > >+// Seconds between each expiration step. > >+const PREF_INTERVAL_SECONDS = "interval_seconds"; > >+const PREF_INTERVAL_SECONDS_NOTSET = 3 * 60; > > i'm concerned that we won't keep up, and expiration will fall behind for people > who powerbrowse. as discussed on irc, please add a check in the maintence code > for how far behind expiration is. we can take it out once we've confirmed > falling-behind is not an issue. do you want this to automatically run and notify in the console (not sure users will notice it though) or being able to run manually to check status? > >+// Minimum number of unique URIs to retain, in case system-info returns bogus values. > >+const MIN_URIS = 1000; > > what if user sets max_pages pref to zero? does it behave as if history is > disabled? Non exactly, if history is disabled nothing is stored, if max pages is 0 things are stored temporarly and then expired. i could set disabled history if it's zero, but then there would be a cycle, user should reset both prefs to use history again. or i could just consider 0 a bad value and use the pref only if > 0 > >+ handleCompletion: function PEX_handleCompletion(aReason) > >+ { > >+ if (aReason == Ci.mozIStorageStatementCallback.REASON_FINISHED) { > >+ // Adapt the aggressivity of partial steps based on the status of history. > >+ // A dirty history will return all the entries we are expecting, while a > >+ // clean one will return less results or no results at all. > >+ this._status = STATUS.UNKNOWN; > >+ if ("_expectedResultsCount" in this && "_expiredResults" in this) { > > this would happen if nothing to expire? any reason to not just set defaults for > these, instead of property presence/absence? better gc and i don't need these objects to be persistent, mostly code is cleaner without tens of null local properties. > >+ _getBoundStatement: function PEX__getBoundStatement(aQueryType, aLimit) > >+ { > >+ // Statements creation can be expensive, so try to cache them if possible. > > why would it not be possible? stmt creation failure, but actually this comment is useless, "so we want to cache them." will be better!

> do you want this to automatically run and notify in the console (not sure users > will notice it though) or being able to run manually to check status? actually, manual would be good, from a qa/testing standpoint. > i could just consider 0 a bad value and use the pref only if > > 0 > sounds good

Comment on attachment 418306 [details] [diff] [review] Part10: Add a new expiration component r=mano

So, i said a wrong thing, i cannot artificially limit the max number of pages to 1000, because i need to be able to set it to any custom value for tests :( On the other side, this pref is hidden and intended for advanced users, so they should know what they are doing and not set it to nonsense values.

Pushed part 1 to 6: 1: http://hg.mozilla.org/mozilla-central/rev/6518af97883b 2: http://hg.mozilla.org/mozilla-central/rev/c51514439397 3: http://hg.mozilla.org/mozilla-central/rev/84c21a03e3d7 4: http://hg.mozilla.org/mozilla-central/rev/6ccd2baba00f 5: http://hg.mozilla.org/mozilla-central/rev/44d046507cf9 6: http://hg.mozilla.org/mozilla-central/rev/85c3437867ae

Fixes comments and a typo. So, i'm going to use the start debug event for db maintenance (Part14 coming), it's the only way to force an expiration, looks like i won't ifdef that after all.

filed bug 536374 against Seamonkey, so they are notified about coming changes.

so, i've changed the maintenance method to be more async (vacuum could run too early or not run due to locked db, now it's enqueued with other cleanups), and it will get an optional callback that i could use for hooking the functionality in about:support page. Maintenance will print out the current max pages values that is used (sorry i have to pass it through a pref :( ), so we can compare it with number of places in the database and see if we were falling behind, plus it will run a full expiration step before vacuuming.

still something to fix: NEXT ERROR TEST-UNEXPECTED-FAIL | (xpcshell/head.js) | [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getIntPref]" nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)" location: "JS frame :: file:///builds/slave/sendchange-linux-unittest/mozilla/objdir/dist/bin/modules/PlacesDBUtils.jsm :: expire :: line 650" data: no] and i should search all removeAllPages() entries in mxr, test_browserhistory.js seems to have some random orangeness.

this fixes the getIntPref issue. About the removeAllPages problem, if we call removeAllPages and immediately try to add an URI (without visits) this uri could be deleted by the async expiration since it does not have any visit, and we are in a bad shape. This happens for example in test_browserhistory.js I need to figure out a way to handle this case.

this is an interesting problem, since we are half sync half async these issues can happen, we should move addVisit to be async soon. since i don't want to run pages expiration on the main thread, the only thing i can do is running a simple query to exclude new place ids... this sucks quite a bit, but i don't have better ideas atm.

Comment on attachment 419303 [details] [diff] [review] Part14: Add a maintenance task to print pages limit and run expiration clearing review request for now, i see a small shutdown glitch in test_maintenance_checkAndFixDatabase, looks like sometimes the db can't be closed (async vacuum running?). Investigating.

ok, since i changed the way output is handled, i had to update the test.

Comment on attachment 419307 [details] [diff] [review] Part15: Fix a bad condition when expiration runs just before an addURI [Checkin: Comment 120] bah. please file a bug for async addVisit (if there's not one already), and note the bug here. r=me.

Comment on attachment 419414 [details] [diff] [review] Part14: Add a maintenance task to print pages limit and run expiration [Checkin: Comment 120] >@@ -593,15 +593,23 @@ nsPlacesDBUtils.prototype = { > return log[logIndex] == "ok"; > } > >- function vacuum() { >+ function vacuum(aVacuumCallback) { > log.push("VACUUM"); > let dirSvc = Cc["@mozilla.org/file/directory_service;1"]. > getService(Ci.nsIProperties); > let placesDBFile = dirSvc.get("ProfD", Ci.nsILocalFile); > placesDBFile.append("places.sqlite"); > log.push("places.sqlite: " + placesDBFile.fileSize + " byte"); >- self._dbConn.executeSimpleSQL("VACUUM"); > log.push(sep); >+ let stmt = self._dbConn.createStatement("VACUUM"); >+ stmt.executeAsync({ >+ handleResult: function() {}, >+ handleError: function() {}, >+ handleCompletion: function(aReason) { >+ aVacuumCallback(); >+ } >+ }); >+ stmt.finalize(); > } maybe Cu.reportError when vacuum fails, so it's not invisible? >@@ -673,12 +700,17 @@ nsPlacesDBUtils.prototype = { > // get some stats. > if (integrityIsGood) { > cleanup(); >- vacuum(); >- stats(); >+ expire(); >+ vacuum(function () { >+ stats(); >+ if (aLogCallback) >+ aLogCallback(log); >+ else >+ Components.utils.reportError(log.join('\n')); >+ }); > } that's not necessarily an error state, right? r=me otherwise.

(In reply to comment #109) > (From update of attachment 419414 [details] [diff] [review]) > maybe Cu.reportError when vacuum fails, so it's not invisible? done > >+ Components.utils.reportError(log.join('\n')); > >+ }); > > } > > that's not necessarily an error state, right? right, my lazyness caused me to use reportError, changed to a simple console logStringMessage.

(In reply to comment #108) > (From update of attachment 419307 [details] [diff] [review]) > bah. please file a bug for async addVisit (if there's not one already), and > note the bug here. r=me. there is bug 499828 but it's a bit generic so i'm making it tracking, and i filed bug 539706. Ideally async addVisit queries would run ordered with other async queries, so this problem would not exist in such a case.

(In reply to comment #111) > Ideally async addVisit queries would run ordered with other async queries, so > this problem would not exist in such a case. Right - once we go async all the way, these race conditions go away.

actually pushed to the Places branch, try server numbers were in the noise, let's see these ones, then we should be fine...

From 2 talos runs i got on Places branch and 2 talos runs i got from Try server, looks like numbers are inside the expected noise, i can see them fluctuating around values got from latest m-c result. Looks like there is some improvement in Tshutdown, but hard to tell since those fluctuate a lot. Ts cold sometimes shows a regression on Mac, but other unrelated pushes known to be good sometimes show even larger regressions on the same box, and i'm pretty sure those are not considered regressions. Since it fluctuates i can't tell if that's bad, but looks inside noise after all. So, finally i think i'll proceed on central.

I got no mail about results changing, so it's all within noise (or the scripts cannot detect it).

do we get mails? or just you since owner of the branch?

dietrich and I both get the mail

Sorry if this is covered somewhere, but I don't read code. Using the latest hourly build from m-c with this large patch, in the Options Panel -> Privacy -> Use the drop-list to get to 'Custom History.... Note that the spinner box for number of days is missing. Intentional ? Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a1pre) Gecko/20100115 Minefield/3.7a1pre Firefox/3.6 ID:20100115104826

(In reply to comment #118) > Intentional ? yes, it is, we now try to retain the max history we can, i'll post something next week to explain everything (there was already a post in the newsgroup some time ago)

changeset 1444ca3a20ec changeset 2bdf21a8a3d6 changeset 7581e042fabd changeset 332e3e26cd97 changeset 531b4d5ddb49 changeset 30fcd9794eb2 changeset 936d1adb21d4 changeset a70955a44726 changeset 219dd0ae2444 changeset adc9abe1362f changeset 15937d3e5834 changeset 1a6355f9da5a changeset 6070563dee67

(In reply to comment #48) > Created an attachment (id=416063) [details] > bool history pref > > this is the privacy preferences panel with the simple "on/off" remember history > preference, in place of our current days pref. Adding the user-doc-needed keyword.

Hmm, are there bugs to deal with the rest of results I see in http://mxr.mozilla.org/mozilla-central/search?string=browser.history_expire or are those intentionally leftß

Pushed http://hg.mozilla.org/mozilla-central/rev/d1f6d326c5a1 to take the magic (number) out of the test_autocomplete.js change.

(In reply to comment #122) > Hmm, are there bugs to deal with the rest of results I see in > http://mxr.mozilla.org/mozilla-central/search?string=browser.history_expire or > are those intentionally leftß well, in regress-308111.js it is just an internal string, in nsNavHistory.cpp it's oviously wanted, we migrate the old disabled state to avoid collecting history for users not willing to. The only interesting entry is in aboutSupport.xhtml, i did know about it though, it does not harm anyone, it's just a dead entry but i prefer having it still around for support purposes (checking old manually-set values).

Marco, thanks, just wanted to make sure we didn't forget anything.