Store Vulkan device as a reference to avoid huge struct sizes

[speedy-lex]

Unfortunately this change is not backwards compatible.

[Jasper-Bekkers]

Could you provide some context about why this change is required, and required to break backwards compatibility (for example: can't we just put the device in a Box and not have to have the lifetime on Allocator)?

[speedy-lex]

This change reduces the Allocator type size from 1584 bytes to 104 bytes as you've probably seen.

why this change is required

It's not required, just a solution to #159.

can't we just put the device in a Box

Wrapping ash::Device in a Box still has the 1.4Kib of pointers duplicated, just that it would be on the heap.

and required to break backwards compatibility

Unfortunately I'm not sure if we can just wrap it in an Arc to avoid lifetimes, as ash-rs/ash#731 hasn't been resolved definitively.
Using a reference is good since it still lets the user use other libraries smoothly (as opposed to managing both an Arc and an owned ash::Device for other libraries).
Drawbacks include:

• No backwards compatibility. We can't do anything about this without duplicating the device since Allocator::new takes AllocatorCreateDesc which owns an ash::Device. The only other option is to leave a device in AllocatorCreateDesc and make it outlive the Allocator, but this doesn't seem like desirable behaviour.
• Lifetimes being added to Allocator and AllocatorCreateDesc. This shouldn't be an issue though since your main ash::Device (and more specifically the vulkan object) has to outlive the allocator anyways.

I think references are a good middle-ground between wrapping an ash::Device and sharing it (impeding on other libraries) or huge struct sizes from owning it (references will be smaller and faster than Arc's or Boxes).

Feel free to merge when ash-rs/ash#731 is resolved or even now

[MarijnS95]

A reference won't work in a self-referential struct. How would we denote the lifetime in this instance?

struct Device {
    device: ash::Device,
    allocator: gpu_allocator::vulkan::Allocator<'self_device>,
}

Here 'self_device is the lifetime of Device::device.

This is a very real scenario for implementations and abstractions around Vulkan.

[speedy-lex]

Hmm, I see. It's either Arc's or owned :/

[MarijnS95]

If we really want to, we could use a Cow<>. That should allow the user to set a 'static lifetime when they provided a Cow::Owned(device) (but then again doesn't allow a Box or Arc) 😅

[speedy-lex]

Nothing works here.
A real solution would be something like Drop trait on ash::Device but then allocation callbacks can't work without being passed in earlier (Drop lets Arc work normally). If I think about it, I'm not even sure everyone would be happy with a Drop implementation.

[MarijnS95]

I don't think that "nothing works" here, just that whichever way we pick is going to be opinionated unless we allow the user to store any form of ash::Device, perhaps:

enum AshDevice<'a> {
    Arc(Arc<ash::Device>),
    Box(Box<ash::Device>),
    // The below two effectively form a Cow:
    Owned(ash::Device),
    Borrowed(&'a ash::Device),
}

But the caller still has to ensure they free all their allocations and drop their Allocator before freeing the device (and likewise consider destruction order of many other Vulkan objects).

We could also decide to store just ash::vk::Device and the few function pointers that gpu-allocator actually calls, at the "cost" of no longer using a few simple higher-level wrappers.

---

Neither do I think that ash should implement ownership / Drop semantics. Regardless, whether ash stores these expensive (and growing, see i.e. Vulkan 1.4) function-pointer tables on the heap or within the struct is orthogonal to implementing ownership management via Drop. With this large, owned struct, the user has maximum flexibility to decide how they want to store their Vulkan device and structure their code. They could for example wrap their entire struct Device (i.e. the one I suggested above) in an Arc and not pay the "cost" for a second Arc inside ash.

At the same time that design doesn't work at all with the current design of Allocator, as you found out and what you made this PR for - it requires a full clone of all function pointers.

---

Given your comment above that this is "just a solution to #159", does that mean you are not impacted by this but only wanted to address this because of seeing a "misleadingly trivial" open issue? I think we're all totally open to change our design and approach here to accommodate most usage patterns in an efficient way (also why #260 was opened), but need real-world usage examples to decide which of these many solutions is the right way forward. Thanks for bringing this to our attention once again!