From bc65d84cdb0b8ca57d68733232afba621aeacb54 Mon Sep 17 00:00:00 2001 From: Nicolas Silva Date: Thu, 11 Jan 2024 11:45:12 +0100 Subject: [PATCH] Remove the Destroyed state from Storage (#4970) * Remove the Destroyed state from Storage It used to be how we handled destroying buffers and textures but we moved to different approach. * Explicit check for destroyed textures/buffers in a few entry points This used to be checked automatically when getting the resource from the registry, but has to be done manually now that we track we track the destroyed state in the resources. --- wgpu-core/src/device/global.rs | 34 +++++++++++++++++++++---- wgpu-core/src/device/queue.rs | 2 -- wgpu-core/src/registry.rs | 2 -- wgpu-core/src/resource.rs | 8 ++++++ wgpu-core/src/storage.rs | 45 +++------------------------------- 5 files changed, 41 insertions(+), 50 deletions(-) diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs index 26b2a0588..dfbd359ba 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -489,8 +489,7 @@ impl Global { let buffer = hub .buffers - .write() - .get_and_mark_destroyed(buffer_id) + .get(buffer_id) .map_err(|_| resource::DestroyError::Invalid)?; let _ = buffer.unmap(); @@ -732,8 +731,7 @@ impl Global { let texture = hub .textures - .write() - .get_and_mark_destroyed(texture_id) + .get(texture_id) .map_err(|_| resource::DestroyError::Invalid)?; texture.destroy() @@ -799,6 +797,12 @@ impl Global { Err(_) => break resource::CreateTextureViewError::InvalidTexture, }; let device = &texture.device; + { + let snatch_guard = device.snatchable_lock.read(); + if texture.is_destroyed(&snatch_guard) { + break resource::CreateTextureViewError::InvalidTexture; + } + } #[cfg(feature = "trace")] if let Some(ref mut trace) = *device.trace.lock() { trace.add(trace::Action::CreateTextureView { @@ -2386,6 +2390,12 @@ impl Global { }, )); } + + let snatch_guard = device.snatchable_lock.read(); + if buffer.is_destroyed(&snatch_guard) { + return Err((op, BufferAccessError::Destroyed)); + } + { let map_state = &mut *buffer.map_state.lock(); *map_state = match *map_state { @@ -2410,10 +2420,11 @@ impl Global { let mut trackers = buffer.device.as_ref().trackers.lock(); trackers.buffers.set_single(&buffer, internal_use); //TODO: Check if draining ALL buffers is correct! - let snatch_guard = device.snatchable_lock.read(); let _ = trackers.buffers.drain_transitions(&snatch_guard); } + drop(snatch_guard); + buffer }; @@ -2438,6 +2449,13 @@ impl Global { .get(buffer_id) .map_err(|_| BufferAccessError::Invalid)?; + { + let snatch_guard = buffer.device.snatchable_lock.read(); + if buffer.is_destroyed(&snatch_guard) { + return Err(BufferAccessError::Destroyed); + } + } + let range_size = if let Some(size) = size { size } else if offset > buffer.size { @@ -2500,6 +2518,12 @@ impl Global { .get(buffer_id) .map_err(|_| BufferAccessError::Invalid)?; + let snatch_guard = buffer.device.snatchable_lock.read(); + if buffer.is_destroyed(&snatch_guard) { + return Err(BufferAccessError::Destroyed); + } + drop(snatch_guard); + if !buffer.device.is_valid() { return Err(DeviceError::Lost.into()); } diff --git a/wgpu-core/src/device/queue.rs b/wgpu-core/src/device/queue.rs index 4667c1571..6a3759a12 100644 --- a/wgpu-core/src/device/queue.rs +++ b/wgpu-core/src/device/queue.rs @@ -1158,8 +1158,6 @@ impl Global { // it, so make sure to set_size on it. used_surface_textures.set_size(hub.textures.read().len()); - // TODO: ideally we would use `get_and_mark_destroyed` but the code here - // wants to consume the command buffer. #[allow(unused_mut)] let mut cmdbuf = match command_buffer_guard.replace_with_error(cmb_id) { Ok(cmdbuf) => cmdbuf, diff --git a/wgpu-core/src/registry.rs b/wgpu-core/src/registry.rs index 79d68921a..feb32903a 100644 --- a/wgpu-core/src/registry.rs +++ b/wgpu-core/src/registry.rs @@ -15,7 +15,6 @@ pub struct RegistryReport { pub num_allocated: usize, pub num_kept_from_user: usize, pub num_released_from_user: usize, - pub num_destroyed_from_user: usize, pub num_error: usize, pub element_size: usize, } @@ -192,7 +191,6 @@ impl> Registry { for element in storage.map.iter() { match *element { Element::Occupied(..) => report.num_kept_from_user += 1, - Element::Destroyed(..) => report.num_destroyed_from_user += 1, Element::Vacant => report.num_released_from_user += 1, Element::Error(..) => report.num_error += 1, } diff --git a/wgpu-core/src/resource.rs b/wgpu-core/src/resource.rs index 433c71175..d88e6947b 100644 --- a/wgpu-core/src/resource.rs +++ b/wgpu-core/src/resource.rs @@ -433,6 +433,10 @@ impl Buffer { self.raw.get(guard) } + pub(crate) fn is_destroyed(&self, guard: &SnatchGuard) -> bool { + self.raw.get(guard).is_none() + } + // Note: This must not be called while holding a lock. pub(crate) fn unmap(self: &Arc) -> Result<(), BufferAccessError> { if let Some((mut operation, status)) = self.unmap_inner()? { @@ -818,6 +822,10 @@ impl Texture { self.inner.get(snatch_guard)?.raw() } + pub(crate) fn is_destroyed(&self, guard: &SnatchGuard) -> bool { + self.inner.get(guard).is_none() + } + pub(crate) fn inner_mut<'a>( &'a self, guard: &mut ExclusiveSnatchGuard, diff --git a/wgpu-core/src/storage.rs b/wgpu-core/src/storage.rs index cf81e65eb..3e7a8f44f 100644 --- a/wgpu-core/src/storage.rs +++ b/wgpu-core/src/storage.rs @@ -14,10 +14,6 @@ pub(crate) enum Element { /// epoch. Occupied(Arc, Epoch), - /// Like `Occupied`, but the resource has been marked as destroyed - /// and hasn't been dropped yet. - Destroyed(Epoch), - /// Like `Occupied`, but an error occurred when creating the /// resource. /// @@ -78,11 +74,9 @@ where let (index, epoch, _) = id.unzip(); match self.map.get(index as usize) { Some(&Element::Vacant) => false, - Some( - &Element::Occupied(_, storage_epoch) - | &Element::Destroyed(storage_epoch) - | &Element::Error(storage_epoch, _), - ) => storage_epoch == epoch, + Some(&Element::Occupied(_, storage_epoch) | &Element::Error(storage_epoch, _)) => { + storage_epoch == epoch + } None => false, } } @@ -99,9 +93,7 @@ where let (result, storage_epoch) = match self.map.get(index as usize) { Some(&Element::Occupied(ref v, epoch)) => (Ok(Some(v)), epoch), Some(&Element::Vacant) => return Ok(None), - Some(&Element::Error(epoch, ..)) | Some(&Element::Destroyed(.., epoch)) => { - (Err(InvalidId), epoch) - } + Some(&Element::Error(epoch, ..)) => (Err(InvalidId), epoch), None => return Err(InvalidId), }; assert_eq!( @@ -120,7 +112,6 @@ where Some(&Element::Occupied(ref v, epoch)) => (Ok(v), epoch), Some(&Element::Vacant) => panic!("{}[{:?}] does not exist", self.kind, id), Some(&Element::Error(epoch, ..)) => (Err(InvalidId), epoch), - Some(&Element::Destroyed(.., epoch)) => (Err(InvalidId), epoch), None => return Err(InvalidId), }; assert_eq!( @@ -151,14 +142,6 @@ where } match std::mem::replace(&mut self.map[index], element) { Element::Vacant => {} - Element::Destroyed(storage_epoch) => { - assert_ne!( - epoch, - storage_epoch, - "Index {index:?} of {} is already occupied", - T::TYPE - ); - } Element::Occupied(_, storage_epoch) => { assert_ne!( epoch, @@ -209,22 +192,6 @@ where } } - pub(crate) fn get_and_mark_destroyed(&mut self, id: I) -> Result, InvalidId> { - let (index, epoch, _) = id.unzip(); - let slot = &mut self.map[index as usize]; - // borrowck dance: we have to move the element out before we can replace it - // with another variant with the same value. - if let &mut Element::Occupied(_, e) = slot { - if let Element::Occupied(value, storage_epoch) = - std::mem::replace(slot, Element::Destroyed(e)) - { - debug_assert_eq!(storage_epoch, epoch); - return Ok(value); - } - } - Err(InvalidId) - } - pub(crate) fn force_replace(&mut self, id: I, value: T) { log::trace!("User is replacing {}{:?}", T::TYPE, id); let (index, epoch, _) = id.unzip(); @@ -239,10 +206,6 @@ where assert_eq!(epoch, storage_epoch); Some(value) } - Element::Destroyed(storage_epoch) => { - assert_eq!(epoch, storage_epoch); - None - } Element::Error(..) => None, Element::Vacant => panic!("Cannot remove a vacant resource"), }