Skip to content

Commit

Permalink
Remove unnecesssary values Vec from DynamicUniformBuffer and DynamicS…
Browse files Browse the repository at this point in the history
…torageBuffer (#8299)

# Objective
Fixes #8284. `values` is being pushed to separately from the actual
scratch buffer in `DynamicUniformBuffer::push` and
`DynamicStorageBuffer::push`. In both types, `values` is really only
used to track the number of elements being added to the buffer, yet is
causing extra allocations, size increments and excess copies.

## Solution
Remove it and its remaining uses. Replace it with accesses to `scratch`
instead.

I removed the `len` accessor, as it may be non-trivial to compute just
from `scratch`. If this is still desirable to have, we can keep a `len`
member field to track it instead of relying on `scratch`.
  • Loading branch information
james7132 authored Apr 4, 2023
1 parent ed50c8b commit 63d89d3
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 24 deletions.
18 changes: 6 additions & 12 deletions crates/bevy_render/src/render_resource/storage_buffer.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#![allow(clippy::doc_markdown)]

use std::marker::PhantomData;

use super::Buffer;
use crate::renderer::{RenderDevice, RenderQueue};
use encase::{
Expand Down Expand Up @@ -160,25 +162,25 @@ impl<T: ShaderType + WriteInto> StorageBuffer<T> {
///
/// [std430 alignment/padding requirements]: https://www.w3.org/TR/WGSL/#address-spaces-storage
pub struct DynamicStorageBuffer<T: ShaderType> {
values: Vec<T>,
scratch: DynamicStorageBufferWrapper<Vec<u8>>,
buffer: Option<Buffer>,
capacity: usize,
label: Option<String>,
changed: bool,
buffer_usage: BufferUsages,
_marker: PhantomData<fn() -> T>,
}

impl<T: ShaderType> Default for DynamicStorageBuffer<T> {
fn default() -> Self {
Self {
values: Vec::new(),
scratch: DynamicStorageBufferWrapper::new(Vec::new()),
buffer: None,
capacity: 0,
label: None,
changed: false,
buffer_usage: BufferUsages::COPY_DST | BufferUsages::STORAGE,
_marker: PhantomData,
}
}
}
Expand All @@ -198,21 +200,14 @@ impl<T: ShaderType + WriteInto> DynamicStorageBuffer<T> {
}))
}

#[inline]
pub fn len(&self) -> usize {
self.values.len()
}

#[inline]
pub fn is_empty(&self) -> bool {
self.values.is_empty()
self.scratch.as_ref().is_empty()
}

#[inline]
pub fn push(&mut self, value: T) -> u32 {
let offset = self.scratch.write(&value).unwrap() as u32;
self.values.push(value);
offset
self.scratch.write(&value).unwrap() as u32
}

pub fn set_label(&mut self, label: Option<&str>) {
Expand Down Expand Up @@ -258,7 +253,6 @@ impl<T: ShaderType + WriteInto> DynamicStorageBuffer<T> {

#[inline]
pub fn clear(&mut self) {
self.values.clear();
self.scratch.as_mut().clear();
self.scratch.set_offset(0);
}
Expand Down
18 changes: 6 additions & 12 deletions crates/bevy_render/src/render_resource/uniform_buffer.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use std::marker::PhantomData;

use crate::{
render_resource::Buffer,
renderer::{RenderDevice, RenderQueue},
Expand Down Expand Up @@ -153,25 +155,25 @@ impl<T: ShaderType + WriteInto> UniformBuffer<T> {
///
/// [std140 alignment/padding requirements]: https://www.w3.org/TR/WGSL/#address-spaces-uniform
pub struct DynamicUniformBuffer<T: ShaderType> {
values: Vec<T>,
scratch: DynamicUniformBufferWrapper<Vec<u8>>,
buffer: Option<Buffer>,
capacity: usize,
label: Option<String>,
changed: bool,
buffer_usage: BufferUsages,
_marker: PhantomData<fn() -> T>,
}

impl<T: ShaderType> Default for DynamicUniformBuffer<T> {
fn default() -> Self {
Self {
values: Vec::new(),
scratch: DynamicUniformBufferWrapper::new(Vec::new()),
buffer: None,
capacity: 0,
label: None,
changed: false,
buffer_usage: BufferUsages::COPY_DST | BufferUsages::UNIFORM,
_marker: PhantomData,
}
}
}
Expand All @@ -191,22 +193,15 @@ impl<T: ShaderType + WriteInto> DynamicUniformBuffer<T> {
}))
}

#[inline]
pub fn len(&self) -> usize {
self.values.len()
}

#[inline]
pub fn is_empty(&self) -> bool {
self.values.is_empty()
self.scratch.as_ref().is_empty()
}

/// Push data into the `DynamicUniformBuffer`'s internal vector (residing on system RAM).
#[inline]
pub fn push(&mut self, value: T) -> u32 {
let offset = self.scratch.write(&value).unwrap() as u32;
self.values.push(value);
offset
self.scratch.write(&value).unwrap() as u32
}

pub fn set_label(&mut self, label: Option<&str>) {
Expand Down Expand Up @@ -257,7 +252,6 @@ impl<T: ShaderType + WriteInto> DynamicUniformBuffer<T> {

#[inline]
pub fn clear(&mut self) {
self.values.clear();
self.scratch.as_mut().clear();
self.scratch.set_offset(0);
}
Expand Down

0 comments on commit 63d89d3

Please sign in to comment.