Bug 755576 - Redundant cyclic reuploading & repositioning of bookmark folders. r=nalexander
authorRichard Newman <rnewman@mozilla.com>
Fri, 18 May 2012 11:55:52 -0700
changeset 98462 87e6d47a1657f53b0386a8bbbd9542b3bf8e00a6
parent 98461 7c0275cfcb1e8c62848227fa5b2021aa77380640
child 98463 ae944ea53b59d417e7f2b11a7514c2b1d7eaa46b
push id1116
push userlsblakk@mozilla.com
push dateMon, 16 Jul 2012 19:38:18 +0000
treeherdermozilla-beta@95f959a8b4dc [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnalexander
bugs755576
milestone15.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 755576 - Redundant cyclic reuploading & repositioning of bookmark folders. r=nalexander
mobile/android/base/sync/repositories/RepositorySession.java
mobile/android/base/sync/repositories/Server11RepositorySession.java
mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java
--- a/mobile/android/base/sync/repositories/RepositorySession.java
+++ b/mobile/android/base/sync/repositories/RepositorySession.java
@@ -118,17 +118,17 @@ public abstract class RepositorySession 
   public void storeDone() {
     // Our default behavior will be to assume that the Runnable is
     // executed as soon as all the stores synchronously finish, so
     // our end timestamp can just be… now.
     storeDone(now());
   }
 
   public void storeDone(final long end) {
-    Logger.debug(LOG_TAG, "Scheduling onStoreCompleted for after storing is done.");
+    Logger.debug(LOG_TAG, "Scheduling onStoreCompleted for after storing is done: " + end);
     Runnable command = new Runnable() {
       @Override
       public void run() {
         delegate.onStoreCompleted(end);
       }
     };
     storeWorkQueue.execute(command);
   }
--- a/mobile/android/base/sync/repositories/Server11RepositorySession.java
+++ b/mobile/android/base/sync/repositories/Server11RepositorySession.java
@@ -354,19 +354,31 @@ public class Server11RepositorySession e
 
       recordsBuffer = new ArrayList<byte[]>();
       byteCount = PER_BATCH_OVERHEAD;
     }
   }
 
   @Override
   public void storeDone() {
+    Logger.debug(LOG_TAG, "storeDone().");
     synchronized (recordsBufferMonitor) {
       flush();
-      storeDone(uploadTimestamp.get());
+      // Do this in a Runnable so that the timestamp is grabbed after any upload.
+      final Runnable r = new Runnable() {
+        @Override
+        public void run() {
+          synchronized (recordsBufferMonitor) {
+            final long end = uploadTimestamp.get();
+            Logger.debug(LOG_TAG, "Calling storeDone with " + end);
+            storeDone(end);
+          }
+        }
+      };
+      storeWorkQueue.execute(r);
     }
   }
 
   /**
    * Make an HTTP request, and convert HTTP request delegate callbacks into
    * store callbacks within the context of this RepositorySession.
    *
    * @author rnewman
@@ -376,36 +388,36 @@ public class Server11RepositorySession e
 
     public final String LOG_TAG = "RecordUploadRunnable";
     private ArrayList<byte[]> outgoing;
     private long byteCount;
 
     public RecordUploadRunnable(RepositorySessionStoreDelegate storeDelegate,
                                 ArrayList<byte[]> outgoing,
                                 long byteCount) {
-      Logger.debug(LOG_TAG, "Preparing RecordUploadRunnable for " +
-                     outgoing.size() + " records (" +
-                     byteCount + " bytes).");
+      Logger.info(LOG_TAG, "Preparing record upload for " +
+                  outgoing.size() + " records (" +
+                  byteCount + " bytes).");
       this.outgoing  = outgoing;
       this.byteCount = byteCount;
     }
 
     @Override
     public String credentials() {
       return serverRepository.credentialsSource.credentials();
     }
 
     @Override
     public String ifUnmodifiedSince() {
       return null;
     }
 
     @Override
     public void handleRequestSuccess(SyncStorageResponse response) {
-      Logger.debug(LOG_TAG, "POST of " + outgoing.size() + " records done.");
+      Logger.info(LOG_TAG, "POST of " + outgoing.size() + " records done.");
 
       ExtendedJSONObject body;
       try {
         body = response.jsonObjectBody(); // jsonObjectBody() throws or returns non-null.
       } catch (Exception e) {
         Logger.error(LOG_TAG, "Got exception parsing POST success body.", e);
         // TODO
         return;
@@ -440,16 +452,17 @@ public class Server11RepositorySession e
           Logger.debug(LOG_TAG, "Failed records: " + failed.object.toString());
           // TODO: notify.
         }
       } catch (UnexpectedJSONException e) {
         Logger.error(LOG_TAG, "Got exception processing success/failed in POST success body.", e);
         // TODO
         return;
       }
+      Logger.info(LOG_TAG, "POST of " + outgoing.size() + " records handled.");
     }
 
     @Override
     public void handleRequestFailure(SyncStorageResponse response) {
       // TODO: ensure that delegate methods don't get called more than once.
       // TODO: call session.interpretHTTPFailure.
       this.handleRequestError(new HTTPFailureException(response));
     }
--- a/mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java
+++ b/mobile/android/base/sync/repositories/android/AndroidBrowserBookmarksRepositorySession.java
@@ -293,29 +293,30 @@ public class AndroidBrowserBookmarksRepo
   /**
    * Retrieve the child array for a record, repositioning and updating the database as necessary.
    *
    * @param folderID
    *        The database ID of the folder.
    * @param persist
    *        True if generated positions should be written to the database. The modified
    *        time of the parent folder is only bumped if this is true.
+   * @param childArray
+   *        A new, empty JSONArray which will be populated with an array of GUIDs.
    * @return
-   *        An array of GUIDs.
+   *        True if the resulting array is "clean" (i.e., reflects the content of the database).
    * @throws NullCursorException
    */
   @SuppressWarnings("unchecked")
-  private JSONArray getChildrenArray(long folderID, boolean persist) throws NullCursorException {
+  private boolean getChildrenArray(long folderID, boolean persist, JSONArray childArray) throws NullCursorException {
     trace("Calling getChildren for androidID " + folderID);
-    JSONArray childArray = new JSONArray();
     Cursor children = dataAccessor.getChildren(folderID);
     try {
       if (!children.moveToFirst()) {
         trace("No children: empty cursor.");
-        return childArray;
+        return true;
       }
       final int positionIndex = children.getColumnIndex(BrowserContract.Bookmarks.POSITION);
       final int count = children.getCount();
       Logger.debug(LOG_TAG, "Expecting " + count + " children.");
 
       // Sorted by requested position.
       TreeMap<Long, ArrayList<String>> guids = new TreeMap<Long, ArrayList<String>>();
 
@@ -339,48 +340,51 @@ public class AndroidBrowserBookmarksRepo
         long pos = entry.getKey().longValue();
         int atPos = entry.getValue().size();
 
         // If every element has a different index, and the indices are
         // in strict natural order, then changed will be false.
         if (atPos > 1 || pos != i) {
           changed = true;
         }
+
+        ++i;
+
         for (String guid : entry.getValue()) {
           if (!forbiddenGUID(guid)) {
             childArray.add(guid);
           }
         }
       }
 
       if (Logger.logVerbose(LOG_TAG)) {
         // Don't JSON-encode unless we're logging.
         Logger.trace(LOG_TAG, "Output child array: " + childArray.toJSONString());
       }
 
       if (!changed) {
         Logger.debug(LOG_TAG, "Nothing moved! Database reflects child array.");
-        return childArray;
+        return true;
       }
 
       if (!persist) {
-        return childArray;
+        Logger.debug(LOG_TAG, "Returned array does not match database, and not persisting.");
+        return false;
       }
 
       Logger.debug(LOG_TAG, "Generating child array required moving records. Updating DB.");
       final long time = now();
       if (0 < dataAccessor.updatePositions(childArray)) {
         Logger.debug(LOG_TAG, "Bumping parent time to " + time + ".");
         dataAccessor.bumpModified(folderID, time);
       }
+      return true;
     } finally {
       children.close();
     }
-
-    return childArray;
   }
 
   protected static boolean isDeleted(Cursor cur) {
     return RepoUtils.getLongFromCursor(cur, BrowserContract.SyncColumns.IS_DELETED) != 0;
   }
 
   @Override
   protected Record retrieveDuringStore(Cursor cur) throws NoGuidForIdException, NullCursorException, ParentNotFoundException {
@@ -483,20 +487,18 @@ public class AndroidBrowserBookmarksRepo
 
   protected JSONArray getChildrenArrayForRecordCursor(Cursor cur, String recordGUID, boolean persist) throws NullCursorException {
     boolean isFolder = rowIsFolder(cur);
     if (!isFolder) {
       return null;
     }
 
     long androidID = parentGuidToIDMap.get(recordGUID);
-    JSONArray childArray = getChildrenArray(androidID, persist);
-    if (childArray == null) {
-      return null;
-    }
+    JSONArray childArray = new JSONArray();
+    getChildrenArray(androidID, persist, childArray);
 
     Logger.debug(LOG_TAG, "Fetched " + childArray.size() + " children for " + recordGUID);
     return childArray;
   }
 
   @Override
   public boolean shouldIgnore(Record record) {
     if (!(record instanceof BookmarkRecord)) {
@@ -676,16 +678,22 @@ public class AndroidBrowserBookmarksRepo
 
     BookmarkRecord reconciled = (BookmarkRecord) super.reconcileRecords(remoteRecord, localRecord,
                                                                         lastRemoteRetrieval,
                                                                         lastLocalRetrieval);
 
     // For now we *always* use the remote record's children array as a starting point.
     // We won't write it into the database yet; we'll record it and process as we go.
     reconciled.children = ((BookmarkRecord) remoteRecord).children;
+
+    // *Always* track folders, though: if we decide we need to reposition items, we'll
+    // untrack later.
+    if (reconciled.isFolder()) {
+      trackRecord(reconciled);
+    }
     return reconciled;
   }
 
   /**
    * Rename mobile folders to "mobile", both in and out. The other half of
    * this logic lives in {@link #computeParentFields(BookmarkRecord, String, String)}, where
    * the parent name of a record is set from {@link #SPECIAL_GUIDS_MAP} rather than
    * from source data.
@@ -869,42 +877,42 @@ public class AndroidBrowserBookmarksRepo
     try {
       flushQueues();
       Logger.debug(LOG_TAG, "Have " + parentToChildArray.size() + " folders whose children might need repositioning.");
       for (Entry<String, JSONArray> entry : parentToChildArray.entrySet()) {
         String guid = entry.getKey();
         JSONArray onServer = entry.getValue();
         try {
           final long folderID = getIDForGUID(guid);
-          JSONArray inDB = getChildrenArray(folderID, false);
+          final JSONArray inDB = new JSONArray();
+          final boolean clean = getChildrenArray(folderID, false, inDB);
+          final boolean sameArrays = Utils.sameArrays(onServer, inDB);
 
           // If the local children and the remote children are already
           // the same, then we don't need to bump the modified time of the
           // parent: we wouldn't upload a different record, so avoid the cycle.
-          if (!Utils.sameArrays(onServer, inDB)) {
+          if (!sameArrays) {
             int added = 0;
             for (Object o : inDB) {
               if (!onServer.contains(o)) {
                 onServer.add(o);
                 added++;
               }
             }
             Logger.debug(LOG_TAG, "Added " + added + " items locally.");
+            Logger.debug(LOG_TAG, "Untracking and bumping " + guid + "(" + folderID + ")");
             dataAccessor.bumpModified(folderID, now());
-            // Wow, this is spectacularly wasteful.
-            Logger.debug(LOG_TAG, "Untracking " + guid);
-            final Record record = retrieveByGUIDDuringStore(guid);
-            if (record == null) {
-              return;
-            }
-            untrackRecord(record);
+            untrackGUID(guid);
           }
-          // Until getChildrenArray can tell us if it needed to make
-          // any changes at all, always update positions.
-          dataAccessor.updatePositions(new ArrayList<String>(onServer));
+
+          // If the arrays are different, or they're the same but not flushed to disk,
+          // write them out now.
+          if (!sameArrays || !clean) {
+            dataAccessor.updatePositions(new ArrayList<String>(onServer));
+          }
         } catch (Exception e) {
           Logger.warn(LOG_TAG, "Error repositioning children for " + guid, e);
         }
       }
     } finally {
       super.storeDone();
     }
   }