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

Consider making rayon dependency of singlepass backend optional #2261

Closed
matklad opened this issue Apr 26, 2021 · 6 comments · Fixed by #2262
Closed

Consider making rayon dependency of singlepass backend optional #2261

matklad opened this issue Apr 26, 2021 · 6 comments · Fixed by #2262
Assignees
Labels
🎉 enhancement New feature! 📦 lib-compiler-singlepass About wasmer-compiler-singlepass

Comments

@matklad
Copy link
Contributor

matklad commented Apr 26, 2021

Motivation

At near, we are using the singlepass backend because we care about determinism and predictability. In particular, we generally avoid using thread parallelism, to avoid situations where two concurrent bits of functionality decide to take advantage of all cores at the same time and starve each other for CPU. So we need to be able to run wasmer with just a single thread.

We can already do this by configuring rayon's global theradpool, but that's not convenient (you need custom setup code for each
main and #[test]), and feels a bit roundabout. Even with RAYON_NUM_THREADS there's still a (degenerate) thread pool, and unnecessary inter-thread situation

Proposed solution

I took a quick look at the code, and it seems that making rayon optional should be fairly straightforward. Something like:

[dependencies]
rayon = { version="1.0", optional = true}

[features]
default = ["rayon"]
let iter = some().itereator().chain().
#[cfg(feature = "rayon")]
let iter = iter.par_iter();

There's even https://github.com/cuviper/rayon-cond, but I am not sure it is worth it.

Alternatives

Controlling this at runtime via RAYON_NUM_THREADS or rayon::ThreadPoolBuilder::build_global is already possible, just inconvenient

@matklad matklad added the 🎉 enhancement New feature! label Apr 26, 2021
@Hywan
Copy link
Contributor

Hywan commented Apr 26, 2021

Hello,

It seems to be a good feature to add indeed.

@Hywan Hywan self-assigned this Apr 26, 2021
@Hywan Hywan added the 📦 lib-compiler-singlepass About wasmer-compiler-singlepass label Apr 26, 2021
@Hywan
Copy link
Contributor

Hywan commented Apr 26, 2021

Do you have time to try a PR?

@matklad
Copy link
Contributor Author

matklad commented Apr 26, 2021

Sure, #2262! One thing I cant fix though is making sure that the code is build with --no-default-features on CI

@mikevoronov
Copy link
Contributor

mikevoronov commented Apr 26, 2021

I think it'd worth to do the same for other backends. We previously stumbled with rayon-rs/rayon#751 while using Wasmer with cranelift in async environment.

@syrusakbary
Copy link
Member

@matklad

For solving the threading situation, I was thinking on a bit bigger scope: allowing the user to set the number of threads that a compiler can make use of.
That way, when creating a compiler, the user can decide if it wants to make use of how many threads. Something along this lines:

use wasmer::{Store, JIT};
use wasmer_compiler_singlepass::Singlepass;

let mut compiler = Singlepass::new();
compiler.num_compilation_threads(1);
// Put it into an engine and add it to the store
let store = Store::new(&JIT::new(compiler).engine());

We can still make use of the rayon feature (if we still don't want to make the dependency optional, and disallowing people to use the num_compilation_threads with another number other than 1 if that's the case).

Thoughts?

@matklad
Copy link
Contributor Author

matklad commented Apr 26, 2021

@syrusakbary not sure that's the right place in the stack for this knob. One of the ideas behind rayon (and rayon-core split in particular) is there should be a single global thread pool for an entire application, such that separate parts of the app don't over-subscribe the CPU. Like, if one part of my application compiles wasm and spawns num_cpu threads, and another part of my app computes FFT and spawns num_cpu threads, the CPU ends up being utilized not that efficiently.

(actually, the pool should be global for system, not for app: see Grand Central Dispatch and make job server)

So I think the ideal API for the user to utilize only two threads for wasm compilation would be to use something like this:

        let pool = rayon::ThreadPoolBuilder::new().num_threads(2).build().unwrap();
        let compiled_code = pool.install(|| wasmer::compile(code));

So the solution for tunability here is to document that wasmer uses rayon internally, and point users to rayon's docs for this.

Note that there's also a difference between a thread pool with one worker and not using a thread pool. Even if the user sets the number of rayon threads to one, this still involves cross-thread communication (and compiling in the rayon machinery, which you might want to avoid for, eg, code size reasons).

TL;DR:

  • I think compile-time (kill)switch for parallelism is useful independently of the runtime tweakability
  • Tweaking || runtime is a global concern, and is already handled by rayon.

EDIT: to clarify, the "would be ideal API" is something that already works.

bors bot added a commit that referenced this issue Apr 26, 2021
2262: Make parallelism optional for singlepass backend r=syrusakbary a=matklad

# Description

closes #2261

# Review

- [ ] Add a short description of the change to the CHANGELOG.md file


Co-authored-by: Aleksey Kladov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎉 enhancement New feature! 📦 lib-compiler-singlepass About wasmer-compiler-singlepass
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants