[string] Make String internalization threadsafe for shared strings

This CL fixes 2 issues with string internalization when the string table
is shared:
1. In-place migration of a string's map to Internalized was done before
   it was sure that the string is going to be internalized (outside the
   critical section). To fix this problem StringTableKey::AsHandle() is
   now split into StringTableKey::PrepareForInsertion(), which is
   invoked outside the critical section and creates a copy if
   necessary, and StringTableKey::GetHandleForInsertion(), which is
   invoked inside the critical section only for string table misses.
   Migration of the map is handled by this method.
2. TryStringToIndexOrLookupExisting() didn't handle already internalized
   strings. So far this was impossible, as this method was only invoked
   for strings that were checked not to be internalized. However with
   a shared string table, the string could be internalized after the
   checks.

Bug: v8:12007
Change-Id: I193d6b54dc41360eee47d21cbcaa36d2652d85dd
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3368103Reviewed-by: Shu-yu Guo <syg@chromium.org>
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Patrick Thier <pthier@chromium.org>
Cr-Commit-Position: refs/heads/main@{#78600}

src/heap/factory.cc

@@ -876,10 +876,6 @@ Handle<String> Factory::NewInternalizedStringImpl(Handle<String> string,
   return AllocateInternalizedStringImpl<false>(string, chars, hash_field);
 }
 
-namespace {
-
-}  // namespace
-
 StringTransitionStrategy Factory::ComputeInternalizationStrategyForString(
     Handle<String> string, MaybeHandle<Map>* internalized_map) {
   // Do not internalize young strings in-place: This allows us to ignore both

src/objects/string-inl.h

@@ -190,9 +190,12 @@ bool StringShape::CanMigrateInParallel() const {
       // Shared ThinStrings do not migrate.
       return false;
     default:
+      // TODO(v8:12007): Set is_shared to true on internalized string when
+      // FLAG_shared_string_table is removed.
+      //
       // If you crashed here, you probably added a new shared string
       // type. Explicitly handle all shared string cases above.
-      DCHECK(!IsShared());
+      DCHECK((FLAG_shared_string_table && IsInternalized()) || !IsShared());
       return false;
   }
 }
@@ -372,27 +375,26 @@ class SequentialStringKey final : public StringTableKey {
     return s.IsEqualTo<String::EqualityType::kNoLengthCheck>(chars_, isolate);
   }
 
-  Handle<String> AsHandle(Isolate* isolate) {
+  template <typename IsolateT>
+  void PrepareForInsertion(IsolateT* isolate) {
     if (sizeof(Char) == 1) {
-      return isolate->factory()->NewOneByteInternalizedString(
+      internalized_string_ = isolate->factory()->NewOneByteInternalizedString(
           base::Vector<const uint8_t>::cast(chars_), raw_hash_field());
+    } else {
+      internalized_string_ = isolate->factory()->NewTwoByteInternalizedString(
+          base::Vector<const uint16_t>::cast(chars_), raw_hash_field());
     }
-    return isolate->factory()->NewTwoByteInternalizedString(
-        base::Vector<const uint16_t>::cast(chars_), raw_hash_field());
   }
 
-  Handle<String> AsHandle(LocalIsolate* isolate) {
-    if (sizeof(Char) == 1) {
-      return isolate->factory()->NewOneByteInternalizedString(
-          base::Vector<const uint8_t>::cast(chars_), raw_hash_field());
-    }
-    return isolate->factory()->NewTwoByteInternalizedString(
-        base::Vector<const uint16_t>::cast(chars_), raw_hash_field());
+  Handle<String> GetHandleForInsertion() {
+    DCHECK(!internalized_string_.is_null());
+    return internalized_string_;
   }
 
  private:
   base::Vector<const Char> chars_;
   bool convert_;
+  Handle<String> internalized_string_;
 };
 
 using OneByteStringKey = SequentialStringKey<uint8_t>;
@@ -440,7 +442,7 @@ class SeqSubStringKey final : public StringTableKey {
         isolate);
   }
 
-  Handle<String> AsHandle(Isolate* isolate) {
+  void PrepareForInsertion(Isolate* isolate) {
     if (sizeof(Char) == 1 || (sizeof(Char) == 2 && convert_)) {
       Handle<SeqOneByteString> result =
           isolate->factory()->AllocateRawOneByteInternalizedString(
@@ -448,7 +450,7 @@ class SeqSubStringKey final : public StringTableKey {
       DisallowGarbageCollection no_gc;
       CopyChars(result->GetChars(no_gc), string_->GetChars(no_gc) + from_,
                 length());
-      return result;
+      internalized_string_ = result;
     }
     Handle<SeqTwoByteString> result =
         isolate->factory()->AllocateRawTwoByteInternalizedString(
@@ -456,13 +458,19 @@ class SeqSubStringKey final : public StringTableKey {
     DisallowGarbageCollection no_gc;
     CopyChars(result->GetChars(no_gc), string_->GetChars(no_gc) + from_,
               length());
-    return result;
+    internalized_string_ = result;
+  }
+
+  Handle<String> GetHandleForInsertion() {
+    DCHECK(!internalized_string_.is_null());
+    return internalized_string_;
   }
 
  private:
   Handle<typename CharTraits<Char>::String> string_;
   int from_;
   bool convert_;
+  Handle<String> internalized_string_;
 };
 
 using SeqOneByteSubStringKey = SeqSubStringKey<SeqOneByteString>;
@@ -633,6 +641,7 @@ const Char* String::GetChars(
                                                               access_guard);
 }
 
+// static
 Handle<String> String::Flatten(Isolate* isolate, Handle<String> string,
                                AllocationType allocation) {
   DisallowGarbageCollection no_gc;  // Unhandlified code.
@@ -662,6 +671,7 @@ Handle<String> String::Flatten(Isolate* isolate, Handle<String> string,
   return handle(s, isolate);
 }
 
+// static
 Handle<String> String::Flatten(LocalIsolate* isolate, Handle<String> string,
                                AllocationType allocation) {
   // We should never pass non-flat strings to String::Flatten when off-thread.

src/objects/string-table.cc

@@ -361,55 +361,73 @@ class InternalizedStringKey final : public StringTableKey {
     return string_->SlowEquals(string);
   }
 
-  Handle<String> AsHandle(Isolate* isolate) {
-    // Internalize the string in-place if possible.
-    MaybeHandle<Map> maybe_internalized_map;
+  void PrepareForInsertion(Isolate* isolate) {
     StringTransitionStrategy strategy =
         isolate->factory()->ComputeInternalizationStrategyForString(
-            string_, &maybe_internalized_map);
+            string_, &maybe_internalized_map_);
     switch (strategy) {
       case StringTransitionStrategy::kCopy:
         break;
       case StringTransitionStrategy::kInPlace:
-        // A relaxed write is sufficient here even with concurrent
-        // internalization. Though it is not synchronizing, a thread that does
-        // not see the relaxed write will wait on the string table write
-        // mutex. When that thread acquires that mutex, the ordering of the
-        // mutex's underlying memory access will force this map update to become
-        // visible to it.
-        string_->set_map_no_write_barrier(
-            *maybe_internalized_map.ToHandleChecked());
-        DCHECK(string_->IsInternalizedString());
-        return string_;
+        // In-place transition will be done in GetHandleForInsertion, when we
+        // are sure that we are going to insert the string into the table.
+        return;
       case StringTransitionStrategy::kAlreadyTransitioned:
         // We can see already internalized strings here only when sharing the
         // string table and allowing concurrent internalization.
         DCHECK(FLAG_shared_string_table);
-        return string_;
+        return;
     }
 
+    // Copying the string here is always threadsafe, as no instance type
+    // requiring a copy can transition any further.
+    StringShape shape(*string_);
     // External strings get special treatment, to avoid copying their
     // contents as long as they are not uncached.
-    StringShape shape(*string_);
     if (shape.IsExternalOneByte() && !shape.IsUncachedExternal()) {
       // TODO(syg): External strings not yet supported.
       DCHECK(!FLAG_shared_string_table);
-      return isolate->factory()
-          ->InternalizeExternalString<ExternalOneByteString>(string_);
+      string_ =
+          isolate->factory()->InternalizeExternalString<ExternalOneByteString>(
+              string_);
     } else if (shape.IsExternalTwoByte() && !shape.IsUncachedExternal()) {
       // TODO(syg): External strings not yet supported.
       DCHECK(!FLAG_shared_string_table);
-      return isolate->factory()
-          ->InternalizeExternalString<ExternalTwoByteString>(string_);
+      string_ =
+          isolate->factory()->InternalizeExternalString<ExternalTwoByteString>(
+              string_);
     } else {
       // Otherwise allocate a new internalized string.
-      return isolate->factory()->NewInternalizedStringImpl(
+      string_ = isolate->factory()->NewInternalizedStringImpl(
           string_, string_->length(), string_->raw_hash_field());
     }
   }
 
+  Handle<String> GetHandleForInsertion() {
+    Handle<Map> internalized_map;
+    // When preparing the string, the strategy was to in-place migrate it.
+    if (maybe_internalized_map_.ToHandle(&internalized_map)) {
+      // It is always safe to overwrite the map. The only transition possible
+      // is another thread migrated the string to internalized already.
+      // Migrations to thin are impossible, as we only call this method on table
+      // misses inside the critical section.
+      string_->set_map_no_write_barrier(*internalized_map);
+      DCHECK(string_->IsInternalizedString());
+      return string_;
+    }
+    // We prepared an internalized copy for the string or the string was already
+    // internalized.
+    // In theory we could have created a copy of a SeqString in young generation
+    // that has been promoted to old space by now. In that case we could
+    // in-place migrate the original string instead of internalizing the copy
+    // and migrating the original string to a ThinString. This scenario doesn't
+    // seem to be common enough to justify re-computing the strategy here.
+    return string_;
+  }
+
  private:
   Handle<String> string_;
+  MaybeHandle<Map> maybe_internalized_map_;
 };
 
 Handle<String> StringTable::LookupString(Isolate* isolate,
@@ -510,14 +528,7 @@ Handle<String> StringTable::LookupKey(IsolateT* isolate, StringTableKey* key) {
   }
 
   // No entry found, so adding new string.
-
-  // Allocate the string before the first insertion attempt, reuse this
-  // allocated value on insertion retries. If another thread concurrently
-  // allocates the same string, the insert will fail, the lookup above will
-  // succeed, and this string will be discarded.
-  Handle<String> new_string = key->AsHandle(isolate);
-  DCHECK_IMPLIES(FLAG_shared_string_table, new_string->IsShared());
-
+  key->PrepareForInsertion(isolate);
   {
     base::MutexGuard table_write_guard(&write_mutex_);
 
@@ -531,12 +542,16 @@ Handle<String> StringTable::LookupKey(IsolateT* isolate, StringTableKey* key) {
     if (element == empty_element()) {
       // This entry is empty, so write it and register that we added an
       // element.
+      Handle<String> new_string = key->GetHandleForInsertion();
+      DCHECK_IMPLIES(FLAG_shared_string_table, new_string->IsShared());
       data->Set(entry, *new_string);
       data->ElementAdded();
       return new_string;
     } else if (element == deleted_element()) {
       // This entry was deleted, so overwrite it and register that we
       // overwrote a deleted element.
+      Handle<String> new_string = key->GetHandleForInsertion();
+      DCHECK_IMPLIES(FLAG_shared_string_table, new_string->IsShared());
       data->Set(entry, *new_string);
       data->DeletedElementOverwritten();
       return new_string;
@@ -663,7 +678,15 @@ Address StringTable::Data::TryStringToIndexOrLookupExisting(Isolate* isolate,
   }
 
   String internalized = String::cast(string_table_data->Get(isolate, entry));
-  string.MakeThin(isolate, internalized);
+  // string can be internalized here, if another thread internalized it.
+  // If we found and entry in the string table and string is not internalized,
+  // there is no way that it can transition to internalized later on. So a last
+  // check here is sufficient.
+  if (!string.IsInternalizedString()) {
+    string.MakeThin(isolate, internalized);
+  } else {
+    DCHECK(FLAG_shared_string_table);
+  }
   return internalized.ptr();
 }
 
@@ -671,7 +694,12 @@ Address StringTable::Data::TryStringToIndexOrLookupExisting(Isolate* isolate,
 Address StringTable::TryStringToIndexOrLookupExisting(Isolate* isolate,
                                                       Address raw_string) {
   String string = String::cast(Object(raw_string));
-  DCHECK(!string.IsInternalizedString());
+  if (string.IsInternalizedString()) {
+    // string could be internalized, if the string table is shared and another
+    // thread internalized it.
+    DCHECK(FLAG_shared_string_table);
+    return raw_string;
+  }
 
   // Valid array indices are >= 0, so they cannot be mixed up with any of
   // the result sentinels, which are negative.

src/snapshot/deserializer.h

@@ -280,15 +280,15 @@ class StringTableInsertionKey final : public StringTableKey {
   template <typename IsolateT>
   bool IsMatch(IsolateT* isolate, String string);
 
-  V8_WARN_UNUSED_RESULT Handle<String> AsHandle(Isolate* isolate) {
+  void PrepareForInsertion(Isolate* isolate) {
     // When sharing the string table, all string table lookups during snapshot
     // deserialization are hits.
     DCHECK(isolate->OwnsStringTable() ||
            deserializing_user_code_ ==
                DeserializingUserCodeOption::kIsDeserializingUserCode);
-    return string_;
   }
-  V8_WARN_UNUSED_RESULT Handle<String> AsHandle(LocalIsolate* isolate) {
+  void PrepareForInsertion(LocalIsolate* isolate) {}
+  V8_WARN_UNUSED_RESULT Handle<String> GetHandleForInsertion() {
     return string_;
   }