8000 Remove the greater-or-equal constraint on maxBindingsPerBindGroup by nical · Pull Request #4484 · gpuweb/gpuweb · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Remove the greater-or-equal constraint on maxBindingsPerBindGroup #4484

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Remove the greater-or-equal constraint on maxBindingsPerBindGroup #4484

wants to merge 1 commit into from

Conversation

nical
Copy link
@nical nical commented Feb 6, 2024

This constraint is problematic for configurations where the theoretical maximum number of bindings is very large. For example on this linux desktop I get the following limits:

Max Sampled Textures Per Shader Stage: 1000000
Max Samplers Per Shader Stage: 1000000
Max Storage Buffers Per Shader Stage: 1000000
Max Storage Textures Per Shader Stage: 1000000
Max Uniform Buffers Per Shader Stage: 1000000

Which would mean maxBindingsPerBindGroup has to be very high.

However, the intent behind maxBindingsPerBindGroup is to restrict the maximum binding index to "allows implementations to treat binding space as an array, within reasonable memory space, rather than a sparse map structure." Which is the case of the vulkan backend in Firefox.

So high theoretical maximum binding limits should not force the maximum binding index to be raised to very large number and this formula in the spec should be removed.

There is also a CTS test webgpu:api,validation,capability_checks,limits,maxBindingsPerBindGroup:validate: that should be removed for the same reasons.

@github-actions GitHub Actions
Copy link
Contributor
github-actions bot commented Feb 6, 2024

Previews, as seen when this build job started (8c11d71):
WebGPU webgpu.idl | Explainer | Correspondence Reference
WGSL grammar.js | wgsl.lalr.txt

@kainino0x
Copy link
Contributor

Changing this probably makes sense (we wrote it conservatively to start).
But I think it still never makes sense for any of these 5 limits to be > maxBindingsPerBindGroup, and would add that as a constraint.

That would require Firefox to choose if it wants to:

  • lower the per-stage limits, e.g.: per-stage limits = 1000, maxBindingsPerBindGroup = 1000 (instead of 10K)
  • raise the maxBindingsPerBindGroup, e.g.: per-stage limits = 1M, maxBindingsPerBindGroup = 1M (instead of 10M)

@kainino0x kainino0x added this to the Milestone 1 milestone Feb 6, 2024
@nical
Copy link
Author
nical commented Feb 7, 2024

Sounds good to me. Should the new constraint be (for each Thing) max[Thing]PerShaderStage <= maxBindingsPerBindGroup or max[Thing]PerShaderStage <= maxBindingsPerBindGroup x maxBindGroups ?

@kainino0x
Copy link
Contributor

Sorry for the slow reply, I had a hard time figuring out the answer. Editors (@jimblandy @toji) just discussed it in our meeting, and @jimblandy pointed out that if maxSampledTexturesPerShaderStage is 1M, it implies that, ignoring maxBindingsPerBindGroup, you can bind 1M textures to a single bind group as there are no additional limits on that. So it makes a bit more sense not to further restrict that, so require max[Thing]PerShaderStage <= maxBindingsPerBindGroup.

But I thought about this a bit more after the meeting, while writing this down. The original hope was that maxBindingsPerBindGroup would never restrict how much you can bind, only the maximum @binding() index you can use. If we want to preserve that property, then the current spec text is correct already.

Practically speaking, right now it's totally impractical to use 1M bindings because we do not have binding arrays (#822), so perhaps we should hold onto this restriction at least until that issue is being resolved. It may not even really be relevant until bindless (#380) is being resolved? I have a vague recollection that super high limits in Vulkan are only really usable with bindless. But I know very little about how bindless works so I'm not sure.

In the meantime, implementations would need to pick a balance of lowering the PerShaderStage limits and raising maxBindingsPerBindGroup. I believe it's also possible to raise maxBindingsPerBindGroup to infinity by repacking sparse bindings to be contiguous.

@nical
Copy link
Author
nical commented Feb 13, 2024

Alright, I preferred your earlier suggestion but in the short term Firefox will restrict max[Thing]PerShaderStage to small values, since it's simple and should not impact users too much.

It may not even really be relevant until bindless (#380) is being resolved? I have a vague recollection that super high limits in Vulkan are only really usable with bindless. But I know very little about how bindless works so I'm not sure.

Let's have this discussion again during or after the resolution of a bindless API, then.

I believe it's also possible to raise maxBindingsPerBindGroup to infinity by repacking sparse bindings to be contiguous.

There is a desire on the wgpu side to keep the vulkan backend simple since the binding model aligns well with WebGPU, instead of renaming the bindings which IIRC requires deferring the creation of shader modules to pipeline creation (granted we already do it on at least one other backend, I know that Dawn does it for all its backends). Just passing that one CTS test isn't motivation enough to change that and I don't think the spec should make renaming bindings mandatory without stronger motivating factors.

@kainino0x kainino0x added the api WebGPU API label May 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
api WebGPU API proposal
Projects
None yet
Milestone
Milestone 1
Development

Successfully merging this pull request may close these issues.
2 participants
@nical @kainino0x
0