From 8670f3d2510900ce11a41e45c8e5fb1620f82256 Mon Sep 17 00:00:00 2001 From: marc0246 <40955683+marc0246@users.noreply.github.com> Date: Fri, 28 Oct 2022 09:23:29 +0200 Subject: [PATCH] #1997 fixes (#2049) * Fix big oopsies * Fix docs * Syntactical improvements * Use more fitting names for some private types --- vulkano/src/memory/allocator/mod.rs | 33 +++--- vulkano/src/memory/allocator/suballocator.rs | 108 +++++++++++-------- 2 files changed, 80 insertions(+), 61 deletions(-) diff --git a/vulkano/src/memory/allocator/mod.rs b/vulkano/src/memory/allocator/mod.rs index 933a7f7044..7d023f5d7f 100644 --- a/vulkano/src/memory/allocator/mod.rs +++ b/vulkano/src/memory/allocator/mod.rs @@ -382,7 +382,6 @@ pub struct AllocationCreateInfo<'d> { /// /// [`alignment`]: MemoryRequirements::alignment /// [`memory_type_bits`]: MemoryRequirements::memory_type_bits - /// /// [`UnsafeBuffer::memory_requirements`]: crate::buffer::sys::UnsafeBuffer::memory_requirements /// [`UnsafeImage::memory_requirements`]: crate::image::sys::UnsafeImage::memory_requirements pub requirements: MemoryRequirements, @@ -465,8 +464,8 @@ pub enum MemoryUsage { /// once and then never again, or resources that are only written and read by the GPU, like /// render targets and intermediary buffers. /// - /// [`device_local`]: super::MemoryPropertyFlags::device_local - /// [`host_visible`]: super::MemoryPropertyFlags::host_visible + /// [`device_local`]: MemoryPropertyFlags::device_local + /// [`host_visible`]: MemoryPropertyFlags::host_visible GpuOnly, /// The memory is intended for upload to the GPU. @@ -479,9 +478,9 @@ pub enum MemoryUsage { /// whose only purpose in life it is to get data into `device_local` memory or texels into an /// optimal image. /// - /// [`host_visible`]: super::MemoryPropertyFlags::host_visible - /// [`host_cached`]: super::MemoryPropertyFlags::host_cached - /// [`device_local`]: super::MemoryPropertyFlags::device_local + /// [`host_visible`]: MemoryPropertyFlags::host_visible + /// [`host_cached`]: MemoryPropertyFlags::host_cached + /// [`device_local`]: MemoryPropertyFlags::device_local Upload, /// The memory is intended for download from the GPU. @@ -493,9 +492,9 @@ pub enum MemoryUsage { /// need to get the results back to the CPU. That might be compute shading, or image or video /// manipulation, or screenshotting for example. /// - /// [`host_visible`]: super::MemoryPropertyFlags::host_visible - /// [`host_cached`]: super::MemoryPropertyFlags::host_cached - /// [`device_local`]: super::MemoryPropertyFlags::device_local + /// [`host_visible`]: MemoryPropertyFlags::host_visible + /// [`host_cached`]: MemoryPropertyFlags::host_cached + /// [`device_local`]: MemoryPropertyFlags::device_local Download, } @@ -1028,7 +1027,11 @@ unsafe impl MemoryAllocator for GenericMemoryAllocator { memory_type_index, create_info.size, None, - self.export_handle_types[memory_type_index as usize], + if !self.export_handle_types.is_empty() { + self.export_handle_types[memory_type_index as usize] + } else { + ExternalMemoryHandleTypes::empty() + }, ) }; } @@ -1216,11 +1219,11 @@ unsafe impl MemoryAllocator for GenericMemoryAllocator { /// - Returns an error if allocating a new block is required and failed. This can be one of the /// OOM errors or [`TooManyObjects`]. /// - Returns [`OutOfPoolMemory`] if `create_info.allocate_preference` is - /// [`MemoryAllocatePreference::NeverAllocate`] and `create_info.requirements.size` is greater - /// than the block size for all heaps of suitable memory types. - /// - Returns [`BlockSizeExceeded`] if `create_info.allocate_preference` is /// [`MemoryAllocatePreference::NeverAllocate`] and none of the pools of suitable memory /// types have enough free space. + /// - Returns [`BlockSizeExceeded`] if `create_info.allocate_preference` is + /// [`MemoryAllocatePreference::NeverAllocate`] and `create_info.requirements.size` is greater + /// than the block size for all heaps of suitable memory types. /// - Returns [`SuballocatorBlockSizeExceeded`] if `S` is `PoolAllocator` and /// `create_info.size` is greater than `BLOCK_SIZE` and a dedicated allocation was not /// created. @@ -1439,7 +1442,9 @@ pub struct GenericMemoryAllocatorCreateInfo<'b, 'e> { /// /// You only need to worry about this if you're using [`PoolAllocator`] as the suballocator, as /// all suballocations that the pool allocator makes inherit their allocation type from the - /// parent allocation. In all other cases it doesn't matter what this is. + /// parent allocation. For the [`FreeListAllocator`] and the [`BuddyAllocator`] this must be + /// [`AllocationType::Unknown`] otherwise you will get panics. It does not matter what this is + /// for when using the [`BumpAllocator`]. /// /// The default value is [`AllocationType::Unknown`]. pub allocation_type: AllocationType, diff --git a/vulkano/src/memory/allocator/suballocator.rs b/vulkano/src/memory/allocator/suballocator.rs index 977f96d0b8..6619f481c0 100644 --- a/vulkano/src/memory/allocator/suballocator.rs +++ b/vulkano/src/memory/allocator/suballocator.rs @@ -1,3 +1,12 @@ +// Copyright (c) 2016 The vulkano developers +// Licensed under the Apache License, Version 2.0 +// or the MIT +// license , +// at your option. All files in the project carrying such +// notice may not be copied, modified, or distributed except +// according to those terms. + //! Suballocators are used to divide a *region* into smaller *suballocations*. //! //! See also [the parent module] for details about memory allocation in Vulkan. @@ -996,7 +1005,7 @@ pub struct FreeListAllocator { atom_size: DeviceSize, // Total memory remaining in the region. free_size: AtomicU64, - inner: Mutex, + state: Mutex, } impl FreeListAllocator { @@ -1036,12 +1045,12 @@ impl FreeListAllocator { let root_id = nodes.allocate(SuballocationListNode { prev: None, next: None, - offset: 0, + offset: region.offset, size: region.size, ty: SuballocationType::Free, }); free_list.push(root_id); - let inner = Mutex::new(FreeListAllocatorInner { nodes, free_list }); + let state = Mutex::new(FreeListAllocatorState { nodes, free_list }); Arc::new(FreeListAllocator { region, @@ -1049,17 +1058,17 @@ impl FreeListAllocator { buffer_image_granularity, atom_size, free_size, - inner, + state, }) } fn free(&self, id: SlotId) { - let mut inner = self.inner.lock(); + let mut state = self.state.lock(); self.free_size - .fetch_add(inner.nodes.get(id).size, Ordering::Release); - inner.nodes.get_mut(id).ty = SuballocationType::Free; - inner.coalesce(id); - inner.free(id); + .fetch_add(state.nodes.get(id).size, Ordering::Release); + state.nodes.get_mut(id).ty = SuballocationType::Free; + state.coalesce(id); + state.free(id); } } @@ -1126,13 +1135,13 @@ unsafe impl Suballocator for Arc { } = create_info; let alignment = DeviceSize::max(alignment, self.atom_size); - let mut inner = self.inner.lock(); + let mut state = self.state.lock(); - match inner.free_list.last() { - Some(&last) if inner.nodes.get(last).size >= size => { - let index = match inner + match state.free_list.last() { + Some(&last) if state.nodes.get(last).size >= size => { + let index = match state .free_list - .binary_search_by_key(&size, |&x| inner.nodes.get(x).size) + .binary_search_by_key(&size, |&x| state.nodes.get(x).size) { // Exact fit. Ok(index) => index, @@ -1141,12 +1150,12 @@ unsafe impl Suballocator for Arc { Err(index) => index, }; - for &id in &inner.free_list[index..] { - let suballoc = inner.nodes.get(id); - let mut offset = align_up(self.region.offset + suballoc.offset, alignment); + for &id in &state.free_list[index..] { + let suballoc = state.nodes.get(id); + let mut offset = align_up(suballoc.offset, alignment); if let Some(prev_id) = suballoc.prev { - let prev = inner.nodes.get(prev_id); + let prev = state.nodes.get(prev_id); if are_blocks_on_same_page( prev.offset, @@ -1160,9 +1169,9 @@ unsafe impl Suballocator for Arc { } if offset + size <= suballoc.offset + suballoc.size { - inner.allocate(id); - inner.split(id, offset, size); - inner.nodes.get_mut(id).ty = allocation_type.into(); + state.allocate(id); + state.split(id, offset, size); + state.nodes.get_mut(id).ty = allocation_type.into(); self.free_size.fetch_sub(size, Ordering::Release); return Ok(MemoryAlloc { @@ -1224,7 +1233,7 @@ unsafe impl DeviceOwned for FreeListAllocator { } #[derive(Debug)] -struct FreeListAllocatorInner { +struct FreeListAllocatorState { nodes: host::PoolAllocator, // Free suballocations sorted by size in ascending order. This means we can always find a // best-fit in *O*(log(*n*)) time in the worst case, and iterating in order is very efficient. @@ -1260,7 +1269,7 @@ impl From for SuballocationType { } } -impl FreeListAllocatorInner { +impl FreeListAllocatorState { /// Removes the target suballocation from the free-list. The free-list must contain it. fn allocate(&mut self, node_id: SlotId) { debug_assert!(self.free_list.contains(&node_id)); @@ -1384,13 +1393,9 @@ impl FreeListAllocatorInner { debug_assert!(!self.free_list.contains(&node_id)); let node = self.nodes.get(node_id); - let index = match self + let (Ok(index) | Err(index)) = self .free_list - .binary_search_by_key(&node.size, |&x| self.nodes.get(x).size) - { - Ok(index) => index, - Err(index) => index, - }; + .binary_search_by_key(&node.size, |&x| self.nodes.get(x).size); self.free_list.insert(index, node_id); } @@ -1502,7 +1507,8 @@ impl FreeListAllocatorInner { /// block_sizes: &[(0, 64 * 1024 * 1024)], /// ..Default::default() /// }, -/// ); +/// ) +/// .unwrap(); /// /// // Now you can use `memory_allocator` to allocate whatever it is you need. /// ``` @@ -1521,7 +1527,7 @@ pub struct BuddyAllocator { atom_size: DeviceSize, // Total memory remaining in the region. free_size: AtomicU64, - inner: Mutex, + state: Mutex, } impl BuddyAllocator { @@ -1569,7 +1575,7 @@ impl BuddyAllocator { let mut free_list = ArrayVec::new(max_order + 1, [EMPTY_FREE_LIST; Self::MAX_ORDERS]); // The root node has the lowest offset and highest order, so it's the whole region. free_list[max_order].push(region.offset); - let inner = Mutex::new(BuddyAllocatorInner { free_list }); + let state = Mutex::new(BuddyAllocatorState { free_list }); Arc::new(BuddyAllocator { region, @@ -1577,15 +1583,15 @@ impl BuddyAllocator { buffer_image_granularity, atom_size, free_size, - inner, + state, }) } fn free(&self, min_order: usize, mut offset: DeviceSize) { - let mut inner = self.inner.lock(); + let mut state = self.state.lock(); // Try to coalesce nodes while incrementing the order. - for (order, free_list) in inner.free_list.iter_mut().enumerate().skip(min_order) { + for (order, free_list) in state.free_list.iter_mut().enumerate().skip(min_order) { let size = Self::MIN_NODE_SIZE << order; let buddy_offset = ((offset - self.region.offset) ^ size) + self.region.offset; @@ -1597,10 +1603,7 @@ impl BuddyAllocator { } // Otherwise free the node. Err(_) => { - let index = match free_list.binary_search(&offset) { - Ok(index) => index, - Err(index) => index, - }; + let (Ok(index) | Err(index)) = free_list.binary_search(&offset); free_list.insert(index, offset); self.free_size .fetch_add(Self::MIN_NODE_SIZE << min_order, Ordering::Release); @@ -1683,17 +1686,17 @@ unsafe impl Suballocator for Arc { let size = DeviceSize::max(size, BuddyAllocator::MIN_NODE_SIZE).next_power_of_two(); let alignment = DeviceSize::max(alignment, self.atom_size); let min_order = (size / BuddyAllocator::MIN_NODE_SIZE).trailing_zeros() as usize; - let mut inner = self.inner.lock(); + let mut state = self.state.lock(); // Start searching at the lowest possible order going up. - for (order, free_list) in inner.free_list.iter_mut().enumerate().skip(min_order) { + for (order, free_list) in state.free_list.iter_mut().enumerate().skip(min_order) { for (index, &offset) in free_list.iter().enumerate() { if offset % alignment == 0 { free_list.remove(index); // Go in the opposite direction, splitting nodes from higher orders. The lowest // order doesn't need any splitting. - for (order, free_list) in inner + for (order, free_list) in state .free_list .iter_mut() .enumerate() @@ -1771,7 +1774,7 @@ unsafe impl DeviceOwned for BuddyAllocator { } #[derive(Debug)] -struct BuddyAllocatorInner { +struct BuddyAllocatorState { // Every order has its own free-list for convenience, so that we don't have to traverse a tree. // Each free-list is sorted by offset because we want to find the first-fit as this strategy // minimizes external fragmentation. @@ -1876,7 +1879,8 @@ struct BuddyAllocatorInner { /// block_sizes: &[(0, 64 * 1024 * 1024)], /// ..Default::default() /// }, -/// ); +/// ) +/// .unwrap(); /// /// // Now you can use `memory_allocator` to allocate whatever it is you need. /// ``` @@ -1963,6 +1967,9 @@ unsafe impl Suballocator for Arc **Note**: `create_info.allocation_type` is silently ignored because all suballocations + /// > inherit the allocation type from the region. + /// /// # Panics /// /// - Panics if `create_info.size` is zero. @@ -1976,6 +1983,7 @@ unsafe impl Suballocator for Arc Suballocator for Arc unaligned_offset + self.block_size { self.free_list.push(index).unwrap(); - return Err(SuballocationCreationError::BlockSizeExceeded); + return if size > self.block_size { + Err(SuballocationCreationError::BlockSizeExceeded) + } else { + // There is not enough space due to alignment requirements. + Err(SuballocationCreationError::OutOfRegionMemory) + }; } Ok(MemoryAlloc { @@ -2255,7 +2269,7 @@ impl BumpAllocator { #[inline] pub unsafe fn reset_unchecked(&self) { self.state - .store(self.region.allocation_type as u64, Ordering::Relaxed); + .store(self.region.allocation_type as u64, Ordering::Release); } }