Computed properties should use Object.defineProperty #357

Closed
DmitrySoshnikov opened this Issue Dec 30, 2014 · 12 comments

Projects

None yet

5 participants

@DmitrySoshnikov
Contributor

As described in this issue, computed properties should compile to Object.defineProperty instead of simple assignment expressions -- in order to correctly support ES5 inherited accessor properties.

@kittens
Member
kittens commented Dec 30, 2014

Correct me if I'm wrong but this is only an issue if there are conflicting accessors defined on Object.prototype?

@zloirock
Member

Current symbols polyfill uses setters in Object.prototype by default -> symbols will be enumerable in for-in & Object.keys.

@monsanto
Member

Right now value properties created with defineProperty are slow in Firefox: see related bug, which has been open since 2012. On the linked benchmark, I'm seeing a 30-36% slowdown.

Considering this performance hit happens at every access of the property, and not just where the computed property is defined, it would be cool if there was a 6to5 flag to not use defineProperty (assuming it is even needed in this case).

@kittens
Member
kittens commented Dec 30, 2014

I'm unsure if this is even an issue since it'll only effect existing properties on Object.prototype. It'll break symbol enumerable polyfills and will add a significant slowdown to property access in Firefox. Seems like the cons far outweigh the extremely small edgecase.

@DmitrySoshnikov
Contributor

Technically it may affect any inherited setter (not exactly form Object.prototype) -- e.g. object, created by Object.create(...). This is why I had two modes of compiling computed properties: ES3 for older browsers, and which don't need to bother about accessors, and also ES5 for correct handling of inherited accessors, since setters consider the prototype chain.

@kittens
Member
kittens commented Dec 31, 2014

Can you give an example where the current implementation will cause conflicts with inherited setters that aren't on Object.prototype?

@zloirock
Member

@sebmck example, but i don't think this a problem.

@kittens
Member
kittens commented Dec 31, 2014

Oh right, I forgot about computed properties on classes.

@DmitrySoshnikov
Contributor

Yes, classes are one of the cases. Also simple inherited objects (e.g. via __proto__). I also have simple assignments for most of the use-cases. However, if this transform aims to support full ES5, it has to be Object.defineProperty for not to break the semantics. Although, it's up to you whether you want to support it, but that's how spec defines it.

@chicoxyzzy
Contributor

👍
this needs to be fixed

@kittens
Member
kittens commented Jan 1, 2015

I'm going to add an edgecase for computed Symbol member expressions due to the limitations of symbol polyfills, otherwise properties will use Object.defineProperty.

@kittens kittens closed this in 800c350 Jan 1, 2015
@kittens
Member
kittens commented Jan 1, 2015

Thanks! Fixed as of 2.3.1

@kittens kittens was assigned by thejameskyle Jan 1, 2015
@kpdecker kpdecker added a commit to kpdecker/babel that referenced this issue Jun 25, 2015
@kpdecker kpdecker Avoid defineProperty when not needed
This lets us use the fast path for most object literal assignments and then utilizes the defineProperty path when there is a chance that we could hit the setter issue described in #357.

10x performance boosts seen for the six-speed test case, going from 200k operations/sec to 2M ops/sec.
1d83ad6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment