[compiler] Remove use of serialized JSObjectRef::elements

.. and replace them by elements read directly from the heap object.

With this change, consistency between `map` and `elements` is
no longer guaranteed. Users were updated, when necessary, to deal
with this, e.g. by being more careful not to read out of bounds,
by inserting new `actual_elements == elements_constant` runtime
checks, or through a new compilation dependency that verifies
unchanged elements at finalization time.

Drive-by: inline GetElementsKind into callsites.

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

src/compiler/compilation-dependencies.cc

@@ -8,6 +8,7 @@
 #include "src/execution/protectors.h"
 #include "src/handles/handles-inl.h"
 #include "src/objects/allocation-site-inl.h"
+#include "src/objects/js-array-inl.h"
 #include "src/objects/js-function-inl.h"
 #include "src/objects/objects-inl.h"
 #include "src/zone/zone-handle-set.h"
@@ -437,15 +438,16 @@ class ElementsKindDependency final : public CompilationDependency {
       : site_(site), kind_(kind) {
     DCHECK(AllocationSite::ShouldTrack(kind_));
     DCHECK_EQ(kind_, site_.PointsToLiteral()
-                         ? site_.boilerplate().value().GetElementsKind()
+                         ? site_.boilerplate().value().map().elements_kind()
                          : site_.GetElementsKind());
   }
 
   bool IsValid() const override {
     Handle<AllocationSite> site = site_.object();
-    ElementsKind kind = site->PointsToLiteral()
-                            ? site->boilerplate(kAcquireLoad).GetElementsKind()
-                            : site->GetElementsKind();
+    ElementsKind kind =
+        site->PointsToLiteral()
+            ? site->boilerplate(kAcquireLoad).map().elements_kind()
+            : site->GetElementsKind();
     return kind_ == kind;
   }
 
@@ -461,6 +463,35 @@ class ElementsKindDependency final : public CompilationDependency {
   ElementsKind kind_;
 };
 
+// Only valid if the holder can use direct reads, since validation uses
+// GetOwnConstantElementFromHeap.
+class OwnConstantElementDependency final : public CompilationDependency {
+ public:
+  OwnConstantElementDependency(const JSObjectRef& holder, uint32_t index,
+                               const ObjectRef& element)
+      : holder_(holder), index_(index), element_(element) {}
+
+  bool IsValid() const override {
+    DisallowGarbageCollection no_gc;
+    JSObject holder = *holder_.object();
+    base::Optional<Object> maybe_element =
+        holder_.GetOwnConstantElementFromHeap(holder.elements(),
+                                              holder.GetElementsKind(), index_);
+    if (!maybe_element.has_value()) return false;
+
+    return maybe_element.value() == *element_.object();
+  }
+
+  void Install(const MaybeObjectHandle& code) const override {
+    // This dependency has no effect after code finalization.
+  }
+
+ private:
+  const JSObjectRef holder_;
+  const uint32_t index_;
+  const ObjectRef element_;
+};
+
 class InitialMapInstanceSizePredictionDependency final
     : public CompilationDependency {
  public:
@@ -620,13 +651,22 @@ void CompilationDependencies::DependOnElementsKind(
   DCHECK(!site.IsNeverSerializedHeapObject());
   // Do nothing if the object doesn't have any useful element transitions left.
   ElementsKind kind = site.PointsToLiteral()
-                          ? site.boilerplate().value().GetElementsKind()
+                          ? site.boilerplate().value().map().elements_kind()
                           : site.GetElementsKind();
   if (AllocationSite::ShouldTrack(kind)) {
     RecordDependency(zone_->New<ElementsKindDependency>(site, kind));
   }
 }
 
+void CompilationDependencies::DependOnOwnConstantElement(
+    const JSObjectRef& holder, uint32_t index, const ObjectRef& element) {
+  // Only valid if the holder can use direct reads, since validation uses
+  // GetOwnConstantElementFromHeap.
+  DCHECK(holder.should_access_heap() || broker_->is_concurrent_inlining());
+  RecordDependency(
+      zone_->New<OwnConstantElementDependency>(holder, index, element));
+}
+
 bool CompilationDependencies::Commit(Handle<Code> code) {
   // Dependencies are context-dependent. In the future it may be possible to
   // restore them in the consumer native context, but for now they are

src/compiler/compilation-dependencies.h

@@ -97,6 +97,9 @@ class V8_EXPORT_PRIVATE CompilationDependencies : public ZoneObject {
   // Record the assumption that {site}'s {ElementsKind} doesn't change.
   void DependOnElementsKind(const AllocationSiteRef& site);
 
+  void DependOnOwnConstantElement(const JSObjectRef& holder, uint32_t index,
+                                  const ObjectRef& element);
+
   // For each given map, depend on the stability of (the maps of) all prototypes
   // up to (and including) the {last_prototype}.
   template <class MapContainer>

src/compiler/heap-refs.h

@@ -188,6 +188,8 @@ class V8_EXPORT_PRIVATE ObjectRef {
   base::Optional<bool> TryGetBooleanValue() const;
   Maybe<double> OddballToNumber() const;
 
+  bool should_access_heap() const;
+
   Isolate* isolate() const;
 
   struct Hash {
@@ -318,8 +320,16 @@ class JSObjectRef : public JSReceiverRef {
   // Return the element at key {index} if {index} is known to be an own data
   // property of the object that is non-writable and non-configurable.
   base::Optional<ObjectRef> GetOwnConstantElement(
-      uint32_t index, SerializationPolicy policy =
-                          SerializationPolicy::kAssumeSerialized) const;
+      const FixedArrayBaseRef& elements_ref, uint32_t index,
+      CompilationDependencies* dependencies = nullptr,
+      SerializationPolicy policy =
+          SerializationPolicy::kAssumeSerialized) const;
+  // The direct-read implementation of the above, extracted into a helper since
+  // it's also called from compilation-dependency validation. This helper is
+  // guaranteed to not create new Ref instances.
+  base::Optional<Object> GetOwnConstantElementFromHeap(
+      FixedArrayBase elements, ElementsKind elements_kind,
+      uint32_t index) const;
 
   // Return the value of the property identified by the field {index}
   // if {index} is known to be an own data property of the object.
@@ -334,10 +344,12 @@ class JSObjectRef : public JSReceiverRef {
       InternalIndex index, SerializationPolicy policy =
                                SerializationPolicy::kAssumeSerialized) const;
 
-  base::Optional<FixedArrayBaseRef> elements() const;
+  // When concurrent inlining is enabled, reads the elements through a direct
+  // relaxed read. This is to ease the transition to unserialized (or
+  // background-serialized) elements.
+  base::Optional<FixedArrayBaseRef> elements(RelaxedLoadTag) const;
   void SerializeElements();
-  bool IsElementsTenured();
-  ElementsKind GetElementsKind() const;
+  bool IsElementsTenured(const FixedArrayBaseRef& elements);
 
   void SerializeObjectCreateMap();
   base::Optional<MapRef> GetObjectCreateMap() const;

src/compiler/js-call-reducer.cc

@@ -4211,7 +4211,7 @@ Reduction JSCallReducer::ReduceCallOrConstructWithArrayLikeOrSpread(
 
     // Speculate on that array's length being equal to the dynamic length of
     // arguments_list; generate a deopt check.
-    ElementsKind elements_kind = boilerplate_array->GetElementsKind();
+    ElementsKind elements_kind = array_map.elements_kind();
     effect = CheckArrayLength(arguments_list, elements_kind, array_length,
                               feedback_source, effect, control);
 

src/compiler/js-create-lowering.cc

@@ -1743,9 +1743,9 @@ base::Optional<Node*> JSCreateLowering::TryAllocateFastLiteral(
   builder.Store(AccessBuilder::ForJSObjectElements(), elements);
   if (boilerplate.IsJSArray()) {
     JSArrayRef boilerplate_array = boilerplate.AsJSArray();
-    builder.Store(
-        AccessBuilder::ForJSArrayLength(boilerplate_array.GetElementsKind()),
-        boilerplate_array.GetBoilerplateLength());
+    builder.Store(AccessBuilder::ForJSArrayLength(
+                      boilerplate_array.map().elements_kind()),
+                  boilerplate_array.GetBoilerplateLength());
   }
   for (auto const& inobject_field : inobject_fields) {
     builder.Store(inobject_field.first, inobject_field.second);
@@ -1756,15 +1756,18 @@ base::Optional<Node*> JSCreateLowering::TryAllocateFastLiteral(
 base::Optional<Node*> JSCreateLowering::TryAllocateFastLiteralElements(
     Node* effect, Node* control, JSObjectRef boilerplate,
     AllocationType allocation) {
-  FixedArrayBaseRef boilerplate_elements = boilerplate.elements().value();
+  base::Optional<FixedArrayBaseRef> maybe_boilerplate_elements =
+      boilerplate.elements(kRelaxedLoad);
+  if (!maybe_boilerplate_elements.has_value()) return {};
+  FixedArrayBaseRef boilerplate_elements = maybe_boilerplate_elements.value();
 
   // Empty or copy-on-write elements just store a constant.
   int const elements_length = boilerplate_elements.length();
   MapRef elements_map = boilerplate_elements.map();
   if (boilerplate_elements.length() == 0 || elements_map.IsFixedCowArrayMap()) {
-    if (allocation == AllocationType::kOld) {
-      if (!boilerplate.IsElementsTenured()) return {};
-      boilerplate_elements = boilerplate.elements().value();
+    if (allocation == AllocationType::kOld &&
+        !boilerplate.IsElementsTenured(boilerplate_elements)) {
+      return {};
     }
     return jsgraph()->HeapConstant(boilerplate_elements.object());
   }

src/compiler/js-heap-broker.h

@@ -568,6 +568,14 @@ class V8_NODISCARD UnparkedScopeIfNeeded {
   base::Optional<UnparkedScope> unparked_scope;
 };
 
+template <class T,
+          typename = std::enable_if_t<std::is_convertible<T*, Object*>::value>>
+base::Optional<typename ref_traits<T>::ref_type> TryMakeRef(
+    JSHeapBroker* broker, ObjectData* data) {
+  if (data == nullptr) return {};
+  return {typename ref_traits<T>::ref_type(broker, data)};
+}
+
 // Usage:
 //
 //  base::Optional<FooRef> ref = TryMakeRef(broker, o);
@@ -583,9 +591,8 @@ base::Optional<typename ref_traits<T>::ref_type> TryMakeRef(
   ObjectData* data = broker->TryGetOrCreateData(object, flags);
   if (data == nullptr) {
     TRACE_BROKER_MISSING(broker, "ObjectData for " << Brief(object));
-    return {};
   }
-  return {typename ref_traits<T>::ref_type(broker, data)};
+  return TryMakeRef<T>(broker, data);
 }
 
 template <class T,
@@ -596,9 +603,8 @@ base::Optional<typename ref_traits<T>::ref_type> TryMakeRef(
   if (data == nullptr) {
     DCHECK_EQ(flags & kCrashOnError, 0);
     TRACE_BROKER_MISSING(broker, "ObjectData for " << Brief(*object));
-    return {};
   }
-  return {typename ref_traits<T>::ref_type(broker, data)};
+  return TryMakeRef<T>(broker, data);
 }
 
 template <class T,

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

@@ -1952,22 +1952,24 @@ Reduction JSNativeContextSpecialization::ReduceElementLoadFromHeapConstant(
     base::Optional<ObjectRef> element;
 
     if (receiver_ref.IsJSObject()) {
-      element = receiver_ref.AsJSObject().GetOwnConstantElement(index);
-      if (!element.has_value() && receiver_ref.IsJSArray()) {
-        // 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();
-        base::Optional<FixedArrayBaseRef> array_elements = array_ref.elements();
-        if (array_elements.has_value()) {
-          element = array_ref.GetOwnCowElement(*array_elements, index);
+      JSObjectRef jsobject_ref = receiver_ref.AsJSObject();
+      base::Optional<FixedArrayBaseRef> elements =
+          jsobject_ref.elements(kRelaxedLoad);
+      if (elements.has_value()) {
+        element = jsobject_ref.GetOwnConstantElement(*elements, index,
+                                                     dependencies());
+        if (!element.has_value() && receiver_ref.IsJSArray()) {
+          // 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(*elements, index);
           if (element.has_value()) {
-            Node* elements = effect = graph()->NewNode(
+            Node* actual_elements = effect = graph()->NewNode(
                 simplified()->LoadField(AccessBuilder::ForJSObjectElements()),
                 receiver, effect, control);
-            Node* check =
-                graph()->NewNode(simplified()->ReferenceEqual(), elements,
-                                 jsgraph()->Constant(*array_elements));
+            Node* check = graph()->NewNode(simplified()->ReferenceEqual(),
+                                           actual_elements,
+                                           jsgraph()->Constant(*elements));
             effect = graph()->NewNode(
                 simplified()->CheckIf(
                     DeoptimizeReason::kCowArrayElementsChanged),

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

@@ -3333,16 +3333,18 @@ void SerializerForBackgroundCompilation::ProcessElementAccess(
         if (key_ref.IsSmi() && key_ref.AsSmi() >= 0) {
           base::Optional<ObjectRef> element;
           if (receiver_ref.IsJSObject()) {
-            receiver_ref.AsJSObject().SerializeElements();
+            JSObjectRef jsobject_ref = receiver_ref.AsJSObject();
+            jsobject_ref.SerializeElements();
             element = receiver_ref.AsJSObject().GetOwnConstantElement(
-                key_ref.AsSmi(), SerializationPolicy::kSerializeIfNeeded);
+                jsobject_ref.elements(kRelaxedLoad).value(), key_ref.AsSmi(),
+                nullptr, SerializationPolicy::kSerializeIfNeeded);
             if (!element.has_value() && receiver_ref.IsJSArray()) {
               // 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.GetOwnCowElement(
-                  array_ref.elements().value(), key_ref.AsSmi(),
+                  array_ref.elements(kRelaxedLoad).value(), key_ref.AsSmi(),
                   SerializationPolicy::kSerializeIfNeeded);
             }
           } else if (receiver_ref.IsString()) {

src/objects/js-objects-inl.h

@@ -55,6 +55,11 @@ DEF_GETTER(JSObject, elements, FixedArrayBase) {
   return TaggedField<FixedArrayBase, kElementsOffset>::load(cage_base, *this);
 }
 
+FixedArrayBase JSObject::elements(RelaxedLoadTag tag) const {
+  PtrComprCageBase cage_base = GetPtrComprCageBase(*this);
+  return elements(cage_base, tag);
+}
+
 FixedArrayBase JSObject::elements(PtrComprCageBase cage_base,
                                   RelaxedLoadTag) const {
   return TaggedField<FixedArrayBase, kElementsOffset>::Relaxed_Load(cage_base,

src/objects/lookup.cc

@@ -1372,6 +1372,7 @@ ConcurrentLookupIterator::TryGetOwnConstantElement(
   // - single_character_string_cache()->get().
 
   if (IsFrozenElementsKind(elements_kind)) {
+    if (!elements.IsFixedArray()) return kGaveUp;
     FixedArray elements_fixed_array = FixedArray::cast(elements);
     if (index >= static_cast<uint32_t>(elements_fixed_array.length())) {
       return kGaveUp;
@@ -1384,7 +1385,7 @@ ConcurrentLookupIterator::TryGetOwnConstantElement(
     *result_out = result;
     return kPresent;
   } else if (IsDictionaryElementsKind(elements_kind)) {
-    DCHECK(elements.IsNumberDictionary());
+    if (!elements.IsNumberDictionary()) return kGaveUp;
     // 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

src/objects/lookup.h

@@ -322,8 +322,9 @@ class ConcurrentLookupIterator final : public AllStatic {
       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.
+  // As above, the contract is that the elements and elements kind should be
+  // read from the same holder, but this function is implemented defensively to
+  // tolerate concurrency issues.
   V8_EXPORT_PRIVATE static Result TryGetOwnConstantElement(
       Object* result_out, Isolate* isolate, LocalIsolate* local_isolate,
       JSObject holder, FixedArrayBase elements, ElementsKind elements_kind,