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

Don't create shader-clear program on GLES if it's not needed #5348

Merged
merged 2 commits into from
Mar 8, 2024

Conversation

Dinnerbone
Copy link
Contributor

@Dinnerbone Dinnerbone commented Mar 5, 2024

Connections
n/a

Description
We've noticed this before in Ruffle, that the built in "shader clear program" can contribute to the startup times on webgl. We've seen each shader program take as much as 100ms to compile before, and they really add up.

wgpu doesn't actually need this in most cases though, so it's wasteful to spend the time creating it. This reduces it down to an Option, that's only populated if the workaround flag is enabled.

I was contemplating removing Workarounds.MESA_I915_SRGB_SHADER_CLEAR and having it simply be "this value is Some() if needed, None if not", which feels much more natural - but the workaround is stored per Adapter and the program per Queue right now, so I opted to leave it*. I'm also unsure if wgpu prefers to have these things listed as clear workarounds instead, as I do see the merit there.

*though this did leave me to question: why are we making so many resources per Queue on gles, rather than Adapter? If you opened up two queues you have two shader clear programs, two zero buffers, two draw fbos, etc. I feel like we can move those to Adapter and save some potential waste, in a future PR.

Testing
It passes tests and seems to run fine!

Checklist

  • Run cargo fmt.
  • Run cargo clippy. If applicable, add:
    • --target wasm32-unknown-unknown
    • --target wasm32-unknown-emscripten
  • Run cargo xtask test to run tests.
  • Add change to CHANGELOG.md. See simple instructions inside file.

@Dinnerbone Dinnerbone requested a review from a team as a code owner March 5, 2024 21:52
@Dinnerbone Dinnerbone force-pushed the gles_clear_program branch from 720e24a to ec82ac0 Compare March 5, 2024 21:53
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

Thanks for the fix, looks good to me!

I agree with both of your points - the program should live on the adapter and the workaround should be incorporated by the optional clear program itself, saving us the except down the line (the Option should be then appropriately named to indicate what workaround it is for).

The only reason I can think of why this doesn't live on the adapter is that it might be to protect against creating the shader alongside with the adapter itself which might be unnecessary overhead when creating the adapter just to probe for available adapters. So ideally we'd actually fully lazily create the shader clear program upon first usage instead.

Would be nice if you or someone else could look into all this in a follow-up :), but I think this PR is a clear improvement, so in it goes!

@Wumpf Wumpf merged commit f86898f into gfx-rs:trunk Mar 8, 2024
27 checks passed
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