476299 - Decay frecency values to estimate recalculating frecencies

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Ed Lee :Mardak]

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.

Comment 1

[Ed Lee :Mardak]

Attachment: v1

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.)

Comment 2

[Dietrich Ayala (: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.

Comment 3

[Shawn Wilsher :sdwilsh]

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?

Comment 4

[Shawn Wilsher :sdwilsh]

Or I bet this is idle, isn't it?

Comment 5

[Ed Lee :Mardak]

(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.

Comment 6

[Shawn Wilsher :sdwilsh]

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.

Comment 7

[Ed Lee :Mardak]

Attachment: v2

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.

Comment 8

[Ed Lee :Mardak]

Oh and switched to async.

Comment 9

[Shawn Wilsher :sdwilsh]

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.

Comment 10

[Ed Lee :Mardak]

Got any suggestions on how to do that only on export but not normal patches in mq?

Comment 11

[Shawn Wilsher :sdwilsh]

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).

Comment 12

[Ed Lee :Mardak]

Well I just do |hg export qtip| or whatever named patch I have not |hg diff|. There's diffopts for diff but not export.

Comment 13

[Ed Lee :Mardak]

Attachment: v2

Same as before but now with more hg export --config "diff.unified=8" with [diff] git = True; showfun = True

Comment 14

[Shawn Wilsher :sdwilsh]

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 15

[Dietrich Ayala (:dietrich)]

Comment on attachment 366163 [details] [diff] [review]
v2

please check rv so that problems are at least visible. r=me otherwise.

Comment 16

[Ed Lee :Mardak]

Attachment: v2.1

Added checks like..
NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to create decayFrecency");

Comment 17

[Ed Lee :Mardak]

http://hg.mozilla.org/mozilla-central/rev/d3956139b618

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

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?

Comment 19

[Ed Lee :Mardak]

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.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 367267 [details] [diff] [review]
v2.1

a191=beltzner

Comment 21

[Ed Lee :Mardak]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/116cd74b54f9