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

Use ESTree-based optimizer for core JS passes, to support ES6+ inputs #7973

Merged
merged 96 commits into from
Feb 8, 2019

Conversation

kripken
Copy link
Member

@kripken kripken commented Feb 1, 2019

Fixes #6000

The key change here is to rewrite the JS optimizer passes that run in a normal -O3 etc. build from the Uglify1 AST to ESTree. With ESTree we can use modern parsers etc. so that we support ES6+ inputs to js libraries, pre-jses, EM_ASM, etc.

Aside from that rewrite, the other changes are less critical and can be altered later. Specifically, this uses acorn for parsing and terser for outputting, but we could switch to anything using ESTree very easily. Acorn is nice for parsing since it's small and standalone. For outputting I experimented with astring, which is small and nice, and escodegen, which looks very robust, but neither could output compact-enough JS to not regress our JS code sizes. This is not truly critical since for minimal code size people should use closure anyhow, however, it's nice for default builds to be small (and we don't run closure by default), and I didn't want to regress anything. Using the terser outputter achieves that. (Since it uses the Uglify2 AST internally, this means using their tool to convert ESTree to Uglify2.) They may be some minor code size changes with this PR, just because we use a different outputter now, but nothing major in either direction. Most changes seem positive actually. Sizes after closure are unchanged.

This uses almost unmodified versions of acorn and terser, but they are stripped down to what we need, and I had to make two modifications, see these PRs: acornjs/acorn#793 (quote the error on parse exceptions) and mishoo/UglifyJS#3323 (preserve quoted properties).

This may very slightly regress compile times when using those passes, as Uglify1 was just very fast. However, the change should be very small.

This does not rewrite every single JS optimizer pass. In particular the asm.js passes don't need to support ES6, and so don't need to be rewritten. There are also optional passes that do not run by default, that we can convert later depending on priority.

@kripken
Copy link
Member Author

kripken commented Feb 6, 2019

@juj - this PR has broken after landing your minimal runtime PR. I fixed the merge conflict and then got the js optimizer tests passing (I had to port your change to the JS optimizer passes to the replacement estree optimizer), but now I get errors on e.g. binaryen3.test_minimal_runtime_global_initializer which I don't understand - can you please take a look?

For example I get var asm = Module["asm"]; in the optimized output, which should be the same across optimizers, but causes a failure.

Other stuff seems broken too, so perhaps the minimal runtime option depends on name minification in ways not properly covered by the js optimizer tests?

@juj
Copy link
Collaborator

juj commented Feb 6, 2019

Added a test in this commit: c71e6b1

and adapted that test to pass in acorn optimizer in this commit: 720134c

The difference here is that in MINIMAL_RUNTIME, global initializers are not exported out to Module or local scope, but they are executed off of the asm object directly, saving space.

@juj
Copy link
Collaborator

juj commented Feb 6, 2019

Ops, the second commit slipped in removal of the other test cases to iterate locally (but you get the point)

@juj
Copy link
Collaborator

juj commented Feb 6, 2019

In particular, it is turning this line c71e6b1#diff-2b8a804b0a6dc702567945ea539b7334R224 to this line c71e6b1#diff-e2817474c1db66fbb3fd9c20e91ab13dR198 , which changed in JS optimizer between trunk incoming and when MINIMAL_RUNTIME landed.

@kripken
Copy link
Member Author

kripken commented Feb 6, 2019

Great, thanks @juj!

@kripken
Copy link
Member Author

kripken commented Feb 8, 2019

Looks like there are no concerns, I'll land this later today if so.

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.

2 participants