mishoo / UglifyJS

JavaScript parser / mangler / compressor / beautifier toolkit
http://lisperator.net/uglifyjs/
Other
13.09k stars 1.25k forks source link

`void 0` can be optimized to an unassigned variable #2585

Open KamilaBorowska opened 6 years ago

KamilaBorowska commented 6 years ago

Bug report or feature request?

Feature request

ES5 or ES6+ input?

ES5

Uglify version (uglifyjs -V)

uglify-js 3.2.2

JavaScript input

function f(){var h=prompt();return[void 0,h/h]}

The uglifyjs CLI command executed or minify() options used.

Default

JavaScript output or error produced.

function f(){var h=prompt();return[void 0,h/h]}

This can be further optimized as this:

function f(){var h=prompt(),u;return[u,h/h]}

This saves 5 bytes per each undefined usage as long at least one variable was declared in a function scope, at cost of having to use 2 extra bytes for var declaration. var declaration can be inserted in a scope above in event there is a function within a function for extra savings.

kzc commented 6 years ago

I initially thought using an uninitialized variable instead of void 0 would incur a runtime symbol lookup cost if used in a hot loop, but I don't see evidence of that in a few quick tests.

The issue is that uglify goes out of its way to eliminate unused variables in order to further simplify code. So this proposal would be counter to that.

$ echo 'function f(){var h=prompt(),u;return[u,h/h]}' | bin/uglifyjs -c
function f(){var h=prompt();return[void 0,h/h]}

$ echo 'function f(){var u;return u;}' | bin/uglifyjs -c
function f(){}

Off topic: In case anyone reading this ticket thinks that array holes are an alternative to void 0 see this article explaining why they are not equivalent.

kzc commented 6 years ago

This proposed void 0 transform could work if it were applied after the final compress pass but before mangle. I verified that it produces a detectable post-gzip improvement in some popular libraries.

The concerns:

  1. Introducing a new top level variable could pollute the global namespace.
  2. If the uninitialized void 0 alias variable is declared in an outer scope it could potentially cause any function using that captured variable to be de-optimized by a JIT engine.

These concerns could be mitigated by only introducing the uninitialized void 0 alias variable local to each function - but only if the function already had another local var declaration. If a local var were already present then even a single void 0 replacement in a given function would result in minify reductions.

If it were to be implemented it would also need a new compress option to gate it.

Skalman commented 6 years ago

I'd be interested in implementing this change, unless you think it'd be too complicated.

Would this be gated by unsafe_undefined? (though I don't understand how this could be unsafe) I'd also appreciate a pointer as to where the code should be added.

kzc commented 6 years ago

@Skalman Sure, go for it. It would be a safe transform but should be gated by some new compress option so that it can disabled if need be.

I made some notes above how I think it should be implemented - after the final compress pass and before mangle. Take care not to break harmony AST_Arrow functions - particularly with an expression body. I think it'd best to only apply the transform locally per function in order to benefit from a shorter variable name and to avoid captured variables which might slow down the code. I'd go further and suggest to only apply it to a function that happens to declare any var.

alexlamsl commented 6 years ago

node test/benchmark.js is a good way to test if your changes make for smaller (gzipped) output.

kzc commented 6 years ago

Look at make_sym in lib/compress.js as an example of how to create a local variable in an already existing var statement.

kzc commented 6 years ago

The biggest obstacle to implementing this undefined transform is to not spend excessive CPU scanning each and every function for undefined and/or void 0 use. If it is too expensive it will not be enabled by default in compress.

For harmony the undefined transform could conceivably be done for let statements as well if declaration precedes usage. const declarations without bodies are syntax errors, so that's a no go.

Skalman commented 6 years ago

(Thanks!) This transform only makes sense when mangle is enabled, so I suppose the default enabled-ness value should depend on mangle.

only apply it to a function that happens to declare any var

I agree. Further improvements could be made at a later stage.

Take care not to break harmony AST_Arrow functions

It looks like AST_Lambda is the base for all functions, so hopefully it would work automatically if I just check this.

not spend excessive CPU scanning each and every function for undefined and/or void 0 use

Yeah, this could be a problem. I believe we only need to check for void 0, since all undefined should either already have been converted to void 0 or will later be mangled.

let statements

This could be implemented at a later stage.


Is this where I'd add a call to this new compression? https://github.com/mishoo/UglifyJS2/blob/d66d86f20bc231bd8d305ee5ba05efa77aa8b6be/lib/compress.js#L201-L202

kzc commented 6 years ago

It looks like AST_Lambda is the base for all functions, so hopefully it would work automatically if I just check this.

If you're looking for an existing AST_Var that's true. But if you're introducing a new var statement then in the harmony branch you'd have to explicitly exclude blockless AST_Arrows.

I believe we only need to check for void 0

True.

If you walk the tree from AST_Toplevel and note each function you happen to be in and any AST_Var in that function then it could be done in one pass.

Yes, you'd invoke this transform at the end of Compressor.prototype.compress.

kzc commented 6 years ago

This transform only makes sense when mangle is enabled, so I suppose the default enabled-ness value should depend on mangle.

Just treat it like a normal compress option ignoring whether mangle is enabled. Other transforms like hoist_props are in the same boat.