Skip to content

Commit 4f472f4

Browse files
KangzDawn LUCI CQ
Kangz
authored and
Dawn LUCI CQ
committed
[dawn][vk] Simplify code after the removal of "dynamic binding array"
 - Inline BindGroupLayoutStaticBindingOnly in BindGroupLayout since it is now the only child class. - Inline BindGroup::InitializeStaticBindings in Initialize since it is now the only code path. - Inline DescriptorSetAllocatorDynamicArray in ResourceTable and simplify the code. Bug: 463925499 Change-Id: Ia80da68543ae8ad1f92e89215b87c6a805655ffe Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/282815 Reviewed-by: dan sinclair <dsinclair@chromium.org> Commit-Queue: Corentin Wallez <cwallez@chromium.org>
1 parent 94498eb commit 4f472f4

File tree

11 files changed

+120
-295
lines changed

11 files changed

+120
-295
lines changed

‎src/dawn/native/vulkan/BindGroupLayoutVk.cpp‎

Lines changed: 36 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -181,8 +181,7 @@ VkDescriptorType VulkanDescriptorType(const BindingInfo& bindingInfo) {
181181
ResultOrError<Ref<BindGroupLayout>> BindGroupLayout::Create(
182182
Device* device,
183183
const UnpackedPtr<BindGroupLayoutDescriptor>& descriptor) {
184-
Ref<BindGroupLayoutStaticBindingOnly> bgl =
185-
AcquireRef(new BindGroupLayoutStaticBindingOnly(device, descriptor));
184+
Ref<BindGroupLayout> bgl = AcquireRef(new BindGroupLayout(device, descriptor));
186185
DAWN_TRY(bgl->Initialize());
187186
return bgl;
188187
}
@@ -194,18 +193,30 @@ BindGroupLayout::BindGroupLayout(DeviceBase* device,
194193

195194
BindGroupLayout::~BindGroupLayout() = default;
196195

197-
MaybeError BindGroupLayout::Initialize(
198-
const VkDescriptorSetLayoutCreateInfo* createInfo,
199-
absl::flat_hash_map<BindingIndex, BindingIndex> textureToStaticSamplerIndex) {
196+
MaybeError BindGroupLayout::Initialize() {
197+
Device* device = ToBackend(GetDevice());
198+
199+
VulkanStaticBindings bindings = ComputeVulkanStaticBindings(this);
200+
201+
mDescriptorSetAllocator =
202+
DescriptorSetAllocator::Create(device, std::move(bindings.descriptorCountPerType));
203+
204+
mTextureToStaticSamplerIndex = std::move(bindings.textureToStaticSamplerIndex);
205+
206+
VkDescriptorSetLayoutCreateInfo createInfo{
207+
.sType = VK_STRUCTURE_TYPE_DESCRIPTOR_SET_LAYOUT_CREATE_INFO,
208+
.pNext = nullptr,
209+
.flags = 0,
210+
.bindingCount = uint32_t(bindings.bindings.size()),
211+
.pBindings = bindings.bindings.data(),
212+
};
213+
200214
// Record cache key information now since the createInfo is not stored.
201215
StreamIn(&mCacheKey, createInfo);
202216

203-
Device* device = ToBackend(GetDevice());
204-
DAWN_TRY(CheckVkSuccess(
205-
device->fn.CreateDescriptorSetLayout(device->GetVkDevice(), createInfo, nullptr, &*mHandle),
206-
"CreateDescriptorSetLayout"));
207-
208-
mTextureToStaticSamplerIndex = std::move(textureToStaticSamplerIndex);
217+
DAWN_TRY(CheckVkSuccess(device->fn.CreateDescriptorSetLayout(device->GetVkDevice(), &createInfo,
218+
nullptr, &*mHandle),
219+
"CreateDescriptorSetLayout"));
209220

210221
SetLabelImpl();
211222

@@ -223,16 +234,30 @@ void BindGroupLayout::DestroyImpl() {
223234
device->fn.DestroyDescriptorSetLayout(device->GetVkDevice(), mHandle, nullptr);
224235
mHandle = VK_NULL_HANDLE;
225236
}
237+
238+
mDescriptorSetAllocator = nullptr;
226239
}
227240

228241
VkDescriptorSetLayout BindGroupLayout::GetHandle() const {
229242
return mHandle;
230243
}
231244

245+
ResultOrError<Ref<BindGroup>> BindGroupLayout::AllocateBindGroup(
246+
const UnpackedPtr<BindGroupDescriptor>& descriptor) {
247+
DescriptorSetAllocation descriptorSetAllocation;
248+
DAWN_TRY_ASSIGN(descriptorSetAllocation, mDescriptorSetAllocator->Allocate(GetHandle()));
249+
250+
return AcquireRef(
251+
mBindGroupAllocator->Allocate(ToBackend(GetDevice()), descriptor, descriptorSetAllocation));
252+
}
253+
232254
void BindGroupLayout::DeallocateBindGroup(BindGroup* bindGroup) {
233255
mBindGroupAllocator->Deallocate(bindGroup);
234256
}
235257

258+
void BindGroupLayout::DeallocateDescriptorSet(DescriptorSetAllocation* descriptorSetAllocation) {
259+
mDescriptorSetAllocator->Deallocate(descriptorSetAllocation);
260+
}
236261

237262
void BindGroupLayout::ReduceMemoryUsage() {
238263
mBindGroupAllocator->DeleteEmptySlabs();
@@ -250,52 +275,4 @@ void BindGroupLayout::SetLabelImpl() {
250275
SetDebugName(ToBackend(GetDevice()), mHandle, "Dawn_BindGroupLayout", GetLabel());
251276
}
252277

253-
// BindGroupLayoutStaticBindingOnly
254-
255-
BindGroupLayoutStaticBindingOnly::BindGroupLayoutStaticBindingOnly(
256-
DeviceBase* device,
257-
const UnpackedPtr<BindGroupLayoutDescriptor>& descriptor)
258-
: BindGroupLayout(device, descriptor) {}
259-
260-
BindGroupLayoutStaticBindingOnly::~BindGroupLayoutStaticBindingOnly() = default;
261-
262-
MaybeError BindGroupLayoutStaticBindingOnly::Initialize() {
263-
VulkanStaticBindings bindings = ComputeVulkanStaticBindings(this);
264-
265-
mDescriptorSetAllocator = DescriptorSetAllocator::Create(
266-
ToBackend(GetDevice()), std::move(bindings.descriptorCountPerType));
267-
268-
VkDescriptorSetLayoutCreateInfo createInfo{
269-
createInfo.sType = VK_STRUCTURE_TYPE_DESCRIPTOR_SET_LAYOUT_CREATE_INFO,
270-
createInfo.pNext = nullptr,
271-
createInfo.flags = 0,
272-
createInfo.bindingCount = uint32_t(bindings.bindings.size()),
273-
createInfo.pBindings = bindings.bindings.data(),
274-
};
275-
276-
return BindGroupLayout::Initialize(&createInfo,
277-
std::move(bindings.textureToStaticSamplerIndex));
278-
}
279-
280-
ResultOrError<Ref<BindGroup>> BindGroupLayoutStaticBindingOnly::AllocateBindGroup(
281-
const UnpackedPtr<BindGroupDescriptor>& descriptor) {
282-
Device* device = ToBackend(GetDevice());
283-
284-
DescriptorSetAllocation descriptorSetAllocation;
285-
DAWN_TRY_ASSIGN(descriptorSetAllocation, mDescriptorSetAllocator->Allocate(GetHandle()));
286-
287-
return AcquireRef(mBindGroupAllocator->Allocate(device, descriptor, descriptorSetAllocation));
288-
}
289-
290-
void BindGroupLayoutStaticBindingOnly::DeallocateDescriptorSet(
291-
DescriptorSetAllocation* descriptorSetAllocation) {
292-
mDescriptorSetAllocator->Deallocate(descriptorSetAllocation);
293-
}
294-
295-
void BindGroupLayoutStaticBindingOnly::DestroyImpl() {
296-
BindGroupLayout::DestroyImpl();
297-
298-
mDescriptorSetAllocator = nullptr;
299-
}
300-
301278
} // namespace dawn::native::vulkan

‎src/dawn/native/vulkan/BindGroupLayoutVk.h‎

Lines changed: 4 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,6 @@ namespace dawn::native::vulkan {
4545

4646
struct DescriptorSetAllocation;
4747
class DescriptorSetAllocator;
48-
class DescriptorSetAllocatorDynamicArray;
4948
class Device;
5049

5150
VkDescriptorType VulkanDescriptorType(const BindingInfo& bindingInfo);
@@ -61,11 +60,10 @@ class BindGroupLayout : public BindGroupLayoutInternalBase {
6160

6261
VkDescriptorSetLayout GetHandle() const;
6362

64-
virtual ResultOrError<Ref<BindGroup>> AllocateBindGroup(
65-
const UnpackedPtr<BindGroupDescriptor>& descriptor) = 0;
66-
virtual void DeallocateDescriptorSet(DescriptorSetAllocation* descriptorSetAllocation) = 0;
67-
63+
ResultOrError<Ref<BindGroup>> AllocateBindGroup(
64+
const UnpackedPtr<BindGroupDescriptor>& descriptor);
6865
void DeallocateBindGroup(BindGroup* bindGroup);
66+
void DeallocateDescriptorSet(DescriptorSetAllocation* descriptorSetAllocation);
6967
void ReduceMemoryUsage() override;
7068

7169
// If the client specified that the texture at `textureBinding` should be
@@ -77,9 +75,7 @@ class BindGroupLayout : public BindGroupLayoutInternalBase {
7775
BindGroupLayout(DeviceBase* device, const UnpackedPtr<BindGroupLayoutDescriptor>& descriptor);
7876
~BindGroupLayout() override;
7977

80-
MaybeError Initialize(
81-
const VkDescriptorSetLayoutCreateInfo* createInfo,
82-
absl::flat_hash_map<BindingIndex, BindingIndex> textureToStaticSamplerIndex);
78+
MaybeError Initialize();
8379
void DestroyImpl() override;
8480

8581
MutexProtected<SlabAllocator<BindGroup>> mBindGroupAllocator;
@@ -93,25 +89,6 @@ class BindGroupLayout : public BindGroupLayoutInternalBase {
9389
// Maps from indices of texture entries that are paired with static samplers
9490
// to indices of the entries of their respective samplers.
9591
absl::flat_hash_map<BindingIndex, BindingIndex> mTextureToStaticSamplerIndex;
96-
};
97-
98-
// BGL implementation with fast VkDescriptorSet allocation used when we only have static bindings.
99-
// TODO(https://issues.chromium.org/463925499): Re-inline all in BindGroupLayout since it is the
100-
// only child class.
101-
class BindGroupLayoutStaticBindingOnly final : public BindGroupLayout {
102-
public:
103-
BindGroupLayoutStaticBindingOnly(DeviceBase* device,
104-
const UnpackedPtr<BindGroupLayoutDescriptor>& descriptor);
105-
106-
MaybeError Initialize();
107-
108-
ResultOrError<Ref<BindGroup>> AllocateBindGroup(
109-
const UnpackedPtr<BindGroupDescriptor>& descriptor) override;
110-
void DeallocateDescriptorSet(DescriptorSetAllocation* descriptorSetAllocation) override;
111-
112-
private:
113-
~BindGroupLayoutStaticBindingOnly() override;
114-
void DestroyImpl() override;
11592

11693
Ref<DescriptorSetAllocator> mDescriptorSetAllocator;
11794
};

‎src/dawn/native/vulkan/BindGroupVk.cpp‎

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -66,14 +66,6 @@ BindGroup::BindGroup(Device* device,
6666
BindGroup::~BindGroup() = default;
6767

6868
MaybeError BindGroup::InitializeImpl() {
69-
DAWN_TRY(InitializeStaticBindings());
70-
71-
SetLabelImpl();
72-
return {};
73-
}
74-
75-
// TODO(https://issues.chromium.org/463925499): Re-inline in InitializeImpl.
76-
MaybeError BindGroup::InitializeStaticBindings() {
7769
const auto* layout = ToBackend(GetLayout());
7870

7971
// Now do a write of a single descriptor set with all possible chained data allocated on the
@@ -211,6 +203,7 @@ MaybeError BindGroup::InitializeStaticBindings() {
211203
// TODO(https://crbug.com/42242088): Batch these updates
212204
device->fn.UpdateDescriptorSets(device->GetVkDevice(), numWrites, writes.data(), 0, nullptr);
213205

206+
SetLabelImpl();
214207
return {};
215208
}
216209

‎src/dawn/native/vulkan/BindGroupVk.h‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,6 @@ class BindGroup final : public BindGroupBase, public PlacementAllocated {
5151

5252
VkDescriptorSet GetHandle() const;
5353

54-
void UpdateDynamicArrayBindings(const std::vector<DynamicArrayState::ResourceUpdate>& updates);
55-
5654
private:
5755
~BindGroup() override;
5856

@@ -63,8 +61,6 @@ class BindGroup final : public BindGroupBase, public PlacementAllocated {
6361
// Dawn API
6462
void SetLabelImpl() override;
6563

66-
MaybeError InitializeStaticBindings();
67-
6864
// The descriptor set in this allocation outlives the BindGroup because it is owned by
6965
// the BindGroupLayout which is referenced by the BindGroup.
7066
DescriptorSetAllocation mDescriptorSetAllocation;

‎src/dawn/native/vulkan/DescriptorSetAllocator.cpp‎

Lines changed: 0 additions & 119 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,6 @@
3737

3838
namespace dawn::native::vulkan {
3939

40-
// DescriptorSetAllocator
41-
4240
// TODO(https://crbug.com/439522242): Consider adding some better heuristic, like an exponential
4341
// increase up to a larger constant.
4442
static constexpr uint32_t kMaxDescriptorsPerPool = 512;
@@ -210,121 +208,4 @@ MaybeError DescriptorSetAllocator::AllocateDescriptorPool(VkDescriptorSetLayout
210208
return {};
211209
}
212210

213-
// DescriptorSetAllocatorDynamicArray
214-
215-
// static
216-
std::unique_ptr<DescriptorSetAllocatorDynamicArray> DescriptorSetAllocatorDynamicArray::Create(
217-
Device* device) {
218-
return std::make_unique<DescriptorSetAllocatorDynamicArray>(device);
219-
}
220-
221-
DescriptorSetAllocatorDynamicArray::DescriptorSetAllocatorDynamicArray(Device* device)
222-
: mDevice(device) {}
223-
224-
DescriptorSetAllocatorDynamicArray::~DescriptorSetAllocatorDynamicArray() {
225-
for (VkDescriptorPool pool : mPools) {
226-
if (pool != VK_NULL_HANDLE) {
227-
mDevice->GetFencedDeleter()->DeleteWhenUnused(pool);
228-
}
229-
}
230-
}
231-
232-
ResultOrError<DescriptorSetAllocation> DescriptorSetAllocatorDynamicArray::Allocate(
233-
VkDescriptorSetLayout dsLayout,
234-
const absl::flat_hash_map<VkDescriptorType, uint32_t>& descriptorCountPerType,
235-
VkDescriptorType dynamicVariableType,
236-
uint32_t dynamicVariableCount) {
237-
// Create the pool (for a single set), accounting for the extra bindings necessary for the
238-
// dynamic array.
239-
std::vector<VkDescriptorPoolSize> sizes;
240-
sizes.reserve(descriptorCountPerType.size());
241-
for (auto [type, count] : descriptorCountPerType) {
242-
if (type == dynamicVariableType) {
243-
count += dynamicVariableCount;
244-
}
245-
sizes.push_back(VkDescriptorPoolSize{type, count});
246-
}
247-
if (!descriptorCountPerType.contains(dynamicVariableType)) {
248-
// Vulkan requires that all specified pool sizes contain at least one descriptor and the
249-
// VVLs complain if the dynamic array (of size 0) uses a VkDescriptorType that doesn't have
250-
// an associated VkDescriptorPoolSize. For that reason always create a pool with at least
251-
// one descriptor in it.
252-
sizes.push_back(
253-
VkDescriptorPoolSize{dynamicVariableType, std::max(dynamicVariableCount, 1u)});
254-
}
255-
256-
VkDescriptorPoolCreateInfo createInfo{
257-
.sType = VK_STRUCTURE_TYPE_DESCRIPTOR_POOL_CREATE_INFO,
258-
.pNext = nullptr,
259-
.flags = VK_DESCRIPTOR_POOL_CREATE_UPDATE_AFTER_BIND_BIT,
260-
.maxSets = 1,
261-
.poolSizeCount = uint32_t(sizes.size()),
262-
.pPoolSizes = sizes.data(),
263-
};
264-
265-
VkDescriptorPool pool;
266-
DAWN_TRY(CheckVkSuccess(
267-
mDevice->fn.CreateDescriptorPool(mDevice->GetVkDevice(), &createInfo, nullptr, &*pool),
268-
"CreateDescriptorPool"));
269-
270-
// Immediately track the pool to make sure it is eventually destroyed if we fail descriptor set
271-
// allocation.
272-
uint32_t slot;
273-
{
274-
Mutex::AutoLock lock(&mMutex);
275-
276-
if (mAvailableSlots.empty()) {
277-
slot = static_cast<uint32_t>(mPools.size());
278-
mPools.push_back(pool);
279-
} else {
280-
slot = mAvailableSlots.back();
281-
mAvailableSlots.pop_back();
282-
283-
DAWN_ASSERT(mPools[slot] == VK_NULL_HANDLE);
284-
mPools[slot] = pool;
285-
}
286-
}
287-
288-
// Create the descriptor set, sized to account for the dynamic array.
289-
// Force the count to be at least one as some Vulkan drivers mishandle 0.
290-
uint32_t descriptorCount = std::max(1u, dynamicVariableCount);
291-
VkDescriptorSetVariableDescriptorCountAllocateInfo variableCountInfo{
292-
.sType = VK_STRUCTURE_TYPE_DESCRIPTOR_SET_VARIABLE_DESCRIPTOR_COUNT_ALLOCATE_INFO,
293-
.pNext = nullptr,
294-
.descriptorSetCount = 1,
295-
.pDescriptorCounts = &descriptorCount,
296-
};
297-
298-
VkDescriptorSetAllocateInfo allocateInfo{
299-
.sType = VK_STRUCTURE_TYPE_DESCRIPTOR_SET_ALLOCATE_INFO,
300-
.pNext = &variableCountInfo,
301-
.descriptorPool = pool,
302-
.descriptorSetCount = 1,
303-
.pSetLayouts = &*dsLayout,
304-
};
305-
306-
VkDescriptorSet set;
307-
DAWN_TRY(CheckVkSuccess(
308-
mDevice->fn.AllocateDescriptorSets(mDevice->GetVkDevice(), &allocateInfo, &*set),
309-
"AllocateDescriptorSets"));
310-
311-
return DescriptorSetAllocation{set, slot, 0};
312-
}
313-
314-
void DescriptorSetAllocatorDynamicArray::Deallocate(DescriptorSetAllocation* allocationInfo) {
315-
Mutex::AutoLock lock(&mMutex);
316-
317-
DAWN_ASSERT(allocationInfo->setIndex == 0);
318-
DAWN_ASSERT(allocationInfo->poolIndex < mPools.size());
319-
DAWN_ASSERT(mPools[allocationInfo->poolIndex] != VK_NULL_HANDLE);
320-
uint32_t slot = allocationInfo->poolIndex;
321-
322-
mDevice->GetFencedDeleter()->DeleteWhenUnused(mPools[slot]);
323-
mPools[slot] = VK_NULL_HANDLE;
324-
mAvailableSlots.push_back(slot);
325-
326-
// Clear the content of the allocation so that use after frees are more visible.
327-
*allocationInfo = {};
328-
}
329-
330211
} // namespace dawn::native::vulkan

0 commit comments

Comments
 (0)