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

[WIP] Google closure compiler js #5464

Closed
wants to merge 10 commits into from
Closed

[WIP] Google closure compiler js #5464

wants to merge 10 commits into from

Conversation

nazar-pc
Copy link
Contributor

@nazar-pc nazar-pc commented Aug 13, 2017

Replacing Google's closure-compiler that requires Java with closure-compiler-js that works in Node (which should be present anyway).

Things that are left to be done here:

  • Rewrite examples in verify_emscripten_environment.rst with something other than Java
  • Investigate why -s WASM=1 -s "BINARYEN_METHOD='interpret-asm2wasm'" takes enormous amount of time and fails without descriptive errors (asmjs and native-wasm methods are good, likely related to Bad compile time performance google/closure-compiler-js#24)
  • Serious testing (looks fine on the surface)

Fixes #4539
Probably obsoletes #5109
Enables #5185 (ECMASCRIPT5 needs to be changed to ECMASCRIPT6 for this, which is trivial)

@nazar-pc
Copy link
Contributor Author

OK, so compilation finishes fine, but returns bad status code (because of warnings), which is impossible to fix with arguments because of google/closure-compiler-js#73

@juj
Copy link
Collaborator

juj commented Aug 14, 2017

This PR is mostly whitespace changes. If you want to do such trailing whitespace removals, any chance you could separate those to a single commmit in a separate PR? Then reviewing this will be easier, and if someone is reading the PR later, they'll have a to the point view to the actual changes. I can also prepare such a PR if you'd like.

@nazar-pc
Copy link
Contributor Author

Sorry about that, my IDE removes those whitespaces automatically whenever the file is edited.

It is in separate commit already (with documentation tweaks). I'll separate it later, this PR is still not ready to be merged anyway, there are some warnings/errors reported by Closure Compiler that I have to fix somehow so that it stops reporting status code 1 instead of 0 (even though compilation seems totally fine).

@juj juj mentioned this pull request Aug 14, 2017
@juj
Copy link
Collaborator

juj commented Aug 14, 2017

Authored #5469, although I'm thinking perhaps it's better to leave these things out, since even if we cleaned trailing whitespaces up, they will inevitably creep back in since there are lots of authors who use different IDEs, and such changes make tracking history and changes more difficult, and git blame gets confused, and all existing PRs need rebasing. Unless we had a strict git pre-commit hook mandating trailing whitespace cleanness permanently, I think it would be better to just drop these types of mass changes. (and just clean trailing whitespaces on those lines that are also otherwise touched by your PR)

@kripken
Copy link
Member

kripken commented Aug 14, 2017

Thanks for looking into this, removing the java dependency would be great. Compilation time and memory usage is something to look at, but even if it's somewhat slower I think it's worth doing this.

@nazar-pc
Copy link
Contributor Author

asmjs and native-wasm compilation time is good. interpret-asm2wasm however takes something like 10 to 11 minutes, but I suspect it might be partially caused by warnings, so I'm trying to fix as many of them as possible.

@dschuff
Copy link
Member

dschuff commented Aug 16, 2017

👍 and double- 👍 if it means we can start using ES6+ features and still support older browsers via transpiling.

@nazar-pc
Copy link
Contributor Author

@dschuff, it does mean ES2015+ is on the horizon!

…pm dependency).

Some new externs added to please Closure Compiler.
…ences left) except for `verify_emscripten_environment.rst` file (examples in this file should be rewritten)
Fixes to externs (added File API externs).
Fix Js optimizer to properly work with added suppression comment (resulted in corrupted JS build).
More externs fixes.
@nazar-pc
Copy link
Contributor Author

nazar-pc commented Aug 20, 2017

I've fixed some compilation errors and restored some trailing spaces to reduce diff and ease reviewing.

Looks like this is ready for initial testing!

I'm not sure how interpret-asm2wasm is important, but it doesn't play well with new Closure Compiler right now and the primary issue is google/closure-compiler-js#23 (memory consumption goes through the roof and it crashes).

Can we disallow closure compiler to be used with interpret-asm2wasm until upstream issue is fixed (I'll keep an eye on it)?

Alternatively I think we can move to newer Java version of Closure Compiler first (most changes here are actually not related to JS version) and merge JS version as soon as mentioned issue is fixed.

I've executed default and asm2 tests, they look good. Some other tests fail with Allocation failed - JavaScript heap out of memory because of mentioned above issue.

UPD: Make sure to use newer Node version than is included with emsdk by default, I've just changed symlink to v8.4.0 installed globally on my system.

@kripken
Copy link
Member

kripken commented Aug 28, 2017

Can we disallow closure compiler to be used with interpret-asm2wasm until upstream issue is fixed (I'll keep an eye on it)?

Yes, the interpret modes are mainly for testing. Ok to disable closure there.

Alternatively I think we can move to newer Java version of Closure Compiler first (most changes here are actually not related to JS version) and merge JS version as soon as mentioned issue is fixed.

That sounds like a good idea - less big change in one PR is better.

@nazar-pc
Copy link
Contributor Author

Great, I will prepare another PR with updated Java version of Closure Compiler and changes necessary for it to work properly. Then will rebase this one against that new PR.

@nazar-pc
Copy link
Contributor Author

Small update on this topic: apparently latest versions of Closure Compiler on NPM support both Java and Node.js operation, which means faster compilation time when Java runtime is available and fallback to JavaScript version if not, kind of the best of 2 worlds.
I'll try to integrate it here when I have enough time.

@stale
Copy link

stale bot commented Sep 18, 2019

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 7 days. Feel free to re-open at any time if this issue is still relevant.

@stale stale bot added the wontfix label Sep 18, 2019
@dschuff
Copy link
Member

dschuff commented Sep 18, 2019

I think we should still do this at some point.

@stale stale bot removed the wontfix label Sep 18, 2019
@sbc100 sbc100 closed this Jan 30, 2020
@sbc100
Copy link
Collaborator

sbc100 commented Jan 30, 2020

I believe this was fixed in #9989

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants