Bug 1416788 - limit number of visits we pass to PlacesUtils.history.insertMany to prevent shutdown hangs. r?kitcambridge draft
authorMark Hammond <mhammond@skippinet.com.au>
Thu, 23 Nov 2017 16:18:55 +1100
changeset 704738 2a4f5bb668b3df9776e7f932eb1254704c4b00ae
parent 703941 f5f03ee9e6abf77964f8dc1b9d69c6ccd3f655fd
child 742150 bc5e4c5070f9979bf280dbb589f29723dfb4b487
push id91234
push userbmo:markh@mozilla.com
push dateTue, 28 Nov 2017 23:09:29 +0000
reviewerskitcambridge
bugs1416788
milestone59.0a1
Bug 1416788 - limit number of visits we pass to PlacesUtils.history.insertMany to prevent shutdown hangs. r?kitcambridge MozReview-Commit-ID: AU9AMUAD1Rw
services/sync/modules/engines/history.js
services/sync/tests/unit/test_history_store.js
--- a/services/sync/modules/engines/history.js
+++ b/services/sync/modules/engines/history.js
@@ -92,16 +92,20 @@ function HistoryStore(name, engine) {
     }
     this._stmts = {};
   }, this);
 }
 HistoryStore.prototype = {
   __proto__: Store.prototype,
 
   __asyncHistory: null,
+
+  // We try and only update this many visits at one time.
+  MAX_VISITS_PER_INSERT: 500,
+
   get _asyncHistory() {
     if (!this.__asyncHistory) {
       this.__asyncHistory = Cc["@mozilla.org/browser/history;1"]
                               .getService(Ci.mozIAsyncHistory);
     }
     return this.__asyncHistory;
   },
 
@@ -207,23 +211,58 @@ HistoryStore.prototype = {
 
       if (shouldApply) {
         k += 1;
       }
     }
     records.length = k; // truncate array
 
     if (records.length) {
-      await PlacesUtils.history.insertMany(records);
+      for (let chunk of this._generateChunks(records)) {
+        await PlacesUtils.history.insertMany(chunk);
+      }
     }
 
     return failed;
   },
 
   /**
+   * Returns a generator that splits records into sanely sized chunks suitable
+   * for passing to places to prevent places doing bad things at shutdown.
+   */
+  * _generateChunks(records) {
+    // We chunk based on the number of *visits* inside each record. However,
+    // we do not split a single record into multiple records, because at some
+    // time in the future, we intend to ensure these records are ordered by
+    // lastModified, and advance the engine's timestamp as we process them,
+    // meaning we can resume exactly where we left off next sync - although
+    // currently that's not done, so we will retry the entire batch next sync
+    // if interrupted.
+    // ie, this means that if a single record has more than MAX_VISITS_PER_INSERT
+    // visits, we will call insertMany() with exactly 1 record, but with
+    // more than MAX_VISITS_PER_INSERT visits.
+    let curIndex = 0;
+    this._log.debug(`adding ${records.length} records to history`);
+    while (curIndex < records.length) {
+      Async.checkAppReady(); // may throw if we are shutting down.
+      let toAdd = []; // what we are going to insert.
+      let count = 0; // a counter which tells us when toAdd is full.
+      do {
+        let record = records[curIndex];
+        curIndex += 1;
+        toAdd.push(record);
+        count += record.visits.length;
+      } while (curIndex < records.length &&
+               count + records[curIndex].visits.length <= this.MAX_VISITS_PER_INSERT);
+      this._log.trace(`adding ${toAdd.length} items in this chunk`);
+      yield toAdd;
+    }
+  },
+
+  /**
    * Converts a Sync history record to a mozIPlaceInfo.
    *
    * Throws if an invalid record is encountered (invalid URI, etc.),
    * returns true if the record is to be applied, false otherwise
    * (no visits to add, etc.),
    */
   async _recordToPlaceInfo(record) {
     // Sort out invalid URIs and ones Places just simply doesn't want.
--- a/services/sync/tests/unit/test_history_store.js
+++ b/services/sync/tests/unit/test_history_store.js
@@ -338,12 +338,76 @@ add_task(async function test_remove() {
   });
   do_check_null(queryres);
   queryres = await PlacesUtils.history.fetch(tburi.spec, {
     includeVisits: true,
   });
   do_check_null(queryres);
 });
 
+add_task(async function test_chunking() {
+  let mvpi = store.MAX_VISITS_PER_INSERT;
+  store.MAX_VISITS_PER_INSERT = 3;
+  let checkChunks = function(input, expected) {
+    let chunks = Array.from(store._generateChunks(input));
+    deepEqual(chunks, expected);
+  };
+  try {
+    checkChunks([{visits: ["x"]}],
+                [[{visits: ["x"]}]]);
+
+    // 3 should still be one chunk.
+    checkChunks([{visits: ["x", "x", "x"]}],
+                [[{visits: ["x", "x", "x"]}]]);
+
+    // 4 should still be one chunk as we don't split individual records.
+    checkChunks([{visits: ["x", "x", "x", "x"]}],
+                [[{visits: ["x", "x", "x", "x"]}]]
+               );
+
+    // 4 in the first and 1 in the second should be 2 chunks.
+    checkChunks([
+                  {visits: ["x", "x", "x", "x"]},
+                  {visits: ["x"]}
+                ],
+                // expected
+                [
+                  [
+                    {visits: ["x", "x", "x", "x"]}
+                  ],
+                  [
+                    {visits: ["x"]}
+                  ],
+                ]
+               );
+
+    // we put multiple records into chunks
+    checkChunks([
+                  {visits: ["x", "x"]},
+                  {visits: ["x"]},
+                  {visits: ["x"]},
+                  {visits: ["x", "x"]},
+                  {visits: ["x", "x", "x", "x"]},
+                ],
+                // expected
+                [
+                  [
+                    {visits: ["x", "x"]},
+                    {visits: ["x"]},
+                  ],
+                  [
+                    {visits: ["x"]},
+                    {visits: ["x", "x"]},
+                  ],
+                  [
+                    {visits: ["x", "x", "x", "x"]},
+                  ],
+                ]
+               );
+  } finally {
+    store.MAX_VISITS_PER_INSERT = mvpi;
+  }
+});
+
 add_task(async function cleanup() {
   _("Clean up.");
   await PlacesTestUtils.clearHistory();
 });