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 profile flags to cli-flags crate #7568

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

rahulchaphalkar
Copy link
Contributor

This change adds profile options like vtune, jitdump and perfmap to cli-flags crate, so that they can be used by Sightglass and other benchmarks (via --engine-flags for sightglass).
This will enable wasm code visibility via profilers like vtune in sightglass, which was lacking.

I have added these flags to the Debug group of options - I can create a separate Profile group if needed instead.

@rahulchaphalkar rahulchaphalkar requested a review from a team as a code owner November 21, 2023 17:33
@rahulchaphalkar rahulchaphalkar requested review from alexcrichton and removed request for a team November 21, 2023 17:33
@alexcrichton
Copy link
Member

Thanks! Given though that there's already a --profile flag for wasmtime run, I think it would be best to reuse that rather than adding a separate version as well. I think that some code in src/common.rs might be good to extract to the cli-flags crate perhaps as I think that should have handling of the --profile flag (or it can be moved there)

@jlb6740 jlb6740 self-requested a review November 21, 2023 18:51
@rahulchaphalkar
Copy link
Contributor Author

Thanks, Alex. Let me take a look at that.

@jlb6740
Copy link
Contributor

jlb6740 commented Nov 22, 2023

@rahulchaphalkar +1, this is a needed patch. Putting profiling under the debug option was even referenced in the CLI refactoring issue #6741 (comment). For this patch isn't what you want to do is move the --profile flag to under DebugOptions option_group here https://github.com/bytecodealliance/wasmtime/pull/7568/files#diff-c971b96993f492c9102c58afb35a6cb1ce72351e8c32ffe9ffed91c656dc8484R129-R130
and also add the supporting code for mapping the profile strategy. It looks like you've done that but don't you also need to delete the code in common.rs, such as here: https://github.com/bytecodealliance/wasmtime/blob/main/src/common.rs#L52-L72
and the implementation code so that the flag is not duplicated? Either that or crates/bench-api/src/lib.rs parse RunCommon at https://github.com/bytecodealliance/wasmtime/blob/main/crates/bench-api/src/lib.rs#L252

@alexcrichton are you suggesting the former, or something different all together?

@alexcrichton
Copy link
Member

Ah so to clarify I'd prefer to not change the CLI, meaning not adding new options or removing any options. So my suggestion is to support profiling via the --profile flag. There's code to support that in src/common.rs which I wouldn't recommend duplicating for the bench-api crate, so I think it'd be best to refactor a bit to share the profiling-related code between the bench-api crate and the main CLI.

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.

3 participants