fix for bug #399266: improve perf of the "Most Visited" pre-defined query in the "Smart Bookmarks" folder on the personal toolbar. r=dietrich initial patch=Marco Bonardo (MaK77) <mak77@supereva.it> a=blocking-firefox-3+
authorsspitzer@mozilla.org
Wed, 05 Dec 2007 23:38:52 -0800
changeset 8789 1b5d9c7613ff9725e9f502da218d6e6f17675b56
parent 8788 621f591cc8bd80626a1c84e1f93231557c21bb4f
child 8790 9fe12a9705a84725db1cf03d32053b64511089b4
push idunknown
push userunknown
push dateunknown
reviewersdietrich, blocking-firefox-3
bugs399266
milestone1.9b2pre
fix for bug #399266: improve perf of the "Most Visited" pre-defined query in the "Smart Bookmarks" folder on the personal toolbar. r=dietrich initial patch=Marco Bonardo (MaK77) <mak77@supereva.it> a=blocking-firefox-3+
toolkit/components/places/src/nsNavHistory.cpp
toolkit/components/places/tests/unit/test_399266.js
--- a/toolkit/components/places/src/nsNavHistory.cpp
+++ b/toolkit/components/places/src/nsNavHistory.cpp
@@ -2129,31 +2129,34 @@ nsNavHistory::ExecuteQueries(nsINavHisto
   return NS_OK;
 }
 
 // determine from our nsNavHistoryQuery array and nsNavHistoryQueryOptions
 // if this is the place query from the history menu.
 // from browser-menubar.inc, our history menu query is:
 // place:type=0&sort=4&maxResults=10
 // note, any maxResult > 0 will still be considered a history menu query
+// or if this is the place query from the "Most Visited" item in the "Smart Bookmarks" folder:
+// place:queryType=0&sort=8&maxResults=10
+// note, any maxResult > 0 will still be considered a Most Visited menu query
 static
