Further optimize object.assign fast path for symbol properties

This is a follow-up CL from https://chromium-review.googlesource.com/c/v8/v8/+/1432597
Indices of first and last symbol properties are recorded and used on a second iteration of DescriptorArrayForEach() to potentially reduce the iteration range

Bug: v8:6705
Change-Id: Iac73909d138214d1128e935eff686f2f058e17f7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1516021
Commit-Queue: Z Nguyen-Huu <duongn@microsoft.com>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60344}

src/code-stub-assembler.cc

@@ -8901,22 +8901,6 @@ void CodeStubAssembler::LookupBinary(TNode<Name> unique_name,
   }
 }
 
-void CodeStubAssembler::DescriptorArrayForEach(
-    VariableList& variable_list, TNode<Uint32T> start_descriptor,
-    TNode<Uint32T> end_descriptor, const ForEachDescriptorBodyFunction& body) {
-  TNode<IntPtrT> start_index = ToKeyIndex<DescriptorArray>(start_descriptor);
-  TNode<IntPtrT> end_index = ToKeyIndex<DescriptorArray>(end_descriptor);
-
-  BuildFastLoop(variable_list, start_index, end_index,
-                [=](Node* index) {
-                  TNode<IntPtrT> descriptor_key_index =
-                      TNode<IntPtrT>::UncheckedCast(index);
-                  body(descriptor_key_index);
-                },
-                DescriptorArray::kEntrySize, INTPTR_PARAMETERS,
-                IndexAdvanceMode::kPost);
-}
-
 void CodeStubAssembler::ForEachEnumerableOwnProperty(
     TNode<Context> context, TNode<Map> map, TNode<JSObject> object,
     ForEachEnumerationMode mode, const ForEachKeyValueFunction& body,
@@ -8933,18 +8917,29 @@ void CodeStubAssembler::ForEachEnumerableOwnProperty(
   TVARIABLE(BoolT, var_has_symbol, Int32FalseConstant());
   // false - iterate only string properties, true - iterate only symbol
   // properties
-  TVARIABLE(BoolT, var_name_filter, Int32FalseConstant());
-  VariableList list({&var_stable, &var_has_symbol, &var_name_filter}, zone());
-  Label descriptor_array_loop(this,
-                              {&var_stable, &var_has_symbol, &var_name_filter});
+  TVARIABLE(BoolT, var_is_symbol_processing_loop, Int32FalseConstant());
+  TVARIABLE(IntPtrT, var_start_key_index,
+            ToKeyIndex<DescriptorArray>(Unsigned(Int32Constant(0))));
+  // Note: var_end_key_index is exclusive for the loop
+  TVARIABLE(IntPtrT, var_end_key_index,
+            ToKeyIndex<DescriptorArray>(nof_descriptors));
+  VariableList list(
+      {&var_stable, &var_has_symbol, &var_is_symbol_processing_loop,
+       &var_start_key_index, &var_end_key_index},
+      zone());
+  Label descriptor_array_loop(
+      this, {&var_stable, &var_has_symbol, &var_is_symbol_processing_loop,
+             &var_start_key_index, &var_end_key_index});
 
   Goto(&descriptor_array_loop);
   BIND(&descriptor_array_loop);
 
-  DescriptorArrayForEach(
-      list, Unsigned(Int32Constant(0)), nof_descriptors,
-      [=, &var_stable, &var_has_symbol,
-       &var_name_filter](TNode<IntPtrT> descriptor_key_index) {
+  BuildFastLoop(
+      list, var_start_key_index.value(), var_end_key_index.value(),
+      [=, &var_stable, &var_has_symbol, &var_is_symbol_processing_loop,
+       &var_start_key_index, &var_end_key_index](Node* index) {
+        TNode<IntPtrT> descriptor_key_index =
+            TNode<IntPtrT>::UncheckedCast(index);
         TNode<Name> next_key =
             LoadKeyByKeyIndex(descriptors, descriptor_key_index);
 
@@ -8953,21 +8948,35 @@ void CodeStubAssembler::ForEachEnumerableOwnProperty(
 
         if (mode == kEnumerationOrder) {
           // |next_key| is either a string or a symbol
-          // Skip strings or symbols depending on var_name_filter value.
+          // Skip strings or symbols depending on
+          // |var_is_symbol_processing_loop|.
           Label if_string(this), if_symbol(this), if_name_ok(this);
-
           Branch(IsSymbol(next_key), &if_symbol, &if_string);
           BIND(&if_symbol);
           {
-            var_has_symbol = Int32TrueConstant();
-            // Process symbol property when |var_name_filer| is true.
-            Branch(var_name_filter.value(), &if_name_ok, &next_iteration);
+            // Process symbol property when |var_is_symbol_processing_loop| is
+            // true.
+            GotoIf(var_is_symbol_processing_loop.value(), &if_name_ok);
+            // First iteration need to calculate smaller range for processing
+            // symbols
+            Label if_first_symbol(this);
+            // var_end_key_index is still inclusive at this point.
+            var_end_key_index = descriptor_key_index;
+            Branch(var_has_symbol.value(), &next_iteration, &if_first_symbol);
+            BIND(&if_first_symbol);
+            {
+              var_start_key_index = descriptor_key_index;
+              var_has_symbol = Int32TrueConstant();
+              Goto(&next_iteration);
+            }
           }
           BIND(&if_string);
           {
             CSA_ASSERT(this, IsString(next_key));
-            // Process string property when |var_name_filer| is false.
-            Branch(var_name_filter.value(), &next_iteration, &if_name_ok);
+            // Process string property when |var_is_symbol_processing_loop| is
+            // false.
+            Branch(var_is_symbol_processing_loop.value(), &next_iteration,
+                   &if_name_ok);
           }
           BIND(&if_name_ok);
         }
@@ -9064,14 +9073,19 @@ void CodeStubAssembler::ForEachEnumerableOwnProperty(
           }
         }
         BIND(&next_iteration);
-      });
+      },
+      DescriptorArray::kEntrySize, INTPTR_PARAMETERS, IndexAdvanceMode::kPost);
 
   if (mode == kEnumerationOrder) {
     Label done(this);
-    GotoIf(var_name_filter.value(), &done);
+    GotoIf(var_is_symbol_processing_loop.value(), &done);
     GotoIfNot(var_has_symbol.value(), &done);
     // All string properties are processed, now process symbol properties.
-    var_name_filter = Int32TrueConstant();
+    var_is_symbol_processing_loop = Int32TrueConstant();
+    // Add DescriptorArray::kEntrySize to make the var_end_key_index exclusive
+    // as BuildFastLoop() expects.
+    Increment(&var_end_key_index, DescriptorArray::kEntrySize,
+              INTPTR_PARAMETERS);
     Goto(&descriptor_array_loop);
 
     BIND(&done);

src/code-stub-assembler.h

@@ -3323,11 +3323,6 @@ class V8_EXPORT_PRIVATE CodeStubAssembler
   typedef std::function<void(TNode<IntPtrT> descriptor_key_index)>
       ForEachDescriptorBodyFunction;
 
-  void DescriptorArrayForEach(VariableList& variable_list,
-                              TNode<Uint32T> start_descriptor,
-                              TNode<Uint32T> end_descriptor,
-                              const ForEachDescriptorBodyFunction& body);
-
   // Descriptor array accessors based on key_index, which is equal to
   // DescriptorArray::ToKeyIndex(descriptor).
   TNode<Name> LoadKeyByKeyIndex(TNode<DescriptorArray> container,

test/mjsunit/es6/object-assign.js

@@ -171,3 +171,58 @@ assertSame(Object.assign(o, {}), o);
   var source = {get k1() { throw "fail" }};
   assertThrows(()=>Object.assign(target, source));
 })();
+
+(function strings_and_symbol_order1() {
+  // first order
+  var log = [];
+
+  var sym1 = Symbol("x"), sym2 = Symbol("y");
+  var source = {
+      get [sym1](){ log.push("get sym1"); },
+      get a() { log.push("get a"); },
+      get b() { log.push("get b"); },
+      get c() { log.push("get c"); },
+      get [sym2](){ log.push("get sym2"); },
+  };
+
+  Object.assign({}, source);
+
+  assertEquals(log, ["get a", "get b", "get c", "get sym1", "get sym2"]);
+})();
+
+(function strings_and_symbol_order2() {
+  // first order
+  var log = [];
+
+  var sym1 = Symbol("x"), sym2 = Symbol("y");
+  var source = {
+      get [sym1](){ log.push("get sym1"); },
+      get a() { log.push("get a"); },
+      get [sym2](){ log.push("get sym2"); },
+      get b() { log.push("get b"); },
+      get c() { log.push("get c"); },
+  };
+
+  Object.assign({}, source);
+
+  assertEquals(log, ["get a", "get b", "get c", "get sym1", "get sym2"]);
+})();
+
+
+(function strings_and_symbol_order3() {
+  // first order
+  var log = [];
+
+  var sym1 = Symbol("x"), sym2 = Symbol("y");
+  var source = {
+      get a() { log.push("get a"); },
+      get [sym1](){ log.push("get sym1"); },
+      get b() { log.push("get b"); },
+      get [sym2](){ log.push("get sym2"); },
+      get c() { log.push("get c"); },
+  };
+
+  Object.assign({}, source);
+
+  assertEquals(log, ["get a", "get b", "get c", "get sym1", "get sym2"]);
+})();