Inline CreateResolvingFunctions

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 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 =
     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 speciesSymbol = utils.ImportNow("species_symbol");
 var toStringTagSymbol = utils.ImportNow("to_string_tag_symbol");
 
 utils.Import(function(from) {
   SpeciesConstructor = from.SpeciesConstructor;
 });
 
 // -------------------------------------------------------------------
 
 // [[PromiseState]] values:
 const kPending = 0;
 const kFulfilled = +1;
 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 )
 var GlobalPromise = function Promise(executor) {
   if (executor === promiseRawSymbol) {
     return %_NewObject(GlobalPromise, new.target);
   }
   if (IS_UNDEFINED(new.target)) throw %make_type_error(kNotAPromise, this);
   if (!IS_CALLABLE(executor)) {
     throw %make_type_error(kResolverNotAFunction, 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();
   }
 
   return promise;
 }
 
 // Core functionality.
 
 function PromiseSet(promise, status, value) {
@@ skipping 15 matching lines @@
   // There are 2 possible states for this symbol --
   // 1) UNDEFINED -- This is the zero state, no deferred object is
   // attached to this symbol. When we want to add a new deferred we
   // directly attach it to this symbol.
   // 2) symbol with attached deferred object -- New deferred objects
   // are not attached to this symbol, but instead they are directly
   // attached to the resolve, reject callback arrays. At this point,
   // 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;
 }
 
 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);
 }
@@ skipping 91 matching lines @@
 function IsPromise(x) {
   return IS_RECEIVER(x) && HAS_DEFINED_PRIVATE(x, promiseStateSymbol);
 }
 
 function PromiseCreate() {
   return new GlobalPromise(PromiseNopResolver)
 }
 
 // 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 {
       var then = resolution.then;
     } catch (e) {
       return RejectPromise(promise, e, true);
     }
 
     // Resolution is a native promise and if it's already resolved or
     // rejected, shortcircuit the resolution procedure by directly
@@ skipping 20 matching lines @@
         SET_PRIVATE(resolution, promiseHasHandlerSymbol, true);
         return;
       }
     }
 
     if (IS_CALLABLE(then)) {
       // PromiseResolveThenableJob
       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 });
         }
       });
       if (instrumenting) {
         id = ++lastMicrotaskId;
         %DebugAsyncTaskEvent({ type: "enqueue", id: id, name: name });
       }
       return;
     }
   }
   FulfillPromise(promise, kFulfilled, resolution, promiseFulfillReactionsSymbol);
 }
 
 // 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) ||
         !HAS_DEFINED_PRIVATE(promise, promiseHasHandlerSymbol)) {
       %PromiseRejectEvent(promise, reason, debugEvent);
     }
   }
+
   FulfillPromise(promise, kRejected, reason, promiseRejectReactionsSymbol)
 }
 
 // Export to bindings
 function DoRejectPromise(promise, reason) {
   return RejectPromise(promise, reason, true);
 }
 
 // ES#sec-newpromisecapability
 // NewPromiseCapability ( C )
 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)
     };
   }
 
   var result = {promise: UNDEFINED, resolve: UNDEFINED, reject: UNDEFINED };
   result.promise = new C((resolve, reject) => {
     if (!IS_UNDEFINED(result.resolve) || !IS_UNDEFINED(result.reject))
         throw %make_type_error(kPromiseExecutorAlreadyInvoked);
     result.resolve = resolve;
     result.reject = reject;
   });
@@ skipping 269 matching lines @@
   to.PromiseCastResolved = PromiseCastResolved;
   to.PromiseThen = PromiseThen;
 
   to.GlobalPromise = GlobalPromise;
   to.NewPromiseCapability = NewPromiseCapability;
   to.PerformPromiseThen = PerformPromiseThen;
   to.RejectPromise = RejectPromise;
 });
 
 })