-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Remove bufferslice lifetime #7764
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
base: trunk
Are you sure you want to change the base?
Remove bufferslice lifetime #7764
Conversation
I think it should be possible to keep lifetimes in |
The existing |
Ah, I see it now what the problem is. The only thing I could come up is this: #[derive(Clone, Debug, PartialEq)]
pub struct BufferSlice2 {
pub(crate) buffer: Buffer,
pub(crate) offset: BufferAddress,
pub(crate) size: BufferSize,
}
impl<'a> From<&'a BufferSlice2> for crate::BufferBinding<'a> {
/// Convert a [`BufferSlice`] to an equivalent [`BufferBinding`],
/// provided that it will be used without a dynamic offset.
fn from(value: &'a BufferSlice2) -> Self {
BufferBinding {
buffer: &value.buffer,
offset: value.offset,
size: Some(value.size),
}
}
} but that you already discovered. Other option is for BufferBinding to have something like Cow, or maybe even better let bufferslice be something like Cow, but that was already proposed in #6974 (comment) I guess situation is now changed as buffer are clonable (or were there at that point too?). EDIT: We still have same problems, it's time for me to go to sleep as I am clearly not able to think anymore. |
We do not want that.
I think not, as you always need to have at least one variable alive (more specifically you need one buffer owner alive), either Buffer or BufferSlice, so I think |
@sagudev regarding the last point, BufferBinding could itself own Buffer so that no additional variables are needed. That's how it's implemented right now. Your approach is perhaps more sensible but it feels slightly less ergonomic somehow. I'd like to get another opinion or two before I completely settle on either approach though. |
Connections
Related issue: #6974
I'm also going to ping a few relevant people
BufferSlice
designsDescription
Currently, BufferSlice is generic over a lifetime, but the lifetime is only used for a reference to a buffer. Since buffers are now Cloneable, it make sense to abandon this and instead have BufferSlice "own" a
Buffer
. I only see 2 issues with this:clone()
in many more placesBufferBinding
similarly lose their lifetimes (BufferBinding
because it needs to be castable fromBufferSlice
, unless we refactor that). This would mean that in cases where people are creating bind groups, they have to dobuffer.clone()
rather than&buffer
, which is maybe slightly worse ergonomically. Other types likeBufferView
would also lose their lifetimes, though I think this is mostly for the better.In general, I'm of the opinion that we should avoid lifetimes in public APIs like the plague in most cases, and that this should outweigh the tiny impact of a few more clones and of .
I just wanna leave 2 more notes here to address (possible) concerns
BufferSlice
already isn't part of the WebGPU spec so this shouldn't cause worries about deviating from the specwgpu
and not part ofwgpu-core
orwgpu-types
. Since I understand most of the logic and heavy lifting occurs back there, the performance issues should be negligible.Testing
No need.
Squash or Rebase?
Probably should be squashed.
Checklist
cargo fmt
.taplo format
.cargo clippy --tests
. If applicable, add:--target wasm32-unknown-unknown
cargo xtask test
to run tests.CHANGELOG.md
entry.