[BUGFIX beta] Move __nextSuper onto meta #5430
Conversation
| test("contains on a froze array does not raise", function() { | ||
| var testArray = EmberArray.apply([]); | ||
| var throws = false; | ||
| Object.freeze(testArray); |
There was a problem hiding this comment.
need to detect if Object.freeze is supported by the runtime here, otherwise will fail in ie8
There was a problem hiding this comment.
Should I just skip the test if Object.free is not present?
Thanks, was thinking of this but was unsure.
Properties cannot be modified on a frozen object, which means even if the functions to not alter the context, the super implementation itself will raise an exception when a wrapped method calls (and __nextSuper is set). Part of the challenge here is that Ember wraps functions defensively- it has no insight into if super will be called by a function and no tools for hinting about non-super-calling functions. Ember also follows a pattern of using hooks, so many methods are intended to be overridden even if the override will never call super. Unfortunately this patterns costs us. But putting super on the meta we kill two birds with one stone. The property does not need to use slow defineProperty (it is already not enumerable on the object) and it can still be set regardless of an object being frozen. This is not expected to have an impact on performance. The number of meta objects created during a run of the tests remains steady at about 22000 objects, and the number of requests for meta triples from 30k to 90k. Fixes emberjs#4970, Fixes emberjs#5324, Fixes emberjs#5374
|
I am definitely in favor of this change, but I am unsure that we should pull it into the current beta channel. It is slated for release as 1.7.0 (todayish), we have already completed the standard beta releases, and vetting process. The current situation has existed since 1.5.0 (released on 2014-03-30), and we have not had major backlash about this problem. In short: this is definitely a valuable fix (please do not read that I am discounting the effort of validity of this), but I think we should wait and make it part of 1.8 (which will release into beta in the next day or two) so that we can get some folks banging on it to make sure there are no new quirks to be ironed out. |
|
Agreed, 100%. Please do not rush it through. @twokul was looking at performance this morning, and I would like to sync with Stef on it. I've not pushed anything new onto meta before. |
|
I haven't found radical perf regressions. Although, it's a known issue that |
|
@stefanpenner 👍 👎 ? IMO this is a good solution knowing that we want to have either an incremental or big improvement to super in 2.0. |
|
|
||
| ok(!throws, "No exception thrown calling contains"); | ||
| }); | ||
| } |
There was a problem hiding this comment.
You might want to test against Object.seal as well.
|
honestly the meta call there is extremely scary re: perf Did you isolate how this worked before the recent super chain? |
|
@stefanpenner maybe quick chat tonight? I'll share what I have |
|
In afew hours I should be free |
|
@mixonic got time to join us? |
|
#5444 does not actually supersede this, it only makes the problem happen less often. However this approach is un-popular and I agree we have some better options. Closing it in favor of a pass at resurrecting oldSuper. |
Properties cannot be modified on a frozen object, which means even if the functions to not alter the context, the super implementation itself will raise an exception when a wrapped method calls (and changes
__nextSuper).Part of the challenge here is that Ember wraps functions defensively- it has no insight into if super will be called by a function and no tools for hinting about non-super-calling functions. Ember also follows a pattern of using hooks, so many methods are intended to be overridden even if the override will never call super. Unfortunately this pattern costs us. With either hinting or a new internals-only super syntax, we can improve the performance regardless.
But putting super on the meta we kill two birds with one stone. The property does not need to use slow defineProperty (it is already not enumerable on the object) and it can still be set regardless of an object being frozen.
This is not expected to have an impact on performance. The number of meta objects created during a run of the tests remains steady at about 22000 objects, and the number of requests for meta triples from 30k to 90k.
Fixes #4970, Fixes #5324, Fixes #5374