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)
Toolkit
Places
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)
19.31 KB,
patch
|
Details | Diff | Splinter Review | |
17.61 KB,
patch
|
Details | Diff | Splinter Review |
History menu query is quite complex and damn slow, especially on trunk, we should do better.
Assignee | ||
Comment 1•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Attachment #372042 -
Attachment is patch: true
Attachment #372042 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Comment 2•15 years ago
|
||
Dietrich, could you try this since you see the issue and tell if you notice any difference?
Assignee | ||
Comment 3•15 years ago
|
||
changing bug title to make it more searchable
Summary: History menu takes ages to open → History menu is slow and takes ages to open
Assignee | ||
Comment 4•15 years ago
|
||
Comment on attachment 372042 [details] [diff] [review] patch v1.0 unluckily this is missing a check
Attachment #372042 -
Attachment is obsolete: true
Assignee | ||
Comment 5•15 years ago
|
||
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).
Assignee | ||
Updated•15 years ago
|
Attachment #372126 -
Flags: review?(dietrich)
Assignee | ||
Comment 6•15 years ago
|
||
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?
Comment 8•15 years ago
|
||
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.
Assignee | ||
Comment 10•15 years ago
|
||
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.
Assignee | ||
Comment 11•15 years ago
|
||
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?
Assignee | ||
Updated•15 years ago
|
Attachment #373128 -
Flags: review? → review?(dietrich)
Comment 12•15 years ago
|
||
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 13•15 years ago
|
||
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 14•15 years ago
|
||
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
Updated•15 years ago
|
Flags: blocking1.9.1? → blocking1.9.1+
Priority: -- → P2
Comment 15•15 years ago
|
||
Comment on attachment 373128 [details] [diff] [review] branch only patch r=me
Attachment #373128 -
Flags: review?(dietrich) → review+
Comment 16•15 years ago
|
||
talked on irc, mak's going to add a test the specifically tests the output of the optimized query,.
Comment 17•15 years ago
|
||
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]
Assignee | ||
Comment 18•15 years ago
|
||
(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.
Assignee | ||
Comment 19•15 years ago
|
||
filed bug 488966 to add the column+index that should make this really faster.
Assignee | ||
Comment 20•15 years ago
|
||
Attachment #372126 -
Attachment is obsolete: true
Assignee | ||
Comment 21•15 years ago
|
||
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*
Assignee | ||
Comment 23•15 years ago
|
||
thanks, i hope and think bug 488966 will bring even more. *blush*
Flags: in-testsuite?
Assignee | ||
Comment 24•15 years ago
|
||
trunk patch: http://hg.mozilla.org/mozilla-central/rev/55cb6c5991d1
Whiteboard: [TSnappiness][has patch][has review] → [TSnappiness]
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Updated•15 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Assignee | ||
Comment 25•15 years ago
|
||
branch patch: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/aa493c1be6e0
Keywords: fixed1.9.1
Comment 26•15 years ago
|
||
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?
Assignee | ||
Comment 27•15 years ago
|
||
which version? How is your history setup? For further improvements you'll need to look into bug 488966.
Comment 28•15 years ago
|
||
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.
Assignee | ||
Comment 29•15 years ago
|
||
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.
Comment 30•15 years ago
|
||
last nightly. default history settings.
Comment 31•15 years ago
|
||
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).
Assignee | ||
Comment 32•15 years ago
|
||
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.
Comment 33•15 years ago
|
||
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
Keywords: fixed1.9.1 → verified1.9.1
And I quote: "omg. history menu so much better" Thanks a ton!
You need to log in
before you can comment on or make changes to this bug.
Description
•