Closed Bug 487777 Opened 15 years ago Closed 15 years ago

History menu is slow and takes ages to open

Categories

(Toolkit :: Places, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla1.9.2a1

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: perf, regression, verified1.9.1, Whiteboard: [TSnappiness])

Attachments

(2 files, 3 obsolete files)

History menu query is quite complex and damn slow, especially on trunk, we should do better.
Blocks: 428690
Attached patch patch v1.0 (obsolete) — Splinter Review
this is 4X faster on trunk and 2X faster on branch, all tests pass.
Still investigating if further improvements are possible.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #372042 - Attachment is patch: true
Attachment #372042 - Attachment mime type: application/octet-stream → text/plain
Dietrich, could you try this since you see the issue and tell if you notice any difference?
changing bug title to make it more searchable
Summary: History menu takes ages to open → History menu is slow and takes ages to open
Comment on attachment 372042 [details] [diff] [review]
patch v1.0

unluckily this is missing a check
Attachment #372042 - Attachment is obsolete: true
Attached patch trunk patch v1.0 (obsolete) — Splinter Review
This brings back performances on trunk at the same level of those on branch, simplifying a lot the code.
Also i noticed we were not testing redirectsMode on these special queries, so i changed the test to cover them, this will make easier to find errors while modifying them.
With this on trunk most visited is 30% faster, history menu is 6X faster.
Comparing with branch, most visited is slower by 60s, history menu is about same speed (notice that now query is more complex because it's excluding redirects).
with 148000 places and 140000 visits, history menu query takes about 1.3s, most visited query about 70ms (i suppose is time to add a last_visit_date column with an index to moz_places).
Attachment #372126 - Flags: review?(dietrich)
Comment on attachment 372126 [details] [diff] [review]
trunk patch v1.0

what about filing a bug to add last_visit_date column and see how much that helps?
actually regression from bug 428690.
Keywords: regression
Is this a trunk only bug or can I chime in about Shiretoko because Shiretoko's history is excruciating slow to open and is rather annoying. :) 

Also the following bugs might be related in some way as they express dissatisfaction with the speed of the History system...

Bug #193236 is 6 years old...
Bug #223476 is almost 6 years old...
(In reply to comment #8)
> Also the following bugs might be related in some way as they express
> dissatisfaction with the speed of the History system...
> Bug #193236 is 6 years old...
> Bug #223476 is almost 6 years old...

Those could most likely be closed since Firefox now uses a new history system.
for now this patch is trunk only.
I'll look if i can come with a low risk patch for branch, since a real fix would require deeper changes we can't afford now due to being risky.
Attached patch branch only patch (obsolete) — Splinter Review
this is a branch only patch, i was not able to make it perfect (e.g. <100ms), we really need to revise the schema, i'll file the last_visit_date bug or directly take care of doing that here on trunk.
Btw this should be about 3X faster than current one (moving from 1.2s to 0.4s here), Dietrich could you try this and tell if you notice any difference? We could then take this for 1.9.1 (since code is already different in this area there should be no problem doing this).
Attachment #373128 - Flags: review?
Attachment #373128 - Flags: review? → review?(dietrich)
shaver hinted that this should be fixed for the beta as it's hitting people that he's talked to, so I'm requesting blocking.
Flags: blocking1.9.1?
Comment on attachment 372126 [details] [diff] [review]
trunk patch v1.0

>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp
>--- a/toolkit/components/places/src/nsNavHistory.cpp
>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>@@ -3995,199 +3995,132 @@ nsresult
> nsresult
> nsNavHistory::ConstructQueryString(
>     const nsCOMArray<nsNavHistoryQuery>& aQueries,
>     nsNavHistoryQueryOptions* aOptions, 
>     nsCString& queryString, 
>     PRBool& aParamsPresent,
>     nsNavHistory::StringHash& aAddParams)
> {
>-  nsresult rv;
>-
>-  // Information about visit_type:
>-  // 4 == TRANSITION_EMBED
>-  // 0 == undefined (see bug #375777 for details)
>+  // For informations about visit_type see nsINavHistoryService.idl.
>+  // visitType == 0 is undefined (see bug #375777 for details).
>   // Some sites, especially Javascript-heavy ones, load things in frames to 
>   // display them, resulting in a lot of these entries. This is the reason 
>   // why such visits are filtered out.

information, not informations

>+  NS_ASSERTION(sortingMode >= nsINavHistoryQueryOptions::SORT_BY_NONE &&
>+               sortingMode <= nsINavHistoryQueryOptions::SORT_BY_ANNOTATION_DESCENDING,
>+               "Invalid sortingMode found while building query!");
>+
>+  if (IsHistoryMenuQuery(aQueries, aOptions,
>+        nsINavHistoryQueryOptions::SORT_BY_DATE_DESCENDING) ||
>+      IsHistoryMenuQuery(aQueries, aOptions,
>+        nsINavHistoryQueryOptions::SORT_BY_VISITCOUNT_DESCENDING)) {
>+    // Generate an optimized query for the history menu and most visited
>+    // smart bookmark.

should rename to IsOptimizableHistoryQuery() or something to that effect.

>     queryString = NS_LITERAL_CSTRING(
>       "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
>-          SQL_STR_FRAGMENT_MAX_VISIT_DATE( "h.id" )
>-          ", f.url, null, null "
>+          SQL_STR_FRAGMENT_MAX_VISIT_DATE( "h.id" ) " AS last_visit_date, "
>+          "f.url, null, null "
>         "FROM moz_places_temp h "
>         "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
>-        "WHERE h.id IN ( ") + sqlFragment + NS_LITERAL_CSTRING(") "
>-      "UNION ALL "
>-      "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
>-          SQL_STR_FRAGMENT_MAX_VISIT_DATE( "h.id" )
>-          ", f.url, null, null "
>+        "WHERE h.hidden <> 1 "
>+          "{QUERY_OPTIONS} "
>+      "UNION ALL "
>+      "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
>+          SQL_STR_FRAGMENT_MAX_VISIT_DATE( "h.id" ) " AS last_visit_date, "
>+          "f.url, null, null "
>         "FROM moz_places h "
>         "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id "
>-        "WHERE h.id IN ( ") + sqlFragment + NS_LITERAL_CSTRING(") "
>-        "AND h.id NOT IN (SELECT id FROM moz_places_temp) "
>-        "ORDER BY 6 DESC " // last visit date
>-        "LIMIT ");
>-    
>+        "WHERE h.hidden <> 1 "
>+          "AND h.id NOT IN (SELECT id FROM moz_places_temp) "
>+          "{QUERY_OPTIONS} "
>+        );
>+
>+    queryString.Append(NS_LITERAL_CSTRING("ORDER BY "));
>+    if (sortingMode == nsINavHistoryQueryOptions::SORT_BY_DATE_DESCENDING)
>+      queryString.Append(NS_LITERAL_CSTRING("last_visit_date DESC "));
>+    else
>+      queryString.Append(NS_LITERAL_CSTRING("visit_count DESC "));

i thought that we were using column numbers because ORDER BY didn't resolve column names (or aliases), is this incorrect, or has changed?

>diff --git a/toolkit/components/places/tests/queries/head_queries.js b/toolkit/components/places/tests/queries/head_queries.js
>--- a/toolkit/components/places/tests/queries/head_queries.js
>+++ b/toolkit/components/places/tests/queries/head_queries.js
>@@ -181,16 +181,28 @@ function populateDB(aArray) {
>                    DBConnection;
>           let stmt = db.createStatement("UPDATE moz_places_view " +
>                                         "SET title = :title WHERE url = :url");
>           stmt.params.title = qdata.title;
>           stmt.params.url = qdata.uri;
>           stmt.execute();
>           stmt.finalize();
>         }
>+        if (qdata.visitCount && !qdata.isDetails) {
>+          // Set the page title synchronously, otherwise setPageTitle is LAZY.
>+          let db = Cc["@mozilla.org/browser/nav-history-service;1"].
>+                   getService(Ci.nsPIPlacesDatabase).
>+                   DBConnection;
>+          let stmt = db.createStatement("UPDATE moz_places_view " +
>+                                        "SET visit_count = :vc WHERE url = :url");
>+          stmt.params.vc = qdata.visitCount;
>+          stmt.params.url = qdata.uri;
>+          stmt.execute();
>+          stmt.finalize();
>+        }
>       }

the comment looks like a copy/paste problem...

>diff --git a/toolkit/components/places/tests/queries/test_redirectsMode.js b/toolkit/components/places/tests/queries/test_redirectsMode.js
>--- a/toolkit/components/places/tests/queries/test_redirectsMode.js
>+++ b/toolkit/components/places/tests/queries/test_redirectsMode.js
>@@ -196,55 +211,59 @@ function cartProd(aSequences, aCallback)
>  */
> function add_visits_to_database() {
>   // Clean up the database.
>   bhistsvc.removeAllPages();
>   remove_all_bookmarks();
> 
>   // We don't really bother on this, but we need a time to add visits.
>   let timeInMicroseconds = Date.now() * 1000;
>+  let visitCount = 1;
> 
>   // Array of all possible transition types we could be redirected from.
>   let t = [
>     Ci.nsINavHistoryService.TRANSITION_LINK,
>     Ci.nsINavHistoryService.TRANSITION_TYPED,
>     Ci.nsINavHistoryService.TRANSITION_BOOKMARK,
>     Ci.nsINavHistoryService.TRANSITION_EMBED,
>-    Ci.nsINavHistoryService.TRANSITION_DOWNLOAD,
>+    //Ci.nsINavHistoryService.TRANSITION_DOWNLOAD, // would make hard sorting by visit date
>   ];

please explain what you mean better in the comment, add a "TODO: " and file a followup bug if necessary.

r=me with these addressed.
Attachment #372126 - Flags: review?(dietrich) → review+
Comment on attachment 372126 [details] [diff] [review]
trunk patch v1.0

>+++ b/toolkit/components/places/tests/queries/head_queries.js
>+          stmt.execute();
>+          stmt.finalize();
Should really be:
try {
  stmt.execut();
}
finally {
  stmt.finalize();
}
See https://developer.mozilla.org/en/Storage#Resetting_a_Statement
Keywords: perf
Whiteboard: [TSnappiness]
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Comment on attachment 373128 [details] [diff] [review]
branch only patch

r=me
Attachment #373128 - Flags: review?(dietrich) → review+
talked on irc, mak's going to add a test the specifically tests the output of the optimized query,.
on branch with my mega-history, the menu took 5-21 seconds to open, unpatched. patched took 2.5 seconds to open.
Whiteboard: [TSnappiness] → [TSnappiness][has patch][has review]
(In reply to comment #13)
> (From update of attachment 372126 [details] [diff] [review])
> >+
> >+    queryString.Append(NS_LITERAL_CSTRING("ORDER BY "));
> >+    if (sortingMode == nsINavHistoryQueryOptions::SORT_BY_DATE_DESCENDING)
> >+      queryString.Append(NS_LITERAL_CSTRING("last_visit_date DESC "));
> >+    else
> >+      queryString.Append(NS_LITERAL_CSTRING("visit_count DESC "));
> 
> i thought that we were using column numbers because ORDER BY didn't resolve
> column names (or aliases), is this incorrect, or has changed?

we use column numbers when order by "can't" resolve column name, that happens sometimes with UNIONs depending on the union type (usually when you have some subquery like SELECT * FROM(query) UNION SELECT * FROM(query) ORDER BY
Since the query changes we can now use name. when possible we should always use column names.
filed bug 488966 to add the column+index that should make this really faster.
Attached patch trunk patch v1.1Splinter Review
Attachment #372126 - Attachment is obsolete: true
Comes with test. the test is practically test_redirectsMode.js renamed in test_optimizableHistoryQueries.js without testing redirectsMode that is trunk only. The test has already been reviewed for trunk, i only removed a not applicable option, and is testing both normal history queries and special optimized queries.
Attachment #373128 - Attachment is obsolete: true
I just wanted to say that I find the work in this bug very exciting!

*waves pompoms*
thanks, i hope and think bug 488966 will bring even more.

*blush*
Flags: in-testsuite?
trunk patch:
http://hg.mozilla.org/mozilla-central/rev/55cb6c5991d1
Whiteboard: [TSnappiness][has patch][has review] → [TSnappiness]
Target Milestone: --- → mozilla1.9.2a1
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
I don't know if this is related but this patch hasn't sped up my history menu much, if at all. It still loads with a large delay.... but normally only for the first time I click it. After that it's fine.

Related or different bug?
Depends on: 489900
which version? How is your history setup?
For further improvements you'll need to look into bug 488966.
Yes. I confirm that menu still slow for the first time and after some history changing. I can see it even on Core 2 2Ghz. Looks like you use cache that works until history will be changed.
I think current speed is inadmissible.
And can give good advice for you - just use separate store for history menu content. It will be table with fixed row count and only 3 columns - name, url, favicon. That is easy to do and very fast. Think about users first and about beauty of code only after that.
there's no useful informations in your comments, i'm asking your browser version and history settings, that's the only thing we need to know atm, this is not the right place to complain, this is a place to help fixing things.
last nightly. default history settings.
The same here with Mozilla/5.0 (Windows; U; Windows NT 6.0; sk; rv:1.9.2a1pre) Gecko/20090424 Minefield/3.6a1pre and my regularly used profile with default history settings and obviously a huge history db. It takes 3-4 secs to open the History menu (P4 3,2GHz).
so we are talking about trunk (3.6).
slow query on trunk is sadly still expected, but should be faster then before. 3s or 4s is about the same values i would expect with large dbs, notice we have moved down from 10s or more.
my target value is still < 500ms. so please move your needs to bug 488966, that should makea huge difference.
Depends on: 493602
Verified fixed on trunk and 1.9.1 with builds on all platforms.

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090522 Minefield/3.6a1pre ID:20090522032716

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090521 Shiretoko/3.5pre ID:20090521135222
Status: RESOLVED → VERIFIED
And I quote: "omg. history menu so much better"

Thanks a ton!
Depends on: 639105
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: