Skip to content
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

Bindless Descriptor fixes and optimizations #2515

Merged
merged 5 commits into from
Jul 2, 2024

Conversation

Firestar99
Copy link
Contributor

@Firestar99 Firestar99 commented Apr 9, 2024

  • added DescriptorSet::invalidate() to make vulkano forget about resources that bound to a descriptor_set, so they can be freed. As (normal) descriptors are validated when bound, this is entirely safe to do.
  • disable validation for individual descriptors if the descriptor set is UPDATE_AFTER_BIND or PARTIALLY_BOUND, making them actually usable
    • I'm not sure these changes are entirely safe. For it to be, we'd need to validate the descriptor set is properly populated just before flushing any commands to the GPU. And I'm not sure if that's done already, or where it should be added. We could ofc also decide to just leave these two flags as unsafe, somehow.
  • some small Descriptor optimizations:
    • More SmallVec use in DescriptorPool to improve variable descriptor count allocation. It only ever allocates one descriptor set at a time, i.e. allocate_infos only ever has one entry in that code path. Annoyed me and just had to fix :D
    • Descriptor updates with no writes or copies return early instead of calling into vulkan. Particularly useful when creating a new descriptor, writing nothing, and updating it properly later.
  1. Update documentation to reflect any user-facing changes - in this repository.

  2. Make sure that the changes are covered by unit-tests.

  3. Run cargo clippy on the changes.

  4. Run cargo +nightly fmt on the changes.

  5. Please put changelog entries in the description of this Pull Request
    if knowledge of this change could be valuable to users. No need to put the
    entries to the changelog directly, they will be transferred to the changelog
    file by maintainers right after the Pull Request merge.

    Please remove any items from the template below that are not applicable.

  6. Describe in common words what is the purpose of this change, related
    Github Issues, and highlight important implementation aspects.

Changelog:

### Additions
* added `DescriptorSet::invalidate()` to make vulkano forget about resources that bound to a descriptor_set, so they can be freed

### Bugs fixed
* fixed descriptor sets with `UPDATE_AFTER_BIND` or `PARTIALLY_BOUND` being wrongly validated on bind

@Firestar99 Firestar99 changed the title more SmallVec use in DescriptorPool small Descriptor optimizations Apr 10, 2024
@Firestar99 Firestar99 marked this pull request as draft April 14, 2024 14:16
@Firestar99
Copy link
Contributor Author

Firestar99 commented Apr 14, 2024

I've turned this PR into a draft, as I've added DescriptorSet::invalidate() to it and haven't gotten around to extensively test it tested it and it works :D

@Rua Rua linked an issue Apr 14, 2024 that may be closed by this pull request
@Firestar99 Firestar99 changed the title small Descriptor optimizations Bindless Descriptor fixes and optimizations May 21, 2024
@Firestar99 Firestar99 marked this pull request as ready for review May 21, 2024 15:48
@marc0246
Copy link
Contributor

marc0246 commented Jun 20, 2024

I think you already know this, but to anyone reading this, I would like to murder DescriptorSet before the next release. There's no point having all these collections in so many places that just keep references to Vulkan objects, cloning and dropping Arcs and allocating and deallocating the Vecs / HashMaps / whatever for them each and every frame multiple times, when there is one central resources collection that tracks everything in one place. I'm willing to merge this regardless if for nothing else but to not inconvenience you, but I wouldn't start designing something new around this invalidation API.

@marc0246
Copy link
Contributor

marc0246 commented Jun 20, 2024

Regarding PARTIALLY_BOUND, I think we decided that all shader executions are unsafe, and it is documented that the shader must not access descriptors that haven't been bound. However, the logic here is not correct. It's only PARTIALLY_BOUND that permits this, and not UPDATE_AFTER_BIND.

@Rua
Copy link
Contributor

Rua commented Jun 30, 2024

If UPDATE_AFTER_BIND is enabled, then it's possible that the resources in the descriptor set, at the time the command is recorded, are not the same as those that will be bound when the command buffer is actually executed. So any record-time checks on the bound resources are probably superfluous?

@Firestar99
Copy link
Contributor Author

Firestar99 commented Jul 1, 2024

So any record-time checks on the bound resources are probably superfluous?

Yes they are, so for now I've just disabled them. Which means they are indeed not validated at all, and honestly I'd keep it that way as I don't think modding that into the old sync has any benefit. Rather invest that time into the new sync. We may want to make these flags unsafe, but wouldn't know how.

@Rua Rua merged commit 7841628 into vulkano-rs:master Jul 2, 2024
4 of 5 checks passed
Rua added a commit that referenced this pull request Jul 2, 2024
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.

Allow releasing resources from descriptor sets
3 participants