Inline CreateResolvingFunctions

src/js/promise.js

Index: src/js/promise.js
diff --git a/src/js/promise.js b/src/js/promise.js
index c057238af696d78416a6198b3c0f68dabb8c31c1..23086b5054c7a7f28bf7aa62d9baaaf07f69fbcd 100644
--- a/src/js/promise.js
+++ b/src/js/promise.js
@@ -14,6 +14,8 @@
 var InternalArray = utils.InternalArray;
 var promiseCombinedDeferredSymbol =
     utils.ImportNow("promise_combined_deferred_symbol");
+var promiseResolvingFunctionCalledSymbol =
+    utils.ImportNow("promise_resolving_function_called_symbol");
 var promiseHasHandlerSymbol =
     utils.ImportNow("promise_has_handler_symbol");
 var promiseRejectReactionsSymbol =
@@ -42,34 +44,6 @@ const kRejected = -1;
 
 var lastMicrotaskId = 0;
 
-// ES#sec-createresolvingfunctions
-// CreateResolvingFunctions ( promise )
-function CreateResolvingFunctions(promise, debugEvent) {

[Dan Ehrenberg, 2016/08/30 22:29:28]

The spec calls this in two places, in the promise constructor and from then. Is
it always valid to share the AlreadyResolved bit between those two? Seems like
we will get stricter checking with this patch in some edge cases, bit seems like
we *should* get that stricter checking.

-  var alreadyResolved = false;
-
-  // ES#sec-promise-resolve-functions
-  // Promise Resolve Functions
-  var resolve = value => {
-    if (alreadyResolved === true) return;
-    alreadyResolved = true;
-    ResolvePromise(promise, value);
-  };
-
-  // ES#sec-promise-reject-functions
-  // Promise Reject Functions
-  var reject = reason => {
-    if (alreadyResolved === true) return;
-    alreadyResolved = true;
-    RejectPromise(promise, reason, debugEvent);
-  };
-
-  return {
-    __proto__: null,
-    resolve: resolve,
-    reject: reject
-  };
-}
-
 
 // ES#sec-promise-executor
 // Promise ( executor )
