From 4995c85f287a3f25961236b252af6003217c7bed Mon Sep 17 00:00:00 2001 From: Benedikt Meurer Date: Sun, 28 Apr 2019 14:44:51 +0200 Subject: [PATCH] [runtime] Optimize general object spread. This adds a new %_CopyDataProperties intrinsic, that reuses most of the existing machinery that we already have in place for Object.assign() and computed property names in object literals. This speeds up the general case for object spread (where the spread is not the first item in an object literal) and brings it on par with Object.assign() at least - in most cases it's significantly faster than Object.assign(). In the test case [1] referenced from the bug, the performance goes from objectSpreadLast: 3624 ms. objectAssignLast: 1938 ms. to objectSpreadLast: 646 ms. objectAssignLast: 1944 ms. which corresponds to a **5-6x performance boost**, making object spread faster than Object.assign() in general. Drive-by-fix: This refactors the Object.assign() fast-path in a way that it can be reused appropriately for object spread, and adds another new builtin SetDataProperties, which does the core of the Object.assign() work. We can teach TurboFan to inline Object.assign() based on the new SetDataProperties builtin at some later point to further optimize Object.assign(). [1]: https://gist.github.com/bmeurer/0dae4a6b0e23f43d5a22d7c91476b6c0 Bug: v8:9167 Change-Id: I57bea7a8781c4a1e8ff3d394873c3cd4c5d73834 Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1587376 Reviewed-by: Sathya Gunasekaran Commit-Queue: Sathya Gunasekaran Auto-Submit: Benedikt Meurer Cr-Commit-Position: refs/heads/master@{#61100} --- src/builtins/builtins-definitions.h | 3 + src/builtins/builtins-internal-gen.cc | 107 ++++++++++++++++++ src/builtins/builtins-object-gen.cc | 64 +---------- src/compiler/js-intrinsic-lowering.cc | 6 + src/compiler/js-intrinsic-lowering.h | 1 + src/ic/keyed-store-generic.cc | 4 +- src/interpreter/bytecode-generator.cc | 2 +- .../interpreter-intrinsics-generator.cc | 7 ++ src/interpreter/interpreter-intrinsics.h | 1 + src/runtime/runtime.h | 2 +- test/mjsunit/es9/object-spread-basic.js | 48 +++++++- 11 files changed, 179 insertions(+), 66 deletions(-) diff --git a/src/builtins/builtins-definitions.h b/src/builtins/builtins-definitions.h index 47afff4dc1..da3397dbc4 100644 --- a/src/builtins/builtins-definitions.h +++ b/src/builtins/builtins-definitions.h @@ -263,6 +263,9 @@ namespace internal { /* Object property helpers */ \ TFS(HasProperty, kObject, kKey) \ TFS(DeleteProperty, kObject, kKey, kLanguageMode) \ + /* ES #sec-copydataproperties */ \ + TFS(CopyDataProperties, kTarget, kSource) \ + TFS(SetDataProperties, kTarget, kSource) \ \ /* Abort */ \ TFC(Abort, Abort) \ diff --git a/src/builtins/builtins-internal-gen.cc b/src/builtins/builtins-internal-gen.cc index 7b195e6237..a5ed4708f1 100644 --- a/src/builtins/builtins-internal-gen.cc +++ b/src/builtins/builtins-internal-gen.cc @@ -599,6 +599,113 @@ TF_BUILTIN(DeleteProperty, DeletePropertyBaseAssembler) { } } +namespace { + +class SetOrCopyDataPropertiesAssembler : public CodeStubAssembler { + public: + explicit SetOrCopyDataPropertiesAssembler(compiler::CodeAssemblerState* state) + : CodeStubAssembler(state) {} + + protected: + TNode SetOrCopyDataProperties(TNode context, + TNode target, + TNode source, Label* if_runtime, + bool use_set = true) { + Label if_done(this), if_noelements(this), + if_sourcenotjsobject(this, Label::kDeferred); + + // JSValue wrappers for numbers don't have any enumerable own properties, + // so we can immediately skip the whole operation if {source} is a Smi. + GotoIf(TaggedIsSmi(source), &if_done); + + // Otherwise check if {source} is a proper JSObject, and if not, defer + // to testing for non-empty strings below. + TNode source_map = LoadMap(CAST(source)); + TNode source_instance_type = LoadMapInstanceType(source_map); + GotoIfNot(IsJSObjectInstanceType(source_instance_type), + &if_sourcenotjsobject); + + TNode source_elements = LoadElements(CAST(source)); + GotoIf(IsEmptyFixedArray(source_elements), &if_noelements); + Branch(IsEmptySlowElementDictionary(source_elements), &if_noelements, + if_runtime); + + BIND(&if_noelements); + { + // If the target is deprecated, the object will be updated on first store. + // If the source for that store equals the target, this will invalidate + // the cached representation of the source. Handle this case in runtime. + TNode target_map = LoadMap(target); + GotoIf(IsDeprecatedMap(target_map), if_runtime); + + if (use_set) { + TNode target_is_simple_receiver = IsSimpleObjectMap(target_map); + ForEachEnumerableOwnProperty( + context, source_map, CAST(source), kEnumerationOrder, + [=](TNode key, TNode value) { + KeyedStoreGenericGenerator::SetProperty( + state(), context, target, target_is_simple_receiver, key, + value, LanguageMode::kStrict); + }, + if_runtime); + } else { + ForEachEnumerableOwnProperty( + context, source_map, CAST(source), kEnumerationOrder, + [=](TNode key, TNode value) { + CallBuiltin(Builtins::kSetPropertyInLiteral, context, target, key, + value); + }, + if_runtime); + } + Goto(&if_done); + } + + BIND(&if_sourcenotjsobject); + { + // Handle other JSReceivers in the runtime. + GotoIf(IsJSReceiverInstanceType(source_instance_type), if_runtime); + + // Non-empty strings are the only non-JSReceivers that need to be + // handled explicitly by Object.assign() and CopyDataProperties. + GotoIfNot(IsStringInstanceType(source_instance_type), &if_done); + TNode source_length = LoadStringLengthAsWord(CAST(source)); + Branch(WordEqual(source_length, IntPtrConstant(0)), &if_done, if_runtime); + } + + BIND(&if_done); + return UndefinedConstant(); + } +}; + +} // namespace + +// ES #sec-copydataproperties +TF_BUILTIN(CopyDataProperties, SetOrCopyDataPropertiesAssembler) { + TNode target = CAST(Parameter(Descriptor::kTarget)); + TNode source = CAST(Parameter(Descriptor::kSource)); + TNode context = CAST(Parameter(Descriptor::kContext)); + + CSA_ASSERT(this, WordNotEqual(target, source)); + + Label if_runtime(this, Label::kDeferred); + Return(SetOrCopyDataProperties(context, target, source, &if_runtime, false)); + + BIND(&if_runtime); + TailCallRuntime(Runtime::kCopyDataProperties, context, target, source); +} + +TF_BUILTIN(SetDataProperties, SetOrCopyDataPropertiesAssembler) { + TNode target = CAST(Parameter(Descriptor::kTarget)); + TNode source = CAST(Parameter(Descriptor::kSource)); + TNode context = CAST(Parameter(Descriptor::kContext)); + + Label if_runtime(this, Label::kDeferred); + Return(SetOrCopyDataProperties(context, target, source, &if_runtime, true)); + + BIND(&if_runtime); + TailCallRuntime(Runtime::kSetDataProperties, context, target, source); +} + TF_BUILTIN(ForInEnumerate, CodeStubAssembler) { Node* receiver = Parameter(Descriptor::kReceiver); Node* context = Parameter(Descriptor::kContext); diff --git a/src/builtins/builtins-object-gen.cc b/src/builtins/builtins-object-gen.cc index da265356fd..f4adcb08e3 100644 --- a/src/builtins/builtins-object-gen.cc +++ b/src/builtins/builtins-object-gen.cc @@ -48,9 +48,6 @@ class ObjectBuiltinsAssembler : public CodeStubAssembler { Node* IsSpecialReceiverMap(SloppyTNode map); TNode IsStringWrapperElementsKind(TNode map); - - void ObjectAssignFast(TNode context, TNode to, - TNode from, Label* slow); }; class ObjectEntriesValuesBuiltinsAssembler : public ObjectBuiltinsAssembler { @@ -499,18 +496,8 @@ TF_BUILTIN(ObjectAssign, ObjectBuiltinsAssembler) { // second argument. // 4. For each element nextSource of sources, in ascending index order, args.ForEach( - [=](Node* next_source_) { - TNode next_source = CAST(next_source_); - Label slow(this), cont(this); - ObjectAssignFast(context, to, next_source, &slow); - Goto(&cont); - - BIND(&slow); - { - CallRuntime(Runtime::kSetDataProperties, context, to, next_source); - Goto(&cont); - } - BIND(&cont); + [=](Node* next_source) { + CallBuiltin(Builtins::kSetDataProperties, context, to, next_source); }, IntPtrConstant(1)); Goto(&done); @@ -520,53 +507,6 @@ TF_BUILTIN(ObjectAssign, ObjectBuiltinsAssembler) { args.PopAndReturn(to); } -// This function mimics what FastAssign() function does for C++ implementation. -void ObjectBuiltinsAssembler::ObjectAssignFast(TNode context, - TNode to, - TNode from, - Label* slow) { - Label done(this); - - // Non-empty strings are the only non-JSReceivers that need to be handled - // explicitly by Object.assign. - GotoIf(TaggedIsSmi(from), &done); - TNode from_map = LoadMap(CAST(from)); - TNode from_instance_type = LoadMapInstanceType(from_map); - { - Label cont(this); - GotoIf(IsJSReceiverInstanceType(from_instance_type), &cont); - GotoIfNot(IsStringInstanceType(from_instance_type), &done); - { - Branch( - Word32Equal(LoadStringLengthAsWord32(CAST(from)), Int32Constant(0)), - &done, slow); - } - BIND(&cont); - } - - // If the target is deprecated, the object will be updated on first store. If - // the source for that store equals the target, this will invalidate the - // cached representation of the source. Handle this case in runtime. - TNode to_map = LoadMap(to); - GotoIf(IsDeprecatedMap(to_map), slow); - TNode to_is_simple_receiver = IsSimpleObjectMap(to_map); - - GotoIfNot(IsJSObjectInstanceType(from_instance_type), slow); - GotoIfNot(IsEmptyFixedArray(LoadElements(CAST(from))), slow); - - ForEachEnumerableOwnProperty( - context, from_map, CAST(from), kEnumerationOrder, - [=](TNode key, TNode value) { - KeyedStoreGenericGenerator::SetProperty(state(), context, to, - to_is_simple_receiver, key, - value, LanguageMode::kStrict); - }, - slow); - - Goto(&done); - BIND(&done); -} - // ES #sec-object.keys TF_BUILTIN(ObjectKeys, ObjectBuiltinsAssembler) { Node* object = Parameter(Descriptor::kObject); diff --git a/src/compiler/js-intrinsic-lowering.cc b/src/compiler/js-intrinsic-lowering.cc index f5ae23a746..e37757fb32 100644 --- a/src/compiler/js-intrinsic-lowering.cc +++ b/src/compiler/js-intrinsic-lowering.cc @@ -30,6 +30,8 @@ Reduction JSIntrinsicLowering::Reduce(Node* node) { Runtime::FunctionForId(CallRuntimeParametersOf(node->op()).id()); if (f->intrinsic_type != Runtime::IntrinsicType::INLINE) return NoChange(); switch (f->function_id) { + case Runtime::kInlineCopyDataProperties: + return ReduceCopyDataProperties(node); case Runtime::kInlineCreateIterResultObject: return ReduceCreateIterResultObject(node); case Runtime::kInlineDeoptimizeNow: @@ -84,6 +86,10 @@ Reduction JSIntrinsicLowering::Reduce(Node* node) { return NoChange(); } +Reduction JSIntrinsicLowering::ReduceCopyDataProperties(Node* node) { + return Change( + node, Builtins::CallableFor(isolate(), Builtins::kCopyDataProperties), 0); +} Reduction JSIntrinsicLowering::ReduceCreateIterResultObject(Node* node) { Node* const value = NodeProperties::GetValueInput(node, 0); diff --git a/src/compiler/js-intrinsic-lowering.h b/src/compiler/js-intrinsic-lowering.h index 8444bdcee1..358e1f327b 100644 --- a/src/compiler/js-intrinsic-lowering.h +++ b/src/compiler/js-intrinsic-lowering.h @@ -39,6 +39,7 @@ class V8_EXPORT_PRIVATE JSIntrinsicLowering final Reduction Reduce(Node* node) final; private: + Reduction ReduceCopyDataProperties(Node* node); Reduction ReduceCreateIterResultObject(Node* node); Reduction ReduceDeoptimizeNow(Node* node); Reduction ReduceCreateJSGeneratorObject(Node* node); diff --git a/src/ic/keyed-store-generic.cc b/src/ic/keyed-store-generic.cc index 03f9d91d4f..7e8574ba0d 100644 --- a/src/ic/keyed-store-generic.cc +++ b/src/ic/keyed-store-generic.cc @@ -851,7 +851,9 @@ void KeyedStoreGenericAssembler::EmitGenericPropertyStore( var_accessor_holder.Bind(receiver); Goto(&accessor); } else { - Goto(&overwrite); + // We must reconfigure an accessor property to a data property + // here, let the runtime take care of that. + Goto(slow); } BIND(&overwrite); diff --git a/src/interpreter/bytecode-generator.cc b/src/interpreter/bytecode-generator.cc index 3f44282e7f..b2fb4cdec0 100644 --- a/src/interpreter/bytecode-generator.cc +++ b/src/interpreter/bytecode-generator.cc @@ -2504,7 +2504,7 @@ void BytecodeGenerator::VisitObjectLiteral(ObjectLiteral* expr) { builder()->MoveRegister(literal, args[0]); builder()->SetExpressionPosition(property->value()); VisitForRegisterValue(property->value(), args[1]); - builder()->CallRuntime(Runtime::kCopyDataProperties, args); + builder()->CallRuntime(Runtime::kInlineCopyDataProperties, args); break; } case ObjectLiteral::Property::PROTOTYPE: diff --git a/src/interpreter/interpreter-intrinsics-generator.cc b/src/interpreter/interpreter-intrinsics-generator.cc index f90e7f60d3..b83374684f 100644 --- a/src/interpreter/interpreter-intrinsics-generator.cc +++ b/src/interpreter/interpreter-intrinsics-generator.cc @@ -194,6 +194,13 @@ Node* IntrinsicsGenerator::IntrinsicAsBuiltinCall( return IntrinsicAsStubCall(args, context, callable); } +Node* IntrinsicsGenerator::CopyDataProperties( + const InterpreterAssembler::RegListNodePair& args, Node* context) { + return IntrinsicAsStubCall( + args, context, + Builtins::CallableFor(isolate(), Builtins::kCopyDataProperties)); +} + Node* IntrinsicsGenerator::CreateIterResultObject( const InterpreterAssembler::RegListNodePair& args, Node* context) { return IntrinsicAsStubCall( diff --git a/src/interpreter/interpreter-intrinsics.h b/src/interpreter/interpreter-intrinsics.h index 2f944c1ee7..38010cdee6 100644 --- a/src/interpreter/interpreter-intrinsics.h +++ b/src/interpreter/interpreter-intrinsics.h @@ -29,6 +29,7 @@ namespace interpreter { V(GeneratorClose, generator_close, 1) \ V(GetImportMetaObject, get_import_meta_object, 0) \ V(Call, call, -1) \ + V(CopyDataProperties, copy_data_properties, 2) \ V(CreateIterResultObject, create_iter_result_object, 2) \ V(CreateAsyncFromSyncIterator, create_async_from_sync_iterator, 1) \ V(HasProperty, has_property, 2) \ diff --git a/src/runtime/runtime.h b/src/runtime/runtime.h index 1c34d30a09..13544da074 100644 --- a/src/runtime/runtime.h +++ b/src/runtime/runtime.h @@ -285,7 +285,7 @@ namespace internal { F(ClassOf, 1, 1) \ F(CollectTypeProfile, 3, 1) \ F(CompleteInobjectSlackTrackingForMap, 1, 1) \ - F(CopyDataProperties, 2, 1) \ + I(CopyDataProperties, 2, 1) \ F(CopyDataPropertiesWithExcludedProperties, -1 /* >= 1 */, 1) \ I(CreateDataProperty, 3, 1) \ I(CreateIterResultObject, 2, 1) \ diff --git a/test/mjsunit/es9/object-spread-basic.js b/test/mjsunit/es9/object-spread-basic.js index 8264da47a5..a0769b3a66 100644 --- a/test/mjsunit/es9/object-spread-basic.js +++ b/test/mjsunit/es9/object-spread-basic.js @@ -104,6 +104,52 @@ assertEquals(z, y = { ...p }); var x = { a:1 }; assertEquals(x, y = { set a(_) { throw new Error(); }, ...x }); +var prop = Object.getOwnPropertyDescriptor(y, 'a'); +assertEquals(prop.value, 1); +assertFalse("set" in prop); +assertTrue(prop.enumerable); +assertTrue(prop.configurable); +assertTrue(prop.writable); -var x = { a:1 }; +var x = { a:2 }; assertEquals(x, y = { get a() { throw new Error(); }, ...x }); +var prop = Object.getOwnPropertyDescriptor(y, 'a'); +assertEquals(prop.value, 2); +assertFalse("get" in prop); +assertTrue(prop.enumerable); +assertTrue(prop.configurable); +assertTrue(prop.writable); + +var x = { a:3 }; +assertEquals(x, y = { + get a() { + throw new Error(); + }, + set a(_) { + throw new Error(); + }, + ...x +}); +var prop = Object.getOwnPropertyDescriptor(y, 'a'); +assertEquals(prop.value, 3); +assertFalse("get" in prop); +assertFalse("set" in prop); +assertTrue(prop.enumerable); +assertTrue(prop.configurable); +assertTrue(prop.writable); + +var x = Object.seal({ a:4 }); +assertEquals(x, y = { ...x }); +var prop = Object.getOwnPropertyDescriptor(y, 'a'); +assertEquals(prop.value, 4); +assertTrue(prop.enumerable); +assertTrue(prop.configurable); +assertTrue(prop.writable); + +var x = Object.freeze({ a:5 }); +assertEquals(x, y = { ...x }); +var prop = Object.getOwnPropertyDescriptor(y, 'a'); +assertEquals(prop.value, 5); +assertTrue(prop.enumerable); +assertTrue(prop.configurable); +assertTrue(prop.writable);