Skip to content

Commit b04f15c

Browse files
authored
feat(lambda-rs): buffers to default to DEVICE_LOCAL as opposed to CPU_VISIBLE (#180)
## Summary This PR changes buffer allocation defaults to favor GPU-optimal residency. Previously, `Properties::default()` (and `BufferBuilder::new()`) defaulted to `CPU_VISIBLE`, which biases buffers toward CPU-visible memory and implicitly adds `COPY_DST`. That default is suboptimal for static vertex/index buffers on discrete GPUs because the memory is being allocated on the hosts RAM as opposed to the GPUs VRAM. Now, buffers default to `DEVICE_LOCAL`, and callers must opt into `CPU_VISIBLE` only when they intend to update the buffer from the CPU via `Buffer::write_*`. ## Related Issues - Resolves #104 ## Changes - Changed `Properties::default()` to return `Properties::DEVICE_LOCAL`. - Updated `BufferBuilder::new()` to inherit the `Properties` default. - Updated `BufferBuilder::build_from_mesh` to create vertex buffers as `DEVICE_LOCAL`. - Added unit tests covering the new default behavior. ## Type of Change - [ ] Bug fix (non-breaking change that fixes an issue) - [ ] Feature (non-breaking change that adds functionality) - [x] Breaking change (fix or feature that would cause existing functionality to change) - [x] Documentation (updates to docs, specs, tutorials, or comments) - [ ] Refactor (code change that neither fixes a bug nor adds a feature) - [x] Performance (change that improves performance) - [x] Test (adding or updating tests) - [ ] Build/CI (changes to build process or CI configuration) ## Affected Crates - [x] `lambda-rs` - [ ] `lambda-rs-platform` - [ ] `lambda-rs-args` - [ ] `lambda-rs-logging` - [ ] Other: ## Checklist - [ ] Code follows the repository style guidelines (`cargo +nightly fmt --all`) - [ ] Code passes clippy (`cargo clippy --workspace --all-targets -- -D warnings`) - [ ] Tests pass (`cargo test --workspace`) - [x] New code includes appropriate documentation - [x] Public API changes are documented - [x] Breaking changes are noted in this PR description ## Testing **Commands run:** ```bash cargo test -p lambda-rs ``` **Manual verification steps (if applicable):** 1. Run a render demo that uses `BufferBuilder::build_from_mesh` (static geometry) and confirm it still renders correctly. 2. Run the uniform-buffer demo to verify CPU-updated buffers still explicitly use `CPU_VISIBLE`. ## Screenshots/Recordings N/A (no UI change). ## Platform Testing - [x] macOS - [ ] Windows - [ ] Linux ## Additional Notes - If you relied on implicit CPU visibility (e.g. build a buffer then call `Buffer::write_value` / `write_slice` / `write_bytes`), add `.with_properties(Properties::CPU_VISIBLE)` when constructing that buffer.
2 parents e115036 + 6071c93 commit b04f15c

File tree

2 files changed

+119
-16
lines changed

2 files changed

+119
-16
lines changed

Cargo.lock

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/lambda-rs/src/render/buffer.rs

Lines changed: 117 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -81,8 +81,13 @@ impl Default for Usage {
8181
/// Buffer allocation properties that control residency and CPU visibility.
8282
#[derive(Clone, Copy, Debug)]
8383
///
84-
/// Use `CPU_VISIBLE` for frequently updated data (e.g., uniform uploads).
85-
/// Prefer `DEVICE_LOCAL` for static geometry uploaded once.
84+
/// Use `CPU_VISIBLE` for buffers you plan to update from the CPU using
85+
/// `Buffer::write_*` (this enables `wgpu::Queue::write_buffer` by adding the
86+
/// required `COPY_DST` usage).
87+
///
88+
/// Prefer `DEVICE_LOCAL` for static geometry uploaded once and never modified.
89+
/// This is typically the best default for vertex and index buffers on discrete
90+
/// GPUs, where CPU-visible memory may live in system RAM rather than VRAM.
8691
pub struct Properties {
8792
cpu_visible: bool,
8893
}
@@ -101,7 +106,7 @@ impl Properties {
101106

102107
impl Default for Properties {
103108
fn default() -> Self {
104-
Properties::CPU_VISIBLE
109+
Properties::DEVICE_LOCAL
105110
}
106111
}
107112

@@ -114,11 +119,15 @@ impl Default for Properties {
114119
/// - Writing is performed via the device queue using `write_value` or by
115120
/// creating CPU‑visible buffers and re‑building with new contents when
116121
/// appropriate.
122+
/// - `write_*` operations require the buffer to be created with
123+
/// `Properties::CPU_VISIBLE`. Use `try_write_*` variants if you want to
124+
/// handle this as an error rather than panicking.
117125
#[derive(Debug)]
118126
pub struct Buffer {
119127
buffer: Rc<platform_buffer::Buffer>,
120128
stride: u64,
121129
buffer_type: BufferType,
130+
cpu_visible: bool,
122131
}
123132

124133
impl Buffer {
@@ -139,12 +148,41 @@ impl Buffer {
139148
return self.buffer_type;
140149
}
141150

151+
/// Whether this buffer supports CPU-side queue writes (`write_*`).
152+
pub fn cpu_visible(&self) -> bool {
153+
return self.cpu_visible;
154+
}
155+
156+
fn validate_cpu_write(&self) -> Result<(), &'static str> {
157+
return validate_cpu_write_supported(self.cpu_visible);
158+
}
159+
142160
/// Write a single plain-old-data value into this buffer at the specified
143161
/// byte offset. This is intended for updating uniform buffer contents from
144162
/// the CPU. The `data` type must implement `PlainOldData`.
163+
///
164+
/// # Panics
165+
/// Panics if the buffer was not created with `Properties::CPU_VISIBLE`.
145166
pub fn write_value<T: PlainOldData>(&self, gpu: &Gpu, offset: u64, data: &T) {
167+
self
168+
.try_write_value(gpu, offset, data)
169+
.expect("Buffer::write_value requires a CPU-visible buffer. Create the buffer with `.with_properties(Properties::CPU_VISIBLE)` or use `try_write_value` to handle the error.");
170+
}
171+
172+
/// Fallible variant of [`Buffer::write_value`].
173+
///
174+
/// Returns an error if the buffer was not created with
175+
/// `Properties::CPU_VISIBLE`.
176+
pub fn try_write_value<T: PlainOldData>(
177+
&self,
178+
gpu: &Gpu,
179+
offset: u64,
180+
data: &T,
181+
) -> Result<(), &'static str> {
182+
self.validate_cpu_write()?;
146183
let bytes = value_as_bytes(data);
147-
self.write_bytes(gpu, offset, bytes);
184+
self.buffer.write_bytes(gpu.platform(), offset, bytes);
185+
return Ok(());
148186
}
149187

150188
/// Write raw bytes into this buffer at the specified byte offset.
@@ -157,8 +195,28 @@ impl Buffer {
157195
/// let raw_data: &[u8] = load_binary_data();
158196
/// buffer.write_bytes(render_context.gpu(), 0, raw_data);
159197
/// ```
198+
///
199+
/// # Panics
200+
/// Panics if the buffer was not created with `Properties::CPU_VISIBLE`.
160201
pub fn write_bytes(&self, gpu: &Gpu, offset: u64, data: &[u8]) {
202+
self
203+
.try_write_bytes(gpu, offset, data)
204+
.expect("Buffer::write_bytes requires a CPU-visible buffer. Create the buffer with `.with_properties(Properties::CPU_VISIBLE)` or use `try_write_bytes` to handle the error.");
205+
}
206+
207+
/// Fallible variant of [`Buffer::write_bytes`].
208+
///
209+
/// Returns an error if the buffer was not created with
210+
/// `Properties::CPU_VISIBLE`.
211+
pub fn try_write_bytes(
212+
&self,
213+
gpu: &Gpu,
214+
offset: u64,
215+
data: &[u8],
216+
) -> Result<(), &'static str> {
217+
self.validate_cpu_write()?;
161218
self.buffer.write_bytes(gpu.platform(), offset, data);
219+
return Ok(());
162220
}
163221

164222
/// Write a slice of plain-old-data values into this buffer at the
@@ -182,12 +240,22 @@ impl Buffer {
182240
offset: u64,
183241
data: &[T],
184242
) -> Result<(), &'static str> {
243+
self.validate_cpu_write()?;
185244
let bytes = slice_as_bytes(data)?;
186-
self.write_bytes(gpu, offset, bytes);
245+
self.buffer.write_bytes(gpu.platform(), offset, bytes);
187246
return Ok(());
188247
}
189248
}
190249

250+
fn validate_cpu_write_supported(cpu_visible: bool) -> Result<(), &'static str> {
251+
if !cpu_visible {
252+
return Err(
253+
"Buffer was not created with Properties::CPU_VISIBLE, so CPU writes are not supported. Recreate the buffer with `.with_properties(Properties::CPU_VISIBLE)`.",
254+
);
255+
}
256+
return Ok(());
257+
}
258+
191259
fn value_as_bytes<T: PlainOldData>(data: &T) -> &[u8] {
192260
let bytes = unsafe {
193261
std::slice::from_raw_parts(
@@ -287,7 +355,7 @@ impl<T: PlainOldData> UniformBuffer<T> {
287355
/// let vertices: Vec<Vertex> = build_vertices();
288356
/// let vb = BufferBuilder::new()
289357
/// .with_usage(Usage::VERTEX)
290-
/// .with_properties(Properties::DEVICE_LOCAL)
358+
/// // Defaults to `Properties::DEVICE_LOCAL` (recommended for static geometry).
291359
/// .with_buffer_type(BufferType::Vertex)
292360
/// .build(render_context, vertices)
293361
/// .unwrap();
@@ -308,11 +376,16 @@ impl Default for BufferBuilder {
308376

309377
impl BufferBuilder {
310378
/// Creates a new buffer builder of type vertex.
379+
///
380+
/// Defaults:
381+
/// - `usage`: `Usage::VERTEX`
382+
/// - `properties`: `Properties::DEVICE_LOCAL`
383+
/// - `buffer_type`: `BufferType::Vertex`
311384
pub fn new() -> Self {
312385
Self {
313386
buffer_length: 0,
314387
usage: Usage::VERTEX,
315-
properties: Properties::CPU_VISIBLE,
388+
properties: Properties::default(),
316389
buffer_type: BufferType::Vertex,
317390
label: None,
318391
}
@@ -384,6 +457,7 @@ impl BufferBuilder {
384457
buffer: Rc::new(buffer),
385458
stride: element_size as u64,
386459
buffer_type: self.buffer_type,
460+
cpu_visible: self.properties.cpu_visible(),
387461
});
388462
}
389463

@@ -396,7 +470,7 @@ impl BufferBuilder {
396470
return builder
397471
.with_length(std::mem::size_of_val(mesh.vertices()))
398472
.with_usage(Usage::VERTEX)
399-
.with_properties(Properties::CPU_VISIBLE)
473+
.with_properties(Properties::DEVICE_LOCAL)
400474
.with_buffer_type(BufferType::Vertex)
401475
.build(gpu, mesh.vertices().to_vec());
402476
}
@@ -426,15 +500,43 @@ impl BufferBuilder {
426500
mod tests {
427501
use super::*;
428502

429-
/// Rejects constructing a buffer with a logical length of zero elements.
503+
/// Rejects CPU-side writes for buffers not created with
504+
/// `Properties::CPU_VISIBLE` (prevents wgpu validation panics).
505+
#[test]
506+
fn validate_cpu_write_supported_rejects_non_cpu_visible() {
507+
let result = validate_cpu_write_supported(false);
508+
assert!(result.is_err());
509+
}
510+
511+
/// Accepts CPU-side writes for buffers created with `Properties::CPU_VISIBLE`.
512+
#[test]
513+
fn validate_cpu_write_supported_accepts_cpu_visible() {
514+
let result = validate_cpu_write_supported(true);
515+
assert!(result.is_ok());
516+
}
517+
518+
/// Confirms `Properties::default()` is `DEVICE_LOCAL` (not CPU-visible).
519+
#[test]
520+
fn properties_default_is_device_local() {
521+
assert!(!Properties::default().cpu_visible());
522+
}
523+
524+
/// Confirms `BufferBuilder::new()` inherits the default properties.
525+
#[test]
526+
fn buffer_builder_defaults_to_device_local_properties() {
527+
let builder = BufferBuilder::new();
528+
assert!(!builder.properties.cpu_visible());
529+
}
530+
531+
/// Validates that size resolution rejects creating a zero-length buffer.
430532
#[test]
431533
fn resolve_length_rejects_zero() {
432534
let builder = BufferBuilder::new();
433535
let result = builder.resolve_length(std::mem::size_of::<u32>(), 0);
434536
assert!(result.is_err());
435537
}
436538

437-
/// Ensures builder labels are stored for later propagation/debugging.
539+
/// Ensures `with_label` stores the label on the builder.
438540
#[test]
439541
fn label_is_recorded_on_builder() {
440542
let builder = BufferBuilder::new().with_label("buffer-test");
@@ -443,23 +545,24 @@ mod tests {
443545
assert_eq!(builder.label.as_deref(), Some("buffer-test"));
444546
}
445547

446-
/// Rejects length computations that would overflow `usize`.
548+
/// Rejects length computations that would overflow `usize` when converting
549+
/// element counts/sizes to byte sizes.
447550
#[test]
448551
fn resolve_length_rejects_overflow() {
449552
let builder = BufferBuilder::new();
450553
let result = builder.resolve_length(usize::MAX, 2);
451554
assert!(result.is_err());
452555
}
453556

454-
/// Confirms `value_as_bytes` uses native-endian byte order and size.
557+
/// Confirms `value_as_bytes` matches the native byte representation.
455558
#[test]
456559
fn value_as_bytes_matches_native_bytes() {
457560
let value: u32 = 0x1122_3344;
458561
let expected = value.to_ne_bytes();
459562
assert_eq!(value_as_bytes(&value), expected.as_slice());
460563
}
461564

462-
/// Confirms `slice_as_bytes` flattens a typed slice to the native bytes.
565+
/// Confirms `slice_as_bytes` matches the expected concatenated native bytes.
463566
#[test]
464567
fn slice_as_bytes_matches_native_bytes() {
465568
let values: [u16; 3] = [0x1122, 0x3344, 0x5566];
@@ -494,7 +597,7 @@ mod tests {
494597
let combined = Usage::VERTEX | Usage::INDEX;
495598
let _ = combined.to_platform();
496599

497-
assert!(Properties::default().cpu_visible());
600+
assert!(!Properties::default().cpu_visible());
498601
assert!(!Properties::DEVICE_LOCAL.cpu_visible());
499602
}
500603

0 commit comments

Comments
 (0)