[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..70663e7dae797857e4d5ffd25c26ebbcb5a8ff18 100644
--- a/src/objects.cc
+++ b/src/objects.cc
@@ -8839,14 +8839,36 @@ 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),
-               MaybeHandle<FixedArray>());
-  Handle<FixedArray> keys = accumulator.GetKeys(CONVERT_TO_STRING);
-  DCHECK(ContainsOnlyValidKeys(keys));
+  Handle<FixedArray> keys;
+  bool use_enum_cache = false;
+  bool maybe_enum_cache = filter == ENUMERABLE_STRINGS &&
+                          object->IsJSObject() &&

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

You could probably include the OnlyHasSimpleProperties() check here.
Since you don't want to generate the enum cache for the non-simple case
(intercptors + access checks will be omitted).

+                          JSObject::cast(*object)->elements() ==
+                              isolate->heap()->empty_fixed_array();
+  int enum_length = object->map()->EnumLength();
+
+  if (maybe_enum_cache && enum_length != kInvalidEnumCacheSentinel) {
+    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);
+    use_enum_cache = true;
+  } else if (maybe_enum_cache && object->HasFastProperties()) {

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

With the above OnlyHasSimpleProperties check the HasFastProperties becomes
redundant here.

+    keys = GetFastEnumPropertyKeys(isolate, Handle<JSObject>::cast(object));
+    use_enum_cache = true;
+  } 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>());
+    keys = accumulator.GetKeys(CONVERT_TO_STRING);
+    DCHECK(ContainsOnlyValidKeys(keys));
+  }
 
   Handle<FixedArray> values_or_entries =
       isolate->factory()->NewFixedArray(keys->length());
@@ -8855,7 +8877,7 @@ 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) && !use_enum_cache) {
       PropertyDescriptor descriptor;
       Maybe<bool> did_get_descriptor = JSReceiver::GetOwnPropertyDescriptor(
           isolate, object, key, &descriptor);
@@ -8868,6 +8890,12 @@ MaybeHandle<FixedArray> GetOwnValuesOrEntries(Isolate* isolate,
         isolate, value, JSReceiver::GetPropertyOrElement(object, key),
         MaybeHandle<FixedArray>());
 
+    if (use_enum_cache &&
+        object->map()->EnumLength() == kInvalidEnumCacheSentinel) {

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

How about just doing a map check (see FastAssign in builtins.cc), that would
probably make the intentions even clearer.

[caitp (gmail), 2016/03/02 11:53:21]

my reasoning for this method is that, accessors can mutate the object, but still
use the enum cache if nothing was deleted or descriptors replaced. so some map
changes should be ok---but if it doesn't really work that way, of course

[Camillo Bruni, 2016/03/02 17:50:07]

IMO that would be partially correct. But it could be during the [[Get]] call
above you might change the map *and* initialize the enum-cache, in which case
you probably don't want to rely on the fast path either.

BTW check again the Object.assign fast-path, which uses the same pattern.
I think you should try to directly collect and read the properties if you have a
simple object (you can directly iterate over the enum cache and use the indices
in there for fast-mode objects). This would avoid using the lookup iterator (but
imply an explicit map check). And if you do that, I would factor it out (like in
Object.assign) into a Fast_GetOwnValuesOrEntries.
A follow-up CL might be better suited for this, if you want :).

[caitp (gmail), 2016/03/02 18:17:28]

Maybe I'll do that in a follow-up just so I can seem more busy while I'm billing
hours for this stuff (just kidding Andy, pay no attention to that!)

Sounds good to me, I should have an update in a few hours incorporating that

+      // A property descriptor was replaced, enumerable status may have changed
+      use_enum_cache = false;
+    }
+
     if (get_entries) {
       Handle<FixedArray> entry_storage =
           isolate->factory()->NewUninitializedFixedArray(2);