Last Comment Bug 488966 - Add a last_visit_date column with an index to moz_places
: Add a last_visit_date column with an index to moz_places
Status: VERIFIED FIXED
[TSnappiness]
: perf, relnote, verified1.9.1
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: P1 normal with 2 votes (vote)
: mozilla1.9.2a1
Assigned To: Marco Bonardo [::mak]
:
Mentors:
: 490653 (view as bug list)
Depends on: 491954 493291 493538 496542
Blocks: 488968 489900 490653
  Show dependency treegraph
 
Reported: 2009-04-18 03:16 PDT by Marco Bonardo [::mak]
Modified: 2010-12-17 06:29 PST (History)
19 users (show)
shaver: wanted1.9.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip v1.0 (51.96 KB, patch)
2009-05-01 15:27 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Review
wip v0.9 (55.20 KB, patch)
2009-05-04 13:17 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Review
patch v0.91 (66.67 KB, patch)
2009-05-06 09:23 PDT, Marco Bonardo [::mak]
sdwilsh: review-
Details | Diff | Review
patch v0.99 (58.35 KB, patch)
2009-05-11 18:00 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Review
patch v0.991 (58.36 KB, patch)
2009-05-11 18:11 PDT, Marco Bonardo [::mak]
sdwilsh: review+
Details | Diff | Review
patch v1.0 (58.01 KB, patch)
2009-05-13 17:51 PDT, Marco Bonardo [::mak]
sdwilsh: review+
Details | Diff | Review
patch v1.1 (As pushed) (58.28 KB, patch)
2009-05-14 16:34 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Review
unbitrot patch for 1.9.1 (60.35 KB, patch)
2009-05-18 13:05 PDT, Marco Bonardo [::mak]
shaver: approval1.9.1+
Details | Diff | Review

Description Marco Bonardo [::mak] 2009-04-18 03:16:29 PDT
We were not in deep need of this column before temp tables, but now some query can become really slow without being able to sort on an index.
We should add this column and check if the gain is worth doing this.

A possible first target is history menu, it's damn slow due to missing index, should take less than 100ms, while on large DBs we are spending seconds.
Comment 1 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-04-18 04:38:07 PDT
Marking w191+ just in case -- would be a huge win for that primary-chrome menu as you say.
Comment 2 Marco Bonardo [::mak] 2009-04-18 04:40:57 PDT
yeah, i'm not sure if we want to take a schema change so late (it would also require changes to our sync triggers), but in case would be good.
Comment 3 Marco Bonardo [::mak] 2009-05-01 15:27:43 PDT
Created attachment 375410 [details] [diff] [review]
wip v1.0

