[compiler] Add (Local)?Isolate parameter to String::Get

If we have a regular isolate (or none at all), we can skip acquiring
the lock check and DCHECK that we are calling from the main thread.
If we have a LocalIsolate, we acquire the string lock if needed.

Bug: v8:7790
Change-Id: Ie3562e8172a3e3eca8d194e8652cb881f765cdb8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2551102
Commit-Queue: Santiago Aboy Solanes <solanes@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71340}

src/compiler/js-heap-broker.cc

@@ -3206,8 +3206,14 @@ uint16_t StringRef::GetFirstChar() {
   if (data_->should_access_heap()) {
     AllowHandleDereferenceIfNeeded allow_handle_dereference(data()->kind(),
                                                             broker()->mode());
+    if (broker()->local_isolate()) {
+      return object()->Get(0, broker()->local_isolate());
+    } else {
+      // TODO(solanes, v8:7790): Remove this case once we always have a local
+      // isolate, i.e. the inlining phase is done concurrently all the time.
       return object()->Get(0);
     }
+  }
   return data()->AsString()->first_char();
 }
 

src/execution/local-isolate.h

@@ -52,6 +52,7 @@ class V8_EXPORT_PRIVATE LocalIsolate final : private HiddenLocalFactory {
   inline Object root(RootIndex index) const;
 
   StringTable* string_table() const { return isolate_->string_table(); }
+  base::SharedMutex* string_access() { return isolate_->string_access(); }
 
   v8::internal::LocalFactory* factory() {
     // Upcast to the privately inherited base-class using c-style casts to avoid

src/heap/factory-base.cc

@@ -569,8 +569,8 @@ MaybeHandle<String> FactoryBase<Impl>::NewConsString(
   int length = left_length + right_length;
 
   if (length == 2) {
-    uint16_t c1 = left->Get(0);
-    uint16_t c2 = right->Get(0);
+    uint16_t c1 = left->Get(0, isolate());
+    uint16_t c2 = right->Get(0, isolate());
     return MakeOrFindTwoCharacterString(c1, c2);
   }
 

src/objects/string-inl.h

@@ -26,16 +26,24 @@ namespace internal {
 
 #include "torque-generated/src/objects/string-tq-inl.inc"
 
-// 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.
 class 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 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)) {
+      mutex_guard.emplace(local_isolate->string_access());
+    }
+  }
+
   static SharedStringAccessGuardIfNeeded NotNeeded() {
     return SharedStringAccessGuardIfNeeded();
   }
@@ -55,6 +63,10 @@ class SharedStringAccessGuardIfNeeded {
     return true;
   }
 
+  static bool IsNeeded(String str, LocalIsolate* local_isolate) {
+    return !local_isolate->heap()->is_main_thread();
+  }
+
  private:
   // Default constructor and move constructor required for the NotNeeded()
   // static constructor.
@@ -471,10 +483,18 @@ Handle<String> String::Flatten(LocalIsolate* isolate, Handle<String> string,
   return string;
 }
 
-uint16_t String::Get(int index) {
-  DCHECK(index >= 0 && index < length());
+uint16_t String::Get(int index, Isolate* isolate) {
+  DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*this));
+  return GetImpl(index);
+}
 
-  SharedStringAccessGuardIfNeeded scope(*this);
+uint16_t String::Get(int index, LocalIsolate* local_isolate) {
+  SharedStringAccessGuardIfNeeded scope(*this, local_isolate);
+  return GetImpl(index);
+}
+
+uint16_t String::GetImpl(int index) {
+  DCHECK(index >= 0 && index < length());
 
   class StringGetDispatcher : public AllStatic {
    public:

src/objects/string.h

@@ -214,7 +214,10 @@ class String : public TorqueGeneratedString<String, Name> {
   inline void Set(int index, uint16_t value);
   // Get individual two byte char in the string.  Repeated calls
   // to this method are not efficient unless the string is flat.
-  V8_INLINE uint16_t Get(int index);
+  // If it is called from a background thread, the LocalIsolate version should
+  // be used.
+  V8_INLINE uint16_t Get(int index, Isolate* isolate = nullptr);
+  V8_INLINE uint16_t Get(int index, LocalIsolate* local_isolate);
 
   // ES6 section 7.1.3.1 ToNumber Applied to the String Type
   static Handle<Object> ToNumber(Isolate* isolate, Handle<String> subject);
@@ -510,6 +513,9 @@ class String : public TorqueGeneratedString<String, Name> {
   friend class StringTableInsertionKey;
   friend class InternalizedStringKey;
 
+  // Implementation of the Get() public methods. Do not use directly.
+  V8_INLINE uint16_t GetImpl(int index);
+
   V8_EXPORT_PRIVATE static Handle<String> SlowFlatten(
       Isolate* isolate, Handle<ConsString> cons, AllocationType allocation);
 

test/cctest/test-concurrent-string.cc

@@ -62,12 +62,12 @@ class TestTwoByteResource : public v8::String::ExternalStringResource {
 
 class ConcurrentStringThread final : public v8::base::Thread {
  public:
-  ConcurrentStringThread(Heap* heap, Handle<String> str,
+  ConcurrentStringThread(Isolate* isolate, Handle<String> str,
                          std::unique_ptr<PersistentHandles> ph,
                          base::Semaphore* sema_started,
                          std::vector<uint16_t> chars)
       : v8::base::Thread(base::Thread::Options("ThreadWithLocalHeap")),
-        heap_(heap),
+        isolate_(isolate),
         str_(str),
         ph_(std::move(ph)),
         sema_started_(sema_started),
@@ -75,21 +75,22 @@ class ConcurrentStringThread final : public v8::base::Thread {
         chars_(chars) {}
 
   void Run() override {
-    LocalHeap local_heap(heap_, ThreadKind::kBackground, std::move(ph_));
-    UnparkedScope unparked_scope(&local_heap);
+    LocalIsolate local_isolate(isolate_, ThreadKind::kBackground);
+    local_isolate.heap()->AttachPersistentHandles(std::move(ph_));
+    UnparkedScope unparked_scope(local_isolate.heap());
 
     sema_started_->Signal();
     // Check the three operations we do from the StringRef concurrently: get the
     // string, the nth character, and convert into a double.
     CHECK_EQ(str_->synchronized_length(), length_);
     for (unsigned int i = 0; i < length_; ++i) {
-      CHECK_EQ(str_->Get(i), chars_[i]);
+      CHECK_EQ(str_->Get(i, &local_isolate), chars_[i]);
     }
     CHECK_EQ(TryStringToDouble(str_).value(), DOUBLE_VALUE);
   }
 
  private:
-  Heap* heap_;
+  Isolate* isolate_;
   Handle<String> str_;
   std::unique_ptr<PersistentHandles> ph_;
   base::Semaphore* sema_started_;
@@ -126,7 +127,7 @@ TEST(InspectOneByteExternalizing) {
   base::Semaphore sema_started(0);
 
   std::unique_ptr<ConcurrentStringThread> thread(new ConcurrentStringThread(
-      isolate->heap(), persistent_string, std::move(ph), &sema_started, chars));
+      isolate, persistent_string, std::move(ph), &sema_started, chars));
   CHECK(thread->Start());
 
   sema_started.Wait();
@@ -171,7 +172,7 @@ TEST(InspectOneIntoTwoByteExternalizing) {
   base::Semaphore sema_started(0);
 
   std::unique_ptr<ConcurrentStringThread> thread(new ConcurrentStringThread(
-      isolate->heap(), persistent_string, std::move(ph), &sema_started, chars));
+      isolate, persistent_string, std::move(ph), &sema_started, chars));
   CHECK(thread->Start());
 
   sema_started.Wait();
@@ -224,7 +225,7 @@ TEST(InspectTwoByteExternalizing) {
   base::Semaphore sema_started(0);
 
   std::unique_ptr<ConcurrentStringThread> thread(new ConcurrentStringThread(
-      isolate->heap(), persistent_string, std::move(ph), &sema_started, chars));
+      isolate, persistent_string, std::move(ph), &sema_started, chars));
   CHECK(thread->Start());
 
   sema_started.Wait();