@@ -84,13 +58,14 @@ var GlobalPromise = function Promise(executor) {
 
   var promise = PromiseInit(%_NewObject(GlobalPromise, new.target));
   // Calling the reject function would be a new exception, so debugEvent = true
-  var callbacks = CreateResolvingFunctions(promise, true);
   var debug_is_active = DEBUG_IS_ACTIVE;
+  var reject = reason => RejectPromise(promise, reason, true, true);

[Dan Ehrenberg, 2016/08/30 22:29:28]

I wonder, how is performance if you call Function.prototype.bind rather than
creating a closure here?

   try {
     if (debug_is_active) %DebugPushPromise(promise);
-    executor(callbacks.resolve, callbacks.reject);
+    executor(value => ResolvePromise(promise, value, true),
+             reject);
   } %catch (e) {  // Natives syntax to mark this catch block.
-    %_Call(callbacks.reject, UNDEFINED, e);
+    %_Call(reject, UNDEFINED, e);
   } finally {
     if (debug_is_active) %DebugPopPromise();
   }
@@ -126,6 +101,7 @@ function PromiseSet(promise, status, value) {
   // the deferred symbol's state is stale, and the deferreds should be
   // read from the reject, resolve callbacks.
   SET_PRIVATE(promise, promiseDeferredReactionsSymbol, UNDEFINED);
+  SET_PRIVATE(promise, promiseResolvingFunctionCalledSymbol, UNDEFINED);
 
   return promise;
 }
@@ -237,12 +213,16 @@ function PromiseCreate() {
 
 // ES#sec-promise-resolve-functions
 // Promise Resolve Functions, steps 6-13
-function ResolvePromise(promise, resolution) {
+function ResolvePromise(promise, resolution, follow) {
+  var status = GET_PRIVATE(promise, promiseResolvingFunctionCalledSymbol);
+  if (!IS_UNDEFINED(status) && follow) return;
+
   if (resolution === promise) {
     return RejectPromise(promise,
                          %make_type_error(kPromiseCyclic, resolution),
                          true);
   }
+
   if (IS_RECEIVER(resolution)) {
     // 25.4.1.3.2 steps 8-12
     try {
@@ -283,17 +263,21 @@ function ResolvePromise(promise, resolution) {
       var id;
       var name = "PromiseResolveThenableJob";
       var instrumenting = DEBUG_IS_ACTIVE;
+      SET_PRIVATE(promise, promiseResolvingFunctionCalledSymbol, true);
+
       %EnqueueMicrotask(function() {
         if (instrumenting) {
           %DebugAsyncTaskEvent({ type: "willHandle", id: id, name: name });
         }
         // These resolving functions simply forward the exception, so
         // don't create a new debugEvent.
-        var callbacks = CreateResolvingFunctions(promise, false);
+
         try {
-          %_Call(then, resolution, callbacks.resolve, callbacks.reject);
+            %_Call(then, resolution,
+                   value => ResolvePromise(promise, value, false),
+                   reason => RejectPromise(promise, reason, false, false));
         } catch (e) {
-          %_Call(callbacks.reject, UNDEFINED, e);
+          %_Call(reason => RejectPromise(promise, reason, false, true), UNDEFINED, e);
         }
         if (instrumenting) {
           %DebugAsyncTaskEvent({ type: "didHandle", id: id, name: name });
@@ -311,13 +295,18 @@ function ResolvePromise(promise, resolution) {
 
 // ES#sec-rejectpromise
 // RejectPromise ( promise, reason )
-function RejectPromise(promise, reason, debugEvent) {
+function RejectPromise(promise, reason, debugEvent, follow) {

[Dan Ehrenberg, 2016/08/30 22:29:28]

I know I made things bad with the debugEvent parameter, but this second boolean
parameter makes things even more confusing to me. Why don't we just want to
always check AlreadyResolved--when is it observable that you could resolve or
reject a Promise multiple times?

+  var status = GET_PRIVATE(promise, promiseResolvingFunctionCalledSymbol);
+  if (!IS_UNDEFINED(status) && follow) return;
+
+  status = GET_PRIVATE(promise, promiseStateSymbol);
+  if (status === kFulfilled) return;

[adamk, 2016/08/30 23:00:35]

This looks like an unrelated bit of inlining, consider leaving it out of this
patch? Alternatively, mention it in the CL description. Do you know if it's a
measurable win?

   // Check promise status to confirm that this reject has an effect.
   // Call runtime for callbacks to the debugger or for unhandled reject.
   // The debugEvent parameter sets whether a debug ExceptionEvent should
   // be triggered. It should be set to false when forwarding a rejection
   // rather than creating a new one.
-  if (GET_PRIVATE(promise, promiseStateSymbol) === kPending) {
+  if (status === kPending) {
     // This check is redundant with checks in the runtime, but it may help
     // avoid unnecessary runtime calls.
     if ((debugEvent && DEBUG_IS_ACTIVE) ||
@@ -325,6 +314,7 @@ function RejectPromise(promise, reason, debugEvent) {
       %PromiseRejectEvent(promise, reason, debugEvent);
     }
   }
+
   FulfillPromise(promise, kRejected, reason, promiseRejectReactionsSymbol)
 }
 
@@ -339,11 +329,10 @@ function NewPromiseCapability(C, debugEvent) {
   if (C === GlobalPromise) {
     // Optimized case, avoid extra closure.
     var promise = PromiseInit(new GlobalPromise(promiseRawSymbol));
-    var callbacks = CreateResolvingFunctions(promise, debugEvent);
     return {
       promise: promise,
-      resolve: callbacks.resolve,
-      reject: callbacks.reject
+      resolve: value => ResolvePromise(promise, value, true),
+      reject: reason => RejectPromise(promise, reason, debugEvent, true)
     };
   }