Closed Bug 476299 Opened 11 years ago Closed 11 years ago

Decay frecency values to estimate recalculating frecencies

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: Mardak, Assigned: Mardak)

References

Details

(Keywords: fixed1.9.1, Whiteboard: fixes bug 482069)

Attachments

(1 file, 3 obsolete files)

This is part 3 of 3 to address bug 458801.

Instead of recalculating old frecencies, we can just multiply them by a scaling factor similar to what we do for adaptive results. If the page does get visited, it'll go through the full frececency recalculation anyway.
Attached patch v1 (obsolete) — Splinter Review
Basically do what we did for adaptive stuff, and move the adaptive stuff to the daily idle as well.

(Yeah, I know I didn't do the UPDATE for moz_places_temp, but this is an estimate anyway, so no big worry that we're missing some recently calculated frecency places.)
Assignee: nobody → edilee
Status: NEW → ASSIGNED
Attachment #359944 - Flags: review?(dietrich)
Comment on attachment 359944 [details] [diff] [review]
v1

>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
>@@ -5302,6 +5302,19 @@
> 
>     // Update frecency values
>     (void)FixInvalidFrecencies();
>+
>+    if (mDBConn) {
>+      // Globally decay places frecency rankings to estimate reduced frecency
>+      // values of infrequently visited pages. Pages that do get visited will
>+      // automatically get their frecency values updated

unfinished sentence?

>+      (void)mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+        "UPDATE moz_places SET frecency = frecency * .95 WHERE frecency > 0"));
>+
>+      // Decay potentially unused adaptive entries (e.g. those that are at 1)
>+      // to allow better chances for new entries that will start at 1
>+      (void)mDBConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>+        "UPDATE moz_inputhistory SET use_count = use_count * .95"));
>+    }
>   } else if (nsCRT::strcmp(aTopic, NS_PRIVATE_BROWSING_SWITCH_TOPIC) == 0) {
>     if (NS_LITERAL_STRING(NS_PRIVATE_BROWSING_ENTER).Equals(aData)) {
>       mInPrivateBrowsing = PR_TRUE;

less precision in the frecency of non-recently-visited URLs is fine, and recently visited get the full treatment. sounds good, r=me with a test.

my only concern is that awesomebar efficacy is very subjective. let's keep a close eye on the build forums and maybe get in touch with support/qa, to stay aware of "awesomebar not finding my stuff" complaints.
Attachment #359944 - Flags: review?(dietrich) → review+
When is this code ran?  I can't quite tell since the patch doesn't apply anymore.  Any reason why it's not updating moz_places_view instead of moz_places?
Or I bet this is idle, isn't it?
(In reply to comment #3)
> When is this code ran?
At most once a day when the user is idle. (This patch depends on a part 2 of 3, but I believe that patch is stale..)

> Any reason why it's not updating moz_places_view
You probably mean _temp? It's an estimate anyway. If we miss some stuff in _temp, it'll be synced to _places eventually and the next time we run this code, it'll decay.

I suppose we /could/ update _temp as well as _places. It would just be an extra query at most once a day.
no, I do mean moz_places_view - the view copies data into _temp, and updates it there.  The idea is to minimize writes to the permanent table.  While that doesn't matter in this case, you will lose all changes in the permanent table that for any entry that exists in _temp when they are synced.
Blocks: 482069
Attached patch v2 (obsolete) — Splinter Review
Added test and put in a delete for old input history as well as tweaked the decay rate.

I switched it to .975 so that in about a month, the original value is now 50%. This is consistent with the values we use for calculating with buckets and bonuses.
Attachment #359944 - Attachment is obsolete: true
Attachment #366145 - Flags: review?(dietrich)
Oh and switched to async.
Note: it'd be really helpful if you provide more lines of context (-U8) and generate your diffs with function headers (-P) to make it easier to figure out what a patch is doing.
Got any suggestions on how to do that only on export but not normal patches in mq?
Don't upload mq patches.  Use hg diff and specify revisions.  Normally I do |hg diff -U8 -rqparent:tip > ~/Desktop/bugnum.patch|

I have something in my .hgrc that adds function headers always (doesn't hurt mq).
Well I just do |hg export qtip| or whatever named patch I have not |hg diff|. There's diffopts for diff but not export.
Attached patch v2 (obsolete) — Splinter Review
Same as before but now with more hg export --config "diff.unified=8" with [diff] git = True; showfun = True
Attachment #366145 - Attachment is obsolete: true
Attachment #366163 - Flags: review?(dietrich)
Attachment #366145 - Flags: review?(dietrich)
Comment on attachment 366163 [details] [diff] [review]
v2

>-  // Decay potentially unused entries (e.g. those that are at 1) to allow
>-  // better chances for new entries that will start at 1
>-  rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
>-      "UPDATE moz_inputhistory "
>-      "SET use_count = use_count * .9"));
>-  NS_ENSURE_SUCCESS(rv, rv);
For what it is worth, this totally shouldn't have been in that function - it's unlike every other expire*Paranoid function, and it's not documented in the .h file.

I probably would have caught this in the original bug had it either been documented, or called something else.
Comment on attachment 366163 [details] [diff] [review]
v2

please check rv so that problems are at least visible. r=me otherwise.
Attachment #366163 - Flags: review?(dietrich) → review+
Attached patch v2.1Splinter Review
Added checks like..
NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to create decayFrecency");
Attachment #366163 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/d3956139b618
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: fixes bug 482069
Target Milestone: --- → mozilla1.9.2a1
Mardak - you pinged me on IRC asking me if I wanted to take this bug. Can you go over cost/benefit here and nominate for approval?
Attachment #367267 - Flags: approval1.9.1?
Comment on attachment 367267 [details] [diff] [review]
v2.1

a191? for this fix for bug 482069. Without it, the adaptive behavior of the location bar will totally suck if the user is idle such as leaving the browser open overnight.
Attachment #367267 - Flags: approval1.9.1? → approval1.9.1+
See Also: → 1464454
You need to log in before you can comment on or make changes to this bug.