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

fix(parser)!: Store args in a group, rather than values #4072

Merged
merged 11 commits into from
Aug 12, 2022

Conversation

epage
Copy link
Member

@epage epage commented Aug 12, 2022

We now publicly expose Id, allowing us to report it in ArgMatches.

If we have to choose one behavior, this is more universal. The user can still
look up the values, this works with groups whose args have different
types, this should make parsing with groups faster as less data is copied, and this allows people to make decisions off of it when otherwise
there isn't enough information.

In preparing to publicly expose Id, this makes some progress on #1041 so we can fully roundtrip the Id. This will also unblock #1206.

To do that, we needed to make some progress on #2870 so we could accept the Id.

Finishing #1041 and #2870 is a lot, so I went the incremental route of a smaller goal by focusing on what this unblocks. We can incrementally finish these issues.

In the commits, the performance on my machine is tracked. The summary is that v4 is a lot faster than before this change and that tracking &'static str to avoid allocations removed almost all of the overhead introduced by the allocations. Unfortunately, we don't have a good performance target to decide if its worth it, so I'm going with it for now. We can easily swap out the internals of Id later. Unfortunately this added about 24 KB to the binary size, only a quarter of that is from skipping allocations.

Fixes #2317
Fixes #3748

This is prep for changing out the types

This dropped the size by 0.4 KB
This is prep for merging name/id
Compared to `master` on `06_rustup`:
- build: 5.74us -> 6.21us
- parse: 7.57us -> 7.55us
- parse_sc: 7.60us -> 7.95us
This is a part of clap-rs#2870 and is prep for clap-rs#1041

Oddly enough, this dropped the binary size by 200 Bytes

Compared to `HEAD~` on `06_rustup`:
- build: 6.21us -> 6.23us
- parse: 7.55us -> 8.17us
- parse_sc: 7.95us -> 7.65us
This is a step towards clap-rs#1041
- `ArgGroup` no longer takes a lifetime
- One less field type needs a lifetime

For now, we are using a more brute force type (`String`) so we can
establish performance base lines.  I was torn on whether to use `&str`
everywhere or make an `IdRef`.  The latter would add a lot of noise that
I'm concerned about, so i left it simple for now.  `IdRef` would help to
communicate the types involved though.

Speaking of communicating types, I'm also torn on whether we should use
`Id` for all strings or if we should have `Id`, `Name`, etc types to
avoid people mixing and matching.

This added 18.7 KB.

Compared to `HEAD~` on `06_rustup`:
- build: 6.23us -> 7.41us
- parse: 8.17us -> 9.36us
- parse_sc: 7.65us -> 9.29us
Unfortunately, this added another 6.6 KB

Compared to `HEAD~` on `06_rustup`:
- build: 7.41us -> 6.28us
- parse: 9.36us -> 8.07us
- parse_sc: 9.29us -> 7.74us

For context, this run compares `HEAD` to `v3-master`
```
build_rustup            time:   [10.765 µs 10.869 µs 10.981 µs]
                        change: [+72.716% +74.316% +75.832%] (p = 0.00 < 0.05)
                        Performance has regressed.
parse_rustup            time:   [14.264 µs 14.329 µs 14.407 µs]
                        change: [+75.565% +76.899% +78.172%] (p = 0.00 < 0.05)
                        Performance has regressed.
parse_rustup_with_sc    time:   [14.947 µs 15.025 µs 15.107 µs]
                        change: [+92.606% +94.079% +95.573%] (p = 0.00 < 0.05)
                        Performance has regressed.
```
So anything we are doing at this point is a large improvement.
Now that `Id` is public, we can have `ArgMatches` report them.  If we
have to choose one behavior, this is more universal.  The user can still
look up the values, this works with groups whose args have different
types, and this allows people to make decisions off of it when otherwise
there isn't enogh information.

Fixes clap-rs#2317
Fixes clap-rs#3748
@epage epage merged commit 3c82418 into clap-rs:master Aug 12, 2022
@epage epage deleted the group branch August 12, 2022 22:00
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.

ArgGroups values don't work well with get_many Get which arg of mutually exclusive ArgGroup was passed
1 participant