Bug 1266232 - be extra careful and mindful of performance when migrating visits r=mcomella
authorGrigory Kruglov <gkruglov@mozilla.com>
Fri, 22 Apr 2016 23:18:04 -0700
changeset 318268 37f04460ddb76d6ef4e7c32a8a6b2fbc44cb8776
parent 318267 3bc647d9bb02ea0284ea7653161f49c9dd2982e4
child 318269 9498777fd5f90dcf3f1b1c587469b1093e54b0f4
child 318273 f4e12b2ddb63ea198ff141427875f4b2f6cfe2b3
child 318401 3a1cffbeb01b4dcf3a6694d3f985a46203abfbf8
push id9480
push userjlund@mozilla.com
push dateMon, 25 Apr 2016 17:12:58 +0000
treeherdermozilla-aurora@0d6a91c76a9e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmcomella
bugs1266232
milestone48.0a1
Bug 1266232 - be extra careful and mindful of performance when migrating visits r=mcomella 1) Use prepared SQL insert statement for insertions 1.2) Use ON CONFLICT IGNORE for our inserts, to avoid failing on possible data clashes 2) Don't synthesize "visits since last sync" - it's bound to cause problems, for not much benefit 3) Fix up some minor issues, cleanup code and add sanity checks 4) If there's evidence Sync was enabled at some point, mark synthsized visits as remote. Otherwise, as local. MozReview-Commit-ID: Gd94A6r4rW
mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java
mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java
@@ -222,16 +222,19 @@ public class BrowserContract {
 
     @RobocopTarget
     public static final class Visits implements CommonColumns, VisitsColumns {
         private Visits() {}
 
         public static final String TABLE_NAME = "visits";
 
         public static final Uri CONTENT_URI = Uri.withAppendedPath(AUTHORITY_URI, "visits");
+
+        public static final int VISIT_IS_LOCAL = 1;
+        public static final int VISIT_IS_REMOTE = 0;
     }
 
     // Combined bookmarks and history
     @RobocopTarget
     public static final class Combined implements CommonColumns, URLColumns, HistoryColumns, FaviconColumns  {
         private Combined() {}
 
         public static final String VIEW_NAME = "combined";
--- a/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java
+++ b/mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java
@@ -25,36 +25,35 @@ import org.mozilla.gecko.db.BrowserContr
 import org.mozilla.gecko.db.BrowserContract.Favicons;
 import org.mozilla.gecko.db.BrowserContract.History;
 import org.mozilla.gecko.db.BrowserContract.Visits;
 import org.mozilla.gecko.db.BrowserContract.Numbers;
 import org.mozilla.gecko.db.BrowserContract.ReadingListItems;
 import org.mozilla.gecko.db.BrowserContract.SearchHistory;
 import org.mozilla.gecko.db.BrowserContract.Thumbnails;
 import org.mozilla.gecko.db.BrowserContract.UrlAnnotations;
+import org.mozilla.gecko.fxa.FirefoxAccounts;
 import org.mozilla.gecko.reader.SavedReaderViewHelper;
 import org.mozilla.gecko.sync.Utils;
 import org.mozilla.gecko.sync.repositories.android.RepoUtils;
 import org.mozilla.gecko.util.FileUtils;
 
 import static org.mozilla.gecko.db.DBUtils.qualifyColumn;
 
 import android.content.ContentValues;
 import android.content.Context;
 import android.database.Cursor;
 import android.database.DatabaseUtils;
 import android.database.SQLException;
-import android.database.sqlite.SQLiteCantOpenDatabaseException;
 import android.database.sqlite.SQLiteDatabase;
 import android.database.sqlite.SQLiteException;
 import android.database.sqlite.SQLiteOpenHelper;
 import android.database.sqlite.SQLiteStatement;
 import android.net.Uri;
 import android.os.Build;
-import android.support.annotation.NonNull;
 import android.util.Log;
 
 
 // public for robocop testing
 public final class BrowserDatabaseHelper extends SQLiteOpenHelper {
     private static final String LOGTAG = "GeckoBrowserDBHelper";
 
     // Replace the Bug number below with your Bug that is conducting a DB upgrade, as to force a merge conflict with any
@@ -162,17 +161,17 @@ public final class BrowserDatabaseHelper
                 + History.GUID + ')');
         db.execSQL("CREATE INDEX history_modified_index ON " + TABLE_HISTORY + '('
                 + History.DATE_MODIFIED + ')');
         db.execSQL("CREATE INDEX history_visited_index ON " + TABLE_HISTORY + '('
                 + History.DATE_LAST_VISITED + ')');
     }
 
     private void createVisitsTable(SQLiteDatabase db) {
-        debug("Creating " + TABLE_VISITS + " talbe");
+        debug("Creating " + TABLE_VISITS + " table");
         db.execSQL("CREATE TABLE " + TABLE_VISITS + "(" +
                 Visits._ID + " INTEGER PRIMARY KEY AUTOINCREMENT," +
                 Visits.HISTORY_GUID + " TEXT NOT NULL," +
                 Visits.VISIT_TYPE + " TINYINT NOT NULL DEFAULT 1," +
                 Visits.DATE_VISITED + " INTEGER NOT NULL, " +
                 Visits.IS_LOCAL + " TINYINT NOT NULL DEFAULT 1, " +
 
                 "FOREIGN KEY (" + Visits.HISTORY_GUID + ") REFERENCES " +
@@ -517,143 +516,133 @@ public final class BrowserDatabaseHelper
     /**
      * We used to have a separate history extensions database which was used by Sync to store arrays
      * of visits for individual History GUIDs. It was only used by Sync.
      * This function migrates contents of that database over to the Visits table.
      *
      * @param historyExtensionDb Source History Extensions database
      * @param db Destination database
      */
-    private void copyHistoryExtensionDataToVisitsTable(SQLiteDatabase historyExtensionDb, SQLiteDatabase db) {
+    private void copyHistoryExtensionDataToVisitsTable(final SQLiteDatabase historyExtensionDb, final SQLiteDatabase db) {
         final String historyExtensionTable = "HistoryExtension";
         final String columnGuid = "guid";
         final String columnVisits = "visits";
 
-        final Cursor cursor = historyExtensionDb.query(historyExtensionTable,
+        final Cursor historyExtensionCursor = historyExtensionDb.query(historyExtensionTable,
                 new String[] {columnGuid, columnVisits},
                 null, null, null, null, null);
         // Ignore null or empty cursor, we can't (or have nothing to) copy at this point.
-        if (cursor == null) {
+        if (historyExtensionCursor == null) {
             return;
         }
         try {
-            if (!cursor.moveToFirst()) {
+            if (!historyExtensionCursor.moveToFirst()) {
                 return;
             }
 
-            final int guidCol = cursor.getColumnIndexOrThrow(columnGuid);
-            while (!cursor.isAfterLast()) {
-                final String guid = cursor.getString(guidCol);
-                final JSONArray visitsInHistoryExtensionDB = RepoUtils.getJSONArrayFromCursor(cursor, columnVisits);
+            final int guidCol = historyExtensionCursor.getColumnIndexOrThrow(columnGuid);
+
+            // Use prepared (aka "compiled") SQL statements because they are much faster when we're inserting
+            // lots of data. We avoid GC churn and recompilation of SQL statements on every insert.
+            // NB #1: OR IGNORE clause applies to UNIQUE, NOT NULL, CHECK, and PRIMARY KEY constraints.
+            // It does not apply to Foreign Key constraints, but in our case, at this point in time, foreign key
+            // constraints are disabled anyway.
+            // We care about OR IGNORE because we want to ensure that in case of (GUID,DATE)
+            // clash (the UNIQUE constraint), we will not fail the transaction, and just skip conflicting row.
+            // Clash might occur if visits array we got from Sync has duplicate (guid,date) records.
+            // NB #2: IS_LOCAL is always 0, since we consider all visits coming from Sync to be remote.
+            final String insertSqlStatement = "INSERT OR IGNORE INTO " + Visits.TABLE_NAME + " (" +
+                    Visits.DATE_VISITED + "," +
+                    Visits.VISIT_TYPE + "," +
+                    Visits.HISTORY_GUID + "," +
+                    Visits.IS_LOCAL + ") VALUES (?, ?, ?, " + Visits.VISIT_IS_REMOTE + ")";
+            final SQLiteStatement compiledInsertStatement = db.compileStatement(insertSqlStatement);
+
+            do {
+                final String guid = historyExtensionCursor.getString(guidCol);
+
+                // Sanity check, let's not risk a bad incoming GUID.
+                if (guid == null || guid.isEmpty()) {
+                    continue;
+                }
+
+                // First, check if history with given GUID exists in the History table.
+                // We might have a lot of entries in the HistoryExtensionDatabase whose GUID doesn't
+                // match one in the History table. Let's avoid doing unnecessary work by first checking if
+                // GUID exists locally.
+                // Note that we don't have foreign key constraints enabled at this point.
+                // See Bug 1266232 for details.
+                if (!isGUIDPresentInHistoryTable(db, guid)) {
+                    continue;
+                }
+
+                final JSONArray visitsInHistoryExtensionDB = RepoUtils.getJSONArrayFromCursor(historyExtensionCursor, columnVisits);
 
                 if (visitsInHistoryExtensionDB == null) {
                     continue;
                 }
 
-                debug("Inserting " + visitsInHistoryExtensionDB.size() + " visits from history extension db");
-                for (int i = 0; i < visitsInHistoryExtensionDB.size(); i++) {
-                    final ContentValues cv = new ContentValues();
+                final int histExtVisitCount = visitsInHistoryExtensionDB.size();
+
+                debug("Inserting " + histExtVisitCount + " visits from history extension db for GUID: " + guid);
+                for (int i = 0; i < histExtVisitCount; i++) {
                     final JSONObject visit = (JSONObject) visitsInHistoryExtensionDB.get(i);
 
-                    cv.put(Visits.DATE_VISITED, (Long) visit.get("date"));
-                    cv.put(Visits.VISIT_TYPE, (Long) visit.get("type"));
-                    cv.put(Visits.HISTORY_GUID, guid);
-                    // Visits which we are working with here arrived from Sync, so unfortunately we
-                    // can't tell how they originated. Our only sane choice here is to mark them all as remote.
-                    cv.put(Visits.IS_LOCAL, 0);
-
-                    // Ignore any failures due to constraint violations (for example, history table
-                    // missing GUID for whatever reason).
-                    db.insertWithOnConflict(Visits.TABLE_NAME, null, cv, SQLiteDatabase.CONFLICT_IGNORE);
-                }
+                    // Sanity check.
+                    if (visit == null) {
+                        continue;
+                    }
 
-                // We might have generated local visits before Sync had a chance to send them up.
-                // Below we figure out how many visits we have locally that Sync isn't aware of, and we
-                // "synthesize" them for insertion into Visits table. Synthesizing a visit entails
-                // generating a visit with a fake date, set to be just prior to the visited date for a given history item.
-                // We have to do this since prior to v31, local visit information was only preserved in aggregate.
-                // Example: t0: sync, t1: browse, t2: migrate, t3: sync
-                // We'll ensure that at t2 all of the visits from t1 will be "preserved", and at t3 they will get synced.
-                final Long baseVisitDateForSynthesis;
-                // set date which will be a base for our "synthesized" dates with an offset just prior
-                if (visitsInHistoryExtensionDB.size() > 0) {
-                    baseVisitDateForSynthesis = (Long) ((JSONObject) visitsInHistoryExtensionDB.get(0)).get("date") - 1;
-                } else {
-                    baseVisitDateForSynthesis = System.currentTimeMillis() - 1;
+                    // Let's not rely on underlying data being correct, and guard against casting failures.
+                    // Since we can't recover from this (other than ignoring this visit), let's not fail user's migration.
+                    final Long date;
+                    final Long visitType;
+                    try {
+                        date = (Long) visit.get("date");
+                        visitType = (Long) visit.get("type");
+                    } catch (ClassCastException e) {
+                        continue;
+                    }
+                    // Sanity check our incoming data.
+                    if (date == null || visitType == null) {
+                        continue;
+                    }
+
+                    // Bind parameters use a 1-based index.
+                    compiledInsertStatement.clearBindings();
+                    compiledInsertStatement.bindLong(1, date);
+                    compiledInsertStatement.bindLong(2, visitType);
+                    compiledInsertStatement.bindString(3, guid);
+                    compiledInsertStatement.executeInsert();
                 }
-
-                insertSynthesizedVisits(db,
-                        generateSynthesizedVisits(
-                                getNumberOfVisitsToSynthesize(db, guid, visitsInHistoryExtensionDB.size()),
-                                guid, baseVisitDateForSynthesis
-                        )
-                );
-
-                cursor.moveToNext();
-            }
+            } while (historyExtensionCursor.moveToNext());
         } finally {
             // We return on a null cursor, so don't have to check it here.
-            cursor.close();
+            historyExtensionCursor.close();
         }
     }
 
-    private int getNumberOfVisitsToSynthesize(@NonNull SQLiteDatabase db, @NonNull String guid, int baseNumberOfVisits) {
-        final int knownVisits;
-
-        final Cursor cursor = db.query(
-                TABLE_HISTORY,
-                new String[] {History.VISITS},
-                History.GUID + " = ?",
-                new String[] {guid},
+    private boolean isGUIDPresentInHistoryTable(final SQLiteDatabase db, String guid) {
+        final Cursor historyCursor = db.query(
+                History.TABLE_NAME,
+                new String[] {History.GUID}, History.GUID + " = ?", new String[] {guid},
                 null, null, null);
-        if (cursor == null) {
-            return 0;
+        if (historyCursor == null) {
+            return false;
         }
         try {
-            if (!cursor.moveToFirst()) {
-                Log.e(LOGTAG, "Expected to get history visit count with guid but failed: " + guid);
-                return 0;
+            // No history record found for given GUID
+            if (!historyCursor.moveToFirst()) {
+                return false;
             }
-
-            knownVisits = cursor.getInt(
-                    cursor.getColumnIndexOrThrow(History.VISITS));
         } finally {
-            cursor.close();
+            historyCursor.close();
         }
 
-        final int visitsToSynthesize = knownVisits - baseNumberOfVisits;
-
-        if (visitsToSynthesize < 0) {
-            Log.w(LOGTAG, guid + " # of visits(" + knownVisits + ") less than # of hist.ext.db visits(" + baseNumberOfVisits + ")");
-            return 0;
-        }
-
-        return visitsToSynthesize;
-    }
-
-    private ContentValues[] generateSynthesizedVisits(int numberOfVisits, @NonNull String guid, @NonNull Long baseDate) {
-        final ContentValues[] fakeVisits = new ContentValues[numberOfVisits];
-
-        for (int i = 0; i < numberOfVisits; i++) {
-            final ContentValues cv = new ContentValues();
-            // NB: visit type has a default value in schema
-            cv.put(Visits.DATE_VISITED, baseDate - i);
-            cv.put(Visits.HISTORY_GUID, guid);
-            cv.put(Visits.IS_LOCAL, 1);
-            fakeVisits[i] = cv;
-        }
-
-        return fakeVisits;
-    }
-
-    private void insertSynthesizedVisits(SQLiteDatabase db, ContentValues[] visits) {
-        debug("Inserting " + visits.length + " synthesized visits");
-        for (ContentValues visit : visits) {
-            db.insert(Visits.TABLE_NAME, null, visit);
-        }
+        return true;
     }
 
     private void createSearchHistoryTable(SQLiteDatabase db) {
         debug("Creating " + SearchHistory.TABLE_NAME + " table");
 
         db.execSQL("CREATE TABLE " + SearchHistory.TABLE_NAME + "(" +
                     SearchHistory._ID + " INTEGER PRIMARY KEY AUTOINCREMENT, " +
                     SearchHistory.QUERY + " TEXT UNIQUE NOT NULL, " +
@@ -1520,72 +1509,125 @@ public final class BrowserDatabaseHelper
     private void upgradeDatabaseFrom31to32(final SQLiteDatabase db) {
         debug("Adding visits table");
         createVisitsTable(db);
 
         debug("Migrating visits from history extension db into visits table");
         String historyExtensionDbName = "history_extension_database";
 
         SQLiteDatabase historyExtensionDb = null;
-        boolean historyExtensionsDbPresent = true;
+        final File historyExtensionsDatabase = mContext.getDatabasePath(historyExtensionDbName);
+
+        // Primary goal of this migration is to improve Top Sites experience by distinguishing between
+        // local and remote visits. If Sync is enabled, we rely on visit data from Sync and treat it as remote.
+        // However, if Sync is disabled but we detect evidence that it was enabled at some point (HistoryExtensionsDB is present)
+        // then we synthesize visits from the History table, but we mark them all as "remote". This will ensure
+        // that once user starts browsing around, their Top Sites will reflect their local browsing history.
+        // Otherwise, we risk overwhelming their Top Sites with remote history, just as we did before this migration.
         try {
-            historyExtensionDb = SQLiteDatabase.openDatabase(
-                    mContext.getDatabasePath(historyExtensionDbName).getPath(),
-                    null, SQLiteDatabase.OPEN_READONLY);
-            copyHistoryExtensionDataToVisitsTable(historyExtensionDb, db);
+            // If FxAccount exists (Sync is enabled) then port data over to the Visits table.
+            if (FirefoxAccounts.firefoxAccountsExist(mContext)) {
+                try {
+                    historyExtensionDb = SQLiteDatabase.openDatabase(historyExtensionsDatabase.getPath(), null,
+                            SQLiteDatabase.OPEN_READONLY);
 
-        // We might not have a history extensions db available - if Sync wasn't set up, for example.
-        } catch (SQLiteCantOpenDatabaseException e) {
-            Log.d(LOGTAG, "No history extensions DB present");
-            historyExtensionsDbPresent = false;
-        } catch (SQLiteException e) {
-            Log.e(LOGTAG, "Error while migrating history extensions visits", e);
+                // If we fail to open HistoryExtensionDatabase, then synthesize visits marking them as remote
+                } catch (SQLiteException e) {
+                    Log.w(LOGTAG, "Couldn't open history extension database; synthesizing visits instead", e);
+                    synthesizeAndInsertVisits(db, false);
+                }
+
+                if (historyExtensionDb != null) {
+                    copyHistoryExtensionDataToVisitsTable(historyExtensionDb, db);
+                }
+
+            // FxAccount doesn't exist, but there's evidence Sync was enabled at some point.
+            // Synthesize visits from History table marking them all as remote.
+            } else if (historyExtensionsDatabase.exists()) {
+                synthesizeAndInsertVisits(db, false);
+
+            // FxAccount doesn't exist and there's no evidence sync was ever enabled.
+            // Synthesize visits from History table marking them all as local.
+            } else {
+                synthesizeAndInsertVisits(db, true);
+            }
         } finally {
             if (historyExtensionDb != null) {
                 historyExtensionDb.close();
             }
         }
 
-        if (historyExtensionsDbPresent) {
-            // Delete history extensions db and friends.
+        // Delete history extensions database if it's present.
+        if (historyExtensionsDatabase.exists()) {
             if (!mContext.deleteDatabase(historyExtensionDbName)) {
                 Log.e(LOGTAG, "Couldn't remove history extension database");
             }
+        }
+    }
 
-            // We're done!
-            return;
-        }
-
-        // History extensions DB wasn't present - we need to synthesize locally recorded visits now.
-        final Cursor cursor = db.query(History.TABLE_NAME, new String[]{History.GUID, History.VISITS, History.DATE_LAST_VISITED}, null, null, null, null, null);
-
+    private void synthesizeAndInsertVisits(final SQLiteDatabase db, boolean markAsLocal) {
+        final Cursor cursor = db.query(
+                History.TABLE_NAME,
+                new String[] {History.GUID, History.VISITS, History.DATE_LAST_VISITED},
+                null, null, null, null, null);
         if (cursor == null) {
             Log.e(LOGTAG, "Null cursor while selecting all history records");
             return;
         }
 
         try {
             if (!cursor.moveToFirst()) {
                 Log.e(LOGTAG, "No history records to synthesize visits for.");
                 return;
             }
 
             int guidCol = cursor.getColumnIndexOrThrow(History.GUID);
             int visitsCol = cursor.getColumnIndexOrThrow(History.VISITS);
             int dateCol = cursor.getColumnIndexOrThrow(History.DATE_LAST_VISITED);
-            while (!cursor.isAfterLast()) {
-                insertSynthesizedVisits(db,
-                        generateSynthesizedVisits(
-                                cursor.getInt(visitsCol),
-                                cursor.getString(guidCol),
-                                cursor.getLong(dateCol)
-                        )
-                );
-                cursor.moveToNext();
-            }
+
+            // Re-use compiled SQL statements for faster inserts.
+            // Visit Type is going to be 1, which is the column's default value.
+            final String insertSqlStatement = "INSERT OR IGNORE INTO " + Visits.TABLE_NAME + "(" +
+                    Visits.DATE_VISITED + "," +
+                    Visits.HISTORY_GUID + "," +
+                    Visits.IS_LOCAL +
+                    ") VALUES (?, ?, ?)";
+            final SQLiteStatement compiledInsertStatement = db.compileStatement(insertSqlStatement);
+
+            // For each history record, insert as many visits as there are recorded in the VISITS column.
+            do {
+                final int numberOfVisits = cursor.getInt(visitsCol);
+                final String guid = cursor.getString(guidCol);
+                final long lastVisitedDate = cursor.getLong(dateCol);
+
+                // Sanity check.
+                if (guid == null) {
+                    continue;
+                }
+
+                // In a strange case that lastVisitedDate is a very low number, let's not introduce
+                // negative timestamps into our data.
+                if (lastVisitedDate - numberOfVisits < 0) {
+                    continue;
+                }
+
+                for (int i = 0; i < numberOfVisits; i++) {
+                    final long offsetVisitedDate = lastVisitedDate - i;
+                    compiledInsertStatement.clearBindings();
+                    compiledInsertStatement.bindLong(1, offsetVisitedDate);
+                    compiledInsertStatement.bindString(2, guid);
+                    // Very old school, 1 is true and 0 is false :)
+                    if (markAsLocal) {
+                        compiledInsertStatement.bindLong(3, Visits.VISIT_IS_LOCAL);
+                    } else {
+                        compiledInsertStatement.bindLong(3, Visits.VISIT_IS_REMOTE);
+                    }
+                    compiledInsertStatement.executeInsert();
+                }
+            } while (cursor.moveToNext());
         } catch (Exception e) {
             Log.e(LOGTAG, "Error while synthesizing visits for history record", e);
         } finally {
             cursor.close();
         }
     }
 
     private void createV19CombinedView(SQLiteDatabase db) {
--- a/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java
+++ b/mobile/android/services/src/main/java/org/mozilla/gecko/sync/repositories/android/VisitsHelper.java
@@ -101,17 +101,17 @@ public class VisitsHelper {
      */
     public static ContentValues getVisitContentValues(@NonNull String guid, @NonNull JSONObject visit, boolean isLocal) {
         if (!visit.containsKey(SYNC_DATE_KEY) || !visit.containsKey(SYNC_TYPE_KEY)) {
             throw new IllegalArgumentException("Visit missing required keys");
         }
 
         final ContentValues cv = new ContentValues();
         cv.put(Visits.HISTORY_GUID, guid);
-        cv.put(Visits.IS_LOCAL, isLocal ? 1 : 0);
+        cv.put(Visits.IS_LOCAL, isLocal ? Visits.VISIT_IS_LOCAL : Visits.VISIT_IS_REMOTE);
         cv.put(Visits.VISIT_TYPE, (Long) visit.get(SYNC_TYPE_KEY));
         cv.put(Visits.DATE_VISITED, (Long) visit.get(SYNC_DATE_KEY));
 
         return cv;
     }
 
     @SuppressWarnings("unchecked")
     private static void insertTupleIntoVisitsUnchecked(JSONArray visits, Integer type, Long date) {