[runtime] Remove try_fast path from GetOwnPropertyNames builtin

GetOwnPropertyNameTryFast uses ENUMERABLE_STRINGS filter to trigger fast
path in KeyAccumulator::GetKeys conditionally when all properties on the
receiver are enumerable. It is not easy to verify if all properties are
enumerable and the current check is incorrect in some cases.
For ex: when we have non-enumerable properties when we have elements on
the receiver. This cl removes this try_fast path from the builtin. This
could impact performance. The long term fix for this would be to fix
KeyAccumulator::GetKeys to use fast path for more cases.


Bug: chromium:977870
Change-Id: Iecde730739c2c452ffa0d893d0d1b3612a45d1b2
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1679499Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Mythri Alle <mythria@chromium.org>
Cr-Commit-Position: refs/heads/master@{#62649}

src/builtins/builtins-object-gen.cc

@@ -590,8 +590,7 @@ TF_BUILTIN(ObjectGetOwnPropertyNames, ObjectBuiltinsAssembler) {
   VARIABLE(var_length, MachineRepresentation::kTagged);
   VARIABLE(var_elements, MachineRepresentation::kTagged);
   Label if_empty(this, Label::kDeferred), if_empty_elements(this),
-      if_fast(this), try_fast(this, Label::kDeferred),
-      if_slow(this, Label::kDeferred), if_join(this);
+      if_fast(this), if_slow(this, Label::kDeferred), if_join(this);
 
   // Check if the {object} has a usable enum cache.
   GotoIf(TaggedIsSmi(object), &if_slow);
@@ -601,7 +600,7 @@ TF_BUILTIN(ObjectGetOwnPropertyNames, ObjectBuiltinsAssembler) {
       DecodeWordFromWord32<Map::EnumLengthBits>(object_bit_field3);
   GotoIf(
       WordEqual(object_enum_length, IntPtrConstant(kInvalidEnumCacheSentinel)),
-      &try_fast);
+      &if_slow);
 
   // Ensure that the {object} doesn't have any elements.
   CSA_ASSERT(this, IsJSObjectMap(object_map));
@@ -644,16 +643,6 @@ TF_BUILTIN(ObjectGetOwnPropertyNames, ObjectBuiltinsAssembler) {
     Return(array);
   }
 
-  BIND(&try_fast);
-  {
-    // Let the runtime compute the elements and try initializing enum cache.
-    Node* elements = CallRuntime(Runtime::kObjectGetOwnPropertyNamesTryFast,
-                                 context, object);
-    var_length.Bind(LoadObjectField(elements, FixedArray::kLengthOffset));
-    var_elements.Bind(elements);
-    Goto(&if_join);
-  }
-
   BIND(&if_empty);
   {
     // The {object} doesn't have any enumerable keys.

src/debug/debug-evaluate.cc

@@ -311,7 +311,6 @@ bool IntrinsicHasNoSideEffect(Runtime::FunctionId id) {
   V(ObjectValues)                             \
   V(ObjectValuesSkipFastPath)                 \
   V(ObjectGetOwnPropertyNames)                \
-  V(ObjectGetOwnPropertyNamesTryFast)         \
   V(ObjectIsExtensible)                       \
   V(RegExpInitializeAndCompile)               \
   V(StackGuard)                               \

src/objects/keys.cc

@@ -395,6 +395,11 @@ MaybeHandle<FixedArray> GetOwnKeysWithElements(Isolate* isolate,
 
 MaybeHandle<FixedArray> FastKeyAccumulator::GetKeys(
     GetKeysConversion keys_conversion) {
+  // TODO(v8:9401): We should extend the fast path of KeyAccumulator::GetKeys to
+  // also use fast path even when filter = SKIP_SYMBOLS. We used to pass wrong
+  // filter to use fast path in cases where we tried to verify all properties
+  // are enumerable. However these checks weren't correct and passing the wrong
+  // filter led to wrong behaviour.
   if (filter_ == ENUMERABLE_STRINGS) {
     Handle<FixedArray> keys;
     if (GetKeysFast(keys_conversion).ToHandle(&keys)) {

src/runtime/runtime-object.cc

@@ -217,6 +217,8 @@ RUNTIME_FUNCTION(Runtime_ObjectGetOwnPropertyNames) {
                                      Object::ToObject(isolate, object));
 
   // Collect the own keys for the {receiver}.
+  // TODO(v8:9401): We should extend the fast path of KeyAccumulator::GetKeys to
+  // also use fast path even when filter = SKIP_SYMBOLS.
   Handle<FixedArray> keys;
   ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
       isolate, keys,
@@ -226,36 +228,6 @@ RUNTIME_FUNCTION(Runtime_ObjectGetOwnPropertyNames) {
   return *keys;
 }
 
-RUNTIME_FUNCTION(Runtime_ObjectGetOwnPropertyNamesTryFast) {
-  HandleScope scope(isolate);
-  Handle<Object> object = args.at(0);
-
-  // Convert the {object} to a proper {receiver}.
-  Handle<JSReceiver> receiver;
-  ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, receiver,
-                                     Object::ToObject(isolate, object));
-
-  Handle<Map> map(receiver->map(), isolate);
-
-  int nod = map->NumberOfOwnDescriptors();
-  Handle<FixedArray> keys;
-  if (nod != 0 && map->NumberOfEnumerableProperties() == nod) {
-    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-        isolate, keys,
-        KeyAccumulator::GetKeys(receiver, KeyCollectionMode::kOwnOnly,
-                                ENUMERABLE_STRINGS,
-                                GetKeysConversion::kConvertToString));
-  } else {
-    ASSIGN_RETURN_FAILURE_ON_EXCEPTION(
-        isolate, keys,
-        KeyAccumulator::GetKeys(receiver, KeyCollectionMode::kOwnOnly,
-                                SKIP_SYMBOLS,
-                                GetKeysConversion::kConvertToString));
-  }
-
-  return *keys;
-}
-
 // ES6 19.1.3.2
 RUNTIME_FUNCTION(Runtime_ObjectHasOwnProperty) {
   HandleScope scope(isolate);

src/runtime/runtime.h

@@ -310,7 +310,6 @@ namespace internal {
   F(ObjectEntries, 1, 1)                                        \
   F(ObjectEntriesSkipFastPath, 1, 1)                            \
   F(ObjectGetOwnPropertyNames, 1, 1)                            \
-  F(ObjectGetOwnPropertyNamesTryFast, 1, 1)                     \
   F(ObjectHasOwnProperty, 2, 1)                                 \
   F(ObjectIsExtensible, 1, 1)                                   \
   F(ObjectKeys, 1, 1)                                           \

test/mjsunit/regress/regress-977870.js

+// Copyright 2019 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.
+
+function f() {
+  v_0 = {};
+  Object.defineProperty(v_0, '0', {});
+  v_0.p_0 = 0;
+  assertArrayEquals(['0', 'p_0'],
+                    Object.getOwnPropertyNames(v_0));
+  assertArrayEquals(['0', 'p_0'],
+                    Object.getOwnPropertyNames(v_0));
+}
+f();