[compiler] Clear reconstructible ObjectData after serialization

This is another step towards improving TSAN and test coverage of
concurrent paths.

By purging reconstructible (background-serialized or never-serialized)
ObjectData instances from the cache after serialization, we increase
ObjectData-construction activity on the background thread.

Note that this means ObjectData is no longer canonicalized - two
instances can point at the same underlying object. Losing this
property is unfortunate, but we can revert back to it once work on the
concurrency project is further advanced.

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

src/compiler/heap-refs.cc

@@ -77,22 +77,6 @@ bool IsReadOnlyHeapObject(Object object) {
           ReadOnlyHeap::Contains(HeapObject::cast(object)));
 }
 
-template <class T>
-constexpr bool IsSerializedHeapObject() {
-  return false;
-}
-
-#define DEFINE_MARKER(Name, Kind)                                        \
-  template <>                                                            \
-  constexpr bool IsSerializedHeapObject<Name>() {                        \
-    return Kind == RefSerializationKind::kSerialized;                    \
-  }                                                                      \
-  /* The static assert is needed to avoid 'unused function' warnings. */ \
-  STATIC_ASSERT(IsSerializedHeapObject<Name>() ==                        \
-                (Kind == RefSerializationKind::kSerialized));
-HEAP_BROKER_OBJECT_LIST(DEFINE_MARKER)
-#undef DEFINE_MARKER
-
 }  // namespace
 
 class ObjectData : public ZoneObject {
@@ -165,6 +149,31 @@ class ObjectData : public ZoneObject {
 #endif                          // DEBUG
 };
 
+namespace {
+
+template <class T>
+constexpr RefSerializationKind RefSerializationKindOf() {
+  CONSTEXPR_DCHECK(false);  // The default impl should never be called.
+  return RefSerializationKind::kSerialized;
+}
+
+#define DEFINE_REF_SERIALIZATION_KIND(Name, Kind)                        \
+  template <>                                                            \
+  constexpr RefSerializationKind RefSerializationKindOf<Name>() {        \
+    return Kind;                                                         \
+  }                                                                      \
+  /* The static assert is needed to avoid 'unused function' warnings. */ \
+  STATIC_ASSERT(RefSerializationKindOf<Name>() == Kind);
+HEAP_BROKER_OBJECT_LIST(DEFINE_REF_SERIALIZATION_KIND)
+#undef DEFINE_REF_SERIALIZATION_KIND
+
+template <class T>
+constexpr bool IsSerializedRef() {
+  return RefSerializationKindOf<T>() == RefSerializationKind::kSerialized;
+}
+
+}  // namespace
+
 class HeapObjectData : public ObjectData {
  public:
   HeapObjectData(JSHeapBroker* broker, ObjectData** storage,
@@ -565,7 +574,7 @@ class JSTypedArrayData : public JSObjectData {
 
   // TODO(v8:7790): Once JSObject is no longer serialized, also make
   // JSTypedArrayRef never-serialized.
-  STATIC_ASSERT(IsSerializedHeapObject<JSObject>());
+  STATIC_ASSERT(IsSerializedRef<JSObject>());
 
   void Serialize(JSHeapBroker* broker);
   bool serialized() const { return serialized_; }
@@ -1185,6 +1194,14 @@ class MapData : public HeapObjectData {
 
   void SerializeForElementStore(JSHeapBroker* broker);
 
+  bool has_extra_serialized_data() const {
+    return serialized_elements_kind_generalizations_ ||
+           serialized_own_descriptors_ || serialized_constructor_ ||
+           serialized_backpointer_ || serialized_prototype_ ||
+           serialized_root_map_ || serialized_for_element_load_ ||
+           serialized_for_element_store_;
+  }
+
  private:
   // The following fields should be const in principle, but construction
   // requires locking the MapUpdater lock. For this reason, it's easier to
@@ -2519,7 +2536,10 @@ bool ObjectRef::equals(const ObjectRef& other) const {
     data_->used_status = ObjectData::Usage::kOnlyIdentityUsed;
   }
 #endif  // DEBUG
-  return data_ == other.data_;
+  // TODO(jgruber): Consider going back to reference-equality on data_ once
+  // ObjectData objects are guaranteed to be canonicalized (see also:
+  // ClearReconstructibleData).
+  return data_->object().is_identical_to(other.data_->object());
 }
 
 bool ObjectRef::ShouldHaveBeenSerialized() const {
@@ -2820,6 +2840,27 @@ struct CreateDataFunctor<RefSerializationKind::kNeverSerialized, DataT,
 
 }  // namespace
 
+void JSHeapBroker::ClearReconstructibleData() {
+  RefsMap::Entry* p = refs_->Start();
+  while (p != nullptr) {
+    Address key = p->key;
+    ObjectData* value = p->value;
+    p = refs_->Next(p);
+    const ObjectDataKind kind = value->kind();
+    if (kind == ObjectDataKind::kNeverSerializedHeapObject ||
+        kind == ObjectDataKind::kBackgroundSerializedHeapObject ||
+        kind == ObjectDataKind::kUnserializedReadOnlyHeapObject) {
+      if (value->IsMap() &&
+          value->kind() == ObjectDataKind::kBackgroundSerializedHeapObject &&
+          value->AsMap()->has_extra_serialized_data()) {
+        continue;
+      }
+      // Can be reconstructed from the background thread.
+      CHECK_NOT_NULL(refs_->Remove(key));
+    }
+  }
+}
+
 ObjectData* JSHeapBroker::TryGetOrCreateData(
     Handle<Object> object, bool crash_on_error,
     ObjectRef::BackgroundSerialization background_serialization) {
@@ -3894,7 +3935,7 @@ base::Optional<ObjectRef> JSObjectRef::GetOwnConstantElement(
     // 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>());
+    STATIC_ASSERT(IsSerializedRef<JSObject>());
 
     base::Optional<FixedArrayBaseRef> maybe_elements_ref = elements();
     if (!maybe_elements_ref.has_value()) {
@@ -3982,7 +4023,7 @@ base::Optional<ObjectRef> JSArrayRef::GetOwnCowElement(
     // 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>());
+    STATIC_ASSERT(IsSerializedRef<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
@@ -4247,7 +4288,7 @@ void RegExpBoilerplateDescriptionRef::Serialize() {
     // Even if the regexp boilerplate object itself is no longer serialized,
     // the `data` still is and thus we need to make sure to visit it.
     // TODO(jgruber,v8:7790): Remove once it is no longer a serialized type.
-    STATIC_ASSERT(IsSerializedHeapObject<FixedArray>());
+    STATIC_ASSERT(IsSerializedRef<FixedArray>());
     FixedArrayRef data_ref{
         broker(), broker()->CanonicalPersistentHandle(object()->data())};
   } else {
@@ -4545,7 +4586,7 @@ void JSTypedArrayRef::Serialize() {
     // TODO(jgruber,v8:7790): Remove once JSObject is no longer serialized.
     static_assert(
         std::is_base_of<JSObject, decltype(object()->buffer())>::value, "");
-    STATIC_ASSERT(IsSerializedHeapObject<JSObject>());
+    STATIC_ASSERT(IsSerializedRef<JSObject>());
     JSObjectRef data_ref{
         broker(), broker()->CanonicalPersistentHandle(object()->buffer())};
   } else {

src/compiler/js-heap-broker.h

@@ -261,6 +261,10 @@ class V8_EXPORT_PRIVATE JSHeapBroker {
     property_access_infos_.clear();
   }
 
+  // As above, clear cached ObjectData that can be reconstructed, i.e. is
+  // either never-serialized or background-serialized.
+  void ClearReconstructibleData();
+
   StringRef GetTypedArrayStringTag(ElementsKind kind);
 
   bool ShouldBeSerializedForCompilation(const SharedFunctionInfoRef& shared,

src/compiler/pipeline.cc

@@ -1553,9 +1553,15 @@ struct SerializationPhase {
       ContextRef(data->broker(),
                  data->specialization_context().FromJust().context);
     }
-    if (FLAG_turbo_concurrent_get_property_access_info) {
-      data->broker()->ClearCachedPropertyAccessInfos();
-      data->dependencies()->ClearForConcurrentGetPropertyAccessInfo();
+    if (data->info()->concurrent_inlining()) {
+      if (FLAG_turbo_concurrent_get_property_access_info) {
+        data->broker()->ClearCachedPropertyAccessInfos();
+        data->dependencies()->ClearForConcurrentGetPropertyAccessInfo();
+      }
+      if (FLAG_stress_concurrent_inlining) {
+        // Force re-serialization from the background thread.
+        data->broker()->ClearReconstructibleData();
+      }
     }
   }
 };

src/compiler/refs-map.cc

@@ -27,6 +27,10 @@ RefsMap::Entry* RefsMap::LookupOrInsert(const Address& key) {
                                        []() { return nullptr; });
 }
 
+ObjectData* RefsMap::Remove(const Address& key) {
+  return UnderlyingMap::Remove(key, RefsMap::Hash(key));
+}
+
 uint32_t RefsMap::Hash(Address addr) { return static_cast<uint32_t>(addr); }
 
 }  // namespace compiler

src/compiler/refs-map.h

@@ -42,6 +42,7 @@ class RefsMap
   // Wrappers around methods from UnderlyingMap
   Entry* Lookup(const Address& key) const;
   Entry* LookupOrInsert(const Address& key);
+  ObjectData* Remove(const Address& key);
 
  private:
   static uint32_t Hash(Address addr);