Stream: git-wasmtime

Topic: wasmtime / issue #7568 Add profile flags to cli-flags crate

view this post on Zulip Wasmtime GitHub notifications bot (Nov 21 2023 at 18:35):

alexcrichton commented on issue #7568:

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)

view this post on Zulip Wasmtime GitHub notifications bot (Nov 21 2023 at 18:55):

rahulchaphalkar commented on issue #7568:

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

view this post on Zulip Wasmtime GitHub notifications bot (Nov 22 2023 at 07:49):

jlb6740 commented on issue #7568:

@rahulchaphalkar +1, this is a needed patch. I think this was even putting profiling under the debug option was even referenced in the CLI refactoring issue https://github.com/bytecodealliance/wasmtime/issues/6741#issuecomment-1639955767. 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 also parse the none CommonOptions 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?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 22 2023 at 07:59):

jlb6740 edited a comment on issue #7568:

@rahulchaphalkar +1, this is a needed patch. I think this was even putting profiling under the debug option was even referenced in the CLI refactoring issue https://github.com/bytecodealliance/wasmtime/issues/6741#issuecomment-1639955767. 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?

view this post on Zulip Wasmtime GitHub notifications bot (Nov 22 2023 at 16:35):

alexcrichton commented on issue #7568:

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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 22 2023 at 19:20):

jlb6740 edited a comment on issue #7568:

@rahulchaphalkar +1, this is a needed patch. Putting profiling under the debug option was even referenced in the CLI refactoring issue https://github.com/bytecodealliance/wasmtime/issues/6741#issuecomment-1639955767. 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?

Last updated: Dec 23 2024 at 13:07 UTC