Bug 1536170 - Replace Async.jankYielder r=tcsc,markh,eoger

`Async.jankYielder` is known to, unfortunately, cause jank by creating a lot of
immediately resolved promises that must be then GCed. For a collection of 50
items, it will create 50 promises and 49 of them will immediately resolve.

Instead of `Async.jankYielder`, we now have `Async.yieldState`, which simply
keeps track of whether or not the caller should yield to the event loop. Two
higher level looping constructs are built on top of it:

* `Async.yieldingIterator`, which has been rewritten to not create extraneous
  promises; and
* `Async.yieldingForEach`, which is a replacement for awaiting
  `Async.jankYielder` in a loop. Instead, it accepts the loop body as a
  function.

Each of these can share an instance of an `Async.yieldState`, which allows an
object with multiple loops to yield every N iterations overall, instead of
every N iterations of each loop, which keeps the behaviour of using one
`Async.jankYielders` in multiple places.

Differential Revision: https://phabricator.services.mozilla.com/D26229

--HG--
extra : moz-landing-system : lando

browser/components/translation/TranslationDocument.jsm

@@ -206,11 +206,7 @@ this.TranslationDocument.prototype = {
       // Let the event loop breath on every 100 nodes
       // that are replaced.
       const YIELD_INTERVAL = 100;
-      let maybeYield = Async.jankYielder(YIELD_INTERVAL);
-      for (let root of this.roots) {
-        root.swapText(target);
-        await maybeYield();
-      }
+      await Async.yieldingForEach(this.roots, root => root.swapText(target), YIELD_INTERVAL);
     })();
   },
 };

services/common/async.js

@@ -99,37 +99,89 @@ var Async = {
     });
   },
 
