Closed Bug 488966 Opened 15 years ago Closed 15 years ago

Add a last_visit_date column with an index to moz_places

Categories

(Toolkit :: Places, defect, P1)

Product:
Toolkit
Component:
Places
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: mak, Assigned: mak)

References

Details

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

Attachments

(2 files, 7 obsolete files)

patch v1.1 (As pushed)
58.28 KB, patch
Details | Diff | Splinter Review
unbitrot patch for 1.9.1
60.35 KB, patch
shaver
: approval1.9.1+
Details | Diff | Splinter Review 
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.
Blocks: 488968
Marking w191+ just in case -- would be a huge win for that primary-chrome menu as you say.
Flags: wanted1.9.1+
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.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Blocks: 490653
Priority: -- → P1
Attached patch wip v1.0 (obsolete) — — Splinter Review 
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.
just for a comparison, a local query taking 2 seconds, now ends up in 20ms.
(In reply to comment #4)
> just for a comparison, a local query taking 2 seconds, now ends up in 20ms.

o_O
Attached patch wip v0.9 (obsolete) — — Splinter Review 
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.
Attachment #375410 - Attachment is obsolete: true
!!!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/
Whiteboard: [TSnappiness]
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.
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.
It would also be difficult for us to draw UI while blocking startup AFAIK.
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?
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.
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.
Attached patch patch v0.91 (obsolete) — — Splinter Review 
more work. i think i'll stop here changes, and start thinking to dowgrade path.
Attachment #375697 - Attachment is obsolete: true
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.
Attachment #376021 - Flags: review-
Depends on: 491954
(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)
(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?
(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
Ah, OK.  That wasn't clear to me.
Whiteboard: [TSnappiness] → [TSnappiness][needs unbitrot and cleanup]
(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.
Attached patch patch v0.99 (obsolete) — — Splinter Review 
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.
Attachment #376021 - Attachment is obsolete: true
Attachment #376831 - Flags: review?(sdwilsh)
Attached patch patch v0.991 (obsolete) — — Splinter Review 
fixed a wrong pragma
Attachment #376831 - Attachment is obsolete: true
Attachment #376833 - Flags: review?(sdwilsh)
Attachment #376831 - Flags: review?(sdwilsh)
Whiteboard: [TSnappiness][needs unbitrot and cleanup] → [TSnappiness][needs final testing and review]
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.
Attachment #376833 - Flags: review?(sdwilsh) → review+
Blocks: 489900
Attached patch patch v1.0 (obsolete) — — Splinter Review 
Addressed all comments, pushed to the tryserver to check everything is fine.
Attachment #376833 - Attachment is obsolete: true
Attachment #377308 - Flags: review?(sdwilsh)
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.
Whiteboard: [TSnappiness][needs final testing and review] → [TSnappiness]
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.
Attachment #377308 - Flags: review?(sdwilsh) → review+
be faster.
http://hg.mozilla.org/mozilla-central/rev/38d50cc03a72
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1

        Attachment #377308 -
        Attachment is obsolete: true

    
  
Depends on: 493291

    
  
Depends on: 493538

    
    

    
  


  
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.

        Attachment #378108 -
        Flags: approval1.9.1?

        Attachment #378108 -
        Flags: approval1.9.1? → approval1.9.1+

    
    

    
  
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.
Keywords: relnote

    
    

    
  
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1

    
  
Depends on: 496542

    
    

    
      
  

    
    

    
      
  

        Attachment #9188235 -
        Attachment is obsolete: true

    
    

    
      
  

    
    

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


Flags: needinfo?(priyasrm964)
Restrict Comments: true

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: