Reland "[heap] Transition/Shortcut strings only during GCs without stack"

This is a reland of commit 7bf94d0336

Changes since revert:
- Update string forwarding table with evacuated objects in mark compact.
- Always mark forward objects in string forwarding table.

Original change's description:
> [heap] Transition/Shortcut strings only during GCs without stack
>
> By limiting transitions of (shared) strings and shortcutting of
> Thin/Cons strings to GC withouts stacks, optimizing compilers can rely on
> the invariant that string maps do not change during a GC, allowing them
> to eliminate map checks and enable more aggressive optimizations.
>
> Change-Id: Ic9c9ed7b04b2ceed369484bf048965c083a9a693
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4030578
> Commit-Queue: Patrick Thier <pthier@chromium.org>
> Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
> Cr-Commit-Position: refs/heads/main@{#84347}

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

src/flags/flag-definitions.h

@@ -735,6 +735,9 @@ DEFINE_BOOL(
 // forwarding table.
 DEFINE_NEG_IMPLICATION(shared_string_table, always_use_string_forwarding_table)
 
+DEFINE_BOOL(transition_strings_during_gc_with_stack, false,
+            "Transition strings during a full GC with stack")
+
 DEFINE_SIZE_T(initial_shared_heap_size, 0,
               "initial size of the shared heap (in Mbytes); "
               "other heap size flags (e.g. initial_heap_size) take precedence")
@@ -1440,6 +1443,8 @@ DEFINE_BOOL(compact_with_stack, true,
 DEFINE_BOOL(
     compact_code_space_with_stack, true,
     "Perform code space compaction when finalizing a full GC with stack")
+DEFINE_BOOL(shortcut_strings_with_stack, false,
+            "Shortcut Strings during GC with stack")
 DEFINE_BOOL(stress_compaction, false,
             "Stress GC compaction to flush out bugs (implies "
             "--force_marking_deque_overflows)")

src/heap/mark-compact.cc

@@ -1797,7 +1797,9 @@ class EvacuateNewSpaceVisitor final : public EvacuateVisitorBase {
         pretenuring_handler_(heap_->pretenuring_handler()),
         local_pretenuring_feedback_(local_pretenuring_feedback),
         is_incremental_marking_(heap->incremental_marking()->IsMarking()),
-        always_promote_young_(always_promote_young) {}
+        always_promote_young_(always_promote_young),
+        shortcut_strings_(!heap_->IsGCWithStack() ||
+                          v8_flags.shortcut_strings_with_stack) {}
 
   inline bool Visit(HeapObject object, int size) override {
     if (TryEvacuateWithoutCopy(object)) return true;
@@ -1843,6 +1845,8 @@ class EvacuateNewSpaceVisitor final : public EvacuateVisitorBase {
   inline bool TryEvacuateWithoutCopy(HeapObject object) {
     DCHECK(!is_incremental_marking_);
 
+    if (!shortcut_strings_) return false;
+
     Map map = object.map();
 
     // Some objects can be evacuated without creating a copy.
@@ -1892,6 +1896,7 @@ class EvacuateNewSpaceVisitor final : public EvacuateVisitorBase {
   PretenturingHandler::PretenuringFeedbackMap* local_pretenuring_feedback_;
   bool is_incremental_marking_;
   AlwaysPromoteYoung always_promote_young_;
+  const bool shortcut_strings_;
 };
 
 template <PageEvacuationMode mode>
@@ -2861,24 +2866,54 @@ class StringForwardingTableCleaner final {
   explicit StringForwardingTableCleaner(Heap* heap)
       : heap_(heap),
         isolate_(heap_->isolate()),
-        marking_state_(heap_->non_atomic_marking_state()) {}
+        marking_state_(heap_->non_atomic_marking_state()),
+        transition_strings_(!heap_->IsGCWithStack() ||
+                            v8_flags.transition_strings_during_gc_with_stack) {}
   void Run() {
     StringForwardingTable* forwarding_table =
         isolate_->string_forwarding_table();
-    forwarding_table->IterateElements(
-        [&](StringForwardingTable::Record* record) {
-          TransitionStrings(record);
-        });
-    forwarding_table->Reset();
+    if (transition_strings_) {
+      forwarding_table->IterateElements(
+          [&](StringForwardingTable::Record* record) {
+            TransitionStrings(record);
+          });
+      forwarding_table->Reset();
+    } else {
+      // When performing GC with a stack, we conservatively assume that
+      // the GC could have been triggered by optimized code. Optimized code
+      // assumes that flat strings don't transition during GCs, so we are not
+      // allowed to transition strings to ThinString/ExternalString in that
+      // case.
+      // Instead we mark forward objects to keep them alive and update entries
+      // of evacuated objects later.
+      forwarding_table->IterateElements(
+          [&](StringForwardingTable::Record* record) {
+            MarkForwardObject(record);
+          });
+    }
   }
 
  private:
+  void MarkForwardObject(StringForwardingTable::Record* record) {
+    Object original = record->OriginalStringObject(isolate_);
+    if (!original.IsHeapObject()) {
+      DCHECK_EQ(original, StringForwardingTable::deleted_element());
+      return;
+    }
+    String original_string = String::cast(original);
+    if (marking_state_->IsBlack(original_string)) {
+      Object forward = record->ForwardStringObjectOrHash(isolate_);
+      if (!forward.IsHeapObject()) return;
+      marking_state_->WhiteToBlack(HeapObject::cast(forward));
+    } else {
+      record->DisposeUnusedExternalResource(original_string);
+      record->set_original_string(StringForwardingTable::deleted_element());
+    }
+  }
+
   void TransitionStrings(StringForwardingTable::Record* record) {
     Object original = record->OriginalStringObject(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(v8_flags.always_use_string_forwarding_table);
       DCHECK_EQ(original, StringForwardingTable::deleted_element());
       return;
     }
@@ -2942,6 +2977,7 @@ class StringForwardingTableCleaner final {
   Heap* const heap_;
   Isolate* const isolate_;
   NonAtomicMarkingState* const marking_state_;
+  bool transition_strings_;
 };
 
 }  // namespace
@@ -2953,7 +2989,8 @@ void MarkCompactCollector::ClearNonLiveReferences() {
     TRACE_GC(heap()->tracer(),
              GCTracer::Scope::MC_CLEAR_STRING_FORWARDING_TABLE);
     // Clear string forwarding table. Live strings are transitioned to
-    // ThinStrings/ExternalStrings in the cleanup process.
+    // ThinStrings/ExternalStrings in the cleanup process, if this is a GC
+    // without stack.
     // Clearing the string forwarding table must happen before clearing the
     // string table, as entries in the forwarding table can keep internalized
     // strings alive.
@@ -5296,6 +5333,12 @@ void MarkCompactCollector::UpdatePointersAfterEvacuation() {
     heap_->UpdateReferencesInExternalStringTable(
         &UpdateReferenceInExternalStringTableEntry);
 
+    // Update pointers in string forwarding table.
+    // When GC was performed without a stack, the table was cleared and this
+    // does nothing. In the case this was a GC with stack, we need to update
+    // the entries for evacuated objects.
+    isolate()->string_forwarding_table()->UpdateAfterFullEvacuation();
+
     EvacuationWeakObjectRetainer evacuation_object_retainer;
     heap()->ProcessWeakListRoots(&evacuation_object_retainer);
   }

src/heap/scavenger-inl.h

@@ -290,7 +290,7 @@ SlotCallbackResult Scavenger::EvacuateThinString(Map map, THeapObjectSlot slot,
   static_assert(std::is_same<THeapObjectSlot, FullHeapObjectSlot>::value ||
                     std::is_same<THeapObjectSlot, HeapObjectSlot>::value,
                 "Only FullHeapObjectSlot and HeapObjectSlot are expected here");
-  if (!is_incremental_marking_) {
+  if (!is_incremental_marking_ && shortcut_strings_) {
     // The ThinString should die after Scavenge, so avoid writing the proper
     // forwarding pointer and instead just signal the actual object as forwarded
     // reference.
@@ -317,7 +317,8 @@ SlotCallbackResult Scavenger::EvacuateShortcutCandidate(Map map,
                     std::is_same<THeapObjectSlot, HeapObjectSlot>::value,
                 "Only FullHeapObjectSlot and HeapObjectSlot are expected here");
   DCHECK(IsShortcutCandidate(map.instance_type()));
-  if (!is_incremental_marking_ &&
+
+  if (!is_incremental_marking_ && shortcut_strings_ &&
       object.unchecked_second() == ReadOnlyRoots(heap()).empty_string()) {
     HeapObject first = HeapObject::cast(object.unchecked_first());
 

src/heap/scavenger.cc

@@ -449,7 +449,7 @@ void ScavengerCollector::CollectGarbage() {
     }
 
     if (V8_UNLIKELY(v8_flags.always_use_string_forwarding_table)) {
-      isolate_->string_forwarding_table()->UpdateAfterEvacuation();
+      isolate_->string_forwarding_table()->UpdateAfterYoungEvacuation();
     }
   }
 
@@ -634,7 +634,9 @@ Scavenger::Scavenger(ScavengerCollector* collector, Heap* heap, bool is_logging,
       is_incremental_marking_(heap->incremental_marking()->IsMarking()),
       is_compacting_(heap->incremental_marking()->IsCompacting()),
       shared_string_table_(shared_old_allocator_.get() != nullptr),
-      mark_shared_heap_(heap->isolate()->is_shared_space_isolate()) {}
+      mark_shared_heap_(heap->isolate()->is_shared_space_isolate()),
+      shortcut_strings_(!heap->IsGCWithStack() ||
+                        v8_flags.shortcut_strings_with_stack) {}
 
 void Scavenger::IterateAndScavengePromotedObject(HeapObject target, Map map,
                                                  int size) {

src/heap/scavenger.h

@@ -214,6 +214,7 @@ class Scavenger {
   const bool is_compacting_;
   const bool shared_string_table_;
   const bool mark_shared_heap_;
+  const bool shortcut_strings_;
 
   friend class IterateAndScavengePromotedObjectsVisitor;
   friend class RootScavengeVisitor;

src/objects/string-forwarding-table-inl.h

@@ -248,8 +248,10 @@ class StringForwardingTable::Block {
     return &elements_[index];
   }
 
-  void UpdateAfterEvacuation(PtrComprCageBase cage_base);
-  void UpdateAfterEvacuation(PtrComprCageBase cage_base, int up_to_index);
+  void UpdateAfterYoungEvacuation(PtrComprCageBase cage_base);
+  void UpdateAfterYoungEvacuation(PtrComprCageBase cage_base, int up_to_index);
+  void UpdateAfterFullEvacuation(PtrComprCageBase cage_base);
+  void UpdateAfterFullEvacuation(PtrComprCageBase cage_base, int up_to_index);
 
  private:
   const int capacity_;

src/objects/string-forwarding-table.cc

@@ -58,32 +58,74 @@ std::unique_ptr<StringForwardingTable::Block> StringForwardingTable::Block::New(
   return std::unique_ptr<Block>(new (capacity) Block(capacity));
 }
 
-void StringForwardingTable::Block::UpdateAfterEvacuation(
+void StringForwardingTable::Block::UpdateAfterYoungEvacuation(
     PtrComprCageBase cage_base) {
-  UpdateAfterEvacuation(cage_base, capacity_);
+  UpdateAfterYoungEvacuation(cage_base, capacity_);
 }
 
-void StringForwardingTable::Block::UpdateAfterEvacuation(
+void StringForwardingTable::Block::UpdateAfterFullEvacuation(
+    PtrComprCageBase cage_base) {
+  UpdateAfterFullEvacuation(cage_base, capacity_);
+}
+
+namespace {
+
+bool UpdateForwardedSlot(HeapObject object, OffHeapObjectSlot slot) {
+  MapWord map_word = object.map_word(kRelaxedLoad);
+  if (map_word.IsForwardingAddress()) {
+    HeapObject forwarded_object = map_word.ToForwardingAddress(object);
+    slot.Release_Store(forwarded_object);
+    return true;
+  }
+  return false;
+}
+
+bool UpdateForwardedSlot(Object object, OffHeapObjectSlot slot) {
+  if (!object.IsHeapObject()) return false;
+  return UpdateForwardedSlot(HeapObject::cast(object), slot);
+}
+
+}  // namespace
+
+void StringForwardingTable::Block::UpdateAfterYoungEvacuation(
     PtrComprCageBase cage_base, int up_to_index) {
-  // This is only used for Scavenger.
-  DCHECK(!v8_flags.minor_mc);
-  DCHECK(v8_flags.always_use_string_forwarding_table);
   for (int index = 0; index < up_to_index; ++index) {
-    Object original = record(index)->OriginalStringObject(cage_base);
+    OffHeapObjectSlot slot = record(index)->OriginalStringSlot();
+    Object original = slot.Acquire_Load(cage_base);
     if (!original.IsHeapObject()) continue;
     HeapObject object = HeapObject::cast(original);
     if (Heap::InFromPage(object)) {
       DCHECK(!object.InSharedWritableHeap());
-      MapWord map_word = object.map_word(kRelaxedLoad);
-      if (map_word.IsForwardingAddress()) {
-        HeapObject forwarded_object = map_word.ToForwardingAddress(object);
-        record(index)->set_original_string(forwarded_object);
-      } else {
-        record(index)->set_original_string(deleted_element());
+      const bool was_forwarded = UpdateForwardedSlot(object, slot);
+      if (!was_forwarded) {
+        // The object died in young space.
+        slot.Release_Store(deleted_element());
       }
     } else {
       DCHECK(!object.map_word(kRelaxedLoad).IsForwardingAddress());
     }
+// No need to update forwarded (internalized) strings as they are never
+// in young space.
+#ifdef DEBUG
+    Object forward = record(index)->ForwardStringObjectOrHash(cage_base);
+    if (forward.IsHeapObject()) {
+      DCHECK(!Heap::InYoungGeneration(HeapObject::cast(forward)));
+    }
+#endif
+  }
+}
+
+void StringForwardingTable::Block::UpdateAfterFullEvacuation(
+    PtrComprCageBase cage_base, int up_to_index) {
+  for (int index = 0; index < up_to_index; ++index) {
+    OffHeapObjectSlot original_slot = record(index)->OriginalStringSlot();
+    Object original = original_slot.Acquire_Load(cage_base);
+    UpdateForwardedSlot(original, original_slot);
+    // During mark compact the forwarded (internalized) string may have been
+    // evacuated.
+    OffHeapObjectSlot forward_slot = record(index)->ForwardStringOrHashSlot();
+    Object forward = forward_slot.Acquire_Load(cage_base);
+    UpdateForwardedSlot(forward, forward_slot);
   }
 }
 
@@ -295,7 +337,9 @@ void StringForwardingTable::Reset() {
   next_free_index_ = 0;
 }
 
-void StringForwardingTable::UpdateAfterEvacuation() {
+void StringForwardingTable::UpdateAfterYoungEvacuation() {
+  // This is only used for the Scavenger.
+  DCHECK(!v8_flags.minor_mc);
   DCHECK(v8_flags.always_use_string_forwarding_table);
 
   if (empty()) return;
@@ -306,12 +350,29 @@ void StringForwardingTable::UpdateAfterEvacuation() {
   for (unsigned int block_index = 0; block_index < last_block_index;
        ++block_index) {
     Block* block = blocks->LoadBlock(block_index, kAcquireLoad);
-    block->UpdateAfterEvacuation(isolate_);
+    block->UpdateAfterYoungEvacuation(isolate_);
   }
   // Handle last block separately, as it is not filled to capacity.
   const int max_index = IndexInBlock(size() - 1, last_block_index) + 1;
   blocks->LoadBlock(last_block_index, kAcquireLoad)
-      ->UpdateAfterEvacuation(isolate_, max_index);
+      ->UpdateAfterYoungEvacuation(isolate_, max_index);
+}
+
+void StringForwardingTable::UpdateAfterFullEvacuation() {
+  if (empty()) return;
+
+  BlockVector* blocks = blocks_.load(std::memory_order_relaxed);
+  const unsigned int last_block_index =
+      static_cast<unsigned int>(blocks->size() - 1);
+  for (unsigned int block_index = 0; block_index < last_block_index;
+       ++block_index) {
+    Block* block = blocks->LoadBlock(block_index, kAcquireLoad);
+    block->UpdateAfterFullEvacuation(isolate_);
+  }
+  // Handle last block separately, as it is not filled to capacity.
+  const int max_index = IndexInBlock(size() - 1, last_block_index) + 1;
+  blocks->LoadBlock(last_block_index, kAcquireLoad)
+      ->UpdateAfterFullEvacuation(isolate_, max_index);
 }
 
 }  // namespace internal

src/objects/string-forwarding-table.h

@@ -65,7 +65,8 @@ class StringForwardingTable {
   // Dispose all external resources stored in the table.
   void TearDown();
   void Reset();
-  void UpdateAfterEvacuation();
+  void UpdateAfterYoungEvacuation();
+  void UpdateAfterFullEvacuation();
 
   class Record;
 

test/cctest/test-shared-strings.cc

@@ -1056,6 +1056,7 @@ UNINITIALIZED_TEST(InternalizedSharedStringsTransitionDuringGC) {
   if (!V8_CAN_CREATE_SHARED_HEAP_BOOL) return;
 
   v8_flags.shared_string_table = true;
+  v8_flags.transition_strings_during_gc_with_stack = true;
 
   constexpr int kStrings = 4096;
 
@@ -1199,6 +1200,7 @@ UNINITIALIZED_TEST(ExternalizedSharedStringsTransitionDuringGC) {
   if (!V8_CAN_CREATE_SHARED_HEAP_BOOL) return;
 
   v8_flags.shared_string_table = true;
+  v8_flags.transition_strings_during_gc_with_stack = true;
 
   ExternalResourceFactory resource_factory;
   MultiClientIsolateTest test;
@@ -1318,6 +1320,7 @@ UNINITIALIZED_TEST(InternalizeSharedExternalString) {
   if (!V8_CAN_CREATE_SHARED_HEAP_BOOL) return;
 
   v8_flags.shared_string_table = true;
+  v8_flags.transition_strings_during_gc_with_stack = true;
 
   ExternalResourceFactory resource_factory;
   MultiClientIsolateTest test;
@@ -1584,6 +1587,7 @@ void TestConcurrentExternalization(bool share_resources) {
   if (!V8_CAN_CREATE_SHARED_HEAP_BOOL) return;
 
   v8_flags.shared_string_table = true;
+  v8_flags.transition_strings_during_gc_with_stack = true;
 
   ExternalResourceFactory resource_factory;
   MultiClientIsolateTest test;
@@ -1681,6 +1685,7 @@ void TestConcurrentExternalizationAndInternalization(
   if (!V8_CAN_CREATE_SHARED_HEAP_BOOL) return;
 
   v8_flags.shared_string_table = true;
+  v8_flags.transition_strings_during_gc_with_stack = true;
 
   ExternalResourceFactory resource_factory;
   MultiClientIsolateTest test;

test/unittests/utils/identity-map-unittest.cc

@@ -788,6 +788,7 @@ TEST_F(IdentityMapTest, GCShortCutting) {
   if (v8_flags.single_generation) return;
   // We don't create ThinStrings immediately when using the forwarding table.
   if (v8_flags.always_use_string_forwarding_table) return;
+  v8_flags.shortcut_strings_with_stack = true;
   ManualGCScope manual_gc_scope(isolate());
   IdentityMapTester t(isolate()->heap(), zone());
   Factory* factory = isolate()->factory();