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

Add BufferVec, an higher-performance alternative to StorageBuffer, and make GpuArrayBuffer use it. #13199

Merged
merged 10 commits into from
May 3, 2024

Conversation

kristoff3r
Copy link
Contributor

This is an adoption of #12670 plus some documentation fixes. See that PR for more details.


Changelog

  • Renamed BufferVec to RawBufferVec and added a new BufferVec type.

Migration Guide

BufferVec has been renamed to RawBufferVec and a new similar type has taken the BufferVec name.

pcwalton and others added 10 commits March 23, 2024 13:25
`StorageBuffer`, and make `GpuArrayBuffer` use it.

`EncasedBufferVec` is like `BufferVec`, but it doesn't require that the
type be `Pod`. Alternately, it's like `StorageBuffer<Vec<T>>`, except it
doesn't allow CPU access to the data after it's been pushed.
`GpuArrayBuffer` already doesn't allow CPU access to the data, so
switching it to use `EncasedBufferVec` doesn't regress any functionality
and offers higher performance.

Shutting off CPU access eliminates the need to copy to a scratch buffer,
which results in significantly higher performance. *Note that this needs
teoxoy/encase#65 from @james7132 to achieve
end-to-end performance benefits*, because `encase` is rather slow at
encoding data without that patch, swamping the benefits of avoiding the
copy. With that patch applied, and `#[inline]` added to `encase`'s
`derive` implementation of `write_into` on structs, this results in a
*16% overall speedup on `many_cubes --no-frustum-culling`*.

I've verified that the generated code is now close to optimal. The only
reasonable potential improvement that I see is to eliminate the zeroing
in `push`. This requires unsafe code, however, so I'd prefer to leave
that to a followup.
Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>
Co-authored-by: IceSentry <IceSentry@users.noreply.github.com>
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for adopting. I'm going to be cheeky and add this to the review queue early. It is basically just resolving some issues on an already-approved branch.

As noted by the migration guide, this is now a breaking change if it wasn't before.

@NthTensor NthTensor added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Needs-Release-Note Work that should be called out in the blog due to impact labels May 3, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 3, 2024
Merged via the queue into bevyengine:main with commit 2089a28 May 3, 2024
32 checks passed
@kristoff3r kristoff3r deleted the encased-buffer-vec branch May 3, 2024 11:55
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1310 if you'd like to help out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Needs-Release-Note Work that should be called out in the blog due to impact C-Performance A change motivated by improving speed, memory usage or compile times S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants