Inconsistent interface & unnecessary forced memory allocations in handcrafted methods.

One thing I noticed messing with Ash is some of the handcrafted method's return a Vec, but some methods don't. This seems weird, unnecessary and kinda against the spirit of matching the C++ code with no major compromises in release build.

Ex: Forces Vec

    /// <https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/vkAllocateDescriptorSets.html>
    #[inline]
    pub unsafe fn allocate_descriptor_sets(
        &self,
        allocate_info: &vk::DescriptorSetAllocateInfo<'_>,
    ) -> VkResult<Vec<vk::DescriptorSet>> {
        let mut desc_set = Vec::with_capacity(allocate_info.descriptor_set_count as usize);
        (self.device_fn_1_0.allocate_descriptor_sets)(
            self.handle(),
            allocate_info,
            desc_set.as_mut_ptr(),
        )
        .set_vec_len_on_success(desc_set, allocate_info.descriptor_set_count as usize)
    }

Ex: Does not return Vec

    /// <https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/vkGetImageSparseMemoryRequirements2.html>
    ///
    /// Call [`get_image_sparse_memory_requirements2_len()`][Self::get_image_sparse_memory_requirements2_len()] to query the number of elements to pass to `out`.
    /// Be sure to [`Default::default()`]-initialize these elements and optionally set their `p_next` pointer.
    #[inline]
    pub unsafe fn get_image_sparse_memory_requirements2(
        &self,
        info: &vk::ImageSparseMemoryRequirementsInfo2<'_>,
        out: &mut [vk::SparseImageMemoryRequirements2<'_>],
    ) {
        let mut count = out.len() as u32;
        (self.device_fn_1_1.get_image_sparse_memory_requirements2)(
            self.handle(),
            info,
            &mut count,
            out.as_mut_ptr(),
        );
        assert_eq!(count as usize, out.len());
    }

Imo, the library should move all methods to match the C++ version so the end user can determine how they want to allocate the memory or potentially reuse existing memory. Rust already has slice syntax which keeps this ergonomic.

Ex: Update descriptor sets to not use Vec

    /// <https://www.khronos.org/registry/vulkan/specs/1.3-extensions/man/html/vkAllocateDescriptorSets.html>
    #[inline]
    pub unsafe fn allocate_descriptor_sets(
        &self,
        allocate_info: &vk::DescriptorSetAllocateInfo<'_>,
        descriptor_sets: &mut[vk::DescriptorSet]
    ) -> VkResult<()> {
        assert_eq!(allocate_info.descriptor_set_count,descriptor_sets.len() as u32);
        (self.device_fn_1_0.allocate_descriptor_sets)(
            self.handle(),
            allocate_info,
            descriptor_sets.as_mut_ptr()
        ).result()
    }

Made a fork with PR if there is any interest here. There were a couple vecs still used for loading data which probably should be removed also but they weren't methods I know a lot about in Vulkan so I just changed the heavily used ones like DescriptorSets, CommandBuffers, etc. https://github.com/ash-rs/ash/pull/883

This is done because VkSparseImageMemoryRequirements2 contains a pNext pointer that the caller can initialize to request more information in this get_image_sparse_memory_requirements2() call. That is not the case with allocate_descriptor_sets().

For next time, it's easier to first create an issue and discuss, before creating a PR with the same load (in that case, it's fine to discuss in the same PR without creating a duplicate).

Gotcha, just showed the PR to give an idea of scope & to visualize the ergonomics changes. Closing it is no issue. There are only ~7 methods that have this opinionated allocation approach so refactoring them isn't a whole lot of work, main issue I imagine is it's a breaking interface change.

Agument I have against pNext it's ok for us to let the user decide and when not pNext we know enough to decide, kinda misses the point that knowing the size isn't the issue. It's opinionating the memory allocation. Most projects that are above a toy level are likely to have managing structs or pools they are wanting to add to. With the convenient slice semantics in Rust it makes a lot more sense to let the end user decide how they wish to allocate that memory imo since in a large percentage of cases they likely can avoid any secondary allocation altogether.

We can always reopen if we decide this is the way forward, but I have a lot more context to add when physically using a keyboard.

Have you looked into the many methods calling read_into_xxx_vector()? Or are you intentionally skipping functions that have a 2-stage call (get the size first, then allocate the vec, then get the context in a second call)?

Note that my answer above isn't jumping into the topic of "who gets to allocate return memory" just yet. It is merely answering the apparent confusion of why some methods are "inconsistently" returning Vec versus taking a mutable slice:

A Vec is returned when the underlying Vulkan exclusively returns data.
A mutable input slice is used when the underlying Vulkan function is reading that data and enacting on it, in most if not all cases reading and initializing extension structs in a pNext chain (and it goes without saying that all sType fields are required to be set to the matching constant as well).

With that cleared up, I think we can discuss whether we want the current Vec- returning functions to take a mutable slice and update it instead?

Few points of context of the top of my head:

accepting slices containing MaybeUninit for true write-only getters (anything with sType/pNext is ruled out);
returning resized mutable slices in case the vulkan function fills less items than the caller requests (and removing MaybeUninit for those N items).

Gotcha, I just didn't understand why some were and were not like that. Given my goal was to reduce memory allocations I probably had a skewed perspective on the consistency.

Few points of context of the top of my head:

returning resized mutable slices in case the vulkan function fills less items than the caller requests (and removing MaybeUninit for those N items).

