Implement shell bypassing #429
Conversation
|
OK, thanks to the CI I was able to:
|
|
I confirm that However, I agree with @sharkdp that using the empty string to disable the shell is quite ugly - especially since on Windows you actually have to use |
|
Thank you very much for your PR. Could you please fix the two "useless conversion" clippy warnings? |
src/shell.rs
Outdated
| return Ok(ExecuteResult { | ||
| status: std::os::windows::process::ExitStatusExt::from_raw(0), | ||
| system_time: 0.0, | ||
| user_time: 0.0, | ||
| }); |
There was a problem hiding this comment.
Previously, in order to compute the shell overhead, the execute_and_time command was called with an empty command, which in turn would run the equivalent of sh -c "" on Windows. (The same happens on Linux, but the code is simpler there as the timing is computed by hyperfine itserf.)
However given that one doesn't use a shell any more, and not to touch the other code too much, I've decided to just "fake" the execution of the empty command, with a success exit code and zero user and system time.
There was a problem hiding this comment.
and not to touch the other code too much
Feel free to touch the other code 😄. The current implementation is a bit "hacky", to be honest. I am not going to merge it in this state. But please feel free to reach out for help.
src/shell.rs
Outdated
| if shell == "" { | ||
| let mut tokens = match shell_words::split(command) { | ||
| Ok(tokens) => tokens.into_iter(), | ||
| Err(error) => return Err(io::Error::new(io::ErrorKind::Other, format!("{}", error))), |
There was a problem hiding this comment.
We should really use a error handling crate like anyhow. Would make this much easier. But we can fix this later.
src/shell.rs
Outdated
| fn prepare_shell_command( | ||
| command: &str, | ||
| shell: &str, | ||
| shell_arg: &str, | ||
| ) -> io::Result<Option<(String, Vec<String>)>> { |
There was a problem hiding this comment.
Can we please add a few simple tests for this function? We don't need to test the functionality of shell_words::split, but maybe a few edge cases that we could experience.
|
I merged #436 in the meantime, knowing that this would introduce some conflicts for this PR. I'm sorry for this. Let me know if you need help resolving them. |
|
@cipriancraciun are you still interested in finishing this work? |
Yes, but I'll have to make some time for this; hopefully in the next week or two. |
|
Fantastic. No hurries! |
|
A friendly reminder to @cipriancraciun I've already been using the binary built from this PR for the past months, but it would also be great if these changes could eventually be incorporated to the main branch ;) |
If the `--no-shell` argument is given, then the command is split into tokens (using the `shell-words` crate), and according to POSIX rules, the first token is taken as the executable, and the rest as arguments. This allows one to completely remove the shell overhead, especially when the benchmarked command is extreemly quick. Also, it allows one to make sure that the executed command is not implemented as a shell builtin. For example `hyperfine true` and `hyperfine --no-shell true` return different times due to the fact that `bash` executes `true` as a NOP. Example usage: ~~~~ hyperfine --no-shell -- true 'bash -c ""' "bash -c ''" ~~~~
|
I've just pushed a new variant of the patch, one that is compatible with the current master. The essentials are exactly like in the previous patch, except that now I've introduced This time the patch is much more graceful due to the new |
|
@cipriancraciun Thank you very much! Just last week, I decided that I want to tackle this task (hyperfine without intermediate shell) myself after this had been open for some time. I started with a large-scale refactoring and cleanup that is still ongoing (see #482). Sorry if this caused a lot of conflicts for your PR. I'm too deep into the refactoring process now and would like to finish that first. But I can then take care of merging your PR myself. I am also planning to introduce a few breaking changes in the upcoming release of hyperfine (2.0). It could therefore make sense to think about the CLI again. Maybe we want to make the I also have a slightly different implementation of this change in mind. Instead of having /// Shell to use for executing benchmarked commands
#[derive(Debug)]
pub enum Shell {
/// Default shell command
Default(&'static str),
/// Custom shell command specified via --shell
Custom(Vec<String>),
/// No shell, each command is parsed (via `shell-words`) and directly executed
None,
}I was thinking of something like a "benchmark executor" (or similar name) enum pub enum Executor {
Direct,
ViaShell(Shell),
Mocked(Fn() -> TimingResult),
}where |
As commented on #336, I've made an initial patch to allow one to disable the shell intermediation, and just parse the command as a list of words (perhaps with quoting).
As @sharkdp mentioned, I currently use
--shell ''to disable the shell, but that is quite ugly UX; however I wanted to keep the patch as small as possible.Also, given that Rust doesn't easily allow cross-compile I can't
cargo check --target ...windows..., so although I've tried to get the code right on Windows most likely there is a compile error.As mentioned in the issue at least in terms of outputs, it seems that
hyperfinealready handles the shell overhead and subtracts it from the outputs. Thus this patch is mostly to speed-up the overall execution by not passing through the shell. It shouldn't change the measured values.