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

Support asynchronous WebAssembly compilation #21476

Closed
fabricedesre opened this issue Aug 22, 2018 · 33 comments · Fixed by #24653 or #24977
Closed

Support asynchronous WebAssembly compilation #21476

fabricedesre opened this issue Aug 22, 2018 · 33 comments · Fixed by #24653 or #24977
Labels
A-content/bindings The DOM bindings B-interesting-project Represents work that is expected to be interesting in some fashion C-assigned There is someone working on resolving the issue

Comments

@fabricedesre
Copy link
Contributor

This is a web assembly game demo.

STR:

Expected:

  • you have fun!

Observed:

  • You are sad: WebAssembly Promise APIs not supported in this runtime

I guess these are the streaming apis from https://webassembly.github.io/spec/web-api/index.html

@jdm
Copy link
Member

jdm commented Aug 22, 2018

This looks like something we need to configure properly to support: https://github.com/servo/mozjs/blob/9b44b1e870cae2d96ea41bf2aa8a7d5eaa4cdce7/mozjs/js/src/wasm/WasmJS.cpp#L2337-L2345

@jdm jdm added the A-content/bindings The DOM bindings label Aug 22, 2018
@jdm jdm added the B-interesting-project Represents work that is expected to be interesting in some fashion label Aug 22, 2018
@jdm jdm changed the title https://www.funkykarts.rocks/ demo doesn't work Support asynchronous WebAssembly compilation Sep 24, 2018
CYBAI added a commit to CYBAI/servo that referenced this issue Oct 16, 2018
CYBAI added a commit to CYBAI/servo that referenced this issue Oct 17, 2018
CYBAI added a commit to CYBAI/servo that referenced this issue Oct 18, 2018
CYBAI added a commit to CYBAI/servo that referenced this issue Oct 18, 2018
CYBAI added a commit to CYBAI/servo that referenced this issue Oct 18, 2018
@asajeffrey
Copy link
Member

Now happens in the tanks wasm demo. This used to work, did the demo get upgraded to use promises at some point?

$ ./mach run -d https://webassembly.org/demo
ERROR 2018-11-06T16:28:06Z: offscreen_gl_context::draw_buffer: The given GLContext doesn't support requested antialising
Loading blob blob:https://webassembly.org/dd95f4ece2b9411dbf9d56db995b10b1
Loading blob blob:https://webassembly.org/c7a0ecbc96f3434bb8d339fb0c6856fc
trying binaryen method: native-wasm
asynchronously preparing wasm
ERROR 2018-11-06T16:28:09Z: script::dom::bindings::error: Error at blob:https://webassembly.org/c7a0ecbc96f3434bb8d339fb0c6856fc:213:5 WebAssembly Promise APIs not supported in this runtime.
Invoking error handler due to
WebAssembly Promise APIs not supported in this runtime.
ALERT: An error occured running the Unity content on this page. See your browser JavaScript console for more info. The error was:
WebAssembly Promise APIs not supported in this runtime.

@jdm
Copy link
Member

jdm commented Nov 6, 2018

It did. It used to use the WebAssembly.compile API, iirc.

@jdm
Copy link
Member

jdm commented Sep 23, 2019

https://wasm.bootcss.com/demo/Tanks/ is the updated tanks game URL.

@jdm
Copy link
Member

jdm commented Oct 24, 2019

Based on https://github.com/servo/servo/wiki/Asynchronous-WebAssembly-compilation-project, from a private communication:

Regarding the first point in the initial step, we are not sure exactly where we need to create the StreamConsumer wrapper
Also can you confirm if we need to actually implement the consumeChunk, streamEnd or other functions present in the wrapper class or just define the virtual functions

Let's put all of the new code in components/script/script_runtime.rs in the Servo code for the short term. And there is no need to implement any functionality, only define methods for the new Rust struct that call these functions:

You can see some other uses of functions from that same glue module in that file based on

use js::glue::{CollectServoSizes, CreateJobQueue, DeleteJobQueue, JobQueueTraps, SetBuildId};
.

