igl | vulkan | Reuse the cached VkDescriptorSet to reduce vkAllocateDescriptorSets().#336
igl | vulkan | Reuse the cached VkDescriptorSet to reduce vkAllocateDescriptorSets().#336vinsentli wants to merge 2 commits intofacebook:mainfrom
Conversation
corporateshark
left a comment
corporateshark
left a comment
There was a problem hiding this comment.
Please post your benchmarking results with and without this approach.
src/igl/vulkan/VulkanContext.cpp
Outdated
| VkDevice device_ = VK_NULL_HANDLE; | ||
| VkDescriptorPool pool_ = VK_NULL_HANDLE; | ||
| uint32_t dsetCursor_ = 0; | ||
| std::vector<VkDescriptorSet> allocedDSet_; |
There was a problem hiding this comment.
How about a hash table descriptor set content → VkDescriptorSet?
There was a problem hiding this comment.
I'm still thinking about it.
src/igl/vulkan/VulkanContext.cpp
Outdated
| VkDescriptorPool pool_ = VK_NULL_HANDLE; | ||
| uint32_t dsetCursor_ = 0; | ||
| std::vector<VkDescriptorSet> allocedDSet_; | ||
| bool isNewPool_ = true; //is a new created pool or an old cached pool |
There was a problem hiding this comment.
Is there a way to cache descriptors from old pools?
Also, a space after // is missing.
There was a problem hiding this comment.
I add a new filed allocatedDSet in ExtinctDescriptorPool to cache descriptors from old pools.
|
before: after:
|
This looks very promising. Any numeric perf improvement values you've got? I'll test it on my side as well. |
|
@corporateshark has imported this pull request. If you are a Meta employee, you can view this in D90555194. |
| VK_ASSERT(ivkAllocateDescriptorSet(&ctx_.vf_, device_, pool_, dsl_, &dset)); | ||
| allocatedDSet_.emplace_back(dset); | ||
| } else { | ||
| dset = allocatedDSet_[dsetCursor_++]; |
There was a problem hiding this comment.
Potential out-of-bounds access when reusing a descriptor pool. When a pool is reused (line 277), allocatedDSet_ is restored from the extinct pool, but numRemainingDSetsInPool_ is always reset to kNumDSetsPerPool (64) at line 266. If the pool was previously switched before all 64 descriptor sets were allocated, allocatedDSet_ will have fewer than 64 elements. Subsequent calls to getNextDescriptorSet() could then access allocatedDSet_[dsetCursor_++] at line 257 beyond the vector's bounds. Consider either: (1) setting numRemainingDSetsInPool_ based on the actual size of the restored allocatedDSet_ vector, or (2) ensuring that pools always have exactly kNumDSetsPerPool descriptor sets allocated before being marked as extinct.
There was a problem hiding this comment.
Are you encountering an out-of-bounds access issue? According to line 249, a new descriptor pool will not switch to another pool before all 64 descriptors have been allocated. I have not encountered any out-of-bounds access issue.
if (!numRemainingDSetsInPool_) {
switchToNewDescriptorPool(ic, nextSubmitHandle);
}2 participants
No description provided.