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

VERIFIED FIXED in Firefox 3 beta2

Status

()

P1
normal
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: moco, Assigned: mak)

Tracking

({perf})

Trunk
Firefox 3 beta2
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

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)
Depends on: 396807
note, with a large amount of history, opening this folder takes several seconds.
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.
Depends on: 399477
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
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.
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".
Assignee: nobody → sspitzer
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 M11
(Assignee)

Comment 6

11 years ago
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
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.
back to nobody, but hoping marco has cycles for it.
Assignee: sspitzer → nobody
(Assignee)

Comment 9

11 years ago
i'll try (and hopefully do) that
Assignee: nobody → mak77
(Assignee)

Comment 10

11 years ago
Created attachment 291650 [details] [diff] [review]
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
Attachment #291650 - Flags: review?(sspitzer)
thanks marco!  I'm going to test and review this, and hopefully drivers will take it for m10 (fx3 / b2)
Status: NEW → ASSIGNED

Comment 12

11 years ago
Heck ya we'll take this.  Marked blocking p1 - if it works out you are clear to land.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P1
Created attachment 291828 [details] [diff] [review]
updated version of marco's patch + a test + code cleanup
Attachment #291650 - Attachment is obsolete: true
Attachment #291828 - Flags: review?(dietrich)
Attachment #291650 - Flags: review?(sspitzer)
> 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.
Created attachment 291836 [details] [diff] [review]
minimal patch for m10, will spin off bug about type=0 / queryType=0 being not necessary
Attachment #291828 - Attachment is obsolete: true
Attachment #291836 - Flags: review?(dietrich)
Attachment #291828 - Flags: review?(dietrich)
Created attachment 291840 [details] [diff] [review]
fix test license boiler plate to be the same as the test I started with (test_385397.js)
Attachment #291836 - Attachment is obsolete: true
Attachment #291840 - Flags: review?(dietrich)
Attachment #291836 - Flags: review?(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.
Attachment #291836 - Attachment is obsolete: false
Attachment #291836 - Attachment is obsolete: true
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.
Attachment #291840 - Flags: review?(dietrich) → review+
Created attachment 291842 [details] [diff] [review]
as checked in (note s/else if/if change)
Attachment #291840 - Attachment is obsolete: true
Attachment #291842 - Flags: review+
> 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
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 3 M11 → Firefox 3 M10
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
Status: RESOLVED → VERIFIED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.