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

Increase GL MAX_PUSH_CONSTANTS from 16 to 64 #3374

Merged
merged 2 commits into from
Jan 13, 2023

Conversation

Dinnerbone
Copy link
Contributor

@Dinnerbone Dinnerbone commented Jan 12, 2023

Checklist

  • Run cargo clippy.
  • Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
  • Add change to CHANGELOG.md. See simple instructions inside file.

Connections
None.

Description
I've found that using the emulated push constants is significantly faster than buffer juggling on our own on the webgl backend. However, it's limited to 16*4 bytes right now, which is too small for our needs.

I can't find any rationale behind it behind just 16*4, it's possible it was a naga limitation but it doesn't seem to be the case anymore? The docs for Limits::max_push_constant_size suggest it should be 256 on GLES so this increases it to that, which can hold a couple of matrices nicely.

If I'm wrong and there's a limitation I missed somewhere and this doesn't actually work somehow, then okay, nevermind! But I've tested this out on Ruffle and it seems to work perfect and performant for us.

Testing
Manually tested and it works fine, not sure it deserves its own unit test.

@codecov-commenter
Copy link

codecov-commenter commented Jan 12, 2023

Codecov Report

Merging #3374 (6738aff) into master (48fbb92) will decrease coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3374      +/-   ##
==========================================
- Coverage   64.70%   64.63%   -0.07%     
==========================================
  Files          66       86      +20     
  Lines       37336    42722    +5386     
==========================================
+ Hits        24159    27615    +3456     
- Misses      13177    15107    +1930     
Impacted Files Coverage Δ
wgpu-hal/src/gles/mod.rs 61.53% <ø> (ø)
wgpu-hal/src/gles/device.rs 81.40% <100.00%> (+1.32%) ⬆️
wgpu-core/src/hub.rs 60.98% <0.00%> (-3.60%) ⬇️
wgpu-core/src/instance.rs 65.67% <0.00%> (-2.04%) ⬇️
wgpu/src/lib.rs 51.06% <0.00%> (-0.28%) ⬇️
wgpu-hal/src/auxil/dxgi/exception.rs 87.30% <0.00%> (ø)
wgpu-hal/src/dx12/device.rs 88.45% <0.00%> (ø)
wgpu-hal/src/dx12/command.rs 73.46% <0.00%> (ø)
wgpu-hal/src/dx12/adapter.rs 81.38% <0.00%> (ø)
wgpu-hal/src/dx12/conv.rs 84.22% <0.00%> (ø)
... and 27 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@cwfitzgerald cwfitzgerald left a comment

Choose a reason for hiding this comment

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

LGTM, 256 is the standard push constant limit anyway

@cwfitzgerald
Copy link
Member

You have a test failing - but also we might have issues with the number of uniforms - we probably should at least be consulting the limits set by GL

@Dinnerbone
Copy link
Contributor Author

Darn, I couldn't test that here trivially. It looks like the test was set up assuming a size of 16 though, if I'm reading this right. Will need to find a way to adjust that.

@Dinnerbone
Copy link
Contributor Author

Actually, am I right in reading that this test wouldn't run if the push constants limit was less than 128? If so, this test was implicitly skipped until now and has never run for GLES. I can still find a way to make it work though!

.limits(Limits {
max_push_constant_size: MAX_BUFFER_SIZE as u32,
..Limits::downlevel_defaults()
}),

@cwfitzgerald
Copy link
Member

Heh, you are. If you can make it work great! Otherwise you can mark it as a failure and file a followup issue.

@Dinnerbone
Copy link
Contributor Author

Looked into this a bit, I'll mark it as failing and open an issue. In short:
The test makes a PushConstantRange(0..128) whilst only allocating (for example) vec2<f32> in the shader. When the render pass tries to use the push constants, in GL that means uploading uniforms and since there's only actually a few bytes for uniforms... that's an error.

It definitely didn't work before this PR, but it was ignored until now. I'm not sure how this is supposed to work though, how do other backends handle var<push_constant> input: CustomStruct; where struct CustomStruct { member: vec2<f32>, } whilst also allowing the user to say "I have 128 bytes", and not error out?

@cwfitzgerald
Copy link
Member

where struct CustomStruct { member: vec2, } whilst also allowing the user to say "I have 128 bytes", and not error out?

The shader is just the view into the push constants - you're allowed to push up more and only view part of it.

@cwfitzgerald cwfitzgerald enabled auto-merge (squash) January 13, 2023 00:17
@cwfitzgerald cwfitzgerald merged commit 4400ff8 into gfx-rs:master Jan 13, 2023
Dinnerbone added a commit to Dinnerbone/wgpu that referenced this pull request Jan 13, 2023
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.

3 participants