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.

Author: nical

wgpu-core/src/device/global.rs

@@ -489,8 +489,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
 
         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<G: GlobalIdentityHandlerFactory> Global<G> {
 
         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<G: GlobalIdentityHandlerFactory> Global<G> {
                 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<G: GlobalIdentityHandlerFactory> Global<G> {
                     },
                 ));
             }
+
+            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<G: GlobalIdentityHandlerFactory> Global<G> {
                 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<G: GlobalIdentityHandlerFactory> Global<G> {
             .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<G: GlobalIdentityHandlerFactory> Global<G> {
             .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());
         }

wgpu-core/src/device/queue.rs

@@ -1158,8 +1158,6 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                         // 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,

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<I: id::TypedId, T: Resource<I>> Registry<I, T> {
         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,
             }

wgpu-core/src/resource.rs

@@ -433,6 +433,10 @@ impl<A: HalApi> Buffer<A> {
         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<Self>) -> Result<(), BufferAccessError> {
         if let Some((mut operation, status)) = self.unmap_inner()? {
@@ -818,6 +822,10 @@ impl<A: HalApi> Texture<A> {
         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,

wgpu-core/src/storage.rs

@@ -14,10 +14,6 @@ pub(crate) enum Element<T> {
     /// epoch.
     Occupied(Arc<T>, 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<Arc<T>, 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"),
         }