[offthread] Acq/rel the string-table data pointer

Since the string-table's data pointer is written inside a mutex, but
read outside of it, we need to acq/rel access to it.

There's no support in C++ for an std::atomic<std::unique_ptr>, so this
patch changes the std::unique_ptr<Data> into a std::atomic<Data*>, and
handles the deletion manually. StringTable::Data still uses

std::unique_ptr as the general pointer-passing contract, we just
carefully set and release the unique_ptrs when accessing and setting
the StringTable's atomic Data pointer.

Change-Id: I711a56825e2f5f9b2db63d1874e09c2627af54b8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2410057
Commit-Queue: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Toon Verwaest <verwaest@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Auto-Submit: Leszek Swirski <leszeks@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69932}

src/execution/isolate.cc

@@ -3484,7 +3484,7 @@ bool Isolate::Init(ReadOnlyDeserializer* read_only_deserializer,
   date_cache_ = new DateCache();
   heap_profiler_ = new HeapProfiler(heap());
   interpreter_ = new interpreter::Interpreter(this);
-  string_table_.reset(new StringTable());
+  string_table_.reset(new StringTable(this));
 
   compiler_dispatcher_ =
       new CompilerDispatcher(this, V8::GetCurrentPlatform(), FLAG_stack_size);

src/objects/string-table.cc

@@ -4,12 +4,15 @@
 
 #include "src/objects/string-table.h"
 
+#include <atomic>
+
 #include "src/base/atomicops.h"
 #include "src/base/macros.h"
 #include "src/common/assert-scope.h"
 #include "src/common/globals.h"
 #include "src/common/ptr-compr-inl.h"
 #include "src/execution/isolate-utils-inl.h"
+#include "src/heap/safepoint.h"
 #include "src/objects/internal-index.h"
 #include "src/objects/object-list-macros.h"
 #include "src/objects/slots-inl.h"
@@ -88,8 +91,8 @@ bool KeyIsMatch(StringTableKey* key, String string) {
 class StringTable::Data {
  public:
   static std::unique_ptr<Data> New(int capacity);
-  static void Resize(const Isolate* isolate, std::unique_ptr<Data>& data,
-                     int capacity);
+  static std::unique_ptr<Data> Resize(const Isolate* isolate,
+                                      std::unique_ptr<Data> data, int capacity);
 
   OffHeapObjectSlot slot(InternalIndex index) const {
     return OffHeapObjectSlot(&elements_[index.as_uint32()]);
@@ -220,8 +223,8 @@ std::unique_ptr<StringTable::Data> StringTable::Data::New(int capacity) {
   return std::unique_ptr<Data>(new (capacity) Data(capacity));
 }
 
-void StringTable::Data::Resize(const Isolate* isolate,
-                               std::unique_ptr<Data>& data, int capacity) {
+std::unique_ptr<StringTable::Data> StringTable::Data::Resize(
+    const Isolate* isolate, std::unique_ptr<Data> data, int capacity) {
   std::unique_ptr<Data> new_data(new (capacity) Data(capacity));
 
   DCHECK_LT(data->number_of_elements(), new_data->capacity());
@@ -240,12 +243,8 @@ void StringTable::Data::Resize(const Isolate* isolate,
   }
   new_data->number_of_elements_ = data->number_of_elements();
 
-  // unique_ptr swap dance to update `data` with `new_data`, move the old `data`
-  // to `new_data->previous_data_`, all while ensuring that `data` is never
-  // nullptr.
-  std::unique_ptr<Data>& data_storage = new_data->previous_data_;
-  data_storage = std::move(new_data);
-  std::swap(data_storage, data);
+  new_data->previous_data_ = std::move(data);
+  return new_data;
 }
 
 template <typename StringTableKey>
@@ -327,12 +326,21 @@ void StringTable::Data::Print(const Isolate* isolate) const {
   os << "}" << std::endl;
 }
 
-StringTable::StringTable() : data_(Data::New(kStringTableMinCapacity)) {}
-StringTable::~StringTable() = default;
+StringTable::StringTable(Isolate* isolate)
+    : data_(Data::New(kStringTableMinCapacity).release())
+#ifdef DEBUG
+      ,
+      isolate_(isolate)
+#endif
+{
+}
+StringTable::~StringTable() { delete data_; }
 
-int StringTable::Capacity() const { return data_->capacity(); }
+int StringTable::Capacity() const {
+  return data_.load(std::memory_order_acquire)->capacity();
+}
 int StringTable::NumberOfElements() const {
-  return data_->number_of_elements();
+  return data_.load(std::memory_order_acquire)->number_of_elements();
 }
 
 // InternalizedStringKey carries a string/internalized-string object as key.
@@ -456,7 +464,7 @@ Handle<String> StringTable::LookupKey(LocalIsolate* isolate,
   while (true) {
     // Load the current string table data, in case another thread updates the
     // data while we're reading.
-    const StringTable::Data* data = data_.get();
+    const Data* data = data_.load(std::memory_order_acquire);
 
     // First try to find the string in the table. This is safe to do even if the
     // table is now reallocated; we won't find a stale entry in the old table
@@ -479,9 +487,7 @@ Handle<String> StringTable::LookupKey(LocalIsolate* isolate,
     {
       base::MutexGuard table_write_guard(&write_mutex_);
 
-      EnsureCapacity(ptr_cmp_isolate, 1);
-      // Reload the data pointer in case EnsureCapacity changed it.
-      StringTable::Data* data = data_.get();
+      Data* data = EnsureCapacity(ptr_cmp_isolate, 1);
 
       // Check one last time if the key is present in the table, in case it was
       // added after the check.
@@ -530,12 +536,14 @@ template Handle<String> StringTable::LookupKey(LocalIsolate* isolate,
 template Handle<String> StringTable::LookupKey(Isolate* isolate,
                                                StringTableInsertionKey* key);
 
-void StringTable::EnsureCapacity(const Isolate* isolate,
-                                 int additional_elements) {
+StringTable::Data* StringTable::EnsureCapacity(const Isolate* isolate,
+                                               int additional_elements) {
   // This call is only allowed while the write mutex is held.
   write_mutex_.AssertHeld();
 
-  StringTable::Data* data = data_.get();
+  // This load can be relaxed as the table pointer can only be modified while
+  // the lock is held.
+  Data* data = data_.load(std::memory_order_relaxed);
 
   // Grow or shrink table if needed. We first try to shrink the table, if it
   // is sufficiently empty; otherwise we make sure to grow it so that it has
@@ -557,8 +565,18 @@ void StringTable::EnsureCapacity(const Isolate* isolate,
   }
 
   if (new_capacity != -1) {
-    Data::Resize(isolate, data_, new_capacity);
+    std::unique_ptr<Data> new_data =
+        Data::Resize(isolate, std::unique_ptr<Data>(data), new_capacity);
+    // `new_data` is the new owner of `data`.
+    DCHECK_EQ(new_data->PreviousData(), data);
+    // Release-store the new data pointer as `data_`, so that it can be
+    // acquire-loaded by other threads. This string table becomes the owner of
+    // the pointer.
+    data = new_data.release();
+    data_.store(data, std::memory_order_release);
   }
+
+  return data;
 }
 
 // static
@@ -602,7 +620,8 @@ Address StringTable::Data::TryStringToIndexOrLookupExisting(Isolate* isolate,
     return Smi::FromInt(ResultSentinel::kUnsupported).ptr();
   }
 
-  StringTable::Data* string_table_data = isolate->string_table()->data_.get();
+  Data* string_table_data =
+      isolate->string_table()->data_.load(std::memory_order_acquire);
 
   InternalIndex entry = string_table_data->FindEntry(isolate, &key, key.hash());
   if (entry.is_not_found()) {
@@ -647,29 +666,44 @@ Address StringTable::TryStringToIndexOrLookupExisting(Isolate* isolate,
     }
   }
 
-  StringTable* table = isolate->string_table();
   if (source.IsOneByteRepresentation()) {
-    return table->data_->TryStringToIndexOrLookupExisting<uint8_t>(
+    return StringTable::Data::TryStringToIndexOrLookupExisting<uint8_t>(
         isolate, string, source, start);
   }
-  return table->data_->TryStringToIndexOrLookupExisting<uint16_t>(
+  return StringTable::Data::TryStringToIndexOrLookupExisting<uint16_t>(
       isolate, string, source, start);
 }
 
-void StringTable::Print(const Isolate* isolate) const { data_->Print(isolate); }
+void StringTable::Print(const Isolate* isolate) const {
+  data_.load(std::memory_order_acquire)->Print(isolate);
+}
 
 size_t StringTable::GetCurrentMemoryUsage() const {
-  return sizeof(*this) + data_->GetCurrentMemoryUsage();
+  return sizeof(*this) +
+         data_.load(std::memory_order_acquire)->GetCurrentMemoryUsage();
 }
 
 void StringTable::IterateElements(RootVisitor* visitor) {
-  data_->IterateElements(visitor);
+  // This should only happen during garbage collection when background threads
+  // are paused, so the load can be relaxed.
+  DCHECK_IMPLIES(FLAG_local_heaps, isolate_->heap()->safepoint()->IsActive());
+  data_.load(std::memory_order_relaxed)->IterateElements(visitor);
 }
 
-void StringTable::DropOldData() { data_->DropPreviousData(); }
+void StringTable::DropOldData() {
+  // This should only happen during garbage collection when background threads
+  // are paused, so the load can be relaxed.
+  DCHECK_IMPLIES(FLAG_local_heaps, isolate_->heap()->safepoint()->IsActive());
+  DCHECK_NE(isolate_->heap()->gc_state(), Heap::NOT_IN_GC);
+  data_.load(std::memory_order_relaxed)->DropPreviousData();
+}
 
 void StringTable::NotifyElementsRemoved(int count) {
-  data_->ElementsRemoved(count);
+  // This should only happen during garbage collection when background threads
+  // are paused, so the load can be relaxed.
+  DCHECK_IMPLIES(FLAG_local_heaps, isolate_->heap()->safepoint()->IsActive());
+  DCHECK_NE(isolate_->heap()->gc_state(), Heap::NOT_IN_GC);
+  data_.load(std::memory_order_relaxed)->ElementsRemoved(count);
 }
 
 }  // namespace internal

src/objects/string-table.h

@@ -55,7 +55,7 @@ class V8_EXPORT_PRIVATE StringTable {
   static constexpr Smi empty_element() { return Smi::FromInt(0); }
   static constexpr Smi deleted_element() { return Smi::FromInt(1); }
 
-  StringTable();
+  explicit StringTable(Isolate* isolate);
   ~StringTable();
 
   int Capacity() const;
@@ -87,11 +87,15 @@ class V8_EXPORT_PRIVATE StringTable {
   void NotifyElementsRemoved(int count);
 
  private:
-  void EnsureCapacity(const Isolate* isolate, int additional_elements);
-
   class Data;
-  std::unique_ptr<Data> data_;
+
+  Data* EnsureCapacity(const Isolate* isolate, int additional_elements);
+
+  std::atomic<Data*> data_;
   base::Mutex write_mutex_;
+#ifdef DEBUG
+  Isolate* isolate_;
+#endif
 };
 
 }  // namespace internal

src/snapshot/snapshot.cc

@@ -8,6 +8,7 @@
 
 #include "src/base/platform/platform.h"
 #include "src/execution/isolate-inl.h"
+#include "src/heap/safepoint.h"
 #include "src/init/bootstrapper.h"
 #include "src/logging/counters.h"
 #include "src/objects/code-kind.h"
@@ -351,6 +352,13 @@ v8::StartupData Snapshot::Create(
   DCHECK_EQ(contexts->size(), embedder_fields_serializers.size());
   DCHECK_GT(contexts->size(), 0);
 
+  // Enter a safepoint so that the heap is safe to iterate.
+  // TODO(leszeks): This safepoint's scope could be tightened to just string
+  // table iteration, as that iteration relies on there not being any concurrent
+  // threads mutating the string table. But, there's currently no harm in
+  // holding it for the entire snapshot serialization.
+  SafepointScope safepoint(isolate->heap());
+
   ReadOnlySerializer read_only_serializer(isolate, flags);
   read_only_serializer.SerializeReadOnlyRoots();
 

test/cctest/test-serialize.cc

@@ -26,11 +26,8 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 
 #include <signal.h>
-
 #include <sys/stat.h>
 
-#include "src/init/v8.h"
-
 #include "src/api/api-inl.h"
 #include "src/codegen/assembler-inl.h"
 #include "src/codegen/compilation-cache.h"
@@ -39,8 +36,10 @@
 #include "src/debug/debug.h"
 #include "src/heap/heap-inl.h"
 #include "src/heap/read-only-heap.h"
+#include "src/heap/safepoint.h"
 #include "src/heap/spaces.h"
 #include "src/init/bootstrapper.h"
+#include "src/init/v8.h"
 #include "src/interpreter/interpreter.h"
 #include "src/numbers/hash-seed-inl.h"
 #include "src/objects/js-array-buffer-inl.h"
@@ -171,6 +170,8 @@ static StartupBlobs Serialize(v8::Isolate* isolate) {
   internal_isolate->heap()->CollectAllAvailableGarbage(
       i::GarbageCollectionReason::kTesting);
 
+  SafepointScope safepoint(internal_isolate->heap());
+
   DisallowHeapAllocation no_gc;
   ReadOnlySerializer read_only_serializer(internal_isolate,
                                           Snapshot::kDefaultSerializerFlags);
@@ -382,6 +383,8 @@ static void SerializeContext(Vector<const byte>* startup_blob_out,
 
     env.Reset();
 
+    SafepointScope safepoint(heap);
+
     DisallowHeapAllocation no_gc;
     SnapshotByteSink read_only_sink;
     ReadOnlySerializer read_only_serializer(isolate,
@@ -532,6 +535,8 @@ static void SerializeCustomContext(Vector<const byte>* startup_blob_out,
 
     env.Reset();
 
+    SafepointScope safepoint(isolate->heap());
+
     DisallowHeapAllocation no_gc;
     SnapshotByteSink read_only_sink;
     ReadOnlySerializer read_only_serializer(isolate,