Closed
Bug 476299
Opened 16 years ago
Closed 16 years ago
Decay frecency values to estimate recalculating frecencies
Categories
(Toolkit :: Places, defect)
Toolkit
Places
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)
(deleted),
patch
|
beltzner
:
approval1.9.1+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•16 years ago
|
||
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•16 years ago
|
||
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+
Comment 3•16 years ago
|
||
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•16 years ago
|
||
Or I bet this is idle, isn't it?
Assignee | ||
Comment 5•16 years ago
|
||
(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•16 years ago
|
||
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.
Assignee | ||
Comment 7•16 years ago
|
||
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)
Assignee | ||
Comment 8•16 years ago
|
||
Oh and switched to async.
Comment 9•16 years ago
|
||
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.
Assignee | ||
Comment 10•16 years ago
|
||
Got any suggestions on how to do that only on export but not normal patches in mq?
Comment 11•16 years ago
|
||
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).
Assignee | ||
Comment 12•16 years ago
|
||
Well I just do |hg export qtip| or whatever named patch I have not |hg diff|. There's diffopts for diff but not export.
Assignee | ||
Comment 13•16 years ago
|
||
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 14•16 years ago
|
||
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•16 years ago
|
||
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+
Assignee | ||
Comment 16•16 years ago
|
||
Added checks like..
NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Failed to create decayFrecency");
Attachment #366163 -
Attachment is obsolete: true
Assignee | ||
Comment 17•16 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Whiteboard: fixes bug 482069
Target Milestone: --- → mozilla1.9.2a1
Comment 18•16 years ago
|
||
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?
Assignee | ||
Updated•16 years ago
|
Attachment #367267 -
Flags: approval1.9.1?
Assignee | ||
Comment 19•16 years ago
|
||
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.
Updated•16 years ago
|
Attachment #367267 -
Flags: approval1.9.1? → approval1.9.1+
Comment 20•16 years ago
|
||
Comment on attachment 367267 [details] [diff] [review]
v2.1
a191=beltzner
Assignee | ||
Comment 21•16 years ago
|
||
Keywords: fixed1.9.1
Assignee | ||
Updated•6 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•