Skip to content
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

void 0 can be optimized to an unassigned variable #2585

Open
KamilaBorowska opened this issue Dec 13, 2017 · 10 comments · May be fixed by #2912
Open

void 0 can be optimized to an unassigned variable #2585

KamilaBorowska opened this issue Dec 13, 2017 · 10 comments · May be fixed by #2912

Comments

@KamilaBorowska
Copy link

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
Copy link
Contributor

kzc commented Dec 13, 2017

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
Copy link
Contributor

kzc commented Dec 13, 2017

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
Copy link
Contributor

Skalman commented Feb 7, 2018

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
Copy link
Contributor

kzc commented Feb 7, 2018

@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
Copy link
Collaborator

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

@kzc
Copy link
Contributor

kzc commented Feb 7, 2018

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
Copy link
Contributor

kzc commented Feb 7, 2018

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
Copy link
Contributor

Skalman commented Feb 7, 2018

(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
Copy link
Contributor

kzc commented Feb 7, 2018

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
Copy link
Contributor

kzc commented Feb 7, 2018

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.

Skalman added a commit to Skalman/UglifyJS2 that referenced this issue Feb 12, 2018
@Skalman Skalman linked a pull request Feb 12, 2018 that will close this issue
alexlamsl added a commit to alexlamsl/UglifyJS that referenced this issue Feb 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants