Choose better data structure for nodeWrappers.

Since NodeWrappers lists:

* must be insertion ordered
* must contain only unique values (based on their key)
* support a fast existence / lookup operation

This PR switches it from an array to a map.

As broccoli pipelines grow large, simply searching the array to detect duplicates on insertion can become very costly, and switching to a Map addresses this specific bottleneck.

lib/builder.js

@@ -20,6 +20,7 @@ const BuildError = require('./errors/build');
 const CancelationRequest = require('./cancelation-request');
 const Cancelation = require('./errors/cancelation');
 const logger = require('heimdalljs-logger')('broccoli:builder');
+const filterMap = require('./utils/filter-map');
 
 // Clean up left-over temporary directories on uncaught exception.
 tmp.setGracefulCleanup();
@@ -58,8 +59,9 @@ module.exports = class Builder extends EventEmitter {
 
     // nodeWrappers store additional bookkeeping information, such as paths.
     // This array contains them in topological (build) order.
-    this.nodeWrappers = [];
-    // This populates this.nodeWrappers as a side effect
+    this._nodeWrappers = new Map();
+
+    // This populates this._nodeWrappers as a side effect
     this.outputNodeWrapper = this.makeNodeWrapper(this.outputNode);
 
     // Catching missing directories here helps prevent later errors when we set
@@ -97,9 +99,7 @@ module.exports = class Builder extends EventEmitter {
 
     this.buildId++;
 
-    this.nodeWrappers.forEach(nw => {
-      // We use `.forEach` instead of `for` to close nested functions over `nw`
-
+    for (const nw of this._nodeWrappers.values()) {
       // Wipe all buildState objects at the beginning of the build
       nw.buildState = {};
 
@@ -130,7 +130,7 @@ module.exports = class Builder extends EventEmitter {
             }
           });
       });
-    });
+    }
 
     this._cancelationRequest = new CancelationRequest(promise);
 
@@ -143,8 +143,11 @@ module.exports = class Builder extends EventEmitter {
           this.buildHeimdallTree(outputNodeWrapper);
         }),
       () => {
-        let buildsSkipped = this.nodeWrappers.filter(nw => nw.buildState.built === false).length;
-        logger.debug(`Total nodes skipped: ${buildsSkipped} out of ${this.nodeWrappers.length}`);
+        let buildsSkipped = filterMap(
+          this._nodeWrappers.values(),
+          nw => nw.buildState.built === false
+        ).length;
+        logger.debug(`Total nodes skipped: ${buildsSkipped} out of ${this._nodeWrappers.size}`);
 
         this._cancelationRequest = null;
       }
@@ -170,11 +173,8 @@ module.exports = class Builder extends EventEmitter {
   makeNodeWrapper(node, _stack) {
     if (_stack == null) _stack = [];
 
-    // Dedupe nodes reachable through multiple paths
-    for (let i = 0; i < this.nodeWrappers.length; i++) {
-      if (this.nodeWrappers[i].originalNode === node) {
-        return this.nodeWrappers[i];
-      }
+    if (this._nodeWrappers.has(node)) {
+      return this._nodeWrappers.get(node);
     }
 
     // Turn string nodes into WatchedDir nodes
@@ -249,21 +249,21 @@ module.exports = class Builder extends EventEmitter {
     // 'source' nodes it's empty.
     nodeWrapper.inputNodeWrappers = inputNodeWrappers;
 
-    nodeWrapper.id = this.nodeWrappers.length;
+    nodeWrapper.id = this._nodeWrappers.size;
 
-    // this.nodeWrappers will contain all the node wrappers in topological
+    // this._nodeWrappers will contain all the node wrappers in topological
     // order, i.e. each node comes after all its input nodes.
     //
-    // It's unfortunate that we're mutating this.nodeWrappers as a side effect,
+    // It's unfortunate that we're mutating this._nodeWrappers as a side effect,
     // but since we work backwards from the output node to discover all the
     // input nodes, it's harder to do a side-effect-free topological sort.
-    this.nodeWrappers.push(nodeWrapper);
+    this._nodeWrappers.set(nodeWrapper.originalNode, nodeWrapper);
 
     return nodeWrapper;
   }
 
   get watchedSourceNodeWrappers() {
-    return this.nodeWrappers.filter(nw => {
+    return filterMap(this._nodeWrappers.values(), nw => {
       return nw.nodeInfo.nodeType === 'source' && nw.nodeInfo.watched;
     });
   }
@@ -310,12 +310,9 @@ module.exports = class Builder extends EventEmitter {
     this.builderTmpDir = tmpobj.name;
     this.builderTmpDirCleanup = tmpobj.removeCallback;
 
-    for (let i = 0; i < this.nodeWrappers.length; i++) {
-      const nodeWrapper = this.nodeWrappers[i];
+    for (let nodeWrapper of this._nodeWrappers.values()) {
       if (nodeWrapper.nodeInfo.nodeType === 'transform') {
-        nodeWrapper.inputPaths = nodeWrapper.inputNodeWrappers.map(function(nw) {
-          return nw.outputPath;
-        });
+        nodeWrapper.inputPaths = nodeWrapper.inputNodeWrappers.map(nw => nw.outputPath);
         nodeWrapper.outputPath = this.mkTmpDir(nodeWrapper, 'out');
 
         if (nodeWrapper.nodeInfo.needsCache) {
@@ -342,7 +339,7 @@ module.exports = class Builder extends EventEmitter {
     // 1 .. 147 -> '001' .. '147'
     const paddedId = underscoreString.pad(
       '' + nodeWrapper.id,
-      ('' + this.nodeWrappers.length).length,
+      ('' + this._nodeWrappers.size).length,
       '0'
     );
     const dirname = type + '-' + paddedId + '-' + suffix;
@@ -351,9 +348,13 @@ module.exports = class Builder extends EventEmitter {
     return tmpDir;
   }
 
+  // for compat
+  get nodeWrappers() {
+    return [...this._nodeWrappers.values()];
+  }
+
   setupNodes() {
-    for (let i = 0; i < this.nodeWrappers.length; i++) {
-      const nw = this.nodeWrappers[i];
+    for (let nw of this._nodeWrappers.values()) {
       try {
         nw.setup(this.features);
       } catch (err) {

lib/utils/filter-map.js

@@ -0,0 +1,11 @@
+'use strict';
+
+module.exports = function filterMap(iterator, cb) {
+  const result = [];
+  for (const entry of iterator) {
+    if (cb(entry)) {
+      result.push(entry);
+    }
+  }
+  return result;
+};

test/builder_test.js

@@ -197,7 +197,6 @@ describe('Builder', function() {
               expect(barBuildSpy).to.have.been.calledOnce;
               expect(buildSpy).to.have.been.calledOnce;
 
-              // Now we simulate a rebuild (and the revisions have not changed)
               builder.nodeWrappers.find(wrap => wrap.outputPath === 'test/fixtures/basic').revise();
 
               return builder.build();

test/utils/filter_map_test.js

@@ -0,0 +1,15 @@
+'use strict';
+
+const chai = require('chai');
+const expect = chai.expect;
+const filterMap = require('../../lib/utils/filter-map');
+
+describe('filterMap', function() {
+  it('works', function() {
+    expect(filterMap([], () => true)).to.eql([]);
+    expect(filterMap([1, false, 2], () => true)).to.eql([1, false, 2]);
+    expect(filterMap([1, true, 2], () => false)).to.eql([]);
+    expect(filterMap([1, true, 2], x => x === 1)).to.eql([1]);
+    expect(filterMap([1, true, 2], x => typeof x === 'number')).to.eql([1, 2]);
+  });
+});