wgpu icon indicating copy to clipboard operation
wgpu copied to clipboard

Remove bufferslice lifetime

Open inner-daemons opened this issue 6 months ago • 5 comments

Connections Related issue: #6974

I'm also going to ping a few relevant people

  • @kpreid - I understand they have contributed to past BufferSlice designs
  • @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:

  1. Obviously, this would require calling clone() in many more places
  2. Also, it would probably require making other types like BufferBinding similarly lose their lifetimes (BufferBinding because it needs to be castable from BufferSlice, unless we refactor that). This would mean that in cases where people are creating bind groups, they have to do buffer.clone() rather than &buffer, which is maybe slightly worse ergonomically. Other types like BufferView 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 spec
  • The change is entirely in wgpu and not part of wgpu-core or wgpu-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] Run cargo xtask test to run tests.
  • [x] If this contains user-facing changes, add a CHANGELOG.md entry.

inner-daemons avatar Jun 06 '25 18:06 inner-daemons

I think it should be possible to keep lifetimes in BufferBinding<'a> thus avoiding clones.

sagudev avatar Jun 06 '25 19:06 sagudev

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

inner-daemons avatar Jun 06 '25 19:06 inner-daemons

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

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.

sagudev avatar Jun 06 '25 19:06 sagudev

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 avatar Jun 07 '25 05:06 sagudev

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.

inner-daemons avatar Jun 07 '25 16:06 inner-daemons

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.

sagudev avatar Aug 01 '25 13:08 sagudev

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

inner-daemons avatar Aug 04 '25 01:08 inner-daemons

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.

sagudev avatar Aug 04 '25 04:08 sagudev