perf(builtins): run builtins transform on multi pass (#859)

Co-authored-by: Boopathi Rajaa <me@boopathi.in> * perf(builtins): run builtins transform on multi pass * don't evaluate in minify-builtins * fix cases where Math is already found in scope * add comments * add test * bail out for already transformed ones
babel · May 17, 2018 · 01eac1c · 01eac1c
1 parent 5dbc73f
commit 01eac1c
Show file tree

Hide file tree

Showing 7 changed files with 109 additions and 47 deletions.
diff --git a/packages/babel-plugin-minify-builtins/__tests__/fixtures/builtin-methods/expected.js b/packages/babel-plugin-minify-builtins/__tests__/fixtures/builtin-methods/expected.js
@@ -1,7 +1,7 @@
 function c() {
   var _Mathmax = Math.max;
   let a = 10;
-  const d = false;
+  const d = Number.isNaN(a);
   _Mathmax(a, b) + _Mathmax(b, a);
-  return d && true;
+  return d && Number.isFinite(a);
 }
diff --git a/packages/babel-plugin-minify-builtins/__tests__/fixtures/evaluate-builtins/expected.js b/packages/babel-plugin-minify-builtins/__tests__/fixtures/evaluate-builtins/expected.js
@@ -1,4 +1,4 @@
-const a = 5;
+const a = Math.max(Math.floor(2), 5);
 let b = 1.8;
-let x = 5;
+let x = Math.floor(Math.max(a, b));
 foo(x);
diff --git a/packages/babel-plugin-minify-builtins/__tests__/fixtures/in-function-scope/expected.js b/packages/babel-plugin-minify-builtins/__tests__/fixtures/in-function-scope/expected.js
@@ -8,7 +8,7 @@ var a = () => {
   c: () => {
     _Mathfloor(d);
 
-    2;
+    Math.max(2, 1);
   };
 };
 

diff --git a/packages/babel-plugin-minify-builtins/__tests__/fixtures/local-binding/actual.js b/packages/babel-plugin-minify-builtins/__tests__/fixtures/local-binding/actual.js
@@ -0,0 +1,18 @@
+function wow() {
+  var Math = foo;
+
+  var nativeMin = Math.min;
+
+  var xMin = Math.min;
+  var yMin = Math.min;
+
+  return {
+    baseInRange: Math.min(foo),
+    min: nativeMin(bar),
+    x: xMin(x),
+    y: yMin(y),
+    xMin,
+    yMin,
+    nativeMin
+  };
+}
diff --git a/packages/babel-plugin-minify-builtins/__tests__/fixtures/local-binding/expected.js b/packages/babel-plugin-minify-builtins/__tests__/fixtures/local-binding/expected.js
@@ -0,0 +1,16 @@
+function wow() {
+  var _Mathmin = (0, eval)("this").Math.min;
+  var Math = foo;
+  var nativeMin = _Mathmin;
+  var xMin = _Mathmin;
+  var yMin = _Mathmin;
+  return {
+    baseInRange: _Mathmin(foo),
+    min: nativeMin(bar),
+    x: xMin(x),
+    y: yMin(y),
+    xMin,
+    yMin,
+    nativeMin
+  };
+}
diff --git a/packages/babel-plugin-minify-builtins/package.json b/packages/babel-plugin-minify-builtins/package.json
@@ -11,8 +11,5 @@
   "license": "MIT",
   "author": "Vignesh Shanmugam <vignesh.shanmugam22@gmail.com> (https://vigneshh.in)",
   "main": "lib/index.js",
-  "repository": "https://github.com/babel/minify/tree/master/packages/babel-plugin-minify-builtins",
-  "dependencies": {
-    "babel-helper-evaluate-path": "^0.4.3"
-  }
+  "repository": "https://github.com/babel/minify/tree/master/packages/babel-plugin-minify-builtins"
 }
diff --git a/packages/babel-plugin-minify-builtins/src/index.js b/packages/babel-plugin-minify-builtins/src/index.js
@@ -1,26 +1,20 @@
 "use strict";
 
-const evaluate = require("babel-helper-evaluate-path");
 // Assuming all the static methods from below array are side effect free evaluation
 // except Math.random
 const VALID_CALLEES = ["String", "Number", "Math"];
 const INVALID_METHODS = ["random"];
 
