perf($q): reduce closures

[jbedard]

First commit:

• Only wrap the internal $$reject / $$resolve when necessary, which is when a promise is resolved with another promise as the result. This is now in a $$delayResolve method.
• Move the "call once" logic into the $$reject / $$resolve methods, it already existed in notify
• Use isPromiseLike instead of redefining it, this is a slight functional change since they were different

Second:

• Only bind the Deferred methods when the Deferred will be returned publicly ($q.defer). Most of the time it is only used internally and the binding isn't necessary. The other option is to stop binding them completely which would be nicer but would be a breaking change.

All the Deferred / Promise methods use somewhere between 20-80% less memory except for a few with no changes ($q.defer(), promise.then() (no args))...

[lgalfaso]

in theory, this looks fine, but I think this will break Promise/A+

[jbedard]

Previously it was (isObject(val) || isFunction(val)) && val && isFunction(val.then). Now it is val && isFunction(val.then). Is the first one more correct then the second?

[lgalfaso]

before val.then was only retrieved once. This changed

[lgalfaso]

also, true.then (if defined), must not be handled as a promise (same with numeric values)

[jbedard]

I'll have to look into the Promise/A+ tests. It looks like the isObject(val) || isFunction(val) is important...

[jbedard]

Clearly I need to look into the aplus tests more (which I'm having trouble running on windows). The second commit could be done on it's own with a few minor changes though.

[jbedard]

I think it's fixed now. The val.then had to be in a try/catch, isPromiseLike is not strict enough for the aplus spec, and as you mentioned doing the $$state check was not enough and I had to revert back to a done flag. This PR now adds more code then deleting :(, but the memory improvements are still there...

[lgalfaso]

There are two parts in this PR, the changes on simpleBind that I like a lot, and the refactor to $$resolve that I am not too fond of as it makes the code harder to read. Out of the memory benefits, what percentage of the memory benefits come from each of these two?

[jbedard]

Personally I thought the $$resolve changes it made it easier to read. Now the error handling for promiseLike.then is separated and the resolving via promise is in it's own method (maybe there's a better name then $$delayResolve?). Removing callOnce adds a bit of code but I think it's easier to read then fns[0] and fns[1] and the weird nested closures created by callOnce...

Here's the simpleBind change on it's own if you want it: dde90e4

Here's a chart showing master, +simpleBind, +simpleBind +callOnce. This would be kb after performing the op 50x. Note that normally the retained memory is the same except when the test didn't resolve a promise so the deferred is still alive - then the difference might be in the retained memory more then GCed memory. P is an already resolved promise.

Action	Master	simpleBind	sb+callOnce	sb+this
resolve(1)	56	34	10	24
reject(1)	32	11	11	11
defer().resolve(1)	55	56	32	47
defer().reject(1)	32	33	33	33
resolve(P)	10 (77 retained)	10 (40 retained)	6 (28 retained)	6 (28 retained)
defer().resolve(P)	10 (78 retained)	10 (63 retained)	6 (50 retained)	6 (50 retained)
P.then(returnP)	5 (30 retained)	5 (9 retained)	5 (9 retained)	5 (9 retained)
Q(noop)	48	26	26	26

[lgalfaso]

@jbedard what about

$$resolve: function(val) {
  var then;
  var that = this;
  var done = false;
  try {
    if (isObject(val) || isFunction(val)) then = val.then;
    if (isFunction(then)) {
      this.promise.$$state.status = -1;
      then.call(val, resolvePromise, rejectPromise, notify);
    } else {
      this.promise.$$state.value = val;
      this.promise.$$state.status = 1;
      scheduleProcessQueue(this.promise.$$state);
    }
  } catch (e) {
    rejectPromise(e);
    exceptionHandler(e);
  }

  function resolvePromise(val) {
    if (done) return;
    done = true;
    that.$$resolve(val);
  }
  function rejectPromise(val) {
    if (done) return;
    done = true;
    that.$$reject(val);
  }
  function notify(progress) { that.notify(progress); }
}

?

[jbedard]

I think that will still create 3 closures every time $$resolve is called, where this PR only creates them if $$delayResolve is called.

I think your snippet is more readable then callOnce though, and I think has 1 less closure then callOnce+simpleBind(this,this.notify). I'll have to test it out though...

[jbedard]

@lgalfaso I've added your $$resolve to my table above. Basically anything calling $$resolve has the extra memory usage, where this PR only has the extra memory usage when passing a promise to $$resolve...

[lgalfaso]

@jbedard Promise/A+ is very strict on the implementation, and having an implementation that is a direct mapping from the spec is a big plus. If this implies that 3 new closures happen for every promise resolution, I still think it is best. As a side not, the closure on notify can be replaced with simpleBind(this, this.notify), this would help the case that a promise is resolved with a value and will create an extra closure when it is resolved with a promise.

[jbedard]

I don't think what I'm proposing deviates from the spec, it just pushes some work one function call deeper so it doesn't occur unless necessary. Does the spec somehow dictate how many internal function calls you can make?

[lgalfaso]

it does not dictate anything about how many functions deep it should go, but the current implementation is a perfect mapping on each check that should be done and the order that they should be performed. If a future clarification/revision is made (as it happened before), having this mapping helps a lot

[jbedard]

I think this PR improves that. This separates 3.i - 3.iv, 4 into separate logical blocks (which could be functions) where currently they are all mixed together and do not map to the spec well.

I find it quite confusing today where fetching .then (3.i) throws (3.ii) to the same try/catch as invoking .then (3.iii) throws (3.iii.d). That shared try/catch involves the call-once logic (3.iii.c) even though we may have never even tried calling anything yet (due to 3.ii). Today that try/catch for (3.ii + 3.iii.d) also wraps (4). That means if anything in (3 - 4) of the spec changes then some refactoring similar to this PR might be required...

[lgalfaso]

it looks like we will have to disagree, I still find the new code at $$resolve harder to read, and having 20 lines to wrap one line that does a then.call does not help

[jbedard]

Do you think it's worth removing these closures at all? I can think of many other ways of doing it... For example your snippet but with the resolve/notify inlined removes 2/3 closures (but the reject can not be inlined because it is also used in the catch block).

I've swapped the order of the commits in this PR so the simpleBind one is first and doesn't depend on the other...

[lgalfaso]

The notify function can be changed to a simpleBind, and the common case of resolving to a value would not pay for that closure. The other two functions, I would leave them as it, at some point in time JavaScript VMs should be smart enough to only create the closures when needed

[jbedard]

Updated...