[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),
+  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());
   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) && (!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);
       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),
@@ skipping 10955 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