search

rust-lang / miri

An interpreter for Rust's mid-level intermediate representation
Apache License 2.0
4.64k stars 349 forks source link

experiment with using clap (builder API version) #4056

Closed Mandragorian closed 6 hours ago

Mandragorian commented 2 days ago

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

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 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

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 commented 1 day ago

@oli-obk @saethlin what do you think?

saethlin commented 1 day ago

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

RalfJung commented 6 hours ago

Okay, closing in favor of https://github.com/rust-lang/miri/pull/4036. Thanks @Mandragorian for exploring this option!