Skip to content
New issue

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

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

Already on GitHub? # to your account

Memory (sub)allocation API 2.0 #2316

Merged
merged 9 commits into from
Sep 3, 2023
Merged

Conversation

marc0246
Copy link
Contributor

@marc0246 marc0246 commented Aug 30, 2023

This fixes most of the horrible design decisions from #1997. After I fix the ones left over, I think there will be no more reason for breaking changes in it pretty much ever. Though I've been wrong before, so I'd appreciate feedback, if there's a way it can be made more generic.

The (many) problems

A relatively highly-requested feature, which is quite obvious when you think about it, is being able to suballocate existing buffers (as opposed to SubbufferAllocator which allocates its own buffer(s)). This is currently not possible because MemoryAlloc is tied to (sub)allocations of DeviceMemory: it holds a reference to the suballocator it came from and the suballocators in turn only accept a MemoryAlloc as a region to suballocate (DeviceMemory being the root of all such hierarchies).

It would have been possible to remedy this while keeping the reference to the suballocator in its returned suballocation, however there are other problems with this:

  • It ties the Suballocator trait to be implemented on Arcs (so that the returned suballocation can hold this reference):
    • Due to the orphan rules, you can't implement a foreign trait on a foreign type (Arc), so if the user wanted to implement an allocator trait it would require double-boxing of the allocator that's put in the allocation, on top of dynamic dispatch. This is less so a consideration for suballocators since there's only so many algorithms and no reason for the user to implement one, but it's a consideration for allocators in general.
    • I forgot the rest of this. There's many more issues here trust me.
  • It forces the suballocators to be Sync (they still are at this point, read "most of the horrible design decisions").
  • It forces the suballocators to be 'static.
  • The allocation holding a reference to the suballocator rather than the MemoryAllocator, excludes many possibilities:
    • Reusing allocations that aren't suballocated. Currently they just die in agony each time.
    • Any other general bookkeeping, such as updating statistics for the whole allocator etc.

I guess this and some other reasons that I forgot is why allocation APIs have an explicit deallocate method which fixes all of that. One only needs to implement the trait for their type, and the library can blanket impl it for all references and other pointer types and standard library types. Whether the allocator is internally or externall synchronized is also up to the implementor, because the allocations are separate from the allocator. etc. I don't know why none of our allocator traits have this method, but I can only imagine the original reasoning was that this method is unsafe. Alas, I highly doubt this is a good enough justification.

Solutions

So now Suballocator has an unsafe deallocate method, and can be used to suballocate absolutely anything: a DeviceMemory block, or suballocations thereof. These can then be bound to buffers which can also be suballocated into subbuffers, or you can suballocate subbuffers. If you really wanted to you could even suballocate regions of descriptors in descriptor arrays, or whatever else you might encounter in Vulkan, such as an array of u32s inside a buffer.

MemoryAllocator also has an unsafe deallocate method, to allow MemoryAllocs to be freed directly to it rather than to a suballocator as described above. The issue is that the specific block the MemoryAlloc belongs to must be somehow known. The solution, while unsafe, is much more elegant than the boxing and dynamic dispatch that would be the alternative IMO: an opaque AllocationHandle that can represent any piece of data used to identify the allocation, most likely a pointer or index. Suballocation also has such a handle to identify the suballocation within the suballocator. These handles can also safely be Send + Sync regardless of how the (sub)allocator is synchronized, because again, (sub)allocators and their (sub)allocations are separate from each other. So the (sub)allocator can be synchronized any which way.

PoolAllocator

Is bye bye. No one liked it anyway! The only thing it gave us is special-casing everywhere because it has an upper bound on the suballocation size unlike everything else, and then there's the whole headache of how to specify said block size. Or how GenericMemoryAllocatorCreateInfo::allocation_type existed solely because of it. That's not to say pool allocation isn't useful, it absolutely is, just that this being a suballocator was frankly never meant to be. The user can create a pool themself with ease, such as can be seen in the implementation of GenericMemoryAllocator.

Changelog:

### Breaking changes
Changes to memory allocation:
- The memory (sub)allocation API has been completely reworked.
  - `Buffer` and `SubbufferAllocator` now take an `Arc<dyn MemoryAllocator>` on construction.
  - `Suballocator` and `MemoryAllocator` now have explicit `deallocate` methods in order to fix all the flexibility issues.
  - `Suballocator` is completely generic now in regards to the type of suballocation.
  - `SuballocationCreateInfo` was removed.
  - `MemoryAllocatePreference` and `AllocationType` are no longer marked `#[non_exhaustive]`.
  - `MemoryAlloc` was replaced by `ResourceMemory`, `MemoryAlloc` now only represents specifically allocations made by `MemoryAllocator`.
  - `PoolAllocator` was removed.
  - `GenericMemoryAllocatorCreateInfo::allocation_type` was removed.

@marc0246 marc0246 marked this pull request as ready for review September 2, 2023 14:32
@Rua Rua merged commit 5578bf3 into vulkano-rs:master Sep 3, 2023
Rua added a commit that referenced this pull request Sep 3, 2023
@marc0246 marc0246 deleted the suballocation branch September 3, 2023 12:26
marc0246 added a commit to marc0246/vulkano that referenced this pull request Sep 7, 2023
Rua pushed a commit that referenced this pull request Sep 9, 2023
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
* Excommunicate `PoolAllocator`

* Switch to manual deallocation

* Move `ResourceMemory` away from the `suballocator` module

* Remove `SuballocatorError::BlockSizeExceeded`

* Fix examples

* Fix atom size

* Address safety TODOs

* Nice English you got there bro
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
hakolao pushed a commit to hakolao/vulkano that referenced this pull request Feb 20, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants