misc: drive-by code cleanup in BaseNode (#7723)

GoogleChrome · Mar 28, 2019 · 21dda6a · 21dda6a
1 parent d02a824
commit 21dda6a
Show file tree

Hide file tree

Showing 2 changed files with 86 additions and 65 deletions.
diff --git a/lighthouse-core/lib/dependency-graph/base-node.js b/lighthouse-core/lib/dependency-graph/base-node.js
@@ -168,96 +168,88 @@ class BaseNode {
 
   /**
    * Clones the entire graph connected to this node filtered by the optional predicate. If a node is
-   * included by the predicate, all nodes along the paths between the two will be included. If the
-   * node that was called clone is not included in the resulting filtered graph, the method will throw.
+   * included by the predicate, all nodes along the paths between the node and the root will be included. If the
+   * node this was called on is not included in the resulting filtered graph, the method will throw.
    * @param {function(Node):boolean} [predicate]
    * @return {Node}
    */
   cloneWithRelationships(predicate) {
     const rootNode = this.getRootNode();
 
-    /** @type {function(Node): boolean} */
-    let shouldIncludeNode = () => true;
-    if (predicate) {
-      const idsToInclude = new Set();
-      rootNode.traverse(node => {
-        if (predicate(node)) {
-          node.traverse(
-            node => idsToInclude.add(node.id),
-            node => node._dependencies.filter(parent => !idsToInclude.has(parent))
-          );
-        }
-      });
-
-      shouldIncludeNode = node => idsToInclude.has(node.id);
-    }
+    /** @type {Map<string, Node>} */
+    const idsToIncludedClones = new Map();
 
-    const idToNodeMap = new Map();
-    rootNode.traverse(originalNode => {
-      if (!shouldIncludeNode(originalNode)) return;
-      const clonedNode = originalNode.cloneWithoutRelationships();
-      idToNodeMap.set(clonedNode.id, clonedNode);
+    // Walk down dependents.
+    rootNode.traverse(node => {
+      if (idsToIncludedClones.has(node.id)) return;
+
+      if (predicate === undefined) {
+        // No condition for entry, so clone every node.
+        idsToIncludedClones.set(node.id, node.cloneWithoutRelationships());
+        return;
+      }
+
+      if (predicate(node)) {
+        // Node included, so walk back up dependencies, cloning nodes from here back to the root.
+        node.traverse(
+          node => idsToIncludedClones.set(node.id, node.cloneWithoutRelationships()),
+          // Dependencies already cloned have already cloned ancestors, so no need to visit again.
+          node => node._dependencies.filter(parent => !idsToIncludedClones.has(parent.id))
+        );
+      }
     });
 
+    // Copy dependencies between nodes.
     rootNode.traverse(originalNode => {
-      if (!shouldIncludeNode(originalNode)) return;
-      const clonedNode = idToNodeMap.get(originalNode.id);
+      const clonedNode = idsToIncludedClones.get(originalNode.id);
+      if (!clonedNode) return;
 
       for (const dependency of originalNode._dependencies) {
-        const clonedDependency = idToNodeMap.get(dependency.id);
+        const clonedDependency = idsToIncludedClones.get(dependency.id);
+        if (!clonedDependency) throw new Error('Dependency somehow not cloned');
         clonedNode.addDependency(clonedDependency);
       }
     });
 
-    if (!idToNodeMap.has(this.id)) throw new Error('Cloned graph missing node');
-    return idToNodeMap.get(this.id);
+    const clonedThisNode = idsToIncludedClones.get(this.id);
+    if (!clonedThisNode) throw new Error('Cloned graph missing node');
+    return clonedThisNode;
   }
 
   /**
-   * Traverses all paths in the graph, calling iterator on each node visited. Decides which nodes to
-   * visit with the getNext function.
-   * @param {function(Node, Node[]): void} iterator
-   * @param {function(Node): Node[]} getNext
+   * Traverses all connected nodes in BFS order, calling `callback` exactly once
+   * on each. `traversalPath` is the shortest (though not necessarily unique)
+   * path from `node` to the root of the iteration.
+   *
+   * The `getNextNodes` function takes a visited node and returns which nodes to
+   * visit next. It defaults to returning the node's dependents.
+   * @param {(node: Node, traversalPath: Node[]) => void} callback
+   * @param {function(Node): Node[]} [getNextNodes]
    */
-  _traversePaths(iterator, getNext) {
-    const stack = [[/** @type {Node} */(/** @type {BaseNode} */(this))]];
-    while (stack.length) {
-      /** @type {Node[]} */
-      // @ts-ignore - stack has length so it's guaranteed to have an item
-      const path = stack.shift();
-      const node = path[0];
-      iterator(node, path);
-
-      const nodesToAdd = getNext(node);
-      for (const nextNode of nodesToAdd) {
-        stack.push([nextNode].concat(path));
-      }
+  traverse(callback, getNextNodes) {
+    if (!getNextNodes) {
+      getNextNodes = node => node.getDependents();
     }
-  }
 
-  /**
-   * Traverses all connected nodes exactly once, calling iterator on each. Decides which nodes to
-   * visit with the getNext function.
-   * @param {function(Node, Node[]): void} iterator
-   * @param {function(Node): Node[]} [getNext] Defaults to returning the dependents.
-   */
-  traverse(iterator, getNext) {
-    if (!getNext) {
-      getNext = node => node.getDependents();
-    }
+    /** @type {Node[][]} */
+    // @ts-ignore - only traverses graphs of Node, so force tsc to treat `this` as one
+    const queue = [[this]];
+    const visited = new Set([this.id]);
 
-    const visited = new Set();
-    const originalGetNext = getNext;
+    while (queue.length) {
+      /** @type {Node[]} */
+      // @ts-ignore - queue has length so it's guaranteed to have an item
+      const traversalPath = queue.shift();
+      const node = traversalPath[0];
+      callback(node, traversalPath);
 
-    getNext = node => {
-      visited.add(node.id);
-      const allNodesToVisit = originalGetNext(node);
-      const nodesToVisit = allNodesToVisit.filter(nextNode => !visited.has(nextNode.id));
-      nodesToVisit.forEach(nextNode => visited.add(nextNode.id));
-      return nodesToVisit;
-    };
+      for (const nextNode of getNextNodes(node)) {
+        if (visited.has(nextNode.id)) continue;
+        visited.add(nextNode.id);
 
-    this._traversePaths(iterator, getNext);
+        queue.push([nextNode, ...traversalPath]);
+      }
+    }
   }
 
   /**

diff --git a/lighthouse-core/test/lib/dependency-graph/base-node-test.js b/lighthouse-core/test/lib/dependency-graph/base-node-test.js
@@ -207,6 +207,15 @@ describe('DependencyGraph/Node', () => {
       assert.equal(clonedIdMap.get('G'), undefined);
       assert.equal(clonedIdMap.get('H'), undefined);
     });
+
+    it('should throw if original node is not in cloned graph', () => {
+      const graph = createComplexGraph();
+      assert.throws(
+        // clone from root to nodeB, but called on nodeD
+        _ => graph.nodeD.cloneWithRelationships(node => node.id === 'B'),
+        /^Error: Cloned graph missing node$/
+      );
+    });
   });
 
   describe('.traverse', () => {
@@ -218,6 +227,26 @@ describe('DependencyGraph/Node', () => {
       assert.deepEqual(ids, ['A', 'B', 'C', 'D', 'E', 'F', 'G', 'H']);
     });
 
+    it('should include a shortest traversal path to every dependent node', () => {
+      const graph = createComplexGraph();
+      const paths = [];
+      graph.nodeA.traverse((node, traversalPath) => {
+        assert.strictEqual(node.id, traversalPath[0].id);
+        paths.push(traversalPath.map(node => node.id));
+      });
+
+      assert.deepStrictEqual(paths, [
+        ['A'],
+        ['B', 'A'],
+        ['C', 'A'],
+        ['D', 'B', 'A'],
+        ['E', 'D', 'B', 'A'],
+        ['F', 'E', 'D', 'B', 'A'],
+        ['G', 'E', 'D', 'B', 'A'],
+        ['H', 'G', 'E', 'D', 'B', 'A'],
+      ]);
+    });
+
     it('should respect getNext', () => {
       const graph = createComplexGraph();
       const ids = [];