-PRBool IsHistoryMenuQuery(const nsCOMArray<nsNavHistoryQuery>& aQueries, nsNavHistoryQueryOptions *aOptions)
+PRBool IsHistoryMenuQuery(const nsCOMArray<nsNavHistoryQuery>& aQueries, nsNavHistoryQueryOptions *aOptions, PRUint16 aSortMode)
 {
   if (aQueries.Count() != 1)
     return PR_FALSE;
 
   nsNavHistoryQuery *aQuery = aQueries[0];
  
   if (aOptions->QueryType() != nsINavHistoryQueryOptions::QUERY_TYPE_HISTORY)
     return PR_FALSE;
 
   if (aOptions->ResultType() != nsINavHistoryQueryOptions::RESULTS_AS_URI)
     return PR_FALSE;
 
-  if (aOptions->SortingMode() != nsINavHistoryQueryOptions::SORT_BY_DATE_DESCENDING)
+  if (aOptions->SortingMode() != aSortMode)
     return PR_FALSE;
 
   if (aOptions->MaxResults() <= 0)
     return PR_FALSE;
 
   PRUint32 groupCount;
   const PRUint16* groupings = aOptions->GroupingMode(&groupCount);
   if (groupings || groupCount)
@@ -2237,17 +2240,18 @@ nsNavHistory::ConstructQueryString(const
   PRInt32 sortingMode = aOptions->SortingMode();
   if (sortingMode < 0 ||
       sortingMode > nsINavHistoryQueryOptions::SORT_BY_ANNOTATION_DESCENDING) {
     return NS_ERROR_INVALID_ARG;
   }
 
   // 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
     // visit_type <> 0 == undefined (see bug #375777 for details)
     queryString = NS_LITERAL_CSTRING(
       "SELECT h.id, h.url, h.title, h.rev_host, h.visit_count, "
       "MAX(v.visit_date), f.url, null, null "
       "FROM moz_places h "
       "JOIN moz_historyvisits v ON h.id = v.place_id "
       "LEFT OUTER JOIN moz_favicons f ON h.favicon_id = f.id WHERE "
@@ -2255,16 +2259,37 @@ nsNavHistory::ConstructQueryString(const
       " moz_places h WHERE place_id = "
       " h.id AND hidden <> 1 AND visit_type <> 4 AND visit_type <> 0 "
       " ORDER BY visit_date DESC LIMIT ");
     queryString.AppendInt(aOptions->MaxResults());
     queryString += NS_LITERAL_CSTRING(")) GROUP BY h.id ORDER BY 6 DESC"); // v.visit_date
     return NS_OK;
   }
 
+  // for the most visited menu query
+  // we generate a super-optimized SQL query
+  if (IsHistoryMenuQuery(aQueries, aOptions, 
+        nsINavHistoryQueryOptions::SORT_BY_VISITCOUNT_DESCENDING)) {
+    // Note:  visit_date column is null, which is acceptable for
+    // a menu (as it is unused) and we are not sorting by it.  
+    // by not requiring the visit_date value, 
+    // we can avoid JOINing against the moz_historyvisits, 
+    // making this query very fast.
+    queryString = NS_LITERAL_CSTRING(
+      "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 ");
+    queryString.AppendInt(aOptions->MaxResults());
+    queryString += NS_LITERAL_CSTRING(") ORDER BY h.visit_count DESC");
+    return NS_OK;
+  }  
+
   PRBool asVisits =
     (aOptions->ResultType() == nsINavHistoryQueryOptions::RESULTS_AS_VISIT ||
      aOptions->ResultType() == nsINavHistoryQueryOptions::RESULTS_AS_FULL_VISIT);
 
   nsCAutoString commonConditions;
 
   if (aOptions->QueryType() == nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS) {
     // only look at bookmarks nodes
new file mode 100644
--- /dev/null
+++ b/toolkit/components/places/tests/unit/test_399266.js
@@ -0,0 +1,124 @@
+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* vim:set ts=2 sw=2 sts=2 et: */
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is mozilla.org code.
+ *
+ * The Initial Developer of the Original Code is Google Inc.
+ * Portions created by the Initial Developer are Copyright (C) 2005
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *  Darin Fisher <darin@meer.net>
+ *  Dietrich Ayala <dietrich@mozilla.com>
+ *  Dan Mills <thunder@mozilla.com>
+ *  Seth Spitzer <sspitzer@mozilla.com>
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+// Get history service
+try {
+  var histsvc = Cc["@mozilla.org/browser/nav-history-service;1"].getService(Ci.nsINavHistoryService);
+} catch(ex) {
+  do_throw("Could not get history service\n");
+} 
+
+// adds a test URI visit to the database, and checks for a valid place ID
+function add_visit(aURI, aType) {
+  var placeID = histsvc.addVisit(uri(aURI),
+                                 Date.now(),
+                                 0, // no referrer
+                                 aType,
+                                 false, // not redirect
+                                 0);
+  do_check_true(placeID > 0);
+  return placeID;
+}
+
+const TOTAL_SITES = 20;
+
+// main
+function run_test() {
+  // add visits
+  for (var i=0; i < TOTAL_SITES; i++) {
+    for (var j=0; j<=i; j++) {
+      add_visit("http://www.test-" + i + ".com/", histsvc.TRANSITION_TYPED);
+      // because these are embedded visits, they should not show up on our
+      // query results.  If they do, we have a problem.
+      add_visit("http://www.hidden.com/hidden.gif", histsvc.TRANSITION_EMBED);
+    }
+  }
+
+  // test our optimized query for the "Most Visited" item
+  // in the "Smart Bookmarks" folder
+  // place:queryType=0&sort=8&maxResults=10
+  // verify our visits AS_URI, ordered by visit count descending
+  // we should get 10 visits:
+  // http://www.test-19.com/
+  // ...
+  // http://www.test-10.com/
+  var options = histsvc.getNewQueryOptions();
+  options.sortingMode = options.SORT_BY_VISITCOUNT_DESCENDING;
+  options.maxResults = 10;
+  options.resultType = options.RESULTS_AS_URI;
+  var query = histsvc.getNewQuery();
+  var result = histsvc.executeQuery(query, options);
+  var root = result.root;
+  root.containerOpen = true;
+  var cc = root.childCount;
+  do_check_eq(cc, options.maxResults);
+  for (var i=0; i < cc; i++) {
+    var node = root.getChild(i);
+    var site = "http://www.test-" + (TOTAL_SITES - 1 - i) + ".com/";
+    do_check_eq(node.uri, site);
+    do_check_eq(node.type, options.RESULTS_AS_URI);
+  }
+  root.containerOpen = false;
+
+  // test without a maxResults, which executes a different query
+  // but the first 10 results should be the same.
+  // verify our visits AS_URI, ordered by visit count descending
+  // we should get 20 visits, but the first 10 should be
+  // http://www.test-19.com/
+  // ...
+  // http://www.test-10.com/
+  var options = histsvc.getNewQueryOptions();
+  options.sortingMode = options.SORT_BY_VISITCOUNT_DESCENDING;
+  options.resultType = options.RESULTS_AS_URI;
+  var query = histsvc.getNewQuery();
+  var result = histsvc.executeQuery(query, options);
+  var root = result.root;
+  root.containerOpen = true;
+  var cc = root.childCount;
+  do_check_eq(cc, TOTAL_SITES);
+  for (var i=0; i < 10; i++) {
+    var node = root.getChild(i);
+    var site = "http://www.test-" + (TOTAL_SITES - 1 - i) + ".com/";
+    do_check_eq(node.uri, site);
+    do_check_eq(node.type, options.RESULTS_AS_URI);
+  }
+  root.containerOpen = false;
+}