-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Allow timeouting on poll #8282
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
Allow timeouting on poll #8282
Conversation
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.
LGTM, minus a seemingly accidental CHANGELOG
entry. Approving and trusting this will be fixed before merge.
timeout: Option<Duration>, | ||
) -> Result<bool, crate::DeviceError> { | ||
let timeout_duration = Duration::from_millis(timeout_ms as u64); | ||
let timeout = timeout.unwrap_or(Duration::MAX); |
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.
If None
means the same as Duration::MAX
, why pass duration by Option
at all?
From just the function declaration it is far from obvious that None
means MAX
. It could just as well mean "Use the default timeout"
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.
none of the implementations do this right now, but often there's simpler fast/path for no timeout. I think for the low-level hal api this is fine like that, but I am missing docstring on the trait this implements
Connections
Description
Introduces
wgpu::PollType::WaitWithTimeout
/wgpu::PollType::WaitForSubmissionIndexWithTimeout
variants to allow specifying a timeout on polls. This is useful for instance for image comparision tests or similar in a CI environment!Decided to expose this as new enum variants since this is the least intrusive for current code and least bothersome if one doesn't care about timeouts here.
Went with
Duration
all the way since it's most expressive and doesn't require unit documentation/assumptions.A soft-breaking change that comes along with this is that the non-timeout waits no longer have a hardcoded timeout of 60s. Note that the constant that set this was misdocumented at this point: our timeouting on device drop is a lot more sophisticated by now and there's no more UB involved in any of this!
Testing
Added test, some coverage by existing
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.