488966 - Add a last_visit_date column with an index to moz_places

Status: VERIFIED FIXED

(Toolkit :: Places, defect, P1)

Description

[Marco Bonardo [:mak]]

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 emeritus)]

Marking w191+ just in case -- would be a huge win for that primary-chrome menu as you say.

Comment 2

[Marco Bonardo [:mak]]

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]]

Attachment: 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]]

just for a comparison, a local query taking 2 seconds, now ends up in 20ms.

Comment 5

[Mike Shaver (:shaver emeritus)]

(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]]

Attachment: 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]]

!!!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:[e-mail]-try-a9acad940df/

Comment 8

[Dietrich Ayala (:dietrich)]

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]]

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]

It would also be difficult for us to draw UI while blocking startup AFAIK.

Comment 11

[Marco Bonardo [:mak]]

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]

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]]

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]]

Attachment: patch v0.91

more work. i think i'll stop here changes, and start thinking to dowgrade path.

Comment 15

[Shawn Wilsher :sdwilsh]

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]]

(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]

(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]]

(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]

Ah, OK.  That wasn't clear to me.

Comment 20

[Marco Bonardo [:mak]]

(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]]

Attachment: 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]]

Attachment: patch v0.991

fixed a wrong pragma

Comment 23

[Shawn Wilsher :sdwilsh]

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]]

Attachment: patch v1.0

Addressed all comments, pushed to the tryserver to check everything is fine.

Comment 25

[Marco Bonardo [:mak]]

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]

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]]

be faster.
http://hg.mozilla.org/mozilla-central/rev/38d50cc03a72

Comment 28

[Marco Bonardo [:mak]]

Attachment: patch v1.1 (As pushed)

Comment 29

[Marco Bonardo [:mak]]

Attachment: 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]

At shaver's request:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9b5c1b3ba645

Comment 31

[Dietrich Ayala (:dietrich)]

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 33

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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

Comment 38

[Mark Banner (:standard8)]

Restricting comments on this bug due to being closed 12 years ago and currently attracting spam.