From 04a5d1d0556908ba99358b913f2b9163229ddedc Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Wed, 13 Dec 2023 12:15:45 +0100 Subject: [PATCH] Move Snatchable to its own module --- wgpu-core/src/command/compute.rs | 2 +- wgpu-core/src/command/mod.rs | 2 +- wgpu-core/src/device/resource.rs | 62 ++--------------------- wgpu-core/src/lib.rs | 1 + wgpu-core/src/resource.rs | 5 +- wgpu-core/src/snatch.rs | 87 ++++++++++++++++++++++++++++++++ wgpu-core/src/track/buffer.rs | 2 +- wgpu-core/src/track/mod.rs | 2 +- 8 files changed, 99 insertions(+), 64 deletions(-) create mode 100644 wgpu-core/src/snatch.rs diff --git a/wgpu-core/src/command/compute.rs b/wgpu-core/src/command/compute.rs index ea6a089adae..8bc8f43cd41 100644 --- a/wgpu-core/src/command/compute.rs +++ b/wgpu-core/src/command/compute.rs @@ -1,5 +1,5 @@ -use crate::device::resource::SnatchGuard; use crate::resource::Resource; +use crate::snatch::SnatchGuard; use crate::{ binding_model::{ BindError, BindGroup, LateMinBufferBindingSizeMismatch, PushConstantUploadError, diff --git a/wgpu-core/src/command/mod.rs b/wgpu-core/src/command/mod.rs index dcf25443c39..64f39030b63 100644 --- a/wgpu-core/src/command/mod.rs +++ b/wgpu-core/src/command/mod.rs @@ -18,11 +18,11 @@ pub use self::{ use self::memory_init::CommandBufferTextureMemoryActions; -use crate::device::resource::SnatchGuard; use crate::device::Device; use crate::error::{ErrorFormatter, PrettyError}; use crate::hub::Hub; use crate::id::CommandBufferId; +use crate::snatch::SnatchGuard; use crate::init_tracker::BufferInitTrackerAction; use crate::resource::{Resource, ResourceInfo, ResourceType}; diff --git a/wgpu-core/src/device/resource.rs b/wgpu-core/src/device/resource.rs index 200e70c91a3..9e27888f39e 100644 --- a/wgpu-core/src/device/resource.rs +++ b/wgpu-core/src/device/resource.rs @@ -26,6 +26,7 @@ use crate::{ TextureViewNotRenderableReason, }, resource_log, + snatch::{SnatchGuard, SnatchLock, Snatchable}, storage::Storage, track::{BindGroupStates, TextureSelector, Tracker}, validation::{self, check_buffer_usage, check_texture_usage}, @@ -34,7 +35,7 @@ use crate::{ use arrayvec::ArrayVec; use hal::{CommandEncoder as _, Device as _}; -use parking_lot::{Mutex, MutexGuard, RwLock, RwLockReadGuard, RwLockWriteGuard}; +use parking_lot::{Mutex, MutexGuard, RwLock}; use smallvec::SmallVec; use thiserror::Error; @@ -42,7 +43,6 @@ use wgt::{DeviceLostReason, TextureFormat, TextureSampleType, TextureViewDimensi use std::{ borrow::Cow, - cell::UnsafeCell, iter, num::NonZeroU32, sync::{ @@ -95,7 +95,7 @@ pub struct Device { //Note: The submission index here corresponds to the last submission that is done. pub(crate) active_submission_index: AtomicU64, //SubmissionIndex, pub(crate) fence: RwLock>, - pub(crate) snatchable_lock: RwLock, + pub(crate) snatchable_lock: SnatchLock, /// Is this device valid? Valid is closely associated with "lose the device", /// which can be triggered by various methods, including at the end of device @@ -256,7 +256,7 @@ impl Device { command_allocator: Mutex::new(Some(com_alloc)), active_submission_index: AtomicU64::new(0), fence: RwLock::new(Some(fence)), - snatchable_lock: RwLock::new(SnatchableMarker), + snatchable_lock: unsafe { SnatchLock::new() }, valid: AtomicBool::new(true), trackers: Mutex::new(Tracker::new()), life_tracker: Mutex::new(life::LifetimeTracker::new()), @@ -1769,7 +1769,7 @@ impl Device { used: &mut BindGroupStates, storage: &'a Storage, id::BufferId>, limits: &wgt::Limits, - snatch_guard: &'a RwLockReadGuard<'a, SnatchableMarker>, + snatch_guard: &'a SnatchGuard<'a>, ) -> Result, binding_model::CreateBindGroupError> { use crate::binding_model::CreateBindGroupError as Error; @@ -3413,55 +3413,3 @@ impl Resource for Device { &mut self.info } } - -pub struct SnatchableMarker; -/// A guard that provides read access to snatchable data. -pub type SnatchGuard<'a> = RwLockReadGuard<'a, SnatchableMarker>; -/// A guard that allows snatching the snatchable data. -pub type ExclusiveSnatchGuard<'a> = RwLockWriteGuard<'a, SnatchableMarker>; - -/// A value that is mostly immutable but can be "snatched" if we need to destroy -/// it early. -/// -/// In order to safely access the underlying data, the device's global snatchable -/// lock must be taken. To guarentee it, methods take a read or write guard of that -/// special lock. -pub struct Snatchable { - value: UnsafeCell>, -} - -impl Snatchable { - pub fn new(val: T) -> Self { - Snatchable { - value: UnsafeCell::new(Some(val)), - } - } - - /// Get read access to the value. Requires a the snatchable lock's read guard. - pub fn get(&self, _guard: &SnatchGuard) -> Option<&T> { - unsafe { (*self.value.get()).as_ref() } - } - - /// Take the value. Requires a the snatchable lock's write guard. - pub fn snatch(&self, _guard: ExclusiveSnatchGuard) -> Option { - unsafe { (*self.value.get()).take() } - } - - /// Take the value without a guard. This can only be used with exclusive access - /// to self, so it does not require locking. - /// - /// Typically useful in a drop implementation. - pub fn take(&mut self) -> Option { - self.value.get_mut().take() - } -} - -// Can't safely print the contents of a snatchable objetc without holding -// the lock. -impl std::fmt::Debug for Snatchable { - fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - write!(f, "") - } -} - -unsafe impl Sync for Snatchable {} diff --git a/wgpu-core/src/lib.rs b/wgpu-core/src/lib.rs index e68debdcb86..a4bb0e71b5e 100644 --- a/wgpu-core/src/lib.rs +++ b/wgpu-core/src/lib.rs @@ -64,6 +64,7 @@ pub mod pipeline; pub mod present; pub mod registry; pub mod resource; +mod snatch; pub mod storage; mod track; // This is public for users who pre-compile shaders while still wanting to diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs index e8ebdc62f72..3e0f790d1e5 100644 --- a/wgpu-core/src/resource.rs +++ b/wgpu-core/src/resource.rs @@ -2,9 +2,7 @@ use crate::device::trace; use crate::{ device::{ - queue, - resource::{SnatchGuard, Snatchable}, - BufferMapPendingClosure, Device, DeviceError, HostMap, MissingDownlevelFlags, + queue, BufferMapPendingClosure, Device, DeviceError, HostMap, MissingDownlevelFlags, MissingFeatures, }, global::Global, @@ -16,6 +14,7 @@ use crate::{ identity::{GlobalIdentityHandlerFactory, IdentityManager}, init_tracker::{BufferInitTracker, TextureInitTracker}, resource, resource_log, + snatch::{SnatchGuard, Snatchable}, track::TextureSelector, validation::MissingBufferUsageError, Label, SubmissionIndex, diff --git a/wgpu-core/src/snatch.rs b/wgpu-core/src/snatch.rs new file mode 100644 index 00000000000..79e7e866b01 --- /dev/null +++ b/wgpu-core/src/snatch.rs @@ -0,0 +1,87 @@ +#![allow(unused)] + +use std::cell::UnsafeCell; +use parking_lot::{RwLockReadGuard, RwLockWriteGuard, RwLock}; + +/// A guard that provides read access to snatchable data. +pub struct SnatchGuard<'a>(RwLockReadGuard<'a, ()>); +/// A guard that allows snatching the snatchable data. +pub struct ExclusiveSnatchGuard<'a>(RwLockWriteGuard<'a, ()>); + +/// A value that is mostly immutable but can be "snatched" if we need to destroy +/// it early. +/// +/// In order to safely access the underlying data, the device's global snatchable +/// lock must be taken. To guarentee it, methods take a read or write guard of that +/// special lock. +pub struct Snatchable { + value: UnsafeCell>, +} + +impl Snatchable { + pub fn new(val: T) -> Self { + Snatchable { + value: UnsafeCell::new(Some(val)), + } + } + + /// Get read access to the value. Requires a the snatchable lock's read guard. + pub fn get(&self, _guard: &SnatchGuard) -> Option<&T> { + unsafe { (*self.value.get()).as_ref() } + } + + /// Take the value. Requires a the snatchable lock's write guard. + pub fn snatch(&self, _guard: ExclusiveSnatchGuard) -> Option { + unsafe { (*self.value.get()).take() } + } + + /// Take the value without a guard. This can only be used with exclusive access + /// to self, so it does not require locking. + /// + /// Typically useful in a drop implementation. + pub fn take(&mut self) -> Option { + self.value.get_mut().take() + } +} + +// Can't safely print the contents of a snatchable objetc without holding +// the lock. +impl std::fmt::Debug for Snatchable { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + write!(f, "") + } +} + +unsafe impl Sync for Snatchable {} + + +/// A Device-global lock for all snatchable data. +pub struct SnatchLock { + lock: RwLock<()>, +} + +impl SnatchLock { + /// The safety of `Snatchable::get` and `Snatchable::snatch` rely on their using the + /// right SnatchLock (the one associated to the same device). This method is unsafe + /// to force force sers to think twice about creating a SnatchLock. The only place this + /// method sould be called is when creating the device. + pub unsafe fn new() -> Self { + SnatchLock { + lock: RwLock::new(()), + } + } + + /// Request read access to snatchable resources. + pub fn read(&self) -> SnatchGuard { + SnatchGuard(self.lock.read()) + } + + /// Request write access to snatchable resources. + /// + /// This should only be called when a resource needs to be snatched. This has + /// a high risk of causing lock contention if called concurrently with other + /// wgpu work. + pub fn write(&self) -> ExclusiveSnatchGuard { + ExclusiveSnatchGuard(self.lock.write()) + } +} diff --git a/wgpu-core/src/track/buffer.rs b/wgpu-core/src/track/buffer.rs index 7b3b20a4642..2c2a6937f9b 100644 --- a/wgpu-core/src/track/buffer.rs +++ b/wgpu-core/src/track/buffer.rs @@ -9,10 +9,10 @@ use std::{borrow::Cow, marker::PhantomData, sync::Arc}; use super::{PendingTransition, ResourceTracker}; use crate::{ - device::resource::SnatchGuard, hal_api::HalApi, id::{BufferId, TypedId}, resource::{Buffer, Resource}, + snatch::SnatchGuard, storage::Storage, track::{ invalid_resource_state, skip_barrier, ResourceMetadata, ResourceMetadataProvider, diff --git a/wgpu-core/src/track/mod.rs b/wgpu-core/src/track/mod.rs index 961b3f658c7..0f0b22b004a 100644 --- a/wgpu-core/src/track/mod.rs +++ b/wgpu-core/src/track/mod.rs @@ -103,10 +103,10 @@ mod texture; use crate::{ binding_model, command, conv, - device::resource::SnatchGuard, hal_api::HalApi, id::{self, TypedId}, pipeline, resource, + snatch::SnatchGuard, storage::Storage, };