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

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 8821 matching lines @@
       MaybeHandle<FixedArray>());
   Handle<FixedArray> keys = accumulator.GetKeys(keys_conversion);
   DCHECK(ContainsOnlyValidKeys(keys));
   return keys;
 }
 
 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());
   int length = 0;
 
   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);
       MAYBE_RETURN(did_get_descriptor, MaybeHandle<FixedArray>());
       if (!did_get_descriptor.FromJust() || !descriptor.enumerable()) continue;
     }
 
     Handle<Object> value;
     ASSIGN_RETURN_ON_EXCEPTION_VALUE(
         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);
       entry_storage->set(0, *key);
       entry_storage->set(1, *value);
       value = isolate->factory()->NewJSArrayWithElements(entry_storage,
                                                          FAST_ELEMENTS, 2);
     }
 
     values_or_entries->set(length, *value);
@@ skipping 10943 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