close
Skip to content

Keep track of the last bound descriptor set (Vulkan backend improvement)#8666

Closed
micb25 wants to merge 5 commits into
ocornut:masterfrom
micb25:master
Closed

Keep track of the last bound descriptor set (Vulkan backend improvement)#8666
micb25 wants to merge 5 commits into
ocornut:masterfrom
micb25:master

Conversation

@micb25
Copy link
Copy Markdown
Contributor

@micb25 micb25 commented Jun 6, 2025

This PR introduces an improvement in the Vulkan backend which reduces descriptor set changes. A local variable (last_desc_set) tracks the last bound descriptor set and avoids a rebinding the identical descriptor set. For my application, this reduced the vkCmdBindDescriptorSets calls by 67% from 19 to 7.

@ocornut
Copy link
Copy Markdown
Owner

ocornut commented Jun 6, 2025

Thank you for your PR.

this reduced the vkCmdBindDescriptorSets calls by 67% from 19 to 7.

Were you able to meaningful measure the cost of those 19 or 7 calls in a profiler?

@micb25
Copy link
Copy Markdown
Contributor Author

micb25 commented Jun 6, 2025

Thanks for your question. For these benchmarks I used a bit more complex scene that ended up in 7 vs 23 calls (patched vs unpatched) to vkCmdBindDescriptorSets. On the CPU side, it took in average 5.7 µs vs 6.4 µs (-11%) to call ImGui::Render(). On the GPU side, however, profiling with NSight Graphics didn't measure any differences (both 0.19 ms for UI rendering on a NVIDIA RTX 2060). I assume that the GPU driver keeps track of descriptor changes itself (at least NVIDIA).

Edit: I accidentally added two more commits to this PR. They have been reverted.

ocornut pushed a commit that referenced this pull request Aug 4, 2025
@ocornut
Copy link
Copy Markdown
Owner

ocornut commented Aug 4, 2025

For these benchmarks I used a bit more complex scene that ended up in 7 vs 23 calls (patched vs unpatched) to vkCmdBindDescriptorSets. On the CPU side, it took in average 5.7 µs vs 6.4 µs (-11%) to call ImGui::Render().

Honestly this sounds not meaningful and barely measurable, but I've narrowed down the patch as 4 lines and it doesn't seem to hurt to do it for now. Merged as 90025a6.

@ocornut ocornut closed this Aug 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants