Fix OOM in Vulkan FetchShaderFeedback for descriptor binding's which use VariableDescriptorCount feature #3105
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
baldurk
requested changes
Nov 1, 2023
| VK_DESCRIPTOR_BINDING_PARTIALLY_BOUND_BIT_EXT, | ||
| }; | ||
| VK_DESCRIPTOR_BINDING_VARIABLE_DESCRIPTOR_COUNT_BIT_EXT}; |
There was a problem hiding this comment.
Not required but if you add a , here after the last entry it formats the array a bit better with the closing brace on a new line.
| @@ -64,6 +64,7 @@ def check_capture(self): | |||
| 0: { 'dynamicallyUsedCount': 1, 'used': [15] }, | |||
| 1: { 'dynamicallyUsedCount': 6, 'used': [4, 19, 20, 21, 49, 59] }, | |||
| 2: { 'dynamicallyUsedCount': 2, 'used': [381, 386] }, | |||
| 3: { 'dynamicallyUsedCount': 0, 'used': [] }, | |||
There was a problem hiding this comment.
I think for this test to be robust we should ensure that one of the samplers in this descriptor set less than the count is actually used. Otherwise in future this test may not cover what we want it to e.g. if we statically determine that the shader doesn't use the descriptor set at all so we skip processing of it.
New Binding 3 Initial size is UINT32_MAX Updated to be sized DESC_ARRAY3_SIZE Used in the shader to ensure the binding is referenced Stress tests the FetchShaderFeedback code when it computes the maximum size of feedbackStorageSize.
For variable sized descriptor sets compute feedbackStorageSize using the active descriptor set layout instead of the creation time data.
baldurk
approved these changes
Nov 1, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Previously the
descriptorCountwas being computed from the create parameters. This did not account for descriptor bindings which used the variable size feature and could lead tofeedbackStorageSizebeing estimated to be too large and triggering OOM.Added a test which has a descriptor binding set to UINT32_MAX at create time and then uses a variable count of 10,000.
The test fails before the code fix and the test succeed after the code fix.