-
Notifications
You must be signed in to change notification settings - Fork 1.1k
[deno] Fix some problems in the handling of device limits #8085
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
[deno] Fix some problems in the handling of device limits #8085
Conversation
ad10cae
to
9d2cb8d
Compare
9d2cb8d
to
b500ca9
Compare
b36e128
to
03bca7d
Compare
The
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM, though I have some questions I'd like to answer before merging.
wgpu-types/src/lib.rs
Outdated
/// This is not what you want to clamp a request that otherwise might be | ||
/// asking for something beyond the supported limits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Modulo maybe some formatting:
/// This is not what you want to clamp a request that otherwise might be | |
/// asking for something beyond the supported limits. | |
/// You should not use this method to clamp a request that otherwise might | |
/// request resources beyond the supported limits. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reworded to something different than your suggestion:
This function is not for clamping requests for values beyond the supported limits. For that purpose the desired function would be
or_worse_values_from
(which doesn't exist, but could be added if needed).
It may be that this comment is not generally useful. I tend to mix up min and max when I'm working quickly by writing max
when I mean "enforce a maximum value" (which needs min
). My motivation for the comment was that it's reasonable for somebody to think "a function to clamp at the maximum should exist", go looking for it, find only this function, and conclude this must be that function. Hence my use of the otherwise somewhat strange language "this is not what you want".
Hopefully my new text still addresses your concerns?
(I found the wording "should not use" strange, it's really "must not be used" or "don't use" -- it won't work. As for choice of better/worse/min/max, see #8084.)
CHANGELOG.md
Outdated
@@ -75,6 +75,7 @@ By @Vecvec in [#7913](https://github.com/gfx-rs/wgpu/pull/7913). | |||
- The function you pass to `Device::on_uncaptured_error()` must now implement `Sync` in addition to `Send`, and be wrapped in `Arc` instead of `Box`. | |||
In exchange for this, it is no longer possible for calling `wgpu` functions while in that callback to cause a deadlock (not that we encourage you to actually do that). | |||
By @kpreid in [#8011](https://github.com/gfx-rs/wgpu/pull/8011). | |||
- `wgpu` now requires that the requested device limits satisfy `min_subgroup_size <= max_subgroup_size`. By @andyleiserson in [#8085](https://github.com/gfx-rs/wgpu/pull/8085). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Does this not also apply to wgpu-core
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if it was on a different PR or I hallucinated it, but when I was revising I thought the suggestion was that specifying wgpu
here was redundant and not a good use of precious changelog verbiage. Based on that I reworded it to "The limits requested for a device must now satisfy...".
To answer the question that you actually asked here, it applies to either wgpu
or wgpu-core
. I think the new text adequately reflects that.
deno_webgpu/adapter.rs
Outdated
serde_json::from_value(serde_json::to_value(descriptor.required_limits)?)?; | ||
// When support for compatibility mode is added, this will need to look | ||
// at whether the adapter is "compatibility-defaulting" or "core-defaulting", | ||
// and choose the appropriate set of defaults. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Could we link this to an issue, so an inquisitive person can follow along with compat
development?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filed and linked #8124.
@@ -19,6 +19,7 @@ webgpu:api,operation,render_pass,storeOp:render_pass_store_op,color_attachment_w | |||
webgpu:api,operation,render_pass,storeOp:render_pass_store_op,color_attachment_only:* | |||
webgpu:api,operation,render_pass,storeOp:render_pass_store_op,multiple_color_attachments:* | |||
webgpu:api,operation,render_pass,storeOp:render_pass_store_op,depth_stencil_attachment_only:* | |||
fails-if(dx12) webgpu:api,validation,capability_checks,limits,maxBindGroups:setBindGroup,* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Thread-ifying this top-level comment:
The
limits,maxBindGroups
test is failing on dx12 with out-of-memory errors, which may be a similar symptom as the intermittents we have seen with these tests in Firefox.[fail] webgpu:api,validation,capability_checks,limits,maxBindGroups:setBindGroup,at_over:limitTest="atMaximum";testValueName="atLimit";encoderType="renderBundle" (60ms). Log: - EXCEPTION: OperationError: Not enough memory left. OperationError: Not enough memory left. at LimitTests.requestDeviceTracked (file:///D:/a/wgpu/wgpu/cts/out/common/framework/fixture.js:200:41) at LimitTests.requestDeviceWithLimits (file:///D:/a/wgpu/wgpu/cts/out/webgpu/api/validation/capability_checks/limits/limit_utils.js:447:19) at LimitTests._getDeviceWithSpecificLimit (file:///D:/a/wgpu/wgpu/cts/out/webgpu/api/validation/capability_checks/limits/limit_utils.js:493:31) at LimitTests._getDeviceWithRequestedMaximumLimit (file:///D:/a/wgpu/wgpu/cts/out/webgpu/api/validation/capability_checks/limits/limit_utils.js:556:17) at LimitTests.testDeviceWithRequestedMaximumLimits (file:///D:/a/wgpu/wgpu/cts/out/webgpu/api/validation/capability_checks/limits/limit_utils.js:640:40) at RunCaseSpecific.fn (file:///D:/a/wgpu/wgpu/cts/out/webgpu/api/validation/capability_checks/limits/maxBindGroups.spec.js:194:11)
Was this not already happening before? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test was not in the wgpu CTS test list prior to this PR. I haven't specifically verified, but I assume that it would fail in cts_runner on dx12 due to the same (deno-specific) functional failures that this PR is addressing.
The OOM problem I do not think is new, but it may or may not have been possible to see it in cts_runner, given the other problems.
Fix two problems with the handling of device limits:
TypeError
when a value was over the limit, but it should be anOperationError
.Testing
Enables some CTS tests.
Squash or Rebase? Squash
Checklist
cargo fmt
.taplo format
.cargo clippy --tests
. If applicable, add:--target wasm32-unknown-unknown
cargo xtask test
to run tests.CHANGELOG.md
entry.