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

Don't enable parallel compilation in the bench API by default #4944

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented Sep 22, 2022

And add a feature to turn it on if desired.

And add a feature to turn it on if desired.
@fitzgen fitzgen requested a review from jameysharp September 22, 2022 22:21
@fitzgen
Copy link
Member Author

fitzgen commented Sep 22, 2022

Hmm something is enabling rayon by default even with this. cargo doesn't provide great visibility into what is turning on what features here.

@jameysharp
Copy link
Contributor

I was just noticing earlier that --no-default-features isn't turning off rayon any more. Thanks for prompting me to dig deeper.

I think the cause is that #4911 made wasmtime-cli-flags depend on the wasmtime/parallel-compilation feature.

I'm very much in favor of disabling parallel compilation by default for benchmarking, so let's figure out how to get the web of features right.

You can get cargo to tell you what's pulling in a particular dependency with e.g. cargo tree -p wasmtime-bench-api -i rayon. It also accepts the usual --no-default-features and --features flags so you can check different combinations.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

I mentioned this in chat as well but I'd probably recommend using Config::parallel_compilation instead of a compile-time feature since it'll be easier to manage.

cargo doesn't provide great visibility into what is turning on what features here.

I mentioned this in chat as well but for posterity what I recommended to debug this was:

$ cargo tree -i wasmtime -e features

which shows all the crates that depend on wasmtime (inverted format) with features in the dependency edges.

@abrown
Copy link
Contributor

abrown commented Sep 23, 2022

@fitzgen, if a runtime flag is needed, I had submitted #4207 for that purpose (along with an --engine-flags flag in Sightglass, IIRC). Looks like something went wrong with #4207 but I can rebase that and figure out the CI error if that would help--what do you think? In the past I've also needed to benchmark single-threaded compilation but I think I used the taskset hammer instead, so I'm in favor of making either of these approaches work.

@jameysharp
Copy link
Contributor

If I'm understanding #4207 correctly, it looks like it's worth merging but isn't necessary for non-WASI options like the recent --disable-parallel-compilation.

@fitzgen
Copy link
Member Author

fitzgen commented Sep 23, 2022

I think we don't want to rely on only a runtime flag because the stacks are still not great when profiling if we do the runtime flag.

I think we will want to make the CLI flags crate not enable a bunch of features and maybe make Config always have methods for things like disabling parallelism even when the rayon dep isn't active (but document that it has no effect in that case or make it panic or return an error or something).

@abrown
Copy link
Contributor

abrown commented Sep 23, 2022

If I'm understanding #4207 correctly, it looks like it's worth merging but isn't necessary for non-WASI options like the recent --disable-parallel-compilation.

No, it actually makes it possible to use any of the CommonOptions, of which disable_parallel_compilation is one.

@alexcrichton
Copy link
Member

because the stacks are still not great when profiling

Could you expand on this? The flag is checked here so this shouldn't affect stacks other than having a run_maybe_parallel frame which otherwise would probably get inlined if rayon is disabled (but whether or not the frame is present doesn't seem like a bad stack to me)

@abrown
Copy link
Contributor

abrown commented Sep 23, 2022

the stacks are still not great when profiling if we do the runtime flag

I agree with that sentiment (not wanting to wade through rayon stacks) but I'm wondering how you're still seeing large stacks when you enable the flags: I see self.config().parallel_compilation as guarding against using rayon at all in a few places. Do you mean the rayon "setup the environment" functions?

@fitzgen
Copy link
Member Author

fitzgen commented Sep 23, 2022

I guess the broken stacks that I'm seeing now are unrelated to rayon, its all in vec iter stuff but it isn't all the same so the flamegraphs and top down views such are kinda unusable. I guess we can close this PR tho.

@fitzgen fitzgen closed this Sep 23, 2022
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