From 23f008b7b8899fda63b8ef3ced7baa7699557c44 Mon Sep 17 00:00:00 2001 From: Vignesh Shanmugam Date: Thu, 17 May 2018 18:16:12 +0200 Subject: [PATCH 1/6] perf(builtins): run builtins transform on multi pass --- .../babel-plugin-minify-builtins/src/index.js | 30 ++++++++----------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/packages/babel-plugin-minify-builtins/src/index.js b/packages/babel-plugin-minify-builtins/src/index.js index ae2dad91f..358108df8 100644 --- a/packages/babel-plugin-minify-builtins/src/index.js +++ b/packages/babel-plugin-minify-builtins/src/index.js @@ -8,19 +8,12 @@ const INVALID_METHODS = ["random"]; module.exports = function({ types: t }) { class BuiltInReplacer { - constructor(program, { tdz }) { - this.program = program; - this.tdz = tdz; + constructor() { // map; this.pathsToUpdate = new Map(); } - run() { - this.collect(); - this.replace(); - } - - collect() { + getCollectVisitor() { const context = this; const collectVisitor = { @@ -55,7 +48,7 @@ module.exports = function({ types: t }) { }, CallExpression: { - exit(path) { + exit(path, { opts: { tdz = false } = {} }) { const callee = path.get("callee"); if (!callee.isMemberExpression()) { return; @@ -66,7 +59,7 @@ 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 }); + const result = evaluate(path, { tdz }); // deopt when we have side effecty evaluate-able arguments // Math.max(foo(), 1) --> untouched // Math.floor(1) --> 1 @@ -81,7 +74,7 @@ module.exports = function({ types: t }) { } }; - this.program.traverse(collectVisitor); + return collectVisitor; } replace() { @@ -111,14 +104,17 @@ module.exports = function({ types: t }) { } } + 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) { From 3ca48449d4a41066914f7b31fb791bcae91b7963 Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Thu, 17 May 2018 19:24:35 +0200 Subject: [PATCH 2/6] don't evaluate in minify-builtins --- .../fixtures/builtin-methods/expected.js | 4 +-- .../fixtures/evaluate-builtins/expected.js | 4 +-- .../fixtures/in-function-scope/expected.js | 2 +- .../babel-plugin-minify-builtins/package.json | 5 +-- .../babel-plugin-minify-builtins/src/index.js | 31 +++++-------------- 5 files changed, 14 insertions(+), 32 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 index 75be64a8d..3a53ba411 100644 --- 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); } \ No newline at end of file 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 index c6f36ac70..d79e7574c 100644 --- 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); \ No newline at end of file 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 index ee6ee1984..955854394 100644 --- 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/package.json b/packages/babel-plugin-minify-builtins/package.json index 59f98970b..aaa2f000b 100644 --- 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 (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 index 358108df8..22f263ee4 100644 --- a/packages/babel-plugin-minify-builtins/src/index.js +++ b/packages/babel-plugin-minify-builtins/src/index.js @@ -1,6 +1,5 @@ "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"]; @@ -48,7 +47,7 @@ module.exports = function({ types: t }) { }, CallExpression: { - exit(path, { opts: { tdz = false } = {} }) { + exit(path) { const callee = path.get("callee"); if (!callee.isMemberExpression()) { return; @@ -58,17 +57,13 @@ 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 }); - // 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); } } } @@ -210,16 +205,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; } From 63348bbab4068c695b8cca46931ff4579d04441b Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Thu, 17 May 2018 20:20:49 +0200 Subject: [PATCH 3/6] fix cases where Math is already found in scope --- .../babel-plugin-minify-builtins/src/index.js | 40 ++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/packages/babel-plugin-minify-builtins/src/index.js b/packages/babel-plugin-minify-builtins/src/index.js index 22f263ee4..ddcd61193 100644 --- a/packages/babel-plugin-minify-builtins/src/index.js +++ b/packages/babel-plugin-minify-builtins/src/index.js @@ -5,6 +5,8 @@ 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() { @@ -92,8 +94,37 @@ 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"); + + 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)) { + throw new Error( + `Unexpected MemberExpression in minify-builtins. ` + + `Please report this at ${newIssueUrl}` + ); + } + + newNode.declarations[0].init = t.memberExpression( + t.memberExpression(getGlobalThis(), prev.object), + prev.property + ); + } + } + + target.unshiftContainer("body", newNode); } } } @@ -196,6 +227,13 @@ module.exports = function({ types: t }) { } } } + + function getGlobalThis() { + return t.callExpression( + t.sequenceExpression([t.valueToNode(0), t.identifier("eval")]), + [t.valueToNode("this")] + ); + } }; function addToMap(map, key, value) { From c79e3faa2d2221b875942cbcdbda91a093ef6c35 Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Thu, 17 May 2018 20:23:21 +0200 Subject: [PATCH 4/6] add comments --- .../babel-plugin-minify-builtins/src/index.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/packages/babel-plugin-minify-builtins/src/index.js b/packages/babel-plugin-minify-builtins/src/index.js index ddcd61193..a2a033ed7 100644 --- a/packages/babel-plugin-minify-builtins/src/index.js +++ b/packages/babel-plugin-minify-builtins/src/index.js @@ -98,6 +98,18 @@ module.exports = function({ types: t }) { // hoist the created var to the top of the function scope 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; @@ -228,6 +240,11 @@ module.exports = function({ types: t }) { } } + /** + * returns + * + * (0, eval)("this") + */ function getGlobalThis() { return t.callExpression( t.sequenceExpression([t.valueToNode(0), t.identifier("eval")]), From b9ce3d34a5c58a0949821aa6fb285e9f8cdbc9de Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Thu, 17 May 2018 20:25:03 +0200 Subject: [PATCH 5/6] add test --- .../__tests__/fixtures/local-binding/actual.js | 18 ++++++++++++++++++ .../fixtures/local-binding/expected.js | 16 ++++++++++++++++ 2 files changed, 34 insertions(+) create mode 100644 packages/babel-plugin-minify-builtins/__tests__/fixtures/local-binding/actual.js create mode 100644 packages/babel-plugin-minify-builtins/__tests__/fixtures/local-binding/expected.js 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 new file mode 100644 index 000000000..11a3a0f48 --- /dev/null +++ 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 new file mode 100644 index 000000000..a08805883 --- /dev/null +++ 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 + }; +} \ No newline at end of file From 06f01bc460f96fbb58dc49ceee81d34e27615166 Mon Sep 17 00:00:00 2001 From: Boopathi Rajaa Date: Thu, 17 May 2018 20:36:59 +0200 Subject: [PATCH 6/6] bail out for already transformed ones --- packages/babel-plugin-minify-builtins/src/index.js | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/packages/babel-plugin-minify-builtins/src/index.js b/packages/babel-plugin-minify-builtins/src/index.js index a2a033ed7..d9da20ca4 100644 --- a/packages/babel-plugin-minify-builtins/src/index.js +++ b/packages/babel-plugin-minify-builtins/src/index.js @@ -122,17 +122,12 @@ module.exports = function({ types: t }) { ); } - if (t.isMemberExpression(prev.object)) { - throw new Error( - `Unexpected MemberExpression in minify-builtins. ` + - `Please report this at ${newIssueUrl}` + if (!t.isMemberExpression(prev.object)) { + newNode.declarations[0].init = t.memberExpression( + t.memberExpression(getGlobalThis(), prev.object), + prev.property ); } - - newNode.declarations[0].init = t.memberExpression( - t.memberExpression(getGlobalThis(), prev.object), - prev.property - ); } }