__nextSuper property on objects

[gordonkristan]

I know there's a lot of changes going on with the __nextSuper stuff, but I think I've found a bug in 1.7.0. __nextSuper is now being added as a property to all objects. Example:

var o = Ember.Object.create();

for (var name in o) {
    if (o.hasOwnProperty(name)) {
        console.log(name);
    }
}

If you run the above code, __nextSuper will be logged to the console. (JSBin for the lazy.) But oddly enough, Object.keys() doesn't include __nextSuper. I'm assuming it's because the property is declared on the object, but not declared enumerable.

I don't know if this is a bug exactly, maybe just a quirk, but it certainly broke my code.

[knownasilya]

@gordonkristan what's __nextSuper? Haven't seen anything about it.

[twokul]

@gordonkristan interesting quirk. the way __nextSuper is added onto the object wasn't changed. See here. In Chrome, when using Ember 1.6.0, __nextSuper doesn't show up in console; when using Ember 1.7.0, __nextSuper shows up. In Firefox, it doesn't show up for any versions.

[mixonic]

Thanks @gordonkristan, good to track. Ideally we could figure out what changed between 1.6 and 1.7.

Possible fixes include going back to an old style of _super (now that we can track when _super is and is not called we might be able to do this efficiently), and replacing hasOwnProperty on Ember.Object. Or, of course, finding out what introduces this post 1.6.

[gordonkristan]

@knownasilya __nextSuper is just an implementation detail of the this._super() functionality. It's been changing quite a bit since 1.5.0. I think we've already had a bug or two caused by it (like the frozen object bug).

As far as fixes go, I like the idea of replacing hasOwnProperty on Ember.Object. It's kind of a dirty trick, but I think it's fairly elegant in that it doesn't require other code changes. (And of course I mean as a temporary fix for any users currently running into issues. We'll obviously need a proper fix.)

[rwjblue]

@gordonkristan - For now you should be able to use Ember.keys (JSBin here):

var o = Ember.Object.create({
  name: 'foo'
});

var keys = Ember.keys(o);
for (var i = 0, l = keys.length; i < l; i++) {
  var name = keys[i];
  console.log(name);
}

console.log('done!')

[twokul]

this was introduced in this commit. reopen adds a new mixin to the list of existing mixins on the CoreObject. When you create a new instance of a CoreObject, proto gets invoked. Because didDefineProperty was added with reopen on CoreObject, didDefineProperty eventually gets wrapped in superWrapper and object gets __nextSuper property.

Moving didDefineProperty into extend fixes this behaviour. @stefanpenner @mixonic if you can confirm that I'm not crazy, I'll be more than happy to submit a patch.

[stefanpenner]

@twokul do all tests pass when you make that move?

[twokul]

@stefanpenner nope, one test fails >_< (the one that tests didDefineProperty). Correct me if I'm wrong, but I don't think there's a way around it. In order for didDefineProperty to be invoked, it has to be added either via CoreObject.reopen or added as a new function onto PrototypeMixin.

JSBin that was provided uses for...in to iterate over properties, for...in iterates only over enumerable properties. When we set __nextSuper in superWrapper we never tell it to be not enumerable. So I went ahead and replaced this line with this one.

I would expect defineProperty to be slower so I ran some benchmarks:

[benchmark.js] superFunctionCall x 91,615 ops/sec ±5.36% (75 runs sampled) // just as assignment
[benchmark.js] superFunctionCall x 116,785 ops/sec ±1.93% (86 runs sampled) // o_defineProperty

o____O

We can mark __nextSuper as not enumerable in superWrapper but I have mega doubts. @mixonic @stefanpenner thoughts?

Here's the benchmark, btw.

update:

The benchmark results that I posted are from Chrome Canary. Stable Chrome:

 // assignment
[benchmark.js] superFunctionCall x 21,715 ops/sec ±2.03% (83 runs sampled)
[benchmark.js] superFunctionCall x 22,409 ops/sec ±2.81% (73 runs sampled)
[benchmark.js] superFunctionCall x 21,702 ops/sec ±2.19% (83 runs sampled)

// o_defineProperty
[benchmark.js] superFunctionCall x 21,394 ops/sec ±3.81% (67 runs sampled)
[benchmark.js] superFunctionCall x 21,981 ops/sec ±5.04% (78 runs sampled)
[benchmark.js] superFunctionCall x 27,461 ops/sec ±5.26% (82 runs sampled)

[mixonic]

Starting to have mega-doubts indeed.

I'm unfamiliar with what is happening on didDefineProperty, and why extend fixes it. I would like to see if the issue is didDefineProperty being called before __nextSuper is set as a non-enumerable here. Just moving that line before proto is maybe worth a test? If we are setting __nextSuper (during didDefineProperty) before defining the as non-enumerable that would certainly explain some bad behavior.

I general, having the post-hook for creating a property call _super is very unfortunate. putting this the didDefineProperty code inside defineProperty is not obviously viable since these are runtime and not metal concerns, but I would question the validity of using _super anywhere in our internals.

I expect having superWrapper use defineProperty is a no-go. My understanding has always been that defineProperty is too slow to use unless required, I would like to see BMs on IE and get more feedback on this. Regardless, I don't think we need to use this solution.

[stefanpenner]

The performance is biased likely because we are only actually changing 2 objects, then repeatedly changing those. But since we are just re-setting the same property with the same configuration I suspect this means we are not re-changing those object shape.

In a real scenario we could be dealing with thousands or tens of thousands of different objects all with multiple different super methods.

[stefanpenner]

@twokul I think not-calling super in that didDefineProperty will solve this problem and I don't think it would cause any issues as it should be essentially the top of this superChain.
Good work tracking this down!

[twokul]

@stefanpenner removing _super call inside didDefineProperty fixes the current behaviour. I'm gonna submit a PR + regression test.

[stefanpenner]

@twokul awesome thanks!

[yagni]

We're seeing this behavior occurring again in 1.13.10, but only in production builds. I think it's fixed by #12062. Any ideas why it would be prod-specific though?

[mixonic]

@yagni __nextSuper is indeed removed in that PR, and #12463 addresses the bug that is likely causing your difference between prod/dev.

If you can re-test your case in 2.2 (beta today, should be stable tomorrow) that would be helpful. 1.13 might have some wonkiness but it should be addressed by 2.2.