add support for property descriptors #38

bryanaka · 2016-06-24T21:12:33Z

closes #29

Node Benchmarks: (couldn't get browser ones to work)

Before:

core-object/create (0)  x 1,179,436 ops/sec ±1.03% (84 runs sampled)
core-object/create (1)  x 728,570 ops/sec ±3.19% (79 runs sampled)
core-object/create (5)  x 791,077 ops/sec ±1.68% (76 runs sampled)
core-object/create (default init) (0)  x 12,368,014 ops/sec ±2.75% (80 runs sampled)
core-object/create (default init) (1)  x 2,472,344 ops/sec ±1.45% (76 runs sampled)
core-object/create (default init) (5)  x 491,107 ops/sec ±1.17% (82 runs sampled)
uberproto/create (0)  x 2,525,956 ops/sec ±1.15% (81 runs sampled)
uberproto/create (1)  x 2,528,991 ops/sec ±1.17% (82 runs sampled)
uberproto/create (5)  x 2,337,608 ops/sec ±2.90% (78 runs sampled)
raw/create (0)  x 20,691,590 ops/sec ±1.97% (78 runs sampled)
raw/create (1)  x 25,702,769 ops/sec ±1.73% (77 runs sampled)
raw/create (5)  x 21,648,817 ops/sec ±1.62% (75 runs sampled)
esnext/create (0)  x 18,154,432 ops/sec ±2.68% (74 runs sampled)
esnext/create (1)  x 22,432,550 ops/sec ±2.52% (75 runs sampled)
esnext/create (5)  x 19,709,753 ops/sec ±1.95% (77 runs sampled)
klass/create (0)  x 5,085,804 ops/sec ±1.84% (77 runs sampled)
klass/create (1)  x 4,942,793 ops/sec ±1.64% (79 runs sampled)
klass/create (5)  x 4,940,045 ops/sec ±1.64% (79 runs sampled)
backbone-metal/create (0)  x 4,740,320 ops/sec ±7.70% (63 runs sampled)
backbone-metal/create (1)  x 2,288,825 ops/sec ±2.54% (81 runs sampled)
backbone-metal/create (5)  x 2,237,392 ops/sec ±2.39% (76 runs sampled)
core-object/extend (1)  x 1,157,571 ops/sec ±1.61% (78 runs sampled)
core-object/extend (5)  x 371,967 ops/sec ±1.89% (79 runs sampled)
uberproto/extend (1)  x 170,055 ops/sec ±1.21% (76 runs sampled)
uberproto/extend (5)  x 53,705 ops/sec ±1.54% (78 runs sampled)
klass/extend (1)  x 181,904 ops/sec ±5.50% (58 runs sampled)
klass/extend (5)  x 91,839 ops/sec ±5.21% (66 runs sampled)
backbone-metal/extend (1)  x 145,464 ops/sec ±3.24% (71 runs sampled)
backbone-metal/extend (5)  x 74,263 ops/sec ±3.25% (71 runs sampled)

After:

core-object/create (0)  x 1,128,621 ops/sec ±1.12% (82 runs sampled)
core-object/create (1)  x 751,751 ops/sec ±1.87% (84 runs sampled)
core-object/create (5)  x 730,235 ops/sec ±1.23% (84 runs sampled)
core-object/create (default init) (0)  x 12,399,318 ops/sec ±1.10% (84 runs sampled)
core-object/create (default init) (1)  x 2,431,482 ops/sec ±1.62% (80 runs sampled)
core-object/create (default init) (5)  x 511,831 ops/sec ±1.21% (82 runs sampled)
uberproto/create (0)  x 2,486,342 ops/sec ±1.15% (83 runs sampled)
uberproto/create (1)  x 2,495,017 ops/sec ±1.22% (79 runs sampled)
uberproto/create (5)  x 2,382,672 ops/sec ±2.45% (79 runs sampled)
raw/create (0)  x 22,135,087 ops/sec ±1.55% (82 runs sampled)
raw/create (1)  x 26,974,420 ops/sec ±1.66% (77 runs sampled)
raw/create (5)  x 23,151,079 ops/sec ±1.56% (80 runs sampled)
esnext/create (0)  x 19,373,088 ops/sec ±2.28% (71 runs sampled)
esnext/create (1)  x 23,830,007 ops/sec ±1.62% (81 runs sampled)
esnext/create (5)  x 21,069,224 ops/sec ±1.74% (77 runs sampled)
klass/create (0)  x 5,189,756 ops/sec ±1.65% (81 runs sampled)
klass/create (1)  x 5,106,455 ops/sec ±1.65% (77 runs sampled)
klass/create (5)  x 5,170,719 ops/sec ±1.46% (63 runs sampled)
backbone-metal/create (0)  x 4,966,532 ops/sec ±6.08% (72 runs sampled)
backbone-metal/create (1)  x 2,359,407 ops/sec ±2.60% (79 runs sampled)
backbone-metal/create (5)  x 2,336,906 ops/sec ±2.28% (81 runs sampled)
core-object/extend (1)  x 434,122 ops/sec ±1.14% (85 runs sampled)
core-object/extend (5)  x 123,252 ops/sec ±1.40% (82 runs sampled)
uberproto/extend (1)  x 170,180 ops/sec ±1.95% (79 runs sampled)
uberproto/extend (5)  x 52,722 ops/sec ±1.42% (78 runs sampled)
klass/extend (1)  x 184,676 ops/sec ±4.89% (60 runs sampled)
klass/extend (5)  x 93,069 ops/sec ±4.48% (64 runs sampled)
backbone-metal/extend (1)  x 145,954 ops/sec ±3.44% (69 runs sampled)
backbone-metal/extend (5)  x 72,164 ops/sec ±4.36% (70 runs sampled)

extend is quite a bit slower, so might need to look at some optimizations somewhere (though create is the main piece we want performant).

bryanaka · 2016-06-24T21:14:33Z

lib/assign-properties.js

@@ -37,7 +45,7 @@ function giveMethodSuper(superclass, name, fn) {

 var sourceAvailable = (function() {
  return this;
-}).toString().indexOf('return this;') > -1;
+}).toString().indexOf('return this') > -1;


