Skip to content

Commit 91c9e27

Browse files
committed
Core: Fix lock-free implementation and release / acquire semantics
Updated `_allocate_rid` to use release when storing max, ensuring earlier stores cannot be reordered **after**. Update of `owns` and `get_or_null` using acquire when loading max, ensuring later reads cannot be ordered **before**.
1 parent 79033f1 commit 91c9e27

1 file changed

Lines changed: 31 additions & 66 deletions

File tree

core/templates/rid_owner.h

Lines changed: 31 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -152,8 +152,8 @@ class RID_Alloc : public RID_AllocBase {
152152
}
153153

154154
if constexpr (THREAD_SAFE) {
155-
// Store atomically to avoid data race with the load in get_or_null().
156-
((std::atomic<uint32_t> *)&max_alloc)->store(max_alloc + elements_in_chunk, std::memory_order_relaxed);
155+
// Release ensures all chunk initialization is visible to acquire-load readers.
156+
((std::atomic<uint32_t> *)&max_alloc)->store(max_alloc + elements_in_chunk, std::memory_order_release);
157157
} else {
158158
max_alloc += elements_in_chunk;
159159
}
@@ -169,8 +169,8 @@ class RID_Alloc : public RID_AllocBase {
169169
id <<= 32;
170170
id |= free_index;
171171

172-
chunks[free_chunk][free_element].validator = validator;
173-
chunks[free_chunk][free_element].validator |= 0x80000000; //mark uninitialized bit
172+
// Release ensures validator is visible to acquire-load readers.
173+
((std::atomic<uint32_t> *)&chunks[free_chunk][free_element].validator)->store(validator | 0x80000000, std::memory_order_release);
174174

175175
alloc_count++;
176176

@@ -203,15 +203,12 @@ class RID_Alloc : public RID_AllocBase {
203203
return nullptr;
204204
}
205205

206-
if constexpr (THREAD_SAFE) {
207-
SYNC_ACQUIRE;
208-
}
209-
210206
uint64_t id = p_rid.get_id();
211207
uint32_t idx = uint32_t(id & 0xFFFFFFFF);
212208
uint32_t ma;
213-
if constexpr (THREAD_SAFE) { // Read atomically to avoid data race with the store in _allocate_rid().
214-
ma = ((std::atomic<uint32_t> *)&max_alloc)->load(std::memory_order_relaxed);
209+
if constexpr (THREAD_SAFE) {
210+
// Acquire synchronizes with release-store in _allocate_rid(), ensuring chunk data is visible.
211+
ma = ((std::atomic<uint32_t> *)&max_alloc)->load(std::memory_order_acquire);
215212
} else {
216213
ma = max_alloc;
217214
}
@@ -224,50 +221,38 @@ class RID_Alloc : public RID_AllocBase {
224221

225222
uint32_t validator = uint32_t(id >> 32);
226223

224+
uint32_t cur_validator;
227225
if constexpr (THREAD_SAFE) {
228-
#ifdef TSAN_ENABLED
229-
__tsan_acquire(&chunks[idx_chunk]); // We know not a race in practice.
230-
__tsan_acquire(&chunks[idx_chunk][idx_element]); // We know not a race in practice.
231-
#endif
232-
}
233-
234-
Chunk &c = chunks[idx_chunk][idx_element];
235-
236-
if constexpr (THREAD_SAFE) {
237-
#ifdef TSAN_ENABLED
238-
__tsan_release(&chunks[idx_chunk]);
239-
__tsan_release(&chunks[idx_chunk][idx_element]);
240-
__tsan_acquire(&c.validator); // We know not a race in practice.
241-
#endif
226+
// Acquire synchronizes with release-store in _allocate_rid().
227+
cur_validator = ((std::atomic<uint32_t> *)&chunks[idx_chunk][idx_element].validator)->load(std::memory_order_acquire);
228+
} else {
229+
cur_validator = chunks[idx_chunk][idx_element].validator;
242230
}
243231

244232
if (unlikely(p_initialize)) {
245-
if (unlikely(!(c.validator & 0x80000000))) {
233+
if (unlikely(!(cur_validator & 0x80000000))) {
246234
ERR_FAIL_V_MSG(nullptr, "Initializing already initialized RID");
247235
}
248236

249-
if (unlikely((c.validator & 0x7FFFFFFF) != validator)) {
237+
if (unlikely((cur_validator & 0x7FFFFFFF) != validator)) {
250238
ERR_FAIL_V_MSG(nullptr, "Attempting to initialize the wrong RID");
251239
}
252240

253-
c.validator &= 0x7FFFFFFF; //initialized
241+
if constexpr (THREAD_SAFE) {
242+
// Release ensures initialized data is visible before clearing init bit.
243+
((std::atomic<uint32_t> *)&chunks[idx_chunk][idx_element].validator)->store(cur_validator & 0x7FFFFFFF, std::memory_order_release);
244+
} else {
245+
chunks[idx_chunk][idx_element].validator = cur_validator & 0x7FFFFFFF;
246+
}
254247

255-
} else if (unlikely(c.validator != validator)) {
256-
if ((c.validator & 0x80000000) && c.validator != 0xFFFFFFFF) {
248+
} else if (unlikely(cur_validator != validator)) {
249+
if ((cur_validator & 0x80000000) && cur_validator != 0xFFFFFFFF) {
257250
ERR_FAIL_V_MSG(nullptr, "Attempting to use an uninitialized RID");
258251
}
259252
return nullptr;
260253
}
261254

262-
if constexpr (THREAD_SAFE) {
263-
#ifdef TSAN_ENABLED
264-
__tsan_release(&c.validator);
265-
#endif
266-
}
267-
268-
T *ptr = &c.data;
269-
270-
return ptr;
255+
return &chunks[idx_chunk][idx_element].data;
271256
}
272257
void initialize_rid(RID p_rid) {
273258
T *mem = get_or_null(p_rid, true);
@@ -314,15 +299,12 @@ class RID_Alloc : public RID_AllocBase {
314299
return false;
315300
}
316301

317-
if constexpr (THREAD_SAFE) {
318-
SYNC_ACQUIRE;
319-
}
320-
321302
uint64_t id = p_rid.get_id();
322303
uint32_t idx = uint32_t(id & 0xFFFFFFFF);
323304
uint32_t ma;
324305
if constexpr (THREAD_SAFE) {
325-
ma = ((std::atomic<uint32_t> *)&max_alloc)->load(std::memory_order_relaxed);
306+
// Acquire synchronizes with release-store in _allocate_rid(), ensuring chunk data is visible.
307+
ma = ((std::atomic<uint32_t> *)&max_alloc)->load(std::memory_order_acquire);
326308
} else {
327309
ma = max_alloc;
328310
}
@@ -336,32 +318,15 @@ class RID_Alloc : public RID_AllocBase {
336318

337319
uint32_t validator = uint32_t(id >> 32);
338320

321+
uint32_t cur_validator;
339322
if constexpr (THREAD_SAFE) {
340-
#ifdef TSAN_ENABLED
341-
__tsan_acquire(&chunks[idx_chunk]); // We know not a race in practice.
342-
__tsan_acquire(&chunks[idx_chunk][idx_element]); // We know not a race in practice.
343-
#endif
344-
}
345-
346-
Chunk &c = chunks[idx_chunk][idx_element];
347-
348-
if constexpr (THREAD_SAFE) {
349-
#ifdef TSAN_ENABLED
350-
__tsan_release(&chunks[idx_chunk]);
351-
__tsan_release(&chunks[idx_chunk][idx_element]);
352-
__tsan_acquire(&c.validator); // We know not a race in practice.
353-
#endif
354-
}
355-
356-
bool owned = (c.validator & 0x7FFFFFFF) == validator;
357-
358-
if constexpr (THREAD_SAFE) {
359-
#ifdef TSAN_ENABLED
360-
__tsan_release(&c.validator);
361-
#endif
323+
// Acquire synchronizes with release-store in _allocate_rid().
324+
cur_validator = ((std::atomic<uint32_t> *)&chunks[idx_chunk][idx_element].validator)->load(std::memory_order_acquire);
325+
} else {
326+
cur_validator = chunks[idx_chunk][idx_element].validator;
362327
}
363328

364-
return owned;
329+
return (cur_validator & 0x7FFFFFFF) == validator;
365330
}
366331

367332
_FORCE_INLINE_ void free(const RID &p_rid) {

0 commit comments

Comments
 (0)