nordic-dev.net/wgpu
nordic-dev.net/wgpu
2
0
Fork 0
mirror of https://github.com/gfx-rs/wgpu.git synced 2024-11-26 00:33:51 +00:00
Code Issues Packages Projects Releases Wiki Activity

Keep the value in its storage after destroy (#4678)

Browse Source 
* Keep the value in its storage after destroy

in #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.

Co-authored-by: Teodor Tanasoaia <28601907+teoxoy@users.noreply.github.com>
This commit is contained in:
Nicolas Silva 2023-11-14 15:23:23 +01:00 committed by GitHub
parent 9f91c95c24
commit a697e4352c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 125 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

74
tests/tests/life_cycle.rs
View File

@ -1,35 +1,67 @@
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();
ctx.device.poll(wgpu::MaintainBase::Wait);
buffer.destroy();
buffer.destroy();
});
buffer.destroy();
ctx.device.poll(wgpu::MaintainBase::Wait);
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| {
let texture = ctx.device.create_texture(&wgpu::TextureDescriptor {

14
wgpu-core/src/device/global.rs
View File

@ -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();

5
wgpu-core/src/device/queue.rs
View File

@ -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,
};

71
wgpu-core/src/storage.rs
View File

@ -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,
}