Repository: ember-cli/ember-cli
Pull request #7994 (2018-08-23 15:47:10)

[PERF] Speed up package info cache
[PERF] PackageInfoCache should not traverse node_modules which will not be considered for add-on discovery

* reduces vanilla ember-app boot up time by 200-400ms
* reduces vanilla ember-app add-on discovery by 2/3’s
* apps with large node_module folders benefit much more.

How does this work?

only walk node_modules of:

* the root dependency (the app, add-on, etc)
* child dependencies marked as ember-addon

Skip over the rest and consider them as empty as our algorithm will not discover add-ons from these node_modules directories anyways.

This reduces unneeded node_module traversals dramatically

stefanpenner: cc @dcombslinkedin 
stefanpenner: From an addon discovery standpoint the algorithm is no different. The exception being it traverses an order to two magnitudes less directories as it knows it cannot discover addons down those paths.

Although for some commands, like build a second or so isn’t much. But it is quite a lot for other commands like help or generate etc. especially for vanilla apps, where the package-info work almost doubles cli boot up time.
stefanpenner: The reason I noticed this, was I am working on a simpler and faster addon/cli test harness and I noticed I was wasting 600ms for each test iteration in this code. Before 800ms and after 200ms.

This change improves my test throughout but nearly 3x.
dcombslinkedin: @stefanpenner , I don't see any problems with the speedup you made. And I'm definitely all for making things like help and generate go faster. I'd noticed that lag myself a couple times, so it's all good. I'm not sure I could prove something regressed at this point, since the original code was intended to just follow the idea of the node resolution algorithm, just remove the repeated file-system accesses. Since you know the algorithm results for addon-discovery are unchanged, that's good enough for me! Thanks. ShipIt!

Patch:
diff --git a/lib/models/package-info-cache/error-list.js b/lib/models/package-info-cache/error-list.js
index 507e78007..8ceb43ce5 100644
--- a/lib/models/package-info-cache/error-list.js
+++ b/lib/models/package-info-cache/error-list.js
@@ -10,7 +10,7 @@ const ErrorEntry = require('./error-entry');
  * @protected
  * @class ErrorList
  */
-class ErrorList {
+module.exports = class ErrorList {
   constructor() {
     this.errors = [];
   }
@@ -36,6 +36,4 @@ class ErrorList {
   hasErrors() {
     return this.errors.length > 0;
   }
-}
-
-module.exports = ErrorList;
+};
diff --git a/lib/models/package-info-cache/index.js b/lib/models/package-info-cache/index.js
index d701b78dd..fd4a9b5e3 100644
--- a/lib/models/package-info-cache/index.js
+++ b/lib/models/package-info-cache/index.js
@@ -240,7 +240,7 @@ class PackageInfoCache {
    */
   loadProject(projectInstance) {
 
-    let pkgInfo = this._readPackage(projectInstance.root, projectInstance.pkg);
+    let pkgInfo = this._readPackage(projectInstance.root, projectInstance.pkg, true);
 
     // NOTE: the returned val may contain errors, or may contain
     // other packages that have errors. We will try to process
@@ -494,8 +494,10 @@ class PackageInfoCache {
    * @method _readPackage
    * @param {String} pkgDir the path of the directory to read the package.json from and
    *  process the contents and create a new cache entry or entries.
+   * @param {Boolean} isRoot, for when this is to be considered the root
+   * package, whose dependencies we must all consider for discovery
    */
-  _readPackage(packageDir, pkg) {
+  _readPackage(packageDir, pkg, isRoot) {
     let normalizedPackageDir = path.normalize(packageDir);
 
     // Most of the time, normalizedPackageDir is already a real path (i.e. fs.realpathSync
@@ -572,7 +574,7 @@ class PackageInfoCache {
 
     // Create a new PackageInfo and load any errors as needed.
     // Note that pkg may be an empty object here.
-    pkgInfo = new PackageInfo(pkg, realPath, this);
+    pkgInfo = new PackageInfo(pkg, realPath, this, isRoot);
 
     if (setupErrors.hasErrors()) {
       pkgInfo.errors = setupErrors;
@@ -582,7 +584,7 @@ class PackageInfoCache {
     // If we have an ember-addon, check that the main exists and points
     // to a valid file.
     if (Array.isArray(pkg.keywords) &&
-        pkg.keywords.indexOf('ember-addon') >= 0) {
+      pkg.keywords.indexOf('ember-addon') >= 0) {
 
       // Note: when we have both 'main' and ember-addon:main, the latter takes precedence
       let main = (pkg['ember-addon'] && pkg['ember-addon'].main) || pkg['main'];
@@ -626,15 +628,21 @@ class PackageInfoCache {
       }
     }
 
-    // read addon modules from node_modules. We read the whole directory
-    // because it's assumed that npm/yarn may have placed addons in the
-    // directory from lower down in the project tree, and we want to get
-    // the data into the cache ASAP. It may not necessarily be a 'real' error
-    // if we find an issue, if nobody below is actually invoking the addon.
-    let nodeModules = this._readNodeModulesList(path.join(realPath, 'node_modules'));
+    if (pkgInfo.mayHaveAddons) {
+      // read addon modules from node_modules. We read the whole directory
+      // because it's assumed that npm/yarn may have placed addons in the
+      // directory from lower down in the project tree, and we want to get
+      // the data into the cache ASAP. It may not necessarily be a 'real' error
+      // if we find an issue, if nobody below is actually invoking the addon.
+      let nodeModules = this._readNodeModulesList(path.join(realPath, 'node_modules'));
 
-    if (nodeModules) {
-      pkgInfo.nodeModules = nodeModules;
+      if (nodeModules) {
+        pkgInfo.nodeModules = nodeModules;
+      }
+    } else {
+      // will not have addons, so even if there are node_modules here, we can
+      // simply pretend there are none.
+      pkgInfo.nodeModules = NodeModulesList.NULL;
     }
 
     return pkgInfo;
diff --git a/lib/models/package-info-cache/node-modules-list.js b/lib/models/package-info-cache/node-modules-list.js
index 9a544c2e4..f1b0b7f09 100644
--- a/lib/models/package-info-cache/node-modules-list.js
+++ b/lib/models/package-info-cache/node-modules-list.js
@@ -2,6 +2,7 @@
 
 const ErrorList = require('./error-list');
 
+let NULL;
 /**
  * Class that stores information about a node_modules directory (i.e., the
  * packages and subdirectories in the directory). It is one of the
@@ -11,12 +12,33 @@ const ErrorList = require('./error-list');
  * @public
  * @class NodeModulesList
  */
-class NodeModulesList {
+module.exports = class NodeModulesList {
   constructor(realPath, cache) {
     this.realPath = realPath;
-    this.cache = cache;
+    this.hasEntries = false;
     this.entries = Object.create(null);
     this.errors = new ErrorList();
+    this.cache = cache;
+  }
+
+  // when we encounter a node_modules we will never traverse, we insert a NULL variant.
+  // https://en.wikipedia.org/wiki/Null_object_pattern
+  // returns a Frozen and Empty NodeModulesList
+  static get NULL() {
+    if (NULL) {
+      return NULL;
+    }
+
+    NULL = new this('/dev/null');
+
+    Object.freeze(NULL.entries);
+
+    Object.freeze(NULL.errors.errors);
+    Object.freeze(NULL.errors);
+
+    Object.freeze(NULL);
+
+    return NULL;
   }
 
   /**
@@ -56,6 +78,7 @@ class NodeModulesList {
    * to the given entry name in the file system.
    */
   addEntry(entryName, entryVal) {
+    this.hasEntries = true;
     this.entries[entryName] = entryVal;
   }
 
@@ -70,6 +93,8 @@ class NodeModulesList {
    * @return the desired PackageInfo if one exists for the given name, else null.
    */
   findPackage(packageName) {
+    if (!this.hasEntries) { return null; }
+
     let val;
 
     if (packageName.startsWith('@')) {
@@ -79,12 +104,12 @@ class NodeModulesList {
         entry instanceof NodeModulesList
           ? entry.findPackage(parts[1]) // the real name
           : null;
-    } else {
+    } else if (packageName in this.entries) {
       val = this.entries[packageName];
+    } else {
+      val = null;
     }
 
     return val;
   }
-}
-
-module.exports = NodeModulesList;
+};
diff --git a/lib/models/package-info-cache/package-info.js b/lib/models/package-info-cache/package-info.js
index cc71a70b4..2faaae196 100644
--- a/lib/models/package-info-cache/package-info.js
+++ b/lib/models/package-info-cache/package-info.js
@@ -5,6 +5,10 @@ const ErrorList = require('./error-list');
 const Errors = require('./errors');
 const AddonInfo = require('../addon-info');
 
+function isEmberAddon(pkg) {
+  return Array.isArray(pkg.keywords) && pkg.keywords.indexOf('ember-addon') > -1;
+}
+
 function lexicographically(a, b) {
   const aIsString = typeof a.name === 'string';
   const bIsString = typeof b.name === 'string';
@@ -52,7 +56,7 @@ function pushUnique(array, entry) {
  * @class PackageInfo
  */
 class PackageInfo {
-  constructor(pkgObj, realPath, cache) {
+  constructor(pkgObj, realPath, cache, isRoot = false) {
     this.pkg = pkgObj;
     this.pkg['ember-addon'] = this.pkg['ember-addon'] || {};
     this.realPath = realPath;
@@ -78,6 +82,8 @@ class PackageInfo {
     // not actually be used.
     this.valid = true;
 
+    this.mayHaveAddons = isRoot || isEmberAddon(this.pkg);
+
     this._hasDumpedInvalidAddonPackages = false;
   }
 


Test patch:
diff --git a/tests/unit/models/package-info-cache-test.js b/tests/unit/models/package-info-cache-test.js
index a77e96407..2e5623c57 100644
--- a/tests/unit/models/package-info-cache-test.js
+++ b/tests/unit/models/package-info-cache-test.js
@@ -100,6 +100,10 @@ describe('models/package-info-cache.js', function() {
       expect(projectPackageInfo.valid).to.be.true;
     });
 
+    it('is a project, so it may have addons', function() {
+      expect(projectPackageInfo.mayHaveAddons).to.eql(true);
+    });
+
     it('shows projectPackageInfo has cliInfo at ember-cli root dir', function() {
       expect(projectPackageInfo.cliInfo).to.exist;
 
@@ -210,6 +214,15 @@ describe('models/package-info-cache.js', function() {
         fixturifyProject.dispose();
       });
 
+      it('has dependencies who have their mayHaveAddons correctly set', function() {
+        expect(projectPackageInfo.devDependencyPackages['non-ember-thingy']).to.have.property('mayHaveAddons', false);
+        expect(projectPackageInfo.devDependencyPackages['ember-cli']).to.have.property('mayHaveAddons', false);
+        expect(projectPackageInfo.dependencyPackages['loader.js']).to.have.property('mayHaveAddons', true);
+        expect(projectPackageInfo.dependencyPackages['ember-resolver']).to.have.property('mayHaveAddons', true);
+        expect(projectPackageInfo.dependencyPackages['ember-random-addon']).to.have.property('mayHaveAddons', true);
+        expect(projectPackageInfo.dependencyPackages['something-else']).to.have.property('mayHaveAddons', true);
+      });
+
       it('validates projectPackageInfo', function() {
         expect(projectPackageInfo).to.exist;
         expect(projectPackageInfo.pkg).to.exist;
diff --git a/tests/unit/models/package-info-cache/node-module-list-test.js b/tests/unit/models/package-info-cache/node-module-list-test.js
new file mode 100644
index 000000000..49c5b5b27
--- /dev/null
+++ b/tests/unit/models/package-info-cache/node-module-list-test.js
@@ -0,0 +1,46 @@
+'use strict';
+
+const expect = require('chai').expect;
+const NodeModulesList = require('../../../../lib/models/package-info-cache/node-modules-list');
+
+describe('NodeModulesList', function() {
+  it('correctly constructs', function() {
+    expect(new NodeModulesList()).to.be.ok;
+    expect(new NodeModulesList('/some/path')).to.be.ok;
+  });
+
+  describe('.NULL', function() {
+    it('returns a singleton, deeply frozen NodeMoudlesList', function() {
+      expect(NodeModulesList.NULL).to.equal(NodeModulesList.NULL);
+
+      expect(NodeModulesList.NULL).to.be.frozen;
+      expect(NodeModulesList.NULL.entries).to.be.frozen;
+      expect(NodeModulesList.NULL.errors).to.be.frozen;
+      expect(NodeModulesList.NULL.errors.errors).to.be.frozen;
+    });
+  });
+
+  describe('findPackage', function() {
+    it('works with no entries', function() {
+      let list = new NodeModulesList();
+      expect(list.findPackage('omg')).to.eql(null);
+    });
+
+    it('supports basic entries (missing, present, scoped)', function() {
+      let list = new NodeModulesList();
+      let scoped = new NodeModulesList();
+      let omg = { name: 'omg' };
+      let scopedOmg = { name: 'omg' };
+      scoped.addEntry('omg', scopedOmg);
+
+      list.addEntry('omg', omg);
+      list.addEntry('@thescope', scoped);
+
+      expect(list.findPackage('omg')).to.eql(omg);
+      expect(list.findPackage('nope')).to.eql(null);
+      expect(list.findPackage('@thescope/omg')).to.eql(scopedOmg);
+      expect(list.findPackage('@thescope/nope')).to.eql(null);
+      expect(list.findPackage('@nope/nope')).to.eql(null);
+    });
+  });
+});