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 possibility to disable intermediate shell #336

Closed
sharkdp opened this issue Oct 16, 2020 · 19 comments
Closed

Add possibility to disable intermediate shell #336

sharkdp opened this issue Oct 16, 2020 · 19 comments

Comments

@sharkdp
Copy link
Owner

sharkdp commented Oct 16, 2020

It would be great to have a way to disable the intermediate shell for very fast commands where we need a resolution that is better than 3 ms (typical standard deviation of the shell spawning time).

This new mode would obviously not be able to benchmark complex commands like seq 1000 | factor, but we could use https://crates.io/crates/shellwords https://crates.io/crates/shell-words to be able to run commands like my_command --foo "some strong" --bar.

@xzfc
Copy link
Contributor

xzfc commented Oct 17, 2020

Hyperfine could detect such simple commands automatically, without introducing an explicit command line option to disable the shell.

@sharkdp
Copy link
Owner Author

sharkdp commented Oct 17, 2020

Hyperfine could detect such simple commands automatically, without introducing an explicit command line option to disable the shell.

I'm not sure how that would work:

hyperfine "foo"

Is foo really a normal command? Or a shell builtin? Or a shell function? I guess we could check somehow (by walking PATH?) if its an actual command, but we wouldn't know if there is a shell function overwriting that.

hyperfine 'foo "$HOME"'

What would we do in this case? Certainly looks like a normal command that is called with a single argument. Does the user want that variable to be expanded or not? If it should be expanded, we need a shell.

And then for actual, obvious shell commands:

hyperfine "foo > /dev/null"
hyperfine "foo $(time)"
hyperfine "foo && bar"

.. how would we detect that we need a shell for those? We would need to know lots of shell syntax to determine that.

@xzfc
Copy link
Contributor

xzfc commented Oct 17, 2020

My basic idea is to define a limited POSIX shell subset: single command, no (unescaped) shell meta-characters with some exceptions like "" '' quotes. Anything that we could run relatively easily preserving the shell semantics without spawning an actual shell. Any other command that doesn't belong to this subset should be spawned using an actual shell. Explicit -S/--shell should disable this optimization.

This way, we don't introduce new syntax. So, no new options, and no subtle differences between an actual shell and our one. Just some commands run faster and are measured more accurately than the others.


hyperfine "foo"
Is foo really a normal command? Or a shell builtin? Or a shell function? I guess we could check somehow (by walking PATH?) if it's an actual command, but we wouldn't know if there is a shell function overwriting that.

It isn't a shell function since there are no predefined functions or aliases in a fresh shell (check it with sh -c 'declare -f').
It may be a shell builtin, but the list of shell-builtins is well-known and limited (we could hardcode them).
If it isn't a builtin, we could just go straight ahead and execute it.

Also, a cautious option: hyperfine "command foo". The foo here is definitely a command, not anything else.

hyperfine 'foo "$HOME"'
Does the user want that variable to be expanded or not?

We should not change semantics here, so yes. We should run an actual shell here if we do not want to handle variable expansion. If the user doesn't want it to be expanded, they should escape it as in the shell: hyperfine 'foo "\$HOME"'. Bonus point: the user may use printf %q to quote vars:

hyperfine "foo $(printf " %q" "$HOME" "$SOME_OTHER_VAR")"
akin to
sh -c "foo $(printf " %q" "$HOME" "$SOME_OTHER_VAR")"

hyperfine "foo > /dev/null"
hyperfine "foo $(time)"
hyperfine "foo && bar"

.. how would we detect that we need a shell for those?

Unquoted metacharacters used here, so we need a shell here.

We may refer to the POSIX shell standard and sh_contains_shell_metas() from bash source code.


We would need to know lots of shell syntax to determine that.

Well, yes… Maybe this should be in a separate crate. Or maybe it is already implemented in some crate since it sounds like a common task?

@xzfc
Copy link
Contributor

xzfc commented Oct 17, 2020

Shameless plug: I did a similar task for my own project, so I am somewhat familiar with parsing shell syntax as I did some research:
https://github.com/xzfc/cached-nix-shell/blob/48dbef37bec653e23c1801cea3979f053e10654b/src/main.rs#L303-L327
https://github.com/xzfc/cached-nix-shell/blob/48dbef37bec653e23c1801cea3979f053e10654b/src/bash.rs#L1-L18

But in my case, it was sufficient to detect a single-word command (e.g. python3 is a common case, and python3 -E is not). And in my case, the shell command is always prefixed with "exec ", so I didn't have to handle shell builtins.

@cipriancraciun
Copy link

cipriancraciun commented Dec 12, 2020

@xzfc although it is technically possible to define and implement a subset of sh so that we eliminate the extra exec, it can easily introduce the possibility of false results.

Imagine one would try to benchmark two sets of commands, and for some reason one of them "tricks" our parser into using sh and the other not to use it. Thus all of a sudden the benchmark is not "right" as one of the cases includes an extra "silent" sh.


However I would have a different proposal:

  • if one sets --shell "" (i.e. to the empty string),
  • then we just do a simple one space split of the command and call the first "item" with arguments the rest of the "items";

Some observations:

  • this is explicitly triggered by an empty shell, thus no surprises;
  • double spaces will yield a empty argument;
  • perhaps we could introduce backslash to escape a space and thus not split by it; (i.e. just \\ to represent a \ and \ to represent a space;)

Another alternative, given that hyperfine is mostly called from bash is to support its "quoted syntax", as obtained by `"${variable@Q}". For example:

> x="abc \\ -'-"
> echo "${x@Q}"
'abc \ -'\''-'

The advantage of this second proposal is that one can now easily compose commands as such:

command=/bin/command
argument='a b c'
array=( '1 2' '3 4' )
hyperfine --shell "" "${command@Q} ${argument@Q} ${array[@]@Q}"

As seen it can handle both single variables, but also arrays.

The syntax is a little-bit more convoluted as it implies handling single quotes, but it's not extremely complex, as I think it's only this:

  • split at single space (just like in my initial proposal);
  • backspace escapes spaces and quotes;
  • a single quote starts an item that can contain anything except another quote;
  • one can "paste" together (i.e. without intermediary spaces) single quoted strings, unquoted characters, or escaped ones; (i.e. the following is a single item 'a b c 'd' e'\ 'f' and it's equal with 'a b c d e f';)

@sharkdp
Copy link
Owner Author

sharkdp commented Dec 12, 2020

However I would have a different proposal:

then we just do a simple one space split of the command and call the first "item" with arguments the rest of the "items";

That is pretty much what I suggested in the first post, I believe. This is what shellwords shell-words does. But it also does quoting properly.

@cipriancraciun
Copy link

OK, so I thought "working code is better than no code", therefore I've made a small patch.

Basically if one sets --shell "" (i.e. the empty string), it just splits the command at spaces (one or many) and just calls that command.

If it's acceptable I can make a pull request.

master...cipriancraciun:feature-336

@sharkdp
Copy link
Owner Author

sharkdp commented Feb 7, 2021

If it's acceptable I can make a pull request.

That would be great. I haven't looked into your code in detail, but it shouldn't be hard to change from split_whitespace to shellwords::split shell_words::split (no need to do that in your PR for now).

@cipriancraciun

This comment was marked as resolved.

@sharkdp

This comment was marked as resolved.

@Pietruszek

This comment was marked as off-topic.

@cipriancraciun
Copy link

@cipriancraciun Would it be possible for you to finish this feature? It would help me with the Master's thesis I'm working on and I figured maybe you just forgot about this.

@Pietruszek I haven't made the pull request yet, however you are welcome to try the patched variant available at:

https://github.com/cipriancraciun/hyperfine/tree/feature-336

In order to trigger the behaviour, use as shell the empty string, i.e. --shell '' (mind the empty quotes). Please report back if you encounter any issues.


Regarding the patch, although I've tested it myself locally, with and without the intermediary shell, I don't think it makes a much difference, mainly because the current code is capable of detecting the shell overhead.

@Pietruszek
Copy link

@cipriancraciun I tried compiling your version on my Windows machine with cargo build (I think that's how I'm supposed to do it?), but it failed on a line added in one of your commits:
obraz

It's a function marked with #[cfg(windows)], so if I understand that correctly, it's possible it compiled for you on Linux, but doesn't on Windows.

@sharkdp
Copy link
Owner Author

sharkdp commented Jul 26, 2021

@cipriancraciun Would you like to open a PR with your changes? I would love to get this integrated into hyperfine. You could even just open a PR and someone else could continue working on it (if you are fine with contributing your code).

Before we integrate this, I would propose we talk about the CLI to support this. I don't think --shell "" would be the best way. The easiest idea would be to add a new --no-shell command line option.

@Pietruszek

This comment was marked as off-topic.

@cipriancraciun
Copy link

@Pietruszek @sharkdp Sorry for the extreemly late reply. See pull request #429.

I couldn't cargo check for Windows, thus let me know if it fails to compile.

@cipriancraciun
Copy link

@Pietruszek as described #429, it should now compile on Windows.

If you have the time, please test.

@sharkdp
Copy link
Owner Author

sharkdp commented Feb 22, 2022

This has now been implemented in #487. Thanks everyone! Especially @cipriancraciun for the initial implementation.

@sharkdp sharkdp closed this as completed Feb 22, 2022
@sharkdp
Copy link
Owner Author

sharkdp commented Mar 5, 2022

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants