Efficient thread-safe ktxTexture_VkUploadEx_WithSuballocator() with queue guard callbacks

[toomuchvoltage]

So the main contribution of this PR is to make ktxTexture_VkUploadEx_WithSuballocator() more efficient in a threaded environment. Previously, the entire call would have to be guarded with a queue guard which would effectively make a single upload call block other upload calls or any Vulkan call needing the same queue. With this PR and the introduction of ktxTexture_VkUploadEx_WithSuballocatorAndQueueGuard(), only submissions to the queue inside the call are guarded individually leaving other calls to UploadEx() (or just general queue accesses from Vulkan) unblocked until they need the queue.

The PR also includes a couple of other fixes as well:

• If ktxTexture_LoadImageData() mid-call failed, it would return a failure code but leave mapped memory dangling. This normally would be wasteful but not fatal. However, if we are utilizing synchronization primitives in the callbacks this would lead to a deadlock. Mapping memory would need to enter the critical section of a memory guard (protecting VkDeviceMemory access) and unmapping would have to leave it. This is originally how the issue was discovered.
• Also changing destStageFlags = VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT | VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT; to destStageFlags = VK_PIPELINE_STAGE_ALL_COMMANDS_BIT; to allow usage of the transfer queue as well. The PR was tested in an engine which exclusively creates textures (KTX or uncompressed) on the transfer queue before doing a queue family ownership transfer to graphics. (erroneous, see below.)

This was tested on a video game environment with 385 KTX textures being loaded by 6 asset loading threads. Resolutions ranged from 5548x3636 to 32x32 with the across the board average being 1806.04x1755.35.
The execution environment had the following hardware specs:

• Intel Core i5 10400F-2.9GHz
• 16GB of RAM
• RTX 2080Ti video card

Timing statistics of upload (including the queue guard) before the optimization (3 runs in ms):

min: 0.13 max: 1105.21 avg: 273.68
min: 10.21 max: 1254.09 avg: 247.52
min: 2.88 max: 869.47 avg: 239.28

Here are the same statistics collected after the optimization:

min: 0.12 max: 448.04 avg: 170.62
min: 1.20 max: 421.39 avg: 153.39
min: 1.18 max: 429.06 avg: 160.20

Here's a screenshot of said game environment:

More information can be provided on the application if requested.

[CLAassistant]

All committers have signed the CLA.

[MarkCallow]

Also changing destStageFlags = VK_PIPELINE_STAGE_ALL_GRAPHICS_BIT | VK_PIPELINE_STAGE_COMPUTE_SHADER_BIT; to destStageFlags = VK_PIPELINE_STAGE_ALL_COMMANDS_BIT;

We used to use VK_PIPELINE_STAGE_ALL_COMMANDS_BIT but ran into a validation error in an update to the Vulkan SDK. This flags a genuine validation error. See issue #1092 and PR #1148 for details. What version of the Vulkan SDK are you using and are you testing with validation on?

[toomuchvoltage]

I'm using 1.4.313.2 and yes, around Vulkan code changes the validation layer is turned on to ensure that it's clean. I'll upgrade and try again. The solution really might be as simple as ORing with VK_PIPELINE_STAGE_TRANSFER_BIT.

[MarkCallow]

I'm using 1.4.313.2

The improved validation was post 1.4.313. The reporter of issue #1092 was using 1.4.335.

[toomuchvoltage]

I'm using 1.4.313.2

The improved validation was post 1.4.313. The reporter of issue #1092 was using 1.4.335.

You were right. This turned out to be unnecessary and I've reverted the change related to it. I realized I could just set the initial layout to TRANSFER_DST via the UploadEx() call and that would automatically line it up for access on the transfer queue. Subsequently, it could just be transitioned towards SHADER_READ_ONLY during the QFOT. The net result of all of this being illustrated here: toomuchvoltage/HighOmega-public@3d27a36

[toomuchvoltage]

Hi @MarkCallow I just added 82a5bad to demonstrate sample usage of the guarded callbacks. Truth is, it won't stress test the feature nor is that really feasible with the current single-texture test cases. Even if there were test cases requiring multiple textures, the guards within would need to be used application-wide where ever applicable. (i.e. if the graphics queue is creating textures, that would mean re-use for all accesses to graphics queue. Or any arena VkDeviceMemory accesses globally.)

If you feel like this is unnecessary, I can revert. Eager to hear back.

[MarkCallow]

If you feel like this is unnecessary, I can revert. Eager to hear back.

It is great to have a test even if it is not a stress test.

I would love to have non-interactive tests of the uploaders, maybe using gtest like texturetests, but I have no idea how to run such tests on GHA CI runners. Do they headless Vulkan or OpenGL graphics?

I will properly review this PR early next week. Please be aware that I will not merge this until v5.0.0 has been released. I can't give a date for that at present.

