From 640e031919dd6db194e97d0eab0401fb2c4c3cd7 Mon Sep 17 00:00:00 2001 From: Kevin Reid Date: Sun, 27 Apr 2025 09:38:23 -0700 Subject: [PATCH] Make `BufferSlice`'s size non-optional. (#7640) It used to be that `wgpu::Buffer` did not know its own size, and so slices had to potentially not know their endpoints. Now, buffers do, so slices can. This makes the code simpler, without modifying the API. --- wgpu/src/api/buffer.rs | 146 +++++++++++--------------- wgpu/src/api/device.rs | 4 +- wgpu/src/api/render_bundle_encoder.rs | 4 +- wgpu/src/api/render_pass.rs | 4 +- wgpu/src/util/mod.rs | 5 +- 5 files changed, 68 insertions(+), 95 deletions(-) diff --git a/wgpu/src/api/buffer.rs b/wgpu/src/api/buffer.rs index c962c8d65c..2f9bd2248e 100644 --- a/wgpu/src/api/buffer.rs +++ b/wgpu/src/api/buffer.rs @@ -240,8 +240,9 @@ impl Buffer { /// /// - If `bounds` is outside of the bounds of `self`. /// - If `bounds` has a length less than 1. + #[track_caller] pub fn slice>(&self, bounds: S) -> BufferSlice<'_> { - let (offset, size) = range_to_offset_size(bounds); + let (offset, size) = range_to_offset_size(bounds, self.size); check_buffer_bounds(self.size, offset, size); BufferSlice { buffer: self, @@ -438,7 +439,7 @@ impl Buffer { pub struct BufferSlice<'a> { pub(crate) buffer: &'a Buffer, pub(crate) offset: BufferAddress, - pub(crate) size: Option, + pub(crate) size: BufferSize, } #[cfg(send_sync)] static_assertions::assert_impl_all!(BufferSlice<'_>: Send, Sync); @@ -456,20 +457,14 @@ impl<'a> BufferSlice<'a> { /// /// - If `bounds` is outside of the bounds of `self`. /// - If `bounds` has a length less than 1. + #[track_caller] pub fn slice>(&self, bounds: S) -> BufferSlice<'a> { - let (offset, size) = range_to_offset_size(bounds); - check_buffer_bounds( - match self.size { - Some(size) => size.get(), - None => self.buffer.size(), - }, - offset, - size, - ); + let (offset, size) = range_to_offset_size(bounds, self.size.get()); + check_buffer_bounds(self.size.get(), offset, size); BufferSlice { buffer: self.buffer, offset: self.offset + offset, // check_buffer_bounds ensures this does not overflow - size: size.or(self.size), // check_buffer_bounds ensures this is essentially min() + size, // check_buffer_bounds ensures this is essentially min() } } @@ -502,10 +497,7 @@ impl<'a> BufferSlice<'a> { ) { let mut mc = self.buffer.map_context.lock(); assert_eq!(mc.initial_range, 0..0, "Buffer is already mapped"); - let end = match self.size { - Some(s) => self.offset + s.get(), - None => mc.total_size, - }; + let end = self.offset + self.size.get(); mc.initial_range = self.offset..end; self.buffer @@ -608,10 +600,7 @@ impl<'a> BufferSlice<'a> { /// Returns the size of this slice. pub fn size(&self) -> BufferSize { - self.size.unwrap_or_else(|| { - (|| BufferSize::new(self.buffer.size().checked_sub(self.offset)?))() - .expect("can't happen: slice has incorrect size for its buffer") - }) + self.size } } @@ -622,7 +611,7 @@ impl<'a> From> for crate::BufferBinding<'a> { BufferBinding { buffer: value.buffer, offset: value.offset, - size: value.size, + size: Some(value.size), } } } @@ -637,16 +626,9 @@ impl<'a> From> for crate::BindingResource<'a> { /// The mapped portion of a buffer, if any, and its outstanding views. /// -/// This ensures that views fall within the mapped range and don't overlap, and -/// also takes care of turning `Option` sizes into actual buffer -/// offsets. +/// This ensures that views fall within the mapped range and don't overlap. #[derive(Debug)] pub(crate) struct MapContext { - /// The overall size of the buffer. - /// - /// This is just a convenient copy of [`Buffer::size`]. - pub(crate) total_size: BufferAddress, - /// The range of the buffer that is mapped. /// /// This is `0..0` if the buffer is not mapped. This becomes non-empty when @@ -664,9 +646,8 @@ pub(crate) struct MapContext { } impl MapContext { - pub(crate) fn new(total_size: BufferAddress) -> Self { + pub(crate) fn new() -> Self { Self { - total_size, initial_range: 0..0, sub_ranges: Vec::new(), } @@ -689,11 +670,8 @@ impl MapContext { /// # Panics /// /// This panics if the given range overlaps with any existing range. - fn add(&mut self, offset: BufferAddress, size: Option) -> BufferAddress { - let end = match size { - Some(s) => offset + s.get(), - None => self.initial_range.end, - }; + fn add(&mut self, offset: BufferAddress, size: BufferSize) -> BufferAddress { + let end = offset + size.get(); assert!(self.initial_range.start <= offset && end <= self.initial_range.end); // This check is essential for avoiding undefined behavior: it is the // only thing that ensures that `&mut` references to the buffer's @@ -716,11 +694,8 @@ impl MapContext { /// passed to [`add`]. /// /// [`add]`: MapContext::add - fn remove(&mut self, offset: BufferAddress, size: Option) { - let end = match size { - Some(s) => offset + s.get(), - None => self.initial_range.end, - }; + fn remove(&mut self, offset: BufferAddress, size: BufferSize) { + let end = offset + size.get(); let index = self .sub_ranges @@ -871,99 +846,100 @@ impl Drop for BufferViewMut<'_> { } } +#[track_caller] fn check_buffer_bounds( buffer_size: BufferAddress, - offset: BufferAddress, - size: Option, + slice_offset: BufferAddress, + slice_size: BufferSize, ) { // A slice of length 0 is invalid, so the offset must not be equal to or greater than the buffer size. - if offset >= buffer_size { + if slice_offset >= buffer_size { panic!( "slice offset {} is out of range for buffer of size {}", - offset, buffer_size + slice_offset, buffer_size ); } - if let Some(size) = size { - // Detect integer overflow. - let end = offset.checked_add(size.get()); - if end.is_none_or(|end| end > buffer_size) { - panic!( - "slice offset {} size {} is out of range for buffer of size {}", - offset, size, buffer_size - ); - } + // Detect integer overflow. + let end = slice_offset.checked_add(slice_size.get()); + if end.is_none_or(|end| end > buffer_size) { + panic!( + "slice offset {} size {} is out of range for buffer of size {}", + slice_offset, slice_size, buffer_size + ); } } +#[track_caller] fn range_to_offset_size>( bounds: S, -) -> (BufferAddress, Option) { + whole_size: BufferAddress, +) -> (BufferAddress, BufferSize) { let offset = match bounds.start_bound() { Bound::Included(&bound) => bound, Bound::Excluded(&bound) => bound + 1, Bound::Unbounded => 0, }; - let size = match bounds.end_bound() { - Bound::Included(&bound) => Some(bound + 1 - offset), - Bound::Excluded(&bound) => Some(bound - offset), - Bound::Unbounded => None, - } - .map(|size| BufferSize::new(size).expect("Buffer slices can not be empty")); + let size = BufferSize::new(match bounds.end_bound() { + Bound::Included(&bound) => bound + 1 - offset, + Bound::Excluded(&bound) => bound - offset, + Bound::Unbounded => whole_size - offset, + }) + .expect("buffer slices can not be empty"); (offset, size) } #[cfg(test)] mod tests { - use super::{check_buffer_bounds, range_to_offset_size, BufferSize}; + use super::{check_buffer_bounds, range_to_offset_size, BufferAddress, BufferSize}; + + fn bs(value: BufferAddress) -> BufferSize { + BufferSize::new(value).unwrap() + } #[test] fn range_to_offset_size_works() { - assert_eq!(range_to_offset_size(0..2), (0, BufferSize::new(2))); - assert_eq!(range_to_offset_size(2..5), (2, BufferSize::new(3))); - assert_eq!(range_to_offset_size(..), (0, None)); - assert_eq!(range_to_offset_size(21..), (21, None)); - assert_eq!(range_to_offset_size(0..), (0, None)); - assert_eq!(range_to_offset_size(..21), (0, BufferSize::new(21))); + let whole = 100; + + assert_eq!(range_to_offset_size(0..2, whole), (0, bs(2))); + assert_eq!(range_to_offset_size(2..5, whole), (2, bs(3))); + assert_eq!(range_to_offset_size(.., whole), (0, bs(whole))); + assert_eq!(range_to_offset_size(21.., whole), (21, bs(whole - 21))); + assert_eq!(range_to_offset_size(0.., whole), (0, bs(whole))); + assert_eq!(range_to_offset_size(..21, whole), (0, bs(21))); } #[test] - #[should_panic] + #[should_panic = "buffer slices can not be empty"] fn range_to_offset_size_panics_for_empty_range() { - range_to_offset_size(123..123); + range_to_offset_size(123..123, 200); } #[test] - #[should_panic] + #[should_panic = "buffer slices can not be empty"] fn range_to_offset_size_panics_for_unbounded_empty_range() { - range_to_offset_size(..0); - } - - #[test] - #[should_panic] - fn check_buffer_bounds_panics_for_offset_at_size() { - check_buffer_bounds(100, 100, None); + range_to_offset_size(..0, 100); } #[test] fn check_buffer_bounds_works_for_end_in_range() { - check_buffer_bounds(200, 100, BufferSize::new(50)); - check_buffer_bounds(200, 100, BufferSize::new(100)); - check_buffer_bounds(u64::MAX, u64::MAX - 100, BufferSize::new(100)); - check_buffer_bounds(u64::MAX, 0, BufferSize::new(u64::MAX)); - check_buffer_bounds(u64::MAX, 1, BufferSize::new(u64::MAX - 1)); + check_buffer_bounds(200, 100, bs(50)); + check_buffer_bounds(200, 100, bs(100)); + check_buffer_bounds(u64::MAX, u64::MAX - 100, bs(100)); + check_buffer_bounds(u64::MAX, 0, bs(u64::MAX)); + check_buffer_bounds(u64::MAX, 1, bs(u64::MAX - 1)); } #[test] #[should_panic] fn check_buffer_bounds_panics_for_end_over_size() { - check_buffer_bounds(200, 100, BufferSize::new(101)); + check_buffer_bounds(200, 100, bs(101)); } #[test] #[should_panic] fn check_buffer_bounds_panics_for_end_wraparound() { - check_buffer_bounds(u64::MAX, 1, BufferSize::new(u64::MAX)); + check_buffer_bounds(u64::MAX, 1, bs(u64::MAX)); } } diff --git a/wgpu/src/api/device.rs b/wgpu/src/api/device.rs index 65fd65d194..2955f322a7 100644 --- a/wgpu/src/api/device.rs +++ b/wgpu/src/api/device.rs @@ -254,7 +254,7 @@ impl Device { /// Creates a [`Buffer`]. #[must_use] pub fn create_buffer(&self, desc: &BufferDescriptor<'_>) -> Buffer { - let mut map_context = MapContext::new(desc.size); + let mut map_context = MapContext::new(); if desc.mapped_at_creation { map_context.initial_range = 0..desc.size; } @@ -330,7 +330,7 @@ impl Device { hal_buffer: A::Buffer, desc: &BufferDescriptor<'_>, ) -> Buffer { - let mut map_context = MapContext::new(desc.size); + let mut map_context = MapContext::new(); if desc.mapped_at_creation { map_context.initial_range = 0..desc.size; } diff --git a/wgpu/src/api/render_bundle_encoder.rs b/wgpu/src/api/render_bundle_encoder.rs index a33c3cb237..483bfd084d 100644 --- a/wgpu/src/api/render_bundle_encoder.rs +++ b/wgpu/src/api/render_bundle_encoder.rs @@ -93,7 +93,7 @@ impl<'a> RenderBundleEncoder<'a> { &buffer_slice.buffer.inner, index_format, buffer_slice.offset, - buffer_slice.size, + Some(buffer_slice.size), ); } @@ -112,7 +112,7 @@ impl<'a> RenderBundleEncoder<'a> { slot, &buffer_slice.buffer.inner, buffer_slice.offset, - buffer_slice.size, + Some(buffer_slice.size), ); } diff --git a/wgpu/src/api/render_pass.rs b/wgpu/src/api/render_pass.rs index 5e150d72a0..0092c2af74 100644 --- a/wgpu/src/api/render_pass.rs +++ b/wgpu/src/api/render_pass.rs @@ -100,7 +100,7 @@ impl RenderPass<'_> { &buffer_slice.buffer.inner, index_format, buffer_slice.offset, - buffer_slice.size, + Some(buffer_slice.size), ); } @@ -121,7 +121,7 @@ impl RenderPass<'_> { slot, &buffer_slice.buffer.inner, buffer_slice.offset, - buffer_slice.size, + Some(buffer_slice.size), ); } diff --git a/wgpu/src/util/mod.rs b/wgpu/src/util/mod.rs index 358563c708..1af0f49488 100644 --- a/wgpu/src/util/mod.rs +++ b/wgpu/src/util/mod.rs @@ -98,10 +98,7 @@ impl DownloadBuffer { buffer: &super::BufferSlice<'_>, callback: impl FnOnce(Result) + Send + 'static, ) { - let size = match buffer.size { - Some(size) => size.into(), - None => buffer.buffer.map_context.lock().total_size - buffer.offset, - }; + let size = buffer.size.into(); let download = device.create_buffer(&super::BufferDescriptor { size,