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

Question: can UglifyJS optimise order of dynamic enumerable property assignment? #2942

Closed
JobLeonard opened this issue Feb 21, 2018 · 9 comments

Comments

@JobLeonard
Copy link

JobLeonard commented Feb 21, 2018

Sorry about the dense title, it's actually very simple to explain by example. In JavaScript, "object keys are iterated in insertion order (with a bizarre exception for arrays)"[0][1]. That results in JS engines treating x below as type-unstable¹, and because of that they are forced to generate less efficient code:

var x = {};
if (..) {
  x.a = "hello";
  x.b = "world";
} else {
  x.b = "world";
  x.a = "hello";
}

In the linked presentation (from 2015), walking a splay tree was 15% faster after swapping the assignment order, so perf regressions can be quite big!

Based on the discussion in #75 I guess that UglifyJS can't do this currently, since it requires DFA.

EDIT: I see now that the discussion in that thread is years out of date, sorry! I guess there currently is some form of DFA already, given that reduce_vars, collapse_vars and hoist_vars exist?

Still, it should be a little bit easier to determine when it is safe to change x.b = E1; x.a = E2 to x.a = E2; x.b = E1, than it would be to change var x = E, y = E; to var x = E, y = x;, pure functions don't have to return the same results (you can safely use Math.random() for E1 and E1, but not for E).

For anyone curious, I fell down this rabbit hole through the JITProf repository[2]. It's quite an interesting project but sadly it looks like it stalled.

¹ I borrowed the Julia concept "type stability", since I think it's more accessible than "x is polymorphic, even though it should be monomorphic and have only one hidden class".

[0] http://mp.binaervarianz.de/fse2015_slides.pdf

[1] https://news.ycombinator.com/item?id=16416144

[2] https://github.com/Berkeley-Correctness-Group/JITProf

@alexlamsl
Copy link
Collaborator

I think you may find enhancing hoist_props as a possible route to achieve your goal.

Pull request is welcomed.

@JobLeonard
Copy link
Author

I suspect I could only wish I had the required knowledge base to pull that off, but who knows? I'll have a look. Sounds like a rabbit hole full of edge-cases though...

@kzc
Copy link
Contributor

kzc commented Feb 21, 2018

Changing the order of property creation would break code relying on iteration order. If this proposal is implemented it would have to be disabled by default.

$ echo 'var x={}; x.b=1; x.a=1; for(var k in x) console.log(k);' | node
b
a

$ echo 'var x={}; x.a=1; x.b=1; for(var k in x) console.log(k);' | node
a
b

@alexlamsl
Copy link
Collaborator

I am expecting this feature to follow the same restrictions as hoist_props, i.e. no direct access to the underlying object. I don't think we should be altering any ordering if the object can be iterated.

@kzc
Copy link
Contributor

kzc commented Feb 21, 2018

The hoist_props restriction not to use the object by reference would essentially eliminate the usefulness of this transform in real life code. Even with the same restrictions as hoist_props one would have to respect (or alter) the order of properties within object literals in other code paths in order to speed up code.

In any case, this proposal is out of scope for uglify - it does not reduce code size and would slow down minification.

@JobLeonard
Copy link
Author

@kzc: oh, it definitely would be an unsafe feature. I should have been explicit about that in my first message, sorry. And I understand the scope-argument. Should I close the issue?

@kzc
Copy link
Contributor

kzc commented Feb 21, 2018

If it's behind an unsafe_prop_order flag and disabled by default - go for it.

@alexlamsl
Copy link
Collaborator

I'm expecting the rearrangement would facilitate collapse_vars or others to further minimised the output. If it doesn't, then I agree this feature is out of scope for uglify-js - we care about the output size and not runtime performance (we did make a mistake and ended up with reduce_funcs, but that's exception rather than the rule).

JavaScript runtime engines are constantly adjusting and improving to optimise against existing code in the wild anyway.

@kzc
Copy link
Contributor

kzc commented Feb 21, 2018

the rearrangement would facilitate collapse_vars or others to further minimised the output

That would be a different transform than the one proposed with a different goal. Assuming hoist_props rules would be in effect for object references it would not improve runtime performance of minified code but potentially could aid collapse_vars if RHS side-effect free property assignments are placed after ones with side effects.

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

No branches or pull requests

3 participants