actually i'm not even sure this compiles correctly since i have to finish a clobber of my local build and my laptop is about to explode. btw, numbers are good, so we should have some interesting gain.  Will post final patch once i'm at home.
Comment 4 Marco Bonardo [::mak] 2009-05-01 15:28:31 PDT
just for a comparison, a local query taking 2 seconds, now ends up in 20ms.
Comment 5 Mike Shaver (:shaver -- probably not reading bugmail closely) 2009-05-01 15:35:20 PDT
(In reply to comment #4)
> just for a comparison, a local query taking 2 seconds, now ends up in 20ms.

o_O
Comment 6 Marco Bonardo [::mak] 2009-05-04 13:17:55 PDT
Created attachment 375697 [details] [diff] [review]
wip v0.9

for anyone willing to try how this performs.
I'm going to push this on tryserver too, will post link for download later when it's done.
Comment 7 Marco Bonardo [::mak] 2009-05-04 15:30:43 PDT
!!!WARNING!!!
Don't use on your default profile, these will upgrade your schema.

builds will appear here:
https://build.mozilla.org/tryserver-builds/2009-05-04_13:23-mak77@bonardo.net-try-a9acad940df/
Comment 8 Dietrich Ayala (:dietrich) 2009-05-04 20:34:30 PDT
tested the patch on a debug build w/ my punishing profile and the history menu opened *instantly*.

the cost of this is a significant (5-10 second) delay during the first post-update startup, as migration occurs. we could probably live with this since it only occurs once, immediately after a major update. ideally, we should throw up a progress bar or some other visual indication that actual work is occurring.
Comment 9 Marco Bonardo [::mak] 2009-05-05 06:24:27 PDT
my concern about the progress bar is that it should be a window on itself since the browser window is not up, and would hit all places users (fennec, seamonkey, ...). So i'm not sure it is what we really want, it would also require some string.
Comment 10 Shawn Wilsher :sdwilsh 2009-05-05 06:58:40 PDT
It would also be difficult for us to draw UI while blocking startup AFAIK.
Comment 11 Marco Bonardo [::mak] 2009-05-05 07:11:47 PDT
my current idea is to temporary move journal file to memory while i execute the update and then set it back to disk, this should reduce by a half our I/O time for that query, allowing us to end the UPDATE in about 4s (actually sqlite has to put all the table in the journal file, and than back to the database).
The drawbacks of doing that is that if we crash while updating there's a small risk of corruption, but since it only happens once and for 4s calling a single sqlite method, i think we could afford that risk.
Shawn, what do you think about that?
Comment 12 Shawn Wilsher :sdwilsh 2009-05-05 07:15:28 PDT
Sounds fine - for the future, we could possibly use the live back up api to copy the db, and peform the work on it asynchronously, and then swap the files too.
Comment 13 Marco Bonardo [::mak] 2009-05-05 15:28:37 PDT
there's a problem with downgrades, adding a column to moz_places will practically make impossible to downgrade to previous version, since dbflush queries expect the same columns in temp and disk tables. So we need to define a downgrade strategy now.
Comment 14 Marco Bonardo [::mak] 2009-05-06 09:23:41 PDT
Created attachment 376021 [details] [diff] [review]
patch v0.91

more work. i think i'll stop here changes, and start thinking to dowgrade path.
Comment 15 Shawn Wilsher :sdwilsh 2009-05-07 11:39:13 PDT
Comment on attachment 376021 [details] [diff] [review]
patch v0.91

This review was unasked for, but I'm just trying to help move this bug along - I swear!

In general, it'd be nice if you broke this into two patches - the first is the refactoring, and the second is the actual changes.  Makes the reviews much easier ;)  The whitespace changes in your refactoring make it a lot harder to see what is actually changing.

>         "CREATE INDEX moz_bookmarks_parentindex "
>-        "ON moz_bookmarks (parent, position)"));
>+          "ON moz_bookmarks (parent, position)"));
Could you please pull all of our index statements out into nsPlacesIndexes.h please?  It'll be easier for us in the long run to be able to see our entire schema because we can then just look at nsPlacesTiggers.h, nsPlacesTables.h, and nsPlacesIndexes.h

>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>+// We use the TRUNCATE journal mode to reduce the number of fsyncs.  Without
>+// this setting we had a Ts hit on Linux.  See bug 460315 for details.
>+#define DEFAULT_JOURNAL_MODE_QUERY NS_LITERAL_CSTRING("PRAGMA journal_mode = TRUNCATE")
Why are you pulling this up to here?

> nsNavHistory::InitDB()
>+  PRBool databaseInitialized;
>+  PRInt32 pageSize = DEFAULT_DB_PAGE_SIZE;
>+
>+  // Initialize the places schema version if this is first run.
>+  nsresult rv = mDBConn->TableExists(NS_LITERAL_CSTRING("moz_places"),
>+                                     &databaseInitialized);
just check mDBConn->GetSchemaVersion - it should be 0.  You need this value later anyway.

>+    nsCAutoString pageSizePragma("PRAGMA page_size=");
>+    pageSizePragma.AppendInt(pageSize);
>+    rv = mDBConn->ExecuteSimpleSQL(pageSizePragma);
>+    NS_ENSURE_SUCCESS(rv, rv);
really would prefer a bound statement here

>+nsNavHistory::MigrateV9Up(mozIStorageConnection *aDBConn)
>+{
>+  mozStorageTransaction transaction(aDBConn, PR_FALSE);
>+  // Bug 488966.  We add a new last_visit_date column that will cache the last
>+  // visit date, its contents are automatically upgraded on every added or
>+  // removed visit through triggers.  This enhances SELECT performances when we
name the trigger(s) in the comment please

>+    // Adding the index later would not give us anything in terms of speed when
>+    // filling values, so we add it immediately.
comment doesn't really make sense here

>+++ b/toolkit/components/places/src/nsPlacesTriggers.h
>@@ -154,11 +154,14 @@
>     "SELECT * " \
>     "FROM moz_places " \
>     "WHERE id = NEW.place_id " \
>-    "AND NEW.visit_type NOT IN (0, 4, 7); " \
>+     "AND NEW.visit_type NOT IN (0, 4, 7); " \
>     "UPDATE moz_places_temp " \
>     "SET visit_count = visit_count + 1 " \
>     "WHERE id = NEW.place_id " \
>-    "AND NEW.visit_type NOT IN (0, 4, 7); " /* invalid, EMBED, DOWNLOAD */ \
>+     "AND NEW.visit_type NOT IN (0, 4, 7); " \
nit: indent spacing is inconsistent with other places

>+    "UPDATE moz_places_temp " \
>+    "SET last_visit_date = MAX(IFNULL(last_visit_date, 0), NEW.visit_date)" \
>+    "WHERE id = NEW.place_id;" \
add a comment explaining what this is doing please

>@@ -183,11 +186,19 @@
>     "SELECT * " \
>     "FROM moz_places " \
>     "WHERE id = OLD.place_id " \
>-    "AND OLD.visit_type NOT IN (0, 4, 7); " \
>+      "AND OLD.visit_type NOT IN (0, 4, 7); " \
>     "UPDATE moz_places_temp " \
>     "SET visit_count = visit_count - 1 " \
>     "WHERE id = OLD.place_id " \
>-    "AND OLD.visit_type NOT IN (0, 4, 7); " /* invalid, EMBED, DOWNLOAD */ \
>+      "AND OLD.visit_type NOT IN (0, 4, 7); " \
>+    "UPDATE moz_places_temp " \
>+      "SET last_visit_date = (SELECT visit_date FROM moz_historyvisits_temp " \
>+                             "WHERE place_id = OLD.place_id " \
>+                             "UNION ALL " \
>+                             "SELECT visit_date FROM moz_historyvisits " \
>+                             "WHERE place_id = OLD.place_id " \
>+                             "ORDER BY visit_date DESC LIMIT 1) " \
nit: put the subselect on a new line indented please?

>+++ b/toolkit/components/places/src/nsTaggingService.js
I think these changes are for a different bug?

>+++ b/toolkit/components/places/tests/queries/head_queries.js
>@@ -375,6 +375,11 @@ function compareArrayToResult(aArray, aR
> 
>   // check expected number of results against actual
>   var expectedResultCount = aArray.filter(function(aEl) { return aEl.isInQuery; }).length;
>+
>+  if (expectedResultCount != aRoot.childCount) {
>+    for (i = 0; i < aRoot.childCount; i++)
>+      LOG("testing result[" + aRoot.getChild(i).uri + "]");
>+  }
What is this for?

>+++ b/toolkit/components/places/tests/queries/test_redirectsMode.js
>@@ -265,6 +265,14 @@ function add_visits_to_database() {
>       visitCount: visitCount++,
>       isInQuery: true }));
> 
>+  // Add an unvisited bookmark in the database, it should never appear.
>+  visits.push({ isBookmark: true,
>+    uri: "http://unvisited.bookmark.com/",
>+    parentFolder: bmsvc.bookmarksMenuFolder,
>+    index: bmsvc.DEFAULT_INDEX,
>+    title: "Unvisited Bookmark",
>+    isInQuery: false });
same with this - what's it for?

As for the down grading stuff, I don't think we have to worry about it.  The existing code can handle extra columns just fine, so if we add one things won't break.  Same with the index and triggers.  As long as we set the schema version to the current version always, we'll be fine.
Comment 16 Marco Bonardo [::mak] 2009-05-11 02:19:06 PDT
(In reply to comment #15)
> >+++ b/toolkit/components/places/src/nsNavHistory.cpp
> >+// We use the TRUNCATE journal mode to reduce the number of fsyncs.  Without
> >+// this setting we had a Ts hit on Linux.  See bug 460315 for details.
> >+#define DEFAULT_JOURNAL_MODE_QUERY NS_LITERAL_CSTRING("PRAGMA journal_mode = TRUNCATE")
> Why are you pulling this up to here?

Since i need to temporarly change this value to MEMORY, i want a common place to set the default value. i've put the squery here, but i could also #define DEFAULT_JOURNAL_MODE "TRUNCATE" and leave the query below.

> >+++ b/toolkit/components/places/src/nsTaggingService.js
> I think these changes are for a different bug?

yep, patches pollution, my queue is becoming too long.

> >+++ b/toolkit/components/places/tests/queries/test_redirectsMode.js
> >@@ -265,6 +265,14 @@ function add_visits_to_database() {
> >       visitCount: visitCount++,
> >       isInQuery: true }));
> > 
> >+  // Add an unvisited bookmark in the database, it should never appear.
> >+  visits.push({ isBookmark: true,
> >+    uri: "http://unvisited.bookmark.com/",
> >+    parentFolder: bmsvc.bookmarksMenuFolder,
> >+    index: bmsvc.DEFAULT_INDEX,
> >+    title: "Unvisited Bookmark",
> >+    isInQuery: false });
> same with this - what's it for?

This is a wanted change, checks that we don't show unvisited bookmarks in history menu. Something we regressed on trunk (not on branch), so better having a test for it.(In reply to comment #15)
Comment 17 Shawn Wilsher :sdwilsh 2009-05-11 09:31:38 PDT
(In reply to comment #16)
> Since i need to temporarly change this value to MEMORY, i want a common place
> to set the default value. i've put the squery here, but i could also #define
> DEFAULT_JOURNAL_MODE "TRUNCATE" and leave the query below.
I would rather you do that, and not have the SQL there.

> This is a wanted change, checks that we don't show unvisited bookmarks in
> history menu. Something we regressed on trunk (not on branch), so better having
> a test for it.
That's not related to this bug though, is it?
Comment 18 Marco Bonardo [::mak] 2009-05-11 09:45:34 PDT
(In reply to comment #17)
> (In reply to comment #16)
> > This is a wanted change, checks that we don't show unvisited bookmarks in
> > history menu. Something we regressed on trunk (not on branch), so better having
> > a test for it.
> That's not related to this bug though, is it?

it is because fixing the query also fixes that bug, so i'm adding a test
Comment 19 Shawn Wilsher :sdwilsh 2009-05-11 09:47:37 PDT
Ah, OK.  That wasn't clear to me.
Comment 20 Marco Bonardo [::mak] 2009-05-11 17:10:56 PDT
(In reply to comment #15)
> (From update of attachment 376021 [details] [diff] [review])
> >+    nsCAutoString pageSizePragma("PRAGMA page_size=");
> >+    pageSizePragma.AppendInt(pageSize);
> >+    rv = mDBConn->ExecuteSimpleSQL(pageSizePragma);
> >+    NS_ENSURE_SUCCESS(rv, rv);
> really would prefer a bound statement here

looks like it is not possible, trying to
CreateStatement(NS_LITERAL_CSTRING("PRAGMA anything = ?1"));
always throws here.
Comment 21 Marco Bonardo [::mak] 2009-05-11 18:00:41 PDT
Created attachment 376831 [details] [diff] [review]
patch v0.99

sorry, i did not yet had time to split this, but now that indexes have been moved apart, the patch is much more clear and smaller. If you think still makes sense to split, let me know.
i'm still thinking to a couple details, but for eventual next changes we can probably review interdiffs, i won't add anything, at last will fix something that is already here.
Also i want to execute more migration tests to be sure we are safe.
Comment 22 Marco Bonardo [::mak] 2009-05-11 18:11:07 PDT
Created attachment 376833 [details] [diff] [review]
patch v0.991

fixed a wrong pragma
Comment 23 Shawn Wilsher :sdwilsh 2009-05-12 01:54:39 PDT
Comment on attachment 376833 [details] [diff] [review]
patch v0.991

Given the patch size and complexity, I probably shouldn't be looking at this at this hour, but whatever.

>+++ b/toolkit/components/places/src/nsNavHistory.cpp
>+  PRBool databaseInitialized = (DBSchemaVersion > 0);
You can just use a regular bool here.  Not going through xpcom.

>@@ -1047,9 +1055,7 @@ nsresult
> nsresult
> nsNavHistory::InitFunctions()
> {
>-  nsresult rv;
>-
>-  rv = mDBConn->CreateFunction(
>+  nsresult rv = mDBConn->CreateFunction(
can you do me a favor and please not make this change?  I've already cleaned it up in a different bug, and you'll bit rot me. (but 455555)

>+nsresult
>+nsNavHistory::MigrateV9Up(mozIStorageConnection *aDBConn)
>+{
>+  mozStorageTransaction transaction(aDBConn, PR_FALSE);
>+  // Bug 488966.  We add a new last_visit_date column that will cache the last
nit: say something like "Added in bug 488966" please

>-  while ((statement->ExecuteStep(&hasMore) == NS_OK) && hasMore) {
>+  while (NS_SUCCEEDED(statement->ExecuteStep(&hasMore)) && hasMore) {
Did we really do that?  That makes me sad :(

>+++ b/toolkit/components/places/src/nsNavHistory.h
>-#define SQL_STR_FRAGMENT_MAX_VISIT_DATE_BASE( __place_relation, __table_name ) \
>-#define SQL_STR_FRAGMENT_MAX_VISIT_DATE( __place_relation ) \
I can't tell you how happy I am to see this go away

This looks good.  I'm pretty happy about some of these changes.  r=sdwilsh, and we can do the rest in a follow-up likely.
Comment 24 Marco Bonardo [::mak] 2009-05-13 17:51:50 PDT
Created attachment 377308 [details] [diff] [review]
patch v1.0

Addressed all comments, pushed to the tryserver to check everything is fine.
Comment 25 Marco Bonardo [::mak] 2009-05-14 07:31:12 PDT
Comment on attachment 377308 [details] [diff] [review]
patch v1.0

I think we could push this. So, asking for a final review.

I tried moving a profile to this version, and then fw and back with 3.0.x, and i've not noticed any issue.
Comment 26 Shawn Wilsher :sdwilsh 2009-05-14 13:20:03 PDT
Comment on attachment 377308 [details] [diff] [review]
patch v1.0

>+        "WHERE 1=1 "
I realize you didn't actually change this, but you should be able to do just "WHERE 1".  We should probably have a small comment about why we do that too.

>+          "AND EXISTS (SELECT id FROM moz_historyvisits_temp WHERE place_id = h.id "
>+                        "AND visit_type NOT IN (0,4) "
>+                       "UNION ALL "
>+                        "SELECT id FROM moz_historyvisits WHERE place_id = h.id "
>+                        "AND visit_type NOT IN (0,4) "
>+                        "LIMIT 1) "
Elsewhere you've used the constants instead of embedding the numbers in the string.  You should probably continue to do that.
Also, the indentation seems odd here.  I think you want it indented one more space?

>+          "AND EXISTS (SELECT id FROM moz_historyvisits_temp WHERE place_id = h.id "
>+                        "AND visit_type NOT IN (0,4) "
>+                       "UNION ALL "
>+                        "SELECT id FROM moz_historyvisits WHERE place_id = h.id "
>+                        "AND visit_type NOT IN (0,4) "
>+                        "LIMIT 1) "
ditto

r=sdwilsh with these changes.
Comment 27 Marco Bonardo [::mak] 2009-05-14 16:32:59 PDT
be faster.
http://hg.mozilla.org/mozilla-central/rev/38d50cc03a72
Comment 28 Marco Bonardo [::mak] 2009-05-14 16:34:41 PDT
Created attachment 377562 [details] [diff] [review]
patch v1.1 (As pushed)
Comment 29 Marco Bonardo [::mak] 2009-05-18 13:05:19 PDT
Created attachment 378108 [details] [diff] [review]
unbitrot patch for 1.9.1

Asking approval for 1.9.1.
This speeds up most of our queries makeing them much better to maintain too, but especially highly visible history queries like history menu (moving it from seconds to milliseconds).
There's a medium risk but practically all paths have some sort of unit test covering them. Btw, the gain highly overloads the risks.

The only known drawback is that this will make impossible to downgrade from RC to a previous beta (or any 1.9.1/trunk nightly after temp tables and before 15/05). This is sadly not fixable due to a separate bug we just fixed on both branches. we have bug 492379 to provide a script to allow such downgrades.
There is no problem downgrading to any 3.0.x version though.
Comment 30 Shawn Wilsher :sdwilsh 2009-05-19 14:52:20 PDT
At shaver's request:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9b5c1b3ba645
Comment 31 Dietrich Ayala (:dietrich) 2009-05-22 09:16:16 PDT
See comment #29. We need to relnote that you can't downgrade to previous 3.1/3.5 betas after this change. Downgrades to 3.0 works ok.
Comment 32 Dietrich Ayala (:dietrich) 2009-05-22 10:58:56 PDT
*** Bug 490653 has been marked as a duplicate of this bug. ***
Comment 33 Henrik Skupin (:whimboo) 2009-05-25 04:32:18 PDT
Marking verified fixed on trunk and 1.9.1 after all my testing in the last days on all platforms. The speed-up looks fantastic.

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/20090524 Shiretoko/3.5pre ID:20090524031149

Note You need to log in before you can comment on or make changes to this bug.