Gotcha, 🤔 if the value is less than the requested numbers there is going to be an associated error with it. There seems to be 2 types of methods. Those that can partially fail and those that can't. Generally an OOM or resource limitation error will be the cause so it makes sense to look at the spec for best way to handle it.

For example vkAllocateDescriptorSets cannot partially fail according to the spec, all failed pointers will be deallocated upon a failure and the entire array shall be set to null handles. https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/vkAllocateDescriptorSets.html .

vkAllocateDescriptorSets can be used to create multiple descriptor sets. If the creation of any of those descriptor sets fails, then the implementation must destroy all successfully created descriptor set objects from this command, set all entries of the pDescriptorSets array to [VK_NULL_HANDLE](https://registry.khronos.org/vulkan/specs/1.3-extensions/man/html/VK_NULL_HANDLE.html) and return the error.

The one mainly I can see partially failing is pipeline methods such as vkCreateGraphicPipelines. However, the corresponding create info will also return as a null handle for the failed pipeline and all create infos later in the array. https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#pipelines-multiple

When attempting to create many pipelines in a single command, it is possible that creation may fail for a subset of them. In this case, the corresponding elements of pPipelines will be set to [VK_NULL_HANDLE](https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VK_NULL_HANDLE). If creation fails for a pipeline despite valid arguments (for example, due to out of memory errors), the [VkResult](https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VkResult) code returned by the pipeline creation command will indicate why. The implementation will attempt to create all pipelines, and only return [VK_NULL_HANDLE](https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VK_NULL_HANDLE) values for those that actually failed.

If creation fails for a pipeline that has the VK_PIPELINE_CREATE_EARLY_RETURN_ON_FAILURE_BIT set in its Vk*PipelineCreateInfo, pipelines at an index in the pPipelines array greater than or equal to that of the failing pipeline will be set to [VK_NULL_HANDLE](https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VK_NULL_HANDLE).

Given these spec requirements it doesn't make much sense imo to mess with the slice by mutating it and fitting it. The slice will be updated with spec defined values the user can handle appropriately. Failed values basically always result in values being set to vk::{HandleType}::null(). Mutating the slice imo could get unnecessarily confusion when the user already knows I have a slice of X length, or having to manage lifetimes of an additional slice object. We can put asserts to make sure the slice matches up with the requested length in the corresponding create info's.

accepting slices containing MaybeUninit for true write-only getters (anything with sType/pNext is ruled out);

Yeah I'm mainly interested in the ones that are highly deterministic according to the spec; I do know pNext is on a lot of structs where it doesn't actually get used currently as a catch all if they want to update something so the only caveat I'd add to this is when pNext is actually used by the spec.

accepting slices containing MaybeUninit for true write-only getters (anything with sType/pNext is ruled out);

As an aside, it's absolutely possible to do extension structs through MaybeUninit by partially initializing structs using raw pointers, just far more trouble than it's worth to save a few nanoseconds.

As an aside, it's absolutely possible to do extension structs through MaybeUninit by partially initializing structs using raw pointers, just far more trouble than it's worth to save a few nanoseconds.

@Ralith we've had this discussion before and while it's definitely possible (it is all in the name: "maybe uninitialized" is not ruling out any optional initialization), but taking MaybeUninit for a full structure in a user-facing signature is setting the wrong precedent IMO. We'd have to rely on the unsafe and # Safety docs to convey to the caller that they're expected to always initialize the sType and pNext field anyway for valid Vulkan usage?

My main reason for bringing up MaybeUninit here is @chase-lewis-akasa' request to replace forced Vec allocations with forced memory initializations via &mut [T], unless taking &mut [MaybeUninit<T>] (for e.g. DescriptorSet which is allowed to be MaybeUninit, as we already do via Vec::with_capacity()).

@chase-lewis-akasa the context for updating mutable slice ranges is specifically in the case of Vulkan functions that update a count pointer. get_image_sparse_memory_requirements2() in your initial request is a prime example of this. Currently ash exposes _len() functions to query the size, so that the user can allocate a Vec (or sub-slice a pre-allocated array) and pass that into the getter. There's an assert_eq!() to ensure the Vulkan API returned the requested number of elements.

But if you already have a pre-allocated array, calling the function twice is mostly useless. It should be possible to call it with a pointer to that array straight away, and get the count and values in one go. Consider the following:

let mut groups = [Default::default(); 128];
instance.enumerate_physical_device_groups(&mut groups);

https://github.com/ash-rs/ash/blob/f2979c866ea29c7397000c39c9500d60fbf30f94/ash/src/instance.rs#L138

This panics on my machine because there's only 1 group, while the slice requested 128. In the change that I intend to make, this'll not panic but look like the following:

let mut groups = [Default::default(); 128];
let actual_groups = instance.enumerate_physical_device_groups(&mut groups).unwrap();
assert_eq!(actual_groups.len(), 1);

For functions which could take &mut [MaybeUninit<T>] we could return &mut [T] for the range [..count_returned_by_vulkan].

It goes without saying that this approach supports returning VK_INCOMPLETE when the input slice is not long enough. Though I'm unsure if we should hand back the full mutable slice range to the user in that case together with the "there's more data" 'error'.

You are right that only a few functions return partial results. I recently worked with Khronos to understand and generalize the documentation for these functions, which resulted in PR https://github.com/ash-rs/ash/pull/828. It is not at all my plan to automatically filter this list down to return non-NULL results exclusively. This "returning a resized slice" only applies to the example above where Vulkan returns a count.

ash-rs / ash

Inconsistent interface & unnecessary forced memory allocations in handcrafted methods. #884