[offthread] Fix StringTable/WriteToFlat performance

Pass the Isolate/LocalIsolate through to StringTable matchers and
WriteToFlat, so avoid having to get the Isolate via the String, and to
avoid locking on the main thread entirely. This allows us to remove the
String overload of the SharedStringAccessGuardIfNeeded constructor
entirely, to avoid this anti-pattern in the future.

Bug: chromium:1146972
Change-Id: I53bba126b105e1c9629d6e64d8bb574e62e3ad45
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2557988
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Reviewed-by: Santiago Aboy Solanes <solanes@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71398}

src/compiler/js-heap-broker.cc

@@ -862,7 +862,7 @@ StringData::StringData(JSHeapBroker* broker, ObjectData** storage,
     : NameData(broker, storage, object),
       length_(object->length()),
       first_char_(length_ > 0 ? object->Get(0) : 0),
-      to_number_(TryStringToDouble(object)),
+      to_number_(TryStringToDouble(broker->local_isolate(), object)),
       is_external_string_(object->IsExternalString()),
       is_seq_string_(object->IsSeqString()),
       chars_as_strings_(broker->zone()) {}
@@ -3223,7 +3223,7 @@ base::Optional<double> StringRef::ToNumber() {
                                                             broker()->mode());
     AllowHandleAllocationIfNeeded allow_handle_allocation(data()->kind(),
                                                           broker()->mode());
-    return TryStringToDouble(object());
+    return TryStringToDouble(broker()->local_isolate(), object());
   }
   return data()->AsString()->to_number();
 }

src/heap/factory-base.cc

@@ -596,18 +596,16 @@ MaybeHandle<String> FactoryBase<Impl>::NewConsString(
       Handle<SeqOneByteString> result =
           NewRawOneByteString(length, allocation).ToHandleChecked();
       DisallowGarbageCollection no_gc;
-      uint8_t* dest =
-          result->GetChars(no_gc, SharedStringAccessGuardIfNeeded::NotNeeded());
+      SharedStringAccessGuardIfNeeded access_guard(isolate());
+      uint8_t* dest = result->GetChars(no_gc, access_guard);
       // Copy left part.
       {
-        SharedStringAccessGuardIfNeeded access_guard(*left);
         const uint8_t* src =
             left->template GetChars<uint8_t>(no_gc, access_guard);
         CopyChars(dest, src, left_length);
       }
       // Copy right part.
       {
-        SharedStringAccessGuardIfNeeded access_guard(*right);
         const uint8_t* src =
             right->template GetChars<uint8_t>(no_gc, access_guard);
         CopyChars(dest + left_length, src, right_length);
@@ -619,10 +617,11 @@ MaybeHandle<String> FactoryBase<Impl>::NewConsString(
         NewRawTwoByteString(length, allocation).ToHandleChecked();
 
     DisallowGarbageCollection no_gc;
-    uc16* sink =
-        result->GetChars(no_gc, SharedStringAccessGuardIfNeeded::NotNeeded());
-    String::WriteToFlat(*left, sink, 0, left->length());
-    String::WriteToFlat(*right, sink + left->length(), 0, right->length());
+    SharedStringAccessGuardIfNeeded access_guard(isolate());
+    uc16* sink = result->GetChars(no_gc, access_guard);
+    String::WriteToFlat(*left, sink, 0, left->length(), access_guard);
+    String::WriteToFlat(*right, sink + left->length(), 0, right->length(),
+                        access_guard);
     return result;
   }
 

src/numbers/conversions.cc

@@ -17,6 +17,7 @@
 #include "src/numbers/strtod.h"
 #include "src/objects/bigint.h"
 #include "src/objects/objects-inl.h"
+#include "src/objects/string-inl.h"
 #include "src/strings/char-predicates-inl.h"
 #include "src/utils/allocation.h"
 #include "src/utils/utils.h"
@@ -1369,7 +1370,8 @@ double StringToDouble(Isolate* isolate, Handle<String> string, int flags,
   }
 }
 
-base::Optional<double> TryStringToDouble(Handle<String> object,
+base::Optional<double> TryStringToDouble(LocalIsolate* isolate,
+                                         Handle<String> object,
                                          int max_length_for_conversion) {
   DisallowGarbageCollection no_gc;
   int length = object->length();
@@ -1379,7 +1381,8 @@ base::Optional<double> TryStringToDouble(Handle<String> object,
 
   const int flags = ALLOW_HEX | ALLOW_OCTAL | ALLOW_BINARY;
   auto buffer = std::make_unique<uc16[]>(max_length_for_conversion);
-  String::WriteToFlat(*object, buffer.get(), 0, length);
+  SharedStringAccessGuardIfNeeded access_guard(isolate);
+  String::WriteToFlat(*object, buffer.get(), 0, length, access_guard);
   Vector<const uc16> v(buffer.get(), length);
   return StringToDouble(v, flags);
 }

src/numbers/conversions.h

@@ -166,7 +166,8 @@ double StringToDouble(Isolate* isolate, Handle<String> string, int flags,
 // {max_length_for_conversion}. 23 was chosen because any representable double
 // can be represented using a string of length 23.
 V8_EXPORT_PRIVATE base::Optional<double> TryStringToDouble(
-    Handle<String> object, int max_length_for_conversion = 23);
+    LocalIsolate* isolate, Handle<String> object,
+    int max_length_for_conversion = 23);
 
 inline bool TryNumberToSize(Object number, size_t* result);
 

src/objects/string-inl.h

@@ -5,6 +5,7 @@
 #ifndef V8_OBJECTS_STRING_INL_H_
 #define V8_OBJECTS_STRING_INL_H_
 
+#include "src/common/assert-scope.h"
 #include "src/common/external-pointer-inl.h"
 #include "src/common/external-pointer.h"
 #include "src/common/globals.h"
@@ -17,6 +18,7 @@
 #include "src/objects/string-table-inl.h"
 #include "src/objects/string.h"
 #include "src/strings/string-hasher-inl.h"
+#include "src/utils/utils.h"
 
 // Has to be the last include (doesn't have include guards):
 #include "src/objects/object-macros.h"
@@ -28,18 +30,14 @@ namespace internal {
 
 class V8_NODISCARD SharedStringAccessGuardIfNeeded {
  public:
-  // Creates a SharedMutexGuard<kShared> for the string access if:
-  // A) {str} is not a read only string, and
-  // B) We are on a background thread.
-  explicit SharedStringAccessGuardIfNeeded(String str) {
-    Isolate* isolate;
-    if (IsNeeded(str, &isolate)) mutex_guard.emplace(isolate->string_access());
-  }
+  // Creates no SharedMutexGuard<kShared> for the string access since it was
+  // called from the main thread.
+  explicit SharedStringAccessGuardIfNeeded(Isolate* isolate) {}
 
   // Creates a SharedMutexGuard<kShared> for the string access if it was called
   // from a background thread.
-  SharedStringAccessGuardIfNeeded(String str, LocalIsolate* local_isolate) {
-    if (IsNeeded(str, local_isolate)) {
+  explicit SharedStringAccessGuardIfNeeded(LocalIsolate* local_isolate) {
+    if (IsNeeded(local_isolate)) {
       mutex_guard.emplace(local_isolate->string_access());
     }
   }
@@ -48,7 +46,8 @@ class V8_NODISCARD SharedStringAccessGuardIfNeeded {
     return SharedStringAccessGuardIfNeeded();
   }
 
-  static bool IsNeeded(String str, Isolate** out_isolate = nullptr) {
+#ifdef DEBUG
+  static bool IsNeeded(String str) {
     LocalHeap* local_heap = LocalHeap::Current();
     // Don't acquire the lock for the main thread.
     if (!local_heap || local_heap->is_main_thread()) return false;
@@ -59,12 +58,15 @@ class V8_NODISCARD SharedStringAccessGuardIfNeeded {
       DCHECK(ReadOnlyHeap::Contains(str));
       return false;
     }
-    if (out_isolate) *out_isolate = isolate;
     return true;
   }
+#endif
 
-  static bool IsNeeded(String str, LocalIsolate* local_isolate) {
-    return !local_isolate->heap()->is_main_thread();
+  static bool IsNeeded(Isolate* isolate) { return false; }
+
+  static bool IsNeeded(LocalIsolate* local_isolate) {
+    // TODO(leszeks): Remove the nullptr check for local_isolate.
+    return local_isolate && !local_isolate->heap()->is_main_thread();
   }
 
  private:
@@ -316,7 +318,10 @@ class SequentialStringKey final : public StringTableKey {
         chars_(chars),
         convert_(convert) {}
 
-  bool IsMatch(String s) override { return s.IsEqualTo(chars_); }
+  template <typename LocalIsolate>
+  bool IsMatch(LocalIsolate* isolate, String s) {
+    return s.IsEqualTo(isolate, chars_);
+  }
 
   Handle<String> AsHandle(Isolate* isolate) {
     if (sizeof(Char) == 1) {
@@ -377,14 +382,15 @@ class SeqSubStringKey final : public StringTableKey {
 #pragma warning(pop)
 #endif
 
-  bool IsMatch(String string) override {
+  bool IsMatch(Isolate* isolate, String string) {
+    DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(string));
+    DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*string_));
     DisallowGarbageCollection no_gc;
     return string.IsEqualTo(
         Vector<const Char>(string_->GetChars(no_gc) + from_, length()));
   }
 
-  template <typename LocalIsolate>
-  Handle<String> AsHandle(LocalIsolate* isolate) {
+  Handle<String> AsHandle(Isolate* isolate) {
     if (sizeof(Char) == 1 || (sizeof(Char) == 2 && convert_)) {
       Handle<SeqOneByteString> result =
           isolate->factory()->AllocateRawOneByteInternalizedString(
@@ -431,6 +437,22 @@ bool String::Equals(Isolate* isolate, Handle<String> one, Handle<String> two) {
 template <typename Char>
 bool String::IsEqualTo(Vector<const Char> str,
                        String::EqualityType eq_type) const {
+  DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*this));
+  return IsEqualToImpl(str, eq_type,
+                       SharedStringAccessGuardIfNeeded::NotNeeded());
+}
+
+template <typename Char>
+bool String::IsEqualTo(LocalIsolate* isolate, Vector<const Char> str,
+                       String::EqualityType eq_type) const {
+  SharedStringAccessGuardIfNeeded access_guard(isolate);
+  return IsEqualToImpl(str, eq_type, access_guard);
+}
+
+template <typename Char>
+bool String::IsEqualToImpl(
+    Vector<const Char> str, String::EqualityType eq_type,
+    const SharedStringAccessGuardIfNeeded& access_guard) const {
   size_t len = str.size();
   switch (eq_type) {
     case EqualityType::kWholeString:
@@ -441,7 +463,6 @@ bool String::IsEqualTo(Vector<const Char> str,
       break;
   }
 
-  SharedStringAccessGuardIfNeeded access_guard(*this);
   DisallowGarbageCollection no_gc;
 
   class IsEqualToDispatcher : public AllStatic {
@@ -504,6 +525,7 @@ bool String::IsOneByteEqualTo(Vector<const char> str) { return IsEqualTo(str); }
 
 template <typename Char>
 const Char* String::GetChars(const DisallowGarbageCollection& no_gc) {
+  DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*this));
   return StringShape(*this).IsExternal()
              ? CharTraits<Char>::ExternalString::cast(*this).GetChars()
              : CharTraits<Char>::String::cast(*this).GetChars(no_gc);
@@ -549,7 +571,7 @@ uint16_t String::Get(int index, Isolate* isolate) {
 }
 
 uint16_t String::Get(int index, LocalIsolate* local_isolate) {
-  SharedStringAccessGuardIfNeeded scope(*this, local_isolate);
+  SharedStringAccessGuardIfNeeded scope(local_isolate);
   return GetImpl(index);
 }
 

src/objects/string-table.cc

@@ -69,11 +69,11 @@ int ComputeStringTableCapacityWithShrink(int current_capacity,
   return new_capacity;
 }
 
-template <typename StringTableKey>
-bool KeyIsMatch(StringTableKey* key, String string) {
+template <typename LocalIsolate, typename StringTableKey>
+bool KeyIsMatch(LocalIsolate* isolate, StringTableKey* key, String string) {
   if (string.hash() != key->hash()) return false;
   if (string.length() != key->length()) return false;
-  return key->IsMatch(string);
+  return key->IsMatch(isolate, string);
 }
 
 }  // namespace
@@ -135,14 +135,14 @@ class StringTable::Data {
   int number_of_elements() const { return number_of_elements_; }
   int number_of_deleted_elements() const { return number_of_deleted_elements_; }
 
-  template <typename StringTableKey>
-  InternalIndex FindEntry(IsolateRoot isolate, StringTableKey* key,
+  template <typename LocalIsolate, typename StringTableKey>
+  InternalIndex FindEntry(LocalIsolate* isolate, StringTableKey* key,
                           uint32_t hash) const;
 
   InternalIndex FindInsertionEntry(IsolateRoot isolate, uint32_t hash) const;
 
-  template <typename StringTableKey>
-  InternalIndex FindEntryOrInsertionEntry(IsolateRoot isolate,
+  template <typename LocalIsolate, typename StringTableKey>
+  InternalIndex FindEntryOrInsertionEntry(LocalIsolate* isolate,
                                           StringTableKey* key,
                                           uint32_t hash) const;
 
@@ -247,8 +247,8 @@ std::unique_ptr<StringTable::Data> StringTable::Data::Resize(
   return new_data;
 }
 
-template <typename StringTableKey>
-InternalIndex StringTable::Data::FindEntry(IsolateRoot isolate,
+template <typename LocalIsolate, typename StringTableKey>
+InternalIndex StringTable::Data::FindEntry(LocalIsolate* isolate,
                                            StringTableKey* key,
                                            uint32_t hash) const {
   uint32_t count = 1;
@@ -262,7 +262,7 @@ InternalIndex StringTable::Data::FindEntry(IsolateRoot isolate,
     if (element == empty_element()) return InternalIndex::NotFound();
     if (element == deleted_element()) continue;
     String string = String::cast(element);
-    if (KeyIsMatch(key, string)) return entry;
+    if (KeyIsMatch(isolate, key, string)) return entry;
   }
 }
 
@@ -281,9 +281,9 @@ InternalIndex StringTable::Data::FindInsertionEntry(IsolateRoot isolate,
   }
 }
 
-template <typename StringTableKey>
+template <typename LocalIsolate, typename StringTableKey>
 InternalIndex StringTable::Data::FindEntryOrInsertionEntry(
-    IsolateRoot isolate, StringTableKey* key, uint32_t hash) const {
+    LocalIsolate* isolate, StringTableKey* key, uint32_t hash) const {
   InternalIndex insertion_entry = InternalIndex::NotFound();
   uint32_t count = 1;
   // EnsureCapacity will guarantee the hash table is never full.
@@ -307,7 +307,7 @@ InternalIndex StringTable::Data::FindEntryOrInsertionEntry(
     }
 
     String string = String::cast(element);
-    if (KeyIsMatch(key, string)) return entry;
+    if (KeyIsMatch(isolate, key, string)) return entry;
   }
 }
 
@@ -358,7 +358,7 @@ class InternalizedStringKey final : public StringTableKey {
     set_raw_hash_field(string->raw_hash_field());
   }
 
-  bool IsMatch(String string) override {
+  bool IsMatch(Isolate* isolate, String string) {
     DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(string));
     return string_->SlowEquals(string);
   }
@@ -532,10 +532,6 @@ template Handle<String> StringTable::LookupKey(LocalIsolate* isolate,
                                                OneByteStringKey* key);
 template Handle<String> StringTable::LookupKey(LocalIsolate* isolate,
                                                TwoByteStringKey* key);
-template Handle<String> StringTable::LookupKey(LocalIsolate* isolate,
-                                               SeqOneByteSubStringKey* key);
-template Handle<String> StringTable::LookupKey(LocalIsolate* isolate,
-                                               SeqTwoByteSubStringKey* key);
 
 template Handle<String> StringTable::LookupKey(Isolate* isolate,
                                                StringTableInsertionKey* key);

src/objects/string-table.h

@@ -22,16 +22,11 @@ class StringTableKey {
   virtual ~StringTableKey() = default;
   inline StringTableKey(uint32_t raw_hash_field, int length);
 
-  // The individual keys will have their own AsHandle, we shouldn't call the
-  // base version.
-  Handle<String> AsHandle(Isolate* isolate) = delete;
-
   uint32_t raw_hash_field() const {
     DCHECK_NE(0, raw_hash_field_);
     return raw_hash_field_;
   }
 
-  virtual bool IsMatch(String string) = 0;
   inline uint32_t hash() const;
   int length() const { return length_; }
 

src/objects/string.cc

@@ -641,8 +641,15 @@ std::unique_ptr<char[]> String::ToCString(AllowNullsFlag allow_nulls,
 
 template <typename sinkchar>
 void String::WriteToFlat(String source, sinkchar* sink, int from, int to) {
+  DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(source));
+  return WriteToFlat(source, sink, from, to,
+                     SharedStringAccessGuardIfNeeded::NotNeeded());
+}
+
+template <typename sinkchar>
+void String::WriteToFlat(String source, sinkchar* sink, int from, int to,
+                         const SharedStringAccessGuardIfNeeded& access_guard) {
   DisallowGarbageCollection no_gc;
-  SharedStringAccessGuardIfNeeded access_guard(source);
   while (from < to) {
     DCHECK_LE(0, from);
     DCHECK_LE(to, source.length());
@@ -679,7 +686,7 @@ void String::WriteToFlat(String source, sinkchar* sink, int from, int to) {
         if (to - boundary >= boundary - from) {
           // Right hand side is longer.  Recurse over left.
           if (from < boundary) {
-            WriteToFlat(first, sink, from, boundary);
+            WriteToFlat(first, sink, from, boundary, access_guard);
             if (from == 0 && cons_string.second() == first) {
               CopyChars(sink + boundary, sink, boundary);
               return;
@@ -706,7 +713,8 @@ void String::WriteToFlat(String source, sinkchar* sink, int from, int to) {
                   SeqOneByteString::cast(second).GetChars(no_gc, access_guard),
                   to - boundary);
             } else {
-              WriteToFlat(second, sink + boundary - from, 0, to - boundary);
+              WriteToFlat(second, sink + boundary - from, 0, to - boundary,
+                          access_guard);
             }
             to = boundary;
           }
@@ -718,7 +726,8 @@ void String::WriteToFlat(String source, sinkchar* sink, int from, int to) {
       case kTwoByteStringTag | kSlicedStringTag: {
         SlicedString slice = SlicedString::cast(source);
         unsigned offset = slice.offset();
-        WriteToFlat(slice.parent(), sink, from + offset, to + offset);
+        WriteToFlat(slice.parent(), sink, from + offset, to + offset,
+                    access_guard);
         return;
       }
       case kOneByteStringTag | kThinStringTag:
@@ -1680,6 +1689,8 @@ const byte* String::AddressOfCharacterAt(
 
 template EXPORT_TEMPLATE_DEFINE(V8_EXPORT_PRIVATE) void String::WriteToFlat(
     String source, uint16_t* sink, int from, int to);
+template EXPORT_TEMPLATE_DEFINE(V8_EXPORT_PRIVATE) void String::WriteToFlat(
+    String source, uint8_t* sink, int from, int to);
 
 }  // namespace internal
 }  // namespace v8

src/objects/string.h

@@ -318,11 +318,39 @@ class String : public TorqueGeneratedString<String, Name> {
                             Handle<String> two);
 
   enum class EqualityType { kWholeString, kPrefix };
+
+  // Check if this string matches the given vector of characters, either as a
+  // whole string or just a prefix.
+  //
+  // This overload should only be called on the main thread.
   template <typename Char>
   inline bool IsEqualTo(
       Vector<const Char> str,
       EqualityType eq_type = EqualityType::kWholeString) const;
 
+  // Check if this string matches the given vector of characters, either as a
+  // whole string or just a prefix.
+  //
+  // The Isolate is passed as "evidence" that this call is on the main thread,
+  // and to distiguish from the LocalIsolate overload. It is otherwise
+  // equivalent to the no-Isolate overload.
+  template <typename Char>
+  inline bool IsEqualTo(
+      Isolate* isolate, Vector<const Char> str,
+      EqualityType eq_type = EqualityType::kWholeString) const {
+    return IsEqualTo(str, eq_type);
+  }
+
+  // Check if this string matches the given vector of characters, either as a
+  // whole string or just a prefix.
+  //
+  // The LocalIsolate is passed to provide access to the string access lock,
+  // which is taken when reading the string's contents on a background thread.
+  template <typename Char>
+  inline bool IsEqualTo(
+      LocalIsolate* isolate, Vector<const Char> str,
+      EqualityType eq_type = EqualityType::kWholeString) const;
+
   V8_EXPORT_PRIVATE bool HasOneBytePrefix(Vector<const char> str);
   V8_EXPORT_PRIVATE inline bool IsOneByteEqualTo(Vector<const char> str);
 
@@ -444,6 +472,9 @@ class String : public TorqueGeneratedString<String, Name> {
   template <typename sinkchar>
   EXPORT_TEMPLATE_DECLARE(V8_EXPORT_PRIVATE)
   static void WriteToFlat(String source, sinkchar* sink, int from, int to);
+  template <typename sinkchar>
+  static void WriteToFlat(String source, sinkchar* sink, int from, int to,
+                          const SharedStringAccessGuardIfNeeded&);
 
   static inline bool IsAscii(const char* chars, int length) {
     return IsAscii(reinterpret_cast<const uint8_t*>(chars), length);
@@ -514,6 +545,12 @@ class String : public TorqueGeneratedString<String, Name> {
   // Implementation of the Get() public methods. Do not use directly.
   V8_INLINE uint16_t GetImpl(int index);
 
+  // Implementation of the IsEqualTo() public methods. Do not use directly.
+  template <typename Char>
+  V8_INLINE bool IsEqualToImpl(
+      Vector<const Char> str, EqualityType eq_type,
+      const SharedStringAccessGuardIfNeeded& access_guard) const;
+
   V8_EXPORT_PRIVATE static Handle<String> SlowFlatten(
       Isolate* isolate, Handle<ConsString> cons, AllocationType allocation);
 
@@ -536,6 +573,8 @@ class String : public TorqueGeneratedString<String, Name> {
 
 // clang-format off
 extern template EXPORT_TEMPLATE_DECLARE(V8_EXPORT_PRIVATE)
+void String::WriteToFlat(String source, uint8_t* sink, int from, int to);
+extern template EXPORT_TEMPLATE_DECLARE(V8_EXPORT_PRIVATE)
 void String::WriteToFlat(String source, uint16_t* sink, int from, int to);
 // clang-format on
 

src/snapshot/deserializer.cc

@@ -311,7 +311,7 @@ StringTableInsertionKey::StringTableInsertionKey(Handle<String> string)
   DCHECK(string->IsInternalizedString());
 }
 
-bool StringTableInsertionKey::IsMatch(String string) {
+bool StringTableInsertionKey::IsMatch(Isolate* isolate, String string) {
   // We want to compare the content of two strings here.
   return string_->SlowEquals(string);
 }

src/snapshot/deserializer.h

@@ -269,7 +269,7 @@ class StringTableInsertionKey final : public StringTableKey {
  public:
   explicit StringTableInsertionKey(Handle<String> string);
 
-  bool IsMatch(String string) override;
+  bool IsMatch(Isolate* isolate, String string);
 
   V8_WARN_UNUSED_RESULT Handle<String> AsHandle(Isolate* isolate);
   V8_WARN_UNUSED_RESULT Handle<String> AsHandle(LocalIsolate* isolate);

test/cctest/test-concurrent-string.cc

@@ -86,7 +86,7 @@ class ConcurrentStringThread final : public v8::base::Thread {
     for (unsigned int i = 0; i < length_; ++i) {
       CHECK_EQ(str_->Get(i, &local_isolate), chars_[i]);
     }
-    CHECK_EQ(TryStringToDouble(str_).value(), DOUBLE_VALUE);
+    CHECK_EQ(TryStringToDouble(&local_isolate, str_).value(), DOUBLE_VALUE);
   }
 
  private: