[esnext] use map instance_descriptors() when possible in Object.values/entries()

src/objects.cc

Index: src/objects.cc
diff --git a/src/objects.cc b/src/objects.cc
index b626e447b75a48cdccb0398b98ebd512610e1157..1f62435029038c2f5ca4324e2a8455ba29250878 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -8835,10 +8835,155 @@ MaybeHandle<FixedArray> JSReceiver::GetKeys(Handle<JSReceiver> object,
   return keys;
 }
 
+MUST_USE_RESULT Maybe<bool> GetOwnElementValuesOrEntries(
+    Isolate* isolate, Handle<JSObject> object, bool get_entries,
+    Handle<FixedArray> values_or_entries, int num_elements, int* count) {
+  Handle<FixedArray> keys = isolate->factory()->NewFixedArray(num_elements);
+  object->GetOwnElementKeys(*keys, ENUMERABLE_STRINGS);
+
+  int elements = 0;
+
+  for (int i = 0; i < num_elements; ++i) {
+    Handle<Object> next_key(keys->get(i), isolate);
+    Handle<Object> prop_value;
+
+    bool success;
+    LookupIterator it =
+        LookupIterator::PropertyOrElement(isolate, object, next_key, &success,
+                                          LookupIterator::OWN_SKIP_INTERCEPTOR);

[Camillo Bruni, 2016/03/04 09:40:25]

I think you will only have the full benefits when implementing this directly in
ElementsAccessor in elements.cc.
If you implement it in elements.cc you won't have to get a LookupIterator (even
though that's most likely to be inlined here).

[caitp (gmail), 2016/03/04 16:34:33]

Acknowledged.

+    USE(success);
+
+    if (!it.IsFound()) continue;
+    DCHECK(it.state() == LookupIterator::DATA ||
+           it.state() == LookupIterator::ACCESSOR);
+    if (!it.IsEnumerable()) continue;

[Camillo Bruni, 2016/03/04 09:40:25]

For instance the enumerability check would only be necessary for
DICTIONARY_ELEMENTS

[caitp (gmail), 2016/03/04 16:34:33]

Acknowledged.

+
+    PropertyDetails details = it.property_details();
+    if (details.kind() == kData) {
+      prop_value = it.GetDataValue();
+    } else {
+      ASSIGN_RETURN_ON_EXCEPTION_VALUE(
+          isolate, prop_value, Object::GetProperty(&it), Nothing<bool>());

[Camillo Bruni, 2016/03/04 09:40:25]

Same here, you can only have a getter with DICTIONARY_ELEMENTS

[caitp (gmail), 2016/03/04 16:34:33]

Acknowledged.

+    }
+
+    if (get_entries) {
+      Handle<FixedArray> entry_storage =
+          isolate->factory()->NewUninitializedFixedArray(2);
+
+      if (!next_key->IsName()) {

[Camillo Bruni, 2016/03/04 09:40:25]

element keys should always be Numbers.
DCHECK(next_key->IsNumber()) and
Uint32ToString(Number::cast(*next_key)->value()) might avoid some additional
indirections.

[caitp (gmail), 2016/03/04 16:34:33]

Dictionary gave me the impression that they could be represented in the array as
Strings, I guess not though.

Anyways, I think this whole routine can be tackled in a followup patch (I will
opt to land this patch where the fast path is not taken when elements are
present).

[Camillo Bruni, 2016/03/04 17:23:26]

Sure :).

DICTIONARY_ELEMENTS is used for elements only (indices), but uses a dictionary
to store the indices => value mapping. You currently get that if the indices are
out of smi, but in the precise double range, or if you define a non-simple
property with an index key (elements.cc tries to be your friend... but I agree
it's rather complex).

+        DCHECK(next_key->IsNumber());
+        Handle<String> property_name;
+        ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate, property_name,
+                                         Object::ToString(isolate, next_key),
+                                         Nothing<bool>());
+        next_key = property_name;
+      }
+      entry_storage->set(0, *next_key);
+      entry_storage->set(1, *prop_value);
+      prop_value = isolate->factory()->NewJSArrayWithElements(entry_storage,
+                                                              FAST_ELEMENTS, 2);
+    }
+
+    values_or_entries->set(elements, *prop_value);
+    elements++;
+  }
+
+  *count = elements;
+  return Just(true);
+}
+
+MUST_USE_RESULT Maybe<bool> FastGetOwnValuesOrEntries(
+    Isolate* isolate, Handle<JSObject> object, bool get_entries,
+    Handle<FixedArray>* result) {
+  Handle<Map> map(JSReceiver::cast(*object)->map(), isolate);
+
+  if (!map->IsJSObjectMap()) return Just(false);
+  if (!map->OnlyHasSimpleProperties()) return Just(false);
+
+  Handle<DescriptorArray> descriptors(map->instance_descriptors(), isolate);
+  int length = map->NumberOfOwnDescriptors();
+  int elements_length = object->NumberOfOwnElements(ENUMERABLE_STRINGS);
+  Handle<FixedArray> values_or_entries =
+      isolate->factory()->NewFixedArray(length + elements_length);
+  int count = 0;
+
+  bool stable = true;
+
+  if (object->elements() != isolate->heap()->empty_fixed_array()) {
+    MAYBE_RETURN(GetOwnElementValuesOrEntries(isolate, object, get_entries,
+                                              values_or_entries,
+                                              elements_length, &count),
+                 Nothing<bool>());
+    stable = object->map() == *map;
+  }
+
+  for (int index = 0; index < length; 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) {
+        if (details.location() == kDescriptor) {
+          prop_value = handle(descriptors->GetValue(index), isolate);
+        } else {
+          Representation representation = details.representation();
+          FieldIndex field_index = FieldIndex::ForDescriptor(*map, index);
+          prop_value =
+              JSObject::FastPropertyAt(object, representation, field_index);
+        }
+      } else {
+        ASSIGN_RETURN_ON_EXCEPTION_VALUE(isolate, prop_value,
+                                         Object::GetProperty(object, next_key),
+                                         Nothing<bool>());
+        stable = object->map() == *map;
+      }
+    } else {
+      // If the map did change, do a slower lookup. We are still guaranteed that
+      // the object has a simple shape, and that the key is a name.
+      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);
+    }
+
+    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);
+}
+
 MaybeHandle<FixedArray> GetOwnValuesOrEntries(Isolate* isolate,
                                               Handle<JSReceiver> object,
                                               PropertyFilter filter,
                                               bool get_entries) {
+  Handle<FixedArray> values_or_entries;
+  if (filter == ENUMERABLE_STRINGS && object->IsJSObject()) {
+    Maybe<bool> fast_values_or_entries =
+        FastGetOwnValuesOrEntries(isolate, Handle<JSObject>::cast(object),
+                                  get_entries, &values_or_entries);
+    if (fast_values_or_entries.IsNothing()) return MaybeHandle<FixedArray>();
+    if (fast_values_or_entries.FromJust()) return values_or_entries;
+  }
+
   PropertyFilter key_filter =
       static_cast<PropertyFilter>(filter & ~ONLY_ENUMERABLE);
   KeyAccumulator accumulator(isolate, OWN_ONLY, key_filter);
@@ -8848,8 +8993,7 @@ MaybeHandle<FixedArray> GetOwnValuesOrEntries(Isolate* isolate,
   Handle<FixedArray> keys = accumulator.GetKeys(CONVERT_TO_STRING);
   DCHECK(ContainsOnlyValidKeys(keys));
 
-  Handle<FixedArray> values_or_entries =
-      isolate->factory()->NewFixedArray(keys->length());
+  values_or_entries = isolate->factory()->NewFixedArray(keys->length());
   int length = 0;
 
   for (int i = 0; i < keys->length(); ++i) {