Promises: Lazily create arrays to store resolve, reject callbacks

src/js/promise.js

Index: src/js/promise.js
diff --git a/src/js/promise.js b/src/js/promise.js
index f2fcc477996afc47d1954b71504ecd7b48f73943..8997c93e0e66fa3d17ad0a804ee8075a25d538d5 100644
--- a/src/js/promise.js
+++ b/src/js/promise.js
@@ -21,6 +21,8 @@ var promiseRejectReactionsSymbol =
     utils.ImportNow("promise_reject_reactions_symbol");
 var promiseFulfillReactionsSymbol =
     utils.ImportNow("promise_fulfill_reactions_symbol");
+var promiseDeferredReactionsSymbol =
+    utils.ImportNow("promise_deferred_reactions_symbol");
 var promiseRawSymbol = utils.ImportNow("promise_raw_symbol");
 var promiseStateSymbol = utils.ImportNow("promise_state_symbol");
 var promiseResultSymbol = utils.ImportNow("promise_result_symbol");
@@ -98,11 +100,13 @@ var GlobalPromise = function Promise(resolver) {
 
 // Core functionality.
 
-function PromiseSet(promise, status, value, onResolve, onReject) {
+function PromiseSet(
+    promise, status, value, onResolve, onReject, deferred) {

[adamk, 2016/05/25 19:18:57]

None of the callers pass more than 3 arguments anymore, I think you can just
leave off the last three arguments and SET_PRIVATE(..., UNDEFINED) below.

[gsathya, 2016/05/25 21:46:35]

Done.

   SET_PRIVATE(promise, promiseStateSymbol, status);
   SET_PRIVATE(promise, promiseResultSymbol, value);
   SET_PRIVATE(promise, promiseFulfillReactionsSymbol, onResolve);

[adamk, 2016/05/25 19:18:57]

Maybe add a comment here with a version of the third paragraph of your CL
description, explaining the states the last three symbols could be in?

[gsathya, 2016/05/25 21:46:35]

Done. Let me know if you can think of a more cohesive explanation

   SET_PRIVATE(promise, promiseRejectReactionsSymbol, onReject);
+  SET_PRIVATE(promise, promiseDeferredReactionsSymbol, deferred);
   return promise;
 }
 
@@ -115,13 +119,17 @@ function PromiseCreateAndSet(status, value) {
 
 function PromiseInit(promise) {
   return PromiseSet(
-      promise, kPending, UNDEFINED, new InternalArray, new InternalArray)
+      promise, kPending, UNDEFINED, UNDEFINED, UNDEFINED, UNDEFINED);
 }
 
 function PromiseDone(promise, status, value, promiseQueue) {
   if (GET_PRIVATE(promise, promiseStateSymbol) === kPending) {
     var tasks = GET_PRIVATE(promise, promiseQueue);
-    if (tasks.length) PromiseEnqueue(value, tasks, status);
+    if (!IS_NULL_OR_UNDEFINED(tasks)) {

[adamk, 2016/05/25 19:18:57]

It should never be null, right? Can this just be IS_UNDEFINED? Same question for
other such calls in this file.

[gsathya, 2016/05/25 21:46:35]

Done.

+      var tasks = GET_PRIVATE(promise, promiseQueue);
+      var deferreds = GET_PRIVATE(promise, promiseDeferredReactionsSymbol);
+      PromiseEnqueue(value, tasks, deferreds, status);
+    }
     PromiseSet(promise, status, value);
   }
 }
@@ -139,14 +147,18 @@ function PromiseHandle(value, handler, deferred) {
   }
 }
 
-function PromiseEnqueue(value, tasks, status) {
+function PromiseEnqueue(value, tasks, deferreds, status) {
   var id, name, instrumenting = DEBUG_IS_ACTIVE;
   %EnqueueMicrotask(function() {
     if (instrumenting) {
       %DebugAsyncTaskEvent({ type: "willHandle", id: id, name: name });
     }
-    for (var i = 0; i < tasks.length; i += 2) {
-      PromiseHandle(value, tasks[i], tasks[i + 1])
+    if (IS_ARRAY(tasks)) {
+      for (var i = 0; i < tasks.length; i += 1) {
+        PromiseHandle(value, tasks[i], deferreds[i]);
+      }
+    } else {
+      PromiseHandle(value, tasks, deferreds);
     }
     if (instrumenting) {
       %DebugAsyncTaskEvent({ type: "didHandle", id: id, name: name });
@@ -319,14 +331,37 @@ function PromiseThen(onResolve, onReject) {
   var deferred = NewPromiseCapability(constructor);
   switch (status) {
     case kPending:

[adamk, 2016/05/25 19:18:57]

This whole block seems like it should be encapsulated in a helper function.

[gsathya, 2016/05/25 21:46:34]

Done.

-      GET_PRIVATE(this, promiseFulfillReactionsSymbol).push(onResolve,
-                                                            deferred);
-      GET_PRIVATE(this, promiseRejectReactionsSymbol).push(onReject, deferred);
+      var resolveCallbacks = GET_PRIVATE(this, promiseFulfillReactionsSymbol);
+      if (IS_NULL_OR_UNDEFINED(resolveCallbacks)) {
+        SET_PRIVATE(this, promiseDeferredReactionsSymbol, deferred);
+        SET_PRIVATE(this, promiseFulfillReactionsSymbol, onResolve);
+        SET_PRIVATE(this, promiseRejectReactionsSymbol, onReject);
+      } else if (!IS_ARRAY(resolveCallbacks)) {

[adamk, 2016/05/25 19:18:57]

Did you figure out if Bluebird goes down this path? It seems like it still might
be worth avoiding creating the deferreds array as previously discussed.

[gsathya, 2016/05/25 21:46:35]

Bluebird doesn't create arrays at all, they just store the callbacks at an index
in the promise object itself like this --
this[deferredOffset] = deferred
this[resolveOffset] = onResolve
this[rejectOffset] = onReject

Removing the deferred array will make the code a little harder to reason about,
for example --
1) For the initial state, we store the deferred object in the deferred symbol
2) For subsequent states where there is more than one callback, we can store the
deferred object in the resolve/reject callback arrays (like we've been doing
before this patch).

When we hit the subsequent state of having more than one callback, we are left
with a stale deferred symbol. I could make this a null but I need to add
additionally state handling, which makes the code harder to understand and
reason about.

I feel like this change can be done in a follow up CL. Thoughts?

[adamk, 2016/05/25 21:57:43]

Sorry, when I say "Bluebird" I meant the benchmarks: do they ever call then()
more than once on a promise?
The additional "end" state is definitely the weirdest part of this. One way to
make it more readable would be to use constants like we do for [[PromiseState]]
for the "zero" and "stale" cases. WDYT?
 
For me this depends on whether the benchmarks we're using do anything to stress
the multiple-listener case.

[adamk, 2016/05/25 23:41:35]

As discussed offline, though this path isn't covered by the benchmark, let's
land this first and then remove the extra array in a followup.

+        var deferreds = new InternalArray();
+        var resolveCallbacks = new InternalArray();
+        var rejectCallbacks = new InternalArray();
+
+        deferreds.push(GET_PRIVATE(this, promiseDeferredReactionsSymbol));
+        resolveCallbacks.push(GET_PRIVATE(this, promiseFulfillReactionsSymbol));

[adamk, 2016/05/25 19:18:57]

This extra GET isn't needed, you already had an array in resolveCallbacks. Just
choose a different name for that var. so it doesn't get shadowed here.

[gsathya, 2016/05/25 21:46:35]

Done. Let me know if you have a better name for that var.

+        rejectCallbacks.push(GET_PRIVATE(this, promiseRejectReactionsSymbol));
+
+        deferreds.push(deferred);
+        resolveCallbacks.push(onResolve);
+        rejectCallbacks.push(onReject);
+
+        SET_PRIVATE(this, promiseDeferredReactionsSymbol, deferreds);
+        SET_PRIVATE(this, promiseFulfillReactionsSymbol, resolveCallbacks);
+        SET_PRIVATE(this, promiseRejectReactionsSymbol, rejectCallbacks);
+      } else {
+        GET_PRIVATE(this, promiseDeferredReactionsSymbol).push(deferred);
+        GET_PRIVATE(this, promiseFulfillReactionsSymbol).push(onResolve);

[adamk, 2016/05/25 19:18:57]

This extra GET isn't needed, you already have an array in resolveCallbacks.

[gsathya, 2016/05/25 21:46:35]

Done.

+        GET_PRIVATE(this, promiseRejectReactionsSymbol).push(onReject);
+      }
       break;
     case kFulfilled:
       PromiseEnqueue(GET_PRIVATE(this, promiseResultSymbol),
-                     [onResolve, deferred],
-                     kFulfilled);
+                     onResolve, deferred,
+                     kFulfilled, 1);

[adamk, 2016/05/25 19:18:57]

Looks like a stray change from the previous approach?

[gsathya, 2016/05/25 21:46:34]

Done.

       break;
     case kRejected:
       if (!HAS_DEFINED_PRIVATE(this, promiseHasHandlerSymbol)) {
@@ -335,8 +370,8 @@ function PromiseThen(onResolve, onReject) {
         %PromiseRevokeReject(this);
       }
       PromiseEnqueue(GET_PRIVATE(this, promiseResultSymbol),
-                     [onReject, deferred],
-                     kRejected);
+                     onReject, deferred,
+                     kRejected, 1);

[adamk, 2016/05/25 19:18:57]

Same here.

[gsathya, 2016/05/25 21:46:35]

Done.

       break;
   }
   // Mark this promise as having handler.
@@ -447,17 +482,24 @@ function PromiseRace(iterable) {
 
 function PromiseHasUserDefinedRejectHandlerRecursive(promise) {
   var queue = GET_PRIVATE(promise, promiseRejectReactionsSymbol);
+  var deferreds = GET_PRIVATE(promise, promiseDeferredReactionsSymbol);
   if (IS_UNDEFINED(queue)) return false;
-  for (var i = 0; i < queue.length; i += 2) {
+  if (!IS_ARRAY(queue)) {
+    queue = [queue];

[adamk, 2016/05/25 19:18:57]

This is a neat trick, I'll have to think about whether I like this more than
just factoring the loop body out into its own function.

[gsathya, 2016/05/25 21:46:35]

:P Happy to change it, let me know

[adamk, 2016/05/25 23:41:35]

Please do change it to a factored-out function and either a loop or just a call
to that function as appropriate.

[gsathya, 2016/05/26 22:18:47]

Done.

+    deferreds = [deferreds];
+  }
+  for (var i = 0; i < queue.length; i += 1) {
     var handler = queue[i];
     if (handler !== PromiseIdRejectHandler) {
-      var deferred = GET_PRIVATE(handler, promiseCombinedDeferredSymbol);
-      if (IS_UNDEFINED(deferred)) return true;
-      if (PromiseHasUserDefinedRejectHandlerRecursive(deferred.promise)) {
+      var combinedDeferred =
+          GET_PRIVATE(handler, promiseCombinedDeferredSymbol);
+      if (IS_UNDEFINED(combinedDeferred)) return true;
+      if (PromiseHasUserDefinedRejectHandlerRecursive(
+          combinedDeferred.promise)) {
         return true;
       }
     } else if (PromiseHasUserDefinedRejectHandlerRecursive(
-                   queue[i + 1].promise)) {
+                   deferreds[i].promise)) {
       return true;
     }
   }