399266 - improve perf of the "most visited pages" pre-defined query

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[(not reading, please use [e-mail] instead)]

improve perf of the "most visited pages" pre-defined query

for bug #385397, I added code to make "place:type=0&sort=4&maxResults=10" fast for large histories.

for bug #387996, I think we could do a similar trick (but for sort == 8, or visit count descending)

Comment 1

[(not reading, please use [e-mail] instead)]

note, with a large amount of history, opening this folder takes several seconds.

Comment 2

[Dietrich Ayala (:dietrich)]

in #testday, this comment indicates that this bug has been mitigated:

"Coce: The problem I had last time (freezing when opening some items in the "places" folder) has disappeared"

likely a result of the massive history pruning implemented last week.

Comment 3

[(not reading, please use [e-mail] instead)]

for place:queryType=0&sort=8&maxResults=10, the current query we will execute is:

SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, MAX(visit_date), f.url, null, null FROM moz_places h LEFT OUTER JOIN moz_historyvisits v ON h.id = v.place_id LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id WHERE (h.hidden <> 1 AND v.visit_type <> 4 AND v.visit_type <> 0 ) GROUP BY h.id ORDER BY 5 DESC LIMIT 10

Comment 4

[(not reading, please use [e-mail] instead)]

a couple thoughts:

if we trust h.hidden, we would not need to join against moz_historyvisits, because the menu doesn't use the visit_date:

we could do something like

SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, null, f.url,
null, null FROM moz_places h LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id WHERE h.hidden <> 1 GROUP BY h.id ORDER BY 5 DESC LIMIT 10

but, if you were to go into the places organizer and attempted to view the "Most Visited" item in the toolbar, and view the visit date column, it would be empty.

Note, this goes for the special "History" menu query. if you were to add a saved search with the same special query that we use for the history menu, you'd have the same problem.


this might be acceptable, but still worth logging.

Comment 5

[(not reading, please use [e-mail] instead)]

now that we have the "Smart Bookmarks" folder on the personal toolbar, the first item is "Most Visited".

for the dreaded "ispiked" account (or really, anyone with lots of history) attempting to open this folder is going to be really slow (see comments above).

Note, if you immediately re-open the query it will be fast (thanks to work from mano), but as soon as you visit a page, the menu becomes invalid and we need to rebuild it.

I think the "Smart Bookmarks" folder + "Most Visited" is really handy, but only if it is fast.

Note, "Recently Bookmarked" performs well, and I don't have nearly enough tags to stress "Recent Tags".

Comment 6

[Marco Bonardo [:mak]]

on my test db your query takes up about 360ms
this is an alternative way (similar to history menu trick) that takes up about 25ms

SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, null, f.url, null, null
FROM moz_places h
LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id
WHERE h.id IN 
(SELECT id FROM moz_places WHERE hidden <> 1 ORDER BY visit_count DESC LIMIT 10)
ORDER BY h.visit_count DESC

Comment 7

[(not reading, please use [e-mail] instead)]

marco, that query looks good to me.

I think all we'd need is to implement IsMostVisitedMenuQuery() in nsNavHistory.cpp and call it from nsNavHistory::ConstructQueryString() and use your query if we have a match.

Feel free to take this bug.

Comment 8

[(not reading, please use [e-mail] instead)]

back to nobody, but hoping marco has cycles for it.

Comment 9

[Marco Bonardo [:mak]]

i'll try (and hopefully do) that

Comment 10

[Marco Bonardo [:mak]]

Attachment: use a special (faster) query for the Most Visited menu

the most visited menu is opening instantly with my test profile and this patch, while it takes about a second with the currenty nightly

Comment 11

[(not reading, please use [e-mail] instead)]

thanks marco!  I'm going to test and review this, and hopefully drivers will take it for m10 (fx3 / b2)

Comment 12

[Mike Schroepfer]

Heck ya we'll take this.  Marked blocking p1 - if it works out you are clear to land.

Comment 13

[(not reading, please use [e-mail] instead)]

Attachment: updated version of marco's patch + a test + code cleanup

Comment 14

[(not reading, please use [e-mail] instead)]

> if you were to go into the places organizer and attempted to view the
> "Most Visited" item in the toolbar, and view the visit date column, it would be
> empty.

this is true, so I've logged bug #407114 on this issue.

> Note, this goes for the special "History" menu query. if you were to add a
> saved search with the same special query that we use for the history menu,
> you'd have the same problem.

this is not true, as we do calculate the visit_date column.  I was mistaken.

Comment 15

[(not reading, please use [e-mail] instead)]

Attachment: minimal patch for m10, will spin off bug about type=0 / queryType=0 being not necessary

Comment 16

[(not reading, please use [e-mail] instead)]

Attachment: fix test license boiler plate to be the same as the test I started with (test_385397.js)

Comment 17

[Dietrich Ayala (:dietrich)]

Comment on attachment 291836 [details] [diff] [review]
minimal patch for m10, will spin off bug about type=0 / queryType=0 being not necessary


>   // for the very special query for the history menu 
>   // we generate a super-optimized SQL query
>-  if (IsHistoryMenuQuery(aQueries, aOptions)) {
>+  if (IsHistoryMenuQuery(aQueries, aOptions, nsINavHistoryQueryOptions::SORT_BY_DATE_DESCENDING)) {
>     // visit_type <> 4 == TRANSITION_EMBED

>+  // for the most visited menu query
>+  // we generate a super-optimized SQL query
>+  else if (IsHistoryMenuQuery(aQueries, aOptions, nsINavHistoryQueryOptions::SORT_BY_VISITCOUNT_DESCENDING)) {

nit: line-length

r=me.

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 291840 [details] [diff] [review]
fix test license boiler plate to be the same as the test I started with (test_385397.js)

r=me w/ the nit from my previous comment fixed.

Comment 19

[(not reading, please use [e-mail] instead)]

Attachment: as checked in (note s/else if/if change)

Comment 20

[(not reading, please use [e-mail] instead)]

> Heck ya we'll take this.  Marked blocking p1 - if it works out you are clear > to land.

thanks for the approval to land, schrep.

thanks to marco for the super-optimized query and the initial patch.

note, I tested with the dreaded "ispiked" profile and it we go from several seconds (wall clock) to no delay.

fixed.

Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHis
tory.cpp
new revision: 1.211; previous revision: 1.210
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_399266.js,v

done
Checking in toolkit/components/places/tests/unit/test_399266.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_399266.js,v  <--  tes
t_399266.js
initial revision: 1.1
done

Comment 21

[Tracy Walker [:tracy]]

verified with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5) Gecko/2008032606 Firefox/3.0b5
and
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-GB; rv:1.9b5) Gecko/2008032604 Firefox/3.0b5

Comment 22

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv