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

experiment with using clap (builder API version) #4056

Closed
wants to merge 2 commits into from

Conversation

Mandragorian
Copy link
Contributor

-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

DISCLAIMER

part of this PR was generated using chatGPT. specifically the build_cli function, and the code that translates the parsed matches to a Command. I have read the generated code and modified slightly. I understand this might raise some questions, but I wanted to have a quick PoC since we haven't committed to anything.

-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

I was wrong and it seems that the compilation is notably faster (2.6s master, 3s builder API, 3.7s derive API).

On the other hand as mentioned in #4036 the code is more complex. There is even a degree of duplication since we have to not only define the parser, but then also translate the matches into a Command.

Also note that clap introduces a help subcommand, so that ./miri --help and ./miri help are equivalent. I don't know if there is a way to opt-out of this, but if it is a problem I can look further into it.

As with the other PR this needs more polishing, which I can do when we decide what we want to do.

@RalfJung
Copy link
Member

This version has basically no compile time cost for me -- old: 2.2s, new: 2.4s. (The other PR makes it 3.1s. I don't know why it is so much faster now than last time I checked, maybe I did those benchmarks on battery?)

@RalfJung
Copy link
Member

A lot of the overhead here is caused by building the Command instance, which however is merely a temporary artifact used to dispatch to the right function in fn exec. So we could reduce the overhead by removing the Command type, and passing the parsed arguments directly to fn exec.

But, still... I am inclined to go with the nice version. This will only be rebuilt when people update their nightly Rust, i.e. at most once a day, likely less often.

@Mandragorian
Copy link
Contributor Author

I agree with going with the derive API. It simplifies the code a lot, and the slowdown is not horrendous. If it proves to be annoying we can always reconsider later.

@RalfJung
Copy link
Member

@oli-obk @saethlin what do you think?

@saethlin
Copy link
Member

Thanks for the investigation. I'm good with the derive version.

@RalfJung
Copy link
Member

Okay, closing in favor of #4036. Thanks @Mandragorian for exploring this option!

@RalfJung RalfJung closed this Nov 25, 2024
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