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

Redundant unary '+' operators not merged #504

Open
chadaustin opened this issue Jun 26, 2014 · 6 comments
Open

Redundant unary '+' operators not merged #504

chadaustin opened this issue Jun 26, 2014 · 6 comments

Comments

@chadaustin
Copy link

asm.js, from a strictly JavaScript perspective, produces a lot of redundant operators. For example, a function that takes a double and returns the same double would be expressed as:

function f(x) {
  x = +x; // tells asm.js that the argument is a double
  return +x;
}

Closure Compiler minifies that into:

function f(a){return+ +a}

Of course, the double + operator isn't necessary.

@chadaustin
Copy link
Author

I'm happy to work on a patch if someone wouldn't mind giving me a sense of where to start.

@MatrixFrog
Copy link
Contributor

I can't think of a case where +x would be a different value than +(+x) so I think you're right, this is an opportunity to save a couple bytes.

However, I'm not sure why you'd want to use the Closure optimizations on asm.js code. If the x = +x is an important hint for asm.js then running your code through the Closure Compiler would remove that hint and the asm.js runner wouldn't be able to do its thing, right? Unless I'm misunderstanding how asm.js works, which is quite possible.

@ChadKillingsworth
Copy link
Collaborator

In addition, I'm dubious that this would either:

a. make a meaningful size difference in real world code
b. occur frequently enough in real world code.

In which case, is it worth the extra time or processing to handle this? See the required justifications at https://github.com/google/closure-compiler/wiki/FAQ#how-do-i-submit-a-feature-request-for-a-new-type-of-optimization

@chadaustin
Copy link
Author

Hi @MatrixFrog and @ChadKillingsworth. You're right, I ought to provide some context.

At IMVU, we are building a 3D engine to render our content on multiple platforms, and, for the web, we compile it with Emscripten and then use Closure to minify.

Emscripten has two compilers: the legacy one which has a couple different code generation modes, and the new 'fastcomp' compiler which only outputs JavaScript in asm.js-like syntax. Unfortunately, we can't use asm.js itself, as asm.js does not yet support growing the heap. To support our use case, the new 'fastcomp' compiler has an "almost asm" mode which DOES allow growing the heap, but otherwise the syntax reads like asm.js with (excessive, you could argue) type hints.

It's true that running Closure on asm.js makes it not asm.js anymore :) but that's fine for our use case. We care more about download size and the time it takes for the browser to parse the JavaScript.

OK, I hope that's enough context!

@ChadKillingsworth I fully understand that this optimization would not affect typical programs, but because Emscripten compiles to "almost asm" and then runs Closure across it, redundant operators show up quite frequently. ++x, x|0|0, and x>>0>>0 are quite common. I would be happy to have this optimization as an optional pass that does not affect compilation times for typical usage.

Does that help?

Thanks!
Chad

@ChadKillingsworth
Copy link
Collaborator

@chadaustin It does indeed.

This can probably be done as a peephole optimization. See https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/PeepholeSubstituteAlternateSyntax.java for a starting point (you might just be able to add it to that file).

Feel free to ask questions. Also, make sure you reference, add to and run the associated unit tests.

@concavelenz
Copy link
Contributor

I expect these will need to be handled on a case by case basis for these "type hint" operators but they are simple enough to add if someone wants to contribute patches.

@MatrixFrog MatrixFrog changed the title Redundant + operators not merged Redundant unary '+' operators not merged Jul 31, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants