From 4213af64b6b9c9d04be89303169562460d38df84 Mon Sep 17 00:00:00 2001 From: Benedikt Meurer Date: Mon, 16 Oct 2017 18:48:11 +0200 Subject: [PATCH] [es2015] Optimize Reflect.has builtin. Port the baseline version of Reflect.has to the CodeStubAssembler and reuse the existing logic for HasProperty (i.e. the HasProperty builtin). Also inline the Reflect.has builtin into TurboFan, by adding a check on the target in front of a use of the JSHasProperty operator. Technically this additional check is not necessary, because the JSHasProperty operator already throws if the target is not a JSReceiver, but the exception message is confusing then. This improves the performance of the micro-benchmark from reflectHasPresent: 337 ms. reflectHasAbsent: 472 ms. to reflectHasPresent: 121 ms. reflectHasAbsent: 216 ms. which is a nice 2.8x improvement in the best case. It also improves the chai test on the web-tooling-benchmark by around 1-2%, which is roughly the expected win (since Reflect.has overall accounts for around 3-4%). Bug: v8:5996, v8:6936, v8:6937 Change-Id: I856183229677a71c19936f06f2a4fc7a794a9a4a Reviewed-on: https://chromium-review.googlesource.com/720959 Commit-Queue: Benedikt Meurer Reviewed-by: Jaroslav Sevcik Cr-Commit-Position: refs/heads/master@{#48608} --- BUILD.gn | 1 + src/builtins/builtins-definitions.h | 2 +- src/builtins/builtins-promise-gen.cc | 22 --------- src/builtins/builtins-promise-gen.h | 4 -- src/builtins/builtins-reflect-gen.cc | 25 ++++++++++ src/builtins/builtins-reflect.cc | 24 ---------- src/code-stub-assembler.cc | 22 +++++++++ src/code-stub-assembler.h | 5 ++ src/compiler/js-call-reducer.cc | 72 ++++++++++++++++++++++++++++ src/compiler/js-call-reducer.h | 1 + src/v8.gyp | 1 + test/mjsunit/compiler/reflect-has.js | 67 ++++++++++++++++++++++++++ 12 files changed, 195 insertions(+), 51 deletions(-) create mode 100644 src/builtins/builtins-reflect-gen.cc create mode 100644 test/mjsunit/compiler/reflect-has.js diff --git a/BUILD.gn b/BUILD.gn index f31f7e541e..d6450f9c10 100644 --- a/BUILD.gn +++ b/BUILD.gn @@ -1016,6 +1016,7 @@ v8_source_set("v8_initializers") { "src/builtins/builtins-promise-gen.h", "src/builtins/builtins-proxy-gen.cc", "src/builtins/builtins-proxy-gen.h", + "src/builtins/builtins-reflect-gen.cc", "src/builtins/builtins-regexp-gen.cc", "src/builtins/builtins-regexp-gen.h", "src/builtins/builtins-sharedarraybuffer-gen.cc", diff --git a/src/builtins/builtins-definitions.h b/src/builtins/builtins-definitions.h index 67d54a2c1a..eaba3208ed 100644 --- a/src/builtins/builtins-definitions.h +++ b/src/builtins/builtins-definitions.h @@ -795,7 +795,7 @@ namespace internal { CPP(ReflectGet) \ CPP(ReflectGetOwnPropertyDescriptor) \ CPP(ReflectGetPrototypeOf) \ - CPP(ReflectHas) \ + TFJ(ReflectHas, 2, kTarget, kKey) \ CPP(ReflectIsExtensible) \ CPP(ReflectOwnKeys) \ CPP(ReflectPreventExtensions) \ diff --git a/src/builtins/builtins-promise-gen.cc b/src/builtins/builtins-promise-gen.cc index 618a8d38d5..7ea0da7f77 100644 --- a/src/builtins/builtins-promise-gen.cc +++ b/src/builtins/builtins-promise-gen.cc @@ -237,28 +237,6 @@ Node* PromiseBuiltinsAssembler::CreatePromiseGetCapabilitiesExecutorContext( return context; } -Node* PromiseBuiltinsAssembler::ThrowIfNotJSReceiver( - Node* context, Node* value, MessageTemplate::Template msg_template, - const char* method_name) { - Label out(this), throw_exception(this, Label::kDeferred); - VARIABLE(var_value_map, MachineRepresentation::kTagged); - - GotoIf(TaggedIsSmi(value), &throw_exception); - - // Load the instance type of the {value}. - var_value_map.Bind(LoadMap(value)); - Node* const value_instance_type = LoadMapInstanceType(var_value_map.value()); - - Branch(IsJSReceiverInstanceType(value_instance_type), &out, &throw_exception); - - // The {value} is not a compatible receiver for this method. - BIND(&throw_exception); - ThrowTypeError(context, msg_template, method_name); - - BIND(&out); - return var_value_map.value(); -} - Node* PromiseBuiltinsAssembler::PromiseHasHandler(Node* promise) { Node* const flags = LoadObjectField(promise, JSPromise::kFlagsOffset); return IsSetWord(SmiUntag(flags), 1 << JSPromise::kHasHandlerBit); diff --git a/src/builtins/builtins-promise-gen.h b/src/builtins/builtins-promise-gen.h index c2cadecfd2..e47e6ede84 100644 --- a/src/builtins/builtins-promise-gen.h +++ b/src/builtins/builtins-promise-gen.h @@ -112,10 +112,6 @@ class PromiseBuiltinsAssembler : public CodeStubAssembler { protected: void PromiseInit(Node* promise); - Node* ThrowIfNotJSReceiver(Node* context, Node* value, - MessageTemplate::Template msg_template, - const char* method_name = nullptr); - Node* SpeciesConstructor(Node* context, Node* object, Node* default_constructor); diff --git a/src/builtins/builtins-reflect-gen.cc b/src/builtins/builtins-reflect-gen.cc new file mode 100644 index 0000000000..3ab21f975d --- /dev/null +++ b/src/builtins/builtins-reflect-gen.cc @@ -0,0 +1,25 @@ +// Copyright 2017 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. + +#include "src/builtins/builtins-utils-gen.h" +#include "src/builtins/builtins.h" +#include "src/code-stub-assembler.h" + +namespace v8 { +namespace internal { + +// ES section #sec-reflect.has +TF_BUILTIN(ReflectHas, CodeStubAssembler) { + Node* target = Parameter(Descriptor::kTarget); + Node* key = Parameter(Descriptor::kKey); + Node* context = Parameter(Descriptor::kContext); + + ThrowIfNotJSReceiver(context, target, MessageTemplate::kCalledOnNonObject, + "Reflect.has"); + + Return(CallBuiltin(Builtins::kHasProperty, context, key, target)); +} + +} // namespace internal +} // namespace v8 diff --git a/src/builtins/builtins-reflect.cc b/src/builtins/builtins-reflect.cc index 9c559ab384..db44803278 100644 --- a/src/builtins/builtins-reflect.cc +++ b/src/builtins/builtins-reflect.cc @@ -138,30 +138,6 @@ BUILTIN(ReflectGetPrototypeOf) { JSReceiver::GetPrototype(isolate, receiver)); } -// ES6 section 26.1.9 Reflect.has -BUILTIN(ReflectHas) { - HandleScope scope(isolate); - DCHECK_EQ(3, args.length()); - Handle target = args.at(1); - Handle key = args.at(2); - - if (!target->IsJSReceiver()) { - THROW_NEW_ERROR_RETURN_FAILURE( - isolate, NewTypeError(MessageTemplate::kCalledOnNonObject, - isolate->factory()->NewStringFromAsciiChecked( - "Reflect.has"))); - } - - Handle name; - ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, name, - Object::ToName(isolate, key)); - - Maybe result = - JSReceiver::HasProperty(Handle::cast(target), name); - return result.IsJust() ? *isolate->factory()->ToBoolean(result.FromJust()) - : isolate->heap()->exception(); -} - // ES6 section 26.1.10 Reflect.isExtensible BUILTIN(ReflectIsExtensible) { HandleScope scope(isolate); diff --git a/src/code-stub-assembler.cc b/src/code-stub-assembler.cc index eb12564dd7..5008bab736 100644 --- a/src/code-stub-assembler.cc +++ b/src/code-stub-assembler.cc @@ -3533,6 +3533,28 @@ Node* CodeStubAssembler::ThrowIfNotInstanceType(Node* context, Node* value, return var_value_map.value(); } +Node* CodeStubAssembler::ThrowIfNotJSReceiver( + Node* context, Node* value, MessageTemplate::Template msg_template, + const char* method_name) { + Label out(this), throw_exception(this, Label::kDeferred); + VARIABLE(var_value_map, MachineRepresentation::kTagged); + + GotoIf(TaggedIsSmi(value), &throw_exception); + + // Load the instance type of the {value}. + var_value_map.Bind(LoadMap(value)); + Node* const value_instance_type = LoadMapInstanceType(var_value_map.value()); + + Branch(IsJSReceiverInstanceType(value_instance_type), &out, &throw_exception); + + // The {value} is not a compatible receiver for this method. + BIND(&throw_exception); + ThrowTypeError(context, msg_template, method_name); + + BIND(&out); + return var_value_map.value(); +} + void CodeStubAssembler::ThrowTypeError(Node* context, MessageTemplate::Template message, char const* arg0, char const* arg1) { diff --git a/src/code-stub-assembler.h b/src/code-stub-assembler.h index 91af2cc8a0..5f3e59c903 100644 --- a/src/code-stub-assembler.h +++ b/src/code-stub-assembler.h @@ -867,6 +867,11 @@ class V8_EXPORT_PRIVATE CodeStubAssembler : public compiler::CodeAssembler { Node* ThrowIfNotInstanceType(Node* context, Node* value, InstanceType instance_type, char const* method_name); + // Throws a TypeError for {method_name} if {value} is not a JSReceiver. + // Returns the {value}'s map. + Node* ThrowIfNotJSReceiver(Node* context, Node* value, + MessageTemplate::Template msg_template, + const char* method_name = nullptr); void ThrowTypeError(Node* context, MessageTemplate::Template message, char const* arg0 = nullptr, char const* arg1 = nullptr); void ThrowTypeError(Node* context, MessageTemplate::Template message, diff --git a/src/compiler/js-call-reducer.cc b/src/compiler/js-call-reducer.cc index 8fec6893d9..8568ae22cd 100644 --- a/src/compiler/js-call-reducer.cc +++ b/src/compiler/js-call-reducer.cc @@ -590,6 +590,76 @@ Reduction JSCallReducer::ReduceReflectGetPrototypeOf(Node* node) { return ReduceObjectGetPrototype(node, target); } +// ES section #sec-reflect.has +Reduction JSCallReducer::ReduceReflectHas(Node* node) { + DCHECK_EQ(IrOpcode::kJSCall, node->opcode()); + CallParameters const& p = CallParametersOf(node->op()); + int arity = static_cast(p.arity() - 2); + DCHECK_LE(0, arity); + Node* target = (arity >= 1) ? NodeProperties::GetValueInput(node, 2) + : jsgraph()->UndefinedConstant(); + Node* key = (arity >= 2) ? NodeProperties::GetValueInput(node, 3) + : jsgraph()->UndefinedConstant(); + Node* context = NodeProperties::GetContextInput(node); + Node* frame_state = NodeProperties::GetFrameStateInput(node); + Node* effect = NodeProperties::GetEffectInput(node); + Node* control = NodeProperties::GetControlInput(node); + + // Check whether {target} is a JSReceiver. + Node* check = graph()->NewNode(simplified()->ObjectIsReceiver(), target); + Node* branch = + graph()->NewNode(common()->Branch(BranchHint::kTrue), check, control); + + // Throw an appropriate TypeError if the {target} is not a JSReceiver. + Node* if_false = graph()->NewNode(common()->IfFalse(), branch); + Node* efalse = effect; + { + if_false = efalse = graph()->NewNode( + javascript()->CallRuntime(Runtime::kThrowTypeError, 2), + jsgraph()->Constant(MessageTemplate::kCalledOnNonObject), + jsgraph()->HeapConstant( + factory()->NewStringFromAsciiChecked("Reflect.has")), + context, frame_state, efalse, if_false); + } + + // Otherwise just use the existing {JSHasProperty} logic. + Node* if_true = graph()->NewNode(common()->IfTrue(), branch); + Node* etrue = effect; + Node* vtrue; + { + vtrue = etrue = if_true = + graph()->NewNode(javascript()->HasProperty(), key, target, context, + frame_state, etrue, if_true); + } + + // Rewire potential exception edges. + Node* on_exception = nullptr; + if (NodeProperties::IsExceptionalCall(node, &on_exception)) { + // Create appropriate {IfException} and {IfSuccess} nodes. + Node* extrue = graph()->NewNode(common()->IfException(), etrue, if_true); + if_true = graph()->NewNode(common()->IfSuccess(), if_true); + Node* exfalse = graph()->NewNode(common()->IfException(), efalse, if_false); + if_false = graph()->NewNode(common()->IfSuccess(), if_false); + + // Join the exception edges. + Node* merge = graph()->NewNode(common()->Merge(2), extrue, exfalse); + Node* ephi = + graph()->NewNode(common()->EffectPhi(2), extrue, exfalse, merge); + Node* phi = + graph()->NewNode(common()->Phi(MachineRepresentation::kTagged, 2), + extrue, exfalse, merge); + ReplaceWithValue(on_exception, phi, ephi, merge); + } + + // Connect the throwing path to end. + if_false = graph()->NewNode(common()->Throw(), efalse, if_false); + NodeProperties::MergeControlToEnd(graph(), common(), if_false); + + // Continue on the regular path. + ReplaceWithValue(node, vtrue, etrue, if_true); + return Changed(vtrue); +} + bool CanInlineArrayIteratingBuiltin(Handle receiver_map) { Isolate* const isolate = receiver_map->GetIsolate(); if (!receiver_map->prototype()->IsJSArray()) return false; @@ -1436,6 +1506,8 @@ Reduction JSCallReducer::ReduceJSCall(Node* node) { return ReduceReflectConstruct(node); case Builtins::kReflectGetPrototypeOf: return ReduceReflectGetPrototypeOf(node); + case Builtins::kReflectHas: + return ReduceReflectHas(node); case Builtins::kArrayForEach: return ReduceArrayForEach(function, node); case Builtins::kArrayMap: diff --git a/src/compiler/js-call-reducer.h b/src/compiler/js-call-reducer.h index 3fce912fde..b7e54af7dc 100644 --- a/src/compiler/js-call-reducer.h +++ b/src/compiler/js-call-reducer.h @@ -69,6 +69,7 @@ class JSCallReducer final : public AdvancedReducer { Reduction ReduceReflectApply(Node* node); Reduction ReduceReflectConstruct(Node* node); Reduction ReduceReflectGetPrototypeOf(Node* node); + Reduction ReduceReflectHas(Node* node); Reduction ReduceArrayForEach(Handle function, Node* node); Reduction ReduceArrayMap(Handle function, Node* node); Reduction ReduceCallOrConstructWithArrayLikeOrSpread( diff --git a/src/v8.gyp b/src/v8.gyp index f3d6eb1557..2f9f40a23a 100644 --- a/src/v8.gyp +++ b/src/v8.gyp @@ -206,6 +206,7 @@ 'builtins/builtins-promise-gen.h', 'builtins/builtins-proxy-gen.cc', 'builtins/builtins-proxy-gen.h', + 'builtins/builtins-reflect-gen.cc', 'builtins/builtins-regexp-gen.cc', 'builtins/builtins-regexp-gen.h', 'builtins/builtins-sharedarraybuffer-gen.cc', diff --git a/test/mjsunit/compiler/reflect-has.js b/test/mjsunit/compiler/reflect-has.js new file mode 100644 index 0000000000..2f9ee1b66a --- /dev/null +++ b/test/mjsunit/compiler/reflect-has.js @@ -0,0 +1,67 @@ +// Copyright 2017 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 + +// Test Reflect.has with wrong (number of) arguments. +(function() { + "use strict"; + function foo() { return Reflect.has(); } + + assertThrows(foo); + assertThrows(foo); + %OptimizeFunctionOnNextCall(foo); + assertThrows(foo); +})(); +(function() { + "use strict"; + function foo(o) { return Reflect.has(o); } + + assertFalse(foo({})); + assertFalse(foo({})); + %OptimizeFunctionOnNextCall(foo); + assertFalse(foo({})); +})(); +(function() { + "use strict"; + function foo(o) { return Reflect.has(o); } + + assertThrows(foo.bind(undefined, 1)); + assertThrows(foo.bind(undefined, undefined)); + %OptimizeFunctionOnNextCall(foo); + assertThrows(foo.bind(undefined, 'o')); +})(); + +// Test Reflect.has within try/catch. +(function() { + "use strict"; + function foo() { + try { + return Reflect.has(); + } catch (e) { + return 1; + } + } + + assertEquals(1, foo()); + assertEquals(1, foo()); + %OptimizeFunctionOnNextCall(foo); + assertEquals(1, foo()); +})(); +(function() { + "use strict"; + const o = {}; + function foo(n) { + try { + return Reflect.has(o, n); + } catch (e) { + return 1; + } + } + + assertEquals(1, foo({[Symbol.toPrimitive]() { throw new Error(); }})); + assertEquals(1, foo({[Symbol.toPrimitive]() { throw new Error(); }})); + %OptimizeFunctionOnNextCall(foo); + assertEquals(1, foo({[Symbol.toPrimitive]() { throw new Error(); }})); +})();