[compiler] Direct heap reads for JSArrayRef

There are two aspects to the non-JSObject parts of JSArrayRef:

- JSArrayRef::length. Relevant only in two spots, 1. when reading
(immutable) array boilerplates and 2. for GetOwnCowElement.

- JSArrayRef::GetOwnCowElement. May read into a copy-on-write backing
store. Relies on the invariant that cow backing stores are immutable.

This CL renames the length accessor to length_unsafe to make the
danger explicit at callsites.

For GetOwnCowElement the refactor is slightly larger, since we now
need to read into the backing store while keeping full control of
object reads (e.g. JSArray::length and JSArray::elements_kind). We
make all reads explicit at the call site by requiring that elements,
elements kind, and length are passed in as arguments to
GetOwnCowElement. Inside GetOwnCowElement, consistency between these
is *not* guaranteed due to concurrency. At runtime, consistency *is*
guaranteed through the reference-equality check on the elements seen
during compilation. The actual elements read is implemented in
ConcurrentLookupIterator::GetOwnCowElement.

Bug: v8:7790
Change-Id: I9aa169ce4f2b1e2bfe1e9232007669eb7654a995
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2695403
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72834}

src/compiler/heap-refs.h

@@ -834,13 +834,24 @@ class JSArrayRef : public JSObjectRef {
 
   Handle<JSArray> object() const;
 
-  ObjectRef length() const;
+  // The `length` property of boilerplate JSArray objects. Boilerplates are
+  // immutable after initialization. Must not be used for non-boilerplate
+  // JSArrays.
+  ObjectRef GetBoilerplateLength() const;
 
   // Return the element at key {index} if the array has a copy-on-write elements
   // storage and {index} is known to be an own data property.
+  // Note the value returned by this function is only valid if we ensure at
+  // runtime that the backing store has not changed.
   base::Optional<ObjectRef> GetOwnCowElement(
-      uint32_t index, SerializationPolicy policy =
-                          SerializationPolicy::kAssumeSerialized) const;
+      FixedArrayBaseRef elements_ref, uint32_t index,
+      SerializationPolicy policy =
+          SerializationPolicy::kAssumeSerialized) const;
+
+  // The `JSArray::length` property; not safe to use in general, but can be
+  // used in some special cases that guarantee a valid `length` value despite
+  // concurrent reads.
+  ObjectRef length_unsafe() const;
 };
 
 class ScopeInfoRef : public HeapObjectRef {

src/compiler/js-create-lowering.cc

@@ -1735,7 +1735,7 @@ Node* JSCreateLowering::AllocateFastLiteral(Node* effect, Node* control,
     JSArrayRef boilerplate_array = boilerplate.AsJSArray();
     builder.Store(
         AccessBuilder::ForJSArrayLength(boilerplate_array.GetElementsKind()),
-        boilerplate_array.length());
+        boilerplate_array.GetBoilerplateLength());
   }
   for (auto const& inobject_field : inobject_fields) {
     builder.Store(inobject_field.first, inobject_field.second);

src/compiler/js-heap-broker.cc

@@ -506,6 +506,7 @@ void JSObjectData::SerializeObjectCreateMap(JSHeapBroker* broker) {
 }
 
 namespace {
+
 base::Optional<ObjectRef> GetOwnElementFromHeap(JSHeapBroker* broker,
                                                 Handle<Object> receiver,
                                                 uint32_t index,
@@ -1799,7 +1800,10 @@ class JSArrayData : public JSObjectData {
               Handle<JSArray> object);
 
   void Serialize(JSHeapBroker* broker);
-  ObjectData* length() const { return length_; }
+  ObjectData* length() const {
+    CHECK(serialized_);
+    return length_;
+  }
 
   ObjectData* GetOwnElement(
       JSHeapBroker* broker, uint32_t index,
@@ -1821,6 +1825,8 @@ JSArrayData::JSArrayData(JSHeapBroker* broker, ObjectData** storage,
     : JSObjectData(broker, storage, object), own_elements_(broker->zone()) {}
 
 void JSArrayData::Serialize(JSHeapBroker* broker) {
+  CHECK(!FLAG_turbo_direct_heap_access);
+
   if (serialized_) return;
   serialized_ = true;
 
@@ -2219,7 +2225,10 @@ bool JSObjectData::cow_or_empty_elements_tenured() const {
   return cow_or_empty_elements_tenured_;
 }
 
-ObjectData* JSObjectData::elements() const { return elements_; }
+ObjectData* JSObjectData::elements() const {
+  CHECK(serialized_elements_ || serialized_as_boilerplate_);
+  return elements_;
+}
 
 void JSObjectData::SerializeAsBoilerplate(JSHeapBroker* broker) {
   SerializeRecursiveAsBoilerplate(broker, kMaxFastLiteralDepth);
@@ -2420,7 +2429,9 @@ void JSObjectData::SerializeRecursiveAsBoilerplate(JSHeapBroker* broker,
     map()->AsMap()->SerializeOwnDescriptors(broker);
   }
 
-  if (IsJSArray()) AsJSArray()->Serialize(broker);
+  if (IsJSArray() && !FLAG_turbo_direct_heap_access) {
+    AsJSArray()->Serialize(broker);
+  }
 }
 
 void RegExpBoilerplateDescriptionData::Serialize(JSHeapBroker* broker) {
@@ -3474,8 +3485,6 @@ BIMODAL_ACCESSOR(HeapObject, Map, map)
 
 BIMODAL_ACCESSOR_C(HeapNumber, double, value)
 
-BIMODAL_ACCESSOR(JSArray, Object, length)
-
 BIMODAL_ACCESSOR(JSBoundFunction, JSReceiver, bound_target_function)
 BIMODAL_ACCESSOR(JSBoundFunction, Object, bound_this)
 BIMODAL_ACCESSOR(JSBoundFunction, FixedArray, bound_arguments)
@@ -4001,8 +4010,68 @@ base::Optional<ObjectRef> JSObjectRef::GetOwnDataProperty(
   return ObjectRef(broker(), property);
 }
 
+ObjectRef JSArrayRef::GetBoilerplateLength() const {
+  // Safe to read concurrently because:
+  // - boilerplates are immutable after initialization.
+  // - boilerplates are published into the feedback vector.
+  return length_unsafe();
+}
+
+ObjectRef JSArrayRef::length_unsafe() const {
+  if (data_->should_access_heap() || FLAG_turbo_direct_heap_access) {
+    Object o = object()->length(broker()->isolate(), kRelaxedLoad);
+    return ObjectRef{broker(), broker()->CanonicalPersistentHandle(o)};
+  } else {
+    return ObjectRef{broker(), data()->AsJSArray()->length()};
+  }
+}
+
 base::Optional<ObjectRef> JSArrayRef::GetOwnCowElement(
-    uint32_t index, SerializationPolicy policy) const {
+    FixedArrayBaseRef elements_ref, uint32_t index,
+    SerializationPolicy policy) const {
+  if (FLAG_turbo_direct_heap_access) {
+    // `elements` are currently still serialized as members of JSObjectRef.
+    // TODO(jgruber,v8:7790): Remove the elements equality DCHECK below once
+    // JSObject is no longer serialized.
+    static_assert(std::is_base_of<JSObject, JSArray>::value, "");
+    STATIC_ASSERT(IsSerializedHeapObject<JSObject>());
+
+    // The elements_ref is passed in by callers to make explicit that it is
+    // also used outside of this function, and must match the `elements` used
+    // inside this function.
+    DCHECK(elements_ref.equals(elements()));
+
+    // Due to concurrency, the kind read here may not be consistent with
+    // `elements_ref`. But consistency is guaranteed at runtime due to the
+    // `elements` equality check in the caller.
+    ElementsKind elements_kind = GetElementsKind();
+
+    // We only inspect fixed COW arrays, which may only occur for fast
+    // smi/objects elements kinds.
+    if (!IsSmiOrObjectElementsKind(elements_kind)) return {};
+    DCHECK(IsFastElementsKind(elements_kind));
+    if (!elements_ref.map().IsFixedCowArrayMap()) return {};
+
+    // As the name says, the `length` read here is unsafe and may not match
+    // `elements`. We rely on the invariant that any `length` change will
+    // also result in an `elements` change to make this safe. The `elements`
+    // equality check in the caller thus also guards the value of `length`.
+    ObjectRef length_ref = length_unsafe();
+
+    // Likewise we only deal with smi lengths.
+    if (!length_ref.IsSmi()) return {};
+
+    base::Optional<Object> result =
+        ConcurrentLookupIterator::TryGetOwnCowElement(
+            broker()->isolate(), *elements_ref.AsFixedArray().object(),
+            elements_kind, length_ref.AsSmi(), index);
+
+    if (!result.has_value()) return {};
+
+    return ObjectRef{broker(),
+                     broker()->CanonicalPersistentHandle(result.value())};
+  }
+
   if (data_->should_access_heap()) {
     if (!object()->elements().IsCowArray()) return base::nullopt;
     return GetOwnElementFromHeap(broker(), object(), index, false);

src/compiler/js-native-context-specialization.cc

@@ -1959,13 +1959,13 @@ Reduction JSNativeContextSpecialization::ReduceElementLoadFromHeapConstant(
       // We didn't find a constant element, but if the receiver is a cow-array
       // we can exploit the fact that any future write to the element will
       // replace the whole elements storage.
-      element = receiver_ref.AsJSArray().GetOwnCowElement(index);
+      JSArrayRef array_ref = receiver_ref.AsJSArray();
+      FixedArrayBaseRef array_elements = array_ref.elements();
+      element = array_ref.GetOwnCowElement(array_elements, index);
       if (element.has_value()) {
         Node* elements = effect = graph()->NewNode(
             simplified()->LoadField(AccessBuilder::ForJSObjectElements()),
             receiver, effect, control);
-        FixedArrayRef array_elements =
-            receiver_ref.AsJSArray().elements().AsFixedArray();
         Node* check = graph()->NewNode(simplified()->ReferenceEqual(), elements,
                                        jsgraph()->Constant(array_elements));
         effect = graph()->NewNode(

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

@@ -3330,8 +3330,10 @@ void SerializerForBackgroundCompilation::ProcessElementAccess(
             // We didn't find a constant element, but if the receiver is a
             // cow-array we can exploit the fact that any future write to the
             // element will replace the whole elements storage.
-            receiver_ref.AsJSArray().GetOwnCowElement(
-                key_ref.AsSmi(), SerializationPolicy::kSerializeIfNeeded);
+            JSArrayRef array_ref = receiver_ref.AsJSArray();
+            array_ref.SerializeElements();
+            array_ref.GetOwnCowElement(array_ref.elements(), key_ref.AsSmi(),
+                                       SerializationPolicy::kSerializeIfNeeded);
           }
         }
       }

src/objects/js-array-inl.h

@@ -23,6 +23,10 @@ CAST_ACCESSOR(JSArrayIterator)
 
 ACCESSORS(JSArray, length, Object, kLengthOffset)
 
+Object JSArray::length(IsolateRoot isolate, RelaxedLoadTag tag) const {
+  return TaggedField<Object, kLengthOffset>::Relaxed_Load(isolate, *this);
+}
+
 void JSArray::set_length(Smi length) {
   // Don't need a write barrier for a Smi.
   set_length(Object(length.ptr()), SKIP_WRITE_BARRIER);

src/objects/js-array.h

@@ -25,6 +25,7 @@ class JSArray : public JSObject {
  public:
   // [length]: The length property.
   DECL_ACCESSORS(length, Object)
+  inline Object length(IsolateRoot isolate, RelaxedLoadTag tag) const;
 
   // Overload the length setter to skip write barrier when the length
   // is set to a smi. This matches the set function on FixedArray.

src/objects/js-objects-inl.h

@@ -50,6 +50,15 @@ CAST_ACCESSOR(JSIteratorResult)
 CAST_ACCESSOR(JSMessageObject)
 CAST_ACCESSOR(JSReceiver)
 
+DEF_GETTER(JSObject, elements, FixedArrayBase) {
+  return TaggedField<FixedArrayBase, kElementsOffset>::load(isolate, *this);
+}
+
+FixedArrayBase JSObject::elements(IsolateRoot isolate, RelaxedLoadTag) const {
+  return TaggedField<FixedArrayBase, kElementsOffset>::Relaxed_Load(isolate,
+                                                                    *this);
+}
+
 MaybeHandle<Object> JSReceiver::GetProperty(Isolate* isolate,
                                             Handle<JSReceiver> receiver,
                                             Handle<Name> name) {

src/objects/js-objects.h

@@ -311,6 +311,9 @@ class JSObject : public TorqueGeneratedJSObject<JSObject, JSReceiver> {
   static V8_WARN_UNUSED_RESULT MaybeHandle<JSObject> ObjectCreate(
       Isolate* isolate, Handle<Object> prototype);
 
+  DECL_GETTER(elements, FixedArrayBase)
+  DECL_RELAXED_GETTER(elements, FixedArrayBase)
+
   inline void initialize_elements();
   static inline void SetMapAndElements(Handle<JSObject> object, Handle<Map> map,
                                        Handle<FixedArrayBase> elements);

src/objects/lookup.cc

@@ -1269,5 +1269,49 @@ bool LookupIterator::LookupCachedProperty(Handle<AccessorPair> accessor_pair) {
   return true;
 }
 
+// static
+base::Optional<Object> ConcurrentLookupIterator::TryGetOwnCowElement(
+    Isolate* isolate, FixedArray array_elements, ElementsKind elements_kind,
+    int array_length, size_t index) {
+  DisallowGarbageCollection no_gc;
+
+  CHECK_EQ(array_elements.map(), ReadOnlyRoots(isolate).fixed_cow_array_map());
+  DCHECK(IsFastElementsKind(elements_kind) &&
+         IsSmiOrObjectElementsKind(elements_kind));
+  USE(elements_kind);
+  DCHECK_GE(array_length, 0);
+
+  //  ________________________________________
+  // ( Check against both JSArray::length and )
+  // ( FixedArray::length.                    )
+  //  ----------------------------------------
+  //         o   ^__^
+  //          o  (oo)\_______
+  //             (__)\       )\/\
+  //                 ||----w |
+  //                 ||     ||
+  // The former is the source of truth, but due to concurrent reads it may not
+  // match the given `array_elements`.
+  if (index >= static_cast<size_t>(array_length)) return {};
+  if (index >= static_cast<size_t>(array_elements.length())) return {};
+
+  Object result = array_elements.get(isolate, static_cast<int>(index));
+
+  //  ______________________________________
+  // ( Filter out holes irrespective of the )
+  // ( elements kind.                       )
+  //  --------------------------------------
+  //         o   ^__^
+  //          o  (..)\_______
+  //             (__)\       )\/\
+  //                 ||----w |
+  //                 ||     ||
+  // The elements kind may not be consistent with the given elements backing
+  // store.
+  if (result == ReadOnlyRoots(isolate).the_hole_value()) return {};
+
+  return result;
+}
+
 }  // namespace internal
 }  // namespace v8

src/objects/lookup.h

@@ -292,6 +292,32 @@ class V8_EXPORT_PRIVATE LookupIterator final {
   InternalIndex number_ = InternalIndex::NotFound();
 };
 
+// Similar to the LookupIterator, but for concurrent accesses from a background
+// thread.
+//
+// Note: This is a work in progress, intended to bundle code related to
+// concurrent lookups here. In its current state, the class is obviously not an
+// 'iterator'. Still, keeping the name for now, with the intent to clarify
+// names and implementation once we've gotten some experience with more
+// involved logic.
+// TODO(jgruber, v8:7790): Consider using a LookupIterator-style interface.
+// TODO(jgruber, v8:7790): Consider merging back into the LookupIterator once
+// functionality and constraints are better known.
+class ConcurrentLookupIterator final : public AllStatic {
+ public:
+  // 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
+  // length passed to this function should all be read from the same JSArray
+  // instance; but due to concurrency it's possible that they may not be
+  // consistent among themselves (e.g. the elements kind may not match the
+  // given elements backing store). We are thus extra-careful to handle
+  // exceptional situations.
+  V8_EXPORT_PRIVATE static base::Optional<Object> TryGetOwnCowElement(
+      Isolate* isolate, FixedArray array_elements, ElementsKind elements_kind,
+      int array_length, size_t index);
+};
+
 }  // namespace internal
 }  // namespace v8
 

test/cctest/BUILD.gn

@@ -214,6 +214,7 @@ v8_source_set("cctest_sources") {
     "test-compiler.cc",
     "test-concurrent-descriptor-array.cc",
     "test-concurrent-feedback-vector.cc",
+    "test-concurrent-js-array.cc",
     "test-concurrent-prototype.cc",
     "test-concurrent-script-context-table.cc",
     "test-concurrent-string.cc",

test/cctest/test-concurrent-js-array.cc

+// Copyright 2021 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/api/api.h"
+#include "src/base/platform/semaphore.h"
+#include "src/handles/handles-inl.h"
+#include "src/handles/local-handles-inl.h"
+#include "src/handles/persistent-handles.h"
+#include "src/heap/heap.h"
+#include "src/heap/local-heap-inl.h"
+#include "src/heap/local-heap.h"
+#include "src/heap/parked-scope.h"
+#include "src/objects/js-array-inl.h"
+#include "test/cctest/cctest.h"
+#include "test/cctest/heap/heap-utils.h"
+
+namespace v8 {
+namespace internal {
+
+static constexpr int kNumArrays = 1024;
+
+namespace {
+
+class BackgroundThread final : public v8::base::Thread {
+ public:
+  BackgroundThread(Heap* heap, std::vector<Handle<JSArray>> handles,
+                   std::unique_ptr<PersistentHandles> ph,
+                   base::Semaphore* sema_started)
+      : v8::base::Thread(base::Thread::Options("ThreadWithLocalHeap")),
+        heap_(heap),
+        handles_(std::move(handles)),
+        ph_(std::move(ph)),
+        sema_started_(sema_started) {}
+
+  void Run() override {
+    LocalHeap local_heap(heap_, ThreadKind::kBackground, std::move(ph_));
+    UnparkedScope unparked_scope(&local_heap);
+    LocalHandleScope scope(&local_heap);
+    Isolate* isolate = heap_->isolate();
+
+    for (int i = 0; i < kNumArrays; i++) {
+      handles_[i] = local_heap.NewPersistentHandle(handles_[i]);
+    }
+
+    sema_started_->Signal();
+
+    // Iterate in the opposite directions as the main thread to make a race at
+    // some point more likely.
+    static constexpr int kIndex = 1;
+    for (int i = 0; i < kNumArrays; i++) {
+      Handle<JSArray> x = handles_[i];
+      Handle<FixedArrayBase> elements =
+          local_heap.NewPersistentHandle(x->elements(isolate, kRelaxedLoad));
+
+      if (elements->map() != ReadOnlyRoots(isolate).fixed_cow_array_map()) {
+        continue;
+      }
+
+      base::Optional<Object> result =
+          ConcurrentLookupIterator::TryGetOwnCowElement(
+              isolate, FixedArray::cast(*elements), x->GetElementsKind(),
+              Smi::ToInt(x->length(isolate, kRelaxedLoad)), kIndex);
+
+      if (result.has_value()) {
+        // On any success, the elements at index 1 must be the original value
+        // Smi(1).
+        CHECK(result.value().IsSmi());
+        CHECK_EQ(Smi::ToInt(result.value()), 1);
+      }
+    }
+  }
+
+ private:
+  Heap* heap_;
+  std::vector<Handle<JSArray>> handles_;
+  std::unique_ptr<PersistentHandles> ph_;
+  base::Semaphore* sema_started_;
+};
+
+TEST(ArrayWithCowElements) {
+  CcTest::InitializeVM();
+  Isolate* isolate = CcTest::i_isolate();
+
+  std::unique_ptr<PersistentHandles> ph = isolate->NewPersistentHandles();
+  std::vector<Handle<JSArray>> handles;
+  std::vector<Handle<JSArray>> persistent_handles;
+
+  HandleScope handle_scope(isolate);
+
+  // Create kNumArrays arrays with COW backing stores.
+  CompileRun(
+      "function f() { return [0,1,2,3,4]; }\n"
+      "const xs = [];\n"
+      "let i = 0;\n");
+
+  for (int i = 0; i < kNumArrays; i++) {
+    Handle<JSArray> x = Handle<JSArray>::cast(Utils::OpenHandle(
+        *CompileRunChecked(CcTest::isolate(), "xs[i++] = f();")));
+    CHECK_EQ(x->elements().map(), ReadOnlyRoots(isolate).fixed_cow_array_map());
+    handles.push_back(x);
+    persistent_handles.push_back(ph->NewHandle(x));
+  }
+
+  base::Semaphore sema_started(0);
+
+  // Pass persistent handles to background thread.
+  std::unique_ptr<BackgroundThread> thread(new BackgroundThread(
+      isolate->heap(), persistent_handles, std::move(ph), &sema_started));
+  CHECK(thread->Start());
+
+  sema_started.Wait();
+
+  // On the main thread, mutate the arrays, converting to a non-COW backing
+  // store.
+  static const char* const kMutators[] = {
+      "xs[--i].length--;", "xs[--i].length++;",  "xs[--i].length = 0;",
+      "xs[--i][1] = 42;",  "delete xs[--i][1];", "xs[--i].push(42);",
+      "xs[--i].pop();",    "xs[--i][1] = 1.5;",  "xs[--i][1] = {};",
+  };
+  static const int kNumMutators = arraysize(kMutators);
+
+  for (int i = kNumArrays - 1; i >= 0; i--) {
+    CompileRunChecked(CcTest::isolate(), kMutators[i % kNumMutators]);
+    CHECK_NE(handles[i]->elements().map(),
+             ReadOnlyRoots(isolate).fixed_cow_array_map());
+  }
+
+  thread->Join();
+}
+
+}  // anonymous namespace
+}  // namespace internal
+}  // namespace v8