[compiler] Create concurrency tests for Strings

Bug: v8:7790
Change-Id: I7a7ed66f66e723192a45a05e68e20c11b8262598
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2495460
Commit-Queue: Santiago Aboy Solanes <solanes@chromium.org>
Reviewed-by: Ross McIlroy <rmcilroy@chromium.org>
Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
Reviewed-by: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71142}

src/compiler/js-heap-broker.cc

@@ -857,31 +857,12 @@ class SymbolData : public NameData {
   }
 };
 
-namespace {
-
-// String to double helper without heap allocation.
-base::Optional<double> StringToDouble(Handle<String> object) {
-  const int kMaxLengthForDoubleConversion = 23;
-  String string = *object;
-  int length = string.length();
-  if (length <= kMaxLengthForDoubleConversion) {
-    const int flags = ALLOW_HEX | ALLOW_OCTAL | ALLOW_BINARY;
-    uc16 buffer[kMaxLengthForDoubleConversion];
-    String::WriteToFlat(*object, buffer, 0, length);
-    Vector<const uc16> v(buffer, length);
-    return StringToDouble(v, flags);
-  }
-  return base::nullopt;
-}
-
-}  // namespace
-
 StringData::StringData(JSHeapBroker* broker, ObjectData** storage,
                        Handle<String> object)
     : NameData(broker, storage, object),
       length_(object->length()),
       first_char_(length_ > 0 ? object->Get(0) : 0),
-      to_number_(StringToDouble(object)),
+      to_number_(TryStringToDouble(object)),
       is_external_string_(object->IsExternalString()),
       is_seq_string_(object->IsSeqString()),
       chars_as_strings_(broker->zone()) {}
@@ -3208,6 +3189,15 @@ bool MapRef::IsUnboxedDoubleField(InternalIndex descriptor_index) const {
       .is_unboxed_double_field;
 }
 
+int StringRef::length() const {
+  if (data_->should_access_heap()) {
+    AllowHandleDereferenceIfNeeded allow_handle_dereference(data()->kind(),
+                                                            broker()->mode());
+    return object()->synchronized_length();
+  }
+  return data()->AsString()->length();
+}
+
 uint16_t StringRef::GetFirstChar() {
   if (data_->should_access_heap()) {
     AllowHandleDereferenceIfNeeded allow_handle_dereference(data()->kind(),
@@ -3223,7 +3213,7 @@ base::Optional<double> StringRef::ToNumber() {
                                                             broker()->mode());
     AllowHandleAllocationIfNeeded allow_handle_allocation(data()->kind(),
                                                           broker()->mode());
-    return StringToDouble(object());
+    return TryStringToDouble(object());
   }
   return data()->AsString()->to_number();
 }
@@ -3603,8 +3593,6 @@ BROKER_SFI_FIELDS(DEF_SFI_ACCESSOR)
 BIMODAL_ACCESSOR_C(SharedFunctionInfo, SharedFunctionInfo::Inlineability,
                    GetInlineability)
 
-BIMODAL_ACCESSOR_C(String, int, length)
-
 BIMODAL_ACCESSOR(FeedbackCell, HeapObject, value)
 
 base::Optional<ObjectRef> MapRef::GetStrongValue(

src/numbers/conversions.cc

@@ -1367,6 +1367,31 @@ double StringToDouble(Isolate* isolate, Handle<String> string, int flags,
   }
 }
 
+base::Optional<double> TryStringToDouble(Handle<String> object,
+                                         int max_length_for_conversion) {
+  DisallowGarbageCollection no_gc;
+  int length = object->length();
+  if (length > max_length_for_conversion) {
+    return base::nullopt;
+  }
+
+  const int flags = ALLOW_HEX | ALLOW_OCTAL | ALLOW_BINARY;
+  auto buffer = std::make_unique<uc16[]>(max_length_for_conversion);
+  Isolate* isolate = nullptr;
+  // Read only strings have no isolate associated to them and we don't need a
+  // lock for those.
+  const bool need_lock = GetIsolateFromHeapObject(*object, &isolate);
+  if (need_lock) {
+    isolate->string_access()->LockShared();
+  }
+  String::WriteToFlat(*object, buffer.get(), 0, length);
+  if (need_lock) {
+    isolate->string_access()->UnlockShared();
+  }
+  Vector<const uc16> v(buffer.get(), length);
+  return StringToDouble(v, flags);
+}
+
 bool IsSpecialIndex(String string) {
   // Max length of canonical double: -X.XXXXXXXXXXXXXXXXX-eXXX
   const int kBufferSize = 24;

src/numbers/conversions.h

@@ -7,6 +7,7 @@
 
 #include "src/base/export-template.h"
 #include "src/base/logging.h"
+#include "src/base/optional.h"
 #include "src/common/globals.h"
 #include "src/utils/vector.h"
 
@@ -160,6 +161,13 @@ inline uint64_t PositiveNumberToUint64(Object number);
 double StringToDouble(Isolate* isolate, Handle<String> string, int flags,
                       double empty_string_val = 0.0);
 
+// String to double helper without heap allocation. It will acquire the string
+// lock in order to be thread-safe. Returns base::nullopt if the string is
+// longer than {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);
+
 inline bool TryNumberToSize(Object number, size_t* result);
 
 // Converts a number into size_t.

test/cctest/BUILD.gn

@@ -209,6 +209,7 @@ v8_source_set("cctest_sources") {
     "test-concurrent-feedback-vector.cc",
     "test-concurrent-prototype.cc",
     "test-concurrent-script-context-table.cc",
+    "test-concurrent-string.cc",
     "test-concurrent-transition-array.cc",
     "test-constantpool.cc",
     "test-conversions.cc",

test/cctest/test-concurrent-string.cc

@@ -0,0 +1,243 @@
+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "src/api/api.h"
+#include "src/base/platform/semaphore.h"
+#include "src/handles/handles-inl.h"
+#include "src/handles/local-handles-inl.h"
+#include "src/handles/persistent-handles.h"
+#include "src/heap/heap.h"
+#include "src/heap/local-heap-inl.h"
+#include "src/heap/local-heap.h"
+#include "test/cctest/cctest.h"
+#include "test/cctest/heap/heap-utils.h"
+
+namespace v8 {
+namespace internal {
+
+namespace {
+
+#define DOUBLE_VALUE 28.123456789
+#define STRING_VALUE "28.123456789"
+#define ARRAY_VALUE \
+  { '2', '8', '.', '1', '2', '3', '4', '5', '6', '7', '8', '9' }
+
+// Adapted from cctest/test-api.cc, and
+// test/cctest/heap/test-external-string-tracker.cc.
+class TestOneByteResource : public v8::String::ExternalOneByteStringResource {
+ public:
+  explicit TestOneByteResource(const char* data)
+      : data_(data), length_(strlen(data)) {}
+
+  ~TestOneByteResource() override { i::DeleteArray(data_); }
+
+  const char* data() const override { return data_; }
+
+  size_t length() const override { return length_; }
+
+ private:
+  const char* data_;
+  size_t length_;
+};
+
+// Adapted from cctest/test-api.cc.
+class TestTwoByteResource : public v8::String::ExternalStringResource {
+ public:
+  explicit TestTwoByteResource(uint16_t* data) : data_(data), length_(0) {
+    while (data[length_]) ++length_;
+  }
+
+  ~TestTwoByteResource() override { i::DeleteArray(data_); }
+
+  const uint16_t* data() const override { return data_; }
+
+  size_t length() const override { return length_; }
+
+ private:
+  uint16_t* data_;
+  size_t length_;
+};
+
+class ConcurrentStringThread final : public v8::base::Thread {
+ public:
+  ConcurrentStringThread(Heap* heap, 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),
+        str_(str),
+        ph_(std::move(ph)),
+        sema_started_(sema_started),
+        length_(chars.size()),
+        chars_(chars) {}
+
+  void Run() override {
+    LocalHeap local_heap(heap_, ThreadKind::kBackground, std::move(ph_));
+    UnparkedScope unparked_scope(&local_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(TryStringToDouble(str_).value(), DOUBLE_VALUE);
+  }
+
+ private:
+  Heap* heap_;
+  Handle<String> str_;
+  std::unique_ptr<PersistentHandles> ph_;
+  base::Semaphore* sema_started_;
+  uint64_t length_;
+  std::vector<uint16_t> chars_;
+};
+
+// Inspect a one byte string, while the main thread externalizes it.
+TEST(InspectOneByteExternalizing) {
+  heap::EnsureFlagLocalHeapsEnabled();
+  CcTest::InitializeVM();
+  Isolate* isolate = CcTest::i_isolate();
+
+  std::unique_ptr<PersistentHandles> ph = isolate->NewPersistentHandles();
+
+  auto factory = isolate->factory();
+  HandleScope handle_scope(isolate);
+
+  // Crate an internalized one-byte string.
+  const char* raw_string = STRING_VALUE;
+  Handle<String> one_byte_string = factory->InternalizeString(
+      factory->NewStringFromAsciiChecked(raw_string));
+  CHECK(one_byte_string->IsOneByteRepresentation());
+  CHECK(!one_byte_string->IsExternalString());
+  CHECK(one_byte_string->IsInternalizedString());
+
+  Handle<String> persistent_string = ph->NewHandle(one_byte_string);
+
+  std::vector<uint16_t> chars;
+  for (int i = 0; i < one_byte_string->length(); ++i) {
+    chars.push_back(one_byte_string->Get(i));
+  }
+
+  base::Semaphore sema_started(0);
+
+  std::unique_ptr<ConcurrentStringThread> thread(new ConcurrentStringThread(
+      isolate->heap(), persistent_string, std::move(ph), &sema_started, chars));
+  CHECK(thread->Start());
+
+  sema_started.Wait();
+
+  // Externalize it to a one-byte external string.
+  // We need to use StrDup in this case since the TestOneByteResource will get
+  // ownership of raw_string otherwise.
+  CHECK(one_byte_string->MakeExternal(
+      new TestOneByteResource(i::StrDup(raw_string))));
+  CHECK(one_byte_string->IsExternalOneByteString());
+  CHECK(one_byte_string->IsInternalizedString());
+
+  thread->Join();
+}
+
+// Inspect a one byte string, while the main thread externalizes it into a two
+// bytes string.
+TEST(InspectOneIntoTwoByteExternalizing) {
+  heap::EnsureFlagLocalHeapsEnabled();
+  CcTest::InitializeVM();
+  Isolate* isolate = CcTest::i_isolate();
+
+  std::unique_ptr<PersistentHandles> ph = isolate->NewPersistentHandles();
+
+  auto factory = isolate->factory();
+  HandleScope handle_scope(isolate);
+
+  // Crate an internalized one-byte string.
+  const char* raw_string = STRING_VALUE;
+  Handle<String> one_byte_string = factory->InternalizeString(
+      factory->NewStringFromAsciiChecked(raw_string));
+  CHECK(one_byte_string->IsOneByteRepresentation());
+  CHECK(!one_byte_string->IsExternalString());
+  CHECK(one_byte_string->IsInternalizedString());
+
+  Handle<String> persistent_string = ph->NewHandle(one_byte_string);
+  std::vector<uint16_t> chars;
+  for (int i = 0; i < one_byte_string->length(); ++i) {
+    chars.push_back(one_byte_string->Get(i));
+  }
+
+  base::Semaphore sema_started(0);
+
+  std::unique_ptr<ConcurrentStringThread> thread(new ConcurrentStringThread(
+      isolate->heap(), persistent_string, std::move(ph), &sema_started, chars));
+  CHECK(thread->Start());
+
+  sema_started.Wait();
+
+  // Externalize it to a two-bytes external string. AsciiToTwoByteString does
+  // the string duplication for us.
+  CHECK(one_byte_string->MakeExternal(
+      new TestTwoByteResource(AsciiToTwoByteString(raw_string))));
+  CHECK(one_byte_string->IsExternalTwoByteString());
+  CHECK(one_byte_string->IsInternalizedString());
+
+  thread->Join();
+}
+
+// Inspect a two byte string, while the main thread externalizes it.
+TEST(InspectTwoByteExternalizing) {
+  heap::EnsureFlagLocalHeapsEnabled();
+  CcTest::InitializeVM();
+  Isolate* isolate = CcTest::i_isolate();
+
+  std::unique_ptr<PersistentHandles> ph = isolate->NewPersistentHandles();
+
+  auto factory = isolate->factory();
+  HandleScope handle_scope(isolate);
+
+  // Crate an internalized two-byte string.
+  // TODO(solanes): Can we have only one raw string?
+  const char* raw_string = STRING_VALUE;
+  // TODO(solanes): Is this the best way to create a two byte string from chars?
+  const int kLength = 12;
+  const uint16_t two_byte_array[kLength] = ARRAY_VALUE;
+  Handle<String> two_bytes_string;
+  {
+    Handle<SeqTwoByteString> raw =
+        factory->NewRawTwoByteString(kLength).ToHandleChecked();
+    DisallowHeapAllocation no_gc;
+    CopyChars(raw->GetChars(no_gc), two_byte_array, kLength);
+    two_bytes_string = raw;
+  }
+  two_bytes_string = factory->InternalizeString(two_bytes_string);
+  CHECK(two_bytes_string->IsTwoByteRepresentation());
+  CHECK(!two_bytes_string->IsExternalString());
+  CHECK(two_bytes_string->IsInternalizedString());
+
+  Handle<String> persistent_string = ph->NewHandle(two_bytes_string);
+  std::vector<uint16_t> chars;
+  for (int i = 0; i < two_bytes_string->length(); ++i) {
+    chars.push_back(two_bytes_string->Get(i));
+  }
+  base::Semaphore sema_started(0);
+
+  std::unique_ptr<ConcurrentStringThread> thread(new ConcurrentStringThread(
+      isolate->heap(), persistent_string, std::move(ph), &sema_started, chars));
+  CHECK(thread->Start());
+
+  sema_started.Wait();
+
+  // Externalize it to a two-bytes external string.
+  CHECK(two_bytes_string->MakeExternal(
+      new TestTwoByteResource(AsciiToTwoByteString(raw_string))));
+  CHECK(two_bytes_string->IsExternalTwoByteString());
+  CHECK(two_bytes_string->IsInternalizedString());
+
+  thread->Join();
+}
+
+}  // anonymous namespace
+
+}  // namespace internal
+}  // namespace v8