From 9c0c876129871b6c7c877acda40c2a9503aef22d Mon Sep 17 00:00:00 2001 From: Sathya Gunasekaran Date: Tue, 23 Apr 2019 09:57:59 -0700 Subject: [PATCH] [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 Reviewed-by: Jakob Gruber Cr-Commit-Position: refs/heads/master@{#60957} --- src/builtins/builtins-promise-gen.cc | 98 ++++++++++++++----- src/builtins/builtins-promise-gen.h | 6 +- src/code-stub-assembler.cc | 15 +++ src/code-stub-assembler.h | 2 + .../promise-perform-all-resolve-lookup.js | 28 ++++++ ...mise-perform-all-settled-resolve-lookup.js | 28 ++++++ .../promise-perfrom-race-resolve-lookup.js | 28 ++++++ test/test262/test262.status | 10 ++ 8 files changed, 186 insertions(+), 29 deletions(-) create mode 100644 test/mjsunit/promise-perform-all-resolve-lookup.js create mode 100644 test/mjsunit/promise-perform-all-settled-resolve-lookup.js create mode 100644 test/mjsunit/promise-perfrom-race-resolve-lookup.js diff --git a/src/builtins/builtins-promise-gen.cc b/src/builtins/builtins-promise-gen.cc index 513a595822..aa55671a40 100644 --- a/src/builtins/builtins-promise-gen.cc +++ b/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 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 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]] »). diff --git a/src/builtins/builtins-promise-gen.h b/src/builtins/builtins-promise-gen.h index b0555b2594..39bc34e9dc 100644 --- a/src/builtins/builtins-promise-gen.h +++ b/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 Node* InvokeThen(Node* native_context, Node* receiver, TArgs... args); diff --git a/src/code-stub-assembler.cc b/src/code-stub-assembler.cc index fe65191016..13ee6a5a93 100644 --- a/src/code-stub-assembler.cc +++ b/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, + TNode 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(message)); diff --git a/src/code-stub-assembler.h b/src/code-stub-assembler.h index bb85ab541a..fcf5111cc4 100644 --- a/src/code-stub-assembler.h +++ b/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, TNode value, + const char* method_name); void ThrowRangeError(Node* context, MessageTemplate message, Node* arg0 = nullptr, Node* arg1 = nullptr, diff --git a/test/mjsunit/promise-perform-all-resolve-lookup.js b/test/mjsunit/promise-perform-all-resolve-lookup.js new file mode 100644 index 0000000000..8e877df63b --- /dev/null +++ b/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); diff --git a/test/mjsunit/promise-perform-all-settled-resolve-lookup.js b/test/mjsunit/promise-perform-all-settled-resolve-lookup.js new file mode 100644 index 0000000000..a2f5f01837 --- /dev/null +++ b/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); diff --git a/test/mjsunit/promise-perfrom-race-resolve-lookup.js b/test/mjsunit/promise-perfrom-race-resolve-lookup.js new file mode 100644 index 0000000000..72c9c401e1 --- /dev/null +++ b/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); diff --git a/test/test262/test262.status b/test/test262/test262.status index 5b45a6eced..d7ff4bd86c 100644 --- a/test/test262/test262.status +++ b/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