-
Notifications
You must be signed in to change notification settings - Fork 59
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
Rethink Ownership Model for Anvil Objects? #30
Comments
This is one of the (many) topics that would earn their well deserved place in a documentation, had we had one. The reason Anvil ultimately ended up with the reversed Instance<-PhysicalDevice<-Device dependency organization was because of the following reasons: 1a. If the app destroys a Vulkan Instance, all Vulkan handles created from that instance become obsolete, and so do their wrapper instances. If you feel strong about the current model being misleading, let me know. If the reasons are convincing enough, I'm happy to reconsider my view. |
Based on your points above though, it actually sounds like you're arguing exactly for why my suggestion is actually more inline with what you want! Examples may help: Usage With Current Implementationstd::weak_ptr<Anvil::SGPUDevice> createAnvilDevice()
{
std::shared_ptr<Anvil::Instance> pInstance = Anvil::Instance::create(...);
std::weak_ptr<Anvil::PhysicalDevice> pPhysicalDevice = Anvil::PhysicalDevice::create(pInstance, ...);
std::weak_ptr<Anvil::SGPUDevice> pDevice = Anvil::SGPUDevice::create(pPhysicalDevice, ...);
return pDevice;
}
int main(int argc, char** argv)
{
std::weak_ptr<Anvil::SGPUDevice> pDevice = createAnvilDevice();
pDevice->something(); // This is invalid!!! The device was already destroyed because I let the
// shared_ptr to the Instance expire at the end of the function, and it cleaned up everything
// it owned
return 0;
} Now let's look at the same idea using a model where Usage With Purposed Changestd::shared_ptr<Anvil::SGPUDevice> createAnvilDevice()
{
std::shared_ptr<Anvil::Instance> pInstance = Anvil::Instance::create(...);
std::shared_ptr<Anvil::PhysicalDevice> pPhysicalDevice = Anvil::PhysicalDevice::create(pInstance, ...);
std::shared_ptr<Anvil::SGPUDevice> pDevice = Anvil::SGPUDevice::create(pPhysicalDevice, ...);
return pDevice;
}
int main(int argc, char** argv)
{
std::shared_ptr<Anvil::SGPUDevice> pDevice = createAnvilDevice();
pDevice->something(); // This is good - We have a strong pointer to the Device keeping the PhysicalDevice alive,
// the PhysicalDevice has a strong pointer to the Instance keeping that alive
// When we return from this function, the shared_ptr will hit '0', which will free the VkDevice and lower the
// ref count on the PhysicalDevice, which will then hit '0'. That will free the VkPhysicalDevice and lower the
// ref count on the Instance, which will finally free the VkInstance.
// All of this happens automatically without me, the user, having to do anything.
return 0;
}
|
I think your current implementation exists to allow the user to destroy the With the behavior the way it is now - allowing the user to delete I tend to think that you probably don't want to ever allow the user to directly delete an Going back to my example above to also illustrate: std::shared_ptr<Anvil::Instance> pInstance = Anvil::Instance::create(...);
std::weak_ptr<Anvil::PhysicalDevice> pPhysicalDevice = Anvil::PhysicalDevice::create(pInstance, ...);
std::weak_ptr<Anvil::SGPUDevice> pDevice = Anvil::SGPUDevice::create(pPhysicalDevice, ...);
pInstance.reset();
// User now has to know that pPhysicalDevice and pDevice are both invalid pointers now |
Also, think in terms of multiples - If you have 10 |
Not sure if anything should be changed at this point due to backward compatibility, but if you asked me for my preferred model, I'd say no implicit ownership rules at all (apart from the relationships as in the original Vulkan API, e.g a physical device already exists in an instance, you only enumerate it). I would be perfectly happy with simply adding RAII to Vulkan objects (think unique_ptr). Then the user can model whatever ownership rules that they need with usual language features. If an instance goes out of scope before a device, then oops you have a bug, deal with it. Full disclosure: my view may be skewed by how Vulkan CTS framework handles this problem. |
@MaciejJesionowskiAMD I would say that pushing ownership management to the end-user is a much lower abstraction then what Anvil seems to aim to provide. Yes, it's efficient in-that you're not using any CPU cycles for reference counting things that may only ever have a 1-to-1 mapping, but I would say that if a user is using Anvil then they've decided they're okay for trading off raw performance for a little bit of luxury. Users who care about nothing-but-performance are always free to just use the Vulkan API directly. I'm not sure at this point that "backward compatibility" is an argument that should really be made about Anvil - I don't think it's being used very much in the wild right now, and I think that it's understood that it's kind of "in-development" software at this point, and that breaking changes may be made to it. |
After thinking about the lifetime model a bit more, I realize one of the things that would easily improve it in Anvil is just to think about what you would need to do to remove all the Basically, you'd have to do these things, which are all good ideas(TM):
|
Anvil is going to make a move to raw pointers in a coming update. Apps can still use automated memory management by wrapping the returned ptrs in unique/shared ptrs, but I agree it was a bad design move to rely on automated ptrs on the library level. |
@DominikWitczakAMD But then why wouldn't you at-least return a |
FWIW don't even take my word for it: C++ Core Guildelines - I.11: Never transfer ownership by a raw pointer (T*) or reference (T&) |
I'm a bit on a fence when it comes to exposing unique pointers. They are a good idea BUT if they were to be used to wrap all objects instantiated by the library, we'd be introducing an implicit requirement of apps using exactly the same implementation of STL as Anvil. This is not a problem in general case, but in situations where someone needs to use the lib built using a different compiler, things can get quickly out of control. Of course, it's still OK for Anvil to use STL internally and I'm not paranoid enough to start reimplementing all containers and algorithms used by the library just to avoid the dependency ;-) It's really the client<->library interfaces that are my biggest worry here. |
In general you can’t design a c++ API around that use case anyway - c++ has no defined ABI, so many implementation details can and do change between compiler versions. If a user were to do that, unique_ptr would be the least of their troubles. Hell - if they even link to a different runtime lib than you do, they’re screwed. Msvcrt.lib and msvcrtxx.dll are mutually incompatible. So already your choices are to provide a different lib for msvc 2013, 2015, 2017 and static/dll versions of each, or make them compile it themselves. |
Good point. Unique pointers it shall be then! |
Addressed internally. This one took nearly a whole week to complete .. Will post the latest version early next week. |
Address #55: Binding arrays very difficult to use Address #54: Change supported NT version to sth more reasonable Address #50: Images created with mipmap data need VK_IMAGE_USAGE_TRANSFER_DST_BIT applied Address #47: Anvil build creates config.h in source tree, not build tree. Address #30: Rethink Ownership Model for Anvil Objects? Add support for VK_AMD_shader_fragment_mask. Add support for VK_AMD_shader_image_load_store_lod. Add support for VK_KHR_bind_memory2. Add support for VK_KHR_descriptor_update_template. Add support for VK_KHR_maintenance3. Expose features and properties using dedicated structs, for cleaner integration with future Vulkan extensions. Update Vulkan headers to VK 1.1.70.
The latest version addresses this issue. Thanks for reporting. |
Hey Dominik,
I'm not so sure this is something that can be easily done overnight, but after working with Anvil a little bit I'm thinking that its ownership model is reversed from what it ought-to-be.
For example, when I create an Anvil Device, I receive a
weak_ptr<SGPUDevice>
. That seems fairly weird to me - I literally just made this thing and I don't even actually own it?And then I got to thinking, "Hey wait - if I only got back a weak_ptr, then how does this object still even exist? Does somebody else have a sneaky
shared_ptr
that I don't know about?"And the answer is "yes" - Behind the scenes, it registered itself with the
PhysicalDevice
it was created from. I found that a little non-obvious given how I thought the ownership model would work, but now I see that it looks like the intended ownership model is that an "Instance-owns-a-PhysicalDevice-which-owns-a-SGPUDevice".I think I understand what the intuition was that drove this design - That you want to make sure that if the Device dies, for example, it will also kill everything downstream that relies on this - but I am wondering if that model needs to be inverted?
That is - Devices should hold shared_ptrs to their PhysicalDevices, and PhysicalDevices should hold shared_ptrs to their instances. And when I create an object, I should hold a shared_ptr to the object I just created.
I think in this model, it makes sense - If I create a Device and you give me the only shared_ptr to it back, and I let that shared_ptr go out-of-scope, you should be cleaning up that device because nobody can access it ever again.
And if that Device was the last device that had a shared_ptr to a certain
PhysicalDevice
, then you should release thatPhysicalDevice
, as nobody has a reference to it anymore.And so forth with the
Instance
.Anyway - I'm not totally sure I'm not missing something, but I am curious to hear your thoughts on that. I suspect it's unfortunately not a trivial effort to fix this if it is, in fact, a problem.
The text was updated successfully, but these errors were encountered: