Support dynamically allocating descriptors when pool is exhausted.

[billhollings]

• MVKDescriptorTypePool if the pool is exhausted, report a warning, allocate a new descriptor instance, and when it is freed, destroy it.
• Deprecate MVKConfiguration::preallocateDescriptors and MVK_CONFIG_PREALLOCATE_DESCRIPTORS, as both preallocation and dynamic allocation of descriptors are now supported.
• vkAllocateDescriptorSets(): Per Vulkan spec, if any descriptor set allocation fails, free any successful allocations, and populate all descriptor set pointers with VK_NULL_HANDLE. In addition, return VK_ERROR_FRAGMENTED_POOL if failure was due to Metal argument buffer fragmentation.
• vkUpdateDescriptorSets() accepts null VkDescriptorSets as no-ops.
• MVKDescriptorPool::getLogDescription() include count of remaining descriptors in the pool, for diagnostic logging during debugging.

Fix #2282.

[billhollings]

@cdavis5e @danginsburg

The Vulkan spec says:

If a call to vkAllocateDescriptorSets would cause the total number of descriptor sets allocated from the pool to exceed the value of VkDescriptorPoolCreateInfo::maxSets used to create pAllocateInfo->descriptorPool, then the allocation may fail due to lack of space in the descriptor pool. Similarly, the allocation may fail due to lack of space if the call to vkAllocateDescriptorSets would cause the number of any given descriptor type to exceed the sum of all the descriptorCount members of each element of VkDescriptorPoolCreateInfo::pPoolSizes with a type equal to that type.

So it's up to the implementation to define how strict it's going to be in enforcing these rules.

Previously, MoltenVK chose to fail and report in both of these situations. But based on the issue that triggered this PR (#2282), other implementations are more lax. This PR relaxes the second directive, dynamically allocating additional descriptors of a type once that type has been exhausted in the pool.

Design questions for you:

1. Doing so means the app does not get feedback that their VkDescriptorPoolCreateInfo numbers are wrong, if the app intends to use this info to allocate an additional VkDescriptorPool once the current one has exhausted a type. Does this matter? Do apps follow this approach, and will suffer if we blindly dynamically allocate descriptors beyond the pool limit.

2. This PR does not relax the limit on VkDescriptorPoolCreateInfo::maxSets, and still reports failure when that limit is exhausted, because it wasn't needed for the issue that triggered this PR, and because it would mean dynamically allocating a Metal argument buffer for each descriptor set requested above the limit. However, we could easily add that capability and eliminate that restriction as well (basically never fail on a descriptor set allocation), if it makes sense

More discussion in the triggering issue.

Thoughts?

[danginsburg]

Thoughts?

A couple of thoughts:

• I've definitely seen this behave differently on different implementations, for example in SDL3 I forgot to include a descriptor type at all in the pool and it was working with no validation errors on NVIDIA and AMD, but then failed on Android (Vulkan Renderer on Android libsdl-org/SDL#10208 (comment)). I considered that an application bug and fixed it.
• The behavior of vkAllocateDescriptorSets changed in VK_KHR_maintenance1, does the app opt-in to that? You might need to check whether that is enabled before returning VK_ERROR_OUT_OF_POOL_MEMORY? I think apps written against Vulkan 1.0 without maintenance1 wouldn't expect that error.

1. Doing so means the app does not get feedback that their VkDescriptorPoolCreateInfo numbers are wrong, if the app intends to use this info to allocate an additional VkDescriptorPool once the current one has exhausted a type. Does this matter? Do apps follow this approach, and will suffer if we blindly dynamically allocate descriptors beyond the pool limit.

That's definitely what we do in our engine, yes. I guess it depends on what the cost/trade-off is going to be having MoltenVK dynamically allocate versus if the app had created its own new descriptor pool.

2. This PR does not relax the limit on VkDescriptorPoolCreateInfo::maxSets, and still reports failure when that limit is exhausted, because it wasn't needed for the issue that triggered this PR, and because it would mean dynamically allocating a Metal argument buffer for each descriptor set requested above the limit. However, we could easily add that capability and eliminate that restriction as well (basically never fail on a descriptor set allocation), if it makes sense

That seems like the right call.

[billhollings]

@danginsburg

Thanks for all the background info.

• The behavior of vkAllocateDescriptorSets changed in VK_KHR_maintenance1, does the app opt-in to that? You might need to check whether that is enabled before returning VK_ERROR_OUT_OF_POOL_MEMORY? I think apps written against Vulkan 1.0 without maintenance1 wouldn't expect that error.

Thanks for pointing that out. The app (vkQuake) is not opting in on VK_KHR_maintenance1, but it is requesting Vulkan 1.1, so MoltenVK's error response was accurate. And even with 1.0 itself, the spec says any error returned should be treated as a failure.

2. This PR does not relax the limit on VkDescriptorPoolCreateInfo::maxSets, and still reports failure when that limit is exhausted, because it wasn't needed for the issue that triggered this PR, and because it would mean dynamically allocating a Metal argument buffer for each descriptor set requested above the limit. However, we could easily add that capability and eliminate that restriction as well (basically never fail on a descriptor set allocation), if it makes sense

That seems like the right call.

Can you clarify which of the two options I suggested you think is the right call?

[danginsburg]

Can you clarify which of the two options I suggested you think is the right call?

Reporting a failure and not allocating a Metal argument buffer.

[cdavis5e]

• Doing so means the app does not get feedback that their VkDescriptorPoolCreateInfo numbers are wrong, if the app intends to use this info to allocate an additional VkDescriptorPool once the current one has exhausted a type. Does this matter? Do apps follow this approach, and will suffer if we blindly dynamically allocate descriptors beyond the pool limit.

It's easy to dismiss vkQuake's behavior as being outside the spec, because it requested 1.1 behavior but failed to handle the out-of-memory condition. I suppose it's even easy enough to fix in vkQuake, given that John Carmack (thankfully) open-sourced his game engines. However, now I'm worried that there are other existing apps out there that can't be so easily updated.

Also, I'm not sure how valid this concern is, given how argument buffers work: memory is memory; there's no separate memory for texture references vs. buffer pointers. I think the approach here is the right call.

• This PR does not relax the limit on VkDescriptorPoolCreateInfo::maxSets, and still reports failure when that limit is exhausted, because it wasn't needed for the issue that triggered this PR, and because it would mean dynamically allocating a Metal argument buffer for each descriptor set requested above the limit. However, we could easily add that capability and eliminate that restriction as well (basically never fail on a descriptor set allocation), if it makes sense

I agree with @danginsburg here. I'd add that supporting this would come at the cost of additional complexity in the descriptor set code--that code is hairy enough as it is.

[billhollings]

@cdavis5e

Thanks for the additional advice and feedback.

If you have the cycles, can you also review the PR content, please?

[cdavis5e]

If you have the cycles, can you also review the PR content, please?

Done. Sorry about the delay.