[keys] Simplify KeyAccumulator

src/elements.cc

Index: src/elements.cc
diff --git a/src/elements.cc b/src/elements.cc
index 6c257ac63f3424b4dd531d1c936e0ff12e38bebd..9f707bc400efbb3f428795e438cf1207ac640870 100644
--- a/src/elements.cc
+++ b/src/elements.cc
@@ -445,6 +445,25 @@ static void TraceTopFrame(Isolate* isolate) {
   JavaScriptFrame::PrintTop(isolate, stdout, false, true);
 }
 
+static void SortIndices(Handle<FixedArray> indices, uint32_t sort_size,
+                        bool emit_write_barrier = true) {

[Jakob Kummerow, 2016/05/23 16:25:21]

Consider using the existing enum WriteBarrierMode instead of a bool.

[Camillo Bruni, 2016/05/23 19:10:13]

updated.

+  struct {
+    bool operator()(Object* a, Object* b) {
+      if (!a->IsUndefined()) {
+        if (b->IsUndefined()) return true;
+        return a->Number() < b->Number();
+      }
+      return !b->IsUndefined();

[Jakob Kummerow, 2016/05/23 16:25:21]

I'm pretty sure this condition is inverted, i.e. you want s/!//.

[Camillo Bruni, 2016/05/23 19:10:13]

good catch!
you're right, if b is smaller than a return false.

+    }
+  } cmp;
+  Object** start =
+      reinterpret_cast<Object**>(indices->GetFirstElementAddress());
+  std::sort(start, start + sort_size, cmp);
+  if (emit_write_barrier) {
+    FIXED_ARRAY_ELEMENTS_WRITE_BARRIER(indices->GetIsolate()->heap(), *indices,
+                                       0, sort_size);
+  }
+}
 
 // Base class for element handler implementations. Contains the
 // the common logic for objects with different ElementsKinds.
@@ -863,7 +882,6 @@ class ElementsAccessorBase : public ElementsAccessor {
       PropertyFilter filter) {
     int count = 0;
     KeyAccumulator accumulator(isolate, OWN_ONLY, ALL_PROPERTIES);
-    accumulator.NextPrototype();
     Subclass::CollectElementIndicesImpl(
         object, handle(object->elements(), isolate), &accumulator);
     Handle<FixedArray> keys = accumulator.GetKeys();
@@ -911,9 +929,10 @@ class ElementsAccessorBase : public ElementsAccessor {
     // Non-dictionary elements can't have all-can-read accessors.
     uint32_t length = GetIterationLength(*object, *backing_store);
     PropertyFilter filter = keys->filter();
+    Factory* factory = keys->isolate()->factory();
     for (uint32_t i = 0; i < length; i++) {
       if (Subclass::HasElementImpl(object, i, backing_store, filter)) {
-        keys->AddKey(i);
+        keys->AddKey(factory->NewNumberFromUint(i));
       }
     }
   }
@@ -968,18 +987,7 @@ class ElementsAccessorBase : public ElementsAccessor {
 
     // Sort the indices list if necessary.
     if (IsDictionaryElementsKind(kind()) || IsSloppyArgumentsElements(kind())) {
-      struct {
-        bool operator()(Object* a, Object* b) {
-          if (!a->IsUndefined()) {
-            if (b->IsUndefined()) return true;
-            return a->Number() < b->Number();
-          }
-          return !b->IsUndefined();
-        }
-      } cmp;
-      Object** start =
-          reinterpret_cast<Object**>(combined_keys->GetFirstElementAddress());
-      std::sort(start, start + nof_indices, cmp);
+      SortIndices(combined_keys, nof_indices, false);
       uint32_t array_length = 0;
       // Indices from dictionary elements should only be converted after
       // sorting.
@@ -1082,16 +1090,9 @@ class DictionaryElementsAccessor
                              ElementsKindTraits<DICTIONARY_ELEMENTS> >(name) {}
 
   static uint32_t GetIterationLength(JSObject* receiver,
-                                     FixedArrayBase* elements) {
-    uint32_t length;
-    if (receiver->IsJSArray()) {
-      // Special-case GetIterationLength for dictionary elements since the
-      // length of the array might be a HeapNumber.
-      JSArray::cast(receiver)->length()->ToArrayLength(&length);
-    } else {
-      length = GetCapacityImpl(receiver, elements);
-    }
-    return length;
+                                     FixedArrayBase* backing_store) {
+    SeededNumberDictionary* dict = SeededNumberDictionary::cast(backing_store);
+    return dict->NumberOfElements();

[Jakob Kummerow, 2016/05/23 16:25:21]

In existing code, GetIterationLength is clearly expected to return the array
length, not the number of actually existing elements, and in the superclass
implementation in line 731ff this is still the case. Having an identically named
overridden method with different semantics seems dangerous.
Maybe "GetIterationLength" is just a too ambiguous name, and we should have
"GetArrayLength" and "GetCapacity"/"GetNumberOfElements" instead?
Or just inline it?

[Camillo Bruni, 2016/05/23 19:10:13]

Renamed to GetMaxNumberOfElements, though the rest of the code stays the same.
In all the cases where we could get a dictionary we do the indirection via
Get/HasEntry which makes this method work in both cases.

   }
 
   static void SetLengthImpl(Isolate* isolate, Handle<JSArray> array,
@@ -1313,21 +1314,27 @@ class DictionaryElementsAccessor
                                         Handle<FixedArrayBase> backing_store,
                                         KeyAccumulator* keys) {
     if (keys->filter() & SKIP_STRINGS) return;
-    Isolate* isolate = keys->isolate();
-    Handle<Object> undefined = isolate->factory()->undefined_value();
-    Handle<Object> the_hole = isolate->factory()->the_hole_value();
+    Factory* factory = keys->isolate()->factory();
+    Handle<Object> undefined = factory->undefined_value();
+    Handle<Object> the_hole = factory->the_hole_value();
     Handle<SeededNumberDictionary> dictionary =
         Handle<SeededNumberDictionary>::cast(backing_store);
     int capacity = dictionary->Capacity();
+    Handle<FixedArray> elements =
+        factory->NewFixedArray(GetIterationLength(*object, *backing_store));
+    int insertion_index = 0;
     PropertyFilter filter = keys->filter();
     for (int i = 0; i < capacity; i++) {
       uint32_t key =
           GetKeyForEntryImpl(dictionary, i, filter, *undefined, *the_hole);
       if (key == kMaxUInt32) continue;
-      keys->AddKey(key);
+      elements->set(insertion_index, *factory->NewNumberFromUint(key));
+      insertion_index++;
+    }
+    SortIndices(elements, insertion_index);
+    for (int i = 0; i < insertion_index; i++) {
+      keys->AddKey(elements->get(i));
     }
-
-    keys->SortCurrentElementsList();
   }
 
   static Handle<FixedArray> DirectCollectElementIndicesImpl(
@@ -2386,18 +2393,16 @@ class SloppyArgumentsElementsAccessor
   static void CollectElementIndicesImpl(Handle<JSObject> object,
                                         Handle<FixedArrayBase> backing_store,
                                         KeyAccumulator* keys) {
-    FixedArray* parameter_map = FixedArray::cast(*backing_store);
-    uint32_t length = parameter_map->length() - 2;
-    for (uint32_t i = 0; i < length; ++i) {
-      if (!parameter_map->get(i + 2)->IsTheHole()) {
-        keys->AddKey(i);
-      }
-    }
-
-    Handle<FixedArrayBase> store(FixedArrayBase::cast(parameter_map->get(1)));
-    ArgumentsAccessor::CollectElementIndicesImpl(object, store, keys);
-    if (Subclass::kind() == FAST_SLOPPY_ARGUMENTS_ELEMENTS) {
-      keys->SortCurrentElementsList();
+    Isolate* isolate = keys->isolate();
+    uint32_t nof_indices = 0;
+    Handle<FixedArray> indices = isolate->factory()->NewFixedArray(
+        GetCapacityImpl(*object, *backing_store));
+    DirectCollectElementIndicesImpl(isolate, object, backing_store,
+                                    KEEP_NUMBERS, ENUMERABLE_STRINGS, indices,
+                                    &nof_indices);
+    SortIndices(indices, nof_indices);
+    for (uint32_t i = 0; i < nof_indices; i++) {
+      keys->AddKey(indices->get(i));
     }
   }
 
@@ -2745,8 +2750,9 @@ class StringWrapperElementsAccessor
                                         Handle<FixedArrayBase> backing_store,
                                         KeyAccumulator* keys) {
     uint32_t length = GetString(*object)->length();
+    Factory* factory = keys->isolate()->factory();
     for (uint32_t i = 0; i < length; i++) {
-      keys->AddKey(i);
+      keys->AddKey(factory->NewNumberFromUint(i));
     }
     BackingStoreAccessor::CollectElementIndicesImpl(object, backing_store,
                                                     keys);