[inspector] Speed up DebugPropertyIterator.

This unblocks https://crrev.com/c/3099011 by speeding up the case for
the DebugPropertyIterator where only non-indexed properties (for large
arrays or typed arrays) are requested. Previously we'd walk through all
properties - including all indexed properties - and only filter out the
indexed properties in the end in `ValueMirror::getProperties()`.

Bug: chromium:1199701, chromium:1162229
Change-Id: I2555e3129fef29da347314eee400ea97ebf5e5b7
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3114135
Commit-Queue: Benedikt Meurer <bmeurer@chromium.org>
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Kim-Anh Tran <kimanh@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76796}

src/debug/debug-interface.cc

@@ -1267,7 +1267,7 @@ MaybeLocal<Message> GetMessageFromPromise(Local<Promise> p) {
 }
 
 std::unique_ptr<PropertyIterator> PropertyIterator::Create(
-    Local<Context> context, Local<Object> object) {
+    Local<Context> context, Local<Object> object, bool skip_indices) {
   internal::Isolate* isolate =
       reinterpret_cast<i::Isolate*>(object->GetIsolate());
   if (IsExecutionTerminatingCheck(isolate)) {
@@ -1275,8 +1275,8 @@ std::unique_ptr<PropertyIterator> PropertyIterator::Create(
   }
   CallDepthScope<false> call_depth_scope(isolate, context);
 
-  auto result =
-      i::DebugPropertyIterator::Create(isolate, Utils::OpenHandle(*object));
+  auto result = i::DebugPropertyIterator::Create(
+      isolate, Utils::OpenHandle(*object), skip_indices);
   if (!result) {
     DCHECK(isolate->has_pending_exception());
     call_depth_scope.Escape();

src/debug/debug-interface.h

@@ -621,7 +621,8 @@ class V8_EXPORT_PRIVATE PropertyIterator {
   // Creating a PropertyIterator can potentially throw an exception.
   // The returned std::unique_ptr is empty iff that happens.
   V8_WARN_UNUSED_RESULT static std::unique_ptr<PropertyIterator> Create(
-      v8::Local<v8::Context> context, v8::Local<v8::Object> object);
+      v8::Local<v8::Context> context, v8::Local<v8::Object> object,
+      bool skip_indices = false);
 
   virtual ~PropertyIterator() = default;
 

src/debug/debug-property-iterator.cc

@@ -15,15 +15,14 @@ namespace v8 {
 namespace internal {
 
 std::unique_ptr<DebugPropertyIterator> DebugPropertyIterator::Create(
-    Isolate* isolate, Handle<JSReceiver> receiver) {
+    Isolate* isolate, Handle<JSReceiver> receiver, bool skip_indices) {
   // Can't use std::make_unique as Ctor is private.
   auto iterator = std::unique_ptr<DebugPropertyIterator>(
-      new DebugPropertyIterator(isolate, receiver));
+      new DebugPropertyIterator(isolate, receiver, skip_indices));
 
   if (receiver->IsJSProxy()) {
     iterator->AdvanceToPrototype();
   }
-  if (iterator->Done()) return iterator;
 
   if (!iterator->FillKeysForCurrentPrototypeAndStage()) return nullptr;
   if (iterator->should_move_to_next_stage() && !iterator->AdvanceInternal()) {
@@ -34,10 +33,15 @@ std::unique_ptr<DebugPropertyIterator> DebugPropertyIterator::Create(
 }
 
 DebugPropertyIterator::DebugPropertyIterator(Isolate* isolate,
-                                             Handle<JSReceiver> receiver)
+                                             Handle<JSReceiver> receiver,
+                                             bool skip_indices)
     : isolate_(isolate),
       prototype_iterator_(isolate, receiver, kStartAtReceiver,
-                          PrototypeIterator::END_AT_NULL) {}
+                          PrototypeIterator::END_AT_NULL),
+      skip_indices_(skip_indices),
+      current_key_index_(0),
+      current_keys_(isolate_->factory()->empty_fixed_array()),
+      current_keys_length_(0) {}
 
 bool DebugPropertyIterator::Done() const { return is_done_; }
 
@@ -54,13 +58,13 @@ bool DebugPropertyIterator::AdvanceInternal() {
   calculated_native_accessor_flags_ = false;
   while (should_move_to_next_stage()) {
     switch (stage_) {
-      case Stage::kExoticIndices:
-        stage_ = Stage::kEnumerableStrings;
+      case kExoticIndices:
+        stage_ = kEnumerableStrings;
         break;
-      case Stage::kEnumerableStrings:
-        stage_ = Stage::kAllProperties;
+      case kEnumerableStrings:
+        stage_ = kAllProperties;
         break;
-      case Stage::kAllProperties:
+      case kAllProperties:
         AdvanceToPrototype();
         break;
     }
@@ -70,20 +74,17 @@ bool DebugPropertyIterator::AdvanceInternal() {
 }
 
 bool DebugPropertyIterator::is_native_accessor() {
-  if (stage_ == kExoticIndices) return false;
   CalculateNativeAccessorFlags();
   return native_accessor_flags_;
 }
 
 bool DebugPropertyIterator::has_native_getter() {
-  if (stage_ == kExoticIndices) return false;
   CalculateNativeAccessorFlags();
   return native_accessor_flags_ &
          static_cast<int>(debug::NativeAccessorType::HasGetter);
 }
 
 bool DebugPropertyIterator::has_native_setter() {
-  if (stage_ == kExoticIndices) return false;
   CalculateNativeAccessorFlags();
   return native_accessor_flags_ &
          static_cast<int>(debug::NativeAccessorType::HasSetter);
@@ -95,7 +96,7 @@ Handle<Name> DebugPropertyIterator::raw_name() const {
     return isolate_->factory()->SizeToString(current_key_index_);
   } else {
     return Handle<Name>::cast(FixedArray::get(
-        *keys_, static_cast<int>(current_key_index_), isolate_));
+        *current_keys_, static_cast<int>(current_key_index_), isolate_));
   }
 }
 
@@ -140,42 +141,38 @@ bool DebugPropertyIterator::is_own() { return is_own_; }
 
 bool DebugPropertyIterator::is_array_index() {
   if (stage_ == kExoticIndices) return true;
-  uint32_t index = 0;
-  return raw_name()->AsArrayIndex(&index);
+  PropertyKey key(isolate_, raw_name());
+  return key.is_element();
 }
 
 bool DebugPropertyIterator::FillKeysForCurrentPrototypeAndStage() {
   current_key_index_ = 0;
-  exotic_length_ = 0;
-  keys_ = Handle<FixedArray>::null();
+  current_keys_ = isolate_->factory()->empty_fixed_array();
+  current_keys_length_ = 0;
   if (is_done_) return true;
   Handle<JSReceiver> receiver =
       PrototypeIterator::GetCurrent<JSReceiver>(prototype_iterator_);
-  bool has_exotic_indices = receiver->IsJSTypedArray();
   if (stage_ == kExoticIndices) {
-    if (!has_exotic_indices) return true;
+    if (skip_indices_ || !receiver->IsJSTypedArray()) return true;
     Handle<JSTypedArray> typed_array = Handle<JSTypedArray>::cast(receiver);
-    exotic_length_ = typed_array->WasDetached() ? 0 : typed_array->length();
+    current_keys_length_ =
+        typed_array->WasDetached() ? 0 : typed_array->length();
     return true;
   }
-  bool skip_indices = has_exotic_indices;
   PropertyFilter filter =
       stage_ == kEnumerableStrings ? ENUMERABLE_STRINGS : ALL_PROPERTIES;
-  if (!KeyAccumulator::GetKeys(receiver, KeyCollectionMode::kOwnOnly, filter,
+  if (KeyAccumulator::GetKeys(receiver, KeyCollectionMode::kOwnOnly, filter,
                               GetKeysConversion::kConvertToString, false,
-                               skip_indices)
-           .ToHandle(&keys_)) {
-    keys_ = Handle<FixedArray>::null();
-    return false;
-  }
+                              skip_indices_ || receiver->IsJSTypedArray())
+          .ToHandle(&current_keys_)) {
+    current_keys_length_ = current_keys_->length();
     return true;
+  }
+  return false;
 }
 
 bool DebugPropertyIterator::should_move_to_next_stage() const {
-  if (is_done_) return false;
-  if (stage_ == kExoticIndices) return current_key_index_ >= exotic_length_;
-  return keys_.is_null() ||
-         current_key_index_ >= static_cast<size_t>(keys_->length());
+  return !is_done_ && current_key_index_ >= current_keys_length_;
 }
 
 namespace {
@@ -210,10 +207,14 @@ base::Flags<debug::NativeAccessorType, int> GetNativeAccessorDescriptorInternal(
 
 void DebugPropertyIterator::CalculateNativeAccessorFlags() {
   if (calculated_native_accessor_flags_) return;
+  if (stage_ == kExoticIndices) {
+    native_accessor_flags_ = 0;
+  } else {
     Handle<JSReceiver> receiver =
         PrototypeIterator::GetCurrent<JSReceiver>(prototype_iterator_);
     native_accessor_flags_ =
         GetNativeAccessorDescriptorInternal(receiver, raw_name());
+  }
   calculated_native_accessor_flags_ = true;
 }
 }  // namespace internal

src/debug/debug-property-iterator.h

@@ -24,7 +24,7 @@ class JSReceiver;
 class DebugPropertyIterator final : public debug::PropertyIterator {
  public:
   V8_WARN_UNUSED_RESULT static std::unique_ptr<DebugPropertyIterator> Create(
-      Isolate* isolate, Handle<JSReceiver> receiver);
+      Isolate* isolate, Handle<JSReceiver> receiver, bool skip_indices);
   ~DebugPropertyIterator() override = default;
   DebugPropertyIterator(const DebugPropertyIterator&) = delete;
   DebugPropertyIterator& operator=(const DebugPropertyIterator&) = delete;
@@ -43,7 +43,8 @@ class DebugPropertyIterator final : public debug::PropertyIterator {
   bool is_array_index() override;
 
  private:
-  DebugPropertyIterator(Isolate* isolate, Handle<JSReceiver> receiver);
+  DebugPropertyIterator(Isolate* isolate, Handle<JSReceiver> receiver,
+                        bool skip_indices);
 
   V8_WARN_UNUSED_RESULT bool FillKeysForCurrentPrototypeAndStage();
   bool should_move_to_next_stage() const;
@@ -54,12 +55,16 @@ class DebugPropertyIterator final : public debug::PropertyIterator {
 
   Isolate* isolate_;
   PrototypeIterator prototype_iterator_;
-  enum Stage { kExoticIndices = 0, kEnumerableStrings = 1, kAllProperties = 2 };
-  Stage stage_ = kExoticIndices;
-
-  size_t current_key_index_ = 0;
-  Handle<FixedArray> keys_;
-  size_t exotic_length_ = 0;
+  enum {
+    kExoticIndices = 0,
+    kEnumerableStrings = 1,
+    kAllProperties = 2
+  } stage_ = kExoticIndices;
+  bool skip_indices_;
+
+  size_t current_key_index_;
+  Handle<FixedArray> current_keys_;
+  size_t current_keys_length_;
 
   bool calculated_native_accessor_flags_ = false;
   int native_accessor_flags_ = 0;

src/inspector/value-mirror.cc

@@ -1211,7 +1211,8 @@ bool ValueMirror::getProperties(v8::Local<v8::Context> context,
     }
   }
 
-  auto iterator = v8::debug::PropertyIterator::Create(context, object);
+  auto iterator = v8::debug::PropertyIterator::Create(context, object,
+                                                      nonIndexedPropertiesOnly);
   if (!iterator) {
     CHECK(tryCatch.HasCaught());
     return false;
@@ -1219,14 +1220,6 @@ bool ValueMirror::getProperties(v8::Local<v8::Context> context,
   while (!iterator->Done()) {
     bool isOwn = iterator->is_own();
     if (!isOwn && ownProperties) break;
-    bool isIndex = iterator->is_array_index();
-    if (isIndex && nonIndexedPropertiesOnly) {
-      if (!iterator->Advance().FromMaybe(false)) {
-        CHECK(tryCatch.HasCaught());
-        return false;
-      }
-      continue;
-    }
     v8::Local<v8::Name> v8Name = iterator->name();
     v8::Maybe<bool> result = set->Has(context, v8Name);
     if (result.IsNothing()) return false;
@@ -1324,7 +1317,7 @@ bool ValueMirror::getProperties(v8::Local<v8::Context> context,
                                  configurable,
                                  enumerable,
                                  isOwn,
-                                 isIndex,
+                                 iterator->is_array_index(),
                                  isAccessorProperty && valueMirror,
                                  std::move(valueMirror),
                                  std::move(getterMirror),

test/unittests/debug/debug-property-iterator-unittest.cc

@@ -98,6 +98,56 @@ TEST_F(DebugPropertyIteratorTest, DoestWalksPrototypeChainIfInaccesible) {
   ASSERT_TRUE(iterator->Done());
 }
 
+TEST_F(DebugPropertyIteratorTest, SkipsIndicesOnArrays) {
+  TryCatch try_catch(isolate());
+
+  Local<Value> elements[2] = {
+      Number::New(isolate(), 21),
+      Number::New(isolate(), 42),
+  };
+  auto array = Array::New(isolate(), elements, arraysize(elements));
+
+  auto iterator = PropertyIterator::Create(context(), array, true);
+  while (!iterator->Done()) {
+    ASSERT_FALSE(iterator->is_array_index());
+    ASSERT_TRUE(iterator->Advance().FromMaybe(false));
+  }
+}
+
+TEST_F(DebugPropertyIteratorTest, SkipsIndicesOnObjects) {
+  TryCatch try_catch(isolate());
+
+  Local<Name> names[2] = {
+      String::NewFromUtf8Literal(isolate(), "42"),
+      String::NewFromUtf8Literal(isolate(), "x"),
+  };
+  Local<Value> values[arraysize(names)] = {
+      Number::New(isolate(), 42),
+      Number::New(isolate(), 21),
+  };
+  Local<Object> object =
+      Object::New(isolate(), Null(isolate()), names, values, arraysize(names));
+
+  auto iterator = PropertyIterator::Create(context(), object, true);
+  while (!iterator->Done()) {
+    ASSERT_FALSE(iterator->is_array_index());
+    ASSERT_TRUE(iterator->Advance().FromMaybe(false));
+  }
+}
+
+TEST_F(DebugPropertyIteratorTest, SkipsIndicesOnTypedArrays) {
+  TryCatch try_catch(isolate());
+
+  auto buffer = ArrayBuffer::New(isolate(), 1024 * 1024);
+  auto array = Uint8Array::New(buffer, 0, 1024 * 1024);
+
+  auto iterator = PropertyIterator::Create(context(), array, true);
+  while (!iterator->Done()) {
+    ASSERT_FALSE(iterator->is_array_index());
+    ASSERT_TRUE(iterator->Advance().FromMaybe(false));
+  }
+}
+
 }  // namespace
 }  // namespace debug
 }  // namespace v8