[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..d7446704bdc10d244eb27fe1da697b6e1c14d9a6 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -8839,14 +8839,31 @@ MaybeHandle<FixedArray> GetOwnValuesOrEntries(Isolate* isolate,
                                               Handle<JSReceiver> object,
                                               PropertyFilter filter,
                                               bool get_entries) {
-  PropertyFilter key_filter =
-      static_cast<PropertyFilter>(filter & ~ONLY_ENUMERABLE);
-  KeyAccumulator accumulator(isolate, OWN_ONLY, key_filter);
-  MAYBE_RETURN(GetKeys_Internal(isolate, object, object, OWN_ONLY, key_filter,
-                                &accumulator),
+  Handle<FixedArray> keys;
+  bool did_use_enum_cache = false;
+  int enum_length = object->map()->EnumLength();
+  if ((filter == ENUMERABLE_STRINGS) &&
+      enum_length != kInvalidEnumCacheSentinel &&
+      JSObject::cast(*object)->elements() ==
+          isolate->heap()->empty_fixed_array()) {
+    DCHECK(!object->IsJSProxy());
+    if (enum_length == 0) {
+      return isolate->factory()->empty_fixed_array();
+    }
+    Handle<FixedArray> cache(
+          object->map()->instance_descriptors()->GetEnumCache());
+    keys = isolate->factory()->CopyFixedArrayUpTo(cache, enum_length);
+    did_use_enum_cache = true;

[Camillo Bruni, 2016/03/01 20:50:13]

I've got a long-term project running that eventually will handle this directly
in the KeyAccumulator.
See https://codereview.chromium.org/1707743002

[caitp (gmail), 2016/03/01 21:02:40]

It's not just for getting the property keys that I want to use this --- I'd like
to be able to use this to skip the [[GetOwnProperty]] call on line 8878, if I
can guarantee that the enum status hasn't changed at any point during the
algorithm.

I think that makes sense, what do you think?

[Camillo Bruni, 2016/03/02 09:21:51]

Right, probably we could make the new key accumulator thingy store whether it
just used the enum cache (aka SimpleEnum case). Then later on you just have to
check whether the map changed or not. We do the same in Object.assign
(FastAssign in builtins.cc).

+  } else {
+    PropertyFilter key_filter =
+        static_cast<PropertyFilter>(filter & ~ONLY_ENUMERABLE);
+    KeyAccumulator accumulator(isolate, OWN_ONLY, key_filter);
+    MAYBE_RETURN(GetKeys_Internal(isolate, object, object, OWN_ONLY, key_filter,
+                                  &accumulator),
                MaybeHandle<FixedArray>());
-  Handle<FixedArray> keys = accumulator.GetKeys(CONVERT_TO_STRING);
-  DCHECK(ContainsOnlyValidKeys(keys));
+    keys = accumulator.GetKeys(CONVERT_TO_STRING);
+    DCHECK(ContainsOnlyValidKeys(keys));
+  }
 
   Handle<FixedArray> values_or_entries =
       isolate->factory()->NewFixedArray(keys->length());
@@ -8855,7 +8872,8 @@ MaybeHandle<FixedArray> GetOwnValuesOrEntries(Isolate* isolate,
   for (int i = 0; i < keys->length(); ++i) {
     Handle<Name> key = Handle<Name>::cast(handle(keys->get(i), isolate));
 
-    if (filter & ONLY_ENUMERABLE) {
+    if ((filter & ONLY_ENUMERABLE) && (!did_use_enum_cache ||
+        object->map()->EnumLength() == kInvalidEnumCacheSentinel)) {

[Camillo Bruni, 2016/03/01 20:50:13]

why the redundant check for the enum_length again here?
did_use_enum_cache implies (object->map()->EnumLength() !=
kInvalidEnumCacheSentinel)?
and if did_use_enum_cache = false you don't care about the enum_length anyway.

[caitp (gmail), 2016/03/01 21:02:40]

On line 8885, we [[GET]] the property, which can replace or add descriptors and
invalidate the enum cache. So, the idea here is, the work can be skipped if the
current EnumLength() is the sentinel. I've cleaned this up a little bit in the
newest patchset

[caitp (gmail), 2016/03/01 21:12:24]

***work can be skipped if the EnumLength() is __not__ the sentinel value, oops
:<

[Camillo Bruni, 2016/03/02 09:21:51]

I realised this once I fell asleep ;) too tired to see the common pattern.

       PropertyDescriptor descriptor;
       Maybe<bool> did_get_descriptor = JSReceiver::GetOwnPropertyDescriptor(
           isolate, object, key, &descriptor);