630225 - Expose frecency as a sorting order for the history sidebar (with slight query builder optimization).

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Most of the analysis is in bug 365992. I'll just report what this bug can improve and what it cannot improve.
While this is not a regression from FX3.x, it's the second most common complain I can find regarding history (first one is history loss), and is in a release we based on speed improvements and ui responsitivity.

improves:
- most visited and last visited are instant (limited to 200 results)
- searching terms with lots of results is much faster (limited to 100 results)
- the matching algorithm is the same as the location bar (code reuse, safe, better matching)
- searches based on terms are more useful (sorted by frecency rather than by title)
- makes the move to async containers easier (less post-filtering)

WILL NOT improve:
- searches returning few results or no results at all (and their live-update)

use-case scenarios:
- using history sidebar (by most/last visited) is a pain
- leaving history sidebar open on browser close sums up the search time to the next startup time (Ts increases by seconds)
- searching history in Library or sidebar is impossible, especially for slow-typers
- User leaves a history search open in a background window and does not associate browser hangs to something he doesn't see, but directly to the browser.

Risk is somehow limited by the small amount of changes and code reuse, clearly there is some risk, but we don't plan further changes to this pieces of code so it will stay easily backed-out-able.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

will do another trybuild from this.

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 508433 [details] [diff] [review]
patch v1.0

r+ on the code changes. however, the frecency result sorting needs a test. i'll r+ with that done.

Comment 3

[Marco Bonardo [:mak]]

builds
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-99847d1a79f4

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch v1.1

added tests

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 508603 [details] [diff] [review]
patch v1.1

r=me.

I'm fully behind limiting the search results so that we don't hang the browser for 12 seconds, but given the late stage of the release, get approval from Johnath or Beltzner, so we get another set of eyes on this change before landing it.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 508603 [details] [diff] [review]
patch v1.1

OK, asking approval, risk and gains are described in comment 0, the change mitigates the most common cases of a longstanding issue that in certain situations can make the user think the browser has awful performances (without actually blaming the right UI piece).
It also tries to be conservative in the changes.

Comment 7

[avada]

Will this limit affect both the library and the sidebar?
I sometimes manually delete entries from the history when I feel like a lot of useless them are useless and clutter the results or when I feel the urlbar is getting slower its getting slower. When I do this I usually search for a domain, sort by visitation number and delete the ones that have 1 (or 0...) visitations and are older than a few weeks or months. And these results can be in the number of hundreds or thousands. If its limited to 100 I wouldn't be able to do this.

Comment 8

[Marco Bonardo [:mak]]

(In reply to comment #7)
> Will this limit affect both the library and the sidebar?

yes

> I sometimes manually delete entries from the history when I feel like a lot of
> useless them are useless and clutter the results or when I feel the urlbar is
> getting slower its getting slower. When I do this I usually search for a
> domain, sort by visitation number and delete the ones that have 1 (or 0...)
> visitations and are older than a few weeks or months. And these results can be
> in the number of hundreds or thousands. If its limited to 100 I wouldn't be
> able to do this.

you can delete domains in the sidebar view "by site".
or you can delete the "older than 6 months" container in the view "by date".
you can search multiple times too, actually with this limit you would take much less time doing so, since with your usual way you have to wait seconds at each refresh, while with the change even if for your specific use case requires multiple searches they will be much faster.
Btw, you don't need to to this manual cleanup, we expire entries when we reach our natural limit, if you think the limit is too large you can reduce it (see http://blog.bonardo.net/2010/01/20/places-got-async-expiration)

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 508603 [details] [diff] [review]
patch v1.1

Too much changing here to be comfortable about taking this at this point

Comment 10

[Marco Bonardo [:mak]]

please don't push this, I want to revise it a bit since there is more time available to make it more awesome.

Comment 11

[Marco Bonardo [:mak]]

Attachment: patch v1.2

I'm slightly reducing scope of the bug, I'm removing from the patch all the numeric limits.
The limited views were a solution I was mostly suggesting for FF4, but now that pressure is solved and I only want to take the good part of the patch without introducing behavioral differences. I actually got new ideas to improve performances in the meanwhile.

Well, there is still one difference, the sidebar, when not sorted by last or most visited, will be sorted by frecency, that should be a better ordering to find stuff, it will act like an infinite awesomebar (with some less awesomeness though).

From various testing the limit was also effective only in some case, while when searching for rare terms we were still going the slow path. This patch goes toward the right direction allowing to reduce some of the post-filtering work, thus pushing it since the changes are not additive from the r+.

Comment 12

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/24f177c8b6f6

Comment 13

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/24f177c8b6f6

Comment 14

[Marco Bonardo [:mak]]

dev-doc-needed for the additional SORT_BY_FRECENCY_* options

Comment 15

[Eric Shepherd [:sheppy]]

Documented:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsINavHistoryQueryOptions#Sorting_methods

Also fixed a couple of the other constants, which had the wrong numbers, and removed two that apparently were documented at one point then removed before shipping.

Also mentioned on Firefox 6 for developers.