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 wasm support via navigator.hardware_concurrency() #89

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add wasm support via navigator.hardware_concurrency() #89

wants to merge 1 commit into from

Conversation

Nemykal
Copy link

@Nemykal Nemykal commented Nov 5, 2019

Currently, num_cpus::get() always returns 1 on web assembly targets.

Although wasm is typically limited to single-threaded execution itself, it is possible to make use of multiple CPUs via Web Workers

On the wasm32-unknown-unknown platform, using the wasm_bindgen and web_sys crates, we can get a more accurate result by using navigator.hardware_concurrency() to get the number of CPUs on a system.

I tried to keep the code as similar as possible to the existing library.

Reference:
[1] https://developer.mozilla.org/en-US/docs/Web/API/NavigatorConcurrentHardware/hardwareConcurrency
[2] https://rustwasm.github.io/wasm-bindgen/api/web_sys/struct.Navigator.html#method.hardware_concurrency

Testing

I created a very basic test based on the wasm-bindgen Hello World
The Cargo.toml is the same as upstream, but also has the 'Navigator' feature enabled for web-sys

lib.rs:

use wasm_bindgen::prelude::*;

// Called by our JS entry point to run the example
#[wasm_bindgen(start)]
pub fn run() -> Result<(), JsValue> {
    // Use `web_sys`'s global `window` function to get a handle on the global
    // window object.
    let window = web_sys::window().expect("no global `window` exists");
    let document = window.document().expect("should have a document on window");
    let body = document.body().expect("document should have a body");

    // Manufacture the element we're gonna append
    let val = document.create_element("p")?;

    val.set_inner_html(&format!("num_cpus::get() = {}", num_cpus::get()));
    body.append_child(&val)?;

    let navigator = window.navigator();
    let logical_cpus = navigator.hardware_concurrency();

    let val2 = document.create_element("p")?;
    val2.set_inner_html(&format!("navigator.hardware_concurrency() = {}", logical_cpus));
    body.append_child(&val2)?;

    Ok(())
}

I build the test crate for wasm without a bundler as follows:

wasm-pack build --target web

I also have a basic index.html page that I copy into the pkg directory that wasm-pack creates:

<!doctype html>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <title>WASM num_cpu Test</title>
  </head>
  <body>
  <script type="module">
      import init from './wasm_example_num_cpus.js';
      async function run() {
        await init();
      }
      run();
    </script>
  </body>
</html>

I then start an http server in the pkg directory and browse to it.

basic-http-server -a '127.0.0.1:4000' ./pkg

Previously, num_cpus::get() would return 1, but now it correctly returns 8 on my system.

This is my first Rust contribution so please let me know if I should change anything.

Thanks,
Michael

@CryZe
Copy link

CryZe commented Nov 5, 2019

This should be gated by a feature. It also probably will cause problems where libraries will attempt to spawn threads, so this is a breaking change in a way.

@seanmonstar
Copy link
Owner

Very cool! I wonder, is it easy-ish to do without the dependencies? It's just wanting to call a single function, I don't know if we can do so with a smaller footprint, and if it's worth it.

Another thought, wasm doesn't necessarily mean "browser", right? It could also be a WASI target, and this will break that?

@CryZe
Copy link

CryZe commented Nov 5, 2019

Yes there‘s wasm outside the browser, there‘s wasm in the browser, there‘s wasi and wasi in the browser. So those need to be handled differently (so 3 cases in total, maybe a 4th if you consider emscripten). However this will break many things regardless of that, as many libraries will start spawning threads if num_cpus > 1. So this should probably only ever return something other than 1 for now if you explicitly activate some wasm-threads feature (in addition to the wasm-web feature or so to specify that you are even targeting a browser).

@seanmonstar
Copy link
Owner

However this will break many things regardless of that, as many libraries will start spawning threads if num_cpus > 1.

I don't understand why this is a problem. If a user is asking for the number of CPUs so they can choose how many threads to spawn, then it shouldn't break people if we give them a more accurate number.

Otherwise, adding support for any new target would be a breaking change.

@Nemykal
Copy link
Author

Nemykal commented Nov 6, 2019

I was hesitant to add a feature-gate for this since the crate doesn't currently have any. I've seen other crates use stdweb vs web-sys [1] features for choosing which tooling to use for wasm targets. We would need to figure out what feature(s) are required and what they should be named, if you want to use features at all

[1] https://github.com/grovesNL/glow/blob/master/Cargo.toml#L18-L19

Should the default for wasm32 stay as 1 unless some combination of features is enabled?

What cfg flags would you recommend for gating this implementation to only wasm32-unknown-unknown in the browser?

@CryZe
Copy link

CryZe commented Nov 6, 2019

However this will break many things regardless of that, as many libraries will start spawning threads if num_cpus > 1.

I don't understand why this is a problem. If a user is asking for the number of CPUs so they can choose how many threads to spawn, then it shouldn't break people if we give them a more accurate number.

Otherwise, adding support for any new target would be a breaking change.

No, because wasm32 is the only target that has std despite not really having working threads (or and OS really, so almost all of std panics). I consider this a mistake, wasm32-unknown-unknown should‘ve been a no_std target. But it isn‘t so we‘ll have to live with that and gate spawning threads in libraries behind appropriate cfg‘s. However I know that many libraries implicitly work for wasm32 because they don‘t spawn any threads when num_cpus is 1. So yes, they should probably have appropriate cfgs, but don‘t. So this will break libraries.

@seanmonstar
Copy link
Owner

However I know that many libraries implicitly work for wasm32 because they don‘t spawn any threads when num_cpus is 1. So yes, they should probably have appropriate cfgs, but don‘t. So this will break libraries.

That's unfortunate, but I don't feel that situation should prevent num_cpus from reporting a more accurate number on a specific target.

@RReverser
Copy link

I agree this should be behind a feature gate - not every wasm32 build is related to wasm-bindgen - but otherwise I'd love to see this merged.

@RReverser
Copy link

I've opened an alternative PR in #103 with few improvements.

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.

4 participants