[dict-proto] Move CollectKeys, CopyEnumKeysTo, and EnumIndexComparator

This turns the member functions CollectKeysTo and CopyEnumKeysTo of
BaseNameDictionary into helper function in keys.cc; they are only used
there. Further, CollectKeysTo is renamed to CollectKeysFromDictionary.

EnumIndexComparator is moved from keys.cc to dictionary.h.

All moves are motivated by
https://chromium-review.googlesource.com/c/v8/v8/+/2489692 needing
these function in other places.

Bug: v8:7569
Change-Id: Ia8039e98fd00cef45dec376f3c401635b2321761
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2509597Reviewed-by: Marja Hölttä <marja@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Frank Emrich <emrich@google.com>
Cr-Commit-Position: refs/heads/master@{#70912}

src/objects/dictionary.h

@@ -141,11 +141,6 @@ class EXPORT_TEMPLATE_DECLARE(V8_EXPORT_PRIVATE) BaseNameDictionary
       AllocationType allocation = AllocationType::kYoung,
       MinimumCapacity capacity_option = USE_DEFAULT_MINIMUM_CAPACITY);
 
-  // Collect the keys into the given KeyAccumulator, in ascending chronological
-  // order of property creation.
-  V8_WARN_UNUSED_RESULT static ExceptionStatus CollectKeysTo(
-      Handle<Derived> dictionary, KeyAccumulator* keys);
-
   // Allocate the next enumeration index. Possibly updates all enumeration
   // indices in the table.
   static int NextEnumerationIndex(Isolate* isolate, Handle<Derived> dictionary);
@@ -157,13 +152,6 @@ class EXPORT_TEMPLATE_DECLARE(V8_EXPORT_PRIVATE) BaseNameDictionary
   static Handle<FixedArray> IterationIndices(Isolate* isolate,
                                              Handle<Derived> dictionary);
 
-  // Copies enumerable keys to preallocated fixed array.
-  // Does not throw for uninitialized exports in module namespace objects, so
-  // this has to be checked separately.
-  static void CopyEnumKeysTo(Isolate* isolate, Handle<Derived> dictionary,
-                             Handle<FixedArray> storage, KeyCollectionMode mode,
-                             KeyAccumulator* accumulator);
-
   template <typename LocalIsolate>
   V8_WARN_UNUSED_RESULT static Handle<Derived> AddNoUpdateNextEnumerationIndex(
       LocalIsolate* isolate, Handle<Derived> dictionary, Key key,
@@ -361,6 +349,22 @@ class NumberDictionary
                       Dictionary<NumberDictionary, NumberDictionaryShape>);
 };
 
+// The comparator is passed two indices |a| and |b|, and it returns < 0 when the
+// property at index |a| comes before the property at index |b| in the
+// enumeration order.
+template <typename Dictionary>
+struct EnumIndexComparator {
+  explicit EnumIndexComparator(Dictionary dict) : dict(dict) {}
+  bool operator()(Tagged_t a, Tagged_t b) {
+    PropertyDetails da(
+        dict.DetailsAt(InternalIndex(Smi(static_cast<Address>(a)).value())));
+    PropertyDetails db(
+        dict.DetailsAt(InternalIndex(Smi(static_cast<Address>(b)).value())));
+    return da.dictionary_index() < db.dictionary_index();
+  }
+  Dictionary dict;
+};
+
 }  // namespace internal
 }  // namespace v8
 

src/objects/keys.cc

@@ -17,6 +17,7 @@
 #include "src/objects/ordered-hash-table-inl.h"
 #include "src/objects/property-descriptor.h"
 #include "src/objects/prototype.h"
+#include "src/objects/slots-atomic-inl.h"
 #include "src/utils/identity-map.h"
 #include "src/zone/zone-hashmap.h"
 
@@ -811,6 +812,61 @@ base::Optional<int> CollectOwnPropertyNamesInternal(
   return first_skipped;
 }
 
+// Copies enumerable keys to preallocated fixed array.
+// Does not throw for uninitialized exports in module namespace objects, so
+// this has to be checked separately.
+template <typename Dict>
+void CopyEnumKeysTo(Isolate* isolate, Handle<Dict> dictionary,
+                    Handle<FixedArray> storage, KeyCollectionMode mode,
+                    KeyAccumulator* accumulator) {
+  DCHECK_IMPLIES(mode != KeyCollectionMode::kOwnOnly, accumulator != nullptr);
+  int length = storage->length();
+  int properties = 0;
+  ReadOnlyRoots roots(isolate);
+  {
+    AllowHeapAllocation allow_gc;
+    for (InternalIndex i : dictionary->IterateEntries()) {
+      Object key;
+      if (!dictionary->ToKey(roots, i, &key)) continue;
+      bool is_shadowing_key = false;
+      if (key.IsSymbol()) continue;
+      PropertyDetails details = dictionary->DetailsAt(i);
+      if (details.IsDontEnum()) {
+        if (mode == KeyCollectionMode::kIncludePrototypes) {
+          is_shadowing_key = true;
+        } else {
+          continue;
+        }
+      }
+      if (is_shadowing_key) {
+        // This might allocate, but {key} is not used afterwards.
+        accumulator->AddShadowingKey(key, &allow_gc);
+        continue;
+      } else {
+        storage->set(properties, Smi::FromInt(i.as_int()));
+      }
+      properties++;
+      if (mode == KeyCollectionMode::kOwnOnly && properties == length) break;
+    }
+  }
+
+  CHECK_EQ(length, properties);
+  {
+    DisallowHeapAllocation no_gc;
+    Dict raw_dictionary = *dictionary;
+    FixedArray raw_storage = *storage;
+    EnumIndexComparator<Dict> cmp(raw_dictionary);
+    // Use AtomicSlot wrapper to ensure that std::sort uses atomic load and
+    // store operations that are safe for concurrent marking.
+    AtomicSlot start(storage->GetFirstElementAddress());
+    std::sort(start, start + length, cmp);
+    for (int i = 0; i < length; i++) {
+      InternalIndex index(Smi::ToInt(raw_storage.get(i)));
+      raw_storage.set(i, raw_dictionary.NameAt(index));
+    }
+  }
+}
+
 template <class T>
 Handle<FixedArray> GetOwnEnumPropertyDictionaryKeys(Isolate* isolate,
                                                     KeyCollectionMode mode,
@@ -823,9 +879,77 @@ Handle<FixedArray> GetOwnEnumPropertyDictionaryKeys(Isolate* isolate,
   }
   int length = dictionary->NumberOfEnumerableProperties();
   Handle<FixedArray> storage = isolate->factory()->NewFixedArray(length);
-  T::CopyEnumKeysTo(isolate, dictionary, storage, mode, accumulator);
+  CopyEnumKeysTo(isolate, dictionary, storage, mode, accumulator);
   return storage;
 }
