-
Notifications
You must be signed in to change notification settings - Fork 931
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
Dynamic dispatch layer breaks on web #3430
Comments
Nice summary, had the same problems for now. expose-ids Feature is currently breaking the build.
-- Chrome+Linux+WebGPU is still broken, but software rendering with swiftshader is working again. Try to use |
Would it be possibile to keep the specific type inside the Identified and use the RefFromwasmAbi in order to not drop it in some way? |
It would be possible via having the dynamic dispatch layer downcast IDs into a ManuallyDrop and then get a reference from that. The other solution I have in mind is to use the Data assoicated types to store the JS objects and leave the id to just being the wasm repr (without taking out the object) + global id. |
I've upgraded to |
I was digging a bit into it yesterday, starting from what is happening on Surface, in order to see if I can implement this. |
So the Context trait defines a bunch of assoicated types. My idea was to no longer downcast from ObjectId (making it just a global id) and store the GPUTexture for example inside the TextureData assoicated type. The same with every other type that holds a web_sys::GPU type |
Is anyone actively working on this? It looks like it's blocking some projects using the WebGPU backend from switching to the new version. Should we consider backing out the changes for the WebGPU backend only, or trying to apply the solution mentioned above? |
I am also wondering on what is the progress of this issue? I am unable to test my WebGPU side of the project due to this issue. Or is there a easy work around? |
We might as well try what I proposed. Arcinzation on native might require the dynamic dispatch layer to change anyways. I've been incredibly busy however so I haven't had time to make the changes |
I wasn't working on it as I thought @i509VCB was working on it - I can prioritize it as it seems to be blocking. |
Hello! It would be great if this could get prioritized as our WASM build and demos haven't been functioning since we updated to 0.15 due to this issue. |
How is the progress in solving this, as it seems to completely break wgpu 0.15 on the web for me and others? Is there any suggested fix or workaround, or what is the plan for solving this issue? |
Stay on an older version seems to be the only possible solution for now. |
run-wasm
helper (cargo-run-wasm
) for testing the wgpu runner on the web.
EmbarkStudios/rust-gpu#1016
pub struct Buffer {
id: ObjectId,
/* ... snip ... */
}
Edit2: Figured out how to get past the iterator problem I was running into. Still working on filling in some other places now, but I have hello-triangle working again I'll keep updating https://github.com/grovesNL/wgpu/tree/dynamic-dispatch-workaround as I work through it and open a PR once it's in slightly better shape. |
I have a PR that should fix this at #3657 - would be great if some larger projects could test against my branch to see if I missed anything. I found it really easy to make mistakes doing the downcasting (would be great to improve this somehow), so it's possible there are some other paths that I didn't hit while testing. |
@grovesNL I just tested https://compute.toys/ against your branch and can confirm it resolves this issue (and didn't notice any breakages) |
I can confirm that this works with maplibre-rs |
I updated to cfc038a (codeart1st/wgpu-layers@2d425cb).
|
Is there any way of getting more of the stack? It cuts off right before the useful bit would be (seeing who is calling downcast_ref |
Is this an offscreen canvas? I wasn't able to test that path and might need a small example to reproduce it. |
I will try to reduce the code to get a better understanding about the problem. I thought the same about the offscreen canvas part, but the integration test with wasm-pack is a pure canvas, because we can't test in worker for now. https://github.com/codeart1st/wgpu-layers/blob/main/tests/utils.rs#L33 More of the stack:
If i don't rely on |
https://github.com/codeart1st/wgpu-wasm-dispatch Removed a lot of code from the original library. I used the following command to start the tests with chromedriver.
Will try it also on my Windows 11 PC in Chrome. Maybe Linux is not the best testing place at the moment. |
Today, I tried the wgpu-wasm-dispatch repo on Windows with Chrome Canary and got the same exception traces. |
Tried using OffscreenCanvas, still seeing an error.
Thanks for the work on this! |
I think the issue needs to be reopened or am I wrong? |
I think a new issue with a reproduction example is the way to go. This issue is fixed. |
Description
The web backend currently has an error like this when running the
hello-triangle
example usingcargo run-wasm --example=hello-triangle
in the browser console output:Now this is not an error that is caused by mismatching types. By inserting some debugging aid log statements we see the following console output before the panic:
What we see here is that the failure occurs on the second downcast of the GpuAdapter. The reason this occurs is because of how the web backend currently converts an
ObjectId
into whatever associated type is desired. In particular the web backend will use theObjectId
and convert the raw number right into whatever wasm_bindgen type was desired (such as GpuAdapter). Currently the web backend uses theIntoWasmAbi
andFromWasmAbi
traits to do this. However there is a less known side effect of this.FromWasmAbi
andIntoWasmAbi
transfer ownership of the JS object. When dropped, it will be destroyed on the JS side. Further use of the object when downcast again will of course fail since the actual object on the JS side was destroyed. It just happens to be caught easily by theis_instance_of
check with strict asserts.When running in release mode (to disable strict asserts) a different error shows up:
The solution to fix this would be to avoid taking ownership of the object when downcasting. This is easier said than done however. You need to make sure you destroy the object on drop to prevent memory leaks. The casting can be done, but we can only receive a
ManuallyOwned<T>
. And of course trying to return a reference to an object defined in a function is not allowed. This means there are one of two solutions left:Box<dyn Whatever>
This would be an easy solution, but will cause a definite increase in memory allocations. It might not be a huge concern on either web or native as the largest allocation size would essentially be a pointer or two. Profiling would make sense here, but of course best to avoid an allocation heavy pattern if possible in the first place.
Gpu*
objects on theXXXXData
associated types.This does mean the ObjectId on web will become effectively inert (unless the expose-ids) feature is enabled. This will also automatically deal with the concerns around memory leaks by downcasting.
This solution however calls into question whether the dynamic dispatch layer should even have separate
Data
andId
types (consolidating instead on aData
type only. But the currently native implementation uses someData
associated types and future out of tree backends could use both.Other notes
I tested this on firefox nightly (webgpu does not work on Linux at all on Chrome dev, no canary). The same error does appear in chrome dev even though the webgpy implementation on chrome is broken.
The text was updated successfully, but these errors were encountered: