[string] Add flag to use string forwarding table instead of ThinString

Add flag --always-use-string-forwarding-table to always use the
forwarding table (usually only used for shared strings) instead of
ThinString migrations initially (during GC strings will be migrated
to normal ThinStrings). The goal is to get more coverage of this code
that is designed for shared strings.

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

src/flags/flag-definitions.h

@@ -746,6 +746,9 @@ DEFINE_BOOL(trace_baseline_concurrent_compilation, false,
 // Internalize into a shared string table in the shared isolate
 DEFINE_BOOL(shared_string_table, false, "internalize strings into shared table")
 DEFINE_IMPLICATION(harmony_struct, shared_string_table)
+DEFINE_BOOL(
+    always_use_string_forwarding_table, false,
+    "use string forwarding table instead of thin strings for all strings")
 
 #if !defined(V8_OS_DARWIN) || !defined(V8_HOST_ARCH_ARM64)
 DEFINE_BOOL(write_code_using_rwx, true,

src/heap/scavenger.cc

@@ -398,6 +398,10 @@ void ScavengerCollector::CollectGarbage() {
     if (V8_UNLIKELY(FLAG_track_retaining_path)) {
       heap_->UpdateRetainersAfterScavenge();
     }
+
+    if (V8_UNLIKELY(FLAG_always_use_string_forwarding_table)) {
+      isolate_->string_forwarding_table()->UpdateAfterScavenge();
+    }
   }
 
   if (FLAG_concurrent_marking) {

src/objects/name-inl.h

@@ -133,13 +133,14 @@ void Name::set_raw_hash_field_if_empty(uint32_t hash) {
       reinterpret_cast<uint32_t*>(FIELD_ADDR(*this, kRawHashFieldOffset)),
       kEmptyHashField, hash);
   USE(result);
-  // CAS can only fail if the string is shared and the hash was already set
-  // (by another thread) or it is a forwarding index (that overwrites the
-  // previous hash).
-  // In all cases we don't want to overwrite the old value, so we don't handle
-  // the failure case.
+  // CAS can only fail if the string is shared or we use the forwarding table
+  // for all strings and the hash was already set (by another thread) or it is
+  // a forwarding index (that overwrites the previous hash).
+  // In all cases we don't want overwrite the old value, so we don't handle the
+  // failure case.
   DCHECK_IMPLIES(result != kEmptyHashField,
-                 String::cast(*this).IsShared() &&
+                 (String::cast(*this).IsShared() ||
+                  FLAG_always_use_string_forwarding_table) &&
                      (result == hash || IsForwardingIndex(hash)));
 }
 

src/objects/string-table.cc

@@ -433,7 +433,8 @@ namespace {
 void SetInternalizedReference(Isolate* isolate, String string,
                               String internalized) {
   // TODO(v8:12007): Support external strings.
-  if (string.IsShared() && !string.IsExternalString()) {
+  if ((string.IsShared() || FLAG_always_use_string_forwarding_table) &&
+      !string.IsExternalString()) {
     uint32_t field = string.raw_hash_field();
     // Don't use the forwarding table for strings that have an integer index.
     // Using the hash field for the integer index is more beneficial than
@@ -853,6 +854,8 @@ class StringForwardingTable::Block {
       std::function<bool(HeapObject object)> is_dead,
       std::function<void(HeapObject object, ObjectSlot slot, HeapObject target)>
           record_thin_slot);
+  void UpdateAfterScavenge(Isolate* isolate);
+  void UpdateAfterScavenge(Isolate* isolate, int up_to_index);
 
  private:
   static constexpr int kRecordSize = 2;
@@ -919,29 +922,63 @@ 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) {
-  String original = String::cast(Get(isolate, IndexOfOriginalString(index)));
-  if (is_dead(original)) return;
+  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.IsThinString()) return;
+  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.set_raw_hash_field(String::kEmptyHashField);
+    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.MakeThin(isolate, forward);
-  original.set_raw_hash_field(forward.raw_hash_field());
+  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).RawField(ThinString::kActualOffset);
-  record_thin_slot(original, slot, forward);
+      ThinString::cast(original_string).RawField(ThinString::kActualOffset);
+  record_thin_slot(original_string, slot, forward);
+}
+
+void StringForwardingTable::Block::UpdateAfterScavenge(Isolate* isolate) {
+  UpdateAfterScavenge(isolate, capacity_);
+}
+
+void StringForwardingTable::Block::UpdateAfterScavenge(Isolate* isolate,
+                                                       int up_to_index) {
+  DCHECK(FLAG_always_use_string_forwarding_table);
+  for (int index = 0; index < up_to_index; ++index) {
+    Object original = Get(isolate, IndexOfOriginalString(index));
+    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();
+        Set(IndexOfOriginalString(index), String::cast(forwarded_object));
+      } else {
+        Set(IndexOfOriginalString(index), deleted_element());
+      }
+    } else {
+      DCHECK(!object.map_word(kRelaxedLoad).IsForwardingAddress());
+    }
+  }
 }
 
 class StringForwardingTable::BlockVector {
@@ -1055,8 +1092,10 @@ StringForwardingTable::BlockVector* StringForwardingTable::EnsureCapacity(
 
 int StringForwardingTable::Add(Isolate* isolate, String string,
                                String forward_to) {
-  DCHECK(string.InSharedHeap());
-  DCHECK(forward_to.InSharedHeap());
+  DCHECK_IMPLIES(!FLAG_always_use_string_forwarding_table,
+                 string.InSharedHeap());
+  DCHECK_IMPLIES(!FLAG_always_use_string_forwarding_table,
+                 forward_to.InSharedHeap());
   int index = next_free_index_++;
   uint32_t index_in_block;
   const uint32_t block = BlockForIndex(index, &index_in_block);
@@ -1123,5 +1162,22 @@ void StringForwardingTable::CleanUpDuringGC(
   next_free_index_ = 0;
 }
 
+void StringForwardingTable::UpdateAfterScavenge() {
+  DCHECK(FLAG_always_use_string_forwarding_table);
+
+  if (next_free_index_ == 0) return;  // Early exit if table is empty.
+
+  BlockVector* blocks = blocks_.load(std::memory_order_relaxed);
+  const unsigned int last_block = static_cast<unsigned int>(blocks->size() - 1);
+  for (unsigned int block = 0; block < last_block; ++block) {
+    Block* data = blocks->LoadBlock(block, kAcquireLoad);
+    data->UpdateAfterScavenge(isolate_);
+  }
+  // Handle last block separately, as it is not filled to capacity.
+  const int max_index = IndexInBlock(next_free_index_ - 1, last_block) + 1;
+  blocks->LoadBlock(last_block, kAcquireLoad)
+      ->UpdateAfterScavenge(isolate_, max_index);
+}
+
 }  // namespace internal
 }  // namespace v8

src/objects/string-table.h

@@ -126,11 +126,13 @@ class StringForwardingTable {
       std::function<bool(HeapObject object)> is_dead,
       std::function<void(HeapObject object, ObjectSlot slot, HeapObject target)>
           record_thin_slot);
+  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);

test/cctest/heap/test-heap.cc

@@ -5285,6 +5285,11 @@ TEST(NewSpaceAllocationCounter) {
 
 
 TEST(OldSpaceAllocationCounter) {
+  // Using the string forwarding table can free allocations during sweeping, due
+  // to ThinString trimming, thus failing this test.
+  // The flag (and handling of the forwarding table/ThinString transitions in
+  // young gen) is only temporary so we just skip this test for now.
+  if (FLAG_always_use_string_forwarding_table) return;
   ManualGCScope manual_gc_scope;
   CcTest::InitializeVM();
   v8::HandleScope scope(CcTest::isolate());

test/cctest/test-identity-map.cc

@@ -785,6 +785,8 @@ TEST(CanonicalHandleScope) {
 
 TEST(GCShortCutting) {
   if (FLAG_single_generation) return;
+  // We don't create ThinStrings immediately when using the forwarding table.
+  if (FLAG_always_use_string_forwarding_table) return;
   ManualGCScope manual_gc_scope;
   IdentityMapTester t;
   Isolate* isolate = CcTest::i_isolate();

test/cctest/test-shared-strings.cc

@@ -571,7 +571,7 @@ UNINITIALIZED_TEST(StringShare) {
     CheckSharedStringIsEqualCopy(shared_two_byte, young_two_byte_seq);
   }
 
-  {
+  if (!FLAG_always_use_string_forwarding_table) {
     // Thin strings
     Handle<String> one_byte_seq1 =
         factory->NewStringFromAsciiChecked(raw_one_byte);

test/cctest/test-strings.cc

@@ -1922,6 +1922,8 @@ class OneByteStringResource : public v8::String::ExternalOneByteStringResource {
 TEST(Regress876759) {
   // Thin strings are used in conjunction with young gen
   if (FLAG_single_generation) return;
+  // We don't create ThinStrings immediately when using the forwarding table.
+  if (FLAG_always_use_string_forwarding_table) return;
   Isolate* isolate = CcTest::i_isolate();
   Factory* factory = isolate->factory();
 

test/unittests/objects/concurrent-string-unittest.cc

@@ -235,6 +235,8 @@ TEST_F(ConcurrentStringTest, InspectTwoByteExternalizing) {
 TEST_F(ConcurrentStringTest, InspectOneByteExternalizing_ThinString) {
   // We will not create a thin string if single_generation is turned on.
   if (FLAG_single_generation) return;
+  // We don't create ThinStrings immediately when using the forwarding table.
+  if (FLAG_always_use_string_forwarding_table) return;
   std::unique_ptr<PersistentHandles> ph = i_isolate()->NewPersistentHandles();
 
   auto factory = i_isolate()->factory();
@@ -295,6 +297,8 @@ TEST_F(ConcurrentStringTest, InspectOneByteExternalizing_ThinString) {
 TEST_F(ConcurrentStringTest, InspectOneIntoTwoByteExternalizing_ThinString) {
   // We will not create a thin string if single_generation is turned on.
   if (FLAG_single_generation) return;
+  // We don't create ThinStrings immediately when using the forwarding table.
+  if (FLAG_always_use_string_forwarding_table) return;
   std::unique_ptr<PersistentHandles> ph = i_isolate()->NewPersistentHandles();
 
   auto factory = i_isolate()->factory();
@@ -355,6 +359,8 @@ TEST_F(ConcurrentStringTest, InspectOneIntoTwoByteExternalizing_ThinString) {
 TEST_F(ConcurrentStringTest, InspectTwoByteExternalizing_ThinString) {
   // We will not create a thin string if single_generation is turned on.
   if (FLAG_single_generation) return;
+  // We don't create ThinStrings immediately when using the forwarding table.
+  if (FLAG_always_use_string_forwarding_table) return;
   std::unique_ptr<PersistentHandles> ph = i_isolate()->NewPersistentHandles();
 
   auto factory = i_isolate()->factory();