[vm/concurrency] Lock weak table to prepare for concurrent access.

Right now there is only one mutator working on a heap and it can
therefore access the heap's weak maps without locking. The GC is the
only other user of the weak maps and it accesses it within a safepoint
operation scope.

Once we move the heap to the isolate group there can be concurrent
accesses to the weak maps. As a preparation step this CL adds locking
around the non-GC API.

Issue #36097

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

Author: mkustermann

runtime/vm/heap/heap.cc

@@ -885,10 +885,10 @@ void Heap::ForwardWeakEntries(RawObject* before_object,
   for (int sel = 0; sel < Heap::kNumWeakSelectors; sel++) {
     const auto selector = static_cast<Heap::WeakSelector>(sel);
     auto before_table = GetWeakTable(before_space, selector);
-    intptr_t entry = before_table->RemoveValue(before_object);
+    intptr_t entry = before_table->RemoveValueExclusive(before_object);
     if (entry != 0) {
       auto after_table = GetWeakTable(after_space, selector);
-      after_table->SetValue(after_object, entry);
+      after_table->SetValueExclusive(after_object, entry);
     }
   }
 

runtime/vm/heap/marker.cc

@@ -501,11 +501,11 @@ void GCMarker::ProcessWeakTables(PageSpace* page_space) {
         heap_->GetWeakTable(Heap::kOld, static_cast<Heap::WeakSelector>(sel));
     intptr_t size = table->size();
     for (intptr_t i = 0; i < size; i++) {
-      if (table->IsValidEntryAt(i)) {
-        RawObject* raw_obj = table->ObjectAt(i);
+      if (table->IsValidEntryAtExclusive(i)) {
+        RawObject* raw_obj = table->ObjectAtExclusive(i);
         ASSERT(raw_obj->IsHeapObject());
         if (!raw_obj->IsMarked()) {
-          table->InvalidateAt(i);
+          table->InvalidateAtExclusive(i);
         }
       }
     }

runtime/vm/heap/scavenger.cc

@@ -851,8 +851,8 @@ void Scavenger::ProcessWeakReferences() {
                               WeakTable* replacement_old) {
     intptr_t size = table->size();
     for (intptr_t i = 0; i < size; i++) {
-      if (table->IsValidEntryAt(i)) {
-        RawObject* raw_obj = table->ObjectAt(i);
+      if (table->IsValidEntryAtExclusive(i)) {
+        RawObject* raw_obj = table->ObjectAtExclusive(i);
         ASSERT(raw_obj->IsHeapObject());
         uword raw_addr = RawObject::ToAddr(raw_obj);
         uword header = *reinterpret_cast<uword*>(raw_addr);
@@ -862,7 +862,7 @@ void Scavenger::ProcessWeakReferences() {
           raw_obj = RawObject::FromAddr(new_addr);
           auto replacement =
               raw_obj->IsNewObject() ? replacement_new : replacement_old;
-          replacement->SetValue(raw_obj, table->ValueAt(i));
+          replacement->SetValueExclusive(raw_obj, table->ValueAtExclusive(i));
         }
       }
     }

runtime/vm/heap/weak_table.cc

@@ -29,11 +29,11 @@ intptr_t WeakTable::SizeFor(intptr_t count, intptr_t size) {
   return result;
 }
 
-void WeakTable::SetValue(RawObject* key, intptr_t val) {
+void WeakTable::SetValueExclusive(RawObject* key, intptr_t val) {
   intptr_t mask = size() - 1;
   intptr_t idx = Hash(key) & mask;
   intptr_t empty_idx = -1;
-  RawObject* obj = ObjectAt(idx);
+  RawObject* obj = ObjectAtExclusive(idx);
 
   while (obj != NULL) {
     if (obj == key) {
@@ -44,7 +44,7 @@ void WeakTable::SetValue(RawObject* key, intptr_t val) {
       empty_idx = idx;  // Insert at this location if not found.
     }
     idx = (idx + 1) & mask;
-    obj = ObjectAt(idx);
+    obj = ObjectAtExclusive(idx);
   }
 
   if (val == 0) {
@@ -60,7 +60,7 @@ void WeakTable::SetValue(RawObject* key, intptr_t val) {
     idx = empty_idx;
   }
 
-  ASSERT(!IsValidEntryAt(idx));
+  ASSERT(!IsValidEntryAtExclusive(idx));
   // Set the key and value.
   SetObjectAt(idx, key);
   SetValueAt(idx, val);
@@ -87,7 +87,7 @@ void WeakTable::Forward(ObjectPointerVisitor* visitor) {
   if (used_ == 0) return;
 
   for (intptr_t i = 0; i < size_; i++) {
-    if (IsValidEntryAt(i)) {
+    if (IsValidEntryAtExclusive(i)) {
       visitor->VisitPointer(ObjectPointerAt(i));
     }
   }
@@ -107,9 +107,9 @@ void WeakTable::Rehash() {
   intptr_t mask = new_size - 1;
   set_used(0);
   for (intptr_t i = 0; i < old_size; i++) {
-    if (IsValidEntryAt(i)) {
+    if (IsValidEntryAtExclusive(i)) {
       // Find the new hash location for this entry.
-      RawObject* key = ObjectAt(i);
+      RawObject* key = ObjectAtExclusive(i);
       intptr_t idx = Hash(key) & mask;
       RawObject* obj = reinterpret_cast<RawObject*>(new_data[ObjectIndex(idx)]);
       while (obj != NULL) {
@@ -119,7 +119,7 @@ void WeakTable::Rehash() {
       }
 
       new_data[ObjectIndex(idx)] = reinterpret_cast<intptr_t>(key);
-      new_data[ValueIndex(idx)] = ValueAt(i);
+      new_data[ValueIndex(idx)] = ValueAtExclusive(i);
       set_used(used() + 1);
     }
   }

runtime/vm/heap/weak_table.h

@@ -8,6 +8,7 @@
 #include "vm/globals.h"
 
 #include "platform/assert.h"
+#include "vm/lockers.h"
 #include "vm/raw_object.h"
 
 namespace dart {
@@ -46,64 +47,82 @@ class WeakTable {
   intptr_t used() const { return used_; }
   intptr_t count() const { return count_; }
 
-  bool IsValidEntryAt(intptr_t i) const {
-    ASSERT(((ValueAt(i) == 0) && ((ObjectAt(i) == NULL) ||
-                                  (data_[ObjectIndex(i)] == kDeletedEntry))) ||
-           ((ValueAt(i) != 0) && (ObjectAt(i) != NULL) &&
-            (data_[ObjectIndex(i)] != kDeletedEntry)));
+  // The following methods can be called concurrently and are guarded by a lock.
+
+  intptr_t GetValue(RawObject* key) {
+    MutexLocker ml(&mutex_);
+    return GetValueExclusive(key);
+  }
+
+  void SetValue(RawObject* key, intptr_t val) {
+    MutexLocker ml(&mutex_);
+    return SetValueExclusive(key, val);
+  }
+
+  // The following "exclusive" methods must only be called from call sites
+  // which are known to have exclusive access to the weak table.
+  //
+  // This is mostly limited to GC related code (e.g. scavenger, marker, ...)
+
+  bool IsValidEntryAtExclusive(intptr_t i) const {
+    ASSERT((ValueAtExclusive(i) == 0 &&
+            (ObjectAtExclusive(i) == NULL ||
+             data_[ObjectIndex(i)] == kDeletedEntry)) ||
+           (ValueAtExclusive(i) != 0 && ObjectAtExclusive(i) != NULL &&
+            data_[ObjectIndex(i)] != kDeletedEntry));
     return (data_[ValueIndex(i)] != 0);
   }
 
-  void InvalidateAt(intptr_t i) {
-    ASSERT(IsValidEntryAt(i));
+  void InvalidateAtExclusive(intptr_t i) {
+    ASSERT(IsValidEntryAtExclusive(i));
     SetValueAt(i, 0);
   }
 
-  RawObject* ObjectAt(intptr_t i) const {
+  RawObject* ObjectAtExclusive(intptr_t i) const {
     ASSERT(i >= 0);
     ASSERT(i < size());
     return reinterpret_cast<RawObject*>(data_[ObjectIndex(i)]);
   }
 
-  intptr_t ValueAt(intptr_t i) const {
+  intptr_t ValueAtExclusive(intptr_t i) const {
     ASSERT(i >= 0);
     ASSERT(i < size());
     return data_[ValueIndex(i)];
   }
 
-  void SetValue(RawObject* key, intptr_t val);
+  void SetValueExclusive(RawObject* key, intptr_t val);
 
-  intptr_t GetValue(RawObject* key) const {
+  intptr_t GetValueExclusive(RawObject* key) const {
     intptr_t mask = size() - 1;
     intptr_t idx = Hash(key) & mask;
-    RawObject* obj = ObjectAt(idx);
+    RawObject* obj = ObjectAtExclusive(idx);
     while (obj != NULL) {
       if (obj == key) {
-        return ValueAt(idx);
+        return ValueAtExclusive(idx);
       }
       idx = (idx + 1) & mask;
-      obj = ObjectAt(idx);
+      obj = ObjectAtExclusive(idx);
     }
-    ASSERT(ValueAt(idx) == 0);
+    ASSERT(ValueAtExclusive(idx) == 0);
     return 0;
   }
 
   // Removes and returns the value associated with |key|. Returns 0 if there is
   // no value associated with |key|.
-  intptr_t RemoveValue(RawObject* key) {
+  intptr_t RemoveValueExclusive(RawObject* key) {
     intptr_t mask = size() - 1;
     intptr_t idx = Hash(key) & mask;
-    RawObject* obj = ObjectAt(idx);
+    RawObject* obj = ObjectAtExclusive(idx);
     while (obj != NULL) {
       if (obj == key) {
-        intptr_t result = ValueAt(idx);
-        InvalidateAt(idx);
+        intptr_t result = ValueAtExclusive(idx);
+        InvalidateAtExclusive(idx);
         return result;
       }
       idx = (idx + 1) & mask;
-      obj = ObjectAt(idx);
+      obj = ObjectAtExclusive(idx);
     }
-    ASSERT(ValueAt(idx) == 0);
+    ASSERT(ValueAtExclusive(idx) == 0);
     return 0;
   }
 
@@ -174,6 +193,8 @@ class WeakTable {
     return reinterpret_cast<uintptr_t>(key) * 92821;
   }
 
+  Mutex mutex_;
+
   // data_ contains size_ tuples of key/value.
   intptr_t* data_;
   // size_ keeps the number of entries in data_. used_ maintains the number of