-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
wasmtime: Option to disable parallel compilation #3169
wasmtime: Option to disable parallel compilation #3169
Conversation
Subscribe to Label Actioncc @peterhuene
This 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@@ -43,10 +43,11 @@ impl<'config> ModuleCacheEntry<'config> { | |||
} | |||
|
|||
/// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
pub fn build( | ||
compiler: &Compiler, | ||
data: &[u8], | ||
use_paged_mem_init: bool, | ||
#[cfg(feature = "parallel-compilation")] parallel_compilation: bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-compilation
feature.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.