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