[async-await] Don't create resolving callbacks for throwaway promises

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();
 
@@ skipping 275 matching lines @@
 }
 
 // Export to bindings
 function DoRejectPromise(promise, reason) {
   %PromiseReject(promise, reason, true);
   PromiseSet(promise, kRejected, reason);
 }
 
 // ES#sec-newpromisecapability
 // NewPromiseCapability ( C )
-function NewPromiseCapability(C, debugEvent) {
+function NewPromiseCapability(C, debugEvent, doNotCreateCallbacks) {

[Dan Ehrenberg, 2016/11/18 08:05:38]

Style suggestion: Rather than making this extra boolean flag (which I think
makes the code harder to read), what if you refactored the true case into a
separate function that either returns PromiseCreate() or falls back to
NewPromiseCapability().promise for other constructors?

[gsathya, 2016/11/18 08:34:28]

Do you mean a function like --
NewPromiseCapabilityWithoutCallbacks() { return { promise: PromiseCreate(),
resolve: UNDEFINED, reject: UNDEFINED} }

This would make code in PromiseThen look a little more verbose --
if (constructor === GlobalPromise) {
  resultCapability = NewPromiseCapabilityWithoutCallbacks();
} else {
  resultCapability = NewPromiseCapability(constructor, false);
}

Do you think this is better?

[Dan Ehrenberg, 2016/11/18 08:55:37]

I was imagining that all of that would be part of the body of
NewPromiseCapabilityWithoutCallbacks. In general, boolean flags are really
difficult for readability/maintainability, and I think it's worth it to add a
tiny bit more verbose code to avoid them.

   if (C === GlobalPromise) {
     // Optimized case, avoid extra closure.
     var promise = PromiseCreate();
+
+    // The resultCapability.promise is only ever fulfilled internally,
+    // so we don't need the closures to protect against accidentally
+    // calling them multiple times.

[caitp, 2016/11/18 05:47:15]

I like that this is all documented so nicely. Since this behaviour is specific
to await expressions, I think we will want to move this out of
NewPromiseCapability, maybe to a code-stub (or just inlined),  since this
internal-ness is always known statically. The documentation in this CL should
help make that transition easier.

LGTM, but would like assurance that it's not visibly different in the debugger

[gsathya, 2016/11/18 06:02:28]

This is used by PromiseThen as well which is why I didn't inline it.
 
There shouldn't be anything visibly different in the debugger since its the same
callbacks (without the alreadyResolved check) that's being run.

[Dan Ehrenberg, 2016/11/18 08:05:38]

Could you mention this in the commit message? Does this benchmark change
Bluebird perf?
This is a decent reason for making it a separate function. The new function does
not need to take debugEvent as an argument, as that argument is just used for
making the callbacks do the right thing.

[gsathya, 2016/11/18 08:34:27]

This isn't a new change. This piece of code was specific to PromiseThen, I
refactored it out to be in NewPromiseCapability.
Yeah, agreed, but it makes code more verbose in other places as mentioned above.

+    if (doNotCreateCallbacks) {
+      return {
+        promise: promise,
+        resolve: UNDEFINED,
+        reject: UNDEFINED
+      };
+    }
+
     // TODO(gsathya): Remove container for callbacks when this is
     // moved to CPP/TF.
     var callbacks = %create_resolving_functions(promise, debugEvent);
     return {
       promise: promise,
       resolve: callbacks[kResolveCallback],
       reject: callbacks[kRejectCallback]
     };
   }
 
@@ skipping 63 matching lines @@
 // ES#sec-promise.prototype.then
 // Promise.prototype.then ( onFulfilled, onRejected )
 // Multi-unwrapped chaining with thenable coercion.
 function PromiseThen(onResolve, onReject) {
   var status = GET_PRIVATE(this, promiseStateSymbol);
   if (IS_UNDEFINED(status)) {
     throw %make_type_error(kNotAPromise, this);
   }
 
   var constructor = SpeciesConstructor(this, GlobalPromise);
-  var resultCapability;
 
-  // The resultCapability.promise is only ever fulfilled internally,
-  // so we don't need the closures to protect against accidentally
-  // calling them multiple times.
-  if (constructor === GlobalPromise) {
-    // TODO(gsathya): Combine this into NewPromiseCapability.
-    resultCapability = {
-      promise: PromiseCreate(),
-      resolve: UNDEFINED,
-      reject: UNDEFINED
-    };
-  } else {
-    // Pass false for debugEvent so .then chaining does not trigger
-    // redundant ExceptionEvents.
-    resultCapability = NewPromiseCapability(constructor, false);
-  }
+  // Pass false for debugEvent so .then chaining does not trigger
+  // redundant ExceptionEvents.
+  var resultCapability = NewPromiseCapability(
+      constructor, false, constructor === GlobalPromise);

[Dan Ehrenberg, 2016/11/18 08:05:38]

Not sure why you do this comparison. Seems like avoiding creating the functions
is only done on GlobalPromise anyway.

[gsathya, 2016/11/18 08:34:27]

Yes, but this comparison is required to not create the functions. You could
create functions in the the GlobalPromise case too (like in Promise.all and
Promise.race).

[Dan Ehrenberg, 2016/11/18 08:55:37]

I'd suggest factoring all this logic into NewPromiseCapabilityWithoutCallbacks.
If C is not GlobalPromise, create the functions. The logic only applies to the
callers of this special variant, so it wouldn't affect Promise.all.

+
   return PerformPromiseThen(this, onResolve, onReject, resultCapability);
 }
 
 // ES#sec-promise.prototype.catch
 // Promise.prototype.catch ( onRejected )
 function PromiseCatch(onReject) {
   return this.then(UNDEFINED, onReject);
 }
 
 // Combinators.
 
 // ES#sec-promise.resolve
 // Promise.resolve ( x )
 function PromiseResolve(x) {
   if (!IS_RECEIVER(this)) {
     throw %make_type_error(kCalledOnNonObject, PromiseResolve);
   }
   if (IsPromise(x) && x.constructor === this) return x;
 
   // Avoid creating resolving functions.
   if (this === GlobalPromise) {
     var promise = PromiseCreate();
     ResolvePromise(promise, x);
     return promise;
   }
 
-  // debugEvent is not so meaningful here as it will be resolved
-  var promiseCapability = NewPromiseCapability(this, true);
+  // debugEvent is not so meaningful here as it will be resolved.
+  // We have already addressed the case where we don't need callbacks created,
+  // so we pass false to create the callbacks.
+  var promiseCapability = NewPromiseCapability(this, true, false);
   %_Call(promiseCapability.resolve, UNDEFINED, x);
   return promiseCapability.promise;
 }
 
 // ES#sec-promise.all
 // Promise.all ( iterable )
 function PromiseAll(iterable) {
   if (!IS_RECEIVER(this)) {
     throw %make_type_error(kCalledOnNonObject, "Promise.all");
   }
 
   // false debugEvent so that forwarding the rejection through all does not
   // trigger redundant ExceptionEvents
-  var deferred = NewPromiseCapability(this, false);
+  // We need to create the resolving callbacks for the promise.
+  var deferred = NewPromiseCapability(this, false, false);
   var resolutions = new InternalArray();
   var count;
 
   // For catch prediction, don't treat the .then calls as handling it;
   // instead, recurse outwards.
   var instrumenting = DEBUG_IS_ACTIVE;
   if (instrumenting) {
     SET_PRIVATE(deferred.reject, promiseForwardingHandlerSymbol, true);
   }
 
@@ skipping 43 matching lines @@
 
 // ES#sec-promise.race
 // Promise.race ( iterable )
 function PromiseRace(iterable) {
   if (!IS_RECEIVER(this)) {
     throw %make_type_error(kCalledOnNonObject, PromiseRace);
   }
 
   // false debugEvent so that forwarding the rejection through race does not
   // trigger redundant ExceptionEvents
-  var deferred = NewPromiseCapability(this, false);
+  // We need to create the resolving callbacks for the promise.
+  var deferred = NewPromiseCapability(this, false, false);
 
   // For catch prediction, don't treat the .then calls as handling it;
   // instead, recurse outwards.
   var instrumenting = DEBUG_IS_ACTIVE;
   if (instrumenting) {
     SET_PRIVATE(deferred.reject, promiseForwardingHandlerSymbol, true);
   }
 
   try {
     for (var value of iterable) {
@@ skipping 125 matching lines @@
   to.PromiseThen = PromiseThen;
 
   to.GlobalPromise = GlobalPromise;
   to.NewPromiseCapability = NewPromiseCapability;
   to.PerformPromiseThen = PerformPromiseThen;
   to.ResolvePromise = ResolvePromise;
   to.RejectPromise = RejectPromise;
 });
 
 })