+const newIssueUrl = "https://github.com/babel/minify/issues/new";
+
 module.exports = function({ types: t }) {
   class BuiltInReplacer {
-    constructor(program, { tdz }) {
-      this.program = program;
-      this.tdz = tdz;
+    constructor() {
       // map<expr_name, path[]>;
       this.pathsToUpdate = new Map();
     }
 
-    run() {
-      this.collect();
-      this.replace();
-    }
-
-    collect() {
+    getCollectVisitor() {
       const context = this;
 
       const collectVisitor = {
@@ -65,23 +59,19 @@ module.exports = function({ types: t }) {
 
             // computed property should not be optimized
             // Math[max]() -> Math.max()
-            if (!isComputed(node) && isBuiltin(node)) {
-              const result = evaluate(path, { tdz: context.tdz });
-              // deopt when we have side effecty evaluate-able arguments
-              // Math.max(foo(), 1) --> untouched
-              // Math.floor(1) --> 1
-              if (result.confident && hasPureArgs(path)) {
-                path.replaceWith(t.valueToNode(result.value));
-              } else if (!getFunctionParent(callee).isProgram()) {
-                const expName = memberToString(node);
-                addToMap(context.pathsToUpdate, expName, callee);
-              }
+            if (
+              !isComputed(node) &&
+              isBuiltin(node) &&
+              !getFunctionParent(callee).isProgram()
+            ) {
+              const expName = memberToString(node);
+              addToMap(context.pathsToUpdate, expName, callee);
             }
           }
         }
       };
 
-      this.program.traverse(collectVisitor);
+      return collectVisitor;
     }
 
     replace() {
@@ -104,21 +94,60 @@ module.exports = function({ types: t }) {
           for (const path of subpaths) {
             path.replaceWith(t.clone(uniqueIdentifier));
           }
+
           // hoist the created var to the top of the function scope
-          parent.get("body").unshiftContainer("body", newNode);
+          const target = parent.get("body");
+
+          /**
+           * Here, we validate a case where there is a local binding of
+           * one of Math, String or Number. Here we have to get the
+           * global Math instead of using the local one - so we do the
+           * following transformation
+           *
+           * var _Mathmax = Math.max;
+           *
+           * to
+           *
+           * var _Mathmax = (0, eval)("this").Math.max;
+           */
+          for (const builtin of VALID_CALLEES) {
+            if (target.scope.getBinding(builtin)) {
+              const prev = newNode.declarations[0].init;
+
+              if (!t.isMemberExpression(prev)) {
+                throw new Error(
+                  `minify-builtins expected a MemberExpression. ` +
+                    `Found ${prev.type}. ` +
+                    `Please report this at ${newIssueUrl}`
+                );
+              }
+
+              if (!t.isMemberExpression(prev.object)) {
+                newNode.declarations[0].init = t.memberExpression(
+                  t.memberExpression(getGlobalThis(), prev.object),
+                  prev.property
+                );
+              }
+            }
+          }
+
+          target.unshiftContainer("body", newNode);
         }
       }
     }
   }
 
+  const builtInReplacer = new BuiltInReplacer();
+
   return {
     name: "minify-builtins",
-    visitor: {
-      Program(path, { opts: { tdz = false } = {} }) {
-        const builtInReplacer = new BuiltInReplacer(path, { tdz });
-        builtInReplacer.run();
+    visitor: Object.assign({}, builtInReplacer.getCollectVisitor(), {
+      Program: {
+        exit() {
+          builtInReplacer.replace();
+        }
       }
-    }
+    })
   };
 
   function memberToString(memberExprNode) {
@@ -205,6 +234,18 @@ module.exports = function({ types: t }) {
       }
     }
   }
+
+  /**
+   * returns
+   *
+   * (0, eval)("this")
+   */
+  function getGlobalThis() {
+    return t.callExpression(
+      t.sequenceExpression([t.valueToNode(0), t.identifier("eval")]),
+      [t.valueToNode("this")]
+    );
+  }
 };
 
 function addToMap(map, key, value) {
@@ -214,16 +255,6 @@ function addToMap(map, key, value) {
   map.get(key).push(value);
 }
 
-function hasPureArgs(path) {
-  const args = path.get("arguments");
-  for (const arg of args) {
-    if (!arg.isPure()) {
-      return false;
-    }
-  }
-  return true;
-}
-
 function isComputed(node) {
   return node.computed;
 }