Skip to content

Use unchecked shaders for better performance#17767

Merged
alice-i-cecile merged 3 commits into
bevyengine:mainfrom
JMS55:unchecked-shaders
Feb 12, 2025
Merged

Use unchecked shaders for better performance#17767
alice-i-cecile merged 3 commits into
bevyengine:mainfrom
JMS55:unchecked-shaders

Conversation

@JMS55
Copy link
Copy Markdown
Contributor

@JMS55 JMS55 commented Feb 10, 2025

Objective

  • Wgpu has some expensive code it injects into shaders to avoid the possibility of things like infinite loops. Generally our shaders are written by users who won't do this, so it just makes our shaders perform worse.

Solution

  • Turn off the checks.
    • We could try to conditionally keep them, but that complicates the code and 99.9% of users won't want this.

Migration Guide

@JMS55 JMS55 added this to the 0.16 milestone Feb 10, 2025
@JMS55 JMS55 added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times labels Feb 10, 2025
@JMS55 JMS55 requested review from IceSentry and cart February 10, 2025 00:37
@JMS55 JMS55 added the X-Contentious There are nontrivial implications that should be thought through label Feb 10, 2025
Comment thread crates/bevy_render/src/renderer/render_device.rs
@alice-i-cecile
Copy link
Copy Markdown
Member

I do think we should do this though. This is the right default for 99% of users. Unless you're making VRChat or a shader playground, you're not running random untrusted shaders.

I'd like to expose an option to enable validation for some shaders, but that needs a real user to inform us of the right design.

Copy link
Copy Markdown
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

I agree that this is unquestionably the better default. I won't push too hard for it in this PR (as this is better and I don't look a gift horse in the mouth), but making this configurable from our Shader type is low hanging fruit, and makes a very valuable and hard-to-implement-yourself feature available to people that want it.

Edit: I don't think doing that needs design work. Just make it an enum on Shader and expose the functionality via a new method on RenderDevice.

Copy link
Copy Markdown
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Eh, let's merge this now :)

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 10, 2025
@alice-i-cecile
Copy link
Copy Markdown
Member

@JMS55 CI is failing due to no safety comment (probably good!). I've taken my best crack at it, but please feel free to revise it as you see fit.

@alice-i-cecile alice-i-cecile added X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed X-Contentious There are nontrivial implications that should be thought through S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Feb 11, 2025
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
auto-merge was automatically disabled February 12, 2025 05:17

Head branch was pushed to by a user without write access

@JMS55
Copy link
Copy Markdown
Contributor Author

JMS55 commented Feb 12, 2025

@alice-i-cecile there's two usages of unsafe, you'll probably need to add the same comment to the second one to satisfy CI

Comment thread crates/bevy_render/src/renderer/render_device.rs
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Feb 12, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 12, 2025
Merged via the queue into bevyengine:main with commit 15b795d Feb 12, 2025
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-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 X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants