Skip to content

Revert to fixed number of argument buffer binding reservations. #2463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 8, 2025

Conversation

squidbus
Copy link
Collaborator

@squidbus squidbus commented Mar 5, 2025

Partially reverts #2417, restoring fixed argument buffer reservation count but only when argument buffers are used by any descriptor set.

I've found that this seems to break the "compatible for set N" rule when push constants are used and the application relies on not having to re-push them after changing pipelines. I got in a situation where, when a pipeline with N + 1 descriptor sets is followed later by pipelines with N descriptor sets, something with the way buffer bindings dirty state works for layout compatibility may end up re-binding that N + 1 argument buffer into the push constants slot that is now at N + 1. Then, since the push constants are not re-pushed, they are not updated by MoltenVK back into that slot. I think that reserving no slots in the case of no argument buffers should be fine, since redefining descriptor sets as push descriptors for example would break the compatibility rule and re-bind properly.

Maybe this could be investigated deeper and some changes figured out to allow optimizing the binding usage this way, but I wasn't able to figure something out that I was satisfied with. Most of what I was thinking of was either kind of intrusive with the push constant checks or would add extra push constant rebinds that could impact performance. So at least for now, I've opted to just revert this part of the change.

Copy link
Contributor

@billhollings billhollings left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this partial reversion mean your original index exhaustion that #2417 fixed will return?

After this reversion, you'll now only be saving indexes if none of the pipeline layouts are using argument buffers.

@squidbus
Copy link
Collaborator Author

squidbus commented Mar 8, 2025

For my case it is fine as I was using only push descriptors or only discrete descriptor sets.

@billhollings billhollings merged commit f743439 into KhronosGroup:main Mar 8, 2025
6 checks passed
@squidbus squidbus deleted the fix-arg-buf branch May 1, 2025 18:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants