Repository navigation
vulkan : reuse descriptor sets when bindings are constant - #29280
Conversation
|
Where is the 20-39% benefit? I don't expect any gains from this change and I think it is just a risk for bugs. |
The numbers are from a layered Vulkan implementation running gemma-2-2b Q4_K_M, same build with the feature on and off: In a layered implementation, the descriptor update crosses a guest-to-host boundary, so its slower than a local write. I agree that on a discrete GPU there may be minimal gains. An earlier version of this was unsafe, and llama.cpp's test suite caught it with 63 IM2COL_3D cases failing with ERR = inf and then a crash in TOP_K. That is why the device counts buffer destroys and a context drops its cached bindings when that count moves. With that guard, all tests pass [1] and perplexity on wikitext-2 is identical to master across all 569 chunks. [1] 18,933 of 18,933 in test-backend-opsand ci/run.sh 52/52 |
|
If we're going to optimize for this case I'd rather do something more like #24720 and reuse the whole command buffer. |
24720 is likely not thread-safe according to its author. This one is a maintainable 40 lines with an on/off flag. To be clear, though, this does become redundant if 24720 is merged. We do actually do graph-replay in our own stack, at the host layer, and it gives us about 2.15x on decode. That said, 24720 has been in draft for a few months whereas this is mergable. |
|
Ok, I don't see any bugs in the logic so I don't object. |
|
Please rebase onto latest master to fix the CI issue (needs #29279) |
Cherry-pick of ggml-org/llama.cpp PR ggml-org#29280 (ggml-org#29280) Author: Andrew Lee (@realrasengan) Reason for our setup: Cuts CPU-side overhead by reusing descriptor sets when bindings are constant
b99d497 to
d2b0b6d
Compare
My apologies for the delay. It's been rebased now, and thank you! |
|
What does "layered Vulkan implementation" mean specifically? #24720 was not pursued further because I couldn't find a system where it provided a measurable benefit. If you have a case we can look into reopening it. |
We run VMs without direct GPU access on Apple's hypervisor, using venus in the guest to serialize Vulkan commands over virtio-gpu to the host, wherein they're translated to Metal. This means every vkAllocateDescriptorSets / vkUpdateDescriptorSets costs a guest-host barrier round trip. Reusing descriptor sets when the bindings are constant avoids most of those round trips, like a cache, and gives a significant speedup! Thank you again! |
|
By the way it looks like the 2 failed tests were pre-existing (not relating to this PR)! |
|
Discrete-GPU data point, as requested: RX 7900 XTX (gfx1100, 24 GB, PCIe 3.0 x16), Linux Mesa 26.2 RADV, llama-bench, dense 27B IQ3_S,
−0.35% pp / −0.1% tg — within run-to-run noise (leg spread ~1% on pp). No functional issues across the runs. So the expectation holds for discrete RDNA3: descriptor re-recording is not a measurable cost when the driver isn't serializing over virtio — the Venus numbers don't transfer. Neutral change here: not a perf win on RADV, but also no regression or observed instability. |
…9280) * vulkan : reuse descriptor sets when bindings are constant * vulkan : bump buffer_destroy_count before destroying the buffer
…9280) * vulkan : reuse descriptor sets when bindings are constant * vulkan : bump buffer_destroy_count before destroying the buffer
Overview
Descriptor sets come from a per-context ring whose cursor resets for every graph, so when the same graph is recorded again it already lands on the same set with the same buffers, offsets and ranges as last time. This keeps the last binding list written to each set and skips
vkUpdateDescriptorSetswhen the new list is identical giving a 20-39% improved decode performance in translation layers and would be interesting to see how much it improves discrete GPU architectures broadly.For the record, a skip keyed on
VkBufferhandles alone is unsafe since a destroyed buffer's handle can be handed to a new buffer, and the set would still point at freed memory. As a result, the device counts buffer destroys and a context drops all of its cached bindings whenever that count has moved. This is coarse on purpose, since destroys are rare in steady-state decode. Without it,test-backend-ops -b Vulkan0fails 63 IM2COL_3D cases with ERR = inf and then crashes in TOP_K but with it, 18933 / 18933 pass.To be clear, the gain depends on how expensive descriptor updates are for the driver. On MoltenVK on an Apple machine there was minimal to no benefit with 0 regression.
Perplexity on wikitext-2 is identical to master, all 569 chunks.
GGML_VK_DISABLE_DESCRIPTOR_REUSE=1is a flag that can disable this patch and is read once on device creation.Requirements