Rethink Ownership Model for Anvil Objects?

[marti4d]

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 that PhysicalDevice, 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.

[DominikWitczakAMD]

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.
1b. It wouldn't take a tremendous effort to modify Anvil so that it would release all living Vulkan object handles while handling app's request to destroy a Vulkan Instance handle. However, that would imply all wrapper instances which are still alive at this point effectively become useless. Can we guarantee all app developers will understand the implications? Don't forget Anvil is about prototyping Vulkan apps after all.
2. An orthogonal aspect is that in Vulkan, an Instance owns all Physical Devices. A Device instance created from a Physical Device, but with the parent Instance handle already destroyed, means bad stuff coming to your app very shortly. Since Anvil is about prototyping, not assuming the developers care about all this dependency stuff, the lowest-hanging fruit was to use the design we currently have.
3. Finally, it proved much more easier to go with the current design, than the way you're suggesting (which I agree would have been much cleaner but perhaps a tad more bitter on the required longer-term maintenance effort). I hadn't spent much time on fleshing it out, but fixing all the cyclic references turned out to be a non-trivial task.

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.

[marti4d]

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 Implementation

std::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 Device has shared ownership of PhysicalDevice, and that has shared ownership of Instance

Usage With Purposed Change

std::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;
}

[marti4d]

I think your current implementation exists to allow the user to destroy the Anvil::Instance and have Anvil automatically clean up everything downstream of it, but I have to ask - Is that really something that you actually want? Based on your answers above, it sounds like 'no'.

With the behavior the way it is now - allowing the user to delete Anvil::Instance directly and having it clean up everything - then that means that now the user potentially has a ton of weak_ptr<Anvil::xxx> lying all over their code pointing to destroyed objects. Is that really what the user would want or expect?

I tend to think that you probably don't want to ever allow the user to directly delete an Anvil::Instance. It should probably only be deleted automatically when it's not needed anymore, which is when everything that references it is destroyed.

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

[marti4d]

Also, think in terms of multiples - If you have 10 Device on a single PhysicalDevice, don't you want the PhysicalDevice to have a refcount of 10? And then if the user frees one of the devices, you don't want PhysicalDevice to be destroyed - you just want its refcount to drop to 9.

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.

[marti4d]

@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.

[marti4d]

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 Destroy() methods from Instance, PhysicalDevice, and Device.

Basically, you'd have to do these things, which are all good ideas(TM):

1. Remove all the code that does the whole "recursive destroy" from Instance all the way to Device.
2. Move the logic to "deregister" the object to the dtor, which is good because RAII
3. Change the register/deregister calls to store weak_ptr<T> instead of the current shared_ptr<T>. If you don't do this, the dtor will never be called because Device and PhysicalDevice would keep each other alive by mutual shared_ptr
4. Have the create functions return a shared_ptr to the user. If you don't do that, the object will delete itself immediately, since it will no longer be kept alive by its parent (which is actually how it should work)

[DominikWitczakAMD]

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.

[marti4d]

@DominikWitczakAMD But then why wouldn't you at-least return a std::unique_ptr<T>? Bjarne Stroustrup would have a fit if he knew you were returning raw pointers from an API in 2018 😛