-  // Returns a method that yields every X calls.
-  // Common case is calling the returned method every iteration in a loop.
-  jankYielder(yieldEvery = 50) {
+  /**
+   * Shared state for yielding every N calls.
+   *
+   * Can be passed to multiple Async.yieldingForEach to have them overall yield
+   * every N iterations.
+   */
+  yieldState(yieldEvery = 50) {
     let iterations = 0;
-    return async () => {
-      Async.checkAppReady(); // Let it throw!
-      if (++iterations % yieldEvery === 0) {
-        await Async.promiseYield();
-      }
+
+    return {
+      shouldYield() {
+        ++iterations;
+        return iterations % yieldEvery === 0;
+      },
     };
   },
 
   /**
-   * Turn a synchronous iterator/iterable into an async iterator that calls a
-   * Async.jankYielder at each step.
+   * Apply the given function to each element of the iterable, yielding the
+   * event loop every yieldEvery iterations.
    *
    * @param iterable {Iterable}
-   *        Iterable or iterator that should be wrapped. (Anything usable in
-   *        for..of should work)
+   *        The iterable or iterator to iterate through.
    *
-   * @param [maybeYield = 50] {number|() => Promise<void>}
-   *        Either an existing jankYielder to use, or a number to provide as the
-   *        argument to Async.jankYielder.
+   * @param fn {(*) -> void|boolean}
+   *        The function to be called on each element of the iterable.
+   *
+   *        Returning true from the function will stop the iteration.
+   *
+   * @param [yieldEvery = 50] {number|object}
+   *        The number of iterations to complete before yielding back to the event
+   *        loop.
+   *
+   * @return {boolean}
+   *         Whether or not the function returned early.
    */
-  async* yieldingIterator(iterable, maybeYield = 50) {
-    if (typeof maybeYield == "number") {
-      maybeYield = Async.jankYielder(maybeYield);
+  async yieldingForEach(iterable, fn, yieldEvery = 50) {
+    const yieldState = typeof yieldEvery === "number" ? Async.yieldState(yieldEvery) : yieldEvery;
+    let iteration = 0;
+
+    for (const item of iterable) {
+      let result = fn(item, iteration++);
+      if (typeof result !== "undefined" && typeof result.then !== "undefined") {
+        // If we await result when it is not a Promise, we create an
+        // automatically resolved promise, which is exactly the case that we
+        // are trying to avoid.
+        result = await result;
+      }
+
+      if (result === true) {
+        return true;
+      }
+
+      if (yieldState.shouldYield()) {
+        await Async.promiseYield();
+        Async.checkAppReady();
+      }
     }
-    for (let item of iterable) {
-      await maybeYield();
+
+    return false;
+  },
+
+  /**
+   * Turn a synchronous iterator/iterable into an async iterator that yields in
+   * the same manner as Async.yieldingForEach.
+   *
+   * @param iterable {Iterable}
+   *        The iterable or iterator that should be wrapped.
+   *
+   * @param yieldEvery {number|object}
+   *        Either an existing Async.yieldState to use, or a number to provide as
+   *        the argument to async.yieldState.
+   */
+  async* yieldingIterator(iterable, yieldEvery = 50) {
+    const yieldState = typeof yieldEvery === "number" ? Async.yieldState(yieldEvery) : yieldEvery;
+
+    for (const item of iterable) {
       yield item;
+
+      if (yieldState.shouldYield()) {
+        await Async.promiseYield();
+        Async.checkAppReady();
+      }
     }
   },
 

services/sync/modules/bookmark_validator.js

@@ -240,9 +240,9 @@ async function detectCycles(records) {
   let currentPath = [];
   let cycles = [];
   let seenEver = new Set();
-  const maybeYield = Async.jankYielder();
-  const traverse = async node => {
-    await maybeYield();
+  const yieldState = Async.yieldState();
+
+  const traverse = async (node) => {
     if (pathLookup.has(node)) {
       let cycleStart = currentPath.lastIndexOf(node);
       let cyclePath = currentPath.slice(cycleStart).map(n => n.id);
@@ -261,18 +261,17 @@ async function detectCycles(records) {
     if (children.length) {
       pathLookup.add(node);
       currentPath.push(node);
-      for (let child of children) {
-        await traverse(child);
-      }
+      await Async.yieldingForEach(children, traverse, yieldState);
       currentPath.pop();
       pathLookup.delete(node);
     }
   };
-  for (let record of records) {
+
+  await Async.yieldingForEach(records, async (record) => {
     if (!seenEver.has(record)) {
       await traverse(record);
     }
-  }
+  }, yieldState);
 
   return cycles;
 }
@@ -297,7 +296,7 @@ class ServerRecordInspection {
     this._orphans = new Map();
     this._multipleParents = new Map();
 
-    this.maybeYield = Async.jankYielder();
+    this.yieldState = Async.yieldState();
   }
 
   static async create(records) {
@@ -351,11 +350,10 @@ class ServerRecordInspection {
     this.serverRecords = records;
     let rootChildren = [];
 
-    for (let record of this.serverRecords) {
-      await this.maybeYield();
+    await Async.yieldingForEach(this.serverRecords, async (record) => {
       if (!record.id) {
         ++this.problemData.missingIDs;
-        continue;
+        return;
       }
 
       if (record.deleted) {
@@ -363,7 +361,7 @@ class ServerRecordInspection {
       }
       if (this.idToRecord.has(record.id)) {
         this.problemData.duplicates.push(record.id);
-        continue;
+        return;
       }
 
       this.idToRecord.set(record.id, record);
@@ -377,7 +375,7 @@ class ServerRecordInspection {
       }
 
       if (!record.children) {
-        continue;
+        return;
       }
 
       if (record.type != "folder") {
@@ -385,7 +383,7 @@ class ServerRecordInspection {
         // subclassing BookmarkFolder) Livemarks will have a children array,
         // but it should still be empty.
         if (!record.children.length) {
-          continue;
+          return;
         }
         // Otherwise we mark it as an error and still try to resolve the children
         this.problemData.childrenOnNonFolder.push(record.id);
@@ -413,12 +411,13 @@ class ServerRecordInspection {
       //
       // The last two are left alone until later `this._linkChildren`, however.
       record.childGUIDs = record.children;
-      for (let id of record.childGUIDs) {
-        await this.maybeYield();
+
+      await Async.yieldingForEach(record.childGUIDs, id => {
         this.noteParent(id, record.id);
-      }
+      }, this.yieldState);
+
       record.children = [];
-    }
+    }, this.yieldState);
 
     // Finish up some parts we can easily do now that we have idToRecord.
     this.deletedRecords = Array.from(this.deletedIds,
@@ -455,10 +454,9 @@ class ServerRecordInspection {
 
   // Adds `parent` to all records it can that have `parentid`
   async _linkParentIDs() {
-    for (let [id, record] of this.idToRecord) {
-      await this.maybeYield();
+    await Async.yieldingForEach(this.idToRecord, ([id, record]) => {
       if (record == this.root || record.deleted) {
-        continue;
+        return false;
       }
 
       // Check and update our orphan map.
@@ -466,17 +464,17 @@ class ServerRecordInspection {
       let parent = this.idToRecord.get(parentID);
       if (!parentID || !parent) {
         this._noteOrphan(id, parentID);
-        continue;
+        return false;
       }
 
       record.parent = parent;
 
       if (parent.deleted) {
         this.problemData.deletedParents.push(id);
-        return;
+        return true;
       } else if (parent.type != "folder") {
         this.problemData.parentNotFolder.push(record.id);
-        return;
+        return true;
       }
 
       if (parent.id !== "place" || this.problemData.rootOnServer) {
@@ -493,49 +491,45 @@ class ServerRecordInspection {
       // actual local parent name, but given this is used only for de-duping a
       // record the first time it is seen and expensive to keep up-to-date, we
       // decided to just stop recording it. See bug 1276969 for more.
-    }
+      return false;
+    }, this.yieldState);
   }
 
   // Build the children and unfilteredChildren arrays, (which are of record
   // objects, not ids)
   async _linkChildren() {
     // Check that we aren't missing any children.
-    for (let folder of this.folders) {
-      await this.maybeYield();
-
+    await Async.yieldingForEach(this.folders, async (folder) => {
       folder.children = [];
       folder.unfilteredChildren = [];
 
       let idsThisFolder = new Set();
 
-      for (let i = 0; i < folder.childGUIDs.length; ++i) {
-        await this.maybeYield();
-        let childID = folder.childGUIDs[i];
-
+      await Async.yieldingForEach(folder.childGUIDs, childID => {
         let child = this.idToRecord.get(childID);
 
         if (!child) {
           this.problemData.missingChildren.push({ parent: folder.id, child: childID });
-          continue;
+          return;
         }
 
         if (child.deleted) {
           this.problemData.deletedChildren.push({ parent: folder.id, child: childID });
-          continue;
+          return;
         }
 
         if (child.parentid != folder.id) {
           this.noteMismatch(childID, folder.id);
-          continue;
+          return;
         }
 
         if (idsThisFolder.has(childID)) {
           // Already recorded earlier, we just don't want to mess up `children`
-          continue;
+          return;
         }
         folder.children.push(child);
-      }
-    }
+      }, this.yieldState);
+    }, this.yieldState);
   }
 
   // Finds the orphans in the tree using something similar to a `mark and sweep`
@@ -544,31 +538,34 @@ class ServerRecordInspection {
   // haven't seen are orphans.
   async _findOrphans() {
     let seen = new Set([this.root.id]);
-    for (let [node] of Utils.walkTree(this.root)) {
-      await this.maybeYield();
+
+    const inCycle = await Async.yieldingForEach(Utils.walkTree(this.root), ([node]) => {
       if (seen.has(node.id)) {
         // We're in an infloop due to a cycle.
         // Return early to avoid reporting false positives for orphans.
-        return;
+        return true;
       }
       seen.add(node.id);
+
+      return false;
+    }, this.yieldState);
+
+    if (inCycle) {
+      return;
     }
 
-    for (let i = 0; i < this.liveRecords.length; ++i) {
-      await this.maybeYield();
-      let record = this.liveRecords[i];
+    await Async.yieldingForEach(this.liveRecords, (record, i) => {
       if (!seen.has(record.id)) {
         // We intentionally don't record the parentid here, since we only record
         // that if the record refers to a parent that doesn't exist, which we
         // have already handled (when linking parentid's).
         this._noteOrphan(record.id);
       }
-    }
+    }, this.yieldState);
 
-    for (const [id, parent] of this._orphans) {
-      await this.maybeYield();
+    await Async.yieldingForEach(this._orphans, ([id, parent]) => {
       this.problemData.orphans.push({id, parent});
-    }
+    }, this.yieldState);
   }
 
   async _finish() {
@@ -597,7 +594,7 @@ class ServerRecordInspection {
 
 class BookmarkValidator {
   constructor() {
-    this.maybeYield = Async.jankYielder();
+    this.yieldState = Async.yieldState();
   }
 
   async canValidate() {
@@ -605,10 +602,9 @@ class BookmarkValidator {
   }
 
   async _followQueries(recordsByQueryId) {
-    for (let entry of recordsByQueryId.values()) {
-      await this.maybeYield();
+    await Async.yieldingForEach(recordsByQueryId.values(), entry => {
       if (entry.type !== "query" && (!entry.bmkUri || !entry.bmkUri.startsWith(QUERY_PROTOCOL))) {
-        continue;
+        return;
       }
       let params = new URLSearchParams(entry.bmkUri.slice(QUERY_PROTOCOL.length));
       // Queries with `excludeQueries` won't form cycles because they'll
@@ -616,7 +612,7 @@ class BookmarkValidator {
       let excludeQueries = params.get("excludeQueries");
       if (excludeQueries === "1" || excludeQueries === "true") {
         // `nsNavHistoryQuery::ParseQueryBooleanString` allows `1` and `true`.
-        continue;
+        return;
       }
       entry.concreteItems = [];
       let queryIds = params.getAll("folder");
@@ -626,7 +622,7 @@ class BookmarkValidator {
           entry.concreteItems.push(concreteItem);
         }
       }
-    }
+    }, this.yieldState);
   }
 
   async createClientRecordsFromTree(clientTree) {
@@ -640,8 +636,8 @@ class BookmarkValidator {
     // refer to folders via their local IDs.
     let recordsByQueryId = new Map();
     let syncedRoots = SYNCED_ROOTS;
+
     const traverse = async (treeNode, synced) => {
-      await this.maybeYield();
       if (!synced) {
         synced = syncedRoots.includes(treeNode.guid);
       }
@@ -703,15 +699,18 @@ class BookmarkValidator {
         if (!treeNode.children) {
           treeNode.children = [];
         }
-        for (let child of treeNode.children) {
+
+        await Async.yieldingForEach(treeNode.children, async (child) => {
           await traverse(child, synced);
           child.parent = treeNode;
           child.parentid = guid;
           treeNode.childGUIDs.push(child.guid);
-        }
+        }, this.yieldState);
       }
     };
+
     await traverse(clientTree, false);
+
     clientTree.id = "places";
     await this._followQueries(recordsByQueryId);
     return records;
@@ -766,23 +765,22 @@ class BookmarkValidator {
 
   async _computeUnifiedRecordMap(serverRecords, clientRecords) {
     let allRecords = new Map();
-    for (let sr of serverRecords) {
-      await this.maybeYield();
+    await Async.yieldingForEach(serverRecords, sr => {
       if (sr.fake) {
-        continue;
+        return;
       }
       allRecords.set(sr.id, {client: null, server: sr});
-    }
+    }, this.yieldState);
 
-    for (let cr of clientRecords) {
-      await this.maybeYield();
+    await Async.yieldingForEach(clientRecords, cr => {
       let unified = allRecords.get(cr.id);
       if (!unified) {
         allRecords.set(cr.id, {client: cr, server: null});
       } else {
         unified.client = cr;
       }
-    }
+    }, this.yieldState);
+
     return allRecords;
   }
 
@@ -902,11 +900,11 @@ class BookmarkValidator {
     let allRecords = await this._computeUnifiedRecordMap(serverRecords, clientRecords);
 
     let serverDeleted = new Set(inspectionInfo.deletedRecords.map(r => r.id));
-    for (let [id, {client, server}] of allRecords) {
-      await this.maybeYield();
+
+    await Async.yieldingForEach(allRecords, ([id, {client, server}]) => {
       if (!client || !server) {
         this._recordMissing(problemData, id, client, server, serverDeleted);
-        continue;
+        return;
       }
       if (server && client && client.ignored) {
         problemData.serverUnexpected.push(id);
@@ -919,7 +917,8 @@ class BookmarkValidator {
       if (structuralDifferences.length) {
         problemData.structuralDifferences.push({ id, differences: structuralDifferences });
       }
-    }
+    }, this.yieldState);
+
     return inspectionInfo;
   }
 
@@ -934,11 +933,10 @@ class BookmarkValidator {
       throw result.response;
     }
     let cleartexts = [];
-    for (let record of result.records) {
-      await this.maybeYield();
+    await Async.yieldingForEach(result.records, async (record) => {
       await record.decrypt(collectionKey);
       cleartexts.push(record.cleartext);
-    }
+    }, this.yieldState);
     return cleartexts;
   }
 

services/sync/modules/collection_validator.js

@@ -76,13 +76,13 @@ class CollectionValidator {
     if (!result.response.success) {
       throw result.response;
     }
-    let maybeYield = Async.jankYielder();
     let cleartexts = [];
-    for (let record of result.records) {
-      await maybeYield();
+
+    await Async.yieldingForEach(result.records, async (record) => {
       await record.decrypt(collectionKey);
       cleartexts.push(record.cleartext);
-    }
+    });
+
     return cleartexts;
   }
 
@@ -144,17 +144,19 @@ class CollectionValidator {
   //   records: Normalized server records,
   //   deletedRecords: Array of ids that were marked as deleted by the server.
   async compareClientWithServer(clientItems, serverItems) {
-    let maybeYield = Async.jankYielder();
+    const yieldState = Async.yieldState();
+
     const clientRecords = [];
-    for (let item of clientItems) {
-      await maybeYield();
+
+    await Async.yieldingForEach(clientItems, item => {
       clientRecords.push(this.normalizeClientItem(item));
-    }
+    }, yieldState);
+
     const serverRecords = [];
-    for (let item of serverItems) {
-      await maybeYield();
+    await Async.yieldingForEach(serverItems, async (item) => {
       serverRecords.push((await this.normalizeServerItem(item)));
-    }
+    }, yieldState);
+
     let problems = this.emptyProblemData();
     let seenServer = new Map();
     let serverDeleted = new Set();

services/sync/modules/engines.js

@@ -308,9 +308,8 @@ Store.prototype = {
    */
   async applyIncomingBatch(records) {
     let failed = [];
-    let maybeYield = Async.jankYielder();
-    for (let record of records) {
-      await maybeYield();
+
+    await Async.yieldingForEach(records, async (record) => {
       try {
         await this.applyIncoming(record);
       } catch (ex) {
@@ -326,7 +325,8 @@ Store.prototype = {
         this._log.warn("Failed to apply incoming record " + record.id, ex);
         failed.push(record.id);
       }
-    }
+    });
+
     return failed;
   },
 
@@ -1184,9 +1184,7 @@ SyncEngine.prototype = {
         throw response;
       }
 
-      let maybeYield = Async.jankYielder();
-      for (let record of records) {
-        await maybeYield();
+      await Async.yieldingForEach(records, async (record) => {
         downloadedIDs.add(record.id);
 
         if (record.modified < oldestModified) {
@@ -1197,14 +1195,14 @@ SyncEngine.prototype = {
         if (error) {
           failedInCurrentSync.add(record.id);
           count.failed++;
-          continue;
+          return;
         }
         if (!shouldApply) {
           count.reconciled++;
-          continue;
+          return;
         }
         recordsToApply.push(record);
-      }
+      });
 
       let failedToApply = await this._applyRecords(recordsToApply);
       Utils.setAddAll(failedInCurrentSync, failedToApply);
@@ -1279,25 +1277,23 @@ SyncEngine.prototype = {
         throw response;
       }
 
-      let maybeYield = Async.jankYielder();
       let backfilledRecordsToApply = [];
       let failedInBackfill = [];
 
-      for (let record of records) {
-        await maybeYield();
-
+      await Async.yieldingForEach(records, async (record) => {
         let { shouldApply, error } = await this._maybeReconcile(record);
         if (error) {
           failedInBackfill.push(record.id);
           count.failed++;
-          continue;
+          return;
         }
         if (!shouldApply) {
           count.reconciled++;
-          continue;
+          return;
         }
         backfilledRecordsToApply.push(record);
-      }
+      });
+
       let failedToApply = await this._applyRecords(backfilledRecordsToApply);
       failedInBackfill.push(...failedToApply);
 

services/sync/modules/engines/bookmarks.js

@@ -499,9 +499,7 @@ BookmarksEngine.prototype = {
       }
     }
 
-    let maybeYield = Async.jankYielder();
-    for (let [node, parent] of walkBookmarksRoots(tree)) {
-      await maybeYield();
+    await Async.yieldingForEach(walkBookmarksRoots(tree), ([node, parent]) => {
       let {guid, type: placeType} = node;
       guid = PlacesSyncUtils.bookmarks.guidToRecordId(guid);
       let key;
@@ -520,7 +518,7 @@ BookmarksEngine.prototype = {
           break;
         default:
           this._log.error("Unknown place type: '" + placeType + "'");
-          continue;
+          return;
       }
 
       let parentName = parent.title || "";
@@ -537,7 +535,7 @@ BookmarksEngine.prototype = {
       // Remember this item's GUID for its parent-name/key pair.
       guidMap[parentName][key] = entry;
       this._log.trace("Mapped: " + [parentName, key, entry, entry.hasDupe]);
-    }
+    });
 
     return guidMap;
   },

toolkit/components/places/SyncedBookmarksMirror.jsm

@@ -127,9 +127,9 @@ const MIRROR_SCHEMA_VERSION = 4;
 const DEFAULT_MAX_FRECENCIES_TO_RECALCULATE = 400;
 
 // Use a shared jankYielder in these functions
-XPCOMUtils.defineLazyGetter(this, "maybeYield", () => Async.jankYielder());
+XPCOMUtils.defineLazyGetter(this, "yieldState", () => Async.yieldState());
 function yieldingIterator(collection) {
-  return Async.yieldingIterator(collection, maybeYield);
+  return Async.yieldingIterator(collection, yieldState);
 }
 
 /** Adapts a `Log.jsm` logger to a `mozISyncedBookmarksMirrorLogger`. */