[vm/concurrency] Have custom weak table per isolate used for snapshot writing

Currently the snapshotter will just reset the weak tables once it's
done. Once we have a shared heap this would not work anymore since the
resetting of the weak tables can happen while another isolate is in the
middle of snapshotting.

We therefore make each isolate have it's own weak table used for
snapshotting messages.

The memory for the weak tables is managed by a std::unqiue_ptr in the isolate.

Issue #36097

Change-Id: Ic0f4c4a96b6e66606be9e004259d2fee995f7099
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/114858
Commit-Queue: Martin Kustermann <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>

Author: mkustermann

runtime/vm/heap/heap.cc

@@ -877,26 +877,39 @@ void Heap::SetWeakEntry(RawObject* raw_obj, WeakSelector sel, intptr_t val) {
 
 void Heap::ForwardWeakEntries(RawObject* before_object,
                               RawObject* after_object) {
+  const auto before_space =
+      before_object->IsNewObject() ? Heap::kNew : Heap::kOld;
+  const auto after_space =
+      after_object->IsNewObject() ? Heap::kNew : Heap::kOld;
+
   for (int sel = 0; sel < Heap::kNumWeakSelectors; sel++) {
-    WeakTable* before_table =
-        GetWeakTable(before_object->IsNewObject() ? Heap::kNew : Heap::kOld,
-                     static_cast<Heap::WeakSelector>(sel));
+    const auto selector = static_cast<Heap::WeakSelector>(sel);
+    auto before_table = GetWeakTable(before_space, selector);
     intptr_t entry = before_table->RemoveValue(before_object);
     if (entry != 0) {
-      WeakTable* after_table =
-          GetWeakTable(after_object->IsNewObject() ? Heap::kNew : Heap::kOld,
-                       static_cast<Heap::WeakSelector>(sel));
+      auto after_table = GetWeakTable(after_space, selector);
       after_table->SetValue(after_object, entry);
     }
   }
+
+  // We only come here during hot reload, in which case we assume that none of
+  // the isolates is in the middle of sending messages.
+  RELEASE_ASSERT(isolate()->forward_table_new() == nullptr);
+  RELEASE_ASSERT(isolate()->forward_table_old() == nullptr);
 }
 
 void Heap::ForwardWeakTables(ObjectPointerVisitor* visitor) {
+  // NOTE: This method is only used by the compactor, so there is no need to
+  // process the `Heap::kNew` tables.
   for (int sel = 0; sel < Heap::kNumWeakSelectors; sel++) {
     WeakSelector selector = static_cast<Heap::WeakSelector>(sel);
-    GetWeakTable(Heap::kNew, selector)->Forward(visitor);
     GetWeakTable(Heap::kOld, selector)->Forward(visitor);
   }
+
+  // Isolates might have forwarding tables (used for during snapshoting in
+  // isolate communication).
+  auto table_old = isolate()->forward_table_old();
+  if (table_old != nullptr) table_old->Forward(visitor);
 }
 
 #ifndef PRODUCT

runtime/vm/heap/scavenger.cc

@@ -847,12 +847,8 @@ uword Scavenger::ProcessWeakProperty(RawWeakProperty* raw_weak,
 }
 
 void Scavenger::ProcessWeakReferences() {
-  // Rehash the weak tables now that we know which objects survive this cycle.
-  for (int sel = 0; sel < Heap::kNumWeakSelectors; sel++) {
-    WeakTable* table =
-        heap_->GetWeakTable(Heap::kNew, static_cast<Heap::WeakSelector>(sel));
-    heap_->SetWeakTable(Heap::kNew, static_cast<Heap::WeakSelector>(sel),
-                        WeakTable::NewFrom(table));
+  auto rehash_weak_table = [](WeakTable* table, WeakTable* replacement_new,
+                              WeakTable* replacement_old) {
     intptr_t size = table->size();
     for (intptr_t i = 0; i < size; i++) {
       if (table->IsValidEntryAt(i)) {
@@ -864,16 +860,40 @@ void Scavenger::ProcessWeakReferences() {
           // The object has survived.  Preserve its record.
           uword new_addr = ForwardedAddr(header);
           raw_obj = RawObject::FromAddr(new_addr);
-          heap_->SetWeakEntry(raw_obj, static_cast<Heap::WeakSelector>(sel),
-                              table->ValueAt(i));
+          auto replacement =
+              raw_obj->IsNewObject() ? replacement_new : replacement_old;
+          replacement->SetValue(raw_obj, table->ValueAt(i));
         }
       }
     }
+  };
+
+  // Rehash the weak tables now that we know which objects survive this cycle.
+  for (int sel = 0; sel < Heap::kNumWeakSelectors; sel++) {
+    const auto selector = static_cast<Heap::WeakSelector>(sel);
+    auto table = heap_->GetWeakTable(Heap::kNew, selector);
+    auto table_old = heap_->GetWeakTable(Heap::kOld, selector);
+
+    // Create a new weak table for the new-space.
+    auto table_new = WeakTable::NewFrom(table);
+    rehash_weak_table(table, table_new, table_old);
+    heap_->SetWeakTable(Heap::kNew, selector, table_new);
+
     // Remove the old table as it has been replaced with the newly allocated
     // table above.
     delete table;
   }
 
+  // Each isolate might have a weak table used for fast snapshot writing (i.e.
+  // isolate communication). Rehash those tables if need be.
+  auto isolate = heap_->isolate();
+  auto table = isolate->forward_table_new();
+  if (table != NULL) {
+    auto replacement = WeakTable::NewFrom(table);
+    rehash_weak_table(table, replacement, isolate->forward_table_old());
+    isolate->set_forward_table_new(replacement);
+  }
+
   // The queued weak properties at this point do not refer to reachable keys,
   // so we clear their key and value fields.
   {

runtime/vm/isolate.cc

@@ -2009,6 +2009,15 @@ void Isolate::MaybeIncreaseReloadEveryNStackOverflowChecks() {
 }
 #endif  // !defined(PRODUCT) && !defined(DART_PRECOMPILED_RUNTIME)
 
+void Isolate::set_forward_table_new(WeakTable* table) {
+  std::unique_ptr<WeakTable> value(table);
+  forward_table_new_ = std::move(value);
+}
+void Isolate::set_forward_table_old(WeakTable* table) {
+  std::unique_ptr<WeakTable> value(table);
+  forward_table_old_ = std::move(value);
+}
+
 void Isolate::Shutdown() {
   ASSERT(this == Isolate::Current());
   BackgroundCompiler::Stop(this);

runtime/vm/isolate.h

@@ -90,6 +90,7 @@ class StoreBuffer;
 class StubCode;
 class ThreadRegistry;
 class UserTag;
+class WeakTable;
 
 class PendingLazyDeopt {
  public:
@@ -945,6 +946,14 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
 
   void MaybeIncreaseReloadEveryNStackOverflowChecks();
 
+  // The weak table used in the snapshot writer for the purpose of fast message
+  // sending.
+  WeakTable* forward_table_new() { return forward_table_new_.get(); }
+  void set_forward_table_new(WeakTable* table);
+
+  WeakTable* forward_table_old() { return forward_table_old_.get(); }
+  void set_forward_table_old(WeakTable* table);
+
   static void NotifyLowMemory();
 
  private:
@@ -1176,6 +1185,10 @@ class Isolate : public BaseIsolate, public IntrusiveDListEntry<Isolate> {
 
   ReversePcLookupCache* reverse_pc_lookup_cache_ = nullptr;
 
+  // Used during message sending of messages between isolates.
+  std::unique_ptr<WeakTable> forward_table_new_;
+  std::unique_ptr<WeakTable> forward_table_old_;
+
   static Dart_IsolateGroupCreateCallback create_group_callback_;
   static Dart_InitializeIsolateCallback initialize_callback_;
   static Dart_IsolateShutdownCallback shutdown_callback_;

runtime/vm/snapshot.cc

@@ -959,10 +959,13 @@ ForwardList::ForwardList(Thread* thread, intptr_t first_object_id)
       nodes_(),
       first_unprocessed_object_id_(first_object_id) {
   ASSERT(first_object_id > 0);
+  isolate()->set_forward_table_new(new WeakTable());
+  isolate()->set_forward_table_old(new WeakTable());
 }
 
 ForwardList::~ForwardList() {
-  heap()->ResetObjectIdTable();
+  isolate()->set_forward_table_new(nullptr);
+  isolate()->set_forward_table_old(nullptr);
 }
 
 intptr_t ForwardList::AddObject(Zone* zone,
@@ -976,17 +979,33 @@ intptr_t ForwardList::AddObject(Zone* zone,
   ASSERT(node != NULL);
   nodes_.Add(node);
   ASSERT(object_id != 0);
-  heap()->SetObjectId(raw, object_id);
+  SetObjectId(raw, object_id);
   return object_id;
 }
 
 intptr_t ForwardList::FindObject(RawObject* raw) {
   NoSafepointScope no_safepoint;
-  intptr_t id = heap()->GetObjectId(raw);
+  intptr_t id = GetObjectId(raw);
   ASSERT(id == 0 || NodeForObjectId(id)->obj()->raw() == raw);
   return (id == 0) ? static_cast<intptr_t>(kInvalidIndex) : id;
 }
 
+void ForwardList::SetObjectId(RawObject* object, intptr_t id) {
+  if (object->IsNewObject()) {
+    isolate()->forward_table_new()->SetValue(object, id);
+  } else {
+    isolate()->forward_table_old()->SetValue(object, id);
+  }
+}
+
+intptr_t ForwardList::GetObjectId(RawObject* object) {
+  if (object->IsNewObject()) {
+    return isolate()->forward_table_new()->GetValue(object);
+  } else {
+    return isolate()->forward_table_old()->GetValue(object);
+  }
+}
+
 bool SnapshotWriter::CheckAndWritePredefinedObject(RawObject* rawobj) {
   // Check if object can be written in one of the following ways:
   // - Smi: the Smi value is written as is (last bit is not tagged).

runtime/vm/snapshot.h

@@ -605,13 +605,16 @@ class ForwardList {
  private:
   intptr_t first_object_id() const { return first_object_id_; }
   intptr_t next_object_id() const { return nodes_.length() + first_object_id_; }
-  Heap* heap() const { return thread_->isolate()->heap(); }
+  Isolate* isolate() const { return thread_->isolate(); }
 
   Thread* thread_;
   const intptr_t first_object_id_;
   GrowableArray<Node*> nodes_;
   intptr_t first_unprocessed_object_id_;
 
+  void SetObjectId(RawObject* object, intptr_t id);
+  intptr_t GetObjectId(RawObject* object);
+
   DISALLOW_COPY_AND_ASSIGN(ForwardList);
 };