[BUGFIX beta] Extracts computed property set into a separate function

[twokul]

This is done to work around V8 (it cannot optimize functions with try blocks).

Comparison from a real app:

defineProperty

Before:

After:

[rwjblue]

Wouldn't we also want to avoid passing arguments around?

[twokul]

apply is special and it's ok to use it. It doesn't get deoptimized.

[stefanpenner]

c

[mixonic]

So confused. Why did we stop using __nextSuper.apply(this, arguments) then? Instead we have a two pass "optimization" there, first build a new array, then pass it to the Ember apply function that tries to use call.

Or is it still slower even if it doesn't de-opt? Just trying to understand where it is appropriate to use Function.apply and not.

[twokul]

@mixonic are you referring to this call?

[mixonic]

The wrapper calls an internal apply function but has to jump through hoops to avoid leaking arguments only to use Function.call in the apply function.

The extra code to unwrap arguments and use call is just crazy faster than using Function.apply? I will not question the gods of v8, but at some point wrap just used Function.apply and we stopped.

[twokul]

@mixonic I honestly don't recall __nextSuper.apply(this, arguments) but I'm pretty new part of the code base. Generally, you want to avoid passing arguments to the function (and in that case apply is a function that calls either Function.call or Function.apply).

As I mentioned before, calling Function.apply is safe but I'm not sure why we stopped doing that. Here's the commit where "array hack" was introduced. Maybe we should revisit and see if there's perf/functionality impact if we switch to calling __nextSuper.apply? Calling apply is expensive.

[mixonic]

Much research to do. We can take it offline of here though and just fold that plan into the oldSuper-is-newSuper refactor. Thanks for certifying my sanity!

[stefanpenner]

that apply hack exists because in some cases it is faster to call

a.call(this, b); and
a.call(this, b, c); then it is to call a.apply(this, arguments);

[stefanpenner]

in isolation this is true, but i wonder if in the context of a more complex system it is true or not.

[wagenet]

Should this be considered a bugfix?

[stefanpenner]

nice!

[stefanpenner]

lets slide this into beta aswell

[krisselden]

@twokul the scope of this try/finally is much larger scope than it needs to be.

The try/finally is meant only to ensure with reset _suspended which is only used in the didChange hook, and only needs to be set while invoking propertyDidChange(obj, keyName) and also is only needed if the CP is watched and cacheable.

Armed with that knowledge I think you should be able to improve this further.

As a separate note, I think though we should make volatile a separate descriptor that just invokes .get() and .set() and doesn't do anything else, remove all the conditionals on cacheable.

[krisselden]

LGTM, thanks @twokul

Since this doesn't change functionality and in my opinion very low risk, can we target release? /cc @rwjblue and @stefanpenner

[rwjblue]

👍 from me for [BUGFIX release].

[mmun]

nice!