Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RAII Constructors Taking Dispatcher #1429

Closed
jnhyatt opened this issue Oct 20, 2022 · 2 comments
Closed

Add RAII Constructors Taking Dispatcher #1429

jnhyatt opened this issue Oct 20, 2022 · 2 comments

Comments

@jnhyatt
Copy link

jnhyatt commented Oct 20, 2022

In creating an ad-hoc RAII VMA-Hpp implementation, I'm running into the following issue:

std::pair<vk::raii::Buffer, vma::raii::Allocation> createBuffer(...) const {
    auto [buffer, allocation] = m_allocator.createBuffer(...);
    return {vk::raii::Buffer(m_device, buffer), vma::raii::Allocation(...)};
}

If I want to create an RAII object that takes ownership of an already-existing Vulkan handle, I also need a vk::raii::Device on hand. Looking through the Vulkan-Hpp RAII source, RAII objects need this so they can take the VkDevice handle and the dispatcher handle. This works fine, except it imposes the restriction that if you want any RAII functionality, you must use the entire RAII ecosystem. In other words, I can't create a vk::raii::Buffer without also first creating a vk::raii::Device. I propose an additional constructor for RAII types that only takes a device/instance handle and a dispatcher:

class Buffer {
public:
    // Existing constructor
    Buffer(const vk::raii::Device& device, VkBuffer buffer)
      : m_device( *device ), m_buffer( buffer ), m_dispatcher(device.getDispatcher()) {}

    // Additional constructor
    Buffer(vk::Device device, const vk::raii::DeviceDispatcher* dispatcher, VkBuffer buffer)
      : m_device(device), m_buffer(buffer), m_dispatcher(dispatcher) {}
};

My personal motivation for this change comes from a seemingly obscure issue I'm having with making my allocator: if all objects I'm creating with it need a handle to a vk::raii::Device, my allocator needs to store a reference to it. This is fine except when the device is move constructed to a new address. The reason this is an important issue is that the other RAII classes don't have this issue. They store only the vk::Device handle and a pointer to the dispatcher, both of which never change unless the device is destroyed. The ideal solution in my vma::raii::Allocator case would be to do the same as the other RAII classes in storing only the device handle and dispatcher and creating all new RAII objects from those -- but the constructors don't exist to do so.

Besides my one use case, the other reason I can think of to add such a constructor is the overly restrictive contract created by the existing constructor. As I said earlier, while using the entire RAII ecosystem is handy, it shouldn't be necessary. The existing constructor claims than "in order to create an owning buffer object, I must be provided with the handle to own and a vk::raii::Device." This is overly restrictive because vk::raii::Buffer really doesn't need a vk::raii::Device; it only needs 1) the handle to own, 2) the dispatcher to call all its functions from, and 3) a device handle to pass as the first argument to most Vulkan functions. I could create a vk::raii::DeviceDispatcher in several other ways besides instantiating a vk::raii::Device, and I can certainly acquire a VkDevice handle in many different ways. I may not even have access to the implementation owning the device handle. In spite of this, I may want to use what parts of the RAII environment are appropriate to me. Thus, the constructor contract should allow an RAII object to be created with a dispatcher and the appropriate Vulkan handles.

As a potential piggyback to this proposal, RAII classes could potentially benefit from having a way to relinquish ownership, similar to std::unique_ptr's release():

namespace vk::raii {
    class Device {
    public:
        vk::Device release() {
            // Should the dispatcher be returned here instead of simply deleted?
            m_dispatcher.reset();
            return std::exchange(m_device, VK_NULL_HANDLE);
        }
    };
}

This would allow for extra versatility and fits with the C++ RAII paradigm. Just as with std::unique_ptr, ownership semantics are clear and explicit: ownership of the VkDevice is being relinquished, the vk::raii::Device is put into a valid, unowning state, and ownership of the device is transferred to the client code. This could (for example) provide a workaround for #929. It also would help in cases where using the entire RAII ecosystem is not desirable or possible. For example, if a device handle is provided by another implementation, I could create a vk::raii::DeviceDispatcher, then pass the device and dispatcher in to create a vk::raii::Device. This would give me the full benefits of using the RAII classes, and before my program exits, I could release ownership so the parent code that provided the device can destroy it properly.

Let me know your thoughts on both of these potential improvements!

@asuessenbach
Copy link
Contributor

Thank you very much for your suggestions! I very much appreciate that.
Regarding the release()-functions: that definitely makes sense. I'll add them.
Regarding the additional constructor, I'm not yet completely convinced... Your vma::raii::Allocator, for example, could as well hold a std::shared_ptr<vk::raii::Device>. And the vk::raii::HandleClass constructors are restrictive, but I would not call it overly restrictive. I don't see any operation that can't be done with the given constructors, at least as soon as that release()-function is in.

@asuessenbach
Copy link
Contributor

See #1433 for the release part of this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants