Promises: Lazily create arrays to store resolve, reject callbacks

src/js/promise.js

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 (function(global, utils, extrasUtils) {
 
 "use strict";
 
 %CheckIsBootstrapping();
 
 // -------------------------------------------------------------------
 // Imports
 
 var InternalArray = utils.InternalArray;
 var MakeTypeError;
 var promiseCombinedDeferredSymbol =
     utils.ImportNow("promise_combined_deferred_symbol");
 var promiseHasHandlerSymbol =
     utils.ImportNow("promise_has_handler_symbol");
 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");
 var SpeciesConstructor;
 var toStringTagSymbol = utils.ImportNow("to_string_tag_symbol");
 
 utils.Import(function(from) {
   MakeTypeError = from.MakeTypeError;
   SpeciesConstructor = from.SpeciesConstructor;
 });
@@ skipping 57 matching lines @@
     %_Call(callbacks.reject, UNDEFINED, e);
   } finally {
     if (debug_is_active) %DebugPopPromise();
   }
 
   return promise;
 }
 
 // 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;
 }
 
 function PromiseCreateAndSet(status, value) {
   var promise = new GlobalPromise(promiseRawSymbol);
   // If debug is active, notify about the newly created promise first.
   if (DEBUG_IS_ACTIVE) PromiseSet(promise, kPending, UNDEFINED);
   return PromiseSet(promise, 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);
   }
 }
 
 function PromiseHandle(value, handler, deferred) {
   var debug_is_active = DEBUG_IS_ACTIVE;
   try {
     if (debug_is_active) %DebugPushPromise(deferred.promise, PromiseHandle);
     var result = handler(value);
     deferred.resolve(result);
   } catch (exception) {
     try { deferred.reject(exception); } catch (e) { }
   } finally {
     if (debug_is_active) %DebugPopPromise();
   }
 }
 
-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 });
     }
   });
   if (instrumenting) {
     id = ++lastMicrotaskId;
     name = status === kFulfilled ? "Promise.resolve" : "Promise.reject";
     %DebugAsyncTaskEvent({ type: "enqueue", id: id, name: name });
   }
@@ skipping 151 matching lines @@
   var status = GET_PRIVATE(this, promiseStateSymbol);
   if (IS_UNDEFINED(status)) {
     throw MakeTypeError(kNotAPromise, this);
   }
 
   var constructor = SpeciesConstructor(this, GlobalPromise);
   onResolve = IS_CALLABLE(onResolve) ? onResolve : PromiseIdResolveHandler;
   onReject = IS_CALLABLE(onReject) ? onReject : PromiseIdRejectHandler;
   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)) {
         // Promise has already been rejected, but had no handler.
         // Revoke previously triggered reject event.
         %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.
   SET_PRIVATE(this, promiseHasHandlerSymbol, true);
   return deferred.promise;
 }
 
 // Unspecified V8-specific legacy function
 // Chain is left around for now as an alias for then
 function PromiseChain(onResolve, onReject) {
@@ skipping 90 matching lines @@
     deferred.reject(e)
   }
   return deferred.promise;
 }
 
 
 // Utility for debugger
 
 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;
     }
   }
   return false;
 }
 
 // Return whether the promise will be handled by a user-defined reject
 // handler somewhere down the promise chain. For this, we do a depth-first
 // search for a reject handler that's not the default PromiseIdRejectHandler.
 function PromiseHasUserDefinedRejectHandler() {
@@ skipping 48 matching lines @@
   to.PromiseChain = PromiseChain;
   to.PromiseDefer = PromiseDefer;
   to.PromiseAccept = PromiseAccept;
 
   to.PromiseCreateRejected = PromiseCreateRejected;
   to.PromiseCreateResolved = PromiseCreateResolved;
   to.PromiseThen = PromiseThen;
 });
 
 })