More complete implementation of "lose the device". #4299
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The LoseDeviceClosure is called in two cases: 1) It is returned as one of the closures from `maintain` if `Device.destroy` has been called. This helps implement the spec for `destroy` that says that "lose the device" is the final step after the queues have emptied. 2) It is called directly by `Device.lose`. To accomplish this, `Device.valid` is set to false in `destroy`, which is not compliant with the spec but is necessary to ensure that no new work is enqueued after `destroy`. This means that such calls will return `DeviceError::InvalidDevice` Errs during this period, similarly to how they already do after "lose the device". Possibly calls during one or both of these periods should instant fail silently and return `Ok`, to be handled in a future patch.
In order to supply a DeviceLoseClosure, the test harness has to be given access to wgpu-core, so this patch modifies the Cargo files to do that. The new test confirms that the closure gets the expected value *if it is run*. As noted in a comment in the test, a more robust test that confirms the closure has been called requires the test function to become async, which imposes restrictions on how the test is run.
Mostly, this moves types into context.rs and cleans up callsites.
nical
reviewed
Nov 6, 2023
There was a problem hiding this comment.
I would prefer the terminology to match the spec more for API entry points:
LoseDeviceClosure->DeviceLostClosureset_lose_device_callback->set_device_lost_callback- etc.
The spec uses "lose the device" for descriptions of the steps to take but APIs use "lost".
With the exception of fn device_lose(...) which if I understand correctly is the internal entry point for marking that device was lost and isn't part of the public API so we can be a bit more free with the terminology, (or you could rename it into device_mark_lost).
|
I'm in some kind of merge hell here, trying to rebase back onto trunk, and I'm really tempted to resubmit as a new PR. If no objections in the next hours, I'll do that. |
|
Abandoned in favor of #4645. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Checklist
cargo clippy.cargo clippy --target wasm32-unknown-unknownif applicable.Connections
N/A
Description
This provides a way for wgpu-core to specify a callback on "lose the device". It ensures this callback is called at the appropriate times: either after
device.destroyhas empty queues, or on demand fromdevice.lose.Testing
A test has been added to device.rs.