Some minification/obfuscation will remove the semicolon above this line and all hell breaks loose.

can you submit this separately, this is pretty obvious good thing to get in asap. I would like to review the rest in more detail

@stefanpenner for sure, done in #39

bryanaka · 2016-06-25T22:33:47Z

Ah, actually this didn't resolve the case of passing in getters and setters at instantiation time.

Will add a test w/ code when I get some downtime.

stefanpenner · 2016-06-27T18:06:21Z

Ah, actually this didn't resolve the case of passing in getters and setters at instantiation time.

I'm unsure if we actually want to do this. Or can you provide a detailed example. I can totally see us wanting to support accessors at extend time though.

stefanpenner · 2016-06-27T18:06:55Z

lib/assign-properties.js


    if (typeof value === 'function' && hasSuper(value)) {
      target[key] = giveMethodSuper(getPrototypeOf(target), key, value);
    } else {
-      target[key] = value;
+      targetDescriptor = Object.getOwnPropertyDescriptor(target.__proto__, key);


i would love for us to use (or use a polyfil of) Object.getOwnPropertyDescriptors (notice the plural form)

bryanaka · 2016-07-02T04:49:09Z

I am in favor of not supporting getters and setters at instantiation time. I think in any OO language, this would either be either discouraged or not/poorly supported.

I figure a similar argument could be made against that behavior that was probably made in some form for not allowing the definition of computed properties on Ember.Object.create()

- support descriptors in assignProperties - add the getOwnPropertyDescriptors polyfill

bryanaka · 2016-07-02T22:50:31Z

@stefanpenner Added Object.getOwnPropertyDescriptors with TC-39's proposal polyfill with one change (Reflect.ownKeys => Object.keys).

Also coalesced the property definition to one call to Object.defineProperties which should help a little too.

stefanpenner · 2016-07-11T15:36:52Z

I am in favor of not supporting getters and setters at instantiation time. I think in any OO language, this would either be either discouraged or not/poorly supported.

sounds good.

I figure a similar argument could be made against that behavior that was probably made in some form for not allowing the definition of computed properties on Ember.Object.create()

yup (it as perf related)

@stefanpenner Added Object.getOwnPropertyDescriptors with TC-39's proposal polyfill with one change (Reflect.ownKeys => Object.keys).

looks good.

stefanpenner · 2016-07-11T15:37:34Z

@bryanaka mind running the benchmarks again with the recent changes?

I'm off vacation so my responses should be more timely now

bryanaka · 2016-08-04T20:33:19Z

@stefanpenner sorry got this just as I was going on a vacation - now I am back :)

Be aware that the numbers are going to look quite different since I am on a different machine than the previous benchmarks I ran.

Before:

core-object/create (0)  x 2,194,521 ops/sec ±2.26% (75 runs sampled)
core-object/create (1)  x 1,979,167 ops/sec ±1.27% (80 runs sampled)
core-object/create (5)  x 1,789,926 ops/sec ±4.53% (76 runs sampled)
core-object/create (default init) (0)  x 40,111,608 ops/sec ±4.46% (73 runs sampled)
core-object/create (default init) (1)  x 16,172,184 ops/sec ±5.07% (69 runs sampled)
core-object/create (default init) (5)  x 4,940,407 ops/sec ±15.62% (61 runs sampled)
uberproto/create (0)  x 2,762,460 ops/sec ±6.32% (76 runs sampled)
uberproto/create (1)  x 2,966,778 ops/sec ±1.55% (80 runs sampled)
uberproto/create (5)  x 2,988,660 ops/sec ±1.49% (72 runs sampled)
raw/create (0)  x 28,326,307 ops/sec ±1.42% (76 runs sampled)
raw/create (1)  x 32,401,993 ops/sec ±1.56% (79 runs sampled)
raw/create (5)  x 26,712,653 ops/sec ±1.53% (78 runs sampled)
esnext/create (0)  x 12,632,076 ops/sec ±75.00% (45 runs sampled)
esnext/create (1)  x 33,228,345 ops/sec ±1.68% (79 runs sampled)
esnext/create (5)  x 30,233,392 ops/sec ±1.79% (75 runs sampled)
klass/create (0)  x 20,560,784 ops/sec ±1.64% (77 runs sampled)
klass/create (1)  x 19,840,010 ops/sec ±1.66% (82 runs sampled)
klass/create (5)  x 18,764,717 ops/sec ±1.91% (77 runs sampled)
backbone-metal/create (0)  x 3,957,427 ops/sec ±30.41% (50 runs sampled)
backbone-metal/create (1)  x 4,404,259 ops/sec ±5.18% (75 runs sampled)
backbone-metal/create (5)  x 4,090,520 ops/sec ±5.78% (72 runs sampled)
core-object/extend (1)  x 392,657 ops/sec ±5.90% (64 runs sampled)
core-object/extend (5)  x 140,637 ops/sec ±3.83% (67 runs sampled)
uberproto/extend (1)  x 104,112 ops/sec ±4.81% (78 runs sampled)
uberproto/extend (5)  x 37,244 ops/sec ±1.48% (83 runs sampled)
klass/extend (1)  x 79,223 ops/sec ±3.69% (73 runs sampled)
klass/extend (5)  x 27,151 ops/sec ±4.25% (71 runs sampled)
backbone-metal/extend (1)  x 59,234 ops/sec ±3.63% (69 runs sampled)
backbone-metal/extend (5)  x 33,804 ops/sec ±3.08% (75 runs sampled)

After:

core-object/create (0)  x 2,085,642 ops/sec ±2.36% (76 runs sampled)
core-object/create (1)  x 2,002,813 ops/sec ±1.56% (80 runs sampled)
core-object/create (5)  x 1,937,289 ops/sec ±1.91% (73 runs sampled)
core-object/create (default init) (0)  x 44,447,911 ops/sec ±1.31% (77 runs sampled)
core-object/create (default init) (1)  x 18,901,320 ops/sec ±1.08% (84 runs sampled)
core-object/create (default init) (5)  x 5,929,880 ops/sec ±1.30% (79 runs sampled)
uberproto/create (0)  x 3,122,887 ops/sec ±1.28% (82 runs sampled)
uberproto/create (1)  x 3,003,756 ops/sec ±1.45% (82 runs sampled)
uberproto/create (5)  x 2,942,867 ops/sec ±1.48% (76 runs sampled)
raw/create (0)  x 27,412,096 ops/sec ±1.67% (83 runs sampled)
raw/create (1)  x 32,391,986 ops/sec ±1.66% (74 runs sampled)
raw/create (5)  x 23,358,272 ops/sec ±5.24% (74 runs sampled)
esnext/create (0)  x 25,771,558 ops/sec ±1.82% (77 runs sampled)
esnext/create (1)  x 32,444,258 ops/sec ±2.07% (78 runs sampled)
esnext/create (5)  x 30,056,537 ops/sec ±1.66% (75 runs sampled)
klass/create (0)  x 19,910,491 ops/sec ±1.48% (79 runs sampled)
klass/create (1)  x 20,207,103 ops/sec ±1.55% (74 runs sampled)
klass/create (5)  x 20,060,888 ops/sec ±1.44% (77 runs sampled)
backbone-metal/create (0)  x 5,704,667 ops/sec ±5.61% (70 runs sampled)
backbone-metal/create (1)  x 4,692,624 ops/sec ±5.04% (75 runs sampled)
backbone-metal/create (5)  x 4,415,965 ops/sec ±5.30% (72 runs sampled)
core-object/extend (1)  x 45,392 ops/sec ±2.23% (79 runs sampled)
core-object/extend (5)  x 25,195 ops/sec ±3.70% (65 runs sampled)
uberproto/extend (1)  x 113,612 ops/sec ±1.61% (74 runs sampled)
uberproto/extend (5)  x 36,785 ops/sec ±1.86% (79 runs sampled)
klass/extend (1)  x 83,610 ops/sec ±3.78% (71 runs sampled)
klass/extend (5)  x 29,501 ops/sec ±3.73% (76 runs sampled)
backbone-metal/extend (1)  x 66,079 ops/sec ±3.42% (70 runs sampled)
backbone-metal/extend (5)  x 36,181 ops/sec ±2.72% (73 runs sampled)

Unfortunately CoreObject#extend still drops pretty significantly.

bryanaka · 2016-08-26T00:36:19Z

ping @stefanpenner

I know you are busy, just want to make sure if you'd like any further work done on this PR, I can start on it.

stefanpenner · 2017-01-05T08:00:10Z

this seems like a vary reasonable addition! Will investigate (and try not to forget)

stefanpenner · 2017-01-05T08:02:44Z

This does not appear to support super in descriptors.

Now that node 0.12 is dropped. One can actually use ES6 classes and CoreObject. I believe that may help?

the following i believe works:

class Foo extends CoreObject {
  get name() { return 'hi'; }
}

stefanpenner · 2017-01-13T04:27:19Z

As we now have legit class syntax + super (node 0.12 is no longer supported), and that syntax supports accessors by default. I suspect we can close this.

Will gladly reopen if I am mistaken.

bryanaka · 2017-01-13T21:52:37Z

@stefanpenner I'm all for that. Less code == win in my book almost always

One thing though is that if we want to recommend that usage when needing descriptors, the super call syntax changes depending on if you use CoreObject.extend or class Foo extends CoreObject which is a little weird.

Would love to unite that concept but would be a huge breaking change just to get this._super to act like super and don't think descriptors are mainstream enough to justify that kind of breaking change ATM. Just food for thought to include in #29

bryanaka reviewed Jun 24, 2016
View reviewed changes

stefanpenner reviewed Jun 27, 2016
View reviewed changes

bryanaka force-pushed the feature/property-descriptors branch from 5654414 to f3582c4 Compare July 2, 2016 22:29

add support for property descriptors

ab513b7

- support descriptors in assignProperties - add the getOwnPropertyDescriptors polyfill

bryanaka force-pushed the feature/property-descriptors branch from f3582c4 to ab513b7 Compare July 2, 2016 22:37

stefanpenner closed this Jan 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for property descriptors #38

add support for property descriptors #38

bryanaka commented Jun 24, 2016 •

edited

Loading

bryanaka Jun 24, 2016

stefanpenner Jun 24, 2016

bryanaka Jun 24, 2016

bryanaka commented Jun 25, 2016

stefanpenner commented Jun 27, 2016

stefanpenner Jun 27, 2016 •

edited

Loading

bryanaka commented Jul 2, 2016 •

edited

Loading

bryanaka commented Jul 2, 2016

stefanpenner commented Jul 11, 2016

stefanpenner commented Jul 11, 2016

bryanaka commented Aug 4, 2016

bryanaka commented Aug 26, 2016

stefanpenner commented Jan 5, 2017

stefanpenner commented Jan 5, 2017 •

edited

Loading

stefanpenner commented Jan 13, 2017

bryanaka commented Jan 13, 2017

add support for property descriptors #38

add support for property descriptors #38

Conversation

bryanaka commented Jun 24, 2016 • edited Loading

bryanaka Jun 24, 2016

Choose a reason for hiding this comment

stefanpenner Jun 24, 2016

Choose a reason for hiding this comment

bryanaka Jun 24, 2016

Choose a reason for hiding this comment

bryanaka commented Jun 25, 2016

stefanpenner commented Jun 27, 2016

stefanpenner Jun 27, 2016 • edited Loading

Choose a reason for hiding this comment

bryanaka commented Jul 2, 2016 • edited Loading

bryanaka commented Jul 2, 2016

stefanpenner commented Jul 11, 2016

stefanpenner commented Jul 11, 2016

bryanaka commented Aug 4, 2016

bryanaka commented Aug 26, 2016

stefanpenner commented Jan 5, 2017

stefanpenner commented Jan 5, 2017 • edited Loading

stefanpenner commented Jan 13, 2017

bryanaka commented Jan 13, 2017

bryanaka commented Jun 24, 2016 •

edited

Loading

stefanpenner Jun 27, 2016 •

edited

Loading

bryanaka commented Jul 2, 2016 •

edited

Loading

stefanpenner commented Jan 5, 2017 •

edited

Loading