Skip to content

Commit 6717bfe

Browse files
mkustermanncommit-bot@chromium.org
mkustermann
authored and
[email protected]
committed
[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]>
1 parent 8329968 commit 6717bfe

File tree

5 files changed

+58
-37
lines changed

5 files changed

+58
-37
lines changed

‎runtime/vm/heap/heap.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -885,10 +885,10 @@ void Heap::ForwardWeakEntries(RawObject* before_object,
885885
for (int sel = 0; sel < Heap::kNumWeakSelectors; sel++) {
886886
const auto selector = static_cast<Heap::WeakSelector>(sel);
887887
auto before_table = GetWeakTable(before_space, selector);
888-
intptr_t entry = before_table->RemoveValue(before_object);
888+
intptr_t entry = before_table->RemoveValueExclusive(before_object);
889889
if (entry != 0) {
890890
auto after_table = GetWeakTable(after_space, selector);
891-
after_table->SetValue(after_object, entry);
891+
after_table->SetValueExclusive(after_object, entry);
892892
}
893893
}
894894

‎runtime/vm/heap/marker.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -501,11 +501,11 @@ void GCMarker::ProcessWeakTables(PageSpace* page_space) {
501501
heap_->GetWeakTable(Heap::kOld, static_cast<Heap::WeakSelector>(sel));
502502
intptr_t size = table->size();
503503
for (intptr_t i = 0; i < size; i++) {
504-
if (table->IsValidEntryAt(i)) {
505-
RawObject* raw_obj = table->ObjectAt(i);
504+
if (table->IsValidEntryAtExclusive(i)) {
505+
RawObject* raw_obj = table->ObjectAtExclusive(i);
506506
ASSERT(raw_obj->IsHeapObject());
507507
if (!raw_obj->IsMarked()) {
508-
table->InvalidateAt(i);
508+
table->InvalidateAtExclusive(i);
509509
}
510510
}
511511
}

‎runtime/vm/heap/scavenger.cc‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -851,8 +851,8 @@ void Scavenger::ProcessWeakReferences() {
851851
WeakTable* replacement_old) {
852852
intptr_t size = table->size();
853853
for (intptr_t i = 0; i < size; i++) {
854-
if (table->IsValidEntryAt(i)) {
855-
RawObject* raw_obj = table->ObjectAt(i);
854+
if (table->IsValidEntryAtExclusive(i)) {
855+
RawObject* raw_obj = table->ObjectAtExclusive(i);
856856
ASSERT(raw_obj->IsHeapObject());
857857
uword raw_addr = RawObject::ToAddr(raw_obj);
858858
uword header = *reinterpret_cast<uword*>(raw_addr);
@@ -862,7 +862,7 @@ void Scavenger::ProcessWeakReferences() {
862862
raw_obj = RawObject::FromAddr(new_addr);
863863
auto replacement =
864864
raw_obj->IsNewObject() ? replacement_new : replacement_old;
865-
replacement->SetValue(raw_obj, table->ValueAt(i));
865+
replacement->SetValueExclusive(raw_obj, table->ValueAtExclusive(i));
866866
}
867867
}
868868
}

‎runtime/vm/heap/weak_table.cc‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,11 @@ intptr_t WeakTable::SizeFor(intptr_t count, intptr_t size) {
2929
return result;
3030
}
3131

32-
void WeakTable::SetValue(RawObject* key, intptr_t val) {
32+
void WeakTable::SetValueExclusive(RawObject* key, intptr_t val) {
3333
intptr_t mask = size() - 1;
3434
intptr_t idx = Hash(key) & mask;
3535
intptr_t empty_idx = -1;
36-
RawObject* obj = ObjectAt(idx);
36+
RawObject* obj = ObjectAtExclusive(idx);
3737

3838
while (obj != NULL) {
3939
if (obj == key) {
@@ -44,7 +44,7 @@ void WeakTable::SetValue(RawObject* key, intptr_t val) {
4444
empty_idx = idx; // Insert at this location if not found.
4545
}
4646
idx = (idx + 1) & mask;
47-
obj = ObjectAt(idx);
47+
obj = ObjectAtExclusive(idx);
4848
}
4949

5050
if (val == 0) {
@@ -60,7 +60,7 @@ void WeakTable::SetValue(RawObject* key, intptr_t val) {
6060
idx = empty_idx;
6161
}
6262

63-
ASSERT(!IsValidEntryAt(idx));
63+
ASSERT(!IsValidEntryAtExclusive(idx));
6464
// Set the key and value.
6565
SetObjectAt(idx, key);
6666
SetValueAt(idx, val);
@@ -87,7 +87,7 @@ void WeakTable::Forward(ObjectPointerVisitor* visitor) {
8787
if (used_ == 0) return;
8888

8989
for (intptr_t i = 0; i < size_; i++) {
90-
if (IsValidEntryAt(i)) {
90+
if (IsValidEntryAtExclusive(i)) {
9191
visitor->VisitPointer(ObjectPointerAt(i));
9292
}
9393
}
@@ -107,9 +107,9 @@ void WeakTable::Rehash() {
107107
intptr_t mask = new_size - 1;
108108
set_used(0);
109109
for (intptr_t i = 0; i < old_size; i++) {
110-
if (IsValidEntryAt(i)) {
110+
if (IsValidEntryAtExclusive(i)) {
111111
// Find the new hash location for this entry.
112-
RawObject* key = ObjectAt(i);
112+
RawObject* key = ObjectAtExclusive(i);
113113
intptr_t idx = Hash(key) & mask;
114114
RawObject* obj = reinterpret_cast<RawObject*>(new_data[ObjectIndex(idx)]);
115115
while (obj != NULL) {
@@ -119,7 +119,7 @@ void WeakTable::Rehash() {
119119
}
120120

121121
new_data[ObjectIndex(idx)] = reinterpret_cast<intptr_t>(key);
122-
new_data[ValueIndex(idx)] = ValueAt(i);
122+
new_data[ValueIndex(idx)] = ValueAtExclusive(i);
123123
set_used(used() + 1);
124124
}
125125
}

‎runtime/vm/heap/weak_table.h‎

Lines changed: 42 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include "vm/globals.h"
99

1010
#include "platform/assert.h"
11+
#include "vm/lockers.h"
1112
#include "vm/raw_object.h"
1213

1314
namespace dart {
@@ -46,64 +47,82 @@ class WeakTable {
4647
intptr_t used() const { return used_; }
4748
intptr_t count() const { return count_; }
4849

49-
bool IsValidEntryAt(intptr_t i) const {
50-
ASSERT(((ValueAt(i) == 0) && ((ObjectAt(i) == NULL) ||
51-
(data_[ObjectIndex(i)] == kDeletedEntry))) ||
52-
((ValueAt(i) != 0) && (ObjectAt(i) != NULL) &&
53-
(data_[ObjectIndex(i)] != kDeletedEntry)));
50+
// The following methods can be called concurrently and are guarded by a lock.
51+
52+
intptr_t GetValue(RawObject* key) {
53+
MutexLocker ml(&mutex_);
54+
return GetValueExclusive(key);
55+
}
56+
57+
void SetValue(RawObject* key, intptr_t val) {
58+
MutexLocker ml(&mutex_);
59+
return SetValueExclusive(key, val);
60+
}
61+
62+
// The following "exclusive" methods must only be called from call sites
63+
// which are known to have exclusive access to the weak table.
64+
//
65+
// This is mostly limited to GC related code (e.g. scavenger, marker, ...)
66+
67+
bool IsValidEntryAtExclusive(intptr_t i) const {
68+
ASSERT((ValueAtExclusive(i) == 0 &&
69+
(ObjectAtExclusive(i) == NULL ||
70+
data_[ObjectIndex(i)] == kDeletedEntry)) ||
71+
(ValueAtExclusive(i) != 0 && ObjectAtExclusive(i) != NULL &&
72+
data_[ObjectIndex(i)] != kDeletedEntry));
5473
return (data_[ValueIndex(i)] != 0);
5574
}
5675

57-
void InvalidateAt(intptr_t i) {
58-
ASSERT(IsValidEntryAt(i));
76+
void InvalidateAtExclusive(intptr_t i) {
77+
ASSERT(IsValidEntryAtExclusive(i));
5978
SetValueAt(i, 0);
6079
}
6180

62-
RawObject* ObjectAt(intptr_t i) const {
81+
RawObject* ObjectAtExclusive(intptr_t i) const {
6382
ASSERT(i >= 0);
6483
ASSERT(i < size());
6584
return reinterpret_cast<RawObject*>(data_[ObjectIndex(i)]);
6685
}
6786

68-
intptr_t ValueAt(intptr_t i) const {
87+
intptr_t ValueAtExclusive(intptr_t i) const {
6988
ASSERT(i >= 0);
7089
ASSERT(i < size());
7190
return data_[ValueIndex(i)];
7291
}
7392

74-
void SetValue(RawObject* key, intptr_t val);
93+
void SetValueExclusive(RawObject* key, intptr_t val);
7594

76-
intptr_t GetValue(RawObject* key) const {
95+
intptr_t GetValueExclusive(RawObject* key) const {
7796
intptr_t mask = size() - 1;
7897
intptr_t idx = Hash(key) & mask;
79-
RawObject* obj = ObjectAt(idx);
98+
RawObject* obj = ObjectAtExclusive(idx);
8099
while (obj != NULL) {
81100
if (obj == key) {
82-
return ValueAt(idx);
101+
return ValueAtExclusive(idx);
83102
}
84103
idx = (idx + 1) & mask;
85-
obj = ObjectAt(idx);
104+
obj = ObjectAtExclusive(idx);
86105
}
87-
ASSERT(ValueAt(idx) == 0);
106+
ASSERT(ValueAtExclusive(idx) == 0);
88107
return 0;
89108
}
90109

91110
// Removes and returns the value associated with |key|. Returns 0 if there is
92111
// no value associated with |key|.
93-
intptr_t RemoveValue(RawObject* key) {
112+
intptr_t RemoveValueExclusive(RawObject* key) {
94113
intptr_t mask = size() - 1;
95114
intptr_t idx = Hash(key) & mask;
96-
RawObject* obj = ObjectAt(idx);
115+
RawObject* obj = ObjectAtExclusive(idx);
97116
while (obj != NULL) {
98117
if (obj == key) {
99-
intptr_t result = ValueAt(idx);
100-
InvalidateAt(idx);
118+
intptr_t result = ValueAtExclusive(idx);
119+
InvalidateAtExclusive(idx);
101120
return result;
102121
}
103122
idx = (idx + 1) & mask;
104-
obj = ObjectAt(idx);
123+
obj = ObjectAtExclusive(idx);
105124
}
106-
ASSERT(ValueAt(idx) == 0);
125+
ASSERT(ValueAtExclusive(idx) == 0);
107126
return 0;
108127
}
109128

@@ -174,6 +193,8 @@ class WeakTable {
174193
return reinterpret_cast<uintptr_t>(key) * 92821;
175194
}
176195

196+
Mutex mutex_;
197+
177198
// data_ contains size_ tuples of key/value.
178199
intptr_t* data_;
179200
// size_ keeps the number of entries in data_. used_ maintains the number of

0 commit comments

Comments
 (0)