Closed Bug 1464454 Opened 2 years ago Closed 2 years ago

Consider an higher use_count limit for adaptive history

Categories

(Toolkit :: Places, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

We currently set use_count to 1 when the user types a word and then selects a specific url.
Then we decay daily by 0,975

finally we 
"DELETE FROM moz_inputhistory WHERE use_count < .01"

Old profiles have a lot of adaptive history, for example for the "g" letter I have something like 21 entries in adaptive history.
These are likely to dominate the Address Bar results completely, since we don't limit them in number when querying, that means the user won't see any frecency-based result.

I'd like these results to change more dinamically, and as such I'm evaluating increasing that 0.01.

Note this may still not stop the Address Bar domination... Should we also limit these results in number, or be even more aggressive on the purging?

As a reminder:
0.5 means an unused entry disappears after 28 days
0.2 means an unused entry disappears after 64 days
0.1 means an unused entry disappears after 91 days
0.05 means an unused entry disappears after 119 days
0.01 means an unused entry disappears after 182 days

Imo, we should aim between 0.2 and 0.1.
Regarding the limit, I was maybe thinking we could pick something like 3 adaptive results, in the end in the common case we have 5 slots.
Flags: needinfo?(edilee)
Flags: needinfo?(adw)
Blocks: 1425029
Assignee: nobody → mak77
Status: NEW → ASSIGNED
> Address Bar domination
I would think the most direct way to address that is putting a LIMIT 3 at the end of SQL_ADAPTIVE_QUERY:
https://searchfox.org/mozilla-central/rev/bf4def01bf8f6ff0d18f02f2d7e9efc73e12c63f/toolkit/components/places/UnifiedComplete.js#224-244

In your example of 21 entries for "g", are these all for the single letter "g" or they start with it and show up because it's a prefix match?

That's particularly relevant because there's a "double use_count" bonus for exact matches, which would affected if we instead changed the decay rate from 0.975 to something smaller (to decay to 0.01 faster). Although looks like originally it was 0.9 decay from bug 406422 comment 4, so perhaps if that's been tweaked, maybe just keep tweaking that number instead of the cleanup threshold? ;)
Flags: needinfo?(edilee)
See Also: → 406422
Oh look, it was me who changed it to 0.975 :P Bug 476299 comment 7. Was targeting 50% in 1 month.
See Also: → 476299
(In reply to Ed Lee :Mardak from comment #1)
> > Address Bar domination
> I would think the most direct way to address that is putting a LIMIT 3 at
> the end of SQL_ADAPTIVE_QUERY:

Yes, that's what I also suggested at the end of comment 0 :)

> In your example of 21 entries for "g", are these all for the single letter
> "g" or they start with it and show up because it's a prefix match?

1 exact match, 20 prefix matches.

> That's particularly relevant because there's a "double use_count" bonus for
> exact matches, which would affected if we instead changed the decay rate
> from 0.975 to something smaller (to decay to 0.01 faster). Although looks
> like originally it was 0.9 decay from bug 406422 comment 4, so perhaps if
> that's been tweaked, maybe just keep tweaking that number instead of the
> cleanup threshold? ;)

Yes, there's a 2x bonus for exact matches, but it's just calculated when we match in unifiedcomplete, not stored in the db. So it doesn't seem to matter?

Changing the decay rate also affects normal frecency decaying though, so I'd prefer not touching it. Unless we want to split the 2 decays.
The 50% after 30 days sounds good, but there's no reason (imo) to wait 182 days before discarding a match.

Thus, my suggestions is still to change 0.01 to 0.1, and add a LIMIT 3 to the unifiedcomplete query.
Ah oops, yeah. Just pick some reasonable amount of time to decay and clean up. 91 days seems fine. I suppose just to compare (although I don't think I actually picked numbers because of this), original 0.9 decay with 0.01 threshold takes 44 days.
This seems kind of hard to evaluate in the abstract, but your comment 0 makes sense (thanks for all the explanation).  91 days still seems like a lot, but considering it's 50% of what we currently use, it's probably a good place to start at least?

I'll look at the patch now.
Flags: needinfo?(adw)
Should we expose adaptive results as a possible result type so that they're captured by telemetry (FX_URLBAR_SELECTED_RESULT_TYPE, FX_URLBAR_SELECTED_RESULT_INDEX_BY_TYPE)?  Unless I'm mistaken it doesn't look like we do that now.
Comment on attachment 8981373 [details]
Bug 1464454 - Expire adaptive history after 90  days and limit the number of top adaptive history matches in the Address Bar.

https://reviewboard.mozilla.org/r/247502/#review254562

::: toolkit/components/places/UnifiedComplete.js:943
(Diff revision 1)
>      }
>    }
>  
> +  // Used to limit the number of adaptive results.
> +  this._adaptiveCount = 0;
> +  this._adaptiveRows = [];

What do you think about calling these _extraAdapativeCount/Rows (or overflow, excess, etc.)?  Since they're only used for rows that aren't added at first.

::: toolkit/components/places/UnifiedComplete.js:2127
(Diff revision 1)
> +      this._addFilteredQueryMatch(row);
> +    } else {
> +      this._adaptiveRows.push(row);
> +    }
> +    this._adaptiveCount++;
> +

Nit: unnecessary blank line

::: toolkit/components/places/UnifiedComplete.js:2287
(Diff revision 1)
>     *
>     * @return an array consisting of the correctly optimized query to search the
>     *         database with and an object containing the params to bound.
>     */
>    get _adaptiveQuery() {
> +    // About one querter of the results can be adaptive results.

Make this comment less descriptive, more prescriptive, like "Allow one quarter of the results to be adaptive results."  (Also misspelling on "quarter" here)
Attachment #8981373 - Flags: review?(adw) → review+
(In reply to Drew Willcoxon :adw from comment #7)
> Should we expose adaptive results as a possible result type so that they're
> captured by telemetry (FX_URLBAR_SELECTED_RESULT_TYPE,
> FX_URLBAR_SELECTED_RESULT_INDEX_BY_TYPE)?  Unless I'm mistaken it doesn't
> look like we do that now.

Great idea, I'll file a follow-up bug for that.
Comment on attachment 8981373 [details]
Bug 1464454 - Expire adaptive history after 90  days and limit the number of top adaptive history matches in the Address Bar.

https://reviewboard.mozilla.org/r/247502/#review254562

> What do you think about calling these _extraAdapativeCount/Rows (or overflow, excess, etc.)?  Since they're only used for rows that aren't added at first.

I'm keeping _adaptiveCount as-is because it counts all adaptive results, not just the extra ones. Renaming _adaptiveRows to _extraAdaptiveRows
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/942f8f4e2437
Expire adaptive history after 90  days and limit the number of top adaptive history matches in the Address Bar. r=adw
https://hg.mozilla.org/mozilla-central/rev/942f8f4e2437
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.