Keep the value in its storage after destroy

nical · nical · commit e8840ffc8f40 · 2023-11-13T21:04:00.000+01:00
in gfx-rs#4657 the destroy implementation was made to remove the value from the storage and leave an error variant in its place. Unfortunately this causes some issues with the tracking code which expects the ID to be unregistered after the value has been fully destroyed, even if the latter is not in storage anymore. To work around that, this commit adds a `Destroyed` variant in storage which keeps the value so that the tracking behavior is preserved while still making sure that most accesses to the destroyed resource lead to validation errors. ... Except for submitted command buffers that need to be consumed right away. These are replaced with `Element::Error` like before this commit.
diff --git a/tests/tests/life_cycle.rs b/tests/tests/life_cycle.rs
@@ -1,34 +1,66 @@
-use wgpu_test::{gpu_test, FailureCase, GpuTestConfiguration, TestParameters};
+use wgpu_test::{fail, gpu_test, GpuTestConfiguration};
 
 #[gpu_test]
-static BUFFER_DESTROY: GpuTestConfiguration = GpuTestConfiguration::new()
-    .parameters(TestParameters::default().expect_fail(FailureCase::always()))
-    .run_sync(|ctx| {
-        let buffer = ctx.device.create_buffer(&wgpu::BufferDescriptor {
-            label: None,
-            size: 256,
-            usage: wgpu::BufferUsages::MAP_WRITE | wgpu::BufferUsages::COPY_SRC,
-            mapped_at_creation: false,
-        });
+static BUFFER_DESTROY: GpuTestConfiguration = GpuTestConfiguration::new().run_sync(|ctx| {
+    let buffer = ctx.device.create_buffer(&wgpu::BufferDescriptor {
+        label: None,
+        size: 256,
+        usage: wgpu::BufferUsages::MAP_WRITE | wgpu::BufferUsages::COPY_SRC,
+        mapped_at_creation: false,
+    });
 
-        buffer.destroy();
+    buffer.destroy();
 
-        buffer.destroy();
+    buffer.destroy();
 
-        ctx.device.poll(wgpu::MaintainBase::Wait);
+    ctx.device.poll(wgpu::MaintainBase::Wait);
 
+    fail(&ctx.device, &|| {
         buffer
             .slice(..)
             .map_async(wgpu::MapMode::Write, move |_| {});
+    });
 
-        buffer.destroy();
+    buffer.destroy();
 
-        ctx.device.poll(wgpu::MaintainBase::Wait);
+    ctx.device.poll(wgpu::MaintainBase::Wait);
 
-        buffer.destroy();
+    buffer.destroy();
 
+    buffer.destroy();
+
+    let descriptor = wgpu::BufferDescriptor {
+        label: None,
+        size: 256,
+        usage: wgpu::BufferUsages::MAP_WRITE | wgpu::BufferUsages::COPY_SRC,
+        mapped_at_creation: false,
+    };
+
+    // Scopes to mix up the drop/poll ordering.
+    {
+        let buffer = ctx.device.create_buffer(&descriptor);
         buffer.destroy();
-    });
+        let buffer = ctx.device.create_buffer(&descriptor);
+        buffer.destroy();
+    }
+    let buffer = ctx.device.create_buffer(&descriptor);
+    buffer.destroy();
+    ctx.device.poll(wgpu::MaintainBase::Wait);
+    let buffer = ctx.device.create_buffer(&descriptor);
+    buffer.destroy();
+    {
+        let buffer = ctx.device.create_buffer(&descriptor);
+        buffer.destroy();
+        let buffer = ctx.device.create_buffer(&descriptor);
+        buffer.destroy();
+        let buffer = ctx.device.create_buffer(&descriptor);
+        ctx.device.poll(wgpu::MaintainBase::Wait);
+        buffer.destroy();
+    }
+    let buffer = ctx.device.create_buffer(&descriptor);
+    buffer.destroy();
+    ctx.device.poll(wgpu::MaintainBase::Wait);
+});
 
 #[gpu_test]
 static TEXTURE_DESTROY: GpuTestConfiguration = GpuTestConfiguration::new().run_sync(|ctx| {
diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs
@@ -495,8 +495,8 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
 
             log::trace!("Buffer::destroy {buffer_id:?}");
             let (mut buffer_guard, _) = hub.buffers.write(&mut token);
-            let mut buffer = buffer_guard
-                .take_and_mark_destroyed(buffer_id)
+            let buffer = buffer_guard
+                .get_and_mark_destroyed(buffer_id)
                 .map_err(|_| resource::DestroyError::Invalid)?;
 
             let device = &mut device_guard[buffer.device_id.value];
@@ -506,7 +506,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                 | &BufferMapState::Init { .. }
                 | &BufferMapState::Active { .. }
                 => {
-                    self.buffer_unmap_inner(buffer_id, &mut buffer, device)
+                    self.buffer_unmap_inner(buffer_id, buffer, device)
                         .unwrap_or(None)
                 }
                 _ => None,
@@ -551,7 +551,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
 
         let (ref_count, last_submit_index, device_id) = {
             let (mut buffer_guard, _) = hub.buffers.write(&mut token);
-            match buffer_guard.get_mut(buffer_id) {
+            match buffer_guard.get_occupied_or_destroyed(buffer_id) {
                 Ok(buffer) => {
                     let ref_count = buffer.life_guard.ref_count.take().unwrap();
                     let last_submit_index = buffer.life_guard.life_count();
@@ -800,8 +800,8 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
         let (mut device_guard, mut token) = hub.devices.write(&mut token);
 
         let (mut texture_guard, _) = hub.textures.write(&mut token);
-        let mut texture = texture_guard
-            .take_and_mark_destroyed(texture_id)
+        let texture = texture_guard
+            .get_and_mark_destroyed(texture_id)
             .map_err(|_| resource::DestroyError::Invalid)?;
 
         let device = &mut device_guard[texture.device_id.value];
@@ -855,7 +855,7 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
 
         let (ref_count, last_submit_index, device_id) = {
             let (mut texture_guard, _) = hub.textures.write(&mut token);
-            match texture_guard.get_mut(texture_id) {
+            match texture_guard.get_occupied_or_destroyed(texture_id) {
                 Ok(texture) => {
                     let ref_count = texture.life_guard.ref_count.take().unwrap();
                     let last_submit_index = texture.life_guard.life_count();
diff --git a/wgpu-core/src/device/queue.rs b/wgpu-core/src/device/queue.rs
@@ -1110,9 +1110,10 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
                         // it, so make sure to set_size on it.
                         used_surface_textures.set_size(texture_guard.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.take_and_mark_destroyed(cmb_id)
-                        {
+                        let mut cmdbuf = match command_buffer_guard.replace_with_error(cmb_id) {
                             Ok(cmdbuf) => cmdbuf,
                             Err(_) => continue,
                         };
diff --git a/wgpu-core/src/identity.rs b/wgpu-core/src/identity.rs
@@ -53,6 +53,7 @@ impl IdentityManager {
     /// The backend is incorporated into the id, so that ids allocated with
     /// different `backend` values are always distinct.
     pub fn alloc<I: id::TypedId>(&mut self, backend: Backend) -> I {
+        println!("alloc id");
         match self.free.pop() {
             Some(index) => I::zip(index, self.epochs[index as usize], backend),
             None => {
@@ -66,6 +67,7 @@ impl IdentityManager {
 
     /// Free `id`. It will never be returned from `alloc` again.
     pub fn free<I: id::TypedId + Debug>(&mut self, id: I) {
+        println!("free id");
         let (index, epoch, _backend) = id.unzip();
         let pe = &mut self.epochs[index as usize];
         assert_eq!(*pe, epoch);
diff --git a/wgpu-core/src/storage.rs b/wgpu-core/src/storage.rs
@@ -14,6 +14,10 @@ pub(crate) enum Element<T> {
     /// epoch.
     Occupied(T, Epoch),
 
+    /// Like `Occupied`, but the resource has been marked as destroyed
+    /// and hasn't been dropped yet.
+    Destroyed(T, Epoch),
+
     /// Like `Occupied`, but an error occurred when creating the
     /// resource.
     ///
@@ -68,9 +72,11 @@ impl<T, I: id::TypedId> Storage<T, I> {
         let (index, epoch, _) = id.unzip();
         match self.map.get(index as usize) {
             Some(&Element::Vacant) => false,
-            Some(&Element::Occupied(_, storage_epoch) | &Element::Error(storage_epoch, _)) => {
-                storage_epoch == epoch
-            }
+            Some(
+                &Element::Occupied(_, storage_epoch)
+                | &Element::Destroyed(_, storage_epoch)
+                | &Element::Error(storage_epoch, _),
+            ) => storage_epoch == epoch,
             None => false,
         }
     }
@@ -87,7 +93,9 @@ impl<T, I: id::TypedId> Storage<T, I> {
         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, ..)) => (Err(InvalidId), epoch),
+            Some(&Element::Error(epoch, ..)) | Some(&Element::Destroyed(.., epoch)) => {
+                (Err(InvalidId), epoch)
+            }
             None => return Err(InvalidId),
         };
         assert_eq!(
@@ -106,6 +114,7 @@ impl<T, I: id::TypedId> Storage<T, I> {
             Some(&Element::Occupied(ref v, epoch)) => (Ok(v), epoch),
             Some(&Element::Vacant) => panic!("{}[{}] does not exist", self.kind, index),
             Some(&Element::Error(epoch, ..)) => (Err(InvalidId), epoch),
+            Some(&Element::Destroyed(.., epoch)) => (Err(InvalidId), epoch),
             None => return Err(InvalidId),
         };
         assert_eq!(
@@ -124,6 +133,29 @@ impl<T, I: id::TypedId> Storage<T, I> {
             Some(&mut Element::Occupied(ref mut v, epoch)) => (Ok(v), epoch),
             Some(&mut Element::Vacant) | None => panic!("{}[{}] does not exist", self.kind, index),
             Some(&mut Element::Error(epoch, ..)) => (Err(InvalidId), epoch),
+            Some(&mut Element::Destroyed(.., epoch)) => (Err(InvalidId), epoch),
+        };
+        assert_eq!(
+            epoch, storage_epoch,
+            "{}[{}] is no longer alive",
+            self.kind, index
+        );
+        result
+    }
+
+    /// Like `get_mut`, but returns the element even if it is destroyed.
+    ///
+    /// In practice, most API entry points should use `get`/`get_mut` so that a
+    /// destroyed resource leads to a validation error. This should be used internally
+    /// in places where we want to do some manipulation potentially after the element
+    /// was destroyed (for example the drop implementation).
+    pub(crate) fn get_occupied_or_destroyed(&mut self, id: I) -> Result<&mut T, InvalidId> {
+        let (index, epoch, _) = id.unzip();
+        let (result, storage_epoch) = match self.map.get_mut(index as usize) {
+            Some(&mut Element::Occupied(ref mut v, epoch))
+            | Some(&mut Element::Destroyed(ref mut v, epoch)) => (Ok(v), epoch),
+            Some(&mut Element::Vacant) | None => panic!("{}[{}] does not exist", self.kind, index),
+            Some(&mut Element::Error(epoch, ..)) => (Err(InvalidId), epoch),
         };
         assert_eq!(
             epoch, storage_epoch,
@@ -137,7 +169,7 @@ impl<T, I: id::TypedId> Storage<T, I> {
         match self.map[id as usize] {
             Element::Occupied(ref v, _) => v,
             Element::Vacant => panic!("{}[{}] does not exist", self.kind, id),
-            Element::Error(_, _) => panic!(""),
+            Element::Error(_, _) | Element::Destroyed(..) => panic!(""),
         }
     }
 
@@ -169,13 +201,13 @@ impl<T, I: id::TypedId> Storage<T, I> {
         self.insert_impl(index as usize, Element::Error(epoch, label.to_string()))
     }
 
-    pub(crate) fn take_and_mark_destroyed(&mut self, id: I) -> Result<T, InvalidId> {
+    pub(crate) fn replace_with_error(&mut self, id: I) -> Result<T, InvalidId> {
         let (index, epoch, _) = id.unzip();
         match std::mem::replace(
             &mut self.map[index as usize],
             Element::Error(epoch, String::new()),
         ) {
-            Element::Vacant => panic!("Cannot mark a vacant resource destroyed"),
+            Element::Vacant => panic!("Cannot access vacant resource"),
             Element::Occupied(value, storage_epoch) => {
                 assert_eq!(epoch, storage_epoch);
                 Ok(value)
@@ -184,6 +216,27 @@ impl<T, I: id::TypedId> Storage<T, I> {
         }
     }
 
+    pub(crate) fn get_and_mark_destroyed(&mut self, id: I) -> Result<&mut 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(..) = slot {
+            if let Element::Occupied(value, storage_epoch) =
+                std::mem::replace(slot, Element::Vacant)
+            {
+                debug_assert_eq!(storage_epoch, epoch);
+                *slot = Element::Destroyed(value, storage_epoch);
+            }
+        }
+
+        if let Element::Destroyed(ref mut value, ..) = *slot {
+            Ok(value)
+        } else {
+            Err(InvalidId)
+        }
+    }
+
     pub(crate) fn force_replace(&mut self, id: I, value: T) {
         let (index, epoch, _) = id.unzip();
         self.map[index as usize] = Element::Occupied(value, epoch);
@@ -192,7 +245,7 @@ impl<T, I: id::TypedId> Storage<T, I> {
     pub(crate) fn remove(&mut self, id: I) -> Option<T> {
         let (index, epoch, _) = id.unzip();
         match std::mem::replace(&mut self.map[index as usize], Element::Vacant) {
-            Element::Occupied(value, storage_epoch) => {
+            Element::Occupied(value, storage_epoch) | Element::Destroyed(value, storage_epoch) => {
                 assert_eq!(epoch, storage_epoch);
                 Some(value)
             }
@@ -239,7 +292,7 @@ impl<T, I: id::TypedId> Storage<T, I> {
         };
         for element in self.map.iter() {
             match *element {
-                Element::Occupied(..) => report.num_occupied += 1,
+                Element::Occupied(..) | Element::Destroyed(..) => report.num_occupied += 1,
                 Element::Vacant => report.num_vacant += 1,
                 Element::Error(..) => report.num_error += 1,
             }
