-
-
Notifications
You must be signed in to change notification settings - Fork 225
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implemented transform-remove-undefined plugin. #197
Conversation
name: "transform-void-to-nothing", | ||
visitor: { | ||
VariableDeclaration({node: {kind: kind, declarations: declarations}}) { | ||
if (kind !== "const") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: maybe early return to avoid already heavy nesting?
const v0 = require("../../babel-helper-is-void-0/src"); | ||
|
||
module.exports = function({ types: t }) { | ||
// "var a = void 0;" -> "var a;" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not safe in loops FYI. e.g.
for (var i = 0; i < 2; i++){
var a = void 0;
console.log(a);
a = i;
}
should print
undefined
undefined
not
undefined
0
VariableDeclaration({node: {kind: kind, declarations: declarations}}) { | ||
if (kind !== "const") { | ||
for (const declaration of declarations) { | ||
if (isVoid0(declaration.init)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should just check for undefined
value. void 0
is just one representation of that value so no need to limit ourselves here (or expect other plugins to kick in before).
I think we should also account for other places, not just declarations. return undefined
is currently not transformed to just return
.
There's also call expressions foo(x)
where we can confidently determine that x
is undefined
. The only issue here is that removing argument has a side-effect — arguments.length is no longer the same. That would have to be marked as an unsafe transform I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See inline
c8da4f4
to
2e62acf
Compare
I updated the plugin to capture simplify all
This is so that the Also simplified function return values iff they evaluate to |
Removes the 'void 0' rval for let-/var-assignments.
2e62acf
to
2509f7c
Compare
LGTM but /cc'ing @hzoo @loganfsmyth @danez for sanity check |
}, | ||
Identifier(path) { | ||
// potential optimization: only store ids that are lvals of assignments. | ||
seenIds.add(path.node.name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it also consider this as seen ?
let a = 1;
{
let a = undefined;
}
?
}); | ||
|
||
it("should remove undefined return value", () => { | ||
const source = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...
...
const source = unpad(`
...
`);
This is how all other tests are aligned.
return seenIds.has(id.node.name); | ||
} | ||
let hasReference = false; | ||
id.traverse({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for non simple declarations, you have getBindingIdentifiers()
to get the binding identifiers. .traverse
is probably not the best idea.
name: "remove-undefined-if-possible", | ||
visitor: { | ||
FunctionParent(path) { | ||
removeUndefinedAssignments(path, t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this creates 2 more levels of traverse for some paths, can you run this with scripts/plugin-timing
if this is taking too much time ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ran the timing script after adding this to the preset list:
for file in ./scripts/fixtures/*.js; do
echo $file
./scripts/plugin-timing.js $file
done
./scripts/fixtures/backbone.js
pluginAlias time(ms) # visits time/visit(ms)
minify-simplify 174.445 5669 0.031
minify-mangle-names 88.821 1 88.821
minify-constant-folding 71.037 12515 0.006
remove-undefined-if-possible 67.165 459 0.146
minify-dead-code-elimination 66.802 1 66.802
minify-guarded-expressions 18.205 808 0.023
internal.shadowFunctions 12.601 6406 0.002
foreign$14 11.361 265 0.043
minify-type-constructors 10.955 822 0.013
minify-infinity 7.723 5453 0.001
foreign$15 6.950 5453 0.001
foreign$10 6.535 2024 0.003
foreign$12 3.333 738 0.005
foreign$11 1.797 209 0.009
minify-flip-comparisons 1.457 265 0.005
foreign$13 0.953 236 0.004
internal.blockHoist 0.776 288 0.003
minify-replace 0.153 1 0.153
./scripts/fixtures/flot.js
pluginAlias time(ms) # visits time/visit(ms)
minify-dead-code-elimination 119.034 1 119.034
minify-simplify 89.467 1182 0.076
minify-constant-folding 47.764 6162 0.008
minify-mangle-names 32.701 1 32.701
remove-undefined-if-possible 23.108 54 0.428
minify-guarded-expressions 8.493 180 0.047
internal.shadowFunctions 6.262 2713 0.002
foreign$15 3.823 2713 0.001
minify-infinity 3.684 2713 0.001
minify-type-constructors 2.568 315 0.008
foreign$12 2.458 504 0.005
foreign$10 2.278 1130 0.002
minify-flip-comparisons 1.714 461 0.004
foreign$14 0.819 461 0.002
foreign$11 0.606 61 0.010
foreign$13 0.363 113 0.003
internal.blockHoist 0.278 50 0.006
minify-replace 0.147 1 0.147
./scripts/fixtures/jquery.js
pluginAlias time(ms) # visits time/visit(ms)
minify-simplify 2379.277 54019 0.044
minify-constant-folding 687.636 124589 0.006
minify-mangle-names 645.566 1 645.566
remove-undefined-if-possible 486.537 3746 0.130
minify-guarded-expressions 218.809 11556 0.019
minify-dead-code-elimination 193.202 1 193.202
internal.shadowFunctions 89.802 58084 0.002
minify-infinity 88.526 56758 0.002
foreign$15 87.342 56758 0.002
minify-type-constructors 68.566 7615 0.009
foreign$14 42.466 4852 0.009
foreign$10 29.695 17269 0.002
foreign$12 19.914 9339 0.002
minify-flip-comparisons 18.298 4852 0.004
internal.blockHoist 5.210 2495 0.002
foreign$13 3.870 1336 0.003
foreign$11 3.486 877 0.004
minify-replace 0.298 1 0.298
./scripts/fixtures/react.js
pluginAlias time(ms) # visits time/visit(ms)
minify-simplify 747.820 31213 0.024
minify-mangle-names 480.056 1 480.056
minify-constant-folding 422.067 65514 0.006
minify-dead-code-elimination 410.427 1 410.427
remove-undefined-if-possible 300.603 2157 0.139
minify-guarded-expressions 83.892 3534 0.024
internal.shadowFunctions 52.667 31267 0.002
minify-infinity 48.516 30538 0.002
foreign$15 47.769 30538 0.002
minify-type-constructors 39.374 4199 0.009
foreign$14 33.705 2254 0.015
foreign$12 26.349 7659 0.003
foreign$10 13.478 7812 0.002
foreign$11 8.993 1240 0.007
minify-flip-comparisons 7.981 2254 0.004
foreign$13 7.097 2816 0.003
internal.blockHoist 3.541 1510 0.002
minify-replace 0.151 1 0.151
break; | ||
} | ||
path.node.declarations.forEach((declarator, index) => { | ||
const declaratorPath = path.get("declarations")[index]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as previous.
Since you need declarationPath, you can simply do const declarations = path.get("declarations")
and just iterate over it, each item will be a declarationPath
case "const": | ||
break; | ||
case "let": | ||
path.node.declarations.forEach((declarator, index) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you need declarationPath, you can simply do const declarations = path.get("declarations")
and just iterate over it, each item will be a declarationPath
Shouldn't this be a part of DCE ? Is it required to be a separate plugin ? |
I guess it could be part of DCE but I don't know where we want to draw the line between stuffing everything of this nature there or making it more granular. |
- used t.getBindingIdentifiers - changed from forEach to for-loop - added tests
486ms on jQuery... yeah, I would say this is a bit too much. We're likely doing unnecessary work. I wonder if merging this into DCE could allow us to utilize DCE's existing traversal/analysis so that we don't have to repeat it all again in another plugin? /cc @boopathi |
@shinew could you please look into optimizing this? Either by reducing traversal or by integrating into DCE. |
I'm not sure if adding how much benefit we get by merging this in DCE. But should try that too - as DCE is a separate and a single pass. I think you can avoid |
@shinew how do the benchmark numbers look now? |
|
I'm not sure |
I think I just realized a mistake in the capturing that makes optimizing function foo() {
(() => {
t = 0;
})();
var t = undefined;
} In the above case, If we just drop support for |
|
Time looks much better. @boopathi anything else before we merge it? |
I'll go through and try it out today. Can we have the name changed to
? |
I like |
…nsform-remove-undefined.
function removeRvalIfUndefined(declaratorPath) { | ||
const rval = declaratorPath.get("init") | ||
.evaluate(); | ||
if (rval.confident === true && rval.value === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs to be pure.
var x = void console.log("hi");
var y = void foo();
shouldn't be evaluated to undefined and be removed.
|
||
function removeRvalIfUndefined(declaratorPath) { | ||
const rval = declaratorPath.get("init") | ||
.evaluate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you push evaluate to the same line. It's not really long to be added to a separate line.
ReturnStatement(path) { | ||
if (path.node.argument !== null) { | ||
const rval = path.get("argument") | ||
.evaluate(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as above. Can you push evaluate to the same line as previous one?
if (path.node.argument !== null) { | ||
const rval = path.get("argument") | ||
.evaluate(); | ||
if (rval.confident === true && rval.value === undefined) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably requires isPure
check here too for
return void console.log("asdf");
var { a: aa, b: bb } = undefined; | ||
}`); | ||
expect(transform(source)).toBe(source); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
test cases for
void foo()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't convert
function foo() {
var x = void 0;
function bar() {
var x = void 0;
function baz() {
var x = void 0;
}
}
}
functionNesting++; | ||
}, | ||
exit() { | ||
functionNesting--; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose you're using this only to check global ? you can use this information to check if different nested function scopes have same variable name and then solve -
function foo() {
var x = void 0;
function bar() {
var x = void 0; // this is not removed now
}
}
Added a few comments inline and those are outdated due to rename. |
Quick question: when I call |
@shinew http://astexplorer.net/#/fT7AjgoWqp . That is indeed weird. Not sure. @hzoo bug in babel or am I missing something ? |
@shinew I just edited your comments with benchmarks to details/summary view. |
Good points. There's also a concern for a case like: function foo() {
bar();
var x = undefined;
console.log(x);
function bar() {
x = 3;
}
} In which case we don't want to remove |
There's no way we can visit every identifier. Instead, figure out which constant violations hamper the minification. Oh, and some test cases. Those should probably be cleaned up. 😁
Track using violations
- purity-check of rvals - added unit tests - refactoring
It's definitely looking better. Thanks @shinew & @jridgewell . I'll try it out ... |
|
@boopathi Did you have a chance to look at it? Anything else we need? |
Not yet ... It looks good. will try it locally. |
expect(transform(source)).toBe(expected); | ||
}); | ||
|
||
it("should remove ...", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be more descriptive? or just make this instead of the previous test?
Removes the 'void 0' rval for let-/var-assignments.