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

Use Queue instead of StagingBelt for buffer/texture writes. #69

Closed
wants to merge 6 commits into from

Conversation

profan
Copy link

@profan profan commented Jun 8, 2021

I was having a situation where StagingBelt was leaking a tremendous amount of memory (like 10MB/second given my use of wgpu_glyph, so I took the liberty of swapping out StagingBelt use for Queue), it seems you may already have had the same idea given the comment inside cache.rs?

I also fixed my issue with the cache being corrupted (it was me leaving in half the buffer use that wasn't necessary anymore, order of writes to queue vs encoder use wasn't what I expected), the only thing which might be a little weird is the now local Vec kept in cache.rs to hold the padded data and the use of unsafe there for set_len, but here it is anyways, ready for review 👀

@Mubelotix
Copy link

This is a great idea but aren't StagingBelt supposed to be more efficient? Did you open an issue?

@profan
Copy link
Author

profan commented Jul 5, 2021

This is a great idea but aren't StagingBelt supposed to be more efficient? Did you open an issue?

StagingBelt currently leaks a dramatic amount of memory and from what I've heard from some of the people working on wgpu-rs side things, it seems while it technically does avoid a copy, it's easier API-side to "do the right" thing apparently efficiency wise if you just use queue's write_buffer directly?

That said.. no I haven't opened an issue but this exists: gfx-rs/wgpu#1441 which is just kind of like.. unresolved, also referenced in the iced repo here: iced-rs/iced#786 (comment)

I'd assumed given this comment: https://github.com/hecrj/wgpu_glyph/pull/69/files#diff-7bd5d211a8e8c645d47359cecc7cd50ee00aebd67c8a2e8e864ee5ef9e8b458eL95

... that moving to use Queue was the end goal anyways, but it's up to hecrj in the end yes, I just made this change for myself largely because it was simpler than using StagingBelt/going to try and supply an official fix for StagingBelt

Copy link
Owner

@hecrj hecrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main issue with using Queue::write_buffer is that we lose composability.

With a StagingBelt, writes are recorded in the CommandEncoder. With a Queue, writes are global and happen all at once during submit. This means that the buffers will be overwritten and only contain the data of the last draw_queued call.

For instance, the clipping example is most likely broken here.

@profan
Copy link
Author

profan commented Jul 5, 2021

The main issue with using Queue::write_buffer is that we lose composability.

With a StagingBelt, writes are recorded in the CommandEncoder. With a Queue, writes are global and happen all at once during submit. This means that the buffers will be overwritten and only contain the data of the last draw_queued call.

For instance, the clipping example is most likely broken here.

Oh yeah, you're right, the expected ordering does fall apart basically, oof.

Hmm, maybe fixing StagingBelt is the easier solution in this case then actually...

@schell
Copy link

schell commented Jul 29, 2021

Just a note because I just figured this out - since these changes now use Queue::write_texture, padding the data to multiples of COPY_BYTES_PER_ROW_ALIGNMENT is not necessary and so padded_cache can disappear.

https://docs.rs/wgpu/0.9.0/wgpu/constant.COPY_BYTES_PER_ROW_ALIGNMENT.html

@profan
Copy link
Author

profan commented Jul 29, 2021

Just a note because I just figured this out - since these changes now use Queue::write_texture, padding the data to multiples of COPY_BYTES_PER_ROW_ALIGNMENT is not necessary and so padded_cache can disappear.

https://docs.rs/wgpu/0.9.0/wgpu/constant.COPY_BYTES_PER_ROW_ALIGNMENT.html

Good catch, changed it, simplifies things a bit, things are still bent as write_texture/write_buffer don't compose the same way the queued encoder operations do with StagingBelt, but I might have a stab at making that work some time soon to make stagingbelt unnecessary

@hecrj hecrj added help wanted Extra attention is needed enhancement New feature or request labels Aug 19, 2021
@hecrj
Copy link
Owner

hecrj commented Jan 20, 2022

Closing in favor of #76.

@hecrj hecrj closed this Jan 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants