From a9b98af7103675319fa81081ca57ede8843f163f Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Mon, 4 Dec 2017 16:17:37 -0500 Subject: [PATCH] Vulkan: Centralized deferred deletion, use it for BufferVk's handle This introduce a new FencedDeleter service as part of the Device objects that tracks when resources are no longer used and deletes them. BufferVk takes advantage of this to defer the deletion of its handle that was previously incorrectly delete directly in ~BufferVk. --- src/backend/CMakeLists.txt | 2 + src/backend/vulkan/BufferUploader.cpp | 10 ++--- src/backend/vulkan/BufferUploader.h | 1 - src/backend/vulkan/BufferVk.cpp | 3 +- src/backend/vulkan/FencedDeleter.cpp | 51 ++++++++++++++++++++++++++ src/backend/vulkan/FencedDeleter.h | 43 ++++++++++++++++++++++ src/backend/vulkan/MemoryAllocator.cpp | 11 ++---- src/backend/vulkan/MemoryAllocator.h | 1 - src/backend/vulkan/VulkanBackend.cpp | 31 +++++++++------- src/backend/vulkan/VulkanBackend.h | 40 +++++++++++--------- 10 files changed, 145 insertions(+), 48 deletions(-) create mode 100644 src/backend/vulkan/FencedDeleter.cpp create mode 100644 src/backend/vulkan/FencedDeleter.h diff --git a/src/backend/CMakeLists.txt b/src/backend/CMakeLists.txt index e693585007..6229f6b34c 100644 --- a/src/backend/CMakeLists.txt +++ b/src/backend/CMakeLists.txt @@ -293,6 +293,8 @@ if (NXT_ENABLE_VULKAN) ${VULKAN_DIR}/BufferUploader.h ${VULKAN_DIR}/BufferVk.cpp ${VULKAN_DIR}/BufferVk.h + ${VULKAN_DIR}/FencedDeleter.cpp + ${VULKAN_DIR}/FencedDeleter.h ${VULKAN_DIR}/MemoryAllocator.cpp ${VULKAN_DIR}/MemoryAllocator.h ${VULKAN_DIR}/VulkanBackend.cpp diff --git a/src/backend/vulkan/BufferUploader.cpp b/src/backend/vulkan/BufferUploader.cpp index 9e05199ae7..48938e1907 100644 --- a/src/backend/vulkan/BufferUploader.cpp +++ b/src/backend/vulkan/BufferUploader.cpp @@ -14,6 +14,7 @@ #include "backend/vulkan/BufferUploader.h" +#include "backend/vulkan/FencedDeleter.h" #include "backend/vulkan/MemoryAllocator.h" #include "backend/vulkan/VulkanBackend.h" @@ -25,7 +26,6 @@ namespace backend { namespace vulkan { } BufferUploader::~BufferUploader() { - ASSERT(mBuffersToDelete.Empty()); } void BufferUploader::BufferSubData(VkBuffer buffer, @@ -93,14 +93,10 @@ namespace backend { namespace vulkan { // TODO(cwallez@chromium.org): Buffers must be deleted before the memory. // This happens to work for now, but is fragile. mDevice->GetMemoryAllocator()->Free(&allocation); - mBuffersToDelete.Enqueue(stagingBuffer, mDevice->GetSerial()); + mDevice->GetFencedDeleter()->DeleteWhenUnused(stagingBuffer); } - void BufferUploader::Tick(Serial completedSerial) { - for (VkBuffer buffer : mBuffersToDelete.IterateUpTo(completedSerial)) { - mDevice->fn.DestroyBuffer(mDevice->GetVkDevice(), buffer, nullptr); - } - mBuffersToDelete.ClearUpTo(completedSerial); + void BufferUploader::Tick(Serial) { } }} // namespace backend::vulkan diff --git a/src/backend/vulkan/BufferUploader.h b/src/backend/vulkan/BufferUploader.h index 94b83a3e2a..eba130f988 100644 --- a/src/backend/vulkan/BufferUploader.h +++ b/src/backend/vulkan/BufferUploader.h @@ -36,7 +36,6 @@ namespace backend { namespace vulkan { private: Device* mDevice = nullptr; - SerialQueue mBuffersToDelete; }; }} // namespace backend::vulkan diff --git a/src/backend/vulkan/BufferVk.cpp b/src/backend/vulkan/BufferVk.cpp index d7f11c6cbf..a4e94de270 100644 --- a/src/backend/vulkan/BufferVk.cpp +++ b/src/backend/vulkan/BufferVk.cpp @@ -15,6 +15,7 @@ #include "backend/vulkan/BufferVk.h" #include "backend/vulkan/BufferUploader.h" +#include "backend/vulkan/FencedDeleter.h" #include "backend/vulkan/VulkanBackend.h" #include @@ -92,7 +93,7 @@ namespace backend { namespace vulkan { device->GetMemoryAllocator()->Free(&mMemoryAllocation); if (mHandle != VK_NULL_HANDLE) { - device->fn.DestroyBuffer(device->GetVkDevice(), mHandle, nullptr); + device->GetFencedDeleter()->DeleteWhenUnused(mHandle); mHandle = VK_NULL_HANDLE; } } diff --git a/src/backend/vulkan/FencedDeleter.cpp b/src/backend/vulkan/FencedDeleter.cpp new file mode 100644 index 0000000000..1ee61a919b --- /dev/null +++ b/src/backend/vulkan/FencedDeleter.cpp @@ -0,0 +1,51 @@ +// Copyright 2017 The NXT Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include "backend/vulkan/FencedDeleter.h" + +#include "backend/vulkan/VulkanBackend.h" + +namespace backend { namespace vulkan { + + FencedDeleter::FencedDeleter(Device* device) : mDevice(device) { + } + + FencedDeleter::~FencedDeleter() { + ASSERT(mBuffersToDelete.Empty()); + ASSERT(mMemoriesToDelete.Empty()); + } + + void FencedDeleter::DeleteWhenUnused(VkBuffer buffer) { + mBuffersToDelete.Enqueue(buffer, mDevice->GetSerial()); + } + + void FencedDeleter::DeleteWhenUnused(VkDeviceMemory memory) { + mMemoriesToDelete.Enqueue(memory, mDevice->GetSerial()); + } + + void FencedDeleter::Tick(Serial completedSerial) { + // Buffers and textures must be deleted before memories because it is invalid to free memory + // that still have resources bound to it. + for (VkBuffer buffer : mBuffersToDelete.IterateUpTo(completedSerial)) { + mDevice->fn.DestroyBuffer(mDevice->GetVkDevice(), buffer, nullptr); + } + mBuffersToDelete.ClearUpTo(completedSerial); + + for (VkDeviceMemory memory : mMemoriesToDelete.IterateUpTo(completedSerial)) { + mDevice->fn.FreeMemory(mDevice->GetVkDevice(), memory, nullptr); + } + mMemoriesToDelete.ClearUpTo(completedSerial); + } + +}} // namespace backend::vulkan diff --git a/src/backend/vulkan/FencedDeleter.h b/src/backend/vulkan/FencedDeleter.h new file mode 100644 index 0000000000..0890d023f4 --- /dev/null +++ b/src/backend/vulkan/FencedDeleter.h @@ -0,0 +1,43 @@ +// Copyright 2017 The NXT Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef BACKEND_VULKAN_FENCEDDELETER_H_ +#define BACKEND_VULKAN_FENCEDDELETER_H_ + +#include "backend/vulkan/vulkan_platform.h" +#include "common/SerialQueue.h" + +namespace backend { namespace vulkan { + + class Device; + + class FencedDeleter { + public: + FencedDeleter(Device* device); + ~FencedDeleter(); + + void DeleteWhenUnused(VkBuffer buffer); + void DeleteWhenUnused(VkDeviceMemory memory); + + void Tick(Serial completedSerial); + + private: + Device* mDevice = nullptr; + SerialQueue mBuffersToDelete; + SerialQueue mMemoriesToDelete; + }; + +}} // namespace backend::vulkan + +#endif // BACKEND_VULKAN_FENCEDDELETER_H_ diff --git a/src/backend/vulkan/MemoryAllocator.cpp b/src/backend/vulkan/MemoryAllocator.cpp index 1762666b45..37e1d99b8d 100644 --- a/src/backend/vulkan/MemoryAllocator.cpp +++ b/src/backend/vulkan/MemoryAllocator.cpp @@ -13,6 +13,8 @@ // limitations under the License. #include "backend/vulkan/MemoryAllocator.h" + +#include "backend/vulkan/FencedDeleter.h" #include "backend/vulkan/VulkanBackend.h" namespace backend { namespace vulkan { @@ -37,7 +39,6 @@ namespace backend { namespace vulkan { } MemoryAllocator::~MemoryAllocator() { - ASSERT(mReleasedMemory.Empty()); } bool MemoryAllocator::Allocate(VkMemoryRequirements requirements, @@ -121,16 +122,12 @@ namespace backend { namespace vulkan { } void MemoryAllocator::Free(DeviceMemoryAllocation* allocation) { - mReleasedMemory.Enqueue(allocation->mMemory, mDevice->GetSerial()); + mDevice->GetFencedDeleter()->DeleteWhenUnused(allocation->mMemory); allocation->mMemory = VK_NULL_HANDLE; allocation->mOffset = 0; allocation->mMappedPointer = nullptr; } - void MemoryAllocator::Tick(Serial finishedSerial) { - for (auto memory : mReleasedMemory.IterateUpTo(finishedSerial)) { - mDevice->fn.FreeMemory(mDevice->GetVkDevice(), memory, nullptr); - } - mReleasedMemory.ClearUpTo(finishedSerial); + void MemoryAllocator::Tick(Serial) { } }} // namespace backend::vulkan diff --git a/src/backend/vulkan/MemoryAllocator.h b/src/backend/vulkan/MemoryAllocator.h index 03e5f43c70..43f6fb7380 100644 --- a/src/backend/vulkan/MemoryAllocator.h +++ b/src/backend/vulkan/MemoryAllocator.h @@ -51,7 +51,6 @@ namespace backend { namespace vulkan { private: Device* mDevice = nullptr; - SerialQueue mReleasedMemory; }; }} // namespace backend::vulkan diff --git a/src/backend/vulkan/VulkanBackend.cpp b/src/backend/vulkan/VulkanBackend.cpp index 938b71fdda..ca54465ad5 100644 --- a/src/backend/vulkan/VulkanBackend.cpp +++ b/src/backend/vulkan/VulkanBackend.cpp @@ -17,6 +17,7 @@ #include "backend/Commands.h" #include "backend/vulkan/BufferUploader.h" #include "backend/vulkan/BufferVk.h" +#include "backend/vulkan/FencedDeleter.h" #include "common/Platform.h" #include @@ -107,9 +108,10 @@ namespace backend { namespace vulkan { GatherQueueFromDevice(); + mBufferUploader = new BufferUploader(this); + mDeleter = new FencedDeleter(this); mMapReadRequestTracker = new MapReadRequestTracker(this); mMemoryAllocator = new MemoryAllocator(this); - mBufferUploader = new BufferUploader(this); } Device::~Device() { @@ -139,20 +141,17 @@ namespace backend { namespace vulkan { } mUnusedFences.clear(); - if (mBufferUploader) { - delete mBufferUploader; - mBufferUploader = nullptr; - } + delete mBufferUploader; + mBufferUploader = nullptr; - if (mMemoryAllocator) { - delete mMemoryAllocator; - mMemoryAllocator = nullptr; - } + delete mDeleter; + mDeleter = nullptr; - if (mMapReadRequestTracker) { - delete mMapReadRequestTracker; - mMapReadRequestTracker = nullptr; - } + delete mMapReadRequestTracker; + mMapReadRequestTracker = nullptr; + + delete mMemoryAllocator; + mMemoryAllocator = nullptr; // VkQueues are destroyed when the VkDevice is destroyed if (mVkDevice != VK_NULL_HANDLE) { @@ -243,6 +242,8 @@ namespace backend { namespace vulkan { mBufferUploader->Tick(mCompletedSerial); mMemoryAllocator->Tick(mCompletedSerial); + mDeleter->Tick(mCompletedSerial); + if (mPendingCommands.pool != VK_NULL_HANDLE) { SubmitPendingCommands(); } @@ -264,6 +265,10 @@ namespace backend { namespace vulkan { return mBufferUploader; } + FencedDeleter* Device::GetFencedDeleter() const { + return mDeleter; + } + Serial Device::GetSerial() const { return mNextSerial; } diff --git a/src/backend/vulkan/VulkanBackend.h b/src/backend/vulkan/VulkanBackend.h index 9a66b78e43..dd49a78205 100644 --- a/src/backend/vulkan/VulkanBackend.h +++ b/src/backend/vulkan/VulkanBackend.h @@ -66,9 +66,10 @@ namespace backend { namespace vulkan { class Texture; using TextureView = TextureViewBase; + class BufferUploader; + class FencedDeleter; class MapReadRequestTracker; class MemoryAllocator; - class BufferUploader; struct VulkanBackendTraits { using BindGroupType = BindGroup; @@ -103,6 +104,24 @@ namespace backend { namespace vulkan { Device(); ~Device(); + // Contains all the Vulkan entry points, vkDoFoo is called via device->fn.DoFoo. + const VulkanFunctions fn; + + const VulkanDeviceInfo& GetDeviceInfo() const; + VkInstance GetInstance() const; + VkDevice GetVkDevice() const; + + BufferUploader* GetBufferUploader() const; + FencedDeleter* GetFencedDeleter() const; + MapReadRequestTracker* GetMapReadRequestTracker() const; + MemoryAllocator* GetMemoryAllocator() const; + + Serial GetSerial() const; + + VkCommandBuffer GetPendingCommandBuffer(); + void SubmitPendingCommands(); + + // NXT API BindGroupBase* CreateBindGroup(BindGroupBuilder* builder) override; BindGroupLayoutBase* CreateBindGroupLayout(BindGroupLayoutBuilder* builder) override; BlendStateBase* CreateBlendState(BlendStateBuilder* builder) override; @@ -125,22 +144,6 @@ namespace backend { namespace vulkan { void TickImpl() override; - const VulkanDeviceInfo& GetDeviceInfo() const; - MapReadRequestTracker* GetMapReadRequestTracker() const; - MemoryAllocator* GetMemoryAllocator() const; - BufferUploader* GetBufferUploader() const; - - Serial GetSerial() const; - - VkCommandBuffer GetPendingCommandBuffer(); - void SubmitPendingCommands(); - - // Contains all the Vulkan entry points, vkDoFoo is called via device->fn.DoFoo. - const VulkanFunctions fn; - - VkInstance GetInstance() const; - VkDevice GetVkDevice() const; - private: bool CreateInstance(VulkanGlobalKnobs* usedKnobs); bool CreateDevice(VulkanDeviceKnobs* usedKnobs); @@ -173,9 +176,10 @@ namespace backend { namespace vulkan { VkQueue mQueue = VK_NULL_HANDLE; VkDebugReportCallbackEXT mDebugReportCallback = VK_NULL_HANDLE; + BufferUploader* mBufferUploader = nullptr; + FencedDeleter* mDeleter = nullptr; MapReadRequestTracker* mMapReadRequestTracker = nullptr; MemoryAllocator* mMemoryAllocator = nullptr; - BufferUploader* mBufferUploader = nullptr; VkFence GetUnusedFence(); void CheckPassedFences();