[Impeller] Submit a Vulkan command buffer only if its fence was successfully added to the FenceWaiterVK#187698
Conversation
…ssfully added to the FenceWaiterVK The FenceWaiterVK may be in a state where it is unable to add a new fence. This could happen if the FenceWaiterVK has been terminated during engine shutdown. If FenceWaiterVK::AddFence fails, then CommandQueueVK::Submit should not submit the command buffer. Prior to this PR, CommandQueueVK::Submit was submitting the command buffer to the queue with the fence before calling AddFence. If AddFence failed, then the fence would be destroyed even though the Vulkan driver still holds its handle. That could cause a crash when the command completes and the Vulkan driver tries to signal the fence.
There was a problem hiding this comment.
Code Review
This pull request modifies the Vulkan backend in Impeller to register fences with the FenceWaiterVK before submitting command buffers to the graphics queue, preventing potential race conditions. It also adds a RemoveFence method for cleanup on submission failure, introduces mock Vulkan support for queue submissions, and adds a unit test. Feedback on these changes includes appending the Vulkan result string to the submission error status, adding a null check for the fence entry in RemoveFence, refactoring the mock implementation to avoid const_cast by returning a non-const queue reference, and asserting that the submission status is a failure in the new unit test.
gaaclarke
left a comment
There was a problem hiding this comment.
LGTM, just some minor comments. Is there an issue that this should be linked to in the description?
| MockDevice& device() const { return device_; } | ||
|
|
||
| private: | ||
| MockDevice& device_; |
There was a problem hiding this comment.
nit: I'd prefer not to have a raw pointer here. It's not as big of a deal in testing code. We should add a comment about the ownership at least.
There was a problem hiding this comment.
Added a comment - the MockDevice owns the MockQueues, and each MockQueue holds a reference to its parent device.
|
|
||
| void FenceWaiterVK::RemoveFence(vk::Fence fence) { | ||
| std::scoped_lock lock(wait_set_mutex_); | ||
| auto match = [fence](const std::shared_ptr<WaitSetEntry>& entry) { |
There was a problem hiding this comment.
nit: I'm not sure if vk::Fence has a move constructor.
| auto match = [fence](const std::shared_ptr<WaitSetEntry>& entry) { | |
| auto match = [fence = std::move(fence)](const std::shared_ptr<WaitSetEntry>& entry) { |
There was a problem hiding this comment.
no - vk::Fence is a thin wrapper over a VkFence and does not represent ownership.
| auto buffer = context->CreateCommandBuffer(); | ||
| context->GetFenceWaiter()->Terminate(); | ||
| auto status = context->GetCommandQueue()->Submit({buffer}); | ||
| EXPECT_FALSE(status.ok()); |
There was a problem hiding this comment.
Instead of asserting ~ok() can we have an equality assert on what the error actually is?
This was a situation I noticed that could trigger a This may be related to other |
|
This approach can lead to a situation where Closing this in favor of #187761 |
The FenceWaiterVK may be in a state where it is unable to add a new fence. This could happen if the FenceWaiterVK has been terminated during engine shutdown.
If FenceWaiterVK::AddFence fails, then CommandQueueVK::Submit should not submit the command buffer.
Prior to this PR, CommandQueueVK::Submit was submitting the command buffer to the queue with the fence before calling AddFence. If AddFence failed, then the fence would be destroyed even though the Vulkan driver still holds its handle. That could cause a crash when the command completes and the Vulkan driver tries to signal the fence.
Fixes #187703