+
+// Collect the keys from |dictionary| into |keys|, in ascending chronological
+// order of property creation.
+template <typename Dictionary>
+ExceptionStatus CollectKeysFromDictionary(Handle<Dictionary> dictionary,
+                                          KeyAccumulator* keys) {
+  Isolate* isolate = keys->isolate();
+  ReadOnlyRoots roots(isolate);
+  // TODO(jkummerow): Consider using a std::unique_ptr<InternalIndex[]> instead.
+  Handle<FixedArray> array =
+      isolate->factory()->NewFixedArray(dictionary->NumberOfElements());
+  int array_size = 0;
+  PropertyFilter filter = keys->filter();
+  // Handle enumerable strings in CopyEnumKeysTo.
+  DCHECK_NE(keys->filter(), ENUMERABLE_STRINGS);
+  {
+    DisallowHeapAllocation no_gc;
+    for (InternalIndex i : dictionary->IterateEntries()) {
+      Object key;
+      Dictionary raw_dictionary = *dictionary;
+      if (!raw_dictionary.ToKey(roots, i, &key)) continue;
+      if (key.FilterKey(filter)) continue;
+      PropertyDetails details = raw_dictionary.DetailsAt(i);
+      if ((details.attributes() & filter) != 0) {
+        AllowHeapAllocation gc;
+        // This might allocate, but {key} is not used afterwards.
+        keys->AddShadowingKey(key, &gc);
+        continue;
+      }
+      if (filter & ONLY_ALL_CAN_READ) {
+        if (details.kind() != kAccessor) continue;
+        Object accessors = raw_dictionary.ValueAt(i);
+        if (!accessors.IsAccessorInfo()) continue;
+        if (!AccessorInfo::cast(accessors).all_can_read()) continue;
+      }
+      array->set(array_size++, Smi::FromInt(i.as_int()));
+    }
+
+    EnumIndexComparator<Dictionary> cmp(*dictionary);
+    // Use AtomicSlot wrapper to ensure that std::sort uses atomic load and
+    // store operations that are safe for concurrent marking.
+    AtomicSlot start(array->GetFirstElementAddress());
+    std::sort(start, start + array_size, cmp);
+  }
+
+  bool has_seen_symbol = false;
+  for (int i = 0; i < array_size; i++) {
+    InternalIndex index(Smi::ToInt(array->get(i)));
+    Object key = dictionary->NameAt(index);
+    if (key.IsSymbol()) {
+      has_seen_symbol = true;
+      continue;
+    }
+    ExceptionStatus status = keys->AddKey(key, DO_NOT_CONVERT);
+    if (!status) return status;
+  }
+  if (has_seen_symbol) {
+    for (int i = 0; i < array_size; i++) {
+      InternalIndex index(Smi::ToInt(array->get(i)));
+      Object key = dictionary->NameAt(index);
+      if (!key.IsSymbol()) continue;
+      ExceptionStatus status = keys->AddKey(key, DO_NOT_CONVERT);
+      if (!status) return status;
+    }
+  }
+  return ExceptionStatus::kSuccess;
+}
+
 }  // namespace
 
 Maybe<bool> KeyAccumulator::CollectOwnPropertyNames(Handle<JSReceiver> receiver,
@@ -886,11 +1010,11 @@ Maybe<bool> KeyAccumulator::CollectOwnPropertyNames(Handle<JSReceiver> receiver,
             object, this, descs, first_symbol.value(), limit));
       }
     } else if (object->IsJSGlobalObject()) {
-      RETURN_NOTHING_IF_NOT_SUCCESSFUL(GlobalDictionary::CollectKeysTo(
+      RETURN_NOTHING_IF_NOT_SUCCESSFUL(CollectKeysFromDictionary(
           handle(JSGlobalObject::cast(*object).global_dictionary(), isolate_),
           this));
     } else {
-      RETURN_NOTHING_IF_NOT_SUCCESSFUL(NameDictionary::CollectKeysTo(
+      RETURN_NOTHING_IF_NOT_SUCCESSFUL(CollectKeysFromDictionary(
           handle(object->property_dictionary(), isolate_), this));
     }
   }
@@ -907,11 +1031,11 @@ ExceptionStatus KeyAccumulator::CollectPrivateNames(Handle<JSReceiver> receiver,
         object->map().instance_descriptors(kRelaxedLoad), isolate_);
     CollectOwnPropertyNamesInternal<false>(object, this, descs, 0, limit);
   } else if (object->IsJSGlobalObject()) {
-    RETURN_FAILURE_IF_NOT_SUCCESSFUL(GlobalDictionary::CollectKeysTo(
+    RETURN_FAILURE_IF_NOT_SUCCESSFUL(CollectKeysFromDictionary(
         handle(JSGlobalObject::cast(*object).global_dictionary(), isolate_),
         this));
   } else {
-    RETURN_FAILURE_IF_NOT_SUCCESSFUL(NameDictionary::CollectKeysTo(
+    RETURN_FAILURE_IF_NOT_SUCCESSFUL(CollectKeysFromDictionary(
         handle(object->property_dictionary(), isolate_), this));
   }
   return ExceptionStatus::kSuccess;
@@ -1022,7 +1146,7 @@ Maybe<bool> KeyAccumulator::CollectOwnJSProxyKeys(Handle<JSReceiver> receiver,
                                                   Handle<JSProxy> proxy) {
   STACK_CHECK(isolate_, Nothing<bool>());
   if (filter_ == PRIVATE_NAMES_ONLY) {
-    RETURN_NOTHING_IF_NOT_SUCCESSFUL(NameDictionary::CollectKeysTo(
+    RETURN_NOTHING_IF_NOT_SUCCESSFUL(CollectKeysFromDictionary(
         handle(proxy->property_dictionary(), isolate_), this));
     return Just(true);
   }

src/objects/objects.cc

@@ -6454,71 +6454,6 @@ int Dictionary<Derived, Shape>::NumberOfEnumerableProperties() {
   return result;
 }
 
-template <typename Dictionary>
-struct EnumIndexComparator {
-  explicit EnumIndexComparator(Dictionary dict) : dict(dict) {}
-  bool operator()(Tagged_t a, Tagged_t b) {
-    PropertyDetails da(
-        dict.DetailsAt(InternalIndex(Smi(static_cast<Address>(a)).value())));
-    PropertyDetails db(
-        dict.DetailsAt(InternalIndex(Smi(static_cast<Address>(b)).value())));
-    return da.dictionary_index() < db.dictionary_index();
-  }
-  Dictionary dict;
-};
-
-template <typename Derived, typename Shape>
-void BaseNameDictionary<Derived, Shape>::CopyEnumKeysTo(
-    Isolate* isolate, Handle<Derived> dictionary, Handle<FixedArray> storage,
-    KeyCollectionMode mode, KeyAccumulator* accumulator) {
-  DCHECK_IMPLIES(mode != KeyCollectionMode::kOwnOnly, accumulator != nullptr);
-  int length = storage->length();
-  int properties = 0;
-  ReadOnlyRoots roots(isolate);
-  {
-    AllowHeapAllocation allow_gc;
-    for (InternalIndex i : dictionary->IterateEntries()) {
-      Object key;
-      if (!dictionary->ToKey(roots, i, &key)) continue;
-      bool is_shadowing_key = false;
-      if (key.IsSymbol()) continue;
-      PropertyDetails details = dictionary->DetailsAt(i);
-      if (details.IsDontEnum()) {
-        if (mode == KeyCollectionMode::kIncludePrototypes) {
-          is_shadowing_key = true;
-        } else {
-          continue;
-        }
-      }
-      if (is_shadowing_key) {
-        // This might allocate, but {key} is not used afterwards.
-        accumulator->AddShadowingKey(key, &allow_gc);
-        continue;
-      } else {
-        storage->set(properties, Smi::FromInt(i.as_int()));
-      }
-      properties++;
-      if (mode == KeyCollectionMode::kOwnOnly && properties == length) break;
-    }
-  }
-
-  CHECK_EQ(length, properties);
-  {
-    DisallowHeapAllocation no_gc;
-    Derived raw_dictionary = *dictionary;
-    FixedArray raw_storage = *storage;
-    EnumIndexComparator<Derived> cmp(raw_dictionary);
-    // Use AtomicSlot wrapper to ensure that std::sort uses atomic load and
-    // store operations that are safe for concurrent marking.
-    AtomicSlot start(storage->GetFirstElementAddress());
-    std::sort(start, start + length, cmp);
-    for (int i = 0; i < length; i++) {
-      InternalIndex index(Smi::ToInt(raw_storage.get(i)));
-      raw_storage.set(i, raw_dictionary.NameAt(index));
-    }
-  }
-}
-
 template <typename Derived, typename Shape>
 Handle<FixedArray> BaseNameDictionary<Derived, Shape>::IterationIndices(
     Isolate* isolate, Handle<Derived> dictionary) {
@@ -6552,71 +6487,6 @@ Handle<FixedArray> BaseNameDictionary<Derived, Shape>::IterationIndices(
   return FixedArray::ShrinkOrEmpty(isolate, array, array_size);
 }
 
-template <typename Derived, typename Shape>
-ExceptionStatus BaseNameDictionary<Derived, Shape>::CollectKeysTo(
-    Handle<Derived> dictionary, KeyAccumulator* keys) {
-  Isolate* isolate = keys->isolate();
-  ReadOnlyRoots roots(isolate);
-  // TODO(jkummerow): Consider using a std::unique_ptr<InternalIndex[]> instead.
-  Handle<FixedArray> array =
-      isolate->factory()->NewFixedArray(dictionary->NumberOfElements());
-  int array_size = 0;
-  PropertyFilter filter = keys->filter();
-  // Handle enumerable strings in CopyEnumKeysTo.
-  DCHECK_NE(keys->filter(), ENUMERABLE_STRINGS);
-  {
-    DisallowHeapAllocation no_gc;
-    for (InternalIndex i : dictionary->IterateEntries()) {
-      Object key;
-      Derived raw_dictionary = *dictionary;
-      if (!raw_dictionary.ToKey(roots, i, &key)) continue;
-      if (key.FilterKey(filter)) continue;
-      PropertyDetails details = raw_dictionary.DetailsAt(i);
-      if ((details.attributes() & filter) != 0) {
-        AllowHeapAllocation gc;
-        // This might allocate, but {key} is not used afterwards.
-        keys->AddShadowingKey(key, &gc);
-        continue;
-      }
-      if (filter & ONLY_ALL_CAN_READ) {
-        if (details.kind() != kAccessor) continue;
-        Object accessors = raw_dictionary.ValueAt(i);
-        if (!accessors.IsAccessorInfo()) continue;
-        if (!AccessorInfo::cast(accessors).all_can_read()) continue;
-      }
-      array->set(array_size++, Smi::FromInt(i.as_int()));
-    }
-
-    EnumIndexComparator<Derived> cmp(*dictionary);
-    // Use AtomicSlot wrapper to ensure that std::sort uses atomic load and
-    // store operations that are safe for concurrent marking.
-    AtomicSlot start(array->GetFirstElementAddress());
-    std::sort(start, start + array_size, cmp);
-  }
-
-  bool has_seen_symbol = false;
-  for (int i = 0; i < array_size; i++) {
-    InternalIndex index(Smi::ToInt(array->get(i)));
-    Object key = dictionary->NameAt(index);
-    if (key.IsSymbol()) {
-      has_seen_symbol = true;
-      continue;
-    }
-    ExceptionStatus status = keys->AddKey(key, DO_NOT_CONVERT);
-    if (!status) return status;
-  }
-  if (has_seen_symbol) {
-    for (int i = 0; i < array_size; i++) {
-      InternalIndex index(Smi::ToInt(array->get(i)));
-      Object key = dictionary->NameAt(index);
-      if (!key.IsSymbol()) continue;
-      ExceptionStatus status = keys->AddKey(key, DO_NOT_CONVERT);
-      if (!status) return status;
-    }
-  }
-  return ExceptionStatus::kSuccess;
-}
-
 // Backwards lookup (slow).
 template <typename Derived, typename Shape>
 Object Dictionary<Derived, Shape>::SlowReverseLookup(Object value) {