[string] Non-transitioning shared strings

Instead of transitioning shared strings to ThinString on
internalization, use a forwarding table to the internalized string and
store the index into the forwarding table in the string's hash field.

This way we don't need to handle concurrent string transitions that
modify the underlying string data.

During stop-the-world GC, live strings in the forwarding table are
migrated to regular ThinStrings.

Bug: v8:12007
Change-Id: I6c6f3d41c6f644e0aaeafbf25ecec5ce0aa0d2d8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3536647Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Reviewed-by: Jakob Linke <jgruber@chromium.org>
Reviewed-by: Shu-yu Guo <syg@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Patrick Thier <pthier@chromium.org>
Cr-Commit-Position: refs/heads/main@{#79801}

src/codegen/code-stub-assembler.cc

@@ -8071,8 +8071,10 @@ void CodeStubAssembler::TryToName(TNode<Object> key, Label* if_keyisindex,
 
     BIND(&if_string);
     {
-      Label if_thinstring(this), if_has_cached_index(this);
+      Label if_thinstring(this), if_has_cached_index(this),
+          if_forwarding_index(this);
 
+      // TODO(v8:12007): LoadNameRawHashField() should be an acquire load.
       TNode<Uint32T> raw_hash_field = LoadNameRawHashField(CAST(key));
       GotoIf(IsClearWord32(raw_hash_field,
                            Name::kDoesNotContainCachedArrayIndexMask),
@@ -8088,6 +8090,12 @@ void CodeStubAssembler::TryToName(TNode<Object> key, Label* if_keyisindex,
       GotoIf(InstanceTypeEqual(var_instance_type.value(),
                                THIN_ONE_BYTE_STRING_TYPE),
              &if_thinstring);
+
+      // Check if the hash field encodes a string forwarding index.
+      GotoIf(IsEqualInWord32<Name::HashFieldTypeBits>(
+                 raw_hash_field, Name::HashFieldType::kForwardingIndex),
+             &if_forwarding_index);
+
       // Finally, check if |key| is internalized.
       STATIC_ASSERT(kNotInternalizedTag != 0);
       GotoIf(IsSetWord32(var_instance_type.value(), kIsNotInternalizedMask),
@@ -8102,6 +8110,21 @@ void CodeStubAssembler::TryToName(TNode<Object> key, Label* if_keyisindex,
             LoadObjectField<String>(CAST(key), ThinString::kActualOffset);
         Goto(if_keyisunique);
       }
+      BIND(&if_forwarding_index);
+      {
+        TNode<ExternalReference> function =
+            ExternalConstant(ExternalReference::string_from_forward_table());
+        const TNode<ExternalReference> isolate_ptr =
+            ExternalConstant(ExternalReference::isolate_address(isolate()));
+        TNode<Object> result = CAST(CallCFunction(
+            function, MachineType::AnyTagged(),
+            std::make_pair(MachineType::Pointer(), isolate_ptr),
+            std::make_pair(MachineType::Int32(),
+                           DecodeWord32<Name::HashBits>(raw_hash_field))));
+
+        *var_unique = CAST(result);
+        Goto(if_keyisunique);
+      }
 
       BIND(&if_has_cached_index);
       {

src/codegen/external-reference.cc

@@ -991,6 +991,8 @@ FUNCTION_REFERENCE(copy_typed_array_elements_to_typed_array,
 FUNCTION_REFERENCE(copy_typed_array_elements_slice, CopyTypedArrayElementsSlice)
 FUNCTION_REFERENCE(try_string_to_index_or_lookup_existing,
                    StringTable::TryStringToIndexOrLookupExisting)
+FUNCTION_REFERENCE(string_from_forward_table,
+                   StringForwardingTable::GetForwardStringAddress)
 FUNCTION_REFERENCE(string_to_array_index_function, String::ToArrayIndex)
 
 static Address LexicographicCompareWrapper(Isolate* isolate, Address smi_x,

src/codegen/external-reference.h

@@ -187,6 +187,7 @@ class StatsCounter;
   V(string_to_array_index_function, "String::ToArrayIndex")                    \
   V(try_string_to_index_or_lookup_existing,                                    \
     "try_string_to_index_or_lookup_existing")                                  \
+  V(string_from_forward_table, "string_from_forward_table")                    \
   IF_WASM(V, wasm_call_trap_callback_for_testing,                              \
           "wasm::call_trap_callback_for_testing")                              \
   IF_WASM(V, wasm_f32_ceil, "wasm::f32_ceil_wrapper")                          \

src/diagnostics/objects-debug.cc

@@ -335,8 +335,10 @@ void HeapObject::VerifyCodePointer(Isolate* isolate, Object p) {
 
 void Symbol::SymbolVerify(Isolate* isolate) {
   TorqueGeneratedClassVerifiers::SymbolVerify(*this, isolate);
-  CHECK(HasHashCode());
-  CHECK_GT(hash(), 0);
+  uint32_t hash;
+  const bool has_hash = TryGetHash(&hash);
+  CHECK(has_hash);
+  CHECK_GT(hash, 0);
   CHECK(description().IsUndefined(isolate) || description().IsString());
   CHECK_IMPLIES(IsPrivateName(), IsPrivate());
   CHECK_IMPLIES(IsPrivateBrand(), IsPrivateName());
@@ -1932,7 +1934,7 @@ class StringTableVerifier : public RootVisitor {
 
 void StringTable::VerifyIfOwnedBy(Isolate* isolate) {
   DCHECK_EQ(isolate->string_table(), this);
-  if (!isolate->OwnsStringTable()) return;
+  if (!isolate->OwnsStringTables()) return;
   StringTableVerifier verifier(isolate);
   IterateElements(&verifier);
 }
@@ -2061,13 +2063,14 @@ bool DescriptorArray::IsSortedNoDuplicates() {
   uint32_t current = 0;
   for (int i = 0; i < number_of_descriptors(); i++) {
     Name key = GetSortedKey(i);
-    CHECK(key.HasHashCode());
+    uint32_t hash;
+    const bool has_hash = key.TryGetHash(&hash);
+    CHECK(has_hash);
     if (key == current_key) {
       Print();
       return false;
     }
     current_key = key;
-    uint32_t hash = key.hash();
     if (hash < current) {
       Print();
       return false;
@@ -2085,8 +2088,9 @@ bool TransitionArray::IsSortedNoDuplicates() {
 
   for (int i = 0; i < number_of_transitions(); i++) {
     Name key = GetSortedKey(i);
-    CHECK(key.HasHashCode());
-    uint32_t hash = key.hash();
+    uint32_t hash;
+    const bool has_hash = key.TryGetHash(&hash);
+    CHECK(has_hash);
     PropertyKind kind = PropertyKind::kData;
     PropertyAttributes attributes = NONE;
     if (!TransitionsAccessor::IsSpecialTransition(key.GetReadOnlyRoots(),

src/execution/isolate.cc

@@ -3968,12 +3968,14 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
   heap_.SetUpSpaces(&isolate_data_.new_allocation_info_,
                     &isolate_data_.old_allocation_info_);
 
-  if (OwnsStringTable()) {
+  if (OwnsStringTables()) {
     string_table_ = std::make_shared<StringTable>(this);
+    string_forwarding_table_ = std::make_shared<StringForwardingTable>(this);
   } else {
     // Only refer to shared string table after attaching to the shared isolate.
     DCHECK_NOT_NULL(shared_isolate());
     string_table_ = shared_isolate()->string_table_;
+    string_forwarding_table_ = shared_isolate()->string_forwarding_table_;
   }
 
   if (V8_SHORT_BUILTIN_CALLS_BOOL && FLAG_short_builtin_calls) {

src/execution/isolate.h

@@ -128,6 +128,7 @@ class RootVisitor;
 class SetupIsolateDelegate;
 class Simulator;
 class SnapshotData;
+class StringForwardingTable;
 class StringTable;
 class StubCache;
 class ThreadManager;
@@ -729,6 +730,9 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
 
   // The isolate's string table.
   StringTable* string_table() const { return string_table_.get(); }
+  StringForwardingTable* string_forwarding_table() const {
+    return string_forwarding_table_.get();
+  }
 
   Address get_address_from_id(IsolateAddressId id);
 
@@ -1935,7 +1939,7 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
 
   GlobalSafepoint* global_safepoint() const { return global_safepoint_.get(); }
 
-  bool OwnsStringTable() { return !FLAG_shared_string_table || is_shared(); }
+  bool OwnsStringTables() { return !FLAG_shared_string_table || is_shared(); }
 
 #if USE_SIMULATOR
   SimulatorData* simulator_data() { return simulator_data_; }
@@ -2080,6 +2084,7 @@ class V8_EXPORT_PRIVATE Isolate final : private HiddenFactory {
   ReadOnlyHeap* read_only_heap_ = nullptr;
   std::shared_ptr<ReadOnlyArtifacts> artifacts_;
   std::shared_ptr<StringTable> string_table_;
+  std::shared_ptr<StringForwardingTable> string_forwarding_table_;
 
   const int id_;
   EntryStackItem* entry_stack_ = nullptr;

src/heap/heap.cc

@@ -209,8 +209,8 @@ Heap::Heap()
       safepoint_(std::make_unique<IsolateSafepoint>(this)),
       external_string_table_(this),
       allocation_type_for_in_place_internalizable_strings_(
-          isolate()->OwnsStringTable() ? AllocationType::kOld
-                                       : AllocationType::kSharedOld),
+          isolate()->OwnsStringTables() ? AllocationType::kOld
+                                        : AllocationType::kSharedOld),
       collection_barrier_(new CollectionBarrier(this)) {
   // Ensure old_generation_size_ is a multiple of kPageSize.
   DCHECK_EQ(0, max_old_generation_size() & (Page::kPageSize - 1));
@@ -4470,7 +4470,7 @@ bool Heap::SharedHeapContains(HeapObject value) const {
 }
 
 bool Heap::ShouldBeInSharedOldSpace(HeapObject value) {
-  if (isolate()->OwnsStringTable()) return false;
+  if (isolate()->OwnsStringTables()) return false;
   if (ReadOnlyHeap::Contains(value)) return false;
   if (Heap::InYoungGeneration(value)) return false;
   if (value.IsExternalString()) return false;
@@ -4825,7 +4825,7 @@ void Heap::IterateWeakRoots(RootVisitor* v, base::EnumSet<SkipRoot> options) {
 
   if (!options.contains(SkipRoot::kOldGeneration) &&
       !options.contains(SkipRoot::kUnserializable) &&
-      isolate()->OwnsStringTable()) {
+      isolate()->OwnsStringTables()) {
     // Do not visit for the following reasons.
     // - Serialization, since the string table is custom serialized.
     // - If we are skipping old generation, since all internalized strings

src/heap/mark-compact.cc

@@ -2486,7 +2486,7 @@ void MarkCompactCollector::MarkLiveObjects() {
 void MarkCompactCollector::ClearNonLiveReferences() {
   TRACE_GC(heap()->tracer(), GCTracer::Scope::MC_CLEAR);
 
-  if (isolate()->OwnsStringTable()) {
+  if (isolate()->OwnsStringTables()) {
     TRACE_GC(heap()->tracer(), GCTracer::Scope::MC_CLEAR_STRING_TABLE);
 
     // Prune the string table removing all strings only pointed to by the
@@ -2497,6 +2497,18 @@ void MarkCompactCollector::ClearNonLiveReferences() {
     string_table->DropOldData();
     string_table->IterateElements(&internalized_visitor);
     string_table->NotifyElementsRemoved(internalized_visitor.PointersRemoved());
+
+    // Clear string forwarding table. Live strings are transitioned to
+    // ThinStrings in the cleanup process.
+    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);
   }
 
   ExternalStringTableCleaner external_visitor(heap());

src/ic/stub-cache.cc

@@ -31,8 +31,8 @@ void StubCache::Initialize() {
 // is scaled by 1 << kCacheIndexShift.
 int StubCache::PrimaryOffset(Name name, Map map) {
   // Compute the hash of the name (use entire hash field).
-  DCHECK(name.HasHashCode());
   uint32_t field = name.raw_hash_field();
+  DCHECK(Name::IsHashFieldComputed(field));
   // Using only the low bits in 64-bit mode is unlikely to increase the
   // risk of collision even if the heap is spread over an area larger than
   // 4Gb (and not at all if it isn't).

src/objects/name-inl.h

@@ -108,6 +108,9 @@ uint32_t Name::CreateHashFieldValue(uint32_t hash, HashFieldType type) {
 }
 
 bool Name::HasHashCode() const { return IsHashFieldComputed(raw_hash_field()); }
+bool Name::HasForwardingIndex() const {
+  return IsForwardingIndex(raw_hash_field(kAcquireLoad));
+}
 
 uint32_t Name::EnsureHash() {
   // Fast case: has hash code already been computed?
@@ -125,12 +128,36 @@ uint32_t Name::EnsureHash(const SharedStringAccessGuardIfNeeded& access_guard) {
   return String::cast(*this).ComputeAndSetHash(access_guard);
 }
 
+void Name::set_raw_hash_field_if_empty(uint32_t hash) {
+  uint32_t result = base::AsAtomic32::Release_CompareAndSwap(
+      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.
+  DCHECK_IMPLIES(result != kEmptyHashField,
+                 String::cast(*this).IsShared() &&
+                     (result == hash || IsForwardingIndex(hash)));
+}
+
 uint32_t Name::hash() const {
   uint32_t field = raw_hash_field();
   DCHECK(IsHashFieldComputed(field));
   return HashBits::decode(field);
 }
 
+bool Name::TryGetHash(uint32_t* hash) const {
+  uint32_t field = raw_hash_field();
+  if (IsHashFieldComputed(field)) {
+    *hash = HashBits::decode(field);
+    return true;
+  }
+  return false;
+}
+
 DEF_GETTER(Name, IsInterestingSymbol, bool) {
   return IsSymbol(cage_base) && Symbol::cast(*this).is_interesting_symbol();
 }

src/objects/name.h

@@ -25,7 +25,12 @@ class SharedStringAccessGuardIfNeeded;
 class Name : public TorqueGeneratedName<Name, PrimitiveHeapObject> {
  public:
   // Tells whether the hash code has been computed.
+  // Note: Use TryGetHash() whenever you want to use the hash, instead of a
+  // combination of HashHashCode() and hash() for thread-safety.
   inline bool HasHashCode() const;
+  // Tells whether the name contains a forwarding index pointing to a row
+  // in the string forwarding table.
+  inline bool HasForwardingIndex() const;
 
   // Returns a hash value used for the property table. Ensures that the hash
   // value is computed.
@@ -39,14 +44,29 @@ class Name : public TorqueGeneratedName<Name, PrimitiveHeapObject> {
     return RELAXED_READ_UINT32_FIELD(*this, kRawHashFieldOffset);
   }
 
+  inline uint32_t raw_hash_field(AcquireLoadTag) const {
+    return ACQUIRE_READ_UINT32_FIELD(*this, kRawHashFieldOffset);
+  }
+
   inline void set_raw_hash_field(uint32_t hash) {
     RELAXED_WRITE_UINT32_FIELD(*this, kRawHashFieldOffset, hash);
   }
 
+  inline void set_raw_hash_field(uint32_t hash, ReleaseStoreTag) {
+    RELEASE_WRITE_UINT32_FIELD(*this, kRawHashFieldOffset, hash);
+  }
+
+  // Sets the hash field only if it is empty. Otherwise does nothing.
+  inline void set_raw_hash_field_if_empty(uint32_t hash);
+
   // Returns a hash value used for the property table (same as Hash()), assumes
   // the hash is already computed.
   inline uint32_t hash() const;
 
+  // Returns true if the hash has been computed, and sets the computed hash
+  // as out-parameter.
+  inline bool TryGetHash(uint32_t* hash) const;
+
   // Equality operations.
   inline bool Equals(Name other);
   inline static bool Equals(Isolate* isolate, Handle<Name> one,

src/objects/string-table-inl.h

@@ -24,6 +24,37 @@ uint32_t StringTableKey::hash() const {
   return Name::HashBits::decode(raw_hash_field_);
 }
 
+int StringForwardingTable::Size() { return next_free_index_; }
+
+// static
+uint32_t StringForwardingTable::BlockForIndex(int index,
+                                              uint32_t* index_in_block) {
+  DCHECK_GE(index, 0);
+  DCHECK_NOT_NULL(index_in_block);
+  // The block is the leftmost set bit of the index, corrected by the size
+  // of the first block.
+  const uint32_t block = kBitsPerInt -
+                         base::bits::CountLeadingZeros(
+                             static_cast<uint32_t>(index + kInitialBlockSize)) -
+                         kInitialBlockSizeHighestBit - 1;
+  *index_in_block = IndexInBlock(index, block);
+  return block;
+}
+
+// static
+uint32_t StringForwardingTable::IndexInBlock(int index, uint32_t block) {
+  DCHECK_GE(index, 0);
+  // Clear out the leftmost set bit (the block) to get the index within the
+  // block.
+  return static_cast<uint32_t>(index + kInitialBlockSize) &
+         ~(1u << (block + kInitialBlockSizeHighestBit));
+}
+
+// static
+uint32_t StringForwardingTable::CapacityForBlock(uint32_t block) {
+  return 1u << (block + kInitialBlockSizeHighestBit);
+}
+
 }  // namespace internal
 }  // namespace v8
 

src/objects/string-table.h

@@ -96,6 +96,71 @@ class V8_EXPORT_PRIVATE StringTable {
   Isolate* isolate_;
 };
 
+// Mapping from forwarding indices (stored in a string's hash field) to
+// internalized strings.
+// The table is organised in "blocks". As writes only append new entries, the
+// organisation in blocks allows lock-free writes. We need a lock only for
+// growing the table (adding more blocks). When the vector holding the blocks
+// needs to grow, we keep a copy of the old vector alive to allow concurrent
+// reads while the vector is relocated.
+class StringForwardingTable {
+ public:
+  // Capacity for the first block.
+  static constexpr int kInitialBlockSize = 16;
+  STATIC_ASSERT(base::bits::IsPowerOfTwo(kInitialBlockSize));
+  static constexpr int kInitialBlockSizeHighestBit =
+      kBitsPerInt - base::bits::CountLeadingZeros32(kInitialBlockSize) - 1;
+  // Initial capacity in the block vector.
+  static constexpr int kInitialBlockVectorCapacity = 4;
+
+  explicit StringForwardingTable(Isolate* isolate);
+  ~StringForwardingTable();
+
+  inline int Size();
+  // Returns the index of the added string pair.
+  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);
+
+ private:
+  class Block;
+  class BlockVector;
+
+  // 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);
+  static inline uint32_t IndexInBlock(int index, uint32_t block);
+  static inline uint32_t CapacityForBlock(uint32_t block);
+
+  void InitializeBlockVector();
+  // Ensure that |block| exists in the BlockVector already. If not, a new block
+  // is created (with capacity double the capacity of the last block) and
+  // 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_;
+  // We need a vector of BlockVectors to keep old BlockVectors alive when we
+  // grow the table, due to concurrent reads that may still hold a pointer to
+  // them. |block_vector_sotrage_| is only accessed while we grow with the mutex
+  // held. All regular access go through |block_|, which holds a pointer to the
+  // current BlockVector.
+  std::vector<std::unique_ptr<BlockVector>> block_vector_storage_;
+  std::atomic<int> next_free_index_;
+  base::Mutex grow_mutex_;
+};
+
 }  // namespace internal
 }  // namespace v8
 

src/objects/string.cc

@@ -308,17 +308,15 @@ void String::MakeThin(IsolateT* isolate, String internalized) {
   Map initial_map = this->map(kAcquireLoad);
   StringShape initial_shape(initial_map);
 
-  // TODO(v8:12007): Support shared ThinStrings.
-  //
-  // Currently in-place migrations to ThinStrings are disabled for shared
-  // strings to unblock prototyping.
-  if (initial_shape.IsShared()) return;
   DCHECK(!initial_shape.IsThin());
+  DCHECK_IMPLIES(initial_shape.IsShared(), HasForwardingIndex());
 
   bool has_pointers = initial_shape.IsIndirect();
   int old_size = this->SizeFromMap(initial_map);
   Map target_map = ComputeThinStringMap(isolate, initial_shape,
                                         internalized.IsOneByteRepresentation());
+  // TODO(pthier): We don't need to migrate under lock anymore, as shared
+  // strings are only transitioned during stop-the-world GC.
   switch (MigrateStringMapUnderLockIfNeeded(
       isolate, *this, initial_map, target_map,
       [=](IsolateT* isolate, String string, StringShape initial_shape) {
@@ -734,7 +732,7 @@ Handle<Object> String::ToNumber(Isolate* isolate, Handle<String> subject) {
         subject->EnsureHash();  // Force hash calculation.
         DCHECK_EQ(subject->raw_hash_field(), raw_hash_field);
 #endif
-        subject->set_raw_hash_field(raw_hash_field);
+        subject->set_raw_hash_field_if_empty(raw_hash_field);
       }
       return handle(Smi::FromInt(d), isolate);
     }
@@ -1035,10 +1033,12 @@ bool String::SlowEquals(
 
   // Fast check: if hash code is computed for both strings
   // a fast negative check can be performed.
-  if (HasHashCode() && other.HasHashCode()) {
+  uint32_t this_hash;
+  uint32_t other_hash;
+  if (TryGetHash(&this_hash) && other.TryGetHash(&other_hash)) {
 #ifdef ENABLE_SLOW_DCHECKS
     if (FLAG_enable_slow_asserts) {
-      if (hash() != other.hash()) {
+      if (this_hash != other_hash) {
         bool found_difference = false;
         for (int i = 0; i < len; i++) {
           if (Get(i) != other.Get(i)) {
@@ -1050,7 +1050,7 @@ bool String::SlowEquals(
       }
     }
 #endif
-    if (hash() != other.hash()) return false;
+    if (this_hash != other_hash) return false;
   }
 
   // We know the strings are both non-empty. Compare the first chars
@@ -1093,10 +1093,12 @@ bool String::SlowEquals(Isolate* isolate, Handle<String> one,
 
   // Fast check: if hash code is computed for both strings
   // a fast negative check can be performed.
-  if (one->HasHashCode() && two->HasHashCode()) {
+  uint32_t one_hash;
+  uint32_t two_hash;
+  if (one->TryGetHash(&one_hash) && two->TryGetHash(&two_hash)) {
 #ifdef ENABLE_SLOW_DCHECKS
     if (FLAG_enable_slow_asserts) {
-      if (one->hash() != two->hash()) {
+      if (one_hash != two_hash) {
         bool found_difference = false;
         for (int i = 0; i < one_length; i++) {
           if (one->Get(i) != two->Get(i)) {
@@ -1108,7 +1110,7 @@ bool String::SlowEquals(Isolate* isolate, Handle<String> one,
       }
     }
 #endif
-    if (one->hash() != two->hash()) return false;
+    if (one_hash != two_hash) return false;
   }
 
   // We know the strings are both non-empty. Compare the first chars
@@ -1631,6 +1633,18 @@ uint32_t String::ComputeAndSetHash(
   // same. The raw hash field is stored with relaxed ordering.
   DCHECK_IMPLIES(!FLAG_shared_string_table, !HasHashCode());
 
+  uint32_t field = raw_hash_field(kAcquireLoad);
+  if (Name::IsForwardingIndex(field)) {
+    // Get the real hash from the forwarded string.
+    Isolate* isolate = GetIsolateFromWritableObject(*this);
+    const int forward_index = Name::HashBits::decode(field);
+    String internalized = isolate->string_forwarding_table()->GetForwardString(
+        isolate, forward_index);
+    uint32_t hash = internalized.raw_hash_field();
+    DCHECK(IsHashFieldComputed(hash));
+    return HashBits::decode(hash);
+  }
+
   // Store the hash code in the object.
   uint64_t seed = HashSeed(GetReadOnlyRoots());
   size_t start = 0;
@@ -1661,10 +1675,11 @@ uint32_t String::ComputeAndSetHash(
                                 access_guard)
           : HashString<uint16_t>(string, start, length(), seed, cage_base,
                                  access_guard);
-  set_raw_hash_field(raw_hash_field);
+  set_raw_hash_field_if_empty(raw_hash_field);
 
-  // Check the hash code is there.
-  DCHECK(HasHashCode());
+  // Check the hash code is there (or a forwarding index if the string was
+  // internalized in parallel).
+  DCHECK(HasHashCode() || HasForwardingIndex());
   uint32_t result = HashBits::decode(raw_hash_field);
   DCHECK_NE(result, 0);  // Ensure that the hash value of 0 is never computed.
   return result;

src/snapshot/deserializer.h

@@ -289,7 +289,7 @@ class StringTableInsertionKey final : public StringTableKey {
   void PrepareForInsertion(Isolate* isolate) {
     // When sharing the string table, all string table lookups during snapshot
     // deserialization are hits.
-    DCHECK(isolate->OwnsStringTable() ||
+    DCHECK(isolate->OwnsStringTables() ||
            deserializing_user_code_ ==
                DeserializingUserCodeOption::kIsDeserializingUserCode);
   }

src/snapshot/shared-heap-deserializer.cc

@@ -32,7 +32,7 @@ void SharedHeapDeserializer::DeserializeIntoIsolate() {
 void SharedHeapDeserializer::DeserializeStringTable() {
   // See SharedHeapSerializer::SerializeStringTable.
 
-  DCHECK(isolate()->OwnsStringTable());
+  DCHECK(isolate()->OwnsStringTables());
 
   // Get the string table size.
   int string_table_size = source()->GetInt();

test/cctest/test-shared-strings.cc

@@ -242,7 +242,7 @@ class ConcurrentStringThreadBase : public v8::base::Thread {
                              base::Semaphore* sema_ready,
                              base::Semaphore* sema_execute_start,
                              base::Semaphore* sema_execute_complete)
-      : v8::base::Thread(base::Thread::Options(name)),  // typeid(this).name?
+      : v8::base::Thread(base::Thread::Options(name)),
         test_(test),
         shared_strings_(shared_strings),
         sema_ready_(sema_ready),
@@ -305,15 +305,11 @@ class ConcurrentInternalizationThread final
     CHECK(input_string->IsShared());
     Handle<String> interned = factory->InternalizeString(input_string);
     CHECK(interned->IsShared());
+    CHECK(interned->IsInternalizedString());
     if (hit_or_miss_ == kTestMiss) {
       CHECK_EQ(*input_string, *interned);
     } else {
-      // TODO(v8:12007): In-place internalization currently do not migrate
-      // shared strings to ThinStrings. This is triviailly threadsafe for
-      // character access but bad for performance, as run-time
-      // internalizations do not speed up comparisons for shared strings.
-      CHECK(!input_string->IsThinString());
-      CHECK_NE(*input_string, *interned);
+      CHECK(input_string->HasForwardingIndex());
       CHECK(String::Equals(i_isolate, input_string, interned));
     }
   }
@@ -701,6 +697,53 @@ UNINITIALIZED_TEST(PromotionScavenge) {
   }
 }
 
+UNINITIALIZED_TEST(SharedStringsTransitionDuringGC) {
+  if (!ReadOnlyHeap::IsReadOnlySpaceShared()) return;
+  if (!COMPRESS_POINTERS_IN_SHARED_CAGE_BOOL) return;
+
+  FLAG_shared_string_table = true;
+
+  MultiClientIsolateTest test;
+
+  constexpr int kStrings = 4096;
+
+  v8::Isolate* isolate = test.NewClientIsolate();
+  Isolate* i_isolate = reinterpret_cast<Isolate*>(isolate);
+  Factory* factory = i_isolate->factory();
+
+  HandleScope scope(i_isolate);
+
+  // Run two times to test that everything is reset correctly during GC.
+  for (int run = 0; run < 2; run++) {
+    Handle<FixedArray> shared_strings =
+        CreateSharedOneByteStrings(i_isolate, factory, kStrings, run == 0);
+
+    // Check strings are in the forwarding table after internalization.
+    for (int i = 0; i < shared_strings->length(); i++) {
+      Handle<String> input_string(String::cast(shared_strings->get(i)),
+                                  i_isolate);
+      Handle<String> interned = factory->InternalizeString(input_string);
+      CHECK(input_string->IsShared());
+      CHECK(!input_string->IsThinString());
+      CHECK(input_string->HasForwardingIndex());
+      CHECK(String::Equals(i_isolate, input_string, interned));
+    }
+
+    // Trigger garbage collection on the shared isolate.
+    i_isolate->heap()->CollectSharedGarbage(GarbageCollectionReason::kTesting);
+
+    // Check that GC cleared the forwarding table.
+    CHECK_EQ(i_isolate->string_forwarding_table()->Size(), 0);
+
+    // Check all strings are transitioned to ThinStrings
+    for (int i = 0; i < shared_strings->length(); i++) {
+      Handle<String> input_string(String::cast(shared_strings->get(i)),
+                                  i_isolate);
+      CHECK(input_string->IsThinString());
+    }
+  }
+}
+
 }  // namespace test_shared_strings
 }  // namespace internal
 }  // namespace v8