[promise] Lookup the resolve property only once

In the PerformPromise{All, Race, AllSettled} operations, the resolve
property of the constructor is looked up only once.

In the implementation, for the fast path, where the constructor's
resolve property is untainted, the resolve function is set to undefined.
Since undefined can't be a valid value for the resolve function,
we can switch on it (in CallResolve) to directly call the  PromiseResolve
builtin. If the resolve property is tainted, we do an observable property
lookup, save this value, and call this property later (in CallResolve).

I ran this CL against the test262 tests locally and they all pass:
https://github.com/tc39/test262/pull/2131

Spec:
- https://github.com/tc39/ecma262/pull/1506
- https://github.com/tc39/proposal-promise-allSettled/pull/40

Bug: v8:9152
Change-Id: Icb36a90b5a244a67a729611c7b3315d2c29de6e9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1574705
Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60957}

src/builtins/builtins-promise-gen.cc

@@ -722,20 +722,18 @@ Node* PromiseBuiltinsAssembler::InvokeThen(Node* native_context, Node* receiver,
   return var_result.value();
 }
 
-Node* PromiseBuiltinsAssembler::InvokeResolve(Node* native_context,
-                                              Node* constructor, Node* value,
-                                              Label* if_exception,
-                                              Variable* var_exception) {
+Node* PromiseBuiltinsAssembler::CallResolve(Node* native_context,
+                                            Node* constructor, Node* resolve,
+                                            Node* value, Label* if_exception,
+                                            Variable* var_exception) {
   CSA_ASSERT(this, IsNativeContext(native_context));
-
+  CSA_ASSERT(this, IsConstructor(constructor));
   VARIABLE(var_result, MachineRepresentation::kTagged);
   Label if_fast(this), if_slow(this, Label::kDeferred), done(this, &var_result);
-  // We can skip the "resolve" lookup on {constructor} if it's the
-  // Promise constructor and the Promise.resolve protector is intact,
-  // as that guards the lookup path for the "resolve" property on the
-  // Promise constructor.
-  BranchIfPromiseResolveLookupChainIntact(native_context, constructor, &if_fast,
-                                          &if_slow);
+
+  // Undefined can never be a valid value for the resolve function,
+  // instead it is used as a special marker for the fast path.
+  Branch(IsUndefined(resolve), &if_fast, &if_slow);
 
   BIND(&if_fast);
   {
@@ -749,9 +747,7 @@ Node* PromiseBuiltinsAssembler::InvokeResolve(Node* native_context,
 
   BIND(&if_slow);
   {
-    Node* const resolve =
-        GetProperty(native_context, constructor, factory()->resolve_string());
-    GotoIfException(resolve, if_exception, var_exception);
+    CSA_ASSERT(this, IsCallable(resolve));
 
     Node* const result = CallJS(
         CodeFactory::Call(isolate(), ConvertReceiverMode::kNotNullOrUndefined),
@@ -2047,8 +2043,32 @@ Node* PromiseBuiltinsAssembler::PerformPromiseAll(
   TVARIABLE(Smi, var_index, SmiConstant(1));
   Label loop(this, &var_index), done_loop(this),
       too_many_elements(this, Label::kDeferred),
-      close_iterator(this, Label::kDeferred);
+      close_iterator(this, Label::kDeferred), if_slow(this, Label::kDeferred);
+
+  // We can skip the "resolve" lookup on {constructor} if it's the
+  // Promise constructor and the Promise.resolve protector is intact,
+  // as that guards the lookup path for the "resolve" property on the
+  // Promise constructor.
+  TVARIABLE(Object, var_promise_resolve_function, UndefinedConstant());
+  GotoIfNotPromiseResolveLookupChainIntact(native_context, constructor,
+                                           &if_slow);
   Goto(&loop);
+
+  BIND(&if_slow);
+  {
+    // 5. Let _promiseResolve_ be ? Get(_constructor_, `"resolve"`).
+    TNode<Object> resolve =
+        GetProperty(native_context, constructor, factory()->resolve_string());
+    GotoIfException(resolve, if_exception, var_exception);
+
+    // 6. If IsCallable(_promiseResolve_) is *false*, throw a *TypeError*
+    // exception.
+    ThrowIfNotCallable(CAST(context), resolve, "resolve");
+
+    var_promise_resolve_function = resolve;
+    Goto(&loop);
+  }
+
   BIND(&loop);
   {
     // Let next be IteratorStep(iteratorRecord.[[Iterator]]).
@@ -2120,8 +2140,7 @@ Node* PromiseBuiltinsAssembler::PerformPromiseAll(
     // the PromiseReaction (aka we can pass undefined to PerformPromiseThen),
     // since this is only necessary for DevTools and PromiseHooks.
     Label if_fast(this), if_slow(this);
-    GotoIfNotPromiseResolveLookupChainIntact(native_context, constructor,
-                                             &if_slow);
+    GotoIfNot(IsUndefined(var_promise_resolve_function.value()), &if_slow);
     GotoIf(IsPromiseHookEnabledOrDebugIsActiveOrHasAsyncEventDelegate(),
            &if_slow);
     GotoIf(IsPromiseSpeciesProtectorCellInvalid(), &if_slow);
@@ -2142,10 +2161,11 @@ Node* PromiseBuiltinsAssembler::PerformPromiseAll(
 
     BIND(&if_slow);
     {
-      // Let nextPromise be ? Invoke(constructor, "resolve", « nextValue »).
-      Node* const next_promise =
-          InvokeResolve(native_context, constructor, next_value,
-                        &close_iterator, var_exception);
+      // Let nextPromise be ? Call(constructor, _promiseResolve_, « nextValue
+      // »).
+      Node* const next_promise = CallResolve(
+          native_context, constructor, var_promise_resolve_function.value(),
+          next_value, &close_iterator, var_exception);
 
       // Perform ? Invoke(nextPromise, "then", « resolveElement,
       //                  resultCapability.[[Reject]] »).
@@ -2587,11 +2607,34 @@ TF_BUILTIN(PromiseRace, PromiseBuiltinsAssembler) {
 
   // Let result be PerformPromiseRace(iteratorRecord, C, promiseCapability).
   {
-    Label loop(this), break_loop(this);
+    // We can skip the "resolve" lookup on {constructor} if it's the
+    // Promise constructor and the Promise.resolve protector is intact,
+    // as that guards the lookup path for the "resolve" property on the
+    // Promise constructor.
+    Label loop(this), break_loop(this), if_slow(this, Label::kDeferred);
+    Node* const native_context = LoadNativeContext(context);
+    TVARIABLE(Object, var_promise_resolve_function, UndefinedConstant());
+    GotoIfNotPromiseResolveLookupChainIntact(native_context, receiver,
+                                             &if_slow);
     Goto(&loop);
+
+    BIND(&if_slow);
+    {
+      // 3. Let _promiseResolve_ be ? Get(_constructor_, `"resolve"`).
+      TNode<Object> resolve =
+          GetProperty(native_context, receiver, factory()->resolve_string());
+      GotoIfException(resolve, &reject_promise, &var_exception);
+
+      // 4. If IsCallable(_promiseResolve_) is *false*, throw a *TypeError*
+      // exception.
+      ThrowIfNotCallable(CAST(context), resolve, "resolve");
+
+      var_promise_resolve_function = resolve;
+      Goto(&loop);
+    }
+
     BIND(&loop);
     {
-      Node* const native_context = LoadNativeContext(context);
       Node* const fast_iterator_result_map = LoadContextElement(
           native_context, Context::ITERATOR_RESULT_MAP_INDEX);
 
@@ -2610,10 +2653,11 @@ TF_BUILTIN(PromiseRace, PromiseBuiltinsAssembler) {
           iter_assembler.IteratorValue(context, next, fast_iterator_result_map,
                                        &reject_promise, &var_exception);
 
-      // Let nextPromise be ? Invoke(constructor, "resolve", « nextValue »).
-      Node* const next_promise =
-          InvokeResolve(native_context, receiver, next_value, &close_iterator,
-                        &var_exception);
+      // Let nextPromise be ? Call(constructor, _promiseResolve_, « nextValue
+      // »).
+      Node* const next_promise = CallResolve(
+          native_context, receiver, var_promise_resolve_function.value(),
+          next_value, &close_iterator, &var_exception);
 
       // Perform ? Invoke(nextPromise, "then", « resolveElement,
       //                  resultCapability.[[Reject]] »).

src/builtins/builtins-promise-gen.h

@@ -115,8 +115,10 @@ class V8_EXPORT_PRIVATE PromiseBuiltinsAssembler : public CodeStubAssembler {
                                             Node* receiver_map, Label* if_fast,
                                             Label* if_slow);
 
-  Node* InvokeResolve(Node* native_context, Node* constructor, Node* value,
-                      Label* if_exception, Variable* var_exception);
+  // If resolve is Undefined, we use the builtin %PromiseResolve%
+  // intrinsic, otherwise we use the given resolve function.
+  Node* CallResolve(Node* native_context, Node* constructor, Node* resolve,
+                    Node* value, Label* if_exception, Variable* var_exception);
   template <typename... TArgs>
   Node* InvokeThen(Node* native_context, Node* receiver, TArgs... args);
 

src/code-stub-assembler.cc

@@ -5827,6 +5827,21 @@ Node* CodeStubAssembler::ThrowIfNotJSReceiver(Node* context, Node* value,
   return var_value_map.value();
 }
 
+void CodeStubAssembler::ThrowIfNotCallable(TNode<Context> context,
+                                           TNode<Object> value,
+                                           const char* method_name) {
+  Label out(this), throw_exception(this, Label::kDeferred);
+
+  GotoIf(TaggedIsSmi(value), &throw_exception);
+  Branch(IsCallable(CAST(value)), &out, &throw_exception);
+
+  // The {value} is not a compatible receiver for this method.
+  BIND(&throw_exception);
+  ThrowTypeError(context, MessageTemplate::kCalledNonCallable, method_name);
+
+  BIND(&out);
+}
+
 void CodeStubAssembler::ThrowRangeError(Node* context, MessageTemplate message,
                                         Node* arg0, Node* arg1, Node* arg2) {
   Node* template_index = SmiConstant(static_cast<int>(message));

src/code-stub-assembler.h

@@ -2114,6 +2114,8 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
   Node* ThrowIfNotJSReceiver(Node* context, Node* value,
                              MessageTemplate msg_template,
                              const char* method_name = nullptr);
+  void ThrowIfNotCallable(TNode<Context> context, TNode<Object> value,
+                          const char* method_name);
 
   void ThrowRangeError(Node* context, MessageTemplate message,
                        Node* arg0 = nullptr, Node* arg1 = nullptr,

test/mjsunit/promise-perform-all-resolve-lookup.js

@@ -0,0 +1,28 @@
+// Copyright 2019 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.
+//
+// Flags: --allow-natives-syntax
+
+let count = 0;
+class MyPromise extends Promise {
+  static get resolve() {
+    count++;
+    return super.resolve;
+  }
+}
+
+MyPromise.all([1, 2, 3, 4, 5]);
+assertEquals(1, count);
+%PerformMicrotaskCheckpoint();
+assertEquals(1, count);
+
+count = 0;
+MyPromise.all([
+  Promise.resolve(1),
+  Promise.resolve(2),
+  Promise.reject(3)
+]);
+assertEquals(1, count);
+%PerformMicrotaskCheckpoint();
+assertEquals(1, count);

test/mjsunit/promise-perform-all-settled-resolve-lookup.js

@@ -0,0 +1,28 @@
+// Copyright 2019 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.
+//
+// Flags: --allow-natives-syntax --harmony-promise-all-settled
+
+let count = 0;
+class MyPromise extends Promise {
+  static get resolve() {
+    count++;
+    return super.resolve;
+  }
+}
+
+MyPromise.allSettled([1, 2, 3, 4, 5]);
+assertEquals(1, count);
+%PerformMicrotaskCheckpoint();
+assertEquals(1, count);
+
+count = 0;
+MyPromise.allSettled([
+  Promise.resolve(1),
+  Promise.resolve(2),
+  Promise.reject(3)
+]);
+assertEquals(1, count);
+%PerformMicrotaskCheckpoint();
+assertEquals(1, count);

test/mjsunit/promise-perfrom-race-resolve-lookup.js

@@ -0,0 +1,28 @@
+// Copyright 2019 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.
+//
+// Flags: --allow-natives-syntax
+
+let count = 0;
+class MyPromise extends Promise {
+  static get resolve() {
+    count++;
+    return super.resolve;
+  }
+}
+
+MyPromise.race([1, 2, 3, 4, 5]);
+assertEquals(1, count);
+%PerformMicrotaskCheckpoint();
+assertEquals(1, count);
+
+count = 0;
+MyPromise.race([
+  Promise.resolve(1),
+  Promise.resolve(2),
+  Promise.reject(3)
+]);
+assertEquals(1, count);
+%PerformMicrotaskCheckpoint();
+assertEquals(1, count);

test/test262/test262.status

@@ -531,6 +531,16 @@
   # https://bugs.chromium.org/p/v8/issues/detail?id=9051
   'language/statements/async-generator/yield-star-return-then-getter-ticks': [FAIL],
 
+  # https://bugs.chromium.org/p/v8/issues/detail?id=9152
+  'built-ins/Promise/all/capability-executor-called-twice': [FAIL],
+  'built-ins/Promise/all/capability-resolve-throws-no-close': [FAIL],
+  'built-ins/Promise/all/capability-resolve-throws-reject': [FAIL],
+  'built-ins/Promise/all/invoke-resolve-get-error-close': [FAIL],
+  'built-ins/Promise/all/species-get-error': [FAIL],
+  'built-ins/Promise/race/capability-executor-called-twice': [FAIL],
+  'built-ins/Promise/race/invoke-resolve-get-error-close': [FAIL],
+  'built-ins/Promise/race/species-get-error': [FAIL],
+
   ######################## NEEDS INVESTIGATION ###########################
 
   # https://bugs.chromium.org/p/v8/issues/detail?id=7833