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

extend escape analysis on constant expression properties #2509

Merged
merged 2 commits into from
Nov 24, 2017

Conversation

alexlamsl
Copy link
Collaborator

fixes #2508

@kzc
Copy link
Contributor

kzc commented Nov 24, 2017

Any idea about this one?

$ echo 'var o = { f: function(x){console.log(x)} }; console.log(o.f);' | bin/uglifyjs --toplevel -mc hoist_props,passes=9
var o={f:function(o){console.log(o)}};console.log(o.f);

@alexlamsl
Copy link
Collaborator Author

Any idea about this one?

console in console.log(x) breaking the is_constant_expression() assumption - I can put in the scope paramter to make it pass.

@kzc
Copy link
Contributor

kzc commented Nov 24, 2017

Okay, how about this?

$ echo 'var o = { f: function(x){console.log(x)} }; o.f(o.f);' | bin/uglifyjs --toplevel -mc hoist_props,passes=9
var f={f:function(f){console.log(f)}};f.f(f.f);

@alexlamsl
Copy link
Collaborator Author

Okay, how about this?

Exact same cause.

@alexlamsl
Copy link
Collaborator Author

#2509 (comment) & #2509 (comment) addressed in 9ddb75c

@kzc
Copy link
Contributor

kzc commented Nov 24, 2017

Good to see you like a challenge.

This PR should bake in the fuzzer for a little bit longer than normal I think.

@alexlamsl
Copy link
Collaborator Author

I'll get this to 100kFuzz, merge, then leave the whole master soaking as soon as the weekend starts.

@alexlamsl
Copy link
Collaborator Author

node test/benchmark.js gives identical results between master and #2509 for -mc and --toplevel -mc keep_fargs=0,unsafe,pure_getters,passes=10000

@kzc
Copy link
Contributor

kzc commented Nov 24, 2017

So no optimization opportunities in the benchmark code. This PR will be useful in scope hoisting bundler code.

@alexlamsl
Copy link
Collaborator Author

alexlamsl commented Nov 24, 2017

Do you have a representative example that is publicly available on the web?

We can expand test/benchmark.js a bit.

@alexlamsl alexlamsl merged commit 3b28b91 into mishoo:master Nov 24, 2017
@alexlamsl alexlamsl deleted the issue-2508 branch November 24, 2017 06:07
@kzc
Copy link
Contributor

kzc commented Nov 24, 2017

Not really, just Rollup test cases - and Rollup itself - it's self-hosted. Mind you uglify-es would have to be used because it's ES6. Once converted to ES5 it complicates the code preventing some optimizations. I'll keep a look out for such code to add to the benchmark.

@kzc
Copy link
Contributor

kzc commented Nov 24, 2017

Here's a Rollup source file with exports that ought to benefit once imported, scope hoisted and bundled now that arrows will work with hoist_props:

https://github.com/rollup/rollup/blob/v0.51.8/src/ast/values.js

Once this PR is merged into harmony I can test that hypothesis on the Rollup ES6 distro bundle.

@kzc
Copy link
Contributor

kzc commented Nov 24, 2017

Unfortunately upon examining the Rollup source code those particular exports are not used in a manner that will benefit from this optimization.

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.

hoist_props does not work with object/array literals
2 participants