Skip to content

Commit ce2b488

Browse files
author
Jeff Niu
authored
[mlir] ThreadLocalCache: make TSAN happy about destructors (#106170)
TSAN warns that `ptr` is read and write without protection in `clearExpiredEntries` and in the destructor of `Owner`. Add an atomic bool to synchronize these without incurring a cost when calling `get`.
1 parent c8cac33 commit ce2b488

File tree

1 file changed

+15
-9
lines changed

1 file changed

+15
-9
lines changed

mlir/include/mlir/Support/ThreadLocalCache.h

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,8 @@ template <typename ValueT>
2727
class ThreadLocalCache {
2828
struct PerInstanceState;
2929

30+
using PointerAndFlag = std::pair<ValueT *, std::atomic<bool>>;
31+
3032
/// The "observer" is owned by a thread-local cache instance. It is
3133
/// constructed the first time a `ThreadLocalCache` instance is accessed by a
3234
/// thread, unless `perInstanceState` happens to get re-allocated to the same
@@ -41,7 +43,8 @@ class ThreadLocalCache {
4143
/// This is the double pointer, explicitly allocated because we need to keep
4244
/// the address stable if the TLC map re-allocates. It is owned by the
4345
/// observer and shared with the value owner.
44-
std::shared_ptr<ValueT *> ptr = std::make_shared<ValueT *>(nullptr);
46+
std::shared_ptr<PointerAndFlag> ptr =
47+
std::make_shared<PointerAndFlag>(std::make_pair(nullptr, false));
4548
/// Because the `Owner` instance that lives inside `PerInstanceState`
4649
/// contains a reference to the double pointer, and likewise this class
4750
/// contains a reference to the value, we need to synchronize destruction of
@@ -62,18 +65,21 @@ class ThreadLocalCache {
6265
/// Save a pointer to the reference and write it to the newly created entry.
6366
Owner(Observer &observer)
6467
: value(std::make_unique<ValueT>()), ptrRef(observer.ptr) {
65-
*observer.ptr = value.get();
68+
observer.ptr->second = true;
69+
observer.ptr->first = value.get();
6670
}
6771
~Owner() {
68-
if (std::shared_ptr<ValueT *> ptr = ptrRef.lock())
69-
*ptr = nullptr;
72+
if (std::shared_ptr<PointerAndFlag> ptr = ptrRef.lock()) {
73+
ptr->first = nullptr;
74+
ptr->second = false;
75+
}
7076
}
7177

7278
Owner(Owner &&) = default;
7379
Owner &operator=(Owner &&) = default;
7480

7581
std::unique_ptr<ValueT> value;
76-
std::weak_ptr<ValueT *> ptrRef;
82+
std::weak_ptr<PointerAndFlag> ptrRef;
7783
};
7884

7985
// Keep a separate shared_ptr protected state that can be acquired atomically
@@ -116,15 +122,15 @@ class ThreadLocalCache {
116122
// back to the data here that is being destroyed.
117123
for (auto &[instance, observer] : *this)
118124
if (std::shared_ptr<PerInstanceState> state = observer.keepalive.lock())
119-
state->remove(*observer.ptr);
125+
state->remove(observer.ptr->first);
120126
}
121127

122128
/// Clear out any unused entries within the map. This method is not
123129
/// thread-safe, and should only be called by the same thread as the cache.
124130
void clearExpiredEntries() {
125131
for (auto it = this->begin(), e = this->end(); it != e;) {
126132
auto curIt = it++;
127-
if (!*curIt->second.ptr)
133+
if (!curIt->second.ptr->second)
128134
this->erase(curIt);
129135
}
130136
}
@@ -142,7 +148,7 @@ class ThreadLocalCache {
142148
// Check for an already existing instance for this thread.
143149
CacheType &staticCache = getStaticCache();
144150
Observer &threadInstance = staticCache[perInstanceState.get()];
145-
if (ValueT *value = *threadInstance.ptr)
151+
if (ValueT *value = threadInstance.ptr->first)
146152
return *value;
147153

148154
// Otherwise, create a new instance for this thread.
@@ -157,7 +163,7 @@ class ThreadLocalCache {
157163
// entries in the static map. The cache is only cleared within the same
158164
// thread to remove the need to lock the cache itself.
159165
staticCache.clearExpiredEntries();
160-
return **threadInstance.ptr;
166+
return *threadInstance.ptr->first;
161167
}
162168
ValueT &operator*() { return get(); }
163169
ValueT *operator->() { return &get(); }

0 commit comments

Comments
 (0)