No way to manually destroy memory blocks before Device is destroyed

[Grimeh]

Hi, I seem to have run up against a wall with how the allocator destroys memory blocks.
I have a GfxCtx struct that holds basically all of my Vulkan state. Contains stuff like vk::Device, vk::Instance, Allocator, etc.

I use the Drop trait on my GfxCtx struct to run the usual destruction code (eg. ash::device::Device::destroy_device) and this is where my issue is. As far as I know, I have no way to manually destroy the last memory block for each memory type that supports general allocations, which Allocator keeps around until its Drop is run. So my GfxCtx::Drop runs, frees all allocations, destroys all Vulkan state, when the device is destroyed I get a DeviceMemory leak validation error because Allocator doesn't get dropped until afterwards.

Usually I would use core::mem::take or core::mem::drop, but that's not an option because Allocator doesn't implement Copy or Default, respectively. Which it shouldn't!

I'm no graphics programmer, but this also seems like it's not an unreasonable use case?

I could wrap it in an Option or something, but it seems reasonable enough to request a destroy_empty_blocks() function for a little more control over when allocations are destroyed.

Thoughts? Or am I missing something obvious?

[MarijnS95]

Hi! Option is a bit verbose as you'd have to constantly unwrap things. I don't think drop()ing a copy is intended use either 😅

We use ManuallyDrop for this, and make sure the allocator is dropped before our device, instance, and entrypoint is dropped:

// ... more destructing here

ManuallyDrop::drop(&mut self.allocator);

ManuallyDrop::drop(&mut self.device);
ManuallyDrop::drop(&mut self.instance);
ManuallyDrop::drop(&mut self.entry);

Perhaps we should list this usage in the readme examples :)

[danielkeller]

I do this the other way around: I have wrappers for vk::Device, vk::Instance, etc that destroy them when they're dropped. That way I can use the regular drop order rules to free everything in the right order. You can even implement Deref on the wrapper and call api functions directly on it.

[jhvst]

This is bit tangential, but since the last version (0.16.0) I faced memory freeing problem in my code as well. I use seemingly a similar approach as @danielkeller in which I use a wrapper for resources. I used to have a drop defined for individual buffers as well, as in here: https://github.com/periferia-labs/rivi-loader/blob/main/src/lib.rs#L744

However, since the last version the allocation cannot be owned anymore. As I have only a single memory allocator in the application, I cannot drop the allocator either because it would invalidate other buffers. I know this ask is maybe rather about Rust in general, but how would you in this case free the memory?

Per this thread it is worth noting that personally I went with the Option type for the allocator itself.

[MarijnS95]

I do this the other way around: I have wrappers for vk::Device, vk::Instance, etc that destroy them when they're dropped.

We have since cleaned up our handling of this as well (funny note: the ManualDrops above were dropping the ash::{Device, Instance, Entry} types which are all just structs with function pointers and have no drop handler to destroy anything - except Entry which would dlclose() the Vulkan library). In our case every Device, as a wrapper over ash::Device, has an Arc<> over an Instance wrapper. That way we're never cloning ash::Instance function pointer tables, and the Instance is never destroyed before all devices are gone. Implicitly our ash::Device wrapper destroys the device, after which Rust takes over and cleans up the other (non-ManuallyDrop) members including our Arc<InstanceWrapper>.

Incidentally our ash::Instance wrapper also holds the ash::Entry, guaranteeing the library is never dropped and closed before all Vulkan functions are unused.

That way I can use the regular drop order rules to free everything in the right order.

Depending on struct order to make sure the allocator is dropped before the device, etc?

You can even implement Deref on the wrapper and call api functions directly on it.

We've only implemented Deref depending on the use-case. Our ash::Device wrapper also provides higher level functions and intentionally does not expose device functions. Though that's something we can easily decouple if need desires.

[MarijnS95]

However, since the last version the allocation cannot be owned anymore. As I have only a single memory allocator in the application, I cannot drop the allocator either because it would invalidate other buffers. I know this ask is maybe rather about Rust in general, but how would you in this case free the memory?

@jhvst You can still own an Allocation, that's in fact the only way to use this type. You just can't clone it anymore, by design. As such, disallowing you to perhaps free the same (cloned) allocation multiple times.

Indeed, depending on your use-case you'll most likely have to switch to an Option<Allocation> and use .take() to move the allocation out of your struct and into fn free().

I used to have a drop defined for individual buffers as well, as in here: https://github.com/periferia-labs/rivi-loader/blob/main/src/lib.rs#L744

Note that this to_owned() function doesn't perform any ownership transition, it is implemented generally for every T: Clone and as such simply called the clone() function on the object you already owned: https://doc.rust-lang.org/src/alloc/borrow.rs.html#84-96

[jhvst]

Thanks @MarijnS95 for the detailed comment! I really appreciate the help. I followed your advice and used Option.

For posteriority, I also had to modify my mapping functions since in my approach these calls then caused the buffer to be dropped (I do not completely understand, nor do I really want to). I have the diff here: rivi-lang/rivi-loader@9a2b306

I'm sure there are better ways to fix this, but for now I don't have the time to make it elegant.

[MarijnS95]

For posteriority, I also had to modify my mapping functions since in my approach these calls then caused the buffer to be dropped (I do not completely understand, nor do I really want to). I have the diff here: periferia-labs/rivi-loader@9a2b306

At risk of explaining it against your will:

I presume you wrote something along the lines of:

if let Some(allocation) = self.allocation {
}

That will move the Option and isn't possible unless it's clone. You can however borrow it:

if let Some(allocation) = &self.allocation {
}

And all should be fine.