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

Queue::write_buffer_with #1670

Closed
cart opened this issue Jul 16, 2021 · 11 comments · Fixed by #2777
Closed

Queue::write_buffer_with #1670

cart opened this issue Jul 16, 2021 · 11 comments · Fixed by #2777
Labels
area: api Issues related to API surface help required We need community help to make this happen. type: enhancement New feature or request

Comments

@cart
Copy link
Collaborator

cart commented Jul 16, 2021

Is your feature request related to a problem? Please describe.

Currently I'm using the Queue to write an array of Std140-convertible data to a Buffer (using Queue::write_buffer).

This takes the following form:

  1. Allocate a Vec of Std140 convertible data
  2. Allocate a Vec<u8> as "scratch space" to write the final Std140-formatted bytes to
  3. Pass the Vec<u8> as a reference to Queue::write_buffer (which accepts &[u8])

Under the hood, I know that Queue::write_buffer is writing to a memory-mapped staging buffer internally. It occurred to me that the intermediate Vec allocation shouldn't be necessary. Instead I could just write the "converted std140 bytes" directly to the memory-mapped staging buffer.

Describe the solution you'd like

It would be nice to have something like:

fn write_buffer_with(&self, buffer: &Buffer, size: BufferAddress, func: impl FnMut(&mut [u8])) {};

This would reserve the requested amount of space in the staging buffer and expose a slice that I could then pass directly into the std140::Writer.

Describe alternatives you've considered

Use a staging buffer manually (this is what i used to do). This requires more machinery because we need manage the staging buffer, call copy_buffer_to_buffer on a CommandEncoder, and then manually submit the encoder. I personally like the ergonomics of Queue::write_buffer.

@cwfitzgerald
Copy link
Member

I've actually had a very similar thought and I support having an API like this where you can see directly into the staging buffer. I do caution against the callback based API though. I did that in my library wgpu-conveyor and I ended up in a nasty situation where I actually wanted to map multiple buffers at the same time (as I was looping and writing the results to multiple buffers) and to do that it would nest in a crazy way very quickly. I ended up having to write some helpers to make it possible, which got pretty ridiculous.

The implementation might not be so easy, but I always wanted an api which worked like buffer mapping

fn write_buffer_with(&self, buffer: &Buffer, offset: BufferAddress size: BufferAddress) -> QueueWriteBufferView

I'm not sure if having a separate view type is necissary, but we might need custom unmap code.

@cwfitzgerald cwfitzgerald added area: api Issues related to API surface help required We need community help to make this happen. type: enhancement New feature or request labels Jul 16, 2021
@kvark
Copy link
Member

kvark commented Jul 16, 2021

We can expose something like this, but we aren't going to be able to do anything smart on the Web for it. Would you be happy using this API on native-only?

@cwfitzgerald
Copy link
Member

cwfitzgerald commented Jul 16, 2021

In my use case I think it would be preferable to have write_buffer_with to fall back on a regular dumb queue.write_buffer. You can't really do any better than that as a user anyway.

@cart
Copy link
Collaborator Author

cart commented Jul 16, 2021

Agreed!

@teoxoy
Copy link
Member

teoxoy commented Mar 9, 2022

I'm trying to implement this however I'm unsure how it should be implemented at the level of wgpu-core. I see there are no complex types being returned/passed in as args most likely due to the API being FFI compatible. I don't have much experience with FFI and would like some guidance on how this should be implemented.

What I've done so far is:

Added the write_buffer_with fn with the folowing signature as per @cwfitzgerald comment

fn write_buffer_with(&self, buffer: &Buffer, offset: BufferAddress, size: BufferSize) -> QueueWriteBufferView

where QueueWriteBufferView is a struct that dereferences to a [u8].

Implemented it for the web backend by creating a temporary Vec<u8> and essentially calling write_buffer on drop.

Started to implement it for the direct backend but unsure how to proceed due to potential API FFI compatibility issues.

The 2 ways I can think of achieving it are:

  • same way it is implemented for the web backend (method returns view that on drop issues a write) - potential FFI incompatibility
  • have 2 functions (one creating the staging buffer and the 2nd one accepting the buffer id and writing) - potentially more error prone, have to keep track of the buffer

On another note, do we want to issue the write on drop or should we require the user to call a method (flush) on the view?

@jimblandy
Copy link
Member

Tentatively:

The overall principle is that wgpu-core is the target for non-Rust language bindings, and wgpu is the idiomatic, Rustic API. So the Drop-driven approach makes sense at the wgpu level. At the wgpu-core level, I think we have to have two separate functions. Error-prone, yes, but it's error-prone because C is error-prone. We're just accommodating reality.

As to whether the flush should be implicit on drop, or explicit: I guess I'd want to try using the API and see what it's like. It makes me a little uncomfortable to imagine saying, "this action which is the whole point of all this activity occurs... here at this closing bracket, where no code is written." If people of the more careful persuasion end up writing comments at the end of blocks, like

{
    let mut buf = queue.write_buffer_with(...);
    ...
    // buffer is written here
}

or perhaps

{
    ...
    drop(buf);
}

just to keep things straight, then I don't think the API is being helpful. Having an explicit flush or submit method that takes self by value seems best.

@jimblandy
Copy link
Member

(@teoxoy Thanks for working on this!)

@jimblandy
Copy link
Member

jimblandy commented Apr 4, 2022

The nice thing about a callback-based API is that the type system can make sure you've dropped all the references to the buffer before you submit. What about an API like this?

queue.scope(|s| {
    let mut bytes1 = s.get_mut(&mut buffer1, 16);
    let mut bytes2 = s.get_mut(&mut buffer2, 256);
    // ... write to bytes1 and bytes2 ...
});

That way, you can write as many buffers as you want, you can have Scope::get_mut just hand out &mut [u8] values, &mut buffer1 ensures you don't have overlapping writes, and if you get the lifetimes right (imitating crossbeam::scope) you can ensure that all the borrows are done by the time scope returns, so they're safe for the next Queue::submit call.

(This is my attempt to address @cwfitzgerald's concern about awkward nesting and helper functions.)

@jimblandy
Copy link
Member

In chat @cwfitzgerald points out that you can have another thread do a submit in the midst of this, so "preventing" the borrows from overlapping with a submit isn't going to be practical.

What might work better is if the writes are collected in the scope object, and they're included in the first submit after scope returns. So other threads doing submissions would have no effect on the writes to buffer1 and buffer2, but once the call to queue.scope returns, the next submit will pick up the writes.

@kvark
Copy link
Member

kvark commented Apr 4, 2022

I think drop-based approach would be more consistent with the rest of our API. And in general the idea is sound.

@teoxoy
Copy link
Member

teoxoy commented Jun 15, 2022

@jimblandy Thanks for your thoughts! I went with a drop-based approach for now. I'm not sure if I understand the benefits of the scope-callback approach; the current approach for write_buffer/write_buffer_with uses internal staging buffers + a copy_buffer_to_buffer. Overlapping writes can happen with write_buffer as well and I think it's a non-issue since they would be enqueued and executed sequentially.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api Issues related to API surface help required We need community help to make this happen. type: enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants