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.

Sign up for GitHub

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

Jump to bottom

WASM, buffer_future panics after commit 32af4f5 in example hello-compute #2715

Closed
TimothyOlsson opened this issue Jun 2, 2022 · 5 comments · Fixed by #2807
Closed

WASM, buffer_future panics after commit 32af4f5 in example hello-compute #2715

TimothyOlsson opened this issue Jun 2, 2022 · 5 comments · Fixed by #2807
Assignees
@cwfitzgerald
Labels
api: webgpu Issues with direct interface with WebGPU area: correctness We're behaving incorrectly type: bug Something isn't working
Milestone
Release 0.13

Comments

@TimothyOlsson
Copy link

Description

I have some issues when running webgpu for wasm32-unknown-unknown after the commit 32af4f5. In commit 8063edc, it works fine

I tested the "wgpu/examples/hello-compute/main.rs"

// Note that we're not calling `.await` here.
let buffer_slice = staging_buffer.slice(..);
// Sets the buffer up for mapping, sending over the result of the mapping back to us when it is finished.
let (sender, receiver) = futures_intrusive::channel::shared::oneshot_channel();
buffer_slice.map_async(wgpu::MapMode::Read, move |v| sender.send(v).unwrap());

// Poll the device in a blocking manner so that our future resolves.
// In an actual application, `device.poll(...)` should
// be called in an event loop or on another thread.
device.poll(wgpu::Maintain::Wait);

// Awaits until `buffer_future` can be read from
if let Some(Ok(())) = receiver.receive().await {  // <-- Fails here, row 160. Returns a None
   ...
} else {
    panic!("failed to run compute on gpu!")
}

Repro steps

Go to wgpu directory

(in windows, in unix remove "set")
set RUSTFLAGS=--cfg=web_sys_unstable_apis

Run the commands
cargo build --no-default-features --target wasm32-unknown-unknown --example hello-compute
wasm-bindgen --out-dir target/generated --web ../target/wasm32-unknown-unknown/debug/examples/hello-compute.wasm

change getObject(arg0).dispatch in target/generated/hello-compute.js

imports.wbg.__wbg_dispatch_b68689d8b6913579 = function(arg0, arg1, arg2, arg3) {
    getObject(arg0).dispatch(arg1 >>> 0, arg2 >>> 0, arg3 >>> 0);
};

to

imports.wbg.__wbg_dispatch_b68689d8b6913579 = function(arg0, arg1, arg2, arg3) {
    getObject(arg0).dispatchWorkgroups(arg1 >>> 0, arg2 >>> 0, arg3 >>> 0);
};

Not sure if it is an issue with the "wrong" dispatch comes from wasm-bindgen or wgpu? Otherwise, it yields the error Uncaught (in promise) TypeError: getObject(...).dispatch is not a function.

Create the file target/generated/index.html and add the following:

<!DOCTYPE html>
<html>
  <head>
    <meta charset="UTF-8" />
    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
  </head>
  <body>
    <script type="module">
      import init from "./hello-compute.js"; // Change this file to the name of the example you are using
      init();
    </script>
  </body>
</html>

Run the command (requires installation)
simple-http-server target/generated

open localhost:8000

open the console (f12) and check output

Expected vs observed behavior

Example should pass / compute in both commits

Extra materials

Fail commit 32af4f5
fail_32af4f56079fd2203c46c9c452cfe33fd60a5721

Ok commit 8063edc
ok_8063edc6482cfc828895ec0feaa446767fecc510

Platform

Windows version 10.0.19044 build 19044
cargo 1.60.0 (d1fd9fe2c 2022-03-01)
rustc 1.60.0 (7737e0b5c 2022-04-04)
wasm-bindgen 0.2.80
Firefox nightly 103.0a1 (2022-05-31) (64-bit) with webgpu enabled
@TimothyOlsson TimothyOlsson changed the title WASM panics after commit 32af4f5 WASM, buffer_future panics after commit 32af4f5 in example hello-compute Jun 2, 2022
@cwfitzgerald cwfitzgerald added type: bug Something isn't working area: correctness We're behaving incorrectly labels Jun 2, 2022
@cwfitzgerald
Copy link
Member

