[vk] implement our own descriptor allocator - #9161
Conversation
inner-daemons
left a comment
There was a problem hiding this comment.
This is really high quality stuff! A few comments, you know the deal
| } | ||
|
|
||
| #[derive(Default)] | ||
| struct Bucket { |
There was a problem hiding this comment.
Maybe comment on exactly what many of these types are?
There was a problem hiding this comment.
I left some more comments but if you have something more specific I should add let me know.
| // vkFreeDescriptorSets is documented to return: | ||
| // - VK_ERROR_UNKNOWN | ||
| // - VK_ERROR_VALIDATION_FAILED (we shouldn't encounter this one) | ||
| // wgpu-hal doesn't currently report errors in destroy functions. | ||
| // Panic here for now. It might be ok to ignore the error but the | ||
| // proper solution would probably be to lose the device. |
There was a problem hiding this comment.
Does this deserve a // TODO to lose the device instead of panicking in hal?
69c4567 to
b1fac16
Compare
|
On my list to re-review. |
inner-daemons
left a comment
There was a problem hiding this comment.
2 opinions carrying over sorta from the last review, your choice on them. I didn't look over everything again, just gonna hand this off to someone with write permission.
| pub uniform_buffer_dynamic: u32, | ||
| pub storage_buffer: u32, | ||
| pub storage_buffer_dynamic: u32, |
There was a problem hiding this comment.
I do still tihnk that the dynamic ones should have a comment explaining their purpose for being separate
There was a problem hiding this comment.
I did add a comment on DescriptorCounts about this. Do you want me to expand on it?
| } | ||
|
|
||
| #[derive(Debug, PartialEq, Eq, Hash)] | ||
| struct BucketKey { |
There was a problem hiding this comment.
This probably also warrants a comment on what exactly it corresponds to
There was a problem hiding this comment.
It's the key of the buckets HashMap. Do you have a suggestion for a doc comment? I thought its name would suffice.
cwfitzgerald
left a comment
There was a problem hiding this comment.
Looking generally good, one potential issue, which seems to be valid.
During my standard LLM audit of this it found a potential panic in the failure path:
Bug: register_layout failure panics in error cleanup
Summary
If register_layout fails during create_bind_group_layout, the error path calls destroy_bind_group_layout, which calls unregister_layout, which does .unwrap() on a bucket that was never inserted. This panics.
Trace
In device.rs, the error path after register_layout fails:
let result = self.desc_allocator.lock()
.register_layout(&self.shared.raw, &layout);
if let Err(err) = result {
unsafe { self.destroy_bind_group_layout(layout) }; // ← calls unregister_layout
return Err(err);
}In descriptor.rs, register_layout on the Vacant path:
Vacant(vacant_entry) => {
let mut bucket = Bucket::default();
bucket.create_pool(device, vacant_entry.key(), POOL_MIN_SETS)?; // ← fails here
vacant_entry.insert(bucket) // ← never reached
}The ? propagates before vacant_entry.insert(bucket), so the bucket is never inserted into self.buckets. Then unregister_layout does:
let bucket = self.buckets.get_mut(&key).unwrap(); // ← panics: no bucket for this keyTrigger conditions
Both must hold:
- First BGL with this
(DescriptorCounts, update_after_bind)combination — takes theVacantarm vkCreateDescriptorPoolreturns an error —VK_ERROR_OUT_OF_HOST_MEMORY,VK_ERROR_OUT_OF_DEVICE_MEMORY, orVK_ERROR_FRAGMENTATION
The Occupied path cannot fail (no Vulkan calls before layouts.insert), so this only affects new bucket keys.
Suggested fix
Don't route through destroy_bind_group_layout on registration failure, since the layout was never registered:
if let Err(err) = result {
// register_layout failed — layout was never registered, so don't
// call destroy_bind_group_layout (which would try to unregister).
unsafe { self.shared.raw.destroy_descriptor_set_layout(layout.raw, None) };
self.counters.bind_group_layouts.sub(1);
return Err(err);
}Making unregister_layout tolerant of missing buckets would also work but would mask genuine logic errors on the non-error paths.
|
@cwfitzgerald I fixed the issue. |
Co-authored-by: Connor Fitzgerald <connorwadefitzgerald@gmail.com>
Connections
Closes #9078.
Description
Replaces
gpu-descriptorwith our own descriptor allocator.The pool growth strategy is the same as
gpu-descriptor's.The set allocation is different,
gpu-descriptortries to allocate from newer/larger/emptier pools. As opposed to the allocator in this PR which prefers smaller/older/fuller pools to prevent fragmentation.Because the set allocation is different, the pool shrinking strategy is different,
gpu-descriptortries to destroy older pools, the allocator in this PR will destroy newer pools.A more substantial difference is that the allocator in this PR will not immediately destroy empty pools (since they might have been recently created). It will wait until there is 1/4th its capacity in other pools.
Pools are also created and destroyed when BGLs are created/destroyed rather than deferring this to BG creation/destruction.
Testing
Existing tests.
Squash or Rebase?
n/a