diff --git a/tests/tests/life_cycle.rs b/tests/tests/life_cycle.rs index 1622547bc3d..d235387a1f5 100644 --- 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 index ca6b756a52f..29432aae923 100644 --- a/wgpu-core/src/device/global.rs +++ b/wgpu-core/src/device/global.rs @@ -495,8 +495,8 @@ impl Global { 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 Global { | &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 Global { 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 Global { 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 Global { 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 index 92396e6e7c7..08805fa78f2 100644 --- a/wgpu-core/src/device/queue.rs +++ b/wgpu-core/src/device/queue.rs @@ -1110,9 +1110,10 @@ impl Global { // 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 index fe10bedb0e0..887cc946ec2 100644 --- 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(&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(&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 index 2dcec0d3276..92d778f95fb 100644 --- a/wgpu-core/src/storage.rs +++ b/wgpu-core/src/storage.rs @@ -14,6 +14,10 @@ pub(crate) enum Element { /// 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 Storage { 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 Storage { 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 Storage { 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 Storage { 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 Storage { 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 Storage { self.insert_impl(index as usize, Element::Error(epoch, label.to_string())) } - pub(crate) fn take_and_mark_destroyed(&mut self, id: I) -> Result { + pub(crate) fn replace_with_error(&mut self, id: I) -> Result { 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,32 @@ impl Storage { } } + pub(crate) fn get_and_mark_destroyed(&mut self, id: I) -> Result<&mut T, InvalidId> { + println!("take_and_mark_destroyed"); + 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 Element::Occupied(..) = slot { + match std::mem::replace(slot, Element::Vacant) { + Element::Occupied(value, storage_epoch) => { + debug_assert_eq!(storage_epoch, epoch); + *slot = Element::Destroyed(value, storage_epoch); + } + _ => {} + } + } + + match slot { + Element::Destroyed(value, ..) => { + return Ok(value); + } + _ => { + return 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 +250,7 @@ impl Storage { pub(crate) fn remove(&mut self, id: I) -> Option { 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 +297,7 @@ impl Storage { }; 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, }