@jdm jdm added the C-assigned There is someone working on resolving the issue label Oct 24, 2019
@shubham-pampattiwar
Copy link

shubham-pampattiwar commented Oct 26, 2019

@jdm We wanted to know how to test our changes functionally, to make sure our changes are in accordance to the actual requirement.

@jdm
Copy link
Member

jdm commented Oct 26, 2019

@shubham-pampattiwar Unfortunately the code added in the initial steps is not expected to have any change in behaviour on its own. The best we can do right now is load https://wasm.bootcss.com/demo/Tanks/ and verify that there is no change in behaviour with your changes.

@apkp7
Copy link
Contributor

apkp7 commented Oct 28, 2019

@jdm we have raised a PR for a few changes related to this issue. We have completed a couple of tasks listed in the initial steps. However, we are not clear on the requirement of the second task,

create an extern "C" function in script_runtime.rs that matches ConsumeStreamCallback that initiates the streaming WebAssembly compilation then creates a wrapper for the stream consumer and stores it in the Response object

Despite having StreamConsumer as an argument in the ConsumeStreamCallback signature and the stated task requires us to create a new StreamConsumer wrapper and stores it in the Response object. Could you correct us if we are not interpreting it correctly?

We have come up with the following function signature:

#[allow(unsafe_code)]
unsafe extern "C" fn ConsumeStreamCallback(
    _cx: *mut JSContext,
    obj: HandleObject,
    mimeType: MimeType,
    consumer: *mut StreamConsumer,
) { }

@jdm
Copy link
Member

jdm commented Oct 28, 2019

The equivalent code in Firefox is a useful model to follow here. You will need to:

These changes should cause some of the automated tests in ./mach test-wpt tests/wpt/web-platform-tests/wasm/webapi to change behaviour, hopefully, since they test the error cases described in the specification.

bors-servo pushed a commit that referenced this issue Nov 3, 2019
…r=jdm

Binding stream consumer to support asynchronous WebAssembly compilation

This PR contains changes related to binding the Stream Consumer wrapper for supporting async WebAssembly compilation in Servo. The changes are listed below:

- Added a StreamConsumer wrapper and implemented its corresponding behaviors such as consumeChunk, streamEnd, streamError, and noteResponseURLs.
- Added ReportStreamErrorCallback for reporting the CompileError/TypeError occurred during the compilation.

---
- [X] `./mach build -d` does not report any errors, however, it has a few warnings of unused functions since we are calling them as yet.
- [X] `./mach test-tidy` does not report any errors
- [X] These changes are part of #21476 fix
@ridhimrastogi
Copy link
Contributor

@jdm Is the mimetype passed to the ConsumeStreamCallback the extracted mimeType from the response object. Although, this doesn't seem very likely. As per the firefox implementation the mimetype passed to the callback is first checked against the 'application/wasm' type and then compared against the response's mimetype. Are we expected to implement something similar here?

@jdm
Copy link
Member

jdm commented Nov 4, 2019

@ridhimrastogi The mimeType argument is the JS engine's way of saying "this is expected to be a WASM mime type (for example), please perform checks appropriately." Given #24628, I recommend getting response.Headers() and using the value of the Content-Type header as the MIME type to compare against.

@ridhimrastogi
Copy link
Contributor

@jdm Since root_from_handle_object returns a Root<Dom< Response>> object we can use the extract_mime_type method implemented in the ResponseMethods trait associated with the Response object. Is it necessary/possible to convert the Root<Dom< Response>> to net_trait implementation of Response done in servo for further processing.

@jdm
Copy link
Member

jdm commented Nov 4, 2019

Good find! It's not necessary to convert a Dom::Response to a net_traits::Response.

@ridhimrastogi
Copy link
Contributor

@jdm we have raised a PR for the issue. Regarding the next step as mentioned below:

create an instance of your new StreamConsumer type from #24563 and store that inside a member in the Response object (Option will be a good starting point)

Do we need to call the consume_chunk method defined in the StreamConsumerWrapper for step 2-6 as mentioned here.

@jdm
Copy link
Member

jdm commented Nov 4, 2019

consume_chunk is not required until the final step of "Subsequent steps".

bors-servo pushed a commit that referenced this issue Nov 11, 2019
…nitial-2, r=jdm

Add consume stream callback

<!-- Please describe your changes on the following line: -->
Added the consume stream callback function as per the steps mentioned [here](https://webassembly.github.io/spec/web-api/index.html#compile-a-potential-webassembly-response)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21476

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo pushed a commit that referenced this issue Nov 12, 2019
…nitial-2, r=jdm

Add consume stream callback

<!-- Please describe your changes on the following line: -->
Added the consume stream callback function as per the steps mentioned [here](https://webassembly.github.io/spec/web-api/index.html#compile-a-potential-webassembly-response)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21476

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo pushed a commit that referenced this issue Nov 12, 2019
…nitial-2, r=jdm

Add consume stream callback

<!-- Please describe your changes on the following line: -->
Added the consume stream callback function as per the steps mentioned [here](https://webassembly.github.io/spec/web-api/index.html#compile-a-potential-webassembly-response)

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21476

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@jdm
Copy link
Member

jdm commented Nov 12, 2019

This is only partially fixed so far.

@jdm jdm reopened this Nov 12, 2019
@apkp7
Copy link
Contributor

apkp7 commented Nov 15, 2019

@jdm We are a little unclear about the requirement of second step in binding runnable dispatching. Which new C function exactly is it refering to?

create rust JS::Runnable wrapper that stores a pointer to the object and has a run method that calls the new C function passing the stored pointer.

And for the third step, it asks us to use the network task source to queue a task (which returns Result object) that calls the run method of the runnable wrapper and at the same time, dispatch_to_event_loop_callback expects bool as the function return type.
Following is the stub I have come up with:

#[allow(unsafe_code)]
unsafe fn new_rt_and_cx_with_parent(
    parent: Option<ParentRuntime>,
    network_task_source: Option<*mut NetworkingTaskSource>,
) -> Runtime {
    let network_task_src = if let Some(source) = network_task_source {
        source as *mut c_void
    } else {
        ptr::null_mut()
    };

    unsafe extern "C" fn dispatch_to_event_loop(
        closure: *mut c_void,
        dispatchable: *mut Dispatchable,
    ) -> bool {
        let network_task_src: &NetworkingTaskSource = &*(closure as *mut NetworkingTaskSource);
        let runnable: &Runnable = &*(dispatchable as *mut Runnable);
        network_task_src.queue_unconditionally(runnable.run); // -> Result<(),()>
        false // -> expects bool
    }
    
    InitDispatchToEventLoop(cx, Some(dispatch_to_event_loop), network_task_src);
}

pub struct Runnable(*mut Runnable);
impl Runnable {
    fn run(&self) {}
}

Also, there are some modules that are calling new_rt_and_cx function so as we have added Option<*mut NetworkingTaskSource> as an fn argument, is it okay if we keep None for NetworkingTaskSource parameter for now?

@jdm
Copy link
Member

jdm commented Nov 15, 2019

Whoops, I previously rewrote some of the steps in the project description and missed the C function reference. It is referring to https://doc.servo.org/mozjs/glue/fn.DispatchableRun.html which wraps Dispatchable::Run from the C++ API.

As for the question about the networking task source, we will need to do a few things:

  • ensure that we have a NetworkingTaskSource object available before calling new_rt_and_cx in script_thread.rs and before calling new_child_runtime in dedicatedworkerglobalscope.rs (and pass that argument through to the call to new_rt_and_cx)
  • avoid calling InitDispatchToEventLoop when there is no networking task source available (like when we call new_rt_and_cx from serviceworkerglobalscope.rs and worklet.rs)

Rather than taking a pointer to a NetworkingTaskSource, we should accept it by value, then box it (Box::new) and call Box::into_raw (https://doc.rust-lang.org/stable/std/boxed/struct.Box.html#method.into_raw) to give us a pointer we can use as the closure argument.

As for interacting with the runnable, you will want to create an instance of your new Rust Runnable type that stores the pointer to the JS Runnable object, then create a task that invokes your run method on this Rust object. This may require adding unsafe impl Send for Runnable {} and/or unsafe impl Sync for Runnable {} if the compiler complains that unsafe pointers cannot be sent between threads.

Finally, you will want to return true according to this documentation.

@apkp7
Copy link
Contributor

apkp7 commented Nov 17, 2019

@jdm I was trying to create task for invoking run method, but getting this error that says can't capture dynamic environment in a fn item for accessing cx, even though I am using || { ... } closure form. Is there something I'm missing here. Adding source for your reference:

use js::jsapi::{Dispatchable as JSRunnable, Dispatchable_MaybeShuttingDown};

unsafe extern "C" fn dispatch_to_event_loop(
        closure: *mut c_void,
        dispatchable: *mut JSRunnable,
    ) -> bool {
        let networking_task_src: &NetworkingTaskSource = &*(closure as *mut NetworkingTaskSource);
        let runnable = Runnable(dispatchable);
        let task = task!(dispatch_to_event_loop_message: move || {
            let cx = cx.root(); // **Error**
            runnable.run(cx, Dispatchable_MaybeShuttingDown::NotShuttingDown);
        });

        match networking_task_src.queue_unconditionally(task) {
            Ok(q) => return true,
            Err(_) => return false,
        };
}

Runnable Definition:

pub struct Runnable(*mut JSRunnable);

#[allow(unsafe_code)]
unsafe impl Sync for Runnable {}
#[allow(unsafe_code)]
unsafe impl Send for Runnable {}

#[allow(unsafe_code)]
impl Runnable {
    fn run(&self, cx: *mut RawJSContext, maybe_shutting_down: Dispatchable_MaybeShuttingDown) {
        unsafe {
            DispatchableRun(cx, self.0, maybe_shutting_down);
        }
    }
}

Also, I am hard-coding Dispatchable_MaybeShuttingDown to NotShuttingDown to prevent the async task abortions. Can you confirm if it is the required behavior.

@jdm
Copy link
Member

jdm commented Nov 17, 2019

The problem you are facing is that cx is not a name that exists within the dispatch_to_event_loop function, only in the scope of the code in which the function is define. Instead, inside the task you can call Runtime::get() (note, this is mozjs::Runtime, not script_runtime::Runtime) to obtain the JSContext pointer that you need.

bors-servo pushed a commit that referenced this issue Nov 17, 2019
…t, r=<try>

Async wasm compilation subsequent

The PR contains changes related to binding the runnable dispatching in script_runtime and is part of the Asynchronous WebAssembly Compilation fix. This is the first step in the subsequent steps mentioned in the [wiki](https://github.com/servo/servo/wiki/Asynchronous-WebAssembly-compilation-project).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes are part of #21476 fix
bors-servo pushed a commit that referenced this issue Nov 20, 2019
…t, r=<try>

Async wasm compilation event loop integration

The PR contains changes related to binding the runnable dispatching in script_runtime and is part of the Asynchronous WebAssembly Compilation fix. This is the first step in the subsequent steps mentioned in the [wiki](https://github.com/servo/servo/wiki/Asynchronous-WebAssembly-compilation-project).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes are part of #21476 fix
bors-servo pushed a commit that referenced this issue Nov 20, 2019
…t, r=jdm

Async wasm compilation event loop integration

The PR contains changes related to binding the runnable dispatching in script_runtime and is part of the Asynchronous WebAssembly Compilation fix. This is the first step in the subsequent steps mentioned in the [wiki](https://github.com/servo/servo/wiki/Asynchronous-WebAssembly-compilation-project).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes are part of #21476 fix
bors-servo pushed a commit that referenced this issue Nov 20, 2019
…t, r=jdm

Async wasm compilation event loop integration

The PR contains changes related to binding the runnable dispatching in script_runtime and is part of the Asynchronous WebAssembly Compilation fix. This is the first step in the subsequent steps mentioned in the [wiki](https://github.com/servo/servo/wiki/Asynchronous-WebAssembly-compilation-project).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes are part of #21476 fix
bors-servo pushed a commit that referenced this issue Nov 20, 2019
…t, r=jdm

Async wasm compilation event loop integration

The PR contains changes related to binding the runnable dispatching in script_runtime and is part of the Asynchronous WebAssembly Compilation fix. This is the first step in the subsequent steps mentioned in the [wiki](https://github.com/servo/servo/wiki/Asynchronous-WebAssembly-compilation-project).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes are part of #21476 fix
bors-servo pushed a commit that referenced this issue Nov 20, 2019
…t, r=jdm

Async wasm compilation event loop integration

The PR contains changes related to binding the runnable dispatching in script_runtime and is part of the Asynchronous WebAssembly Compilation fix. This is the first step in the subsequent steps mentioned in the [wiki](https://github.com/servo/servo/wiki/Asynchronous-WebAssembly-compilation-project).

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes are part of #21476 fix
@ridhimrastogi
Copy link
Contributor

@jdm In the next step we have to store a StreamConsumer wrapper object in the Response. Does this imply creating a new Response object with the call to constructor and store StreamConsumer object in the body.

@jdm
Copy link
Member

jdm commented Nov 29, 2019

@ridhimrastogi No; there's is already a Response object available in consume_stream that will need to store the StreamConsumer.

@ridhimrastogi
Copy link
Contributor

@jdm Right now I have added this implementation in the consume_stream function that continues the steps 6 and 7 menioned in the Web assembly implementation.

let body_promise = unwrapped_source.ArrayBuffer();
        match body_promise.is_fulfilled() {
            true => {
                let mut stable_bytes = body_promise.resolve_native(&());
            },
            false => {
                body_promise.reject_native(&());
                throw_dom_exception(
                    cx,
                    &global,
                    Error::Type("There was an error consuming the Response".to_string()),
                );
                return false;
            },
        }

Here is what I am not sure of :

  1. How to add the StreamConsumer field to the unwrapped_source response.
  2. Response::finish() is already defined here. Should I overwrite it to include the streamend method in script_runtime.rs

@jdm
Copy link
Member

jdm commented Nov 29, 2019

With respect to step 6 and 7, you're going to be adding something that is equivalent but not exactly the same. We won't be using promises here, so that code you quoted won't help us, unfortunately.

  1. Add a new DomRefCell<Option<StreamConsumer>> member to Response in components/script/dom/response.rs, and add a public method that you can call that sets it to the provided argument.
  2. You should modify the implementation to check whether the Response contains a StreamConsumer value and call stream_end on it if it does.

bors-servo pushed a commit that referenced this issue Dec 3, 2019
Add StreamConsumer wrapper and methods to response

<!-- Please describe your changes on the following line: -->
Add Streamconsumer wrapper to Response

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21476
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo pushed a commit that referenced this issue Dec 3, 2019
Add StreamConsumer wrapper and methods to response

<!-- Please describe your changes on the following line: -->
Add Streamconsumer wrapper to Response

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes fix #21476
<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@jdm
Copy link
Member

jdm commented Dec 3, 2019

@ridhimrastogi Both https://www.funkykarts.rocks/demo.html and https://wasm.bootcss.com/demo/Tanks/ work with your group's changes! In an older build (like https://download.servo.org/nightly/mac/2019-09-16T11:09:48Z-servo-tech-demo.dmg) they don't run at all.

@ridhimrastogi
Copy link
Contributor

ridhimrastogi commented Dec 6, 2019

@jdm Can you provide a link to an older build for windows or linux.

@jdm
Copy link
Member

jdm commented Dec 6, 2019

Anything from http://servo-builds.s3.amazonaws.com/?list-type=2&prefix=nightly/mac/2019 (or replace mac with Linux, for example), and append the key name to download.servo.org like:

http://download.seevo.org/nightly/mac/2019-08-03T11:08:27Z-servo-tech-demo.dmg

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/bindings The DOM bindings B-interesting-project Represents work that is expected to be interesting in some fashion C-assigned There is someone working on resolving the issue
Projects
None yet
6 participants