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

Add support for WASM targets #45

Merged
merged 2 commits into from
Jun 15, 2021
Merged

Add support for WASM targets #45

merged 2 commits into from
Jun 15, 2021

Conversation

Luthaf
Copy link
Contributor

@Luthaf Luthaf commented Jun 4, 2021

This PR adds support for Monotonic on both WASM targets: wasm32-unknown-unknown (web) and wasm32-wasi (wasi). On the web, it uses window.performance.now(); and on wasi it uses clock_time_get with a monotonic clock.

I did not ran the tests on these targets, but I'll try to do so in the next few days.

@tobz
Copy link
Member

tobz commented Jun 4, 2021

At first glance, this looks reasonable to me. I'll try to do a full review on it in the next few days. The cfg conditionals are starting to get a bit hairy and I want to make sure things are 👌🏻 😅

@Luthaf
Copy link
Contributor Author

Luthaf commented Jun 4, 2021

The cfg conditionals are starting to get a bit hairy and I want to make sure things are 👌🏻 😅

Yeah, wasm being available on both web and wasi do not help here ...

tobz
tobz previously requested changes Jun 7, 2021
src/monotonic.rs Outdated Show resolved Hide resolved
src/monotonic.rs Outdated Show resolved Hide resolved
@Luthaf
Copy link
Contributor Author

Luthaf commented Jun 7, 2021

I cleaned up the code as suggested in the review, and added support to run the tests.

running tests for wasm32-wasi

# install wasmtime
curl https://wasmtime.dev/install.sh -sSf | bash
export CARGO_TARGET_WASM32_WASI_RUNNER="wasmtime run"
cargo test --target wasm32-wasi

running tests for wasm32-unknown-unknown

# install wasm-pack
curl https://rustwasm.github.io/wasm-pack/installer/init.sh -sSf | sh 
# run tests on firefox
wasm-pack test --firefox --headless
# run tests on chrome
wasm-pack test --chrome --headless

# run tests on node (require commenting out the configure_wasm_tests module in lib.rs)
# this currently fails with ReferenceError: Window is not defined due to 
wasm-pack test --node

@@ -155,6 +155,7 @@ mod tests {
use std::time::Duration;

#[test]
#[cfg_attr(target_arch = "wasm32", ignore)] // WASM is single threaded
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my own curiosity: does std::thread::spawn not work at all on WASM?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least currently, yes. There are proposals to add threading to WASM, both in browsers and WASI, but they are still in early stage.

@Luthaf
Copy link
Contributor Author

Luthaf commented Jun 14, 2021

I've added some documentation on accuracy, let me know if there is a better way to do express these limitations

@tobz
Copy link
Member

tobz commented Jun 14, 2021

@Luthaf Changes look good to me, just need to run cargo fmt and this should be set to merge.

@tobz tobz dismissed their stale review June 14, 2021 22:09

stale

@tobz tobz merged commit 7a5b179 into metrics-rs:main Jun 15, 2021
@Luthaf Luthaf deleted the wasm-support branch June 15, 2021 14:29
@tobz
Copy link
Member

tobz commented Jun 17, 2021

@Luthaf Sorry for the delay here. [email protected] is now released and contains your PR.

Thanks again for your contribution! 🌮

@Luthaf
Copy link
Contributor Author

Luthaf commented Jun 17, 2021

thanks a lot for the release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants