vulkano icon indicating copy to clipboard operation
vulkano copied to clipboard

Resetting command pools instead of individual command buffers, for performance

Open Rua opened this issue 3 years ago • 1 comments

Running my Vulkano project inside vkconfig, I get the following performance warning from the validation layers:

Validation Performance Warning: [ UNASSIGNED-BestPractices-vkCreateCommandPool-command-buffer-reset ] Object 0: handle = 0x5600fad74430, type = VK_OBJECT_TYPE_DEVICE; | MessageID = 0x8728e724 | vkCreateCommandPool(): VK_COMMAND_POOL_CREATE_RESET_COMMAND_BUFFER_BIT is set. Consider resetting entire pool instead.

Vulkano uses the ability to reset and reuse command buffers in its StandardCommandPool. Whenever an AutoCommandBuffer is dropped, it returns the inner command buffer to the pool, which keeps it around until someone asks for it again. The recording process is then applied to the already-recorded command buffer, which requires the flag named in the message above. According to the message, this is less efficient than omitting this flag, which means every command buffer object must be recorded at most once. Command buffers would not be reset individually, but the entire pool would be reset at once.

At the moment, command pool resets aren't used anywhere in Vulkano, but UnsafeCommandPool has an unsafe function that does this. This function is indeed quite unsafe, because it will instantly invalidate any command buffer allocated from that pool. It's as if every command buffer turns back into a builder, ready to be recorded over again. Command pool resets seem to be the preferred strategy for Vulkan, so I wonder how this could be made safe. One way or another, it seems that a safe reset function must keep track of all its allocated command buffers, and refuse to operate if any of them are still in use. The current StandardCommandPoolPerThread already keeps a list of unused command buffers, so adding a list of used ones seems relatively simple.

If pool resets are going to be added as a replacement for resetting individual command buffers, then the current one-pool-per-thread approach used in Vulkano isn't going to be sufficient anymore. A pool can't be reset until all of its command buffers are finished, but command buffers can be recorded once and reused many times. As long as they stay alive, they block all the short-lived command buffers from being reset too. Really, what's needed is a two-tier implementation, something like what's already used for descriptor sets and buffers:

  • PersistentCommandPool: A per-thread pool that's used for long-lived command buffers. This pool could allow resetting individual command buffers, because they may have widely varying lifetimes. This is what Vulkano already has.
  • TransientCommandPool: A pool-of-pools used for short-lived (single-frame) command buffers. It operates similar to the first, but on a per-pool basis: instead of giving you individual command buffers, it hands out entire command pools. The idea is that the user receives a fresh/reused command pool every frame, allocates some (probably one-time-use) command buffers from it, then after the frame is done it gets returned to its master pool. The master pool keeps track of the state of its pools so that it knows which are resettable and which still have command buffers in use.

Rua avatar Mar 28 '21 19:03 Rua

I just dug into the RADV sources in Mesa, which handle the userspace side of Vulkan for AMD for Linux. And it turns out here, resetting a command pool actually calls the same function as resetting an individual command buffer, just in a loop for all the command buffers in the pool. So there is literally no speed difference with this driver.

There may of course be speed differences with other drivers, but it's quite possible that most of them implement command pool resets the same way. @nanokatze on the Vulkan Discord server said "best practice validation has its own belief system and is overzealous about it". So performance is not much of a motivating factor for making this change, and it could probably be scaled down. However, resetting a pool with one command buffer isn't noticeably different than resetting the command buffer itself either. So if Vulkano really cares about getting rid of the performance warning, a substitute for the current approach would be to pair every command buffer with its own command pool, and reset the pool instead of the buffer.

Rua avatar Apr 09 '21 09:04 Rua

Implemented by #2053.

Rua avatar Oct 29 '22 17:10 Rua