From 22aababaed7716d502ee80ae3201a1ec5536c428 Mon Sep 17 00:00:00 2001 From: Marijn Suijten Date: Sat, 28 May 2022 20:04:44 +0200 Subject: [PATCH] ndk-glue: Switch to `parking_lot` for `map`pable and `Send`able lock guards It has come up previously that the `native_window()` function is easier to use if we could transpose a `LockGuard>` into an `Option>`. After all, as soon as you have the lock you only need to check the `Option<>` value once and are thereafter guaranteed that its value won't change, until the lock is given up. At the same time `parking_lot` has a `send_guard` feature which allows moving a lock guard to another thread (as long as `deadlock_detection` is disabled); a use case for this recently came up [in glutin] where the `NativeWindow` handle lock should be stored with a GL context so that our `fn on_window_destroyed()` callback doesn't return until after the `Surface` created on it is removed from the context and destroyed. Glutin forces this context to be `Send`, and a lock guard already allows `Sync` if `NativeWindow` is `Sync` (which it is). [in glutin]: https://github.com/rust-windowing/glutin/pull/1411#discussion_r883677680 --- ndk-glue/CHANGELOG.md | 1 + ndk-glue/Cargo.toml | 7 ++++--- ndk-glue/src/lib.rs | 37 +++++++++++++++++++++++++------------ 3 files changed, 30 insertions(+), 15 deletions(-) diff --git a/ndk-glue/CHANGELOG.md b/ndk-glue/CHANGELOG.md index eb95b776..1227e014 100644 --- a/ndk-glue/CHANGELOG.md +++ b/ndk-glue/CHANGELOG.md @@ -1,6 +1,7 @@ # Unreleased - **Breaking:** Provide a `LockReadGuard` newtype around `NativeWindow`/`InputQueue` to hide the underlying lock implementation. (#288) +- **Breaking:** Transpose `LockReadGuard>` into `Option>` to only necessitate an `Option` unpack/`unwrap()` once. (#282) # 0.6.2 (2022-04-19) diff --git a/ndk-glue/Cargo.toml b/ndk-glue/Cargo.toml index 8eca8e65..22ca3eef 100644 --- a/ndk-glue/Cargo.toml +++ b/ndk-glue/Cargo.toml @@ -12,14 +12,15 @@ homepage = "https://github.com/rust-windowing/android-ndk-rs" repository = "https://github.com/rust-windowing/android-ndk-rs" [dependencies] +android_logger = { version = "0.11", optional = true } +libc = "0.2.84" +log = "0.4.14" ndk = { path = "../ndk", version = "0.6.0" } ndk-context = { path = "../ndk-context", version = "0.1.1" } ndk-macro = { path = "../ndk-macro", version = "0.3.0" } ndk-sys = { path = "../ndk-sys", version = "0.3.0" } once_cell = "1" -libc = "0.2.84" -log = "0.4.14" -android_logger = { version = "0.11", optional = true } +parking_lot = "0.12" [features] default = [] diff --git a/ndk-glue/src/lib.rs b/ndk-glue/src/lib.rs index d8222fd9..bb625b8e 100644 --- a/ndk-glue/src/lib.rs +++ b/ndk-glue/src/lib.rs @@ -7,6 +7,7 @@ use ndk::native_activity::NativeActivity; use ndk::native_window::NativeWindow; use ndk_sys::{AInputQueue, ANativeActivity, ANativeWindow, ARect}; use once_cell::sync::Lazy; +use parking_lot::{MappedRwLockReadGuard, RwLock, RwLockReadGuard}; use std::ffi::{CStr, CString}; use std::fmt; use std::fs::File; @@ -15,7 +16,7 @@ use std::ops::Deref; use std::os::raw; use std::os::unix::prelude::*; use std::ptr::NonNull; -use std::sync::{Arc, Condvar, Mutex, RwLock, RwLockReadGuard}; +use std::sync::{Arc, Condvar, Mutex}; use std::thread; #[cfg(feature = "logger")] @@ -67,7 +68,20 @@ pub fn native_activity() -> &'static NativeActivity { unsafe { NATIVE_ACTIVITY.as_ref().unwrap() } } -pub struct LockReadGuard(RwLockReadGuard<'static, T>); +pub struct LockReadGuard(MappedRwLockReadGuard<'static, T>); + +impl LockReadGuard { + /// Transpose an [`Option`] wrapped inside a [`LockReadGuard`] + /// + /// This is a _read_ lock for which the contents can't change; hence allowing the user to only + /// check for [`None`] once and hold a a lock containing `T` directly thereafter, without + /// subsequent infallible [`Option::unwrap()`]s. + fn from_wrapped_option(wrapped: RwLockReadGuard<'static, Option>) -> Option { + RwLockReadGuard::try_map(wrapped, Option::as_ref) + .ok() + .map(Self) + } +} impl Deref for LockReadGuard { type Target = T; @@ -102,8 +116,8 @@ impl fmt::Display for LockReadGuard { /// # Warning /// This function accesses a `static` variable internally and must only be used if you are sure /// there is exactly one version of `ndk_glue` in your dependency tree. -pub fn native_window() -> LockReadGuard> { - LockReadGuard(NATIVE_WINDOW.read().unwrap()) +pub fn native_window() -> Option> { + LockReadGuard::from_wrapped_option(NATIVE_WINDOW.read()) } /// Returns an [`InputQueue`] held inside a lock, preventing Android from freeing it immediately @@ -117,14 +131,14 @@ pub fn native_window() -> LockReadGuard> { /// # Warning /// This function accesses a `static` variable internally and must only be used if you are sure /// there is exactly one version of `ndk_glue` in your dependency tree. -pub fn input_queue() -> LockReadGuard> { - LockReadGuard(INPUT_QUEUE.read().unwrap()) +pub fn input_queue() -> Option> { + LockReadGuard::from_wrapped_option(INPUT_QUEUE.read()) } /// This function accesses a `static` variable internally and must only be used if you are sure /// there is exactly one version of `ndk_glue` in your dependency tree. pub fn content_rect() -> Rect { - CONTENT_RECT.read().unwrap().clone() + CONTENT_RECT.read().clone() } static PIPE: Lazy<[RawFd; 2]> = Lazy::new(|| { @@ -345,7 +359,6 @@ unsafe extern "C" fn on_window_focus_changed( unsafe extern "C" fn on_window_created(activity: *mut ANativeActivity, window: *mut ANativeWindow) { NATIVE_WINDOW .write() - .unwrap() .replace(NativeWindow::clone_from_ptr(NonNull::new(window).unwrap())); wake(activity, Event::WindowCreated); } @@ -369,7 +382,7 @@ unsafe extern "C" fn on_window_destroyed( window: *mut ANativeWindow, ) { wake(activity, Event::WindowDestroyed); - let mut native_window_guard = NATIVE_WINDOW.write().unwrap(); + let mut native_window_guard = NATIVE_WINDOW.write(); assert_eq!(native_window_guard.as_ref().unwrap().ptr().as_ptr(), window); native_window_guard.take(); } @@ -384,7 +397,7 @@ unsafe extern "C" fn on_input_queue_created( // future code cleans it up and sets it back to `None` again. let looper = locked_looper.as_ref().expect("Looper does not exist"); input_queue.attach_looper(looper, NDK_GLUE_LOOPER_INPUT_QUEUE_IDENT); - INPUT_QUEUE.write().unwrap().replace(input_queue); + INPUT_QUEUE.write().replace(input_queue); wake(activity, Event::InputQueueCreated); } @@ -393,7 +406,7 @@ unsafe extern "C" fn on_input_queue_destroyed( queue: *mut AInputQueue, ) { wake(activity, Event::InputQueueDestroyed); - let mut input_queue_guard = INPUT_QUEUE.write().unwrap(); + let mut input_queue_guard = INPUT_QUEUE.write(); assert_eq!(input_queue_guard.as_ref().unwrap().ptr().as_ptr(), queue); let input_queue = InputQueue::from_ptr(NonNull::new(queue).unwrap()); input_queue.detach_looper(); @@ -407,6 +420,6 @@ unsafe extern "C" fn on_content_rect_changed(activity: *mut ANativeActivity, rec right: (*rect).right as _, bottom: (*rect).bottom as _, }; - *CONTENT_RECT.write().unwrap() = rect; + *CONTENT_RECT.write() = rect; wake(activity, Event::ContentRectChanged); }