Revert "[compiler] Direct heap reads for JSArrayRef"

This reverts commit 76a2ab06.

Reason for revert: A few issues, e.g.
https://logs.chromium.org/logs/v8/buildbucket/cr-buildbucket.appspot.com/8854931126653780144/+/u/Check__flakes_/ArrayWithCowElements

Original change's description:
> [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}

Bug: v8:7790, chromium:1180012
Change-Id: I50e72380c544b2b78e1e3dc87a8249281b710912
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2704666
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Commit-Queue: Georg Neis <neis@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#72860}

src/compiler/heap-refs.h

@@ -834,24 +834,13 @@ class JSArrayRef : public JSObjectRef {
 
   Handle<JSArray> object() const;
 
-  // The `length` property of boilerplate JSArray objects. Boilerplates are
-  // immutable after initialization. Must not be used for non-boilerplate
-  // JSArrays.
-  ObjectRef GetBoilerplateLength() const;
+  ObjectRef length() 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(
-      FixedArrayBaseRef elements_ref, uint32_t index,
-      SerializationPolicy policy =
+      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.GetBoilerplateLength());
+        boilerplate_array.length());
   }
   for (auto const& inobject_field : inobject_fields) {
     builder.Store(inobject_field.first, inobject_field.second);

src/compiler/js-heap-broker.cc

@@ -506,7 +506,6 @@ void JSObjectData::SerializeObjectCreateMap(JSHeapBroker* broker) {
 }
 
 namespace {
-
 base::Optional<ObjectRef> GetOwnElementFromHeap(JSHeapBroker* broker,
                                                 Handle<Object> receiver,
                                                 uint32_t index,
@@ -1800,10 +1799,7 @@ class JSArrayData : public JSObjectData {
               Handle<JSArray> object);
 
   void Serialize(JSHeapBroker* broker);
-  ObjectData* length() const {
-    CHECK(serialized_);
-    return length_;
-  }
+  ObjectData* length() const { return length_; }
 
   ObjectData* GetOwnElement(
       JSHeapBroker* broker, uint32_t index,
@@ -1825,8 +1821,6 @@ 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;
 
@@ -2225,10 +2219,7 @@ bool JSObjectData::cow_or_empty_elements_tenured() const {
   return cow_or_empty_elements_tenured_;
 }
 
-ObjectData* JSObjectData::elements() const {
-  CHECK(serialized_elements_ || serialized_as_boilerplate_);
-  return elements_;
-}
+ObjectData* JSObjectData::elements() const { return elements_; }
 
 void JSObjectData::SerializeAsBoilerplate(JSHeapBroker* broker) {
   SerializeRecursiveAsBoilerplate(broker, kMaxFastLiteralDepth);
@@ -2429,9 +2420,7 @@ void JSObjectData::SerializeRecursiveAsBoilerplate(JSHeapBroker* broker,
     map()->AsMap()->SerializeOwnDescriptors(broker);
   }
 
-  if (IsJSArray() && !FLAG_turbo_direct_heap_access) {
-    AsJSArray()->Serialize(broker);
-  }
+  if (IsJSArray()) AsJSArray()->Serialize(broker);
 }
 
 void RegExpBoilerplateDescriptionData::Serialize(JSHeapBroker* broker) {
@@ -3485,6 +3474,8 @@ 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)
@@ -4010,68 +4001,8 @@ 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(
-    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())};
-  }
-
+    uint32_t index, SerializationPolicy policy) const {
   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.
-      JSArrayRef array_ref = receiver_ref.AsJSArray();
-      FixedArrayBaseRef array_elements = array_ref.elements();
-      element = array_ref.GetOwnCowElement(array_elements, index);
+      element = receiver_ref.AsJSArray().GetOwnCowElement(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,10 +3330,8 @@ 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.
-            JSArrayRef array_ref = receiver_ref.AsJSArray();
-            array_ref.SerializeElements();
-            array_ref.GetOwnCowElement(array_ref.elements(), key_ref.AsSmi(),
-                                       SerializationPolicy::kSerializeIfNeeded);
+            receiver_ref.AsJSArray().GetOwnCowElement(
+                key_ref.AsSmi(), SerializationPolicy::kSerializeIfNeeded);
           }
         }
       }

src/objects/js-array-inl.h

@@ -23,10 +23,6 @@ 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,7 +25,6 @@ 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,15 +50,6 @@ 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,9 +311,6 @@ 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,49 +1269,5 @@ 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,32 +292,6 @@ 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

@@ -213,7 +213,6 @@ 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