Skip to content

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

Draft
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

SupaMaggie70Incorporated
Copy link
Contributor

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

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • If this contains user-facing changes, add a CHANGELOG.md entry.

@sagudev
Copy link
Collaborator

sagudev commented Jun 6, 2025

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

@SupaMaggie70Incorporated
Copy link
Contributor Author

@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
Copy link
Collaborator

sagudev commented Jun 6, 2025

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

@sagudev
Copy link
Collaborator

sagudev commented Jun 7, 2025

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.

@SupaMaggie70Incorporated
Copy link
Contributor Author

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.

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.

2 participants