Use a memoized key lookup for rules

On my project, this makes ESLint as a whole roughly 8% faster, and it seems to result in consistently faster benchmarks. Note a behavior change - member expressions used to find the first matching rule, while they now favor the more general rule if relevant.
amilajack · May 10, 2024 · b3b017d · b3b017d
1 parent eba26a7
commit b3b017d
Show file tree

Hide file tree

Showing 4 changed files with 100 additions and 37 deletions.
diff --git a/src/helpers.ts b/src/helpers.ts
@@ -10,6 +10,7 @@ import {
   BrowsersListOpts,
 } from "./types";
 import { TargetNameMappings } from "./constants";
+import { RulesLookup, lookupKey } from "./rules-lookup";
 
 /*
 3) Figures out which browsers user is targeting
@@ -40,12 +41,12 @@ function checkNotInsideIfStatementAndReport(
 export function lintCallExpression(
   context: Context,
   handleFailingRule: HandleFailingRule,
-  rules: AstMetadataApiWithTargetsResolver[],
+  rulesByObject: RulesLookup,
   node: ESLintNode
 ) {
   if (!node.callee) return;
   const calleeName = node.callee.name;
-  const failingRule = rules.find((rule) => rule.object === calleeName);
+  const failingRule = rulesByObject.get(calleeName);
   if (failingRule)
     checkNotInsideIfStatementAndReport(
       context,
@@ -58,12 +59,12 @@ export function lintCallExpression(
 export function lintNewExpression(
   context: Context,
   handleFailingRule: HandleFailingRule,
-  rules: Array<AstMetadataApiWithTargetsResolver>,
+  rulesByObject: RulesLookup,
   node: ESLintNode
 ) {
   if (!node.callee) return;
   const calleeName = node.callee.name;
-  const failingRule = rules.find((rule) => rule.object === calleeName);
+  const failingRule = rulesByObject.get(calleeName);
   if (failingRule)
     checkNotInsideIfStatementAndReport(
       context,
@@ -76,13 +77,11 @@ export function lintNewExpression(
 export function lintExpressionStatement(
   context: Context,
   handleFailingRule: HandleFailingRule,
-  rules: AstMetadataApiWithTargetsResolver[],
+  rulesByObject: RulesLookup,
   node: ESLintNode
 ) {
   if (!node?.expression?.name) return;
-  const failingRule = rules.find(
-    (rule) => rule.object === node?.expression?.name
-  );
+  const failingRule = rulesByObject.get(node?.expression?.name);
   if (failingRule)
     checkNotInsideIfStatementAndReport(
       context,
@@ -118,7 +117,8 @@ function protoChainFromMemberExpression(node: ESLintNode): string[] {
 export function lintMemberExpression(
   context: Context,
   handleFailingRule: HandleFailingRule,
-  rules: Array<AstMetadataApiWithTargetsResolver>,
+  rulesByProtoChainId: RulesLookup,
+  rulesByObjectProperty: RulesLookup,
   node: ESLintNode
 ) {
   if (!node.object || !node.property) return;
@@ -134,9 +134,7 @@ export function lintMemberExpression(
         ? rawProtoChain.slice(1)
         : rawProtoChain;
     const protoChainId = protoChain.join(".");
-    const failingRule = rules.find(
-      (rule) => rule.protoChainId === protoChainId
-    );
+    const failingRule = rulesByProtoChainId.get(protoChainId);
     if (failingRule) {
       checkNotInsideIfStatementAndReport(
         context,
@@ -148,11 +146,9 @@ export function lintMemberExpression(
   } else {
     const objectName = node.object.name;
     const propertyName = node.property.name;
-    const failingRule = rules.find(
-      (rule) =>
-        rule.object === objectName &&
-        (rule.property == null || rule.property === propertyName)
-    );
+    const failingRule =
+      rulesByObjectProperty.get(lookupKey(objectName, null)) ||
+      rulesByObjectProperty.get(lookupKey(objectName, propertyName));
     if (failingRule)
       checkNotInsideIfStatementAndReport(
         context,

diff --git a/src/rules-lookup.ts b/src/rules-lookup.ts
@@ -0,0 +1,35 @@
+import { AstMetadataApiWithTargetsResolver } from "./types";
+
+// https://stackoverflow.com/q/49752151/25507
+type KeysOfType<T, TProp> = keyof T &
+  { [P in keyof T]: T[P] extends TProp ? P : never }[keyof T];
+
+export type RulesLookup = Map<
+  string | undefined,
+  AstMetadataApiWithTargetsResolver
+>;
+
+export function lookupKey(...args: Array<string | null | undefined>) {
+  return args.map((i) => (i == null ? null : i)).join("\0");
+}
+
+export function makeLookup(
+  rules: AstMetadataApiWithTargetsResolver[],
+  ...keys: Array<
+    KeysOfType<Required<AstMetadataApiWithTargetsResolver>, string>
+  >
+) {
+  const lookup = new Map<
+    string | undefined,
+    AstMetadataApiWithTargetsResolver
+  >();
+  // Iterate in inverse order to favor earlier rules in case of conflict.
+  for (let i = rules.length - 1; i >= 0; i--) {
+    const key =
+      keys.length === 1
+        ? rules[i][keys[0]]
+        : lookupKey(...keys.map((k) => rules[i][k]));
+    lookup.set(key, rules[i]);
+  }
+  return lookup;
+}
diff --git a/src/rules/compat.ts b/src/rules/compat.ts
@@ -26,6 +26,7 @@ import {
   BrowsersListOpts,
 } from "../types";
 import { nodes } from "../providers";
+import { RulesLookup, makeLookup } from "../rules-lookup";
 
 type ESLint = {
   [astNodeTypeName: string]: (node: ESLintNode) => void;
@@ -112,29 +113,63 @@ type RulesFilteredByTargets = {
   ExpressionStatement: AstMetadataApiWithTargetsResolver[];
 };
 
+type RuleLookupsSet = {
+  CallExpressionsByObject: RulesLookup;
+  NewExpressionsByObject: RulesLookup;
+  ExpressionStatementsByObject: RulesLookup;
+  MemberExpressionsByProtoChainId: RulesLookup;
+  MemberExpressionsByObjectProperty: RulesLookup;
+};
+
 /**
  * A small optimization that only lints APIs that are not supported by targeted browsers.
  * For example, if the user is targeting chrome 50, which supports the fetch API, it is
  * wasteful to lint calls to fetch.
  */
 const getRulesForTargets = memoize(
-  (targetsJSON: string, lintAllEsApis: boolean): RulesFilteredByTargets => {
-    const result = {
-      CallExpression: [] as AstMetadataApiWithTargetsResolver[],
-      NewExpression: [] as AstMetadataApiWithTargetsResolver[],
-      MemberExpression: [] as AstMetadataApiWithTargetsResolver[],
-      ExpressionStatement: [] as AstMetadataApiWithTargetsResolver[],
+  (targetsJSON: string, lintAllEsApis: boolean): RuleLookupsSet => {
+    const rules: RulesFilteredByTargets = {
+      CallExpression: [],
+      NewExpression: [],
+      MemberExpression: [],
+      ExpressionStatement: [],
     };
     const targets = JSON.parse(targetsJSON);
 
     nodes
       .filter((node) => (lintAllEsApis ? true : node.kind !== "es"))
       .forEach((node) => {
         if (!node.getUnsupportedTargets(node, targets).length) return;
-        result[node.astNodeType].push(node);
+        rules[node.astNodeType].push(node);
       });
 
-    return result;
+    const expressionStatementRules = [
+      ...rules.MemberExpression,
+      ...rules.CallExpression,
+    ];
+    const memberExpressionRules = [
+      ...rules.MemberExpression,
+      ...rules.CallExpression,
+      ...rules.NewExpression,
+    ];
+
+    return {
+      CallExpressionsByObject: makeLookup(rules.CallExpression, "object"),
+      NewExpressionsByObject: makeLookup(rules.NewExpression, "object"),
+      ExpressionStatementsByObject: makeLookup(
+        expressionStatementRules,
+        "object"
+      ),
+      MemberExpressionsByProtoChainId: makeLookup(
+        memberExpressionRules,
+        "protoChainId"
+      ),
+      MemberExpressionsByObjectProperty: makeLookup(
+        memberExpressionRules,
+        "object",
+        "property"
+      ),
+    };
   }
 );
 
@@ -217,29 +252,26 @@ export default {
         null,
         context,
         handleFailingRule,
-        targetedRules.CallExpression
+        targetedRules.CallExpressionsByObject
       ),
       NewExpression: lintNewExpression.bind(
         null,
         context,
         handleFailingRule,
-        targetedRules.NewExpression
+        targetedRules.NewExpressionsByObject
       ),
       ExpressionStatement: lintExpressionStatement.bind(
         null,
         context,
         handleFailingRule,
-        [...targetedRules.MemberExpression, ...targetedRules.CallExpression]
+        targetedRules.ExpressionStatementsByObject
       ),
       MemberExpression: lintMemberExpression.bind(
         null,
         context,
         handleFailingRule,
-        [
-          ...targetedRules.MemberExpression,
-          ...targetedRules.CallExpression,
-          ...targetedRules.NewExpression,
-        ]
+        targetedRules.MemberExpressionsByProtoChainId,
+        targetedRules.MemberExpressionsByObjectProperty
       ),
       // Keep track of all the defined variables. Do not report errors for nodes that are not defined
       Identifier(node: ESLintNode) {

diff --git a/test/e2e.spec.ts b/test/e2e.spec.ts
@@ -540,7 +540,7 @@ ruleTester.run("compat", rule, {
       settings: { browsers: ["ie 10"] },
       errors: [
         {
-          message: "Promise.resolve() is not supported in IE 10",
+          message: "Promise is not supported in IE 10",
           type: "MemberExpression",
         },
       ],
@@ -550,7 +550,7 @@ ruleTester.run("compat", rule, {
       settings: { browsers: ["ie 10"] },
       errors: [
         {
-          message: "Promise.all() is not supported in IE 10",
+          message: "Promise is not supported in IE 10",
           type: "MemberExpression",
         },
       ],
@@ -560,7 +560,7 @@ ruleTester.run("compat", rule, {
       settings: { browsers: ["ie 10"] },
       errors: [
         {
-          message: "Promise.race() is not supported in IE 10",
+          message: "Promise is not supported in IE 10",
           type: "MemberExpression",
         },
       ],
@@ -570,7 +570,7 @@ ruleTester.run("compat", rule, {
       settings: { browsers: ["ie 10"] },
       errors: [
         {
-          message: "Promise.reject() is not supported in IE 10",
+          message: "Promise is not supported in IE 10",
           type: "MemberExpression",
         },
       ],