[toomuchvoltage]

Hi @MarkCallow , just circling back on this. It's perfectly fine if this goes out post-5.0.0. Truth is these are on-the-field improvements resulting from a commercial game on Steam shipped with LibKTX2. I'm hesitant to link it since I personally wouldn't feel comfortable with the self promotion here, but of course figuring out the title is trivial given my handle. And I personally do not have experience with GHA CI, but I suspect paid plans (which this should be?) should have no issues with GPU'd instances.

[MarkCallow]

Is it necessary to use guarded memory allocation callbacks when using the queue guards

As I am no expert in this, I would like to find an expert to review it. From my side it looks fine except for a couple of minor comment issues.

[toomuchvoltage]

Hi @MarkCallow , appreciate the feedback. The guarded memory callbacks are absolutely necessary. That said, I may have done a more heavy handed version than is necessary with VMA since I used my own pattern from my engine (which obviously needs to be more explicit): https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L313-L474

VMA's allocation, image/buffer bind and free calls are all thread-safe. However, mapping and unmapping calls are not. They only check to ensure that no VkDeviceMemory arena is mapped twice. But no guarantees if you try to map the same one from separate threads (to the best of my understanding). The memory guard is also necessary when accessing AllocMemCWrapperDirectory, irrespective of whether VMA is used or not. I will be pushing an updated unit test addressing these.

For an expert pair of eyes, I would solicit Adam Sawicki's advice. He is the original author of VMA. His handle is @sawickiap on GitHub. I'm confident he's within reach for Khronos.

[toomuchvoltage]

All done @MarkCallow , ready for another pair of eyes.

[MarkCallow]

All done @MarkCallow , ready for another pair of eyes.

Thanks. Working on finding a reviewer.

[MarkCallow]

One of my Khronos colleagues asked codex to analyze this. This is what it said.

---

