Last Comment Bug 630225 - Expose frecency as a sorting order for the history sidebar (with slight query builder optimization).
: Expose frecency as a sorting order for the history sidebar (with slight query...
Status: RESOLVED FIXED
[not-ready for cedar][fixed-in-places]
: dev-doc-complete, perf
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: unspecified
: All All
: -- normal with 1 vote (vote)
: mozilla6
Assigned To: Marco Bonardo [::mak]
:
Mentors:
Depends on: 658242 661767
Blocks: 365992 411591
  Show dependency treegraph
 
Reported: 2011-01-31 09:12 PST by Marco Bonardo [::mak]
Modified: 2011-06-03 00:59 PDT (History)
7 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1.0 (42.44 KB, patch)
2011-01-31 09:21 PST, Marco Bonardo [::mak]
dietrich: review-
Details | Diff | Splinter Review
patch v1.1 (36.06 KB, patch)
2011-01-31 16:23 PST, Marco Bonardo [::mak]
dietrich: review+
dtownsend: approval2.0-
Details | Diff | Splinter Review
patch v1.2 (34.10 KB, patch)
2011-04-13 13:09 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review

Description Marco Bonardo [::mak] 2011-01-31 09:12:29 PST
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] 2011-01-31 09:21:29 PST
Created attachment 508433 [details] [diff] [review]
patch v1.0

will do another trybuild from this.
Comment 2 Dietrich Ayala (:dietrich) 2011-01-31 09:39:05 PST
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 4 Marco Bonardo [::mak] 2011-01-31 16:23:18 PST
Created attachment 508603 [details] [diff] [review]
patch v1.1

added tests
Comment 5 Dietrich Ayala (:dietrich) 2011-02-01 02:04:48 PST
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] 2011-02-01 04:34:37 PST
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 2011-02-01 05:04:45 PST
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] 2011-02-01 05:17:17 PST
(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] 2011-02-07 14:40:04 PST
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] 2011-03-22 08:25:33 PDT
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] 2011-04-13 13:09:04 PDT
Created attachment 525777 [details] [diff] [review]
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] 2011-04-13 13:26:27 PDT
http://hg.mozilla.org/projects/places/rev/24f177c8b6f6
Comment 13 Marco Bonardo [::mak] 2011-04-18 06:06:21 PDT
http://hg.mozilla.org/mozilla-central/rev/24f177c8b6f6
Comment 14 Marco Bonardo [::mak] 2011-04-18 06:09:24 PDT
dev-doc-needed for the additional SORT_BY_FRECENCY_* options
Comment 15 Eric Shepherd [:sheppy] 2011-04-22 11:03:18 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.