Skip to content

Core: Fix RID_Alloc Lock-Free Reads on Weakly-Ordered Architectures #114937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
stuartcarnie wants to merge 2 commits into from
Closed

Core: Fix RID_Alloc Lock-Free Reads on Weakly-Ordered Architectures #114937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
91c9e27
Core: Fix lock-free implementation and release / acquire semantics
stuartcarnie Jan 14, 2026
8d0f4f9
Build: Update to Xcode 26.2
stuartcarnie Jan 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/ios_builds.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ jobs:

# From https://github.com/actions/runner-images/blob/main/images/macos
- name: Select Xcode 26
run: sudo xcode-select -s /Applications/Xcode_26.0.1.app
run: sudo xcode-select -s /Applications/Xcode_26.2.app

- name: Restore Godot build cache
uses: ./.github/actions/godot-cache-restore
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/macos_builds.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ jobs:

# From https://github.com/actions/runner-images/blob/main/images/macos
- name: Select Xcode 26
run: sudo xcode-select -s /Applications/Xcode_26.0.1.app
run: sudo xcode-select -s /Applications/Xcode_26.2.app

- name: Restore Godot build cache
uses: ./.github/actions/godot-cache-restore
Expand Down
97 changes: 31 additions & 66 deletions core/templates/rid_owner.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,8 +152,8 @@ class RID_Alloc : public RID_AllocBase {
}

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

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

alloc_count++;

Expand Down Expand Up @@ -203,15 +203,12 @@ class RID_Alloc : public RID_AllocBase {
return nullptr;
}

if constexpr (THREAD_SAFE) {
SYNC_ACQUIRE;
}

uint64_t id = p_rid.get_id();
uint32_t idx = uint32_t(id & 0xFFFFFFFF);
uint32_t ma;
if constexpr (THREAD_SAFE) { // Read atomically to avoid data race with the store in _allocate_rid().
ma = ((std::atomic<uint32_t> *)&max_alloc)->load(std::memory_order_relaxed);
if constexpr (THREAD_SAFE) {
// Acquire synchronizes with release-store in _allocate_rid(), ensuring chunk data is visible.
ma = ((std::atomic<uint32_t> *)&max_alloc)->load(std::memory_order_acquire);
} else {
ma = max_alloc;
}
Expand All @@ -224,50 +221,38 @@ class RID_Alloc : public RID_AllocBase {

uint32_t validator = uint32_t(id >> 32);

uint32_t cur_validator;
if constexpr (THREAD_SAFE) {
#ifdef TSAN_ENABLED
__tsan_acquire(&chunks[idx_chunk]); // We know not a race in practice.
__tsan_acquire(&chunks[idx_chunk][idx_element]); // We know not a race in practice.
#endif
}

Chunk &c = chunks[idx_chunk][idx_element];

if constexpr (THREAD_SAFE) {
#ifdef TSAN_ENABLED
__tsan_release(&chunks[idx_chunk]);
__tsan_release(&chunks[idx_chunk][idx_element]);
__tsan_acquire(&c.validator); // We know not a race in practice.
#endif
// Acquire synchronizes with release-store in _allocate_rid().
cur_validator = ((std::atomic<uint32_t> *)&chunks[idx_chunk][idx_element].validator)->load(std::memory_order_acquire);
} else {
cur_validator = chunks[idx_chunk][idx_element].validator;
}

if (unlikely(p_initialize)) {
if (unlikely(!(c.validator & 0x80000000))) {
if (unlikely(!(cur_validator & 0x80000000))) {
ERR_FAIL_V_MSG(nullptr, "Initializing already initialized RID");
}

if (unlikely((c.validator & 0x7FFFFFFF) != validator)) {
if (unlikely((cur_validator & 0x7FFFFFFF) != validator)) {
ERR_FAIL_V_MSG(nullptr, "Attempting to initialize the wrong RID");
}

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

} else if (unlikely(c.validator != validator)) {
if ((c.validator & 0x80000000) && c.validator != 0xFFFFFFFF) {
} else if (unlikely(cur_validator != validator)) {
if ((cur_validator & 0x80000000) && cur_validator != 0xFFFFFFFF) {
ERR_FAIL_V_MSG(nullptr, "Attempting to use an uninitialized RID");
}
return nullptr;
}

if constexpr (THREAD_SAFE) {
#ifdef TSAN_ENABLED
__tsan_release(&c.validator);
#endif
}

T *ptr = &c.data;

return ptr;
return &chunks[idx_chunk][idx_element].data;
}
void initialize_rid(RID p_rid) {
T *mem = get_or_null(p_rid, true);
Expand Down Expand Up @@ -314,15 +299,12 @@ class RID_Alloc : public RID_AllocBase {
return false;
}

if constexpr (THREAD_SAFE) {
SYNC_ACQUIRE;
}

uint64_t id = p_rid.get_id();
uint32_t idx = uint32_t(id & 0xFFFFFFFF);
uint32_t ma;
if constexpr (THREAD_SAFE) {
ma = ((std::atomic<uint32_t> *)&max_alloc)->load(std::memory_order_relaxed);
// Acquire synchronizes with release-store in _allocate_rid(), ensuring chunk data is visible.
ma = ((std::atomic<uint32_t> *)&max_alloc)->load(std::memory_order_acquire);
} else {
ma = max_alloc;
}
Expand All @@ -336,32 +318,15 @@ class RID_Alloc : public RID_AllocBase {

uint32_t validator = uint32_t(id >> 32);

uint32_t cur_validator;
if constexpr (THREAD_SAFE) {
#ifdef TSAN_ENABLED
__tsan_acquire(&chunks[idx_chunk]); // We know not a race in practice.
__tsan_acquire(&chunks[idx_chunk][idx_element]); // We know not a race in practice.
#endif
}

Chunk &c = chunks[idx_chunk][idx_element];

if constexpr (THREAD_SAFE) {
#ifdef TSAN_ENABLED
__tsan_release(&chunks[idx_chunk]);
__tsan_release(&chunks[idx_chunk][idx_element]);
__tsan_acquire(&c.validator); // We know not a race in practice.
#endif
}

bool owned = (c.validator & 0x7FFFFFFF) == validator;

if constexpr (THREAD_SAFE) {
#ifdef TSAN_ENABLED
__tsan_release(&c.validator);
#endif
// Acquire synchronizes with release-store in _allocate_rid().
cur_validator = ((std::atomic<uint32_t> *)&chunks[idx_chunk][idx_element].validator)->load(std::memory_order_acquire);
} else {
cur_validator = chunks[idx_chunk][idx_element].validator;
}

return owned;
return (cur_validator & 0x7FFFFFFF) == validator;
}

_FORCE_INLINE_ void free(const RID &p_rid) {
Expand Down