TY-1637 Add bindgen for wasm/js #51

Robert-Steiner · 2021-04-21T15:12:32Z

first implementation of wasm bindings

Prerequisites

wasm-pack
rustup target add wasm32-unknown-unknown

Build

wasm-pack build

Test

wasm-pack test --node

you can also run cargo expand ai if you are interested to see what wasm-bindgen does on the rust side

Run it in the browser

ln -s $(pwd)/../data/ $(pwd)/data
wasm-pack build --target web --release
python3 -m http.server

Profiling

with wasm-pack build --target web --profiling you can create an optimised build with debug-info turned on that will allow you to profile the execution of the wasm module using the build-in browser developer tools

Chrome

Firefox

`catch_unwind`

Catching panics is not possible at the current stage of development. The default panic strategy for wasm32-unknown-unknown is PanicStrategy::Abort but it also seems that it is the only possible value.

I have tested it with panic="unwind" and panic="abort" and the result was the same. The closure in catch_unwind was never executed.

So what will happens in case of a panic?

In case of a panic, the process aborts, which causes a trap in WASM which in turn leads to a WebAssembly.RuntimeError which can be caught on the JS side with try\catch.

Summarized:

Rust Panic -> Abort -> WASM trap -> WASM RuntimeError -> catch on JS side via try\catch

It is important to know that we can continue to use the WASM module, however it might be faulty.

Link

dont-panic

What can we still do?

we can't catch panics at the moment but we can at least print the stack trace to the console by using console_error_panic_hook. Without it, we only get a runtime error which does not contain any useful information.

More Links

Error handling

In the event of a panic, a WebAssembly.RuntimeError is thrown, which can be caught using a try/catch on the JS side. (Not sure if we can do something similar in Dart)

try {
	ai = new WXaynAi(vocab_buffer, model_buffer, undefined);
}
catch (e) {
	if (e instanceof WebAssembly.RuntimeError) {
		// Panic
	} else {
		...
	}
}

I used wasm-tracing-allocator to check for memory leaks after a panic and yes, there is memory that has not been freed. As suggested in we should in this case recreate the wasm instance, however, I couldn't find any resources describing how to achieve this. Also I couldn't find anything about how to handle a WebAssembly.RuntimeError in general. (I'll ask in the rust discord)

In any case, calling ai.free() in the catch block doesn’t seem to be the solution:

Uncaught (in promise) Error: recursive use of an object detected which would lead to unsafe aliasing in rust
    at imports.wbg.__wbindgen_throw (xayn_ai_ffi_wasm.js:357)
    at wasm_bindgen::throw_str::h0f1c6b79f7e0b42b (:8000/pkg/xayn_ai_ffi_wasm_bg.wasm:wasm-function[6172]:0x47f1de)
    at wasm_bindgen::__rt::borrow_fail::h9eaf8de49d649f35 (:8000/pkg/xayn_ai_ffi_wasm_bg.wasm:wasm-function[6171]:0x47f1d3)
    at __wbg_wxaynai_free (:8000/pkg/xayn_ai_ffi_wasm_bg.wasm:wasm-function[381]:0x1f4104)
    at WXaynAi.free (xayn_ai_ffi_wasm.js:175)
    at run ((index):39)

Nonetheless, I added console_error_panic_hook so that we can at least see the stack trace in the console.

Besides the WebAssembly.RuntimeError we have to handle our error type (ExternError). It is automatically deserialized into a JS object, so we can directly access the code and message fields.

{
	code: 4, 
	message: "Failed to initialize the ai: Failed to build the m…decode Protobuf message: unexpected end group tag"
}

TODO:

tests
documentation

some useful links:

acrrd · 2021-04-22T07:50:45Z

Regarding error types would be nice to have a type similar to ExternalError that looks like { code: i32, msg: String } and then convert that to a JsValue. We can later also try to have CCode in common between the two so that we can share all the conversion code in dart.

Robert-Steiner · 2021-04-22T08:26:16Z

Regarding error types would be nice to have a type similar to ExternalError that looks like { code: i32, msg: String } and then convert that to a JsValue. We can later also try to have CCode in common between the two so that we can share all the conversion code in dart.

good idea, I will take a look today what we can do

janpetschexain

this looks much slimmer than the c ffi 😀

currently the functions are stand-alone, if i understood correctly we can even put them into a plain old impl WXaynAi {} block and even refer to &self as usual (with some additional wasm-bindgen attribute usage).

xayn-ai-ffi-wasm/src/utils.rs

xayn-ai-ffi-wasm/src/ai.rs

Robert-Steiner · 2021-04-22T17:24:42Z

this looks much slimmer than the c ffi 😀

currently the functions are stand-alone, if i understood correctly we can even put them into a plain old impl WXaynAi {} block and even refer to &self as usual (with some additional wasm-bindgen attribute usage).

Good point👍 I moved them into the impl block of WXaynAi

Robert-Steiner · 2021-04-22T17:28:33Z

Regarding error types would be nice to have a type similar to ExternalError that looks like { code: i32, msg: String } and then convert that to a JsValue. We can later also try to have CCode in common between the two so that we can share all the conversion code in dart.

What we can do is serialising ExternError into a js object and deserialise it on the dart side into a XaynAiException. This way we could reuse the CCode + some of the dart error code.

acrrd · 2021-04-23T08:46:04Z

What we can do is serialising ExternError into a js object and deserialise it on the dart side into a XaynAiException. This way we could reuse the CCode + some of the dart error code.

I'm not sure about this. ExternError has a pointer, so we will need to serialize it and then manually release the memory. Maybe we can have a diffferent version ExternError in the wasm crate that use a String. Not sure how much code we can reuse at the moment apart from the error code because with the C ffi we need to call free, while with wasm the data that we return will be owned by dart.

acrrd · 2021-04-23T08:48:58Z

this looks much slimmer than the c ffi grinning
currently the functions are stand-alone, if i understood correctly we can even put them into a plain old impl WXaynAi {} block and even refer to &self as usual (with some additional wasm-bindgen attribute usage).

Good point+1 I moved them into the impl block of WXaynAi

Please check if this will create a javascript object and methods on it instead of functions. Would be nice to have the same way of calling the functions from dart for the ffi so that in the future we can try to unify some code and hopefully have an abstract ffi layer that the rest of lib can use.

Robert-Steiner · 2021-04-26T08:14:31Z

What we can do is serialising ExternError into a js object and deserialise it on the dart side into a XaynAiException. This way we could reuse the CCode + some of the dart error code.

I'm not sure about this. ExternError has a pointer, so we will need to serialize it and then manually release the memory. Maybe we can have a diffferent version ExternError in the wasm crate that use a String. Not sure how much code we can reuse at the moment apart from the error code because with the C ffi we need to call free, while with wasm the data that we return will be owned by dart.

Sorry, I wan't clear here. I didn't mean the ffi_support::ExternError, but a structure in wasm that has the same name like you wrote here

Maybe we can have a diffferent version ExternError in the wasm crate that use a String

#[derive(Serialize)]
pub struct ExternError {
   code: i32,
   message: String,
}

Not sure how much code we can reuse at the moment apart from the error code because with the C ffi we need to call free, while with wasm the data that we return will be owned by dart.

true but my hope is that we can (in wasm) return an ExternError and deserialize it into a XaynAiException

Robert-Steiner · 2021-04-26T12:59:00Z

Please check if this will create a javascript object and methods on it instead of functions.

It indeed creates a JS object but i think it's worth it because it makes the api and code simpler.

Would be nice to have the same way of calling the functions from dart for the ffi so that in the future we can try to unify some code and hopefully have an abstract ffi layer that the rest of lib can use.

I'm not sure if this would bring us any advantages if the wasm API is similar to the c ffi, since in wasm we don't have to pass the external error, but instead can use the result type. The Wasm API is more similar to the API from CXaynAi. But I'm open to change it if you want.

xayn-ai-ffi-wasm/src/error.rs

janpetschexain · 2021-04-28T11:33:36Z

xayn-ai-ffi-wasm/data

@@ -0,0 +1 @@
+/Users/robert/projects/xayn-ai/xayn-ai-ffi-wasm/../data/


even though this is a symlink file, it seems to resolve to your local machine. do you know if this is just a git artifact or do we need to set up the symlink differently?

when i try to follow the symlink locally, after checking out the branch on my machine, it says that the path is invalid, which probably hints at an incorrect symlink setup.

I've removed it. I added it to commit by mistake. I included the command to generate the symlink in the readme

sorry, this wasn't meant to be included in the review, i saw that you made changes to this file while i was reviewing the pr and i thought my comment was removed.

is saw that you temporarily used relative paths in get_data("../../data/etc"), it seems this didn't work out?

It is possible, but we then have to start the web server in the root of the repo.
I added a git-friendly symlink to the repo like @acrrd suggested :)

xayn-ai-ffi-wasm/src/ai.rs

janpetschexain · 2021-04-28T12:39:01Z

xayn-ai-ffi-wasm/src/ai.rs

+    }
+
+    // #[wasm_bindgen_test]
+    // fn test_model_invalid() {


i guess we could set this test up similarly to test_vocab_invalid(), couldn't we?

