[compiler] Concurrent JSGlobalObjectRef::GetPropertyCell

.. and make JSGlobalObjectRef bg-serialized.

GetPropertyCell was implemented as:

 LookupIterator it(holder, isolate, name, LookupIterator::OWN);
 it.TryLookupCachedProperty();
 if (it.state() == LookupIterator::DATA) it.GetPropertyCell();

Due to concurrency requirements, we essentially have to reimplement
this entire path for use in a concurrent setting:

 - Reads in some cases have to use relaxed or acquire semantics.
 - The IsPendingAllocation predicate must be called on some objects
   before reading into them.
 - Repeated reads of the same field must be avoided due to the
   possibility of concurrent modifications.

This CL introduces two new methods:

ConcurrentLookupIterator::TryGetPropertyCell implements the outer
lookup logic, including the repeated lookup for accessors / cached
property names.

GlobalDictionary::TryFindPropertyCellForConcurrentLookupIterator is a
slightly modified HashTable::FindEntry which follows the above rules.

Bug: v8:7790
Change-Id: Ic9a52da766afdfedce8efcbda92876845a17eed9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2959616Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#75467}

src/compiler/heap-refs.cc

@@ -1321,11 +1321,6 @@ void AllocationSiteData::SerializeBoilerplate(JSHeapBroker* broker) {
 HeapObjectData::HeapObjectData(JSHeapBroker* broker, ObjectData** storage,
                                Handle<HeapObject> object, ObjectDataKind kind)
     : ObjectData(broker, storage, object, kind),
-      // We have to use a raw cast below instead of AsMap() because of
-      // recursion. AsMap() would call IsMap(), which accesses the
-      // instance_type_ member. In the case of constructing the MapData for the
-      // meta map (whose map is itself), this member has not yet been
-      // initialized.
       map_(broker->GetOrCreateData(object->map(kAcquireLoad),
                                    kAssumeMemoryFence)) {
   CHECK_IMPLIES(kind == kSerializedHeapObject,
@@ -2019,15 +2014,28 @@ class CellData : public HeapObjectData {
 class JSGlobalObjectData : public JSObjectData {
  public:
   JSGlobalObjectData(JSHeapBroker* broker, ObjectData** storage,
-                     Handle<JSGlobalObject> object);
-  bool IsDetached() const { return is_detached_; }
+                     Handle<JSGlobalObject> object,
+                     ObjectDataKind kind = kSerializedHeapObject)
+      : JSObjectData(broker, storage, object, kind),
+        properties_(broker->zone()) {
+    if (!broker->is_concurrent_inlining()) {
+      is_detached_ = object->IsDetached();
+    }
+  }
+
+  bool IsDetached() const {
+    DCHECK_EQ(kind(), kSerializedHeapObject);
+    return is_detached_;
+  }
 
   ObjectData* GetPropertyCell(
       JSHeapBroker* broker, ObjectData* name,
       SerializationPolicy policy = SerializationPolicy::kAssumeSerialized);
 
  private:
-  bool const is_detached_;
+  // Only valid if not concurrent inlining.
+
+  bool is_detached_ = false;
 
   // Properties that either
   // (1) are known to exist as property cells on the global object, or
@@ -2036,13 +2044,6 @@ class JSGlobalObjectData : public JSObjectData {
   ZoneVector<std::pair<ObjectData*, ObjectData*>> properties_;
 };
 
-JSGlobalObjectData::JSGlobalObjectData(JSHeapBroker* broker,
-                                       ObjectData** storage,
-                                       Handle<JSGlobalObject> object)
-    : JSObjectData(broker, storage, object),
-      is_detached_(object->IsDetached()),
-      properties_(broker->zone()) {}
-
 class JSGlobalProxyData : public JSObjectData {
  public:
   JSGlobalProxyData(JSHeapBroker* broker, ObjectData** storage,
@@ -2055,17 +2056,12 @@ namespace {
 
 base::Optional<PropertyCellRef> GetPropertyCellFromHeap(JSHeapBroker* broker,
                                                         Handle<Name> name) {
-  LookupIterator it(
-      broker->isolate(),
-      handle(broker->target_native_context().object()->global_object(),
-             broker->isolate()),
-      name, LookupIterator::OWN);
-  it.TryLookupCachedProperty();
-  if (it.state() == LookupIterator::DATA &&
-      it.GetHolder<JSObject>()->IsJSGlobalObject()) {
-    return TryMakeRef(broker, it.GetPropertyCell());
-  }
-  return base::nullopt;
+  base::Optional<PropertyCell> maybe_cell =
+      ConcurrentLookupIterator::TryGetPropertyCell(
+          broker->isolate(), broker->local_isolate_or_isolate(),
+          broker->target_native_context().global_object().object(), name);
+  if (!maybe_cell.has_value()) return {};
+  return TryMakeRef(broker, *maybe_cell);
 }
 
 }  // namespace
@@ -2073,6 +2069,8 @@ base::Optional<PropertyCellRef> GetPropertyCellFromHeap(JSHeapBroker* broker,
 ObjectData* JSGlobalObjectData::GetPropertyCell(JSHeapBroker* broker,
                                                 ObjectData* name,
                                                 SerializationPolicy policy) {
+  DCHECK_EQ(kind(), kSerializedHeapObject);
+
   CHECK_NOT_NULL(name);
   for (auto const& p : properties_) {
     if (p.first == name) return p.second;
@@ -4353,9 +4351,10 @@ bool NativeContextRef::GlobalIsDetached() const {
 
 base::Optional<PropertyCellRef> JSGlobalObjectRef::GetPropertyCell(
     NameRef const& name, SerializationPolicy policy) const {
-  if (data_->should_access_heap()) {
+  if (data_->should_access_heap() || broker()->is_concurrent_inlining()) {
     return GetPropertyCellFromHeap(broker(), name.object());
   }
+
   ObjectData* property_cell_data = data()->AsJSGlobalObject()->GetPropertyCell(
       broker(), name.data(), policy);
   return TryMakeRef<PropertyCell>(broker(), property_cell_data);

src/compiler/heap-refs.h

@@ -84,7 +84,7 @@ enum class RefSerializationKind {
   V(JSBoundFunction, RefSerializationKind::kBackgroundSerialized)         \
   V(JSDataView, RefSerializationKind::kBackgroundSerialized)              \
   V(JSFunction, RefSerializationKind::kSerialized)                        \
-  V(JSGlobalObject, RefSerializationKind::kSerialized)                    \
+  V(JSGlobalObject, RefSerializationKind::kBackgroundSerialized)          \
   V(JSGlobalProxy, RefSerializationKind::kBackgroundSerialized)           \
   V(JSTypedArray, RefSerializationKind::kBackgroundSerialized)            \
   /* Subtypes of Context */                                               \
@@ -996,13 +996,7 @@ class JSGlobalObjectRef : public JSObjectRef {
 
   bool IsDetachedFrom(JSGlobalProxyRef const& proxy) const;
 
-  // If {serialize} is false:
-  //   If the property is known to exist as a property cell (on the global
-  //   object), return that property cell. Otherwise (not known to exist as a
-  //   property cell or known not to exist as a property cell) return nothing.
-  // If {serialize} is true:
-  //   Like above but potentially access the heap and serialize the necessary
-  //   information.
+  // Can be called even when there is no property cell for the given name.
   base::Optional<PropertyCellRef> GetPropertyCell(
       NameRef const& name, SerializationPolicy policy =
                                SerializationPolicy::kAssumeSerialized) const;

src/compiler/pipeline.cc

@@ -1212,6 +1212,9 @@ PipelineCompilationJob::Status PipelineCompilationJob::PrepareJobImpl(
 
   if (compilation_info()->is_osr()) data_.InitializeOsrHelper();
 
+  // Serialize() and CreateGraph() may already use IsPendingAllocation.
+  isolate->heap()->PublishPendingAllocations();
+
   pipeline_.Serialize();
 
   if (!data_.broker()->is_concurrent_inlining()) {
@@ -1222,6 +1225,7 @@ PipelineCompilationJob::Status PipelineCompilationJob::PrepareJobImpl(
   }
 
   if (compilation_info()->concurrent_inlining()) {
+    // Serialization may have allocated.
     isolate->heap()->PublishPendingAllocations();
   }
 

src/heap/heap-inl.h

@@ -660,6 +660,10 @@ bool Heap::IsPendingAllocation(HeapObject object) {
   UNREACHABLE();
 }
 
+bool Heap::IsPendingAllocation(Object object) {
+  return object.IsHeapObject() && IsPendingAllocation(HeapObject::cast(object));
+}
+
 void Heap::ExternalStringTable::AddString(String string) {
   DCHECK(string.IsExternalString());
   DCHECK(!Contains(string));

src/heap/heap.h

@@ -1548,6 +1548,7 @@ class Heap {
   // as uninitialized to background threads.
   // This predicate may be invoked from a background thread.
   inline bool IsPendingAllocation(HeapObject object);
+  inline bool IsPendingAllocation(Object object);
 
   // Notifies that all previously allocated objects are properly initialized
   // and ensures that IsPendingAllocation returns false for them. This function

src/objects/dictionary.h

@@ -243,6 +243,9 @@ class V8_EXPORT_PRIVATE GlobalDictionary
   inline Name NameAt(PtrComprCageBase cage_base, InternalIndex entry);
   inline void ValueAtPut(InternalIndex entry, Object value);
 
+  base::Optional<PropertyCell> TryFindPropertyCellForConcurrentLookupIterator(
+      Isolate* isolate, Handle<Name> name, RelaxedLoadTag tag);
+
   OBJECT_CONSTRUCTORS(
       GlobalDictionary,
       BaseNameDictionary<GlobalDictionary, GlobalDictionaryShape>);

src/objects/hash-table-inl.h

@@ -146,7 +146,6 @@ InternalIndex HashTable<Derived, Shape>::FindEntry(PtrComprCageBase cage_base,
   uint32_t count = 1;
   Object undefined = roots.undefined_value();
   Object the_hole = roots.the_hole_value();
-  USE(the_hole);
   // EnsureCapacity will guarantee the hash table is never full.
   for (InternalIndex entry = FirstProbe(hash, capacity);;
        entry = NextProbe(entry, count++, capacity)) {
@@ -196,6 +195,20 @@ Object HashTable<Derived, Shape>::KeyAt(PtrComprCageBase cage_base,
   return get(cage_base, EntryToIndex(entry) + kEntryKeyIndex);
 }
 
+template <typename Derived, typename Shape>
+Object HashTable<Derived, Shape>::KeyAt(InternalIndex entry,
+                                        RelaxedLoadTag tag) {
+  PtrComprCageBase cage_base = GetPtrComprCageBase(*this);
+  return KeyAt(cage_base, entry, tag);
+}
+
+template <typename Derived, typename Shape>
+Object HashTable<Derived, Shape>::KeyAt(PtrComprCageBase cage_base,
+                                        InternalIndex entry,
+                                        RelaxedLoadTag tag) {
+  return get(cage_base, EntryToIndex(entry) + kEntryKeyIndex, tag);
+}
+
 template <typename Derived, typename Shape>
 void HashTable<Derived, Shape>::set_key(int index, Object value) {
   DCHECK(!IsEphemeronHashTable());

src/objects/hash-table.h

@@ -157,6 +157,9 @@ class EXPORT_TEMPLATE_DECLARE(V8_EXPORT_PRIVATE) HashTable
   // Returns the key at entry.
   inline Object KeyAt(InternalIndex entry);
   inline Object KeyAt(PtrComprCageBase cage_base, InternalIndex entry);
+  inline Object KeyAt(InternalIndex entry, RelaxedLoadTag tag);
+  inline Object KeyAt(PtrComprCageBase cage_base, InternalIndex entry,
+                      RelaxedLoadTag tag);
 
   static const int kElementsStartIndex = kPrefixStartIndex + Shape::kPrefixSize;
   static const int kEntrySize = Shape::kEntrySize;

src/objects/lookup.cc

@@ -1511,5 +1511,43 @@ ConcurrentLookupIterator::TryGetOwnConstantElement(
   UNREACHABLE();
 }
 
+// static
+base::Optional<PropertyCell> ConcurrentLookupIterator::TryGetPropertyCell(
+    Isolate* isolate, LocalIsolate* local_isolate,
+    Handle<JSGlobalObject> holder, Handle<Name> name) {
+  DisallowGarbageCollection no_gc;
+
+  Map holder_map = holder->map();
+  CHECK(!holder_map.is_access_check_needed());
+  CHECK(!holder_map.has_named_interceptor());
+
+  GlobalDictionary dict = holder->global_dictionary(kAcquireLoad);
+  base::Optional<PropertyCell> cell =
+      dict.TryFindPropertyCellForConcurrentLookupIterator(isolate, name,
+                                                          kRelaxedLoad);
+  if (!cell.has_value()) return {};
+
+  if (cell->property_details().kind() == kAccessor) {
+    Object maybe_accessor_pair = cell->value(kAcquireLoad);
+    if (!maybe_accessor_pair.IsAccessorPair()) return {};
+
+    base::Optional<Name> maybe_cached_property_name =
+        FunctionTemplateInfo::TryGetCachedPropertyName(
+            isolate, AccessorPair::cast(maybe_accessor_pair)
+                         .getter(isolate, kRelaxedLoad));
+    if (!maybe_cached_property_name.has_value()) return {};
+
+    cell = dict.TryFindPropertyCellForConcurrentLookupIterator(
+        isolate, handle(*maybe_cached_property_name, local_isolate),
+        kRelaxedLoad);
+    if (!cell.has_value()) return {};
+    if (cell->property_details().kind() != kData) return {};
+  }
+
+  DCHECK(cell.has_value());
+  DCHECK_EQ(cell->property_details().kind(), kData);
+  return cell;
+}
+
 }  // namespace internal
 }  // namespace v8

src/objects/lookup.h

@@ -344,6 +344,15 @@ class ConcurrentLookupIterator final : public AllStatic {
       Object* result_out, Isolate* isolate, LocalIsolate* local_isolate,
       JSObject holder, FixedArrayBase elements, ElementsKind elements_kind,
       size_t index);
+
+  // This method reimplements the following sequence in a concurrent setting:
+  //
+  // LookupIterator it(holder, isolate, name, LookupIterator::OWN);
+  // it.TryLookupCachedProperty();
+  // if (it.state() == LookupIterator::DATA) it.GetPropertyCell();
+  V8_EXPORT_PRIVATE static base::Optional<PropertyCell> TryGetPropertyCell(
+      Isolate* isolate, LocalIsolate* local_isolate,
+      Handle<JSGlobalObject> holder, Handle<Name> name);
 };
 
 }  // namespace internal

src/objects/objects.cc

@@ -5864,6 +5864,37 @@ InternalIndex HashTable<Derived, Shape>::FindInsertionEntry(Isolate* isolate,
   return FindInsertionEntry(isolate, ReadOnlyRoots(isolate), hash);
 }
 
+base::Optional<PropertyCell>
+GlobalDictionary::TryFindPropertyCellForConcurrentLookupIterator(
+    Isolate* isolate, Handle<Name> name, RelaxedLoadTag tag) {
+  // This reimplements HashTable::FindEntry for use in a concurrent setting.
+  // 1) Atomic loads.
+  // 2) IsPendingAllocation checks.
+  // 3) Return the PropertyCell value instead of the InternalIndex to avoid a
+  //   repeated load (unsafe with concurrent modifications).
+
+  DisallowGarbageCollection no_gc;
+  PtrComprCageBase cage_base{isolate};
+  ReadOnlyRoots roots(isolate);
+  const int32_t hash = ShapeT::Hash(roots, name);
+  const uint32_t capacity = Capacity();
+  uint32_t count = 1;
+  Object undefined = roots.undefined_value();
+  Object the_hole = roots.the_hole_value();
+  // EnsureCapacity will guarantee the hash table is never full.
+  for (InternalIndex entry = FirstProbe(hash, capacity);;
+       entry = NextProbe(entry, count++, capacity)) {
+    Object element = KeyAt(cage_base, entry, kRelaxedLoad);
+    if (isolate->heap()->IsPendingAllocation(element)) return {};
+    if (element == undefined) return {};
+    if (ShapeT::kMatchNeedsHoleCheck && element == the_hole) continue;
+    if (!ShapeT::IsMatch(name, element)) continue;
+    CHECK(element.IsPropertyCell(cage_base));
+    return PropertyCell::cast(element);
+  }
+  return {};
+}
+
 Handle<StringSet> StringSet::New(Isolate* isolate) {
   return HashTable::New(isolate, 0);
 }

src/objects/struct-inl.h

@@ -40,6 +40,14 @@ void AccessorPair::set(AccessorComponent component, Object value) {
   }
 }
 
+DEF_GETTER(AccessorPair, getter, Object) {
+  return TorqueGeneratedClass::getter(cage_base);
+}
+
+DEF_RELAXED_GETTER(AccessorPair, getter, Object) {
+  return TaggedField<Object, kGetterOffset>::Relaxed_Load(cage_base, *this);
+}
+
 void AccessorPair::SetComponents(Object getter, Object setter) {
   if (!getter.IsNull()) set_getter(getter);
   if (!setter.IsNull()) set_setter(setter);

src/objects/struct.h

@@ -48,6 +48,9 @@ class AccessorPair : public TorqueGeneratedAccessorPair<AccessorPair, Struct> {
   inline Object get(AccessorComponent component);
   inline void set(AccessorComponent component, Object value);
 
+  DECL_GETTER(getter, Object)
+  DECL_RELAXED_GETTER(getter, Object)
+
   // Note: Returns undefined if the component is not set.
   static Handle<Object> GetComponent(Isolate* isolate,
                                      Handle<NativeContext> native_context,