Hmm, there's something up with the plumbing, the sender is being dropped without being called.

@cwfitzgerald cwfitzgerald added this to the Release 0.13 milestone Jun 2, 2022
@cwfitzgerald cwfitzgerald self-assigned this Jun 2, 2022
@cwfitzgerald cwfitzgerald added the api: webgpu Issues with direct interface with WebGPU label Jun 4, 2022
@TimothyOlsson
Copy link
Author

Hi again,

I tested some more and found a fix for the issue, but I'm not sure it is what we are looking for. I'll explain why further down in the comment.

In the following file: \wgpu\src\backend\web.rs at row 1753, function buffer_map_async

let rc_callback_clone = rc_callback.clone();
let closure_success = wasm_bindgen::closure::Closure::once(move |_| {
    rc_callback.borrow_mut().take().unwrap()(Ok(()))
});
let closure_rejected = wasm_bindgen::closure::Closure::once(move |_| {
    rc_callback_clone.borrow_mut().take().unwrap()(Err(crate::BufferAsyncError))
});

let _ = map_promise.then2(&closure_success, &closure_rejected);

// Adding these two lines makes it work on wasm
closure_success.forget();
closure_rejected.forget();

working

The issue with this fix is that it seems that the "forget" call causes memory-leaks:
https://stackoverflow.com/questions/63640557/clousure-invocation-error-when-creating-a-callback-with-a-clousure-on-mouse-inpu

An workaround is found here, but it is from 2018 and it briefly describes that "weak-refs" would solve this issue:
https://stackoverflow.com/questions/53214434/how-to-return-a-rust-closure-to-javascript-via-webassembly/53219594#53219594

This seems to be what the above link was about. However, it requires the flag "--weak-refs" to be enabled for wasm-bindgen cli:
https://rustwasm.github.io/wasm-bindgen/reference/weak-references.html

wasm-pack seems to not have the flag for "--weak-refs":
rustwasm/wasm-pack#930

Perhaps you have found a better way to resolve this issue, without using the "forget" call?

Also, any findings about the issue which requires the line getObject(arg0).dispatch(arg1 >>> 0, arg2 >>> 0, arg3 >>> 0); to be changed to getObject(arg0).dispatchWorkgroups(arg1 >>> 0, arg2 >>> 0, arg3 >>> 0); in target/generated/hello-compute.js? Should I create a new issue for that one?

Best regards,
Timothy

@cwfitzgerald
Copy link
Member

Also, any findings about the issue which requires the line getObject(arg0).dispatch(arg1 >>> 0, arg2 >>> 0, arg3 >>> 0); to be changed to getObject(arg0).dispatchWorkgroups(arg1 >>> 0, arg2 >>> 0, arg3 >>> 0); in target/generated/hello-compute.js? Should I create a new issue for that one?

web_sys is just out of date, see #2795

Bleh that's annoying. https://docs.rs/wasm-bindgen/latest/wasm_bindgen/closure/struct.Closure.html#method.once_into_js though seems to support turning FnOnce functions into closures without leaking what they contain. However because we share resources with a function that doesn't get called, we should add a call to https://doc.rust-lang.org/std/rc/struct.Rc.html#method.decrement_strong_count to cause the Rc to fully die.

@cwfitzgerald cwfitzgerald mentioned this issue Jun 25, 2022
Merged
@cwfitzgerald
Copy link
Member

@TimothyBergstrom could you try out #2807? I don't have a WebGPU browser handy.

@TimothyOlsson
Copy link
Author

Yes, it seems to work fine with the fix

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@cwfitzgerald cwfitzgerald

Labels
api: webgpu Issues with direct interface with WebGPU area: correctness We're behaving incorrectly type: bug Something isn't working
Projects
None yet
Milestone
Release 0.13
Development

Successfully merging a pull request may close this issue.

Solve crash on WebGPU cwfitzgerald/wgpu
2 participants
@cwfitzgerald @TimothyOlsson