yes but it does not fail if we pass an empty array :/
Should we create a ticket to fix this?

that's definitely worth a ticket. i guess it might produce some "default empty" model in tract, which we probably don't want to allow in our pipeline. in the end we need more tests for the builder in xayn-ai.

janpetschexain · 2021-04-28T12:47:19Z

xayn-ai-ffi-wasm/example/index.html

+        }
+        catch (e) {
+            if (e instanceof WebAssembly.RuntimeError) {
+                // Panic


since you said we can't use ai.free() in here, it seems to me that the only safe way to ensure that no memory is leaked on a panic, is to run this code in a separate thread/isolate and to kill it on panic. but this is quite coarse, maybe we can find a more sensitve solution, i'm not sure though given the current state of development of wasm.

The part isn't clear to me either. I'm not sure if it is fine just to recreate the wasm model via await init(); or if we actually need to reload the website. Maybe it's worth creating a ticket and diving deeper into this topic.

is to run this code in a separate thread/isolate and to kill it on panic.

I'm not sure if a dart isolate will produce something similar like an isolate on the js side. I think dart will just look for the function name in the window object and call that if it exists.

https://github.com/xaynetwork/xayn_search/blob/ca352dca3ca5a89c26b4bc8a8b0dc4eac4cfe130/xayn_web/web/index.html#L40-L44

But I could be wrong

Maybe we can utilise web workers for that.

Darts isolates are basically modeled after WebWorkers, partially because google initially intended it to be a language focused on transpilling to JS (by now it's more complex, but I think dart in browser still uses WebWorkers for Isolates).

Furthermore due to computations taking time the apps anyway run it in their own isolate (and with that thread) and the browser app should probably run it in a WebWorker (which it probably will do anyway as it runs it in a dart isolate transpilled to js I think).

But we probably don't want to "fully" kill it, instead it would be enough to kill it's "state" but not it's "code" instance (to reuse JITed code etc.). (Sorry I don't remember the proper WebAssembly terms.)

So I also think that it's best to open an issue and investigate it later one.

Especially I don't know the state of "moving" WebAssembly instances into isolates, we might need to make sure we only initiate it inside of the isolate/WebWorker, or it might just work 🤷.

In the integration on mobile if a panic is thrown the isolate is shout down and recreated, we can have the same thing on wasm and WebWorks. On wasm is even more important to avoid memory leak because a web page can live long, while the app is closed more frequently. The app create us already in the isolate so don't have to care about that.

Yes.

Through it turns out dart doesn't automatically map Isolates to WebWorkers when sharing a code base
(through there are libraries which abstract over it).

Relevant parts would be:

https://api.dart.dev/stable/2.12.4/dart-html/Worker-class.html

Through if panics are not rare it should be better to clear state, e.g. by reruning WebAssembly.instantiate(...) but not rerun the compilation or worse download step. But that would be an optimization either way.

Robert-Steiner · 2021-04-28T13:09:08Z

regarding "running tests in the browser", we just need to put wasm_bindgen_test::wasm_bindgen_test_configure!(run_in_browser); in the #[cfg(test)] module. The downside is that we can no longer run the tests in node. Only one of the two options is possible. All tests are passing in firefox, chrome and safari. (For safari i had to enable the WebDriver support via safaridriver --enable)

But i think running the tests in node is enough right now, wdyt?

acrrd · 2021-04-28T13:27:10Z

Since we target a browser would be nice to have them running in a browser, especially if we can target all the main one; so if we can run them on the ci on the browser I'm voting for that.
If we want both I think a feature flag would be enough to enable the option in the code.

Do you know how reading the model is handled? was that automatic or you needed to do something?

Robert-Steiner · 2021-04-28T15:13:05Z

Since we target a browser would be nice to have them running in a browser, especially if we can target all the main one; so if we can run them on the ci on the browser I'm voting for that.
If we want both I think a feature flag would be enough to enable the option in the code.

Alright I will change it

Do you know how reading the model is handled? was that automatic or you needed to do something?

The vocab + model are embedded in the wasm model via include_bytes!. I haven't found a better solution :(

acrrd · 2021-04-28T18:28:45Z

Since we target a browser would be nice to have them running in a browser, especially if we can target all the main one; so if we can run them on the ci on the browser I'm voting for that.
If we want both I think a feature flag would be enough to enable the option in the code.

Alright I will change it

Do you think that we will gain something to test also on node?

Do you know how reading the model is handled? was that automatic or you needed to do something?

The vocab + model are embedded in the wasm model via include_bytes!. I haven't found a better solution :(

It's ok, it is only in test. It could be a good general solution if we don't want to have the app downloading the models in the future to reduce install size.

janpetschexain

lgtm 👍

janpetschexain · 2021-04-29T13:50:58Z

xayn-ai-ffi-wasm/src/ai.rs

+
+#[wasm_bindgen]
+impl WXaynAi {
+    #[wasm_bindgen(constructor)]


i think our current order of attributes is to first have the doc attribute and below that all other code related attributes.

janpetschexain · 2021-04-29T14:11:21Z

xayn-ai-ffi-wasm/src/ai.rs

+    pub fn new(
+        vocab: &[u8],
+        model: &[u8],
+        serialized: Option<Box<[u8]>>,


wherever we take a Box<T> or a Vec<T> in this api, it transfers ownership and as the docs state this thing has to be allocated properly before. depending on where the allocation is performed we could replace some of the owned arguments with references, but i guess it makes sense to revisit that case by case later on.

xayn-ai-ffi-wasm/src/ai.rs

acrrd · 2021-04-29T15:01:22Z

xayn-ai-ffi-wasm/README.md

+## Running the example
+
+```shell
+ln -s $(pwd)/../data/ $(pwd)/example/data


We can have a relative symlink to data already in the repo.

acrrd · 2021-04-29T15:12:21Z

xayn-ai-ffi-wasm/src/ai.rs

+    }
+}
+
+#[cfg(target_arch = "wasm32")]


We can group the two in one line

Suggested change

#[cfg(target_arch = "wasm32")]

#[cfg(test, target_arch = "wasm32")]

xayn-ai-ffi-wasm/src/error.rs

xayn-ai-ffi-wasm/src/utils.rs

Co-authored-by: janpetschexain <[email protected]>

Co-authored-by: Andrea Corradi <[email protected]>

acrrd · 2021-04-30T11:03:43Z

xayn-ai-ffi-wasm/README.md

@@ -0,0 +1,40 @@
+# Xayn AI WASM


We can move this in the README in root of the project. In this or in a future PR. There there are already the info about android and ios there.

good idea 👍

janpetschexain mentioned this pull request Apr 22, 2021

TY-1759 move serialized bytes allocation from dart to rust #52

Merged

janpetschexain reviewed Apr 22, 2021

View reviewed changes

Robert-Steiner force-pushed the TY-1637-wasm-bindgen branch from e741d4a to 15a8546 Compare April 26, 2021 08:06

Robert-Steiner force-pushed the TY-1637-wasm-bindgen branch 2 times, most recently from 0d7ec57 to a5ad9ed Compare April 28, 2021 12:36

janpetschexain reviewed Apr 28, 2021

View reviewed changes

Robert-Steiner force-pushed the TY-1637-wasm-bindgen branch from a5ad9ed to 5f88cc0 Compare April 28, 2021 15:42

Robert-Steiner marked this pull request as ready for review April 29, 2021 13:12

janpetschexain approved these changes Apr 29, 2021

View reviewed changes

acrrd self-requested a review April 29, 2021 14:52

acrrd reviewed Apr 29, 2021

View reviewed changes

acrrd self-requested a review April 29, 2021 18:54

acrrd approved these changes Apr 29, 2021

View reviewed changes

Robert-Steiner and others added 5 commits April 30, 2021 11:37

wasm poc

b15f9fd

browser example

066aadc

better profiling options [skip ci]

07ce160

add comment [skip ci]

1c09040

Update xayn-ai-ffi-wasm/src/utils.rs [skip ci]

977e704

Co-authored-by: janpetschexain <[email protected]>

Robert-Steiner and others added 22 commits April 30, 2021 11:38

Update xayn-ai-ffi-wasm/src/utils.rs [skip ci]

e2f2ecb

Co-authored-by: janpetschexain <[email protected]>

Update xayn-ai-ffi-wasm/src/ai.rs [skip ci]

93ab4a3

Co-authored-by: janpetschexain <[email protected]>

Update xayn-ai-ffi-wasm/src/ai.rs [skip ci]

375e762

Co-authored-by: janpetschexain <[email protected]>

move fn into impl block

3495121

wip

ddcd7fa

reworked error handling

8d97fc2

add console panic hook

6881edd

test analytics

c8b808c

add tests

b52e54c

move index.html into example folder

493cda2

run tests in browser by default

16f7c49

simplify tests

9486ac1

fmt html

c8a97c1

enable test

53ee28d

expect instead of unwrap

3c93959

Update xayn-ai-ffi-wasm/src/ai.rs [skip ci]

1ff3733

Co-authored-by: janpetschexain <[email protected]>

clean up

1735e49

Update xayn-ai-ffi-wasm/src/error.rs [skip ci]

c004f0f

Co-authored-by: Andrea Corradi <[email protected]>

Update xayn-ai-ffi-wasm/src/error.rs [skip ci]

f146d68

Co-authored-by: Andrea Corradi <[email protected]>

Update xayn-ai-ffi-wasm/src/utils.rs [skip ci]

3ebb854

Co-authored-by: Andrea Corradi <[email protected]>

add symlink in repo

55b9e9e

make cfg one-liner

7b8db24

Robert-Steiner force-pushed the TY-1637-wasm-bindgen branch from d146afc to 7b8db24 Compare April 30, 2021 09:39

add semicolon

11b33f8

acrrd reviewed Apr 30, 2021

View reviewed changes

move content to root readme

9379068

Robert-Steiner force-pushed the TY-1637-wasm-bindgen branch from 92265af to 9379068 Compare April 30, 2021 12:56

Robert-Steiner merged commit 7292b36 into master Apr 30, 2021

Robert-Steiner deleted the TY-1637-wasm-bindgen branch April 30, 2021 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TY-1637 Add bindgen for wasm/js #51

TY-1637 Add bindgen for wasm/js #51

Robert-Steiner commented Apr 21, 2021 •

edited

Loading

acrrd commented Apr 22, 2021

Robert-Steiner commented Apr 22, 2021

janpetschexain left a comment •

edited

Loading

Robert-Steiner commented Apr 22, 2021

Robert-Steiner commented Apr 22, 2021

acrrd commented Apr 23, 2021 •

edited

Loading

acrrd commented Apr 23, 2021

Robert-Steiner commented Apr 26, 2021

Robert-Steiner commented Apr 26, 2021

janpetschexain Apr 28, 2021

Robert-Steiner Apr 28, 2021 •

edited

Loading

janpetschexain Apr 29, 2021

Robert-Steiner Apr 30, 2021

janpetschexain Apr 28, 2021

Robert-Steiner Apr 29, 2021

janpetschexain Apr 29, 2021

janpetschexain Apr 28, 2021

Robert-Steiner Apr 29, 2021 •

edited

Loading

rustonaut Apr 29, 2021 •

edited

Loading

acrrd Apr 29, 2021

rustonaut Apr 29, 2021 •

edited

Loading

Robert-Steiner commented Apr 28, 2021

acrrd commented Apr 28, 2021

Robert-Steiner commented Apr 28, 2021 •

edited

Loading

acrrd commented Apr 28, 2021

janpetschexain left a comment

janpetschexain Apr 29, 2021

janpetschexain Apr 29, 2021

acrrd Apr 29, 2021

acrrd Apr 29, 2021

acrrd Apr 30, 2021

Robert-Steiner Apr 30, 2021

		@@ -0,0 +1 @@
		/Users/robert/projects/xayn-ai/xayn-ai-ffi-wasm/../data/

	#[cfg(target_arch = "wasm32")]
	#[cfg(test, target_arch = "wasm32")]

TY-1637 Add bindgen for wasm/js #51

TY-1637 Add bindgen for wasm/js #51

Conversation

Robert-Steiner commented Apr 21, 2021 • edited Loading

Profiling

catch_unwind

Error handling

acrrd commented Apr 22, 2021

Robert-Steiner commented Apr 22, 2021

janpetschexain left a comment • edited Loading

Choose a reason for hiding this comment

Robert-Steiner commented Apr 22, 2021

Robert-Steiner commented Apr 22, 2021

acrrd commented Apr 23, 2021 • edited Loading

acrrd commented Apr 23, 2021

Robert-Steiner commented Apr 26, 2021

Robert-Steiner commented Apr 26, 2021

Choose a reason for hiding this comment

Robert-Steiner Apr 28, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robert-Steiner Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

rustonaut Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rustonaut Apr 29, 2021 • edited Loading

Choose a reason for hiding this comment

Robert-Steiner commented Apr 28, 2021

acrrd commented Apr 28, 2021

Robert-Steiner commented Apr 28, 2021 • edited Loading

acrrd commented Apr 28, 2021

janpetschexain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Robert-Steiner commented Apr 21, 2021 •

edited

Loading

`catch_unwind`

janpetschexain left a comment •

edited

Loading

acrrd commented Apr 23, 2021 •

edited

Loading

Robert-Steiner Apr 28, 2021 •

edited

Loading

Robert-Steiner Apr 29, 2021 •

edited

Loading

rustonaut Apr 29, 2021 •

edited

Loading

rustonaut Apr 29, 2021 •

edited

Loading

Robert-Steiner commented Apr 28, 2021 •

edited

Loading