replace OnceLock with AtomicPtr in NATIVE_DISPLAY
#525
Conversation
- reduce locking overhead by replacing `OnceLock<Mutex<T>>` with `AtomicPtr<Mutex<T>>`. - add `drop_display` to allow safely resetting `NATIVE_DISPLAY` while ensuring proper deallocation. - implement `Drop` for `GlContext` and `MetalContext` to guarantee cleanup on context destruction. - ensure memory safety using `Release/Acquire` ordering and explicit deallocation. - add `native_display_blocking` and `native_display_nonblocking` for safer access. - update documentation and rustfmt.
|
You have a Mutex, so it makes no sense to wrap the Mutex in an AtomicPtr. Why not just do this? /// This for now is Android specific since the process can continue running but the display
/// is restarted. We support reinitializing the display.
fn set_or_replace_display(display: native::NativeDisplayData) {
if let Some(m) = NATIVE_DISPLAY.get() {
// Replace existing display
*m.lock().unwrap() = display;
} else {
// First time initialization
set_display(display);
}
} |
|
@narodnik A single mutex can handle updates but cannot safely manage dropping. The core issue is twofold: holding the mutex during drop risks deadlocks (particularly if the destructor requires synchronization), while releasing it first risks use-after-free as other threads might access the data mid-destruction. The proposed atomic solution solves this through two key mechanisms: atomic pointer swaps replace data without holding the mutex during destruction, while the ready-flag prevents access during teardown. Additionally the ready-flag enables lightweight, contention-free initialization checks, while the atomic operations ensure clean state transitions, and the destructors remain safe even when accessing thread-local storage or acquiring other locks. |
I wanted to be able to start and stop a miniquad context and after trying several solutions to make
NATIVE_DISPLAYdroppable (e.g. usingRwLock), I believe this one withAtomicPtrplus an staticAtomicBoolhas a very good balance between performance and security.A simple test:
I also got the idea of the
native_displayfunction wrappers from #505 to improve ergonomics.EDIT: This commit would effectively lower MSRV to 1.64. Related: #524.