[keys] Speed up Array serialization

Using KeyAccumulator::GetKeys directly enables fast-paths by checking
if the enum-cache is set.

Drive-by-fix:
- Reduce public interface of KeyAccumulator to prevent these
  performance issues in the future.
- Fix value-serializer.cc includes

Change-Id: I2cc7b3bf9d1e42e699829427163ecbdee92c9007
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2520898
Commit-Queue: Camillo Bruni <cbruni@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71050}

src/objects/keys.cc

@@ -654,14 +654,11 @@ bool FastKeyAccumulator::TryPrototypeInfoCache(Handle<JSReceiver> receiver) {
   return true;
 }
 
-namespace {
-
-enum IndexedOrNamed { kIndexed, kNamed };
-
-V8_WARN_UNUSED_RESULT ExceptionStatus FilterForEnumerableProperties(
+V8_WARN_UNUSED_RESULT ExceptionStatus
+KeyAccumulator::FilterForEnumerableProperties(
     Handle<JSReceiver> receiver, Handle<JSObject> object,
-    Handle<InterceptorInfo> interceptor, KeyAccumulator* accumulator,
-    Handle<JSObject> result, IndexedOrNamed type) {
+    Handle<InterceptorInfo> interceptor, Handle<JSObject> result,
+    IndexedOrNamed type) {
   DCHECK(result->IsJSArray() || result->HasSloppyArgumentsElements());
   ElementsAccessor* accessor = result->GetElementsAccessor();
 
@@ -670,8 +667,8 @@ V8_WARN_UNUSED_RESULT ExceptionStatus FilterForEnumerableProperties(
     if (!accessor->HasEntry(*result, entry)) continue;
 
     // args are invalid after args.Call(), create a new one in every iteration.
-    PropertyCallbackArguments args(accumulator->isolate(), interceptor->data(),
-                                   *receiver, *object, Just(kDontThrow));
+    PropertyCallbackArguments args(isolate_, interceptor->data(), *receiver,
+                                   *object, Just(kDontThrow));
 
     Handle<Object> element = accessor->Get(result, entry);
     Handle<Object> attributes;
@@ -689,8 +686,7 @@ V8_WARN_UNUSED_RESULT ExceptionStatus FilterForEnumerableProperties(
       int32_t value;
       CHECK(attributes->ToInt32(&value));
       if ((value & DONT_ENUM) == 0) {
-        RETURN_FAILURE_IF_NOT_SUCCESSFUL(
-            accumulator->AddKey(element, DO_NOT_CONVERT));
+        RETURN_FAILURE_IF_NOT_SUCCESSFUL(AddKey(element, DO_NOT_CONVERT));
       }
     }
   }
@@ -698,17 +694,14 @@ V8_WARN_UNUSED_RESULT ExceptionStatus FilterForEnumerableProperties(
 }
 
 // Returns |true| on success, |nothing| on exception.
-Maybe<bool> CollectInterceptorKeysInternal(Handle<JSReceiver> receiver,
-                                           Handle<JSObject> object,
-                                           Handle<InterceptorInfo> interceptor,
-                                           KeyAccumulator* accumulator,
-                                           IndexedOrNamed type) {
-  Isolate* isolate = accumulator->isolate();
-  PropertyCallbackArguments enum_args(isolate, interceptor->data(), *receiver,
+Maybe<bool> KeyAccumulator::CollectInterceptorKeysInternal(
+    Handle<JSReceiver> receiver, Handle<JSObject> object,
+    Handle<InterceptorInfo> interceptor, IndexedOrNamed type) {
+  PropertyCallbackArguments enum_args(isolate_, interceptor->data(), *receiver,
                                       *object, Just(kDontThrow));
 
   Handle<JSObject> result;
-  if (!interceptor->enumerator().IsUndefined(isolate)) {
+  if (!interceptor->enumerator().IsUndefined(isolate_)) {
     if (type == kIndexed) {
       result = enum_args.CallIndexedEnumerator(interceptor);
     } else {
@@ -716,25 +709,23 @@ Maybe<bool> CollectInterceptorKeysInternal(Handle<JSReceiver> receiver,
       result = enum_args.CallNamedEnumerator(interceptor);
     }
   }
-  RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate, Nothing<bool>());
+  RETURN_VALUE_IF_SCHEDULED_EXCEPTION(isolate_, Nothing<bool>());
   if (result.is_null()) return Just(true);
 
-  if ((accumulator->filter() & ONLY_ENUMERABLE) &&
-      !interceptor->query().IsUndefined(isolate)) {
+  if ((filter_ & ONLY_ENUMERABLE) &&
+      !interceptor->query().IsUndefined(isolate_)) {
     RETURN_NOTHING_IF_NOT_SUCCESSFUL(FilterForEnumerableProperties(
-        receiver, object, interceptor, accumulator, result, type));
+        receiver, object, interceptor, result, type));
   } else {
-    RETURN_NOTHING_IF_NOT_SUCCESSFUL(accumulator->AddKeys(
+    RETURN_NOTHING_IF_NOT_SUCCESSFUL(AddKeys(
         result, type == kIndexed ? CONVERT_TO_ARRAY_INDEX : DO_NOT_CONVERT));
   }
   return Just(true);
 }
 
-Maybe<bool> CollectInterceptorKeys(Handle<JSReceiver> receiver,
+Maybe<bool> KeyAccumulator::CollectInterceptorKeys(Handle<JSReceiver> receiver,
                                                    Handle<JSObject> object,
-                                   KeyAccumulator* accumulator,
                                                    IndexedOrNamed type) {
-  Isolate* isolate = accumulator->isolate();
   if (type == kIndexed) {
     if (!object->HasIndexedInterceptor()) return Just(true);
   } else {
@@ -743,17 +734,13 @@ Maybe<bool> CollectInterceptorKeys(Handle<JSReceiver> receiver,
   Handle<InterceptorInfo> interceptor(type == kIndexed
                                           ? object->GetIndexedInterceptor()
                                           : object->GetNamedInterceptor(),
-                                      isolate);
-  if ((accumulator->filter() & ONLY_ALL_CAN_READ) &&
-      !interceptor->all_can_read()) {
+                                      isolate_);
+  if ((filter() & ONLY_ALL_CAN_READ) && !interceptor->all_can_read()) {
     return Just(true);
   }
-  return CollectInterceptorKeysInternal(receiver, object, interceptor,
-                                        accumulator, type);
+  return CollectInterceptorKeysInternal(receiver, object, interceptor, type);
 }
 
-}  // namespace
-
 Maybe<bool> KeyAccumulator::CollectOwnElementIndices(
     Handle<JSReceiver> receiver, Handle<JSObject> object) {
   if (filter_ & SKIP_STRINGS || skip_indices_) return Just(true);
@@ -761,7 +748,7 @@ Maybe<bool> KeyAccumulator::CollectOwnElementIndices(
   ElementsAccessor* accessor = object->GetElementsAccessor();
   RETURN_NOTHING_IF_NOT_SUCCESSFUL(
       accessor->CollectElementIndices(object, this));
-  return CollectInterceptorKeys(receiver, object, this, kIndexed);
+  return CollectInterceptorKeys(receiver, object, kIndexed);
 }
 
 namespace {
@@ -1064,7 +1051,7 @@ Maybe<bool> KeyAccumulator::CollectOwnPropertyNames(Handle<JSReceiver> receiver,
     }
   }
   // Add the property keys from the interceptor.
-  return CollectInterceptorKeys(receiver, object, this, kNamed);
+  return CollectInterceptorKeys(receiver, object, kNamed);
 }
 
 ExceptionStatus KeyAccumulator::CollectPrivateNames(Handle<JSReceiver> receiver,
@@ -1098,7 +1085,7 @@ Maybe<bool> KeyAccumulator::CollectAccessCheckInterceptorKeys(
                      handle(InterceptorInfo::cast(
                                 access_check_info->indexed_interceptor()),
                             isolate_),
-                     this, kIndexed)),
+                     kIndexed)),
                  Nothing<bool>());
   }
   MAYBE_RETURN(
@@ -1106,7 +1093,7 @@ Maybe<bool> KeyAccumulator::CollectAccessCheckInterceptorKeys(
           receiver, object,
           handle(InterceptorInfo::cast(access_check_info->named_interceptor()),
                  isolate_),
-          this, kNamed)),
+          kNamed)),
       Nothing<bool>());
   return Just(true);
 }

src/objects/keys.h

@@ -13,6 +13,7 @@ namespace v8 {
 namespace internal {
 
 class JSProxy;
+class FastKeyAccumulator;
 
 enum AddKeyConversion { DO_NOT_CONVERT, CONVERT_TO_ARRAY_INDEX };
 
@@ -50,15 +51,6 @@ class KeyAccumulator final {
       GetKeysConversion convert = GetKeysConversion::kKeepNumbers);
   Maybe<bool> CollectKeys(Handle<JSReceiver> receiver,
                           Handle<JSReceiver> object);
-  Maybe<bool> CollectOwnElementIndices(Handle<JSReceiver> receiver,
-                                       Handle<JSObject> object);
-  Maybe<bool> CollectOwnPropertyNames(Handle<JSReceiver> receiver,
-                                      Handle<JSObject> object);
-  V8_WARN_UNUSED_RESULT ExceptionStatus
-  CollectPrivateNames(Handle<JSReceiver> receiver, Handle<JSObject> object);
-  Maybe<bool> CollectAccessCheckInterceptorKeys(
-      Handle<AccessCheckInfo> access_check_info, Handle<JSReceiver> receiver,
-      Handle<JSObject> object);
 
   // Might return directly the object's enum_cache, copy the result before using
   // as an elements backing store for a JSObject.
@@ -71,10 +63,6 @@ class KeyAccumulator final {
   AddKey(Object key, AddKeyConversion convert = DO_NOT_CONVERT);
   V8_WARN_UNUSED_RESULT ExceptionStatus
   AddKey(Handle<Object> key, AddKeyConversion convert = DO_NOT_CONVERT);
-  V8_WARN_UNUSED_RESULT ExceptionStatus AddKeys(Handle<FixedArray> array,
-                                                AddKeyConversion convert);
-  V8_WARN_UNUSED_RESULT ExceptionStatus AddKeys(Handle<JSObject> array_like,
-                                                AddKeyConversion convert);
 
   // Jump to the next level, pushing the current |levelLength_| to
   // |levelLengths_| and adding a new list to |elements_|.
@@ -84,43 +72,74 @@ class KeyAccumulator final {
   // The collection mode defines whether we collect the keys from the prototype
   // chain or only look at the receiver.
   KeyCollectionMode mode() { return mode_; }
-  // In case of for-in loops we have to treat JSProxy keys differently and
-  // deduplicate them. Additionally we convert JSProxy keys back to array
-  // indices.
-  void set_is_for_in(bool value) { is_for_in_ = value; }
   void set_skip_indices(bool value) { skip_indices_ = value; }
-  void set_first_prototype_map(Handle<Map> value) {
-    first_prototype_map_ = value;
-  }
-  void set_try_prototype_info_cache(bool value) {
-    try_prototype_info_cache_ = value;
-  }
-  void set_receiver(Handle<JSReceiver> object) { receiver_ = object; }
-  // The last_non_empty_prototype is used to limit the prototypes for which
-  // we have to keep track of non-enumerable keys that can shadow keys
-  // repeated on the prototype chain.
-  void set_last_non_empty_prototype(Handle<JSReceiver> object) {
-    last_non_empty_prototype_ = object;
-  }
-  void set_may_have_elements(bool value) { may_have_elements_ = value; }
   // Shadowing keys are used to filter keys. This happens when non-enumerable
   // keys appear again on the prototype chain.
   void AddShadowingKey(Object key, AllowHeapAllocation* allow_gc);
   void AddShadowingKey(Handle<Object> key);
 
  private:
+  enum IndexedOrNamed { kIndexed, kNamed };
+
+  V8_WARN_UNUSED_RESULT ExceptionStatus
+  CollectPrivateNames(Handle<JSReceiver> receiver, Handle<JSObject> object);
+  Maybe<bool> CollectAccessCheckInterceptorKeys(
+      Handle<AccessCheckInfo> access_check_info, Handle<JSReceiver> receiver,
+      Handle<JSObject> object);
+
+  Maybe<bool> CollectInterceptorKeysInternal(
+      Handle<JSReceiver> receiver, Handle<JSObject> object,
+      Handle<InterceptorInfo> interceptor, IndexedOrNamed type);
+  Maybe<bool> CollectInterceptorKeys(Handle<JSReceiver> receiver,
+                                     Handle<JSObject> object,
+                                     IndexedOrNamed type);
+
+  Maybe<bool> CollectOwnElementIndices(Handle<JSReceiver> receiver,
+                                       Handle<JSObject> object);
+  Maybe<bool> CollectOwnPropertyNames(Handle<JSReceiver> receiver,
+                                      Handle<JSObject> object);
   Maybe<bool> CollectOwnKeys(Handle<JSReceiver> receiver,
                              Handle<JSObject> object);
   Maybe<bool> CollectOwnJSProxyKeys(Handle<JSReceiver> receiver,
                                     Handle<JSProxy> proxy);
   Maybe<bool> CollectOwnJSProxyTargetKeys(Handle<JSProxy> proxy,
                                           Handle<JSReceiver> target);
+
+  V8_WARN_UNUSED_RESULT ExceptionStatus FilterForEnumerableProperties(
+      Handle<JSReceiver> receiver, Handle<JSObject> object,
+      Handle<InterceptorInfo> interceptor, Handle<JSObject> result,
+      IndexedOrNamed type);
+
   Maybe<bool> AddKeysFromJSProxy(Handle<JSProxy> proxy,
                                  Handle<FixedArray> keys);
+  V8_WARN_UNUSED_RESULT ExceptionStatus AddKeys(Handle<FixedArray> array,
+                                                AddKeyConversion convert);
+  V8_WARN_UNUSED_RESULT ExceptionStatus AddKeys(Handle<JSObject> array_like,
+                                                AddKeyConversion convert);
+
   bool IsShadowed(Handle<Object> key);
   bool HasShadowingKeys();
   Handle<OrderedHashSet> keys();
 
+  // In case of for-in loops we have to treat JSProxy keys differently and
+  // deduplicate them. Additionally we convert JSProxy keys back to array
+  // indices.
+  void set_is_for_in(bool value) { is_for_in_ = value; }
+  void set_first_prototype_map(Handle<Map> value) {
+    first_prototype_map_ = value;
+  }
+  void set_try_prototype_info_cache(bool value) {
+    try_prototype_info_cache_ = value;
+  }
+  void set_receiver(Handle<JSReceiver> object) { receiver_ = object; }
+  // The last_non_empty_prototype is used to limit the prototypes for which
+  // we have to keep track of non-enumerable keys that can shadow keys
+  // repeated on the prototype chain.
+  void set_last_non_empty_prototype(Handle<JSReceiver> object) {
+    last_non_empty_prototype_ = object;
+  }
+  void set_may_have_elements(bool value) { may_have_elements_ = value; }
+
   Isolate* isolate_;
   // keys_ is either an Handle<OrderedHashSet> or in the case of own JSProxy
   // keys a Handle<FixedArray>. The OrderedHashSet is in-place converted to the
@@ -139,6 +158,8 @@ class KeyAccumulator final {
   bool skip_shadow_check_ = true;
   bool may_have_elements_ = true;
   bool try_prototype_info_cache_ = false;
+
+  friend FastKeyAccumulator;
 };
 
 // The FastKeyAccumulator handles the cases where there are no elements on the

src/objects/value-serializer.cc

@@ -7,6 +7,7 @@
 #include <type_traits>
 
 #include "include/v8-value-serializer-version.h"
+#include "include/v8.h"
 #include "src/api/api-inl.h"
 #include "src/base/logging.h"
 #include "src/execution/isolate.h"
@@ -20,9 +21,11 @@
 #include "src/objects/js-collection-inl.h"
 #include "src/objects/js-regexp-inl.h"
 #include "src/objects/objects-inl.h"
+#include "src/objects/objects.h"
 #include "src/objects/oddball-inl.h"
 #include "src/objects/ordered-hash-table-inl.h"
 #include "src/objects/property-descriptor.h"
+#include "src/objects/property-details.h"
 #include "src/objects/smi.h"
 #include "src/objects/transitions-inl.h"
 #include "src/snapshot/code-serializer.h"
@@ -717,13 +720,14 @@ Maybe<bool> ValueSerializer::WriteJSArray(Handle<JSArray> array) {
       }
     }
 
-    KeyAccumulator accumulator(isolate_, KeyCollectionMode::kOwnOnly,
-                               ENUMERABLE_STRINGS);
-    if (!accumulator.CollectOwnPropertyNames(array, array).FromMaybe(false)) {
+    Handle<FixedArray> keys;
+    if (!KeyAccumulator::GetKeys(array, KeyCollectionMode::kOwnOnly,
+                                 ENUMERABLE_STRINGS,
+                                 GetKeysConversion::kKeepNumbers, false, true)
+             .ToHandle(&keys)) {
       return Nothing<bool>();
     }
-    Handle<FixedArray> keys =
-        accumulator.GetKeys(GetKeysConversion::kConvertToString);
+
     uint32_t properties_written;
     if (!WriteJSObjectPropertiesSlow(array, keys).To(&properties_written)) {
       return Nothing<bool>();