Repository navigation
[vulkan] Reduce the fixed per-operation cost of the runtime - #9676
mcourteaux wants to merge 5 commits into
Conversation
Measures small kernel launches on device-resident buffers (back to back and synchronized), an idle device_sync, small uploads and downloads, device_malloc + device_free, and the bandwidth of a 48 MB upload and download. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FRemi1N2Cn82dNxq2aZkF
Every operation created and destroyed a command pool and a command buffer, and waited for the whole queue to become idle. Operations now take the next submission of a ring of 16 per device, each with its own command pool, command buffer and fence, and wait on the fences of the work they submitted. halide_vulkan_device_sync only waits for Halide's own work: with nothing in flight it returns at once (from 30 us to 0.05 us on an RTX 4070), and it no longer waits for other work on a queue shared with the application. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FRemi1N2Cn82dNxq2aZkF
Like the work on a CUDA stream, kernel launches, uploads, device to device copies and the clear of a new allocation are submitted without waiting for them: a barrier at the start of each command buffer orders it after everything submitted before. Copies to the host, halide_vulkan_device_sync and halide_vulkan_device_release wait for the work in flight, as do freeing memory without reusing it, and collecting unused memory. The descriptor set and the scalar arguments of a kernel launch now belong to the submission rather than to the kernel, so a kernel can be launched again while its previous launch is still running. An upload's staging buffer is released once the upload has finished; uploads wait rather than have more than 64 MB of staging buffers in flight. A copy to the host makes the device's writes visible to the host with a barrier before its fence signals. On an RTX 4070, a small kernel launch goes from 49 us to 15 us, a 64 KiB upload from 48 us to 18 us, and a device_malloc + device_free of 64 KiB from 40 us to 13 us. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FRemi1N2Cn82dNxq2aZkF
… don't change Kernels with a dynamic workgroup size or dynamic shared memory destroyed and recreated their compute pipeline on every launch. The pipeline is now kept while the constants it was specialized with stay the same, and recreating it first waits for the work in flight that may still use it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FRemi1N2Cn82dNxq2aZkF
derek-gerstmann
left a comment
There was a problem hiding this comment.
PR Review: [vulkan] Reduce the fixed per-operation cost of the runtime
Summary
This PR replaces per-operation transient command pools/buffers and synchronous vkQueueWaitIdle calls with a per-device ring of 16 reusable VulkanSubmission slots guarded by fences and in-order execution/memory barriers, moves descriptor sets and scalar uniform buffers onto each submission, defers upload staging buffer release until fence completion (throttled at 64 MB), and caches compute pipelines across dispatches when specialization constants are unchanged. The scope matches the PR description and adds a reusable GPU dispatch overhead benchmark (test/performance/gpu_dispatch_overhead.cpp). Overall risk is high due to an asynchronous VkBuffer lifetime bug when RegionAllocator splits or coalesces a released region while prior GPU work is still in flight.
Change Surface
- Symbols: 15 changed, 16 new, 6 deleted
- High-impact entry points:
halide_vulkan_run,halide_vulkan_copy_to_device,halide_vulkan_copy_to_host,halide_vulkan_buffer_copy,halide_vulkan_device_malloc,halide_vulkan_device_free,halide_vulkan_device_sync,halide_vulkan_device_release - Surfaces touched: Vulkan runtime (
src/runtime/vulkan*.{cpp,h}), performance benchmarks (test/performance/gpu_dispatch_overhead.cpp) - Scope: Match
Blast Radius
- Deleted symbols:
ScopedVulkanCommandBufferAndPool,vk_fill_command_buffer_with_dispatch_call, andvk_submit_command_bufferhave zero remaining references. - Allocator / RegionAllocator interaction:
VulkanMemoryAllocator::collectwaits on in-flight submissions (retire = false), butRegionAllocator::split_block_regionandBlockAllocator::reserve_memory_region->RegionAllocator::collect->coalesce_block_regionsinvokeVulkanMemoryAllocator::deallocate_region(vkDestroyBuffer) directly without callingVulkanMemoryAllocator::collect. - Unupdated callers:
halide_vulkan_buffer_copy(from_host && !to_host) was not updated to callvk_throttle_stagingor clean upsubmission->staging_regionon submission failure. - Dead state:
VulkanSubmissionQueue::queue(src/runtime/vulkan_resources.h:98) is assigned invk_create_submission_queueand never read.
Test Gaps
- Region split/coalesce after async
device_free: Existinggpu_object_lifetime_*andgpu_dispatch_overheadtests allocate and free buffers of the same size (which reuses the existingBlockRegionandVkBufferin-place without callingdeallocate_region), so they do not exerciseRegionAllocator::split_block_regionorRegionAllocator::collectcoalescing while a freed buffer's kernel/clear is still in flight. - Uninitialized / repeated
halide_vulkan_device_release: Callinghalide_device_release(nullptr, halide_vulkan_device_interface())before any Vulkan allocation/dispatch or twice consecutively is not covered.
Verdict
Request changes — Please resolve the deallocate_region in-flight VkBuffer destruction, the allocator == nullptr dereference in halide_vulkan_device_release / vk_wait_for_submissions, and the halide_vulkan_buffer_copy staging throttling/cleanup issues before merge.
| // Released regions may still be in use by submitted work: wait for it to finish | ||
| // before freeing them. (Retiring the submissions would release their staging | ||
| // buffers, which must not happen here: this may be called from inside reserve().) | ||
| vk_wait_for_submissions(user_context, this, false); |
There was a problem hiding this comment.
[CRITICAL] VkBuffer handles for released device regions can still be destroyed via vkDestroyBuffer while in flight on the GPU (VUID-vkDestroyBuffer-buffer-00922).
In halide_vulkan_device_free (src/runtime/vulkan.cpp:163) and halide_vulkan_device_malloc (src/runtime/vulkan.cpp:336), when halide_can_reuse_device_allocations is true, ctx.allocator->release() marks the BlockRegion as Available without waiting for in-flight submissions. Although VulkanMemoryAllocator::collect waits here, two normal reserve() paths destroy a released region's VkBuffer (VulkanMemoryAllocator::deallocate_region -> vkDestroyBuffer) without calling VulkanMemoryAllocator::collect:
- Region splitting:
RegionAllocator::reserve->RegionAllocator::split_block_region(src/runtime/internal/region_allocator.h:198, 382) callsdeallocate_block_region_if_unused->VulkanMemoryAllocator::deallocate_regionwhen a smaller allocation splits a released region. - Region coalescing:
BlockAllocator::reserve_memory_region->RegionAllocator::collect->coalesce_block_regions(src/runtime/internal/block_allocator.h:277,src/runtime/internal/region_allocator.h:335-366) callsRegionAllocator::collectdirectly (notVulkanMemoryAllocator::collect) when a larger allocation coalesces free regions within a block.
Calling vk_wait_for_submissions(user_context, instance, false); inside VulkanMemoryAllocator::deallocate_region before vkDestroyBuffer fixes both paths while keeping same-region VkBuffer reuse wait-free.
There was a problem hiding this comment.
Agreed, fixed in 98bde6f: VulkanMemoryAllocator::deallocate_region now waits for in-flight submissions (without retiring them, since it runs inside reserve()) before vkDestroyBuffer, which covers both the split and the coalesce path. Same-size reuse stays wait-free; performance_gpu_dispatch_overhead is unchanged. Added correctness_gpu_free_while_in_flight, which frees buffers while their kernels may still run and then allocates smaller and larger ones, so released regions are split and coalesced with work in flight.
🤖 Generated with Claude Code
| &instance, &device, &physical_device, &queue, &queue_family_index, &messenger, false); | ||
|
|
||
| if (acquire_status == halide_error_code_success) { | ||
| destroy_status = vk_wait_for_submissions(user_context, allocator); |
There was a problem hiding this comment.
[HIGH] Null pointer dereference when halide_vulkan_device_release is called when no Vulkan context is active (or called twice in a row).
When cached_instance == nullptr, halide_default_vulkan_acquire_context(..., create = false) returns halide_error_code_success with allocator = nullptr (cached_allocator). Calling vk_wait_for_submissions(user_context, allocator) here unconditionally dereferences allocator->submission_queue at src/runtime/vulkan_resources.h:300 (and vk_destroy_submission_queue does the same at src/runtime/vulkan_resources.h:338).
Please guard if (allocator == nullptr) { return halide_error_code_success; } at the top of vk_wait_for_submissions and vk_destroy_submission_queue (or check if (allocator != nullptr) here).
There was a problem hiding this comment.
Agreed, fixed in 98bde6f: vk_wait_for_submissions and vk_destroy_submission_queue return early for a null allocator. halide_vulkan_device_release also no longer overwrites the wait's error with the destroy result. Added correctness_gpu_device_release_idle (release before any use, and twice after use); it segfaults without the fix.
🤖 Generated with Claude Code
| //// submit the copy. An upload's staging buffer is released by the submission | ||
| //// once it is done; a download waits for it. | ||
| if (from_host && !to_host) { | ||
| vk_attach_staging_region(submission, staging_region, src->size_in_bytes()); |
There was a problem hiding this comment.
[HIGH] halide_vulkan_buffer_copy (from_host && !to_host) attaches staging_region to submission here (which adds src->size_in_bytes() to staging_bytes_in_flight in vk_end_submission), but unlike halide_vulkan_copy_to_device (src/runtime/vulkan.cpp:461), the from_host && !to_host branch at line 815 never calls vk_throttle_staging(user_context, ctx.allocator, src->size_in_bytes()) before reserving staging_region, allowing halide_buffer_copy uploads to bypass the 64 MB vk_max_staging_bytes_in_flight limit.
There was a problem hiding this comment.
Agreed, fixed in 98bde6f: the host-to-device branch of halide_vulkan_buffer_copy now calls vk_throttle_staging before reserving the staging buffer, like halide_vulkan_copy_to_device.
🤖 Generated with Claude Code
| vk_attach_staging_region(submission, staging_region, src->size_in_bytes()); | ||
| staging_region = nullptr; | ||
| } | ||
| error_code = vk_end_submission(user_context, ctx.allocator, submission, to_host); |
There was a problem hiding this comment.
[HIGH] Above, vk_attach_staging_region sets submission->staging_region = staging_region and clears staging_region = nullptr before vk_end_submission. If vk_end_submission fails here, submission->in_flight remains false, leaving submission->staging_region dangling on an idle ring slot (unlike halide_vulkan_copy_to_device at lines 565-566 which resets submission->staging_region = nullptr and releases staging_region).
Also note that early error returns after ctx.allocator->reserve in halide_vulkan_buffer_copy (lines 836-912), halide_vulkan_copy_to_device (lines 482-536), and halide_vulkan_copy_to_host (lines 614-680) return without releasing staging_region.
There was a problem hiding this comment.
Agreed, fixed in 98bde6f. If the submit fails, the staging buffer is detached from the submission again (vk_detach_staging_region) and released. For the early returns after reserve (several of which predate this PR), the three copy functions now hold the staging buffer in a small ScopedStagingRegion that releases it on any return unless it was handed to a submission.
🤖 Generated with Claude Code
| } | ||
|
|
||
| VulkanSubmissionQueue *submission_queue = allocator->submission_queue; | ||
| if (submission_queue->queue_family_index != queue_family_index) { |
There was a problem hiding this comment.
[MEDIUM] vk_begin_submission checks submission_queue->queue_family_index != queue_family_index, but never checks submission_queue->queue != queue (leaving VulkanSubmissionQueue::queue at line 98 unread).
Because the vkCmdPipelineBarrier at line 427 only orders commands submitted to the same VkQueue, a custom acquire_context handler returning a different VkQueue within the same queue family would break inter-submission ordering unless vk_begin_submission waits for in-flight submissions on queue change (or rejects the change).
There was a problem hiding this comment.
Agreed, fixed in 98bde6f: vk_begin_submission now waits for (and retires) all in-flight submissions when the queue differs from the one used before, then switches to the new queue. This keeps ordering correct for a custom acquire_context that returns different queues of the same family.
🤖 Generated with Claude Code
| specialization_info.pData = dispatch_constant_values; | ||
|
|
||
| // Reuse the pipeline if it was created with the same constants | ||
| if (shader_bindings->compute_pipeline && (shader_bindings->specialized_constant_count == dispatch_constant_count)) { |
There was a problem hiding this comment.
[MEDIUM] If a kernel with a dynamic shared memory specialization constant is first dispatched with dispatch_constant_count > 0 (shared_mem_bytes > static_shared_mem_bytes) and later dispatched with dispatch_constant_count == 0 (shared_mem_bytes == static_shared_mem_bytes), control enters the else branch at line 1402 where shader_bindings->compute_pipeline == VK_NULL_HANDLE is false, reusing the pipeline specialized for the previous non-zero dispatch_constant_count.
Also, if vk_create_compute_pipeline fails at line 1391 after destroying the old pipeline at line 1383, shader_bindings->specialized_constant_count is not reset to 0.
There was a problem hiding this comment.
Agreed, fixed in 98bde6f. The unspecialized branch now drops a pipeline that was specialized by an earlier dispatch and creates the unspecialized one. (main had the same behavior; the stale pipeline only had a larger dynamic shared array than needed.) Both branches now share vk_drop_compute_pipeline, which also resets specialized_constant_count when the pipeline is destroyed. A failed create was already safe, since reuse also requires a non-null pipeline, but the count no longer goes stale.
🤖 Generated with Claude Code
|
|
||
| // Allocate and free a small device buffer. | ||
| Buffer<float> tmp(W, H, C); | ||
| double t_malloc_free = benchmark(10, 1, [&]() { |
There was a problem hiding this comment.
[LOW] On Vulkan, tmp.device_malloc(api, target) now submits an asynchronous vkCmdFillBuffer clear and tmp.device_free() releases the region without waiting. Because t_malloc_free does not call device_sync() at the end of the timed lambda (unlike t_dispatch and t_upload), up to vk_submission_count (16) clear submissions remain in flight at the end of each sample and spill into big.copy_to_device below.
There was a problem hiding this comment.
Agreed, fixed in 98bde6f: the malloc+free lambda ends with tmp.device_sync(), so no clears spill into the next measurement. On an RTX 4070 the number is unchanged (13.05 µs).
🤖 Generated with Claude Code
…p, release - Wait for in-flight work before destroying a buffer when a released region is split or coalesced. - Don't dereference a missing allocator in device_release, and keep the first error it returns. - Throttle staging in halide_vulkan_buffer_copy uploads, and release staging buffers on all error paths (also when a submit fails). - Wait for in-flight work when the queue changes within a queue family. - Replace a specialized pipeline when a dispatch no longer specializes it. - Sync at the end of the malloc+free benchmark. - Add correctness_gpu_free_while_in_flight and correctness_gpu_device_release_idle. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011FRemkorK6ZfoSzt7ZTqXw
Every operation of the Vulkan runtime had a fixed cost of tens of microseconds, an order of magnitude more than CUDA: a small kernel launch took ~54 µs, a 64 KiB upload ~65 µs. Pipelines with many small dispatches spent most of their time there.
Where the time went (RTX 4070, one 64×64×4 kernel per call)
Timed per step with temporary instrumentation:
vkQueueWaitIdle≈50 µs of it (≈30 µs even on an idle queue); creating and destroying the transient command pool ≈4 µs; everything else (lookup, pipeline, uniform map/memcpy, descriptor update, record,vkQueueSubmit≈0.3 µs) ≈3 µs.device_mallocalso submitted a buffer clear and waited for it.Changes
test/performance/gpu_dispatch_overhead.cpp(launches, syncs, small and large uploads and downloads, malloc/free; runs on any GPU target).vkQueueWaitIdle, sodevice_syncwaits only for Halide's own work.device_sync,device_release, frees when allocations aren't reused, andcollectwait for in-flight work.Results (
performance_gpu_dispatch_overhead, RTX 4070, µs unless noted)device_syncdevice_syncIn an application exporting a 24 MP RAW photo through 13 Halide Vulkan pipelines (one of them launched 249 times on small buffers), Halide's share went from 150 ms to 44 ms per export (with denoising: 2372 ms to 492 ms).
What's left
Behaviour changes to be aware of
halide_vulkan_get_vk_bufferfor interop) must callhalide_device_syncfirst.device_freedoesn't wait: freed memory is only reused by work submitted later, which the barrier orders after it.Tests
On the RTX 4070 with
HL_JIT_TARGET=host-vulkan-vk_v13-vk_int8-vk_int16-vk_int64(ctestforcesHL_JIT_TARGET=host, so the binaries were run directly): a 140-test correctness subset (gpu, device, async, atomics, vulkan, math, tuple, shared, extern-stage and similar) gives 129 pass, 11 skip, 0 fail, the same as on the base. Thegpu_object_lifetime_*andleak_device_memorytests pass.performance_async_gpustill fails on Vulkan when its skip is removed, as it did before.🤖 Generated with Claude Code
https://claude.ai/code/session_011FRemi1N2Cn82dNxq2aZkF