Revert "Slab-allocate VkDescriptorSets"

This reverts commit 96c4019214.

Reason for revert: Breaks the roll, WebGPU CTS hits the
ASSERT(totalDescriptorCount > 0)

Original change's description:
> Slab-allocate VkDescriptorSets
> 
> This introduces a slab allocator for VkDescriptorSets which creates
> a VkDescriptorPool pre-allocated with multiple VkDescriptorSets per
> BindGroupLayout. In the future, we can deduplicate pools that have
> the same, or roughly the same, descriptor counts.
> 
> This CL also removes the old DescriptorSetService and moves most of
> the functionality onto the DescriptorSetAllocator itself to keep
> the tracking logic in one place.
> 
> Bug: dawn:340
> Change-Id: I785b17f4353fb3d40c9ccc33746600d6794efe7c
> Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/19320
> Reviewed-by: Austin Eng <enga@chromium.org>
> Commit-Queue: Austin Eng <enga@chromium.org>

TBR=cwallez@chromium.org,kainino@chromium.org,enga@chromium.org,bryan.bernhart@intel.com

Change-Id: Icb58485f1080eab79b24fbcd834a89fc6206b80e
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: dawn:340
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/20280
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>

src/dawn_native/BUILD.gn

@@ -472,9 +472,8 @@ source_set("dawn_native_sources") {
       "vulkan/CommandRecordingContext.h",
       "vulkan/ComputePipelineVk.cpp",
       "vulkan/ComputePipelineVk.h",
-      "vulkan/DescriptorSetAllocation.h",
+      "vulkan/DescriptorSetService.cpp",
-      "vulkan/DescriptorSetAllocator.cpp",
+      "vulkan/DescriptorSetService.h",
-      "vulkan/DescriptorSetAllocator.h",
       "vulkan/DeviceVk.cpp",
       "vulkan/DeviceVk.h",
       "vulkan/ExternalHandle.h",

src/dawn_native/CMakeLists.txt

@@ -365,9 +365,8 @@ if (DAWN_ENABLE_VULKAN)
         "vulkan/CommandRecordingContext.h"
         "vulkan/ComputePipelineVk.cpp"
         "vulkan/ComputePipelineVk.h"
-        "vulkan/DescriptorSetAllocation.h"
+        "vulkan/DescriptorSetService.cpp"
-        "vulkan/DescriptorSetAllocator.cpp"
+        "vulkan/DescriptorSetService.h"
-        "vulkan/DescriptorSetAllocator.h"
         "vulkan/DeviceVk.cpp"
         "vulkan/DeviceVk.h"
         "vulkan/ExternalHandle.h"

src/dawn_native/vulkan/BindGroupLayoutVk.cpp

@@ -16,7 +16,7 @@
 
 #include "common/BitSetIterator.h"
 #include "dawn_native/vulkan/BindGroupVk.h"
-#include "dawn_native/vulkan/DescriptorSetAllocator.h"
+#include "dawn_native/vulkan/DescriptorSetService.h"
 #include "dawn_native/vulkan/DeviceVk.h"
 #include "dawn_native/vulkan/FencedDeleter.h"
 #include "dawn_native/vulkan/VulkanError.h"
@@ -127,10 +127,11 @@ namespace dawn_native { namespace vulkan {
             descriptorCountPerType[vulkanType]++;
         }
 
-        // TODO(enga): Consider deduping allocators for layouts with the same descriptor type
+        mPoolSizes.reserve(descriptorCountPerType.size());
-        // counts.
+        for (const auto& it : descriptorCountPerType) {
-        mDescriptorSetAllocator =
+            mPoolSizes.push_back(VkDescriptorPoolSize{it.first, it.second});
-            std::make_unique<DescriptorSetAllocator>(this, std::move(descriptorCountPerType));
+        }
+
         return {};
     }
 
@@ -144,15 +145,17 @@ namespace dawn_native { namespace vulkan {
         Device* device = ToBackend(GetDevice());
 
         // DescriptorSetLayout aren't used by execution on the GPU and can be deleted at any time,
-        // so we can destroy mHandle immediately instead of using the FencedDeleter.
+        // so we destroy mHandle immediately instead of using the FencedDeleter
-        // (Swiftshader implements this wrong b/154522740).
-        // In practice, the GPU is done with all descriptor sets because bind group deallocation
-        // refs the bind group layout so that once the bind group is finished being used, we can
-        // recycle its descriptor set.
         if (mHandle != VK_NULL_HANDLE) {
             device->fn.DestroyDescriptorSetLayout(device->GetVkDevice(), mHandle, nullptr);
             mHandle = VK_NULL_HANDLE;
         }
+
+        FencedDeleter* deleter = device->GetFencedDeleter();
+        for (const SingleDescriptorSetAllocation& allocation : mAllocations) {
+            deleter->DeleteWhenUnused(allocation.pool);
+        }
+        mAllocations.clear();
     }
 
     VkDescriptorSetLayout BindGroupLayout::GetHandle() const {
@@ -163,19 +166,79 @@ namespace dawn_native { namespace vulkan {
         Device* device,
         const BindGroupDescriptor* descriptor) {
         DescriptorSetAllocation descriptorSetAllocation;
-        DAWN_TRY_ASSIGN(descriptorSetAllocation, mDescriptorSetAllocator->Allocate());
+        DAWN_TRY_ASSIGN(descriptorSetAllocation, AllocateOneDescriptorSet());
-
         return mBindGroupAllocator.Allocate(device, descriptor, descriptorSetAllocation);
     }
 
-    void BindGroupLayout::DeallocateBindGroup(BindGroup* bindGroup,
+    void BindGroupLayout::DeallocateBindGroup(BindGroup* bindGroup) {
-                                              DescriptorSetAllocation* descriptorSetAllocation) {
-        mDescriptorSetAllocator->Deallocate(descriptorSetAllocation);
         mBindGroupAllocator.Deallocate(bindGroup);
     }
 
-    void BindGroupLayout::FinishDeallocation(Serial completedSerial) {
+    ResultOrError<DescriptorSetAllocation> BindGroupLayout::AllocateOneDescriptorSet() {
-        mDescriptorSetAllocator->FinishDeallocation(completedSerial);
+        Device* device = ToBackend(GetDevice());
+
+        // Reuse a previous allocation if available.
+        if (!mAvailableAllocations.empty()) {
+            size_t index = mAvailableAllocations.back();
+            mAvailableAllocations.pop_back();
+            return {{index, mAllocations[index].set}};
+        }
+
+        // Create a pool to hold our descriptor set.
+        // TODO(cwallez@chromium.org): This horribly inefficient, have more than one descriptor
+        // set per pool.
+        VkDescriptorPoolCreateInfo createInfo;
+        createInfo.sType = VK_STRUCTURE_TYPE_DESCRIPTOR_POOL_CREATE_INFO;
+        createInfo.pNext = nullptr;
+        createInfo.flags = 0;
+        createInfo.maxSets = 1;
+        createInfo.poolSizeCount = static_cast<uint32_t>(mPoolSizes.size());
+        createInfo.pPoolSizes = mPoolSizes.data();
+
+        VkDescriptorPool descriptorPool;
+        DAWN_TRY(CheckVkSuccess(device->fn.CreateDescriptorPool(device->GetVkDevice(), &createInfo,
+                                                                nullptr, &*descriptorPool),
+                                "CreateDescriptorPool"));
+
+        // Allocate our single set.
+        VkDescriptorSetAllocateInfo allocateInfo;
+        allocateInfo.sType = VK_STRUCTURE_TYPE_DESCRIPTOR_SET_ALLOCATE_INFO;
+        allocateInfo.pNext = nullptr;
+        allocateInfo.descriptorPool = descriptorPool;
+        allocateInfo.descriptorSetCount = 1;
+        allocateInfo.pSetLayouts = &*mHandle;
+
+        VkDescriptorSet descriptorSet;
+        MaybeError result =
+            CheckVkSuccess(device->fn.AllocateDescriptorSets(device->GetVkDevice(), &allocateInfo,
+                                                             &*descriptorSet),
+                           "AllocateDescriptorSets");
+
+        if (result.IsError()) {
+            // On an error we can destroy the pool immediately because no command references it.
+            device->fn.DestroyDescriptorPool(device->GetVkDevice(), descriptorPool, nullptr);
+            return result.AcquireError();
+        }
+
+        mAllocations.push_back({descriptorPool, descriptorSet});
+        return {{mAllocations.size() - 1, descriptorSet}};
+    }
+
+    void BindGroupLayout::DeallocateDescriptorSet(
+        DescriptorSetAllocation* descriptorSetAllocation) {
+        // We can't reuse the descriptor set right away because the Vulkan spec says in the
+        // documentation for vkCmdBindDescriptorSets that the set may be consumed any time between
+        // host execution of the command and the end of the draw/dispatch.
+        ToBackend(GetDevice())
+            ->GetDescriptorSetService()
+            ->AddDeferredDeallocation(this, descriptorSetAllocation->index);
+
+        // Clear the content of allocation so that use after frees are more visible.
+        *descriptorSetAllocation = {};
+    }
+
+    void BindGroupLayout::FinishDeallocation(size_t index) {
+        mAvailableAllocations.push_back(index);
     }
 
 }}  // namespace dawn_native::vulkan

src/dawn_native/vulkan/BindGroupLayoutVk.h

@@ -17,7 +17,6 @@
 
 #include "dawn_native/BindGroupLayout.h"
 
-#include "common/Serial.h"
 #include "common/SlabAllocator.h"
 #include "common/vulkan_platform.h"
 
@@ -26,12 +25,16 @@
 namespace dawn_native { namespace vulkan {
 
     class BindGroup;
-    struct DescriptorSetAllocation;
-    class DescriptorSetAllocator;
     class Device;
 
     VkDescriptorType VulkanDescriptorType(wgpu::BindingType type, bool isDynamic);
 
+    // Contains a descriptor set along with data necessary to track its allocation.
+    struct DescriptorSetAllocation {
+        size_t index = 0;
+        VkDescriptorSet set = VK_NULL_HANDLE;
+    };
+
     // In Vulkan descriptor pools have to be sized to an exact number of descriptors. This means
     // it's hard to have something where we can mix different types of descriptor sets because
     // we don't know if their vector of number of descriptors will be similar.
@@ -55,18 +58,31 @@ namespace dawn_native { namespace vulkan {
 
         ResultOrError<BindGroup*> AllocateBindGroup(Device* device,
                                                     const BindGroupDescriptor* descriptor);
-        void DeallocateBindGroup(BindGroup* bindGroup,
+        void DeallocateBindGroup(BindGroup* bindGroup);
-                                 DescriptorSetAllocation* descriptorSetAllocation);
+
-        void FinishDeallocation(Serial completedSerial);
+        ResultOrError<DescriptorSetAllocation> AllocateOneDescriptorSet();
+        void DeallocateDescriptorSet(DescriptorSetAllocation* descriptorSetAllocation);
+
+        // Interaction with the DescriptorSetService.
+        void FinishDeallocation(size_t index);
 
       private:
         ~BindGroupLayout() override;
         MaybeError Initialize();
 
+        std::vector<VkDescriptorPoolSize> mPoolSizes;
+
+        struct SingleDescriptorSetAllocation {
+            VkDescriptorPool pool = VK_NULL_HANDLE;
+            // Descriptor sets are freed when the pool is destroyed.
+            VkDescriptorSet set = VK_NULL_HANDLE;
+        };
+        std::vector<SingleDescriptorSetAllocation> mAllocations;
+        std::vector<size_t> mAvailableAllocations;
+
         VkDescriptorSetLayout mHandle = VK_NULL_HANDLE;
 
         SlabAllocator<BindGroup> mBindGroupAllocator;
-        std::unique_ptr<DescriptorSetAllocator> mDescriptorSetAllocator;
     };
 
 }}  // namespace dawn_native::vulkan

src/dawn_native/vulkan/BindGroupVk.cpp

@@ -115,7 +115,8 @@ namespace dawn_native { namespace vulkan {
     }
 
     BindGroup::~BindGroup() {
-        ToBackend(GetLayout())->DeallocateBindGroup(this, &mDescriptorSetAllocation);
+        ToBackend(GetLayout())->DeallocateDescriptorSet(&mDescriptorSetAllocation);
+        ToBackend(GetLayout())->DeallocateBindGroup(this);
     }
 
     VkDescriptorSet BindGroup::GetHandle() const {

src/dawn_native/vulkan/BindGroupVk.h

@@ -20,7 +20,6 @@
 #include "common/PlacementAllocated.h"
 #include "common/vulkan_platform.h"
 #include "dawn_native/vulkan/BindGroupLayoutVk.h"
-#include "dawn_native/vulkan/DescriptorSetAllocation.h"
 
 namespace dawn_native { namespace vulkan {
 

src/dawn_native/vulkan/DescriptorSetAllocation.h

@@ -1,29 +0,0 @@
-// Copyright 2020 The Dawn 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 DAWNNATIVE_VULKAN_DESCRIPTORSETALLOCATION_H_
-#define DAWNNATIVE_VULKAN_DESCRIPTORSETALLOCATION_H_
-
-namespace dawn_native { namespace vulkan {
-
-    // Contains a descriptor set along with data necessary to track its allocation.
-    struct DescriptorSetAllocation {
-        VkDescriptorSet set = VK_NULL_HANDLE;
-        uint32_t poolIndex;
-        uint16_t setIndex;
-    };
-
-}}  // namespace dawn_native::vulkan
-
-#endif  // DAWNNATIVE_VULKAN_DESCRIPTORSETALLOCATION_H_

src/dawn_native/vulkan/DescriptorSetAllocator.cpp

@@ -1,169 +0,0 @@
-// Copyright 2020 The Dawn 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 "dawn_native/vulkan/DescriptorSetAllocator.h"
-
-#include "dawn_native/vulkan/BindGroupLayoutVk.h"
-#include "dawn_native/vulkan/DeviceVk.h"
-#include "dawn_native/vulkan/FencedDeleter.h"
-#include "dawn_native/vulkan/VulkanError.h"
-
-namespace dawn_native { namespace vulkan {
-
-    // TODO(enga): Figure out this value.
-    static constexpr uint32_t kMaxDescriptorsPerPool = 512;
-
-    DescriptorSetAllocator::DescriptorSetAllocator(
-        BindGroupLayout* layout,
-        std::map<VkDescriptorType, uint32_t> descriptorCountPerType)
-        : mLayout(layout) {
-        ASSERT(layout != nullptr);
-
-        // Compute the total number of descriptors for this layout.
-        uint32_t totalDescriptorCount = 0;
-        mPoolSizes.reserve(descriptorCountPerType.size());
-        for (const auto& it : descriptorCountPerType) {
-            totalDescriptorCount += it.second;
-            mPoolSizes.push_back(VkDescriptorPoolSize{it.first, it.second});
-        }
-        ASSERT(totalDescriptorCount <= kMaxBindingsPerGroup);
-        ASSERT(totalDescriptorCount > 0);
-
-        // Compute the total number of descriptors sets that fits given the max.
-        mMaxSets = kMaxDescriptorsPerPool / totalDescriptorCount;
-        ASSERT(mMaxSets > 0);
-
-        // Grow the number of desciptors in the pool to fit the computed |mMaxSets|.
-        for (auto& poolSize : mPoolSizes) {
-            poolSize.descriptorCount *= mMaxSets;
-        }
-    }
-
-    DescriptorSetAllocator::~DescriptorSetAllocator() {
-        for (auto& pool : mDescriptorPools) {
-            ASSERT(pool.freeSetIndices.size() == mMaxSets);
-            if (pool.vkPool != VK_NULL_HANDLE) {
-                Device* device = ToBackend(mLayout->GetDevice());
-                device->GetFencedDeleter()->DeleteWhenUnused(pool.vkPool);
-            }
-        }
-    }
-
-    ResultOrError<DescriptorSetAllocation> DescriptorSetAllocator::Allocate() {
-        if (mAvailableDescriptorPoolIndices.empty()) {
-            DAWN_TRY(AllocateDescriptorPool());
-        }
-
-        ASSERT(!mAvailableDescriptorPoolIndices.empty());
-
-        const PoolIndex poolIndex = mAvailableDescriptorPoolIndices.back();
-        DescriptorPool* pool = &mDescriptorPools[poolIndex];
-
-        ASSERT(!pool->freeSetIndices.empty());
-
-        SetIndex setIndex = pool->freeSetIndices.back();
-        pool->freeSetIndices.pop_back();
-
-        if (pool->freeSetIndices.empty()) {
-            mAvailableDescriptorPoolIndices.pop_back();
-        }
-
-        return DescriptorSetAllocation{pool->sets[setIndex], poolIndex, setIndex};
-    }
-
-    void DescriptorSetAllocator::Deallocate(DescriptorSetAllocation* allocationInfo) {
-        ASSERT(allocationInfo != nullptr);
-        ASSERT(allocationInfo->set != VK_NULL_HANDLE);
-
-        // We can't reuse the descriptor set right away because the Vulkan spec says in the
-        // documentation for vkCmdBindDescriptorSets that the set may be consumed any time between
-        // host execution of the command and the end of the draw/dispatch.
-        Device* device = ToBackend(mLayout->GetDevice());
-        const Serial serial = device->GetPendingCommandSerial();
-        mPendingDeallocations.Enqueue({allocationInfo->poolIndex, allocationInfo->setIndex},
-                                      serial);
-
-        if (mLastDeallocationSerial != serial) {
-            device->EnqueueDeferredDeallocation(mLayout);
-            mLastDeallocationSerial = serial;
-        }
-
-        // Clear the content of allocation so that use after frees are more visible.
-        *allocationInfo = {};
-    }
-
-    void DescriptorSetAllocator::FinishDeallocation(Serial completedSerial) {
-        for (const Deallocation& dealloc : mPendingDeallocations.IterateUpTo(completedSerial)) {
-            ASSERT(dealloc.poolIndex < mDescriptorPools.size());
-
-            auto& freeSetIndices = mDescriptorPools[dealloc.poolIndex].freeSetIndices;
-            if (freeSetIndices.empty()) {
-                mAvailableDescriptorPoolIndices.emplace_back(dealloc.poolIndex);
-            }
-            freeSetIndices.emplace_back(dealloc.setIndex);
-        }
-        mPendingDeallocations.ClearUpTo(completedSerial);
-    }
-
-    MaybeError DescriptorSetAllocator::AllocateDescriptorPool() {
-        VkDescriptorPoolCreateInfo createInfo;
-        createInfo.sType = VK_STRUCTURE_TYPE_DESCRIPTOR_POOL_CREATE_INFO;
-        createInfo.pNext = nullptr;
-        createInfo.flags = 0;
-        createInfo.maxSets = mMaxSets;
-        createInfo.poolSizeCount = static_cast<PoolIndex>(mPoolSizes.size());
-        createInfo.pPoolSizes = mPoolSizes.data();
-
-        Device* device = ToBackend(mLayout->GetDevice());
-
-        VkDescriptorPool descriptorPool;
-        DAWN_TRY(CheckVkSuccess(device->fn.CreateDescriptorPool(device->GetVkDevice(), &createInfo,
-                                                                nullptr, &*descriptorPool),
-                                "CreateDescriptorPool"));
-
-        std::vector<VkDescriptorSetLayout> layouts(mMaxSets, mLayout->GetHandle());
-
-        VkDescriptorSetAllocateInfo allocateInfo;
-        allocateInfo.sType = VK_STRUCTURE_TYPE_DESCRIPTOR_SET_ALLOCATE_INFO;
-        allocateInfo.pNext = nullptr;
-        allocateInfo.descriptorPool = descriptorPool;
-        allocateInfo.descriptorSetCount = mMaxSets;
-        allocateInfo.pSetLayouts = AsVkArray(layouts.data());
-
-        std::vector<VkDescriptorSet> sets(mMaxSets);
-        MaybeError result =
-            CheckVkSuccess(device->fn.AllocateDescriptorSets(device->GetVkDevice(), &allocateInfo,
-                                                             AsVkArray(sets.data())),
-                           "AllocateDescriptorSets");
-        if (result.IsError()) {
-            // On an error we can destroy the pool immediately because no command references it.
-            device->fn.DestroyDescriptorPool(device->GetVkDevice(), descriptorPool, nullptr);
-            DAWN_TRY(std::move(result));
-        }
-
-        std::vector<SetIndex> freeSetIndices;
-        freeSetIndices.reserve(mMaxSets);
-
-        for (SetIndex i = 0; i < mMaxSets; ++i) {
-            freeSetIndices.push_back(i);
-        }
-
-        mAvailableDescriptorPoolIndices.push_back(mDescriptorPools.size());
-        mDescriptorPools.emplace_back(
-            DescriptorPool{descriptorPool, std::move(sets), std::move(freeSetIndices)});
-
-        return {};
-    }
-
-}}  // namespace dawn_native::vulkan

src/dawn_native/vulkan/DescriptorSetAllocator.h

@@ -1,70 +0,0 @@
-// Copyright 2020 The Dawn 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 DAWNNATIVE_VULKAN_DESCRIPTORSETALLOCATOR_H_
-#define DAWNNATIVE_VULKAN_DESCRIPTORSETALLOCATOR_H_
-
-#include "common/SerialQueue.h"
-#include "common/vulkan_platform.h"
-#include "dawn_native/Error.h"
-#include "dawn_native/vulkan/DescriptorSetAllocation.h"
-
-#include <map>
-#include <vector>
-
-namespace dawn_native { namespace vulkan {
-
-    class BindGroupLayout;
-
-    class DescriptorSetAllocator {
-        using PoolIndex = uint32_t;
-        using SetIndex = uint16_t;
-
-      public:
-        DescriptorSetAllocator(BindGroupLayout* layout,
-                               std::map<VkDescriptorType, uint32_t> descriptorCountPerType);
-        ~DescriptorSetAllocator();
-
-        ResultOrError<DescriptorSetAllocation> Allocate();
-        void Deallocate(DescriptorSetAllocation* allocationInfo);
-        void FinishDeallocation(Serial completedSerial);
-
-      private:
-        MaybeError AllocateDescriptorPool();
-
-        BindGroupLayout* mLayout;
-
-        std::vector<VkDescriptorPoolSize> mPoolSizes;
-        SetIndex mMaxSets;
-
-        struct DescriptorPool {
-            VkDescriptorPool vkPool;
-            std::vector<VkDescriptorSet> sets;
-            std::vector<SetIndex> freeSetIndices;
-        };
-
-        std::vector<PoolIndex> mAvailableDescriptorPoolIndices;
-        std::vector<DescriptorPool> mDescriptorPools;
-
-        struct Deallocation {
-            PoolIndex poolIndex;
-            SetIndex setIndex;
-        };
-        SerialQueue<Deallocation> mPendingDeallocations;
-        Serial mLastDeallocationSerial = 0;
-    };
-
-}}  // namespace dawn_native::vulkan
-
-#endif  // DAWNNATIVE_VULKAN_DESCRIPTORSETALLOCATOR_H_

src/dawn_native/vulkan/DescriptorSetService.cpp

@@ -0,0 +1,41 @@
+// Copyright 2019 The Dawn 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 "dawn_native/vulkan/DescriptorSetService.h"
+
+#include "dawn_native/vulkan/BindGroupLayoutVk.h"
+#include "dawn_native/vulkan/DeviceVk.h"
+
+namespace dawn_native { namespace vulkan {
+
+    DescriptorSetService::DescriptorSetService(Device* device) : mDevice(device) {
+    }
+
+    DescriptorSetService::~DescriptorSetService() {
+        ASSERT(mDeallocations.Empty());
+    }
+
+    void DescriptorSetService::AddDeferredDeallocation(BindGroupLayout* layout, size_t index) {
+        mDeallocations.Enqueue({layout, index}, mDevice->GetPendingCommandSerial());
+    }
+
+    void DescriptorSetService::Tick(Serial completedSerial) {
+        for (Deallocation& dealloc : mDeallocations.IterateUpTo(completedSerial)) {
+            dealloc.layout->FinishDeallocation(dealloc.index);
+        }
+
+        mDeallocations.ClearUpTo(completedSerial);
+    }
+
+}}  // namespace dawn_native::vulkan

src/dawn_native/vulkan/DescriptorSetService.h

@@ -0,0 +1,53 @@
+// Copyright 2019 The Dawn 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 DAWNNATIVE_VULKAN_DESCRIPTORSETSERVICE_H_
+#define DAWNNATIVE_VULKAN_DESCRIPTORSETSERVICE_H_
+
+#include "common/SerialQueue.h"
+
+#include "dawn_native/vulkan/BindGroupLayoutVk.h"
+
+#include <vector>
+
+namespace dawn_native { namespace vulkan {
+
+    class BindGroupLayout;
+    class Device;
+
+    // Handles everything related to descriptor sets that isn't tied to a particular
+    // BindGroupLayout.
+    class DescriptorSetService {
+      public:
+        DescriptorSetService(Device* device);
+        ~DescriptorSetService();
+
+        // Will call layout->FinishDeallocation when the serial is passed.
+        void AddDeferredDeallocation(BindGroupLayout* layout, size_t index);
+
+        void Tick(Serial completedSerial);
+
+      private:
+        Device* mDevice;
+
+        struct Deallocation {
+            Ref<BindGroupLayout> layout;
+            size_t index;
+        };
+        SerialQueue<Deallocation> mDeallocations;
+    };
+
+}}  // namespace dawn_native::vulkan
+
+#endif  // DAWNNATIVE_VULKAN_DESCRIPTORSETSERVICE_H_

src/dawn_native/vulkan/DeviceVk.cpp

@@ -27,6 +27,7 @@
 #include "dawn_native/vulkan/BufferVk.h"
 #include "dawn_native/vulkan/CommandBufferVk.h"
 #include "dawn_native/vulkan/ComputePipelineVk.h"
+#include "dawn_native/vulkan/DescriptorSetService.h"
 #include "dawn_native/vulkan/FencedDeleter.h"
 #include "dawn_native/vulkan/PipelineLayoutVk.h"
 #include "dawn_native/vulkan/QueueVk.h"
@@ -82,6 +83,7 @@ namespace dawn_native { namespace vulkan {
             mDeleter = std::make_unique<FencedDeleter>(this);
         }
 
+        mDescriptorSetService = std::make_unique<DescriptorSetService>(this);
         mMapRequestTracker = std::make_unique<MapRequestTracker>(this);
         mRenderPassCache = std::make_unique<RenderPassCache>(this);
         mResourceMemoryAllocator = std::make_unique<ResourceMemoryAllocator>(this);
@@ -171,12 +173,7 @@ namespace dawn_native { namespace vulkan {
         CheckPassedFences();
         RecycleCompletedCommands();
 
-        for (Ref<BindGroupLayout>& bgl :
+        mDescriptorSetService->Tick(mCompletedSerial);
-             mBindGroupLayoutsPendingDeallocation.IterateUpTo(mCompletedSerial)) {
-            bgl->FinishDeallocation(mCompletedSerial);
-        }
-        mBindGroupLayoutsPendingDeallocation.ClearUpTo(mCompletedSerial);
-
         mMapRequestTracker->Tick(mCompletedSerial);
         mResourceMemoryAllocator->Tick(mCompletedSerial);
         mDeleter->Tick(mCompletedSerial);
@@ -216,6 +213,10 @@ namespace dawn_native { namespace vulkan {
         return mMapRequestTracker.get();
     }
 
+    DescriptorSetService* Device::GetDescriptorSetService() const {
+        return mDescriptorSetService.get();
+    }
+
     FencedDeleter* Device::GetFencedDeleter() const {
         return mDeleter.get();
     }
@@ -224,10 +225,6 @@ namespace dawn_native { namespace vulkan {
         return mRenderPassCache.get();
     }
 
-    void Device::EnqueueDeferredDeallocation(BindGroupLayout* bindGroupLayout) {
-        mBindGroupLayoutsPendingDeallocation.Enqueue(bindGroupLayout, GetPendingCommandSerial());
-    }
-
     CommandRecordingContext* Device::GetPendingRecordingContext() {
         ASSERT(mRecordingContext.commandBuffer != VK_NULL_HANDLE);
         mRecordingContext.used = true;
@@ -811,6 +808,7 @@ namespace dawn_native { namespace vulkan {
         mDeleter->Tick(mCompletedSerial);
 
         mMapRequestTracker = nullptr;
+        mDescriptorSetService = nullptr;
 
         // The VkRenderPasses in the cache can be destroyed immediately since all commands referring
         // to them are guaranteed to be finished executing.

src/dawn_native/vulkan/DeviceVk.h

@@ -34,8 +34,8 @@
 namespace dawn_native { namespace vulkan {
 
     class Adapter;
-    class BindGroupLayout;
     class BufferUploader;
+    class DescriptorSetService;
     class FencedDeleter;
     class MapRequestTracker;
     class RenderPassCache;
@@ -58,6 +58,7 @@ namespace dawn_native { namespace vulkan {
         VkQueue GetQueue() const;
 
         BufferUploader* GetBufferUploader() const;
+        DescriptorSetService* GetDescriptorSetService() const;
         FencedDeleter* GetFencedDeleter() const;
         MapRequestTracker* GetMapRequestTracker() const;
         RenderPassCache* GetRenderPassCache() const;
@@ -66,8 +67,6 @@ namespace dawn_native { namespace vulkan {
         Serial GetPendingCommandSerial() const override;
         MaybeError SubmitPendingCommands();
 
-        void EnqueueDeferredDeallocation(BindGroupLayout* bindGroupLayout);
-
         // Dawn Native API
 
         TextureBase* CreateTextureWrappingVulkanImage(
@@ -147,7 +146,7 @@ namespace dawn_native { namespace vulkan {
         uint32_t mQueueFamily = 0;
         VkQueue mQueue = VK_NULL_HANDLE;
 
-        SerialQueue<Ref<BindGroupLayout>> mBindGroupLayoutsPendingDeallocation;
+        std::unique_ptr<DescriptorSetService> mDescriptorSetService;
         std::unique_ptr<FencedDeleter> mDeleter;
         std::unique_ptr<MapRequestTracker> mMapRequestTracker;
         std::unique_ptr<ResourceMemoryAllocator> mResourceMemoryAllocator;

src/tests/end2end/DeprecatedAPITests.cpp

@@ -288,10 +288,6 @@ TEST_P(DeprecationTests, BGDescBindingAndEntriesBothZeroEmitsNoWarning) {
 
 // Test that creating a BG with bindings still does correct state tracking
 TEST_P(DeprecationTests, BGDescBindingStateTracking) {
-    // TODO(cwallez@chromium.org): In Vulkan it is disallowed to create 0-sized descriptor pools
-    // but the Vulkan backend doesn't special case it yet.
-    DAWN_SKIP_TEST_IF(IsVulkan());
-
     wgpu::BindGroupLayout layout = utils::MakeBindGroupLayout(device, {});
 
     // Test a case where if |bindings| wasn't taken into account, no validation error would happen