Bug 1382647 - Improve eslint-plugin-mozilla's performance when searching for globals by avoiding rebuilding source when we don't need to. r=mossop
authorMark Banner <standard8@mozilla.com>
Tue, 18 Jul 2017 22:23:58 +0100
changeset 418843 bfeea9fdf02f5d4a3063162b6987b7c88ce8c19b
parent 418842 344d6f11ef151e9bf6dbe5d5d4397f96df60ebe0
child 418844 a1cdfee7cb078b53bbdb06b28039522a7e3da7cb
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmossop
bugs1382647
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1382647 - Improve eslint-plugin-mozilla's performance when searching for globals by avoiding rebuilding source when we don't need to. r=mossop MozReview-Commit-ID: 84uHuepWhZR
tools/lint/eslint/eslint-plugin-mozilla/lib/globals.js
tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
tools/lint/eslint/eslint-plugin-mozilla/lib/rules/mark-test-function-used.js
tools/lint/eslint/eslint-plugin-mozilla/package.json
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/globals.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/globals.js
@@ -117,21 +117,25 @@ GlobalsForNode.prototype = {
       globals = globals.concat(module.exports.getGlobalsForFile(filePath));
     }
 
     return globals;
   },
 
   ExpressionStatement(node, parents, globalScope) {
     let isGlobal = helpers.getIsGlobalScope(parents);
-    let globals = helpers.convertExpressionToGlobals(node, isGlobal);
-    // Map these globals now, as getGlobalsForFile is pre-mapped.
-    globals = globals.map(name => {
-      return { name, writable: true };
-    });
+    let globals = [];
+
+    // Note: We check the expression types here and only call the necessary
+    // functions to aid performance.
+    if (node.expression.type === "AssignmentExpression") {
+      globals = helpers.convertThisAssignmentExpressionToGlobals(node, isGlobal);
+    } else if (node.expression.type === "CallExpression") {
+      globals = helpers.convertCallExpressionToGlobals(node, isGlobal);
+    }
 
     // Here we assume that if importScripts is set in the global scope, then
     // this is a worker. It would be nice if eslint gave us a way of getting
     // the environment directly.
     if (globalScope && globalScope.set.get("importScripts")) {
       let workerDetails = helpers.convertWorkerExpressionToGlobals(node,
         isGlobal, this.dirname);
       globals = globals.concat(workerDetails);
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/helpers.js
@@ -1,49 +1,48 @@
 /**
  * @fileoverview A collection of helper functions.
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/.
  */
 "use strict";
 
-var espree = require("espree");
-var estraverse = require("estraverse");
-var path = require("path");
-var fs = require("fs");
-var ini = require("ini-parser");
+const espree = require("espree");
+const estraverse = require("estraverse");
+const path = require("path");
+const fs = require("fs");
+const ini = require("ini-parser");
 
 var gModules = null;
 var gRootDir = null;
 var directoryManifests = new Map();
 
-var definitions = [
+const callExpressionDefinitions = [
   /^loader\.lazyGetter\(this, "(\w+)"/,
   /^loader\.lazyImporter\(this, "(\w+)"/,
   /^loader\.lazyServiceGetter\(this, "(\w+)"/,
   /^loader\.lazyRequireGetter\(this, "(\w+)"/,
   /^XPCOMUtils\.defineLazyGetter\(this, "(\w+)"/,
   /^XPCOMUtils\.defineLazyModuleGetter\(this, "(\w+)"/,
   /^XPCOMUtils\.defineLazyPreferenceGetter\(this, "(\w+)"/,
   /^XPCOMUtils\.defineLazyServiceGetter\(this, "(\w+)"/,
   /^XPCOMUtils\.defineConstant\(this, "(\w+)"/,
   /^DevToolsUtils\.defineLazyModuleGetter\(this, "(\w+)"/,
   /^DevToolsUtils\.defineLazyGetter\(this, "(\w+)"/,
   /^Object\.defineProperty\(this, "(\w+)"/,
   /^Reflect\.defineProperty\(this, "(\w+)"/,
-  /^this\.__defineGetter__\("(\w+)"/,
-  /^this\.(\w+) =/
+  /^this\.__defineGetter__\("(\w+)"/
 ];
 
-var imports = [
+const imports = [
   /^(?:Cu|Components\.utils)\.import\(".*\/((.*?)\.jsm?)"(?:, this)?\)/
 ];
 
-var workerImportFilenameMatch = /(.*\/)*(.*?\.jsm?)/;
+const workerImportFilenameMatch = /(.*\/)*(.*?\.jsm?)/;
 
 module.exports = {
   get modulesGlobalData() {
     if (!gModules) {
       if (this.isMozillaCentralBased()) {
         gModules = require(path.join(this.rootDir, "tools", "lint", "eslint", "modules.json"));
       } else {
         gModules = require("./modules.json");
@@ -191,50 +190,96 @@ module.exports = {
           }
         }
       }
     }
 
     return results;
   },
 
-  convertExpressionToGlobals(node, isGlobal) {
+  /**
+   * Attempts to convert an AssignmentExpression into a global variable
+   * definition if it applies to `this` in the global scope.
+   *
+   * @param  {Object} node
+   *         The AST node to convert.
+   * @param  {boolean} isGlobal
+   *         True if the current node is in the global scope.
+   *
+   * @return {Array}
+   *         An array of objects that contain details about the globals:
+   *         - {String} name
+   *                    The name of the global.
+   *         - {Boolean} writable
+   *                     If the global is writeable or not.
+   */
+  convertThisAssignmentExpressionToGlobals(node, isGlobal) {
+    if (isGlobal &&
+        node.expression.left &&
+        node.expression.left.object &&
+        node.expression.left.object.type === "ThisExpression" &&
+        node.expression.left.property &&
+        node.expression.left.property.type === "Identifier") {
+      return [{ name: node.expression.left.property.name, writable: true }];
+    }
+    return [];
+  },
+
+  /**
+   * Attempts to convert an CallExpressions that look like module imports
+   * into global variable definitions, using modules.json data if appropriate.
+   *
+   * @param  {Object} node
+   *         The AST node to convert.
+   * @param  {boolean} isGlobal
+   *         True if the current node is in the global scope.
+   *
+   * @return {Array}
+   *         An array of objects that contain details about the globals:
+   *         - {String} name
+   *                    The name of the global.
+   *         - {Boolean} writable
+   *                     If the global is writeable or not.
+   */
+  convertCallExpressionToGlobals(node, isGlobal) {
+    let source;
     try {
-      var source = this.getASTSource(node);
+      source = this.getASTSource(node);
     } catch (e) {
       return [];
     }
 
-    for (var reg of definitions) {
-      let match = source.match(reg);
-      if (match) {
-        // Must be in the global scope
-        if (!isGlobal) {
-          return [];
-        }
-
-        return [match[1]];
-      }
-    }
-
-    let globalModules = this.modulesGlobalData;
-
-    for (reg of imports) {
+    for (let reg of imports) {
       let match = source.match(reg);
       if (match) {
         // The two argument form is only acceptable in the global scope
         if (node.expression.arguments.length > 1 && !isGlobal) {
           return [];
         }
 
+        let globalModules = this.modulesGlobalData;
+
         if (match[1] in globalModules) {
-          return globalModules[match[1]];
+          return globalModules[match[1]].map(name => ({ name, writable: true }));
         }
 
-        return [match[2]];
+        return [{ name: match[2], writable: true }];
+      }
+    }
+
+    // The definition matches below must be in the global scope for us to define
+    // a global, so bail out early if we're not a global.
+    if (!isGlobal) {
+      return [];
+    }
+
+    for (let reg of callExpressionDefinitions) {
+      let match = source.match(reg);
+      if (match) {
+        return [{ name: match[1], writable: true }];
       }
     }
 
     return [];
   },
 
   /**
    * Add a variable to the current scope.
--- a/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/mark-test-function-used.js
+++ b/tools/lint/eslint/eslint-plugin-mozilla/lib/rules/mark-test-function-used.js
@@ -18,20 +18,20 @@ var helpers = require("../helpers");
 
 module.exports = function(context) {
   // ---------------------------------------------------------------------------
   // Public
   // ---------------------------------------------------------------------------
 
   return {
     Program() {
-      if (helpers.getTestType(context) == "browser") {
+      let testType = helpers.getTestType(context);
+      if (testType == "browser") {
         context.markVariableAsUsed("test");
         return;
       }
 
-      if (helpers.getTestType(context) == "xpcshell") {
+      if (testType == "xpcshell") {
         context.markVariableAsUsed("run_test");
-
       }
     }
   };
 };
--- a/tools/lint/eslint/eslint-plugin-mozilla/package.json
+++ b/tools/lint/eslint/eslint-plugin-mozilla/package.json
@@ -1,11 +1,11 @@
 {
   "name": "eslint-plugin-mozilla",
-  "version": "0.4.1",
+  "version": "0.4.2",
   "description": "A collection of rules that help enforce JavaScript coding standard in the Mozilla project.",
   "keywords": [
     "eslint",
     "eslintplugin",
     "eslint-plugin",
     "mozilla",
     "firefox"
   ],