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

Optimize class properties output #6656

Merged
merged 1 commit into from Apr 15, 2018

Conversation

Andarist
Copy link
Member

@Andarist Andarist commented Nov 1, 2017

Q                       A
Fixed Issues? n/a
Patch: Bug Fix? no
Major: Breaking Change? no
Minor: New Feature? no
Tests Added + Pass? Yes
Documentation PR no
Any Dependency Changes? no

Similar to change in #6652 . Additionally I switch to using defineProperty helper instead of using bare Object.defineProperty call - which should have benefit of not having to hit slow Object.defineProperty path in most cases.

@Andarist Andarist added area: perf PR: Polish 💅 A type of pull request used for our changelog categories labels Nov 1, 2017
@babel-bot
Copy link
Collaborator

babel-bot commented Nov 1, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/7606/

@babel babel deleted a comment from babel-bot Nov 1, 2017
@nicolo-ribaudo
Copy link
Member

Using the defineProperty helper looks good, but I don't really like the nested defineProperty calls.

_defineProperty(_defineProperty(this, "foo", 1), "bar", 2);
_defineProperty(this, "foo", 1);
_defineProperty(this, "bar", 2);

The second is objectively longer after minification (4 bytes), but I think it is much easier to read.

@Andarist
Copy link
Member Author

Andarist commented Nov 1, 2017

The second is objectively longer after minification (4 bytes), but I think it is much easier to read.

That is a valid concern, was thinking about it too, but ain't sure if we should optimize compiled output for readability.

I've implemented this defineProperty calls folding as little experiment, because I've noticed it's easier for a minifier (at least UglifyJS) to detect single call's result as unused. Assignments to objects (static properties in this case) and calling functions (defineProperty here) with that object effectively prevent dead code elimination of the class in most cases - that's also probably true when considering imperfect algorithm for dead code elimination, but we could do a little bit to help it here.

Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for delay in review, 👍.

FWIW, I don't have a terribly strong opinion re: folding vs readability, but I tend to favor not optimizing for readability.

@jridgewell jridgewell merged commit 5166eef into babel:master Apr 15, 2018
jridgewell added a commit to jridgewell/babel that referenced this pull request Apr 26, 2018
This undoes the property call folding from babel#6656.

It complicates the private property transforms, since they boil down to `map.set(this, vlaue)` and we definitely don't want the next call define a property on the map.

/cc @Andarist
@Andarist Andarist deleted the polish/class-properties branch April 26, 2018 08:37
existentialism pushed a commit that referenced this pull request Apr 26, 2018
This undoes the property call folding from #6656.

It complicates the private property transforms, since they boil down to `map.set(this, vlaue)` and we definitely don't want the next call define a property on the map.

/cc @Andarist
@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: perf outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Polish 💅 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants