experiment with using clap #4036
Conversation
There was a problem hiding this comment.
Thanks for trying this! I agree it is quite elegant.
However, it also doubles our build times for the script. The script hopefully doesn't get built too often, so this is mostly a thing for first-time users -- they get no feedback while miri-script itself is built. Ideally we'd build it on stable Rust so it wouldn't be rebuilt a lot, but we need some nightly cargo features for the diagnostic integration...
Overall I am leaning slightly in favor of this, just to make maintenance of the script easier. I'd still be curious how this would look like without the derive; is there a good example somewhere of what using the clap builder directly looks like?
| flags.push("--".into()); | ||
| flags.extend(remainder); | ||
| }, | ||
| _ => (), |
There was a problem hiding this comment.
Shouldn't this produce some sort of error?
| pub command: Command, | ||
| } | ||
|
|
||
| const _HELP: &str = r#" COMMANDS |
There was a problem hiding this comment.
clap can generate this for us, can't it? (It's okay to only do this once we decided, but I want to know whether further simplification is possible here.)
There was a problem hiding this comment.
I actually just forgot to delete this in this pr 😅. Since I wasn't sure this would be useful, I kept the PoC low effort. Clap generates the usage string automatically.
| } | ||
| }; | ||
| let miri_args: Vec<_> = | ||
| std::env::args().peekable().take_while(|x| *x != "--").filter(|x| *x != "--").collect(); |
There was a problem hiding this comment.
Why peekable? And why the extra filter?
There was a problem hiding this comment.
Yeap, peekable is not needed. I just quickly copy pasted the code from the args module without thinking too much for the prototype. I used the extra filter because for some reason i was under the impression that the -- was included in the result. This is wrong and the filter is not needed.
| let miri_args: Vec<_> = | ||
| std::env::args().peekable().take_while(|x| *x != "--").filter(|x| *x != "--").collect(); | ||
| let remainder: Vec<_> = | ||
| std::env::args().peekable().skip_while(|x| *x != "--").skip(1).collect(); |
There was a problem hiding this comment.
I'd remove the skip here and make the -- part of the remainder, that also simplifies add_remainder.
There was a problem hiding this comment.
I agree this is a much better way of doing it.
There was a problem hiding this comment.
There should be no reason for this file to change.
|
Also @rust-lang/miri what do you all think? |
I see a regression from 2.5s to 3s, which would be fine with me. I like clap, and think it's worth a bit of compile time overhead, but doubling the compile time of miri-script seems a bit steep. I'd quite like to know what the compile time is without using the proc macro (maybe you can just |
|
I am measuring a slowdown from 4s to 5.2s. |
|
what command do you use to build? I can write a small script to measure the build times a bit more systematically. maybe I got an outlier. |
|
|
|
Using this script: #!/usr/bin/env python
import argparse
import shutil
import subprocess
import sys
import time
parser = argparse.ArgumentParser(
prog='ProgramName',
description='What the program does',
epilog='Text at the bottom of help')
parser.add_argument('-c', '--count', type=int, default=10)
if __name__ == "__main__":
args = parser.parse_args()
total_time = 0
for _ in range(args.count):
shutil.rmtree('miri-script/target', ignore_errors=True)
start_time = time.time()
subprocess.run(['./miri', '--help'], capture_output=True)
end_time = time.time()
total_time += end_time - start_time
print('.', end='')
sys.stdout.flush()
print('total time = {}, average execution time = {}'.format(total_time, total_time / args.count))(don't judge my python skills too much) I get that without the changes i have
And with the changes
So less than 5s but also more than 3s that @saethlin observed (comparing with their results since we seem to have similar baselines). |
|
Absolute times are obviously going to differ. What's the relative slowdown? Seems to be 40% for you, 30% for me, 20% for saethlin. |
|
☔ The latest upstream changes (presumably #4041) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Yes, these are the relative slowdowns. I compared with saethlin's numbers since we had similar baselines. What would be the threshold for an acceptable slowdown? |
|
IMO that is the wrong question. The right question is, how low can we get the slowdown while still getting most of the benefits of using clap (or some other similar library)? |
|
Ok, then I will try to make a poc using the builder api when I have a bit more time on my hands. In the meantime I built
If I interpret this correctly I don't expect the builder API to save us much time after the first compilation. But then, as long as As for how the builder API looks like, there is an example in the tutorial. Most notably, we lose some compile time checks (but of course this might be a feature in our case). For example instead of |
It gets rebuild whenever the compiler changes, which is |
I did a quick experiment to see if it is worth changing to clap or not, following the discussion in #3592.
I think the code simplifies quite a bit, but as mentioned in the issue I did notice a much longer compilation time from 2.5s to 5s after the changes. It might be better to use the builder API, which I understand correctly might allow us to sacrifice some runtime cost, to gain faster build times (?).
I don't have much experience in using
miri-scriptso couldn't think of a great way to see if the parser fails to parse something that currently is accepted. If you have any ideas let me know and I can add tests. I did manually try the example mentioned in clap-rs/clap#5055 (./miri clippy -- -D warnings) and it worked. Hopefully the test suit will exercise the changes to some extend.Since it is possible that we might not want to do that I didn't spend much time polishing the code. I can do that if we decide to keep it.