Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Document that write_buffer_with is sound to read from. #3006

Merged
merged 2 commits into from
Sep 2, 2022

Conversation

kpreid
Copy link
Contributor

@kpreid kpreid commented Sep 1, 2022

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Description

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.

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.)

@kpreid kpreid marked this pull request as draft September 1, 2022 01:53
@kpreid kpreid marked this pull request as ready for review September 1, 2022 01:58
Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@cwfitzgerald
Copy link
Member

As for:

it suggests that it might be a bad idea to write data into the view and then sort it in-place

Currently it will be fast, but we reserve the right (and we might do it) to make it terribly slow

@kpreid
Copy link
Contributor Author

kpreid commented Sep 1, 2022

Currently it will be fast, but we reserve the right (and we might do it) to make it terribly slow

Another question that comes to mind is whether this also applies to buffers created as mapped_at_creation. If so, that is worth documenting too. In my own thinking about perf optimization, I've thought about doing (as I mentioned in the description) sorting or other read-write operations inside of newly allocated buffers or StagingBelt views, on the principle of minimizing copies.

wgpu/src/lib.rs Outdated Show resolved Hide resolved
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.)
@jimblandy jimblandy enabled auto-merge (squash) September 2, 2022 23:34
@jimblandy jimblandy merged commit 4a7fc68 into gfx-rs:master Sep 2, 2022
@kpreid kpreid deleted the writedoc branch December 12, 2022 03:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants