Switch to a ref-counted solution

renderdoc/driver/vulkan/vk_core.cpp

@@ -705,28 +705,17 @@ void WrappedVulkan::SetDebugMessageSink(WrappedVulkan::ScopedDebugMessageSink *s
   Threading::SetTLSValue(debugMessageSinkTLSSlot, (void *)sink);
 }
 
-void WrappedVulkan::MarkPendingCommandBufferAsDeleted(VkCommandBuffer commandBuffer)
+void WrappedVulkan::InsertPendingCommandBufferCallbacksEvent(VkCommandBuffer commandBuffer)
 {
-  SCOPED_LOCK(m_PendingCmdBufferCallbacksLock);
-
-  for(PendingCommandBufferCallbacks &pending : m_PendingCmdBufferCallbacks)
-  {
-    if(pending.commandBuffer == commandBuffer)
-    {
-      pending.commandBuffer = VK_NULL_HANDLE;
-      return;
-    }
-  }
-}
-
-void WrappedVulkan::AddPendingCommandBufferCallbacks(VkCommandBuffer commandBuffer)
-{
-  SCOPED_LOCK(m_PendingCmdBufferCallbacksLock);
+  // This occurs pre-baking as the event needs to be in the command buffer before vkEndCommandBuffer
+  // is called
 
   VkResourceRecord *cmdRecord = GetRecord(commandBuffer);
-  rdcarray<std::function<void()>> &callbacks = cmdRecord->cmdInfo->pendingSubmissionCompleteCallbacks;
+  VkPendingSubmissionCompleteCallbacks *pending =
+      cmdRecord->cmdInfo->pendingSubmissionCompleteCallbacks;
+  RDCASSERT(pending->event == VK_NULL_HANDLE);
 
-  if(callbacks.empty())
+  if(pending->callbacks.empty())
     return;
 
   const VkEventCreateInfo info = {VK_STRUCTURE_TYPE_EVENT_CREATE_INFO};
@@ -736,41 +725,61 @@ void WrappedVulkan::AddPendingCommandBufferCallbacks(VkCommandBuffer commandBuff
 
   ObjDisp(commandBuffer)->CmdSetEvent(Unwrap(commandBuffer), event, VK_PIPELINE_STAGE_TOP_OF_PIPE_BIT);
 
-  m_PendingCmdBufferCallbacks.push_back({event, commandBuffer, std::move(callbacks)});
+  pending->device = cmdRecord->cmdInfo->device;
+  pending->event = event;
+}
+
+void WrappedVulkan::AddPendingCommandBufferCallbacks(VkCommandBuffer commandBuffer)
+{
+  VkResourceRecord *cmdRecord = GetRecord(commandBuffer);
+  VkPendingSubmissionCompleteCallbacks *pending =
+      cmdRecord->bakedCommands->cmdInfo->pendingSubmissionCompleteCallbacks;
+
+  if(pending->callbacks.empty())
+    return;
+
+  RDCASSERT(pending->event != VK_NULL_HANDLE);
+
+  pending->AddRef();
+
+  SCOPED_LOCK(m_PendingCmdBufferCallbacksLock);
+  m_PendingCmdBufferCallbacks.push_back(pending);
 }
 
 void WrappedVulkan::CheckPendingCommandBufferCallbacks()
 {
+  // This approach is bad for contention, so a future optimisation could be to:
+  // 1. Acquire the lock
+  // 2. Move m_PendingCmdBufferCallbacks into a local
+  // 3. Release the lock
+  // 4. Do the checks/execution
+  // 5. Acquire the lock
+  // 6. Merge any remaining entries to m_PendingCmdBufferCallbacks (which may have accumulated new
+  //    entries from other threads)
+  // 7. Release the lock
+
   SCOPED_LOCK(m_PendingCmdBufferCallbacksLock);
 
-  // Delete any events whose command buffers have been deleted and their callbacks called
-  m_PendingCmdBufferCallbacks.removeIf([d = m_Device](const PendingCommandBufferCallbacks &pending) {
-    const bool destroyEvent = pending.commandBuffer == VK_NULL_HANDLE && pending.callbacks.empty();
-    if(destroyEvent)
-      ObjDisp(d)->DestroyEvent(Unwrap(d), pending.event, NULL);
-
-    return destroyEvent;
-  });
-
-  for(PendingCommandBufferCallbacks &pending : m_PendingCmdBufferCallbacks)
+  for(size_t i = 0; i < m_PendingCmdBufferCallbacks.size();)
   {
-    // We've already executed the callbacks, we're now just waiting for the command buffer to be
-    // destroyed/reused so we can delete the event
-    if(pending.callbacks.empty())
-      continue;
+    VkPendingSubmissionCompleteCallbacks *pending = m_PendingCmdBufferCallbacks[i];
 
-    const VkResult vkr = ObjDisp(m_Device)->GetEventStatus(Unwrap(m_Device), pending.event);
+    const VkResult vkr = ObjDisp(m_Device)->GetEventStatus(Unwrap(m_Device), pending->event);
     if(vkr == VK_EVENT_SET)
     {
-      for(std::function<void()> &f : pending.callbacks)
+      for(std::function<void()> &f : pending->callbacks)
         f();
 
-      pending.callbacks.clear();
+      pending->Release();
+      m_PendingCmdBufferCallbacks.erase(i);
+      continue;
     }
     else if(vkr != VK_EVENT_RESET)
     {
       CheckVkResult(vkr);
     }
+
+    ++i;
   }
 }
 

renderdoc/driver/vulkan/vk_core.h

@@ -979,16 +979,9 @@ private:
 
   bytebuf m_MaskedMapData;
 
-  struct PendingCommandBufferCallbacks
-  {
-    VkEvent event;
-    VkCommandBuffer commandBuffer;
-    rdcarray<std::function<void()>> callbacks;
-  };
-
   Threading::CriticalSection m_PendingCmdBufferCallbacksLock;
-  rdcarray<PendingCommandBufferCallbacks> m_PendingCmdBufferCallbacks;
-  void MarkPendingCommandBufferAsDeleted(VkCommandBuffer commandBuffer);
+  rdcarray<VkPendingSubmissionCompleteCallbacks *> m_PendingCmdBufferCallbacks;
+  void InsertPendingCommandBufferCallbacksEvent(VkCommandBuffer commandBuffer);
   void AddPendingCommandBufferCallbacks(VkCommandBuffer commandBuffer);
   void CheckPendingCommandBufferCallbacks();
 

renderdoc/driver/vulkan/vk_resources.cpp

@@ -3391,6 +3391,19 @@ VkImageAspectFlags FormatImageAspects(VkFormat fmt)
     return VK_IMAGE_ASPECT_COLOR_BIT;
 }
 
+void VkPendingSubmissionCompleteCallbacks::Release()
+{
+  int32_t ref = Atomic::Dec32(&refCount);
+  RDCASSERT(ref >= 0);
+  if(ref <= 0)
+  {
+    if(event != VK_NULL_HANDLE)
+      ObjDisp(device)->DestroyEvent(Unwrap(device), event, NULL);
+
+    delete this;
+  }
+}
+
 RenderPassInfo::RenderPassInfo(const VkRenderPassCreateInfo &ci)
 {
   // *2 in case we need separate barriers for depth and stencil, +1 for the terminating null
@@ -3843,12 +3856,12 @@ InitReqType ImgRefs::SubresourceRangeMaxInitReq(VkImageSubresourceRange range, I
   return initReq;
 }
 
-rdcarray<rdcpair<VkImageSubresourceRange, InitReqType> > ImgRefs::SubresourceRangeInitReqs(
+rdcarray<rdcpair<VkImageSubresourceRange, InitReqType>> ImgRefs::SubresourceRangeInitReqs(
     VkImageSubresourceRange range, InitPolicy policy, bool initialized) const
 {
   VkImageSubresourceRange out(range);
-  rdcarray<rdcpair<VkImageSubresourceRange, InitReqType> > res;
-  rdcarray<rdcpair<int, VkImageAspectFlags> > splitAspects;
+  rdcarray<rdcpair<VkImageSubresourceRange, InitReqType>> res;
+  rdcarray<rdcpair<int, VkImageAspectFlags>> splitAspects;
   if(areAspectsSplit)
   {
     int aspectIndex = 0;
@@ -4825,7 +4838,7 @@ TEST_CASE("Vulkan formats", "[format][vulkan]")
   {
     const uint32_t width = 24, height = 24;
 
-    rdcarray<rdcpair<VkFormat, rdcarray<uint32_t> > > tests = {
+    rdcarray<rdcpair<VkFormat, rdcarray<uint32_t>>> tests = {
         {VK_FORMAT_G8_B8_R8_3PLANE_420_UNORM, {576, 144, 144}},
         {VK_FORMAT_G8_B8R8_2PLANE_420_UNORM, {576, 288}},
         {VK_FORMAT_G8_B8_R8_3PLANE_422_UNORM, {576, 288, 288}},
@@ -4852,7 +4865,7 @@ TEST_CASE("Vulkan formats", "[format][vulkan]")
         {VK_FORMAT_G16_B16R16_2PLANE_444_UNORM, {1152, 2304}},
     };
 
-    for(rdcpair<VkFormat, rdcarray<uint32_t> > e : tests)
+    for(rdcpair<VkFormat, rdcarray<uint32_t>> e : tests)
     {
       INFO("Format is " << ToStr(e.first));
       for(uint32_t p = 0; p < e.second.size(); p++)

renderdoc/driver/vulkan/vk_resources.h

@@ -1088,6 +1088,24 @@ struct MemRefs
 struct ImgRefs;
 struct ImageState;
 
+class VkPendingSubmissionCompleteCallbacks
+{
+public:
+  VkPendingSubmissionCompleteCallbacks() = default;
+  VkPendingSubmissionCompleteCallbacks(const VkPendingSubmissionCompleteCallbacks &) = delete;
+  VkPendingSubmissionCompleteCallbacks(VkPendingSubmissionCompleteCallbacks &&) = delete;
+
+  void AddRef() { Atomic::Inc32(&refCount); }
+  void Release();
+
+  VkDevice device = VK_NULL_HANDLE;
+  VkEvent event = VK_NULL_HANDLE;
+  rdcarray<std::function<void()>> callbacks;
+
+private:
+  int32_t refCount = 1;
+};
+
 struct CmdPoolInfo
 {
   CmdPoolInfo() : pool(4 * 1024) {}
@@ -1104,13 +1122,19 @@ struct CmdPoolInfo
 
 struct CmdBufferRecordingInfo
 {
-  CmdBufferRecordingInfo(CmdPoolInfo &pool) : alloc(pool.pool) {}
+  CmdBufferRecordingInfo(CmdPoolInfo &pool)
+      : alloc(pool.pool),
+        pendingSubmissionCompleteCallbacks(new VkPendingSubmissionCompleteCallbacks())
+  {
+  }
   CmdBufferRecordingInfo(const CmdBufferRecordingInfo &) = delete;
   CmdBufferRecordingInfo(CmdBufferRecordingInfo &&) = delete;
   CmdBufferRecordingInfo &operator=(const CmdBufferRecordingInfo &) = delete;
   ~CmdBufferRecordingInfo()
   {
     // nothing to do explicitly, the alloc destructor will clean up any pages it holds
+    // pendingSubmissionCompleteCallbacks manages itself via ref-counting
+    pendingSubmissionCompleteCallbacks->Release();
   }
 
   VkDevice device;
@@ -1144,8 +1168,8 @@ struct CmdBufferRecordingInfo
   // A list of acceleration structures that this command buffer will build or copy
   rdcarray<VkResourceRecord *> accelerationStructures;
 
-  // A list of callbacks to be executed once the command buffer execution has been completed
-  rdcarray<std::function<void()>> pendingSubmissionCompleteCallbacks;
+  // The VkEvent and the list of callbacks to be executed once it has been signalled
+  VkPendingSubmissionCompleteCallbacks *pendingSubmissionCompleteCallbacks = NULL;
 
   // AdvanceFrame/Present should be called after this buffer is submitted
   bool present;
@@ -2250,8 +2274,8 @@ public:
     cmdInfo->imageStates.swap(bakedCommands->cmdInfo->imageStates);
     cmdInfo->memFrameRefs.swap(bakedCommands->cmdInfo->memFrameRefs);
     cmdInfo->accelerationStructures.swap(bakedCommands->cmdInfo->accelerationStructures);
-    cmdInfo->pendingSubmissionCompleteCallbacks.swap(
-        bakedCommands->cmdInfo->pendingSubmissionCompleteCallbacks);
+    std::swap(cmdInfo->pendingSubmissionCompleteCallbacks,
+              bakedCommands->cmdInfo->pendingSubmissionCompleteCallbacks);
   }
 
   // we have a lot of 'cold' data in the resource record, as it can be accessed

renderdoc/driver/vulkan/wrappers/vk_cmd_funcs.cpp

@@ -1382,10 +1382,7 @@ VkResult WrappedVulkan::vkBeginCommandBuffer(VkCommandBuffer commandBuffer,
     // then begin is spec'd to implicitly reset. That means we need to tidy up
     // any existing baked commands before creating a new set.
     if(record->bakedCommands)
-    {
-      MarkPendingCommandBufferAsDeleted(commandBuffer);
       record->bakedCommands->Delete(GetResourceManager());
-    }
 
     record->bakedCommands = GetResourceManager()->AddResourceRecord(ResourceIDGen::GetNewUniqueID());
     record->bakedCommands->resType = eResCommandBuffer;
@@ -1674,7 +1671,7 @@ VkResult WrappedVulkan::vkEndCommandBuffer(VkCommandBuffer commandBuffer)
   RDCASSERT(record);
 
   if(IsCaptureMode(m_State))
-    AddPendingCommandBufferCallbacks(commandBuffer);
+    InsertPendingCommandBufferCallbacksEvent(commandBuffer);
 
   VkResult ret;
   SERIALISE_TIME_CALL(ret = ObjDisp(commandBuffer)->EndCommandBuffer(Unwrap(commandBuffer)));

renderdoc/driver/vulkan/wrappers/vk_misc_funcs.cpp

@@ -345,8 +345,6 @@ void WrappedVulkan::vkFreeCommandBuffers(VkDevice device, VkCommandPool commandP
     if(pCommandBuffers[c] == VK_NULL_HANDLE)
       continue;
 
-    MarkPendingCommandBufferAsDeleted(pCommandBuffers[c]);
-
     WrappedVkDispRes *wrapped = (WrappedVkDispRes *)GetWrapped(pCommandBuffers[c]);
 
 #if ENABLED(VERBOSE_PARTIAL_REPLAY)

renderdoc/driver/vulkan/wrappers/vk_queue_funcs.cpp

@@ -1043,6 +1043,8 @@ void WrappedVulkan::CaptureQueueSubmit(VkQueue queue,
 
       record->bakedCommands->AddRef();
     }
+
+    AddPendingCommandBufferCallbacks(commandBuffers[i]);
   }
 
   if(backframe)