Remove bufferslice lifetime
Connections Related issue: #6974
I'm also going to ping a few relevant people
- @kpreid - I understand they have contributed to past
BufferSlicedesigns - @sagudev - They have voiced support for this change in the matrix chat
Description
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:
- Obviously, this would require calling
clone()in many more places - Also, it would probably require making other types like
BufferBindingsimilarly lose their lifetimes (BufferBindingbecause 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 likeBufferViewwould 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
BufferSlicealready isn't part of the WebGPU spec so this shouldn't cause worries about deviating from the spec- The change is entirely in
wgpuand not part ofwgpu-coreorwgpu-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
- [x] Run
cargo fmt. - [x] Run
taplo format. - [x] Run
cargo clippy --tests. If applicable, add:- [x]
--target wasm32-unknown-unknown
- [x]
- [x] Run
cargo xtask testto run tests. - [x] If this contains user-facing changes, add a
CHANGELOG.mdentry.
I think it should be possible to keep lifetimes in BufferBinding<'a> thus avoiding clones.
@sagudev
I think it should be possible to keep lifetimes in
BufferBinding<'a>thus avoiding clones.
The existing BufferBinding takes a reference to a buffer, hence if we kept the lifetime we couldn't still implement From<BufferSlice>. We might instead have to keep a reference to the BufferSlice itself(another ergonomics issue as the user would then have to keep another variable I think). If you have an idea I'm missing, let me know.
@sagudev
I think it should be possible to keep lifetimes in
BufferBinding<'a>thus avoiding clones.The existing
BufferBindingtakes a reference to a buffer, hence if we kept the lifetime we couldn't still implementFrom<BufferSlice>. We might instead have to keep a reference to the BufferSlice itself(another ergonomics issue as the user would then have to keep another variable I think). If you have an idea I'm missing, let me know.
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 https://github.com/gfx-rs/wgpu/issues/6974#issuecomment-2672058696 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.
Other option is for BufferBinding to have something like Cow
We do not want that.
We might instead have to keep a reference to the BufferSlice itself(another ergonomics issue as the user would then have to keep another variable I think)
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 impl<'a> From<&'a BufferSlice> for crate::BufferBinding<'a> is the way to go.
Other option is for BufferBinding to have something like Cow
We do not want that.
We might instead have to keep a reference to the BufferSlice itself(another ergonomics issue as the user would then have to keep another variable I think)
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
impl<'a> From<&'a BufferSlice> for crate::BufferBinding<'a>is the way to go.
@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.
What was reason for closing this? Because I will be wiling to pursue this:
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
impl<'a> From<&'a BufferSlice> for crate::BufferBinding<'a>is the way to go.
What was reason for closing this? Because I will be wiling to pursue this:
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
impl<'a> From<&'a BufferSlice> for crate::BufferBinding<'a>is the way to go.
My approach was wrong, if you want to pursue this go ahead! I just am not interested enough to do that right now
What was reason for closing this? Because I will be wiling to pursue this:
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
impl<'a> From<&'a BufferSlice> for crate::BufferBinding<'a>is the way to go.My approach was wrong, if you want to pursue this go ahead! I just am not interested enough to do that right now
Opened #8046.