From 0f3543685efc732897eb800067aeafcccd23cef0 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Mon, 25 Nov 2024 12:15:58 +0100 Subject: [PATCH 1/8] use `FnOnce` instead of `Fn` for the device lost closure --- wgpu-core/src/device/mod.rs | 4 ++-- wgpu/src/context.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs index 18e35206b6..a77b9622a1 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -191,9 +191,9 @@ impl UserClosures { } #[cfg(send_sync)] -pub type DeviceLostCallback = Box; +pub type DeviceLostCallback = Box; #[cfg(not(send_sync))] -pub type DeviceLostCallback = Box; +pub type DeviceLostCallback = Box; pub struct DeviceLostClosureRust { pub callback: DeviceLostCallback, diff --git a/wgpu/src/context.rs b/wgpu/src/context.rs index e9e23a55f3..4a17be7e33 100644 --- a/wgpu/src/context.rs +++ b/wgpu/src/context.rs @@ -777,9 +777,9 @@ pub type SubmittedWorkDoneCallback = Box; #[cfg(not(send_sync))] pub type SubmittedWorkDoneCallback = Box; #[cfg(send_sync)] -pub type DeviceLostCallback = Box; +pub type DeviceLostCallback = Box; #[cfg(not(send_sync))] -pub type DeviceLostCallback = Box; +pub type DeviceLostCallback = Box; /// An object safe variant of [`Context`] implemented by all types that implement [`Context`]. pub(crate) trait DynContext: Debug + WasmNotSendSync { From c3ad7b7bd089e17d50b78bb52732abeb1230f289 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Mon, 25 Nov 2024 13:13:57 +0100 Subject: [PATCH 2/8] remove `DeviceLostClosure` consumed check --- wgpu-core/src/device/mod.rs | 46 ++++--------------------------------- 1 file changed, 4 insertions(+), 42 deletions(-) diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs index a77b9622a1..9f695d2638 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -197,35 +197,17 @@ pub type DeviceLostCallback = Box Self { - let inner = DeviceLostClosureRust { - callback, - consumed: false, - }; + let inner = DeviceLostClosureRust { callback }; Self { inner: DeviceLostClosureInner::Rust { inner }, } @@ -261,18 +240,7 @@ impl DeviceLostClosure { /// /// - Both pointers must point to `'static` data, as the callback may happen at /// an unspecified time. - pub unsafe fn from_c(mut closure: DeviceLostClosureC) -> Self { - // Build an inner with the values from closure, ensuring that - // inner.consumed is false. - let inner = DeviceLostClosureC { - callback: closure.callback, - user_data: closure.user_data, - consumed: false, - }; - - // Mark the original closure as consumed, so we can safely drop it. - closure.consumed = true; - + pub unsafe fn from_c(inner: DeviceLostClosureC) -> Self { Self { inner: DeviceLostClosureInner::C { inner }, } @@ -280,15 +248,9 @@ impl DeviceLostClosure { pub(crate) fn call(self, reason: DeviceLostReason, message: String) { match self.inner { - DeviceLostClosureInner::Rust { mut inner } => { - inner.consumed = true; - - (inner.callback)(reason, message) - } + DeviceLostClosureInner::Rust { inner } => (inner.callback)(reason, message), // SAFETY: the contract of the call to from_c says that this unsafe is sound. - DeviceLostClosureInner::C { mut inner } => unsafe { - inner.consumed = true; - + DeviceLostClosureInner::C { inner } => unsafe { // Ensure message is structured as a null-terminated C string. It only // needs to live as long as the callback invocation. let message = std::ffi::CString::new(message).unwrap(); From 4a9b7d36f06144013e8710c25af69d6d3bd519f7 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Mon, 25 Nov 2024 13:14:09 +0100 Subject: [PATCH 3/8] remove `Dropped` & `ReplacedCallback` variants of `DeviceLostReason` --- tests/tests/device.rs | 50 -------------------------------- wgpu-core/src/device/global.rs | 16 +++------- wgpu-core/src/device/resource.rs | 4 --- wgpu-types/src/lib.rs | 14 --------- 4 files changed, 4 insertions(+), 80 deletions(-) diff --git a/tests/tests/device.rs b/tests/tests/device.rs index 87935c831a..edcb3132e7 100644 --- a/tests/tests/device.rs +++ b/tests/tests/device.rs @@ -625,56 +625,6 @@ static DEVICE_DESTROY_THEN_LOST: GpuTestConfiguration = GpuTestConfiguration::ne ); }); -#[gpu_test] -static DEVICE_DROP_THEN_LOST: GpuTestConfiguration = GpuTestConfiguration::new() - .parameters(TestParameters::default().expect_fail(FailureCase::webgl2())) - .run_sync(|ctx| { - // This test checks that when the device is dropped (such as in a GC), - // the provided DeviceLostClosure is called with reason DeviceLostReason::Dropped. - // Fails on webgl because webgl doesn't implement drop. - static WAS_CALLED: std::sync::atomic::AtomicBool = AtomicBool::new(false); - - // Set a LoseDeviceCallback on the device. - let callback = Box::new(|reason, message| { - WAS_CALLED.store(true, std::sync::atomic::Ordering::SeqCst); - assert_eq!(reason, wgt::DeviceLostReason::Dropped); - assert_eq!(message, "Device dropped."); - }); - ctx.device.set_device_lost_callback(callback); - - drop(ctx); - - assert!( - WAS_CALLED.load(std::sync::atomic::Ordering::SeqCst), - "Device lost callback should have been called." - ); - }); - -#[gpu_test] -static DEVICE_LOST_REPLACED_CALLBACK: GpuTestConfiguration = GpuTestConfiguration::new() - .parameters(TestParameters::default()) - .run_sync(|ctx| { - // This test checks that a device_lost_callback is called when it is - // replaced by another callback. - static WAS_CALLED: AtomicBool = AtomicBool::new(false); - - // Set a LoseDeviceCallback on the device. - let callback = Box::new(|reason, _m| { - WAS_CALLED.store(true, std::sync::atomic::Ordering::SeqCst); - assert_eq!(reason, wgt::DeviceLostReason::ReplacedCallback); - }); - ctx.device.set_device_lost_callback(callback); - - // Replace the callback. - let replacement_callback = Box::new(move |_r, _m| {}); - ctx.device.set_device_lost_callback(replacement_callback); - - assert!( - WAS_CALLED.load(std::sync::atomic::Ordering::SeqCst), - "Device lost callback should have been called." - ); - }); - #[gpu_test] static DIFFERENT_BGL_ORDER_BW_SHADER_AND_API: GpuTestConfiguration = GpuTestConfiguration::new() .parameters(TestParameters::default()) diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index 42881f1bd1..358c77ff43 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -8,7 +8,7 @@ use crate::{ }, command::{self, CommandBuffer}, conv, - device::{bgl, life::WaitIdleError, DeviceError, DeviceLostClosure, DeviceLostReason}, + device::{bgl, life::WaitIdleError, DeviceError, DeviceLostClosure}, global::Global, hal_api::HalApi, id::{self, AdapterId, DeviceId, QueueId, SurfaceId}, @@ -2083,8 +2083,7 @@ impl Global { self.hub.devices.remove(device_id); } - // This closure will be called exactly once during "lose the device", - // or when it is replaced. + /// This closure will be called exactly once during "lose the device". pub fn device_set_device_lost_closure( &self, device_id: DeviceId, @@ -2092,22 +2091,15 @@ impl Global { ) { let device = self.hub.devices.get(device_id); - let old_device_lost_closure = device + device .device_lost_closure .lock() .replace(device_lost_closure); - - if let Some(old_device_lost_closure) = old_device_lost_closure { - old_device_lost_closure.call(DeviceLostReason::ReplacedCallback, "".to_string()); - } } pub fn device_unregister_device_lost_closure(&self, device_id: DeviceId) { let device = self.hub.devices.get(device_id); - let closure = device.device_lost_closure.lock().take(); - if let Some(closure) = closure { - closure.call(DeviceLostReason::ReplacedCallback, "".to_string()); - } + device.device_lost_closure.lock().take(); } pub fn device_destroy(&self, device_id: DeviceId) { diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index 618cfb6ea8..c9db29c43f 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -149,10 +149,6 @@ impl Drop for Device { fn drop(&mut self) { resource_log!("Drop {}", self.error_ident()); - if let Some(closure) = self.device_lost_closure.lock().take() { - closure.call(DeviceLostReason::Dropped, String::from("Device dropped.")); - } - // SAFETY: We are in the Drop impl and we don't use self.zero_buffer anymore after this point. let zero_buffer = unsafe { ManuallyDrop::take(&mut self.zero_buffer) }; // SAFETY: We are in the Drop impl and we don't use self.fence anymore after this point. diff --git a/wgpu-types/src/lib.rs b/wgpu-types/src/lib.rs index 1d5f6ef2df..d1f9d4fef4 100644 --- a/wgpu-types/src/lib.rs +++ b/wgpu-types/src/lib.rs @@ -7673,18 +7673,4 @@ pub enum DeviceLostReason { Unknown = 0, /// After Device::destroy Destroyed = 1, - /// After Device::drop - /// - /// WebGPU does not invoke the device lost callback when the device is - /// dropped to prevent garbage collection from being observable. In wgpu, - /// we invoke the callback on drop to help with managing memory owned by - /// the callback. - Dropped = 2, - /// After replacing the device_lost_callback - /// - /// WebGPU does not have a concept of a device lost callback, but wgpu - /// does. wgpu guarantees that any supplied callback will be invoked - /// exactly once before it is dropped, which helps with managing the - /// memory owned by the callback. - ReplacedCallback = 3, } From dffc5698327961fe7ce742c8ee94c4634b98a9d2 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Mon, 25 Nov 2024 13:19:22 +0100 Subject: [PATCH 4/8] simplify `SubmittedWorkDoneClosure` by removing its C variant --- wgpu-core/src/device/mod.rs | 2 +- wgpu-core/src/device/queue.rs | 55 ++--------------------------------- wgpu/src/backend/wgpu_core.rs | 3 +- 3 files changed, 4 insertions(+), 56 deletions(-) diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs index 9f695d2638..22f5e55cef 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -180,7 +180,7 @@ impl UserClosures { } } for closure in self.submissions { - closure.call(); + closure(); } for invocation in self.device_lost_invocations { invocation diff --git a/wgpu-core/src/device/queue.rs b/wgpu-core/src/device/queue.rs index 0983eb901c..1bb0d892c4 100644 --- a/wgpu-core/src/device/queue.rs +++ b/wgpu-core/src/device/queue.rs @@ -240,61 +240,10 @@ impl Drop for Queue { } } -#[repr(C)] -pub struct SubmittedWorkDoneClosureC { - pub callback: unsafe extern "C" fn(user_data: *mut u8), - pub user_data: *mut u8, -} - -#[cfg(send_sync)] -unsafe impl Send for SubmittedWorkDoneClosureC {} - -pub struct SubmittedWorkDoneClosure { - // We wrap this so creating the enum in the C variant can be unsafe, - // allowing our call function to be safe. - inner: SubmittedWorkDoneClosureInner, -} - #[cfg(send_sync)] -type SubmittedWorkDoneCallback = Box; +pub type SubmittedWorkDoneClosure = Box; #[cfg(not(send_sync))] -type SubmittedWorkDoneCallback = Box; - -enum SubmittedWorkDoneClosureInner { - Rust { callback: SubmittedWorkDoneCallback }, - C { inner: SubmittedWorkDoneClosureC }, -} - -impl SubmittedWorkDoneClosure { - pub fn from_rust(callback: SubmittedWorkDoneCallback) -> Self { - Self { - inner: SubmittedWorkDoneClosureInner::Rust { callback }, - } - } - - /// # Safety - /// - /// - The callback pointer must be valid to call with the provided `user_data` - /// pointer. - /// - /// - Both pointers must point to `'static` data, as the callback may happen at - /// an unspecified time. - pub unsafe fn from_c(inner: SubmittedWorkDoneClosureC) -> Self { - Self { - inner: SubmittedWorkDoneClosureInner::C { inner }, - } - } - - pub(crate) fn call(self) { - match self.inner { - SubmittedWorkDoneClosureInner::Rust { callback } => callback(), - // SAFETY: the contract of the call to from_c says that this unsafe is sound. - SubmittedWorkDoneClosureInner::C { inner } => unsafe { - (inner.callback)(inner.user_data) - }, - } - } -} +pub type SubmittedWorkDoneClosure = Box; /// A texture or buffer to be freed soon. /// diff --git a/wgpu/src/backend/wgpu_core.rs b/wgpu/src/backend/wgpu_core.rs index c385750a3e..b016ec284f 100644 --- a/wgpu/src/backend/wgpu_core.rs +++ b/wgpu/src/backend/wgpu_core.rs @@ -2105,8 +2105,7 @@ impl crate::Context for ContextWgpuCore { queue_data: &Self::QueueData, callback: crate::context::SubmittedWorkDoneCallback, ) { - let closure = wgc::device::queue::SubmittedWorkDoneClosure::from_rust(callback); - self.0.queue_on_submitted_work_done(queue_data.id, closure); + self.0.queue_on_submitted_work_done(queue_data.id, callback); } fn device_start_capture(&self, device_data: &Self::DeviceData) { From d851778728483821030325b944b69fe828ef3dee Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Mon, 25 Nov 2024 13:34:35 +0100 Subject: [PATCH 5/8] simplify `BufferMapCallback` by removing its C variant --- deno_webgpu/buffer.rs | 2 +- player/tests/test.rs | 4 +- wgpu-core/src/device/global.rs | 2 +- wgpu-core/src/device/mod.rs | 2 +- wgpu-core/src/resource.rs | 136 +++------------------------------ wgpu/src/backend/wgpu_core.rs | 10 +-- 6 files changed, 20 insertions(+), 136 deletions(-) diff --git a/deno_webgpu/buffer.rs b/deno_webgpu/buffer.rs index 08afcd133d..460a8cf233 100644 --- a/deno_webgpu/buffer.rs +++ b/deno_webgpu/buffer.rs @@ -108,7 +108,7 @@ pub async fn op_webgpu_buffer_get_map_async( 2 => wgpu_core::device::HostMap::Write, _ => unreachable!(), }, - callback: Some(wgpu_core::resource::BufferMapCallback::from_rust(callback)), + callback: Some(callback), }, ) .err(); diff --git a/player/tests/test.rs b/player/tests/test.rs index 107481b742..6a1aea176b 100644 --- a/player/tests/test.rs +++ b/player/tests/test.rs @@ -144,9 +144,7 @@ impl Test<'_> { Some(expect.data.len() as u64), wgc::resource::BufferMapOperation { host: wgc::device::HostMap::Read, - callback: Some(wgc::resource::BufferMapCallback::from_rust(Box::new( - map_callback, - ))), + callback: Some(Box::new(map_callback)), }, ) .unwrap(); diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index 358c77ff43..830376b4be 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -2170,7 +2170,7 @@ impl Global { Ok(submission_index) => Ok(submission_index), Err((mut operation, err)) => { if let Some(callback) = operation.callback.take() { - callback.call(Err(err.clone())); + callback(Err(err.clone())); } log::error!("Buffer::map_async error: {err}"); Err(err) diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs index 22f5e55cef..5078d0e4ff 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -176,7 +176,7 @@ impl UserClosures { // a on_submitted_work_done callback to be fired before the on_submitted_work_done callback. for (mut operation, status) in self.mappings { if let Some(callback) = operation.callback.take() { - callback.call(status); + callback(status); } } for closure in self.submissions { diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs index 055c254508..b72abf4b11 100644 --- a/wgpu-core/src/resource.rs +++ b/wgpu-core/src/resource.rs @@ -188,36 +188,6 @@ macro_rules! impl_trackable { }; } -/// The status code provided to the buffer mapping callback. -/// -/// This is very similar to `BufferAccessResult`, except that this is FFI-friendly. -#[repr(C)] -#[derive(Debug)] -pub enum BufferMapAsyncStatus { - /// The Buffer is successfully mapped, `get_mapped_range` can be called. - /// - /// All other variants of this enum represent failures to map the buffer. - Success, - /// The buffer is already mapped. - /// - /// While this is treated as an error, it does not prevent mapped range from being accessed. - AlreadyMapped, - /// Mapping was already requested. - MapAlreadyPending, - /// An unknown error. - Error, - /// The context is Lost. - ContextLost, - /// The buffer is in an invalid state. - Invalid, - /// The range isn't fully contained in the buffer. - InvalidRange, - /// The range isn't properly aligned. - InvalidAlignment, - /// Incompatible usage flags. - InvalidUsageFlags, -} - #[derive(Debug)] pub(crate) enum BufferMapState { /// Mapped at creation. @@ -239,107 +209,25 @@ unsafe impl Send for BufferMapState {} #[cfg(send_sync)] unsafe impl Sync for BufferMapState {} -#[repr(C)] -pub struct BufferMapCallbackC { - pub callback: unsafe extern "C" fn(status: BufferMapAsyncStatus, user_data: *mut u8), - pub user_data: *mut u8, -} - -#[cfg(send_sync)] -unsafe impl Send for BufferMapCallbackC {} - -#[derive(Debug)] -pub struct BufferMapCallback { - // We wrap this so creating the enum in the C variant can be unsafe, - // allowing our call function to be safe. - inner: BufferMapCallbackInner, -} - #[cfg(send_sync)] -type BufferMapCallbackCallback = Box; +pub type BufferMapCallback = Box; #[cfg(not(send_sync))] -type BufferMapCallbackCallback = Box; +pub type BufferMapCallback = Box; -enum BufferMapCallbackInner { - Rust { callback: BufferMapCallbackCallback }, - C { inner: BufferMapCallbackC }, +pub struct BufferMapOperation { + pub host: HostMap, + pub callback: Option, } -impl Debug for BufferMapCallbackInner { +impl Debug for BufferMapOperation { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - match *self { - BufferMapCallbackInner::Rust { callback: _ } => f.debug_struct("Rust").finish(), - BufferMapCallbackInner::C { inner: _ } => f.debug_struct("C").finish(), - } - } -} - -impl BufferMapCallback { - pub fn from_rust(callback: BufferMapCallbackCallback) -> Self { - Self { - inner: BufferMapCallbackInner::Rust { callback }, - } - } - - /// # Safety - /// - /// - The callback pointer must be valid to call with the provided user_data - /// pointer. - /// - /// - Both pointers must point to valid memory until the callback is - /// invoked, which may happen at an unspecified time. - pub unsafe fn from_c(inner: BufferMapCallbackC) -> Self { - Self { - inner: BufferMapCallbackInner::C { inner }, - } - } - - pub(crate) fn call(self, result: BufferAccessResult) { - match self.inner { - BufferMapCallbackInner::Rust { callback } => { - callback(result); - } - // SAFETY: the contract of the call to from_c says that this unsafe is sound. - BufferMapCallbackInner::C { inner } => unsafe { - let status = match result { - Ok(_) => BufferMapAsyncStatus::Success, - Err(BufferAccessError::Device(_)) => BufferMapAsyncStatus::ContextLost, - Err(BufferAccessError::InvalidResource(_)) - | Err(BufferAccessError::DestroyedResource(_)) => BufferMapAsyncStatus::Invalid, - Err(BufferAccessError::AlreadyMapped) => BufferMapAsyncStatus::AlreadyMapped, - Err(BufferAccessError::MapAlreadyPending) => { - BufferMapAsyncStatus::MapAlreadyPending - } - Err(BufferAccessError::MissingBufferUsage(_)) => { - BufferMapAsyncStatus::InvalidUsageFlags - } - Err(BufferAccessError::UnalignedRange) - | Err(BufferAccessError::UnalignedRangeSize { .. }) - | Err(BufferAccessError::UnalignedOffset { .. }) => { - BufferMapAsyncStatus::InvalidAlignment - } - Err(BufferAccessError::OutOfBoundsUnderrun { .. }) - | Err(BufferAccessError::OutOfBoundsOverrun { .. }) - | Err(BufferAccessError::NegativeRange { .. }) => { - BufferMapAsyncStatus::InvalidRange - } - Err(BufferAccessError::Failed) - | Err(BufferAccessError::NotMapped) - | Err(BufferAccessError::MapAborted) => BufferMapAsyncStatus::Error, - }; - - (inner.callback)(status, inner.user_data); - }, - } + f.debug_struct("BufferMapOperation") + .field("host", &self.host) + .field("callback", &self.callback.as_ref().map(|_| "?")) + .finish() } } -#[derive(Debug)] -pub struct BufferMapOperation { - pub host: HostMap, - pub callback: Option, -} - #[derive(Clone, Debug, Error)] #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))] #[non_exhaustive] @@ -637,7 +525,7 @@ impl Buffer { // We can safely unwrap below since we just set the `map_state` to `BufferMapState::Waiting`. let (mut operation, status) = self.map(&device.snatchable_lock.read()).unwrap(); if let Some(callback) = operation.callback.take() { - callback.call(status); + callback(status); } 0 }; @@ -711,7 +599,7 @@ impl Buffer { buffer_id, )? { if let Some(callback) = operation.callback.take() { - callback.call(status); + callback(status); } } diff --git a/wgpu/src/backend/wgpu_core.rs b/wgpu/src/backend/wgpu_core.rs index b016ec284f..714d764cdb 100644 --- a/wgpu/src/backend/wgpu_core.rs +++ b/wgpu/src/backend/wgpu_core.rs @@ -1406,12 +1406,10 @@ impl crate::Context for ContextWgpuCore { MapMode::Read => wgc::device::HostMap::Read, MapMode::Write => wgc::device::HostMap::Write, }, - callback: Some(wgc::resource::BufferMapCallback::from_rust(Box::new( - |status| { - let res = status.map_err(|_| crate::BufferAsyncError); - callback(res); - }, - ))), + callback: Some(Box::new(|status| { + let res = status.map_err(|_| crate::BufferAsyncError); + callback(res); + })), }; match self.0.buffer_map_async( From e8057843d6686b10223b68ddbfdcc14e219a5148 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Mon, 25 Nov 2024 13:36:48 +0100 Subject: [PATCH 6/8] simplify `DeviceLostClosure` by removing its C variant --- wgpu-core/src/device/mod.rs | 68 ++------------------------------ wgpu-core/src/device/resource.rs | 2 +- wgpu/src/backend/wgpu_core.rs | 5 +-- 3 files changed, 6 insertions(+), 69 deletions(-) diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs index 5078d0e4ff..8b4da3b2ec 100644 --- a/wgpu-core/src/device/mod.rs +++ b/wgpu-core/src/device/mod.rs @@ -12,7 +12,6 @@ use crate::{ use arrayvec::ArrayVec; use smallvec::SmallVec; -use std::os::raw::c_char; use thiserror::Error; use wgt::{BufferAddress, DeviceLostReason, TextureFormat}; @@ -183,36 +182,15 @@ impl UserClosures { closure(); } for invocation in self.device_lost_invocations { - invocation - .closure - .call(invocation.reason, invocation.message); + (invocation.closure)(invocation.reason, invocation.message); } } } #[cfg(send_sync)] -pub type DeviceLostCallback = Box; +pub type DeviceLostClosure = Box; #[cfg(not(send_sync))] -pub type DeviceLostCallback = Box; - -pub struct DeviceLostClosureRust { - pub callback: DeviceLostCallback, -} - -#[repr(C)] -pub struct DeviceLostClosureC { - pub callback: unsafe extern "C" fn(user_data: *mut u8, reason: u8, message: *const c_char), - pub user_data: *mut u8, -} - -#[cfg(send_sync)] -unsafe impl Send for DeviceLostClosureC {} - -pub struct DeviceLostClosure { - // We wrap this so creating the enum in the C variant can be unsafe, - // allowing our call function to be safe. - inner: DeviceLostClosureInner, -} +pub type DeviceLostClosure = Box; pub struct DeviceLostInvocation { closure: DeviceLostClosure, @@ -220,46 +198,6 @@ pub struct DeviceLostInvocation { message: String, } -enum DeviceLostClosureInner { - Rust { inner: DeviceLostClosureRust }, - C { inner: DeviceLostClosureC }, -} - -impl DeviceLostClosure { - pub fn from_rust(callback: DeviceLostCallback) -> Self { - let inner = DeviceLostClosureRust { callback }; - Self { - inner: DeviceLostClosureInner::Rust { inner }, - } - } - - /// # Safety - /// - /// - The callback pointer must be valid to call with the provided `user_data` - /// pointer. - /// - /// - Both pointers must point to `'static` data, as the callback may happen at - /// an unspecified time. - pub unsafe fn from_c(inner: DeviceLostClosureC) -> Self { - Self { - inner: DeviceLostClosureInner::C { inner }, - } - } - - pub(crate) fn call(self, reason: DeviceLostReason, message: String) { - match self.inner { - DeviceLostClosureInner::Rust { inner } => (inner.callback)(reason, message), - // SAFETY: the contract of the call to from_c says that this unsafe is sound. - DeviceLostClosureInner::C { inner } => unsafe { - // Ensure message is structured as a null-terminated C string. It only - // needs to live as long as the callback invocation. - let message = std::ffi::CString::new(message).unwrap(); - (inner.callback)(inner.user_data, reason as u8, message.as_ptr()) - }, - } - } -} - pub(crate) fn map_buffer( buffer: &Buffer, offset: BufferAddress, diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index c9db29c43f..0dec93127e 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -3600,7 +3600,7 @@ impl Device { // 1) Resolve the GPUDevice device.lost promise. if let Some(device_lost_closure) = self.device_lost_closure.lock().take() { - device_lost_closure.call(DeviceLostReason::Unknown, message.to_string()); + device_lost_closure(DeviceLostReason::Unknown, message.to_string()); } // 2) Complete any outstanding mapAsync() steps. diff --git a/wgpu/src/backend/wgpu_core.rs b/wgpu/src/backend/wgpu_core.rs index 714d764cdb..824b84fc01 100644 --- a/wgpu/src/backend/wgpu_core.rs +++ b/wgpu/src/backend/wgpu_core.rs @@ -24,7 +24,7 @@ use std::{ sync::Arc, }; use wgc::error::ContextErrorSource; -use wgc::{command::bundle_ffi::*, device::DeviceLostClosure, pipeline::CreateShaderModuleError}; +use wgc::{command::bundle_ffi::*, pipeline::CreateShaderModuleError}; use wgt::WasmNotSendSync; pub struct ContextWgpuCore(wgc::global::Global); @@ -1352,9 +1352,8 @@ impl crate::Context for ContextWgpuCore { device_data: &Self::DeviceData, device_lost_callback: crate::context::DeviceLostCallback, ) { - let device_lost_closure = DeviceLostClosure::from_rust(device_lost_callback); self.0 - .device_set_device_lost_closure(device_data.id, device_lost_closure); + .device_set_device_lost_closure(device_data.id, device_lost_callback); } fn device_destroy(&self, device_data: &Self::DeviceData) { self.0.device_destroy(device_data.id); From e829557dc4e18f11340ed29e5275fefdc90c8089 Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Mon, 25 Nov 2024 13:38:05 +0100 Subject: [PATCH 7/8] Remove `device_unregister_device_lost_closure` --- wgpu-core/src/device/global.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index 830376b4be..d86b66b98f 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -2097,11 +2097,6 @@ impl Global { .replace(device_lost_closure); } - pub fn device_unregister_device_lost_closure(&self, device_id: DeviceId) { - let device = self.hub.devices.get(device_id); - device.device_lost_closure.lock().take(); - } - pub fn device_destroy(&self, device_id: DeviceId) { api_log!("Device::destroy {device_id:?}"); From c0d3fd13e7740957ef9053e26a50bfb53cb5400d Mon Sep 17 00:00:00 2001 From: teoxoy <28601907+teoxoy@users.noreply.github.com> Date: Tue, 26 Nov 2024 12:50:46 +0100 Subject: [PATCH 8/8] [wgpu-core] document which closures are guaranteed to be called --- wgpu-core/src/device/global.rs | 3 ++- wgpu-core/src/device/queue.rs | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index d86b66b98f..b2bdba04b0 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -2083,7 +2083,7 @@ impl Global { self.hub.devices.remove(device_id); } - /// This closure will be called exactly once during "lose the device". + /// `device_lost_closure` might never be called. pub fn device_set_device_lost_closure( &self, device_id: DeviceId, @@ -2144,6 +2144,7 @@ impl Global { self.hub.queues.remove(queue_id); } + /// `op.callback` is guaranteed to be called. pub fn buffer_map_async( &self, buffer_id: id::BufferId, diff --git a/wgpu-core/src/device/queue.rs b/wgpu-core/src/device/queue.rs index 1bb0d892c4..ce59294e7d 100644 --- a/wgpu-core/src/device/queue.rs +++ b/wgpu-core/src/device/queue.rs @@ -1396,6 +1396,7 @@ impl Queue { unsafe { self.raw().get_timestamp_period() } } + /// `closure` is guaranteed to be called. pub fn on_submitted_work_done( &self, closure: SubmittedWorkDoneClosure,