[compiler] Concurrent JSObjectRef::GetOwnConstantElement

This CL implements the above in a concurrent setting without relying
on serialization (except existing serialization to read a consistent
JSObject state, which should be addressed in future work).

There are three main cases in which GetOwnConstantElement can succeed:

- Frozen elements are always constant. The backing store is immutable
after initialization and can be accessed through relaxed reads.
- String wrapper elements are always constant. The JSPrimitiveWrapper
is immutable after initialization, and internalized Strings are
protected by a mutex (other string kinds are currently not handled).
- Dictionary elements may be constant. Since this case is not
particularly important for the optimization, we leave it unimplemented
for now.

Bug: v8:7790
Cq-Include-Trybots: luci.v8.try:v8_linux64_tsan_no_cm_rel_ng
Change-Id: If2fbced50218ebd3930da8157cd2ae5eb83a8e02
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2717308Reviewed-by: Santiago Aboy Solanes <solanes@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#73442}

src/compiler/js-heap-broker.cc

@@ -3972,14 +3972,47 @@ Maybe<double> ObjectRef::OddballToNumber() const {
 
 base::Optional<ObjectRef> JSObjectRef::GetOwnConstantElement(
     uint32_t index, SerializationPolicy policy) const {
-  if (data_->should_access_heap()) {
-    CHECK_EQ(data_->kind(), ObjectDataKind::kUnserializedHeapObject);
-    return GetOwnElementFromHeap(broker(), object(), index, true);
+  if (data_->should_access_heap() || FLAG_turbo_direct_heap_access) {
+    // `elements` are currently still serialized as members of JSObjectRef.
+    // TODO(jgruber,v8:7790): Once JSObject is no longer serialized, we must
+    // guarantee consistency between `object`, `elements_kind` and `elements`
+    // through other means (store/load order? locks? storing elements_kind in
+    // elements.map?).
+    STATIC_ASSERT(IsSerializedHeapObject<JSObject>());
+
+    base::Optional<FixedArrayBaseRef> maybe_elements_ref = elements();
+    if (!maybe_elements_ref.has_value()) {
+      TRACE_BROKER_MISSING(broker(), "JSObject::elements" << *this);
+      return {};
+    }
+
+    FixedArrayBaseRef elements_ref = maybe_elements_ref.value();
+    ElementsKind elements_kind = GetElementsKind();
+
+    DCHECK_LE(index, JSObject::kMaxElementIndex);
+
+    Object maybe_element;
+    auto result = ConcurrentLookupIterator::TryGetOwnConstantElement(
+        &maybe_element, broker()->isolate(), broker()->local_isolate(),
+        *object(), *elements_ref.object(), elements_kind, index);
+
+    if (result == ConcurrentLookupIterator::kGaveUp) {
+      TRACE_BROKER_MISSING(broker(), "JSObject::GetOwnConstantElement on "
+                                         << *this << " at index " << index);
+      return {};
+    } else if (result == ConcurrentLookupIterator::kNotPresent) {
+      return {};
+    }
+
+    DCHECK_EQ(result, ConcurrentLookupIterator::kPresent);
+    return ObjectRef{broker(),
+                     broker()->CanonicalPersistentHandle(maybe_element)};
+  } else {
+    ObjectData* element =
+        data()->AsJSObject()->GetOwnConstantElement(broker(), index, policy);
+    if (element == nullptr) return base::nullopt;
+    return ObjectRef(broker(), element);
   }
-  ObjectData* element =
-      data()->AsJSObject()->GetOwnConstantElement(broker(), index, policy);
-  if (element == nullptr) return base::nullopt;
-  return ObjectRef(broker(), element);
 }
 
 base::Optional<ObjectRef> JSObjectRef::GetOwnFastDataProperty(

src/compiler/serializer-for-background-compilation.cc

@@ -3331,6 +3331,7 @@ void SerializerForBackgroundCompilation::ProcessElementAccess(
         if (key_ref.IsSmi() && key_ref.AsSmi() >= 0) {
           base::Optional<ObjectRef> element;
           if (receiver_ref.IsJSObject()) {
+            receiver_ref.AsJSObject().SerializeElements();
             element = receiver_ref.AsJSObject().GetOwnConstantElement(
                 key_ref.AsSmi(), SerializationPolicy::kSerializeIfNeeded);
             if (!element.has_value() && receiver_ref.IsJSArray()) {
@@ -3338,7 +3339,6 @@ void SerializerForBackgroundCompilation::ProcessElementAccess(
               // cow-array we can exploit the fact that any future write to the
               // element will replace the whole elements storage.
               JSArrayRef array_ref = receiver_ref.AsJSArray();
-              array_ref.SerializeElements();
               array_ref.GetOwnCowElement(
                   array_ref.elements().value(), key_ref.AsSmi(),
                   SerializationPolicy::kSerializeIfNeeded);

src/objects/lookup.cc

@@ -1303,5 +1303,92 @@ base::Optional<Object> ConcurrentLookupIterator::TryGetOwnCowElement(
   return result;
 }
 
+// static
+ConcurrentLookupIterator::Result
+ConcurrentLookupIterator::TryGetOwnConstantElement(
+    Object* result_out, Isolate* isolate, LocalIsolate* local_isolate,
+    JSObject holder, FixedArrayBase elements, ElementsKind elements_kind,
+    size_t index) {
+  DisallowGarbageCollection no_gc;
+
+  DCHECK_LE(index, JSObject::kMaxElementIndex);
+
+  // Own 'constant' elements (PropertyAttributes READ_ONLY|DONT_DELETE) occur in
+  // three main cases:
+  //
+  // 1. Frozen elements: guaranteed constant.
+  // 2. Dictionary elements: may be constant.
+  // 3. String wrapper elements: guaranteed constant.
+
+  // Interesting field reads below:
+  //
+  // - elements.length (immutable on FixedArrays).
+  // - elements[i] (immutable if constant; be careful around dictionaries).
+  // - holder.AsJSPrimitiveWrapper.value.AsString.length (immutable).
+  // - holder.AsJSPrimitiveWrapper.value.AsString[i] (immutable).
+  // - single_character_string_cache()->get().
+
+  if (IsFrozenElementsKind(elements_kind)) {
+    FixedArray elements_fixed_array = FixedArray::cast(elements);
+    if (index >= static_cast<uint32_t>(elements_fixed_array.length())) {
+      return kGaveUp;
+    }
+    Object result = elements_fixed_array.get(isolate, static_cast<int>(index));
+    if (IsHoleyElementsKindForRead(elements_kind) &&
+        result == ReadOnlyRoots(isolate).the_hole_value()) {
+      return kNotPresent;
+    }
+    *result_out = result;
+    return kPresent;
+  } else if (IsDictionaryElementsKind(elements_kind)) {
+    DCHECK(elements.IsNumberDictionary());
+    // TODO(jgruber, v8:7790): Add support. Dictionary elements require racy
+    // NumberDictionary lookups. This should be okay in general (slot iteration
+    // depends only on the dict's capacity), but 1. we'd need to update
+    // NumberDictionary methods to do atomic reads, and 2. the dictionary
+    // elements case isn't very important for callers of this function.
+    return kGaveUp;
+  } else if (IsStringWrapperElementsKind(elements_kind)) {
+    // In this case we don't care about the actual `elements`. All in-bounds
+    // reads are redirected to the wrapped String.
+
+    JSPrimitiveWrapper js_value = JSPrimitiveWrapper::cast(holder);
+    String wrapped_string = String::cast(js_value.value());
+
+    // The access guard below protects only internalized string accesses.
+    // TODO(jgruber): Support other string kinds.
+    Map wrapped_string_map = wrapped_string.synchronized_map(isolate);
+    if (!InstanceTypeChecker::IsInternalizedString(
+            wrapped_string_map.instance_type())) {
+      return kGaveUp;
+    }
+
+    const uint32_t length = static_cast<uint32_t>(wrapped_string.length());
+    if (index >= length) return kGaveUp;
+
+    uint16_t charcode;
+    {
+      SharedStringAccessGuardIfNeeded access_guard(local_isolate);
+      charcode = wrapped_string.Get(static_cast<int>(index));
+    }
+
+    if (charcode > unibrow::Latin1::kMaxChar) return kGaveUp;
+
+    Object value = isolate->factory()->single_character_string_cache()->get(
+        charcode, kRelaxedLoad);
+    if (value == ReadOnlyRoots(isolate).undefined_value()) return kGaveUp;
+
+    *result_out = value;
+    return kPresent;
+  } else {
+    DCHECK(!IsFrozenElementsKind(elements_kind));
+    DCHECK(!IsDictionaryElementsKind(elements_kind));
+    DCHECK(!IsStringWrapperElementsKind(elements_kind));
+    return kGaveUp;
+  }
+
+  UNREACHABLE();
+}
+
 }  // namespace internal
 }  // namespace v8

src/objects/lookup.h

@@ -303,6 +303,13 @@ class V8_EXPORT_PRIVATE LookupIterator final {
 // functionality and constraints are better known.
 class ConcurrentLookupIterator final : public AllStatic {
  public:
+  // Tri-state to distinguish between 'not-present' and 'who-knows' failures.
+  enum Result {
+    kPresent,     // The value was found.
+    kNotPresent,  // No value exists.
+    kGaveUp,      // The operation can't be completed.
+  };
+
   // Implements the own data property lookup for the specialized case of
   // fixed_cow_array backing stores (these are only in use for array literal
   // boilerplates). The contract is that the elements, elements kind, and array
@@ -314,6 +321,13 @@ class ConcurrentLookupIterator final : public AllStatic {
   V8_EXPORT_PRIVATE static base::Optional<Object> TryGetOwnCowElement(
       Isolate* isolate, FixedArray array_elements, ElementsKind elements_kind,
       int array_length, size_t index);
+
+  // Unlike above, the contract is that holder, elements, and elements_kind are
+  // a consistent view of the world; and index must be a valid element index.
+  V8_EXPORT_PRIVATE static Result TryGetOwnConstantElement(
+      Object* result_out, Isolate* isolate, LocalIsolate* local_isolate,
+      JSObject holder, FixedArrayBase elements, ElementsKind elements_kind,
+      size_t index);
 };
 
 }  // namespace internal