• [P1] Queue locking does not make a shared ktxVulkanDeviceInfo thread-safe. The mutex is acquired only around vkQueueSubmit (

  KTX-Software/lib/src/vkloader.c

  Lines 1317 to 1320 in 339c047

  	if (useQueueMutex) queueMutexCallbacks->queueLockFuncPtr();
  	VK_CHECK_RESULT(
  	vdi->vkFuncs.vkQueueSubmit(vdi->queue, 1, &submitInfo, copyFence));
  	if (useQueueMutex) queueMutexCallbacks->queueUnlockFuncPtr();

  ), but every upload begins and records into the single vdi->cmdBuffer (

  KTX-Software/lib/src/vkloader.c

  Lines 1027 to 1030 in 339c047

  	VK_CHECK_RESULT(
  	vdi->vkFuncs.vkBeginCommandBuffer(vdi->cmdBuffer, &cmdBufBeginInfo)
  	);

  ). Concurrent calls sharing a vdi therefore concurrently access the same Vulkan command buffer, violating Vulkan’s external-synchronization requirements. Either use per-call/per-thread command buffers and command pools, or explicitly require each concurrent caller to supply a thread-confined vdi and command pool. Until then, the documentation’s “fully thread-safe (https://github.com/KhronosGroup/KTX-Software/blob/339c04731e38806a288caf7ca4820accdfd08af6/lib/include/ktxvulkan.h#L247-L251)” claim is unsafe.
  • [P2] Queue callbacks need state and preferably the queue handle. The callbacks take no arguments (

    KTX-Software/lib/include/ktxvulkan.h

    Lines 137 to 138 in 339c047

    	typedef void (*ktxVulkanTexture_queueLockFuncPtr)();
    	typedef void (*ktxVulkanTexture_queueUnlockFuncPtr)();

    ), so a C application cannot associate a mutex with a particular VkQueue except through global state. A global mutex is correct but unnecessarily serializes independent queues, undermining the optimization this API introduces. I’d add void* userData to the callback struct and invoke callbacks with that and/or vdi->queue.
  • [P2] The “guarded” sample allocator contains a data race. mt64() is called before acquiring memoryAccessGuard (

    KTX-Software/tests/loadtests/vkloadtests/VulkanLoadTestSample.cpp

    Lines 136 to 164 in 339c047

    	std::mutex memoryAccessGuard;
    	uint64_t AllocMemCWrapperGuarded(VkMemoryAllocateInfo* allocInfo, VkMemoryRequirements* memReq, uint64_t* numPages)
    	{
    	uint64_t allocId = mt64();
    	VmaAllocationCreateInfo pCreateInfo = {};
    	if ((cachedDevMemProps.memoryTypes[allocInfo->memoryTypeIndex].propertyFlags & VK_MEMORY_PROPERTY_HOST_VISIBLE_BIT) ||
    	(cachedDevMemProps.memoryTypes[allocInfo->memoryTypeIndex].propertyFlags & VK_MEMORY_PROPERTY_HOST_COHERENT_BIT))
    	{
    	pCreateInfo.usage = VMA_MEMORY_USAGE_CPU_TO_GPU;
    	pCreateInfo.flags = VMA_ALLOCATION_CREATE_HOST_ACCESS_SEQUENTIAL_WRITE_BIT;
    	}
    	else
    	{
    	pCreateInfo.usage = VMA_MEMORY_USAGE_GPU_ONLY;
    	}
    	pCreateInfo.memoryTypeBits = memReq->memoryTypeBits;
    	VmaAllocation allocation;
    	VkResult result = vmaAllocateMemory(vmaAllocator, memReq, &pCreateInfo, &allocation, VMA_NULL);
    	if (result != VK_SUCCESS)
    	{
    	return 0ull;
    	}
    	{
    	std::lock_guard<std::mutex> lk(memoryAccessGuard);
    	AllocMemCWrapperDirectory[allocId].allocation = allocation;
    	AllocMemCWrapperDirectory[allocId].mapSize = memReq->size;
    	}

    ). std::mt19937_64 mutates internal state, so concurrent guarded allocations have undefined behavior and can produce corrupt or duplicate IDs. Generate the ID under the mutex, or preferably use a nonzero atomic counter.
  • [P2] The new load-error handling still performs incomplete cleanup. On ktxTexture_LoadImageData failure (

    KTX-Software/lib/src/vkloader.c

    Lines 1170 to 1180 in 339c047

    	kResult = ktxTexture_LoadImageData(This,
    	pMappedStagingBuffer,
    	(ktx_size_t)memAllocInfo.allocationSize);
    	if (kResult != KTX_SUCCESS)
    	{
    	if (!useSuballocator)
    	vdi->vkFuncs.vkUnmapMemory(vdi->device, stagingMemory);
    	else
    	subAllocatorCallbacks->memoryUnmapFuncPtr(stagingAllocId, 0ull);
    	return kResult;
    	}

    ), the code unmaps and returns but leaks copyRegions, the staging buffer, and its allocation; it also leaves the command buffer recording. This needs a common cleanup path.

  Coverage note: the new “[threaded]” sample creates one worker and immediately joins it, so no accesses ever overlap (

  KTX-Software/tests/loadtests/vkloadtests/Texture.cpp

  Lines 128 to 140 in 339c047

  	if (useQueueGuard == UseQueueGuard::Yes)
  	{
  	std::thread uploaderThread([&]() {
  	ktxresult = ktxTexture_VkUploadEx_WithSuballocatorAndQueueGuard(kTexture, &vdi, &texture,
  	static_cast<VkImageTiling>(tiling),
  	VK_IMAGE_USAGE_SAMPLED_BIT,
  	VK_IMAGE_LAYOUT_SHADER_READ_ONLY_OPTIMAL,
  	&subAllocatorCallbacksGuarded, &queueGuardCallbacks);
  	});
  	uploaderThread.join(); // We really have no choice here. This crucible/test-environment only loads one texture at a time.
  	// Additionally `QUEUE_GUARD_CALLBACKS::queueAccessGuard` and `QUEUE_GUARD_CALLBACKS::memoryAccessGuard` would
  	// need to be re-used when arena `VkDeviceMemory`s or the same `VkQueue`s are being accessed simultaneously.
  	}

  ). It cannot detect the command-buffer or RNG races above. A deterministic two-caller test—using separate thread-confined VDIs if that is the intended contract—would be much more valuable.

---

@toomuchvoltage you have already pointed out the last item. It sounds like we need to look at the vdi queue handling. What do you think?

[sawickiap]

Hi, I'm the developer of the VMA library. I'm sorry for the delayed response.

Mapping in VMA is thread-safe.

About raw Vulkan (functions vkMapMemory, vkUnmapMemory), you are right:

• Mapping a single VkDeviceMemory block multiple times (whether the same or disjoint memory regions) is illegal.
• It is not thread-safe.

However, using the recommended library functions vmaMapMemory, vmaUnmapMemory (or other convenient ways like VMA_ALLOCATION_CREATE_MAPPED_BIT flag or vmaCopyMemoryToAllocation function):

• They are thread-safe, synchronized internally (unless you specified VMA_ALLOCATOR_CREATE_EXTERNALLY_SYNCHRONIZED_BIT).
• You can map the same allocation or different allocations coming from the same VkDeviceMemory block multiple times (mapping is reference-counted internally).
• You can safely map different allocations coming from the same VkDeviceMemory block because VMA internally maps the entire memory block, not just the region of that one allocation.

For more information, see this documentation chapter:
https://gpuopen-librariesandsdks.github.io/VulkanMemoryAllocator/html/memory_mapping.html

[MarkCallow]

Thanks @sawickiap. @toomuchvoltage do you have any comments on this or the codex review?

[toomuchvoltage]

Hi @MarkCallow @sawickiap , deeply appreciate the reviews. Fantastic to be informed about vmaMapMemory and vmaUnmapMemory being thread-safe.

On the P1 issue: once again, since I was applying the learning from my own engine, I simply brought the assumptions as well for general use. The point is absolutely correct and my engine has a thread-safe cache for per-thread KTX VDIs.

Thread-safe objects:
https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L52-L53

Creation:
https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L4510-L4519

Destruction:
https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L4166-L4172

I guess this becoming the general usage pattern was implicit in my assumptions. We can ask for assurances on this in the documentation.

On the first P2 issue: my engine effectively has 1 copy queue, 1 transfer queue and 1 graphics queue. Is it safe to assume that this is a globally recommended pattern? Is it safe or performant for other engines to have multiple transfer queues for example? If so, we can simply pass a typed VkQueue to the function where there's no ambiguity about what is we want guarded.

On the second P2 issue: Very fine point regarding mt64(). I'll address that.

On the third P2 issue: Good point, I guess this really falls under an overall discovery in this process that can be addressed in this PR.

I'll get another commit together to address these.

[toomuchvoltage]

Hi @MarkCallow , I just pushed a commit addressing the raised issues. Note that I finally decided to pass a VkQueue handle to the queue guard callbacks. This makes their roles much more explicit. Additionally, from what I gather online not all IHVs support multiple queues per family. And in practice it may not yield a performance benefit in all cases. (Knowing this is naturally compatible with the design we have.) It would be interesting to verify this with Khronos's perhaps broader overview of what is actually available in the wild.

[MarkCallow]

Perhaps https://vulkan.gpuinfo.org can answer your questions about support for multiple queues per family.

In one of your commit messages you write that a per-thread VDI is necessary. If that is the case does it no make sense to pass the device from that to the allocator callbacks, similar to what was requested in issue #1212? Doing so would require changing the signatures of the allocator callback functions. I wonder if there is any way to do that without having to create another set of *_VkUploadEx_WithSubAllocator functions.

[toomuchvoltage]

In one of your commit messages you write that a per-thread VDI is necessary. If that is the case does it no make sense to pass the device from that to the allocator callbacks, similar to what was requested in issue #1212? Doing so would require changing the signatures of the allocator callback functions. I wonder if there is any way to do that without having to create another set of *_VkUploadEx_WithSubAllocator functions.

Hi @MarkCallow, no that is not necessary. ktxTexture_VkUploadEx(), ktxTexture_VkUploadEx_WithSuballocator() and ktxTexture_VkUploadEx_WithSuballocatorAndQueueGuard() all readily take in a KTX VDI (second parameter). In fact the quoted snippets demonstrate how a per thread KTX VDI is provided in a thread-safe manner:

Thread-safe objects: https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L52-L53

Creation: https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L4510-L4519

Destruction: https://github.com/toomuchvoltage/HighOmega-public/blob/sauray_vkquake2/HighOmega/src/gl.cpp#L4166-L4172

Each ktx2VDIPools.dir[ThreadID].elem (with ThreadID being thread_local and generated with mt64()) is a per-thread KTX VDI supplying its own command pool and command buffer to UploadEx(). In fact these elements are reference counted via ktx2VDIPools.dir[ThreadID].elemCount perhaps much in the same manner and spirit that VMA handles maps/unmaps internally. Once elemCount goes to zero, the KTX VDI is deemed unnecessary and destroyed (i.e. all last images created with this VDI are gone). These operations are synchronized via ktx2VDIPools.mtx. My own status quo usage issues with mt64() not-withstanding, this is effectively sufficient for our UploadEx() set of calls to complete their tasks efficiently and in a thread-safe manner. This solution is currently shipped and tested in the video game I screenshotted in the original post (coupled with performance metrics and the environment specs). It shares the same core engine with the repo posted above.

Ultimately, what is being proposed in #1212 breaks separation of concerns. A type erased void * being passed in would leave the actual implementation guessing as to what it even means in the first place. I obviously welcome the original author of 1212 to fork and create their own interface and implementation. However, I would be very hesitant to modify the standard version advertised globally taking in nebulous type-erased parameters with no well defined purpose.

I will proceed to prepare a commit for exhaustive clean-ups in all failure cases of UploadEx() shortly.

[toomuchvoltage]

All done @MarkCallow . A quick review of the specification for freeUploadResources() would be appreciated to see if it is in line with other specifications. Note that it is used to clean-up mappableMemory and the texture's main allocationId rather than any staging resources in the linear tiling case (as staging resources do not seem to apply there).

EDIT: Force pushed to get the checks running again. There was a test infrastructure failure and I wanted to make sure I'm not introducing new issues with this commit.