[esnext] handle elements in FastObjectValuesOrEntries()

src/objects.cc

 // Copyright 2015 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/objects.h"
 
 #include <cmath>
 #include <iomanip>
 #include <sstream>
 
@@ skipping 8725 matching lines @@
   Isolate* isolate = object->GetIsolate();
   KeyAccumulator accumulator(isolate, type, filter);
   MAYBE_RETURN(
       GetKeys_Internal(isolate, object, object, type, filter, &accumulator),
       MaybeHandle<FixedArray>());
   Handle<FixedArray> keys = accumulator.GetKeys(keys_conversion);
   DCHECK(ContainsOnlyValidKeys(keys));
   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) {
   Handle<Map> map(JSReceiver::cast(*receiver)->map(), isolate);
 
   if (!map->IsJSObjectMap()) return Just(false);
   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;
     Handle<Object> prop_value;
 
     // Directly decode from the descriptor array if |from| did not change shape.
     if (stable) {
       PropertyDetails details = descriptors->GetDetails(index);
       if (!details.IsEnumerable()) continue;
       if (details.kind() == kData) {
@@ skipping 17 matching lines @@
       LookupIterator it(object, next_key, LookupIterator::OWN_SKIP_INTERCEPTOR);
       if (!it.IsFound()) continue;
       DCHECK(it.state() == LookupIterator::DATA ||
              it.state() == LookupIterator::ACCESSOR);
       if (!it.IsEnumerable()) continue;
       ASSIGN_RETURN_ON_EXCEPTION_VALUE(
           isolate, prop_value, Object::GetProperty(&it), Nothing<bool>());
     }
 
     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);
     count++;
   }
 
   if (count < values_or_entries->length()) values_or_entries->Shrink(count);
   *result = values_or_entries;
   return Just(true);
 }
@@ skipping 10947 matching lines @@
   if (cell->value() != *new_value) {
     cell->set_value(*new_value);
     Isolate* isolate = cell->GetIsolate();
     cell->dependent_code()->DeoptimizeDependentCodeGroup(
         isolate, DependentCode::kPropertyCellChangedGroup);
   }
 }
 
 }  // namespace internal
 }  // namespace v8