Vulkan: export allocator for custom contexts#8871
Vulkan: export allocator for custom contexts#8871xFile3160 wants to merge 3 commits intohalide:mainfrom
Conversation
|
Full discussion available here: #8715 (comment) |
|
@derek-gerstmann could you review this PR? Seems you were discussing the related issue. |
|
Any news about this? Should I rebase? |
|
Hi @xFile3160 -- happy new year! Yes, please rebase (looks like you just did). I'll ping @derek-gerstmann again to look at this since he has in-depth knowledge of the Vulkan backend. |
|
Thanks for the reminder! I'll look this over this week! |
Add documented hooks that let an override of halide_vulkan_acquire_context return its own VkInstance/device yet still bootstrap Halide's allocator. The weak runtime now exposes halide_vulkan_export_memory_allocator/halide_vulkan_memory_allocator_release plus a vk_release_memory_allocator helper that only tears down Halide-owned caches. VulkanContext will lazily allocate+export Halide's allocator when an override returns nullptr. Example integration and validation lives at 9041848.
| // with the same locking used by the custom acquire/release implementations. This allows the allocator to be | ||
| // saved for future halide_vulkan_acquire_context calls that Halide will automatically issue to retrieve | ||
| // the custom context. | ||
| extern int halide_vulkan_export_memory_allocator(void *user_context, |
There was a problem hiding this comment.
I don't understand the need for this method, or for the corresponding release method. The allocator should be stored in your custom context, and held onto for the lifetime of the context. The context manages lifespan of the allocator.
| // - halide_vulkan_memory_allocator_release | ||
| // releases the internally allocated memory allocator, important for proper memory cleanup. Must have overridden halide_vulkan_acquire_context | ||
| // and halide_vulkan_release_context, and must coordinate with the same locking as the custom implementations. | ||
| extern int halide_vulkan_memory_allocator_release(void *user_context, |
There was a problem hiding this comment.
See above comment.
| return is_initialized; | ||
| } | ||
|
|
||
| WEAK int halide_vulkan_export_memory_allocator(void *user_context, halide_vulkan_memory_allocator *allocator) { |
There was a problem hiding this comment.
This doesn't actually do anything other than check to see if the allocator is null.
| return destroy_status; | ||
| } | ||
|
|
||
| WEAK int halide_vulkan_memory_allocator_release(void *user_context, |
There was a problem hiding this comment.
Not sure I understand the intent ... was it to have a public method to invoke the destructor for the allocator?
| error = halide_error_code_device_interface_no_device; | ||
| halide_error_no_device_interface(user_context); | ||
| } | ||
| // If user overrode halide_vulkan_acquire_context and returned nullptr for allocator, |
There was a problem hiding this comment.
This class shouldn't be doing anything other than holding a lock on the context. It's just a convenient wrapper for the internal methods to have a lock that lives within a scope.
| // with the same locking used by the custom acquire/release implementations. This allows the allocator to be | ||
| // saved for future halide_vulkan_acquire_context calls that Halide will automatically issue to retrieve | ||
| // the custom context. | ||
| extern int halide_vulkan_export_memory_allocator(void *user_context, |
There was a problem hiding this comment.
I'd suggest following the conventions of the context methods and naming this halide_vulkan_acquire_memory_allocator.
| // - halide_vulkan_memory_allocator_release | ||
| // releases the internally allocated memory allocator, important for proper memory cleanup. Must have overridden halide_vulkan_acquire_context | ||
| // and halide_vulkan_release_context, and must coordinate with the same locking as the custom implementations. | ||
| extern int halide_vulkan_memory_allocator_release(void *user_context, |
There was a problem hiding this comment.
Same as above. I'd suggest I'd suggest naming this halide_vulkan_release_memory_allocator.
| } | ||
|
|
||
| WEAK int halide_vulkan_export_memory_allocator(void *user_context, halide_vulkan_memory_allocator *allocator) { | ||
| halide_mutex_lock(&thread_lock); |
There was a problem hiding this comment.
This default implementation doesn't actually do anything ... shouldn't it return the allocator associated with the context?
| return halide_error_code_buffer_argument_is_null; | ||
| } | ||
|
|
||
| return vk_release_memory_allocator(user_context, (VulkanMemoryAllocator *)allocator, |
There was a problem hiding this comment.
Lifetime management is an issue here. How do we know there are no remaining uses for the allocator? Also, allocators are specific to the context, so we need to make sure the given allocator matches the one associated with the given context.
| halide_start_clock(user_context); | ||
| #endif | ||
| // make sure halide vulkan is loaded BEFORE creating allocator | ||
| debug(user_context) << "VulkanContext: Loading Vulkan function pointers for context override...\n"; |
There was a problem hiding this comment.
This is not the right place to initialize device function pointers. They are specific to the context, and should only be initialized once, which is why they are only done in the acquire_context method.
|
I've changed this patch quiet a bit actually. But @derek-gerstmann your comments make absolutely sense, and I'm going to address/explain the intent and the new API a bit better soon. Sorry, I haven't followed up either updating this patch. |
3 participants
Add documented hooks that let an override of halide_vulkan_acquire_context return its own VkInstance/device yet still bootstrap Halide's allocator. The weak runtime now exposes halide_vulkan_export_memory_allocator/halide_vulkan_memory_allocator_release plus a vk_release_memory_allocator helper that only tears down Halide-owned caches.
VulkanContext will lazily allocate+export Halide's allocator when an override returns nullptr. Example integration and validation lives at xFile3160@9041848.