[heap] Mark internalized strings in forwarding table

For entries in the string forwarding table, mark the internalized string
if the original string is marked.

The logic is moved from the string forwarding table implementation to
the mark compact implementation, using RootVisitor.

Bug: v8:12007
Change-Id: I860de75077c864dd4e5f2c47ab647d2eafcc5ced
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3610625Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Commit-Queue: Patrick Thier <pthier@chromium.org>
Cr-Commit-Position: refs/heads/main@{#80326}

src/heap/gc-tracer.cc

@@ -882,6 +882,7 @@ void GCTracer::PrintNVP() const {
           "heap.external.weak_global_handles=%.1f "
           "clear=%1.f "
           "clear.external_string_table=%.1f "
+          "clear.string_forwarding_table=%.1f "
           "clear.weak_global_handles=%.1f "
           "clear.dependent_code=%.1f "
           "clear.maps=%.1f "
@@ -971,6 +972,7 @@ void GCTracer::PrintNVP() const {
           current_scope(Scope::HEAP_EXTERNAL_WEAK_GLOBAL_HANDLES),
           current_scope(Scope::MC_CLEAR),
           current_scope(Scope::MC_CLEAR_EXTERNAL_STRING_TABLE),
+          current_scope(Scope::MC_CLEAR_STRING_FORWARDING_TABLE),
           current_scope(Scope::MC_CLEAR_WEAK_GLOBAL_HANDLES),
           current_scope(Scope::MC_CLEAR_DEPENDENT_CODE),
           current_scope(Scope::MC_CLEAR_MAPS),

src/heap/mark-compact.cc

@@ -1388,6 +1388,64 @@ class InternalizedStringTableCleaner final : public RootVisitor {
   int pointers_removed_ = 0;
 };
 
+class StringForwardingTableCleaner final : public RootVisitor {
+ public:
+  explicit StringForwardingTableCleaner(Heap* heap) : heap_(heap) {}
+
+  void VisitRootPointers(Root root, const char* description,
+                         FullObjectSlot start, FullObjectSlot end) override {
+    UNREACHABLE();
+  }
+
+  void VisitRootPointers(Root root, const char* description,
+                         OffHeapObjectSlot start,
+                         OffHeapObjectSlot end) override {
+    DCHECK_EQ(root, Root::kStringForwardingTable);
+    // Visit all HeapObject pointers in [start, end).
+    // The forwarding table is organized in pairs of [orig string, forward
+    // string].
+    auto* marking_state =
+        heap_->mark_compact_collector()->non_atomic_marking_state();
+    Isolate* isolate = heap_->isolate();
+    for (OffHeapObjectSlot p = start; p < end; p += 2) {
+      Object original = p.load(isolate);
+      if (!original.IsHeapObject()) {
+        // Only if we always use the forwarding table, the string could be a
+        // smi, indicating that the entry died during scavenge.
+        DCHECK(FLAG_always_use_string_forwarding_table);
+        DCHECK_EQ(original, StringForwardingTable::deleted_element());
+        continue;
+      }
+      if (marking_state->IsBlack(HeapObject::cast(original))) {
+        String original_string = String::cast(original);
+        // Check if the string was already transitioned. This happens if we have
+        // multiple entries for the same string in the table.
+        if (original_string.IsThinString()) continue;
+
+        // The second slot of each record is the forward string.
+        Object forward = (p + 1).load(isolate);
+        String forward_string = String::cast(forward);
+
+        // Mark the forwarded string.
+        marking_state->WhiteToBlack(forward_string);
+
+        // Transition the original string to a ThinString and override the
+        // forwarding index with the correct hash.
+        original_string.MakeThin(isolate, forward_string);
+        original_string.set_raw_hash_field(forward_string.raw_hash_field());
+        // Record the slot in the old-to-old remembered set. This is required as
+        // the internalized string could be relocated during compaction.
+        ObjectSlot slot = ThinString::cast(original_string)
+                              .RawField(ThinString::kActualOffset);
+        MarkCompactCollector::RecordSlot(original_string, slot, forward_string);
+      }
+    }
+  }
+
+ private:
+  Heap* heap_;
+};
+
 class ExternalStringTableCleaner : public RootVisitor {
  public:
   explicit ExternalStringTableCleaner(Heap* heap) : heap_(heap) {}
@@ -2563,17 +2621,18 @@ void MarkCompactCollector::ClearNonLiveReferences() {
   TRACE_GC(heap()->tracer(), GCTracer::Scope::MC_CLEAR);
 
   if (isolate()->OwnsStringTables()) {
+    TRACE_GC(heap()->tracer(),
+             GCTracer::Scope::MC_CLEAR_STRING_FORWARDING_TABLE);
     // Clear string forwarding table. Live strings are transitioned to
     // ThinStrings in the cleanup process.
+    // Clearing the string forwarding table must happen before clearing the
+    // string table, as entries in the forwarding table can keep internalized
+    // strings alive.
     StringForwardingTable* forwarding_table =
         isolate()->string_forwarding_table();
-    auto is_dead = [=](HeapObject object) {
-      return non_atomic_marking_state_.IsWhite(object);
-    };
-    auto record_slot = [](HeapObject object, ObjectSlot slot, Object target) {
-      RecordSlot(object, slot, HeapObject::cast(target));
-    };
-    forwarding_table->CleanUpDuringGC(is_dead, record_slot);
+    StringForwardingTableCleaner visitor(heap());
+    forwarding_table->IterateElements(&visitor);
+    forwarding_table->Reset();
   }
 
   auto clearing_job = std::make_unique<ParallelClearingJob>();

src/init/heap-symbols.h

@@ -536,6 +536,7 @@
   TOP_MC_SCOPES(F)                                   \
   F(MC_CLEAR_DEPENDENT_CODE)                         \
   F(MC_CLEAR_EXTERNAL_STRING_TABLE)                  \
+  F(MC_CLEAR_STRING_FORWARDING_TABLE)                \
   F(MC_CLEAR_FLUSHABLE_BYTECODE)                     \
   F(MC_CLEAR_FLUSHED_JS_FUNCTIONS)                   \
   F(MC_CLEAR_JOIN_JOB)                               \

src/objects/string-table.cc

@@ -841,11 +841,12 @@ class StringForwardingTable::Block {
     return String::cast(Get(isolate, IndexOfForwardString(index)));
   }
 
-  void TransitionStringIfAlive(
-      Isolate* isolate, int index,
-      std::function<bool(HeapObject object)> is_dead,
-      std::function<void(HeapObject object, ObjectSlot slot, HeapObject target)>
-          record_thin_slot);
+  void IterateElements(RootVisitor* visitor, int up_to_index) {
+    OffHeapObjectSlot first_slot = slot(0);
+    OffHeapObjectSlot end_slot = slot(IndexOfOriginalString(up_to_index));
+    visitor->VisitRootPointers(Root::kStringForwardingTable, nullptr,
+                               first_slot, end_slot);
+  }
   void UpdateAfterScavenge(Isolate* isolate);
   void UpdateAfterScavenge(Isolate* isolate, int up_to_index);
 
@@ -910,43 +911,6 @@ std::unique_ptr<StringForwardingTable::Block> StringForwardingTable::Block::New(
   return std::unique_ptr<Block>(new (capacity) Block(capacity));
 }
 
-void StringForwardingTable::Block::TransitionStringIfAlive(
-    Isolate* isolate, int index, std::function<bool(HeapObject object)> is_dead,
-    std::function<void(HeapObject object, ObjectSlot slot, HeapObject target)>
-        record_thin_slot) {
-  Object original = Get(isolate, IndexOfOriginalString(index));
-  if (!original.IsHeapObject()) {
-    // Only if we always use the forwarding table, the string could be a smi,
-    // indicating that the entry died during scavenge.
-    DCHECK(FLAG_always_use_string_forwarding_table);
-    DCHECK_EQ(original, deleted_element());
-    return;
-  }
-  if (is_dead(HeapObject::cast(original))) return;
-  String original_string = String::cast(original);
-
-  // Check if the string was already transitioned. This happens if we have
-  // multiple entries for the same string in the table.
-  if (original_string.IsThinString()) return;
-
-  String forward = String::cast(Get(isolate, IndexOfForwardString(index)));
-  if (is_dead(forward)) {
-    // TODO(v8:12007): We should mark the internalized strings if the original
-    // is alive to keep them alive in the StringTable.
-    original_string.set_raw_hash_field(String::kEmptyHashField);
-    return;
-  }
-  // Transition the original string to a ThinString and override the forwarding
-  // index with the correct hash.
-  original_string.MakeThin(isolate, forward);
-  original_string.set_raw_hash_field(forward.raw_hash_field());
-  // Record the slot in the old-to-old remembered set. This is required as the
-  // internalized string could be relocated during compaction.
-  ObjectSlot slot =
-      ThinString::cast(original_string).RawField(ThinString::kActualOffset);
-  record_thin_slot(original_string, slot, forward);
-}
-
 void StringForwardingTable::Block::UpdateAfterScavenge(Isolate* isolate) {
   UpdateAfterScavenge(isolate, capacity_);
 }
@@ -1115,20 +1079,7 @@ Address StringForwardingTable::GetForwardStringAddress(Isolate* isolate,
       .ptr();
 }
 
-void StringForwardingTable::TransitionAliveStrings(
-    StringForwardingTable::Block* block, int up_to_index,
-    std::function<bool(HeapObject object)> is_dead,
-    std::function<void(HeapObject object, ObjectSlot slot, HeapObject target)>
-        record_thin_slot) {
-  for (int index = 0; index < up_to_index; ++index) {
-    block->TransitionStringIfAlive(isolate_, index, is_dead, record_thin_slot);
-  }
-}
-
-void StringForwardingTable::CleanUpDuringGC(
-    std::function<bool(HeapObject object)> is_dead,
-    std::function<void(HeapObject object, ObjectSlot slot, HeapObject target)>
-        record_thin_slot) {
+void StringForwardingTable::IterateElements(RootVisitor* visitor) {
   isolate_->heap()->safepoint()->AssertActive();
   DCHECK_NE(isolate_->heap()->gc_state(), Heap::NOT_IN_GC);
 
@@ -1138,17 +1089,23 @@ void StringForwardingTable::CleanUpDuringGC(
   const uint32_t last_block = static_cast<uint32_t>(blocks->size() - 1);
   for (uint32_t block = 0; block < last_block; ++block) {
     Block* data = blocks->LoadBlock(block);
-    TransitionAliveStrings(data, data->capacity(), is_dead, record_thin_slot);
-    // Free block after all strings have been processed.
-    delete data;
+    data->IterateElements(visitor, data->capacity());
   }
   // Handle last block separately, as it is not filled to capacity.
   const uint32_t max_index = IndexInBlock(next_free_index_ - 1, last_block) + 1;
   Block* data = blocks->LoadBlock(last_block);
-  TransitionAliveStrings(data, max_index, is_dead, record_thin_slot);
-  delete data;
+  data->IterateElements(visitor, max_index);
+}
+
+void StringForwardingTable::Reset() {
+  isolate_->heap()->safepoint()->AssertActive();
+  DCHECK_NE(isolate_->heap()->gc_state(), Heap::NOT_IN_GC);
+
+  BlockVector* blocks = blocks_.load(std::memory_order_relaxed);
+  for (uint32_t block = 0; block < blocks->size(); ++block) {
+    delete blocks->LoadBlock(block);
+  }
 
-  // Reset the whole table and clear old copies.
   block_vector_storage_.clear();
   InitializeBlockVector();
   next_free_index_ = 0;

src/objects/string-table.h

@@ -111,6 +111,7 @@ class StringForwardingTable {
       kBitsPerInt - base::bits::CountLeadingZeros32(kInitialBlockSize) - 1;
   // Initial capacity in the block vector.
   static constexpr int kInitialBlockVectorCapacity = 4;
+  static constexpr Smi deleted_element() { return Smi::FromInt(0); }
 
   explicit StringForwardingTable(Isolate* isolate);
   ~StringForwardingTable();
@@ -120,18 +121,14 @@ class StringForwardingTable {
   int Add(Isolate* isolate, String string, String forward_to);
   String GetForwardString(Isolate* isolate, int index) const;
   static Address GetForwardStringAddress(Isolate* isolate, int index);
-  // Clears the table and transitions all alive strings to ThinStrings.
-  void CleanUpDuringGC(
-      std::function<bool(HeapObject object)> is_dead,
-      std::function<void(HeapObject object, ObjectSlot slot, HeapObject target)>
-          record_thin_slot);
+  void IterateElements(RootVisitor* visitor);
+  void Reset();
   void UpdateAfterScavenge();
 
  private:
   class Block;
   class BlockVector;
 
-  static constexpr Smi deleted_element() { return Smi::FromInt(0); }
   // Returns the block for a given index and sets the index within this block
   // as out parameter.
   static inline uint32_t BlockForIndex(int index, uint32_t* index_in_block_out);
@@ -144,11 +141,6 @@ class StringForwardingTable {
   // inserted into the BlockVector. The BlockVector itself might grow (to double
   // the capacity).
   BlockVector* EnsureCapacity(uint32_t block);
-  void TransitionAliveStrings(
-      Block* block, int up_to_index,
-      std::function<bool(HeapObject object)> is_dead,
-      std::function<void(HeapObject object, ObjectSlot slot, HeapObject target)>
-          record_thin_slot);
 
   Isolate* isolate_;
   std::atomic<BlockVector*> blocks_;

src/objects/visitors.h

@@ -18,6 +18,7 @@ class CodeDataContainer;
 
 #define ROOT_ID_LIST(V)                                 \
   V(kStringTable, "(Internalized strings)")             \
+  V(kStringForwardingTable, "(Forwarded strings)")      \
   V(kExternalStringsTable, "(External strings)")        \
   V(kReadOnlyRootList, "(Read-only roots)")             \
   V(kStrongRootList, "(Strong roots)")                  \