[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 <gsathya@chromium.org>
Commit-Queue: Sathya Gunasekaran <gsathya@chromium.org>
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#61100}

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)                                                            \

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<Object> SetOrCopyDataProperties(TNode<Context> context,
+                                        TNode<JSReceiver> target,
+                                        TNode<Object> 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<Map> source_map = LoadMap(CAST(source));
+    TNode<Int32T> source_instance_type = LoadMapInstanceType(source_map);
+    GotoIfNot(IsJSObjectInstanceType(source_instance_type),
+              &if_sourcenotjsobject);
+
+    TNode<FixedArrayBase> 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<Map> target_map = LoadMap(target);
+      GotoIf(IsDeprecatedMap(target_map), if_runtime);
+
+      if (use_set) {
+        TNode<BoolT> target_is_simple_receiver = IsSimpleObjectMap(target_map);
+        ForEachEnumerableOwnProperty(
+            context, source_map, CAST(source), kEnumerationOrder,
+            [=](TNode<Name> key, TNode<Object> 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<Name> key, TNode<Object> 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<IntPtrT> 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<JSObject> target = CAST(Parameter(Descriptor::kTarget));
+  TNode<Object> source = CAST(Parameter(Descriptor::kSource));
+  TNode<Context> 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<JSReceiver> target = CAST(Parameter(Descriptor::kTarget));
+  TNode<Object> source = CAST(Parameter(Descriptor::kSource));
+  TNode<Context> 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);

src/builtins/builtins-object-gen.cc

@@ -48,9 +48,6 @@ class ObjectBuiltinsAssembler : public CodeStubAssembler {
   Node* IsSpecialReceiverMap(SloppyTNode<Map> map);
 
   TNode<Word32T> IsStringWrapperElementsKind(TNode<Map> map);
-
-  void ObjectAssignFast(TNode<Context> context, TNode<JSReceiver> to,
-                        TNode<Object> 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<Object> 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> context,
-                                               TNode<JSReceiver> to,
-                                               TNode<Object> 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<Map> from_map = LoadMap(CAST(from));
-  TNode<Int32T> 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<Map> to_map = LoadMap(to);
-  GotoIf(IsDeprecatedMap(to_map), slow);
-  TNode<BoolT> 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<Name> key, TNode<Object> 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);

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);

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);

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);

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:

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(

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)                                    \

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)                               \

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);