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

Should we unwind the pledge to "lose the device" on device drop? #5049

Closed
bradwerth opened this issue Jan 11, 2024 Discussed in #5048 · 3 comments
Closed

Should we unwind the pledge to "lose the device" on device drop? #5049

bradwerth opened this issue Jan 11, 2024 Discussed in #5048 · 3 comments

Comments

@bradwerth
Copy link
Contributor

Discussed in #5048

Originally posted by bradwerth January 11, 2024
I've become convinced that the motivating case for the drop-must-lose-device is not strong enough to overcome the severe weirdness this could introduce. Behold!

let resurrectingPromise;
{
  const device = await adapter.requestDevice();
  resurrectingPromise = device.lost.then(() => {device.someNewProp = 0;});
}
await resurrectingPromise;

In such a case, either the device is never dropped and therefore the code hangs (confusing for the developer), or the device is dropped and the promise brings it temporarily back to life (crazy hard for the embedder to implement correctly). I don't think this is going anywhere good and the initial claim that we need this behavior just to support memory management for C-based user agents is not a strong enough motivator. I'm strongly tempted to unwind the changes made in #4862 and abandon the cleanup in #5032. Any objections?

@cwfitzgerald
Copy link
Member

I don't remember off the top of my head what the current state of the world is, but I don't think this is our decision to make- this is either a webgpu problem or a webgpu.h problem. I believe the ladder currently expects that the device lost call back is called on drop.

@nical
Copy link
Contributor

nical commented Jan 12, 2024

At the wgpu-core level we could decide to call the callback on drop, passing a status code that says "device dropped" (basically add DeviceLostReason::Dropped). In the browser we would be able to decide whether or not it needs to be forwarded to JS land (my understanding is that we would not, because we can't make the GC/CC observable).
At the wgpu level it's a mater of whether we want the behavior to be portable on the web, since the latter will not invoke the promise when the device is GC'ed.

I still think that at least the low level APIs should guarantee that the callback is called (with an appropriate status code), so that the memory management of the C callback and what it holds on to remains sane.

@bradwerth
Copy link
Contributor Author

bradwerth commented Jan 12, 2024

Alright, we'll keep wgpu behavior as-is ("lose the device" on device drop) and it will be up to user agents on how to handle that. Since drop may happen during Garbage Collection and/or Cycle Collection in some embeddings, it's up to the user agent on whether or how to expose the device lost promise in such a situation.

If there's a desire to add a new enum DeviceLostReason::Dropped let's open a separate issue for that. For now, all "lose the device" (including during device drop) is emitted as DeviceLostReason::Unknown.

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

No branches or pull requests

3 participants