Skip to content

Commit

Permalink
Document that write_buffer_with is sound to read from. (#3006)
Browse files Browse the repository at this point in the history
* Document that `write_buffer_with` is sound to read from.

To a reader informed about Rust's memory model, the existing claim that

> dereferencing it to a `&[u8]` panics

without further context sounds awfully like someone thinks they've
invented a write-only reference, and that the API might actually be
exposing undefined behavior via an uninitialized `&mut [u8]`. Therefore,
let's specify what happens if you *do* read through the mutable
reference.  The text added in this commit is based on what was said in
the review when `write_buffer_with` was added:
https://github.com/gfx-rs/wgpu/pull/2777/files#r901392551

This is also relevant information to someone considering using
`write_buffer_with()` for performance gains: for example, it suggests
that it might be a bad idea to write data into the view and then sort
it in-place. (Or is that not a bad idea? Is it not slow if the CPU
already wrote over all the memory contiguously? I don't know.)

* Changelog addition (+ fixing duplicate documentation section)
  • Loading branch information
kpreid authored Sep 2, 2022
1 parent 7e84bb2 commit 4a7fc68
Show file tree
Hide file tree
Showing 2 changed files with 13 additions and 11 deletions.
16 changes: 7 additions & 9 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -95,21 +95,19 @@ the same every time it is rendered, we now warn if it is missing.
- Remove use of Vulkan12Features/Properties types. By @i509VCB in [#2936](https://github.com/gfx-rs/wgpu/pull/2936)
- Provide a means for `wgpu` users to access `vk::Queue` and the queue index. By @anlumo in [#2950](https://github.com/gfx-rs/wgpu/pull/2950)

### Performance

- Made `StagingBelt::write_buffer()` check more thoroughly for reusable memory; by @kpreid in [#2906](https://github.com/gfx-rs/wgpu/pull/2906)

### Documentation

#### General
- Add WGSL examples to complement existing examples written in GLSL by @norepimorphism in [#2888](https://github.com/gfx-rs/wgpu/pull/2888)
- Document `wgpu_core` resource allocation. @jimb in [#2973](https://github.com/gfx-rs/wgpu/pull/2973)

- Document `wgpu_core` resource allocation. @jimblandy in [#2973](https://github.com/gfx-rs/wgpu/pull/2973)
- Expanded `StagingBelt` documentation by @kpreid in [#2905](https://github.com/gfx-rs/wgpu/pull/2905)

- Fixed documentation for `Instance::create_surface_from_canvas` and
`Instance::create_surface_from_offscreen_canvas` regarding their
safety contract. These functions are not unsafe. [#2990](https://github.com/gfx-rs/wgpu/pull/2990)

### Performance

- Made `StagingBelt::write_buffer()` check more thoroughly for reusable memory; by @kpreid in [#2906](https://github.com/gfx-rs/wgpu/pull/2906)
safety contract. These functions are not unsafe. By @jimblandy [#2990](https://github.com/gfx-rs/wgpu/pull/2990)
- Document that `write_buffer_with()` is sound but unwise to read from by @kpreid in [#3006](https://github.com/gfx-rs/wgpu/pull/3006)

### Build Configuration

Expand Down
8 changes: 6 additions & 2 deletions wgpu/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3462,9 +3462,13 @@ impl Queue {
Context::queue_write_buffer(&*self.context, &self.id, &buffer.id, offset, data)
}

/// Schedule a data write into `buffer` starting at `offset` via the returned [QueueWriteBufferView].
/// Schedule a data write into `buffer` starting at `offset` via the returned
/// [QueueWriteBufferView].
///
/// The returned value can be dereferenced to a `&mut [u8]`; dereferencing it to a `&[u8]` panics!
/// The returned value can be dereferenced to a `&mut [u8]`; dereferencing it to a
/// `&[u8]` panics!
/// (It is not unsound to read through the `&mut [u8]` anyway, but doing so will not
/// yield the existing contents of `buffer` from the GPU, and it is likely to be slow.)
///
/// This method is intended to have low performance costs.
/// As such, the write is not immediately submitted, and instead enqueued
Expand Down

0 comments on commit 4a7fc68

Please sign in to comment.