add initial support for wasm target #92

dgmachado · 2023-05-11T21:30:32Z

Related issue #53

fmeringdal · 2023-05-15T22:29:04Z

Hey and thanks for the MR! Please ping me again next week if I haven't reviewed it before then.

rrule-wasm/example/nodejs/app.js

rrule-wasm/example/web/index.html

rrule-wasm/src/lib.rs

Cargo.toml

fmeringdal · 2023-05-17T19:24:03Z

rrule-wasm/src/lib.rs

+     limit: Limit must be set in order to prevent infinite loops
+*/
+#[wasm_bindgen]
+pub fn get_all_date_recurrences(rule_set_string: &str, limit: Option<u16>) -> Vec<JsValue> {


Can we try to mimic the API for the rrule.js library? I think that would make it much simpler for users to swap out rrule.js for this wasm library.

Sure I think it's a good idea

rrule-wasm/tests/wasm.rs

rrule-wasm/src/lib.rs

rrule-wasm/example/web/index.html

rrule/src/wasm/mod.rs

rrule/examples/wasm/web/index.html

fmeringdal · 2023-05-19T12:34:23Z

rrule/Cargo.toml

+[lib]
+#[cfg(feature = "wasm")]
+crate-type = ["cdylib", "rlib"] 


Shouldn't this also contain the default "lib" crate type?

I'm not sure. In my tests, it wasn't necessary. What do you think?

Should contain the default "lib" as well. I will need to read up on this setting more before merging this PR

@fmeringdal setting the Cargo.toml file as:

[lib] crate-type = ["lib", "cdylib", "rlib"]

Once I run cargo build it's showing the below error:

cargo build 101 err took 6s at 06:42:39 Blocking waiting for file lock on build directory warning: output filename collision. The lib target `rrule` in package `rrule v0.10.0 (/Users/douglasmachado/Documents/Rust/Projects/rust-rrule/rrule)` has the same output filename as the lib target `rrule` in package `rrule v0.10.0 (/Users/douglasmachado/Documents/Rust/Projects/rust-rrule/rrule)`. Colliding filename is: /Users/douglasmachado/Documents/Rust/Projects/rust-rrule/target/debug/deps/librrule.rlib The targets should have unique names. Consider changing their names to be unique or compiling them separately. This may become a hard error in the future; see <https://github.com/rust-lang/cargo/issues/6313>. warning: output filename collision. The lib target `rrule` in package `rrule v0.10.0 (/Users/douglasmachado/Documents/Rust/Projects/rust-rrule/rrule)` has the same output filename as the lib target `rrule` in package `rrule v0.10.0 (/Users/douglasmachado/Documents/Rust/Projects/rust-rrule/rrule)`. Colliding filename is: /Users/douglasmachado/Documents/Rust/Projects/rust-rrule/target/debug/librrule.rlib The targets should have unique names. Consider changing their names to be unique or compiling them separately. This may become a hard error in the future; see <https://github.com/rust-lang/cargo/issues/6313>. thread 'main' panicked at 'assertion failed: mtimes.insert(output.clone(), mtime).is_none()', src/cargo/core/compiler/fingerprint/mod.rs:1096:13 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

How do you recommend to fix this?

Does it build with just "lib"? Why was "lib" replaced with ["cdylib", "rlib"]. I am not that familiar with how WASM internals work and don't want to merge anything I don't fully understand.

@fmeringdal @antoinepairet

Assuming in our context the below command as Rust Build:

cargo build

And the below command as Wasm Build:

wasm-pack build --release --target web --features "wasm"

We can analyze the different outputs:

Using the below config (Cargo.toml file):

[lib] crate-type = ["lib"]

1.a) the Rust Build works.

1.b) the Wasm Build fails:

Error: crate-type must be cdylib to compile to wasm32-unknown-unknown. Add the following to your Cargo.toml file: [lib] crate-type = ["cdylib", "rlib"] Caused by: crate-type must be cdylib to compile to wasm32-unknown-unknown. Add the following to your Cargo.toml file: [lib] crate-type = ["cdylib", "rlib"]

Using the below config (Cargo.toml file):

[lib] crate-type = ["lib", "cdylib", "rlib"]

2.a) the Rust Build and Wasm Build command fail and return the same output:

Blocking waiting for file lock on build directory warning: output filename collision. The lib target `rrule` in package `rrule v0.10.0 (/Users/douglasmachado/Documents/Rust/Projects/rust-rrule/rrule)` has the same output filename as the lib target `rrule` in package `rrule v0.10.0 (/Users/douglasmachado/Documents/Rust/Projects/rust-rrule/rrule)`. Colliding filename is: /Users/douglasmachado/Documents/Rust/Projects/rust-rrule/target/debug/deps/librrule.rlib The targets should have unique names. Consider changing their names to be unique or compiling them separately. This may become a hard error in the future; see <https://github.com/rust-lang/cargo/issues/6313>. warning: output filename collision. The lib target `rrule` in package `rrule v0.10.0 (/Users/douglasmachado/Documents/Rust/Projects/rust-rrule/rrule)` has the same output filename as the lib target `rrule` in package `rrule v0.10.0 (/Users/douglasmachado/Documents/Rust/Projects/rust-rrule/rrule)`. Colliding filename is: /Users/douglasmachado/Documents/Rust/Projects/rust-rrule/target/debug/librrule.rlib The targets should have unique names. Consider changing their names to be unique or compiling them separately. This may become a hard error in the future; see <https://github.com/rust-lang/cargo/issues/6313>. thread 'main' panicked at 'assertion failed: mtimes.insert(output.clone(), mtime).is_none()', src/cargo/core/compiler/fingerprint/mod.rs:1096:13 note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

Using the below config (Cargo.toml file):

[lib] crate-type = ["cdylib", "rlib"]

3.a) the Rust Build and Wasm Build command build successfully (the current PR is using this config).

I've found two docs that does not answer this type of problem:

Doc

Doc

Thanks for the context @dgmachado . I think the wasm build is fine, my main concern is the implications of the Rust build and how it might affect applications relying on this library. Before resolving this discussion I will have to do a deeper dive into WASM myself to understand what is going on and why this is required. Sorry for holding up this PR, but I can't merge anything that I don't fully understand as I have to maintain this.

rrule/Cargo.toml

rrule/src/wasm/mod.rs

antoinepairet · 2023-05-25T13:16:08Z

Great work @dgmachado 💪🏻

@fmeringdal

Could you review it again, please? 🙌🏻
Do you have any preferences regarding publishing this to the NPM registry? If you do not want to manage this, we can handle it. Just let us know what you prefer.

fmeringdal · 2023-05-25T21:47:21Z

@antoinepairet There are still some pending threads before this can be merged. We still need to nail down the wasm exported API and figure out what crate types the library should use.

Regarding NPM registry; I am not sure exactly how this should best be handled, will think about it 👍

tomquist · 2023-12-24T07:39:29Z

This is awesome. I was considering replacing rrule.js with the Napi Wrapper because we were running into performance problems with the JS lib. However, a wasm target would be even better. Generally I think there is high demand for this in the JS community as there’s no good alternative to rrule.js right now. Is there anything I can help to get this over the finish line?

fmeringdal · 2023-12-26T01:03:02Z

I agree that this is a useful use-case. I am not yet sure wether this should belong and be maintained in this repository. It seems that a separate repository that specifically focuses on an ergonomic JS API + test sute + docs + up to date deps would be preferable. What are your thoughts?

tomquist · 2023-12-31T11:52:46Z

I now implemented a WASM wrapper to compare it to the Napi Wrapper mentioned above. Unsurprisingly, the Napi wrapper was faster, but I didn't expect it to be that much faster. Here are some initial benchmarks I ran on my M1 Max:

Running "UTC TZ" suite...
Progress: 100%

  wasm:
    18 405 ops/s, ±6.61%   | 74.89% slower

  napi:
    73 288 ops/s, ±2.21%   | fastest

  rrule:
    14 780 ops/s, ±0.33%   | slowest, 79.83% slower

Finished 3 cases!
  Fastest: napi
  Slowest: rrule
Running "Other TZ" suite...
Progress: 100%

  wasm:
    12 742 ops/s, ±1.73%   | 80.61% slower

  napi:
    65 728 ops/s, ±5.18%   | fastest

  rrule:
    267 ops/s, ±1.31%      | slowest, 99.59% slower

Finished 3 cases!
  Fastest: napi
  Slowest: rrule

We primarily use rrules in a nodejs environment so the speed gains of using napi are appealing but I'd be happy to push the WASM wrapper to some separate repo.

Douglas Machado added 2 commits May 11, 2023 18:28

add initial support for wasm target

dbb25c0

add nodejs wasm example

f6b499c

fmeringdal self-requested a review May 15, 2023 22:29

fmeringdal requested changes May 17, 2023

View reviewed changes

Douglas Machado added 2 commits May 17, 2023 19:43

wasm integration refactoring

4738b66

add clippy lint into wasm integration

446d53a

dgmachado requested a review from fmeringdal May 17, 2023 22:58

fmeringdal reviewed May 18, 2023

View reviewed changes

rrule-wasm/example/web/index.html Outdated Show resolved Hide resolved

feat(wasm); created wasm module inside the rrule crate

883c735

dgmachado requested a review from fmeringdal May 19, 2023 10:34

fmeringdal requested changes May 19, 2023

View reviewed changes

Douglas Machado added 2 commits May 19, 2023 23:40

code review fixes

1a6e951

code review fixes

391b3f2

dgmachado requested a review from fmeringdal May 20, 2023 02:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add initial support for wasm target #92

add initial support for wasm target #92

dgmachado commented May 11, 2023

fmeringdal commented May 15, 2023

fmeringdal May 17, 2023

dgmachado May 17, 2023

fmeringdal May 19, 2023

dgmachado May 20, 2023

fmeringdal May 22, 2023

dgmachado May 23, 2023

fmeringdal May 25, 2023

dgmachado May 26, 2023 •

edited

Loading

fmeringdal May 31, 2023

antoinepairet commented May 25, 2023

fmeringdal commented May 25, 2023 •

edited

Loading

tomquist commented Dec 24, 2023

fmeringdal commented Dec 26, 2023

tomquist commented Dec 31, 2023

add initial support for wasm target #92

Are you sure you want to change the base?

add initial support for wasm target #92

Conversation

dgmachado commented May 11, 2023

fmeringdal commented May 15, 2023

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

dgmachado May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoinepairet commented May 25, 2023

fmeringdal commented May 25, 2023 • edited Loading

tomquist commented Dec 24, 2023

fmeringdal commented Dec 26, 2023

tomquist commented Dec 31, 2023

dgmachado May 26, 2023 •

edited

Loading

fmeringdal commented May 25, 2023 •

edited

Loading