wasmtime: Option to disable parallel compilation#3169
wasmtime: Option to disable parallel compilation#3169alexcrichton merged 7 commits intobytecodealliance:mainfrom
Conversation
Subscribe to Label Actioncc @peterhuene DetailsThis issue or pull request has been labeled: "wasmtime:api"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
e22b9fc to
482aa9e
Compare
This is produced by rust and forgotten to be placed into the release tarball. Closes bytecodealliance#3169
alexcrichton
left a comment
There was a problem hiding this comment.
Seems reasonable enough to me! I suspect we'll continue to grow configuration settings over time for parallelism here, but having whatever works in the meantime I think is ok too.
crates/cache/src/lib.rs
Outdated
|
|
||
| /// Gets cached data if state matches, otherwise calls the `compute`. | ||
| pub fn get_data<T, U, E>(&self, state: T, compute: fn(T) -> Result<U, E>) -> Result<U, E> | ||
| pub fn get_data<T, U, E, F>(&self, state: T, compute: F) -> Result<U, E> |
There was a problem hiding this comment.
This is currently intentional in that it takes fn instead of a closure because this is doing caching internally and trying to guarantee that the closure doesn't close over anything that isn't hashed and computed for caching. Is it possible to use the function-based version of this?
crates/jit/src/instantiate.rs
Outdated
| compiler: &Compiler, | ||
| data: &[u8], | ||
| use_paged_mem_init: bool, | ||
| #[cfg(feature = "parallel-compilation")] parallel_compilation: bool, |
There was a problem hiding this comment.
Could this argument be baked into the Compiler itself rather than passed as a parameter? I also think it's fine for it to be an unconditional parameter to avoid the #[cfg] on usage
crates/jit/src/lib.rs
Outdated
| ($condition:ident, $e:ident.($serial:ident | $parallel:ident), $iter_name:ident => { $body:expr }) => {{ | ||
| if $condition { | ||
| let $iter_name = $e.$parallel(); | ||
| $body |
There was a problem hiding this comment.
I think this macro be be out-living its usefulness as at this point it's branching into somewhat more obscure syntax.
I think our parallel operations are basically all map/collect so perhaps a new method could be exposed on Compiler which does that operation "take this list and run this closure and return the results" where the one method has internal #[cfg] and such for rayon vs not?
482aa9e to
d66ebee
Compare
This is produced by rust and forgotten to be placed into the release tarball. Closes #3169
|
I think you @alexcrichton meant to close the other issue. Thanks for the review btw appreciate it. I will do the update. |
|
Oh dear sorry about that! Dunno how I fat fingered that copy/paste |
alexcrichton
left a comment
There was a problem hiding this comment.
Looks great to me, thanks! Just one small nit but otherwise I think this is good to land
src/obj.rs
Outdated
| let compilation = compiler.compile( | ||
| &mut translation[0], | ||
| &types, | ||
| #[cfg(feature = "parallel-compilation")] |
There was a problem hiding this comment.
I think this no longer needs the #[cfg]?
Also remove the now unneeded feature in /Cargo.toml
This small PR introduces a configuration parameter to control whether parallel compilation is used in run-time. Currently, it is only possible to do this via toggling the
parallel-compilationfeature.In our projects we use wasmtime extensively. Specifically, it is used for powering several execution environments. Most of times, parallel compilation is beneficial. However, there is a weird one that kind of requires single-threaded compilation. This is because we want consistent memory consumption and less variance for compilation time.
What we could do is to basically settle on turning off the feature for our project. But that would be a shame.
This didn't go through a discussion just because I thought it would be really easy to implement and I only noticed that the discussion is required after I created the PR. I hope this is not a big issue.