[esnext] handle elements in FastObjectValuesOrEntries()

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index ca21a99f99dc77819e02144d49e221eac2c66004..f3e38f7c3fc9ea4dfd16041887022812f6c39d5b 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -8743,6 +8743,34 @@ MaybeHandle<FixedArray> JSReceiver::GetKeys(Handle<JSReceiver> object,
   return keys;
 }
 
+Handle<Object> MakeEntryPair(Isolate* isolate, uint32_t index, Object* value) {
+  AllowHeapAllocation allow_allocation;
+  Handle<Object> key = isolate->factory()->Uint32ToString(index);
+  Handle<FixedArray> entry_storage =
+      isolate->factory()->NewUninitializedFixedArray(2);

[Camillo Bruni, 2016/03/15 13:36:14]

That's unsafe here, value is still a raw pointer.

[Camillo Bruni, 2016/03/15 13:36:14]

That's unsafe here, value is still a raw pointer.

+  {
+    DisallowHeapAllocation no_allocation;
+    entry_storage->set(0, *key);
+    entry_storage->set(1, value);
+  }
+  return isolate->factory()->NewJSArrayWithElements(entry_storage,
+                                                    FAST_ELEMENTS, 2);
+}
+
+Handle<Object> MakeEntryPair(Isolate* isolate, Handle<Name> key,
+                             Object* value) {
+  AllowHeapAllocation allow_allocation;
+  Handle<FixedArray> entry_storage =
+      isolate->factory()->NewUninitializedFixedArray(2);

[Camillo Bruni, 2016/03/15 13:36:14]

same here.

+  {
+    DisallowHeapAllocation no_allocation;
+    entry_storage->set(0, *key);
+    entry_storage->set(1, value);
+  }
+  return isolate->factory()->NewJSArrayWithElements(entry_storage,
+                                                    FAST_ELEMENTS, 2);
+}
+
 MUST_USE_RESULT Maybe<bool> FastGetOwnValuesOrEntries(
     Isolate* isolate, Handle<JSReceiver> receiver, bool get_entries,
     Handle<FixedArray>* result) {
@@ -8752,18 +8780,109 @@ MUST_USE_RESULT Maybe<bool> FastGetOwnValuesOrEntries(
   if (!map->OnlyHasSimpleProperties()) return Just(false);
 
   Handle<JSObject> object(JSObject::cast(*receiver));
-  if (object->elements() != isolate->heap()->empty_fixed_array()) {
-    return Just(false);
-  }
 
   Handle<DescriptorArray> descriptors(map->instance_descriptors(), isolate);
   int number_of_own_descriptors = map->NumberOfOwnDescriptors();
-  Handle<FixedArray> values_or_entries =
-      isolate->factory()->NewFixedArray(number_of_own_descriptors);
+  int number_of_own_elements = object->NumberOfOwnElements(ALL_PROPERTIES);
+  Handle<FixedArray> values_or_entries = isolate->factory()->NewFixedArray(
+      number_of_own_descriptors + number_of_own_elements);
   int count = 0;
 
   bool stable = true;
 
+  if (object->elements() != isolate->heap()->empty_fixed_array()) {
+    ElementsKind kind = object->GetElementsKind();

[Camillo Bruni, 2016/03/15 13:36:14]

I would feel more comfortable if this where in elements.cc :)

[caitp (gmail), 2016/03/15 15:55:09]

I've moved it

+    switch (kind) {
+      case FAST_SMI_ELEMENTS:
+      case FAST_HOLEY_SMI_ELEMENTS:
+      case FAST_ELEMENTS:
+      case FAST_HOLEY_ELEMENTS: {
+        DisallowHeapAllocation no_allocation;

[Camillo Bruni, 2016/03/15 13:36:14]

again doens't hold... MakeEntryPair allocates an array, potentially triggering a
GC.
Same for all the loops further down.

+        FixedArray* elements = FixedArray::cast(object->elements());
+        for (int i = 0; i < elements->length(); ++i) {
+          Object* value = elements->get(i);
+          if (!value->IsTheHole()) {
+            if (get_entries) {
+              value = *MakeEntryPair(isolate, i, value);
+            }
+            values_or_entries->set(count++, value);
+          }
+        }
+        DCHECK_EQ(object->map(), *map);
+        break;
+      }
+
+      case FAST_DOUBLE_ELEMENTS:
+      case FAST_HOLEY_DOUBLE_ELEMENTS: {
+        FixedDoubleArray* elements = FixedDoubleArray::cast(object->elements());
+        for (int i = 0; i < elements->length(); ++i) {
+          if (!elements->is_the_hole(i)) {
+            Handle<Object> value = FixedDoubleArray::get(elements, i, isolate);
+            if (get_entries) {
+              value = MakeEntryPair(isolate, i, *value);
+            }
+            values_or_entries->set(count++, *value);
+          }
+        }
+        DCHECK_EQ(object->map(), *map);
+        break;
+      }
+
+      case NO_ELEMENTS:
+        break;
+
+      default: {
+        bool is_slow_elements = IsDictionaryElementsKind(kind) ||
+                                IsSloppyArgumentsElements(kind) ||
+                                IsStringWrapperElementsKind(kind);
+        ElementsAccessor* accessor = object->GetElementsAccessor();
+        PropertyFilter filter =
+            is_slow_elements ? ALL_PROPERTIES : ENUMERABLE_STRINGS;
+        KeyAccumulator accumulator(isolate, OWN_ONLY, filter);
+        accumulator.NextPrototype();
+        accessor->CollectElementIndices(object, &accumulator, kMaxUInt32,
+                                        filter);
+        Handle<FixedArray> keys = accumulator.GetKeys();
+
+        if (is_slow_elements) {

[Camillo Bruni, 2016/03/15 13:36:14]

I think this would be automatically cleaner when pushed to elements.cc.
Also I think you can bail out to the slow path at this point... did you you
measure a difference?

[caitp (gmail), 2016/03/15 15:55:09]

The fast elements case definitely makes a big difference --- I'll measure
DICT_ELEMENTS to see if it's really worth it there

+          for (int i = 0; i < keys->length(); ++i) {
+            Handle<Object> key(keys->get(i), isolate);
+            uint32_t index = key->Number();
+            LookupIterator it(isolate, object, index, LookupIterator::OWN);
+            if (it.property_attributes() & ENUMERABLE_STRINGS) continue;
+            if (it.property_details().kind() == kData) {
+              Handle<Object> value = it.GetDataValue();
+              if (get_entries) {
+                value = MakeEntryPair(isolate, index, *value);
+              }
+              values_or_entries->set(count++, *value);
+            } else {
+              Handle<Object> value;
+              ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+                  isolate, value, Object::GetProperty(&it), Nothing<bool>());
+              if (get_entries) {
+                value = MakeEntryPair(isolate, index, *value);
+              }
+              values_or_entries->set(count++, *value);
+              stable = object->map() == *map;

[Camillo Bruni, 2016/03/15 13:36:14]

That's not used anywhere...

[caitp (gmail), 2016/03/15 15:55:09]

It's used when walking instance_descriptors() below

+            }
+          }
+        } else {
+          for (int i = 0; i < keys->length(); ++i) {
+            Handle<Object> key(keys->get(i), isolate);
+            int entry = key->Number();
+            Handle<Object> value = accessor->Get(object, entry);
+            if (get_entries) {
+              value = MakeEntryPair(isolate, entry, *value);
+            }
+            values_or_entries->set(count++, *value);
+          }
+        }
+        break;
+      }
+    }
+  }
+
   for (int index = 0; index < number_of_own_descriptors; index++) {
     Handle<Name> next_key(descriptors->GetKey(index), isolate);
     if (!next_key->IsString()) continue;
@@ -8801,12 +8920,7 @@ MUST_USE_RESULT Maybe<bool> FastGetOwnValuesOrEntries(
     }
 
     if (get_entries) {
-      Handle<FixedArray> entry_storage =
-          isolate->factory()->NewUninitializedFixedArray(2);
-      entry_storage->set(0, *next_key);
-      entry_storage->set(1, *prop_value);
-      prop_value = isolate->factory()->NewJSArrayWithElements(entry_storage,
-                                                              FAST_ELEMENTS, 2);
+      prop_value = MakeEntryPair(isolate, next_key, *prop_value);
     }
 
     values_or_entries->set(count, *prop_value);