[vulkan] Fix command serial ordering & uses less queue submits #3818
Conversation
|
✔️ Deploy Preview for jovial-fermat-aa59dc ready! 🔨 Explore the source changes: 63e32c1 🔍 Inspect the deploy log: https://app.netlify.com/sites/jovial-fermat-aa59dc/deploys/61befb642f9d830007d65f52 😎 Browse the preview: https://deploy-preview-3818--jovial-fermat-aa59dc.netlify.app |
taichi/backends/vulkan/runtime.cpp
Outdated
| if (ti_kernel->get_ctx_buffer_size()) { | ||
| ctx_buffer = device_->allocate_memory_unique( | ||
| {ti_kernel->get_ctx_buffer_size(), | ||
| /*host_write=*/true, /*host_read=*/false, | ||
| /*export_sharing=*/false, AllocUsage::Storage}); | ||
| ctx_buffer_host = device_->allocate_memory_unique( | ||
| {ti_kernel->get_ctx_buffer_size(), | ||
| /*host_write=*/false, /*host_read=*/true, | ||
| /*export_sharing=*/false, AllocUsage::Storage}); | ||
| } |
There was a problem hiding this comment.
r we creating new buffers every time we launch a kernel?
There was a problem hiding this comment.
.. that can't be good
There was a problem hiding this comment.
It is fine, it is actually faster than before
There was a problem hiding this comment.
be that as it may, we should still at least try to reuse some of these buffers. It is fast now because VMA is doing a superb job, but what if we want to unify the runtime code for different backends?
There was a problem hiding this comment.
Also for DX11 and OpenGL this is going to be faster than a ring buffer because the drivers know for sure there isn't overlap and is free to use them async.
There was a problem hiding this comment.
So in the end only maybe Vulkan / DX12 / and Metal will benefit from a ring buffer, and all of these requires a robust memory allocator anyways.
There was a problem hiding this comment.
What about the overhead of allocating buffers on DX and GL? Are we confident that the performance gain from having things async outweighs these overheads?
There was a problem hiding this comment.
If it's buffer reuse it should be implemented in the allocator instead of the runtime
There was a problem hiding this comment.
Let's do the naive way for now, we can figure out a better way later
|
/format |
taichi/backends/vulkan/runtime.cpp
Outdated
| } | ||
|
|
||
| void CompiledTaichiKernel::command_list(CommandList *cmdlist) const { | ||
| void CompiledTaichiKernel::command_list( |
There was a problem hiding this comment.
Out of curiosity, what does the function name command_list mean here, run_command_list?
There was a problem hiding this comment.
This means record command list
There was a problem hiding this comment.
nit: Can we just rename it to record_command_list then :-) ?
| std::unique_ptr<DeviceAllocationGuard> ctx_buffer_host, ctx_buffer; | ||
|
|
||
| if (ti_kernel->get_ctx_buffer_size()) { | ||
| ctx_buffer = device_->allocate_memory_unique( |
There was a problem hiding this comment.
Would this result in memory fragmentation? Or the VMA should take care of it?
There was a problem hiding this comment.
It would to some extent, but VMA is generally pretty good, let's do the naive one for now until we kick ext arr out of context buffer and add a ring of buffer (not ring buffer but ring of buffer)
Co-authored-by: Ye Kuang <k-ye@users.noreply.github.com>
|
/format |
taichi/backends/vulkan/runtime.cpp
Outdated
| } | ||
|
|
||
| void CompiledTaichiKernel::command_list(CommandList *cmdlist) const { | ||
| void CompiledTaichiKernel::command_list( |
There was a problem hiding this comment.
nit: Can we just rename it to record_command_list then :-) ?
| std::unique_ptr<DeviceAllocationGuard> ctx_buffer_host, ctx_buffer; | ||
|
|
||
| if (ti_kernel->get_ctx_buffer_size()) { | ||
| ctx_buffer = device_->allocate_memory_unique( |
